Re: [PATCH v7 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50

From: Mark Brown <broonie@kernel.org>
To: James Ogletree <jogletre@opensource.cirrus.com>
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
	krzysztof.kozlowski+dt@linaro.org, conor+dt@kernel.org,
	lee@kernel.org, jeff@labundy.com, patches@opensource.cirrus.com,
	linux-sound@vger.kernel.org, linux-input@vger.kernel.org,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v7 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50
Date: Tue, 13 Feb 2024 19:55:54 +0000	[thread overview]
Message-ID: <2e22f00a-fbf4-4e6d-8aed-dc78f423c735@sirena.org.uk> (raw)
In-Reply-To: <20240212173111.771107-6-jogletre@opensource.cirrus.com>

[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]

On Mon, Feb 12, 2024 at 05:31:11PM +0000, James Ogletree wrote:

> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,

Please use C++ style for the whole comment to make things look more
intentional.

> +#define CS40L50_PLL_CLK_FRQ_32768	32768
> +#define CS40L50_PLL_CLK_FRQ_1536000	1536000
> +#define CS40L50_PLL_CLK_FRQ_3072000	3072000
> +#define CS40L50_PLL_CLK_FRQ_6144000	6144000
> +#define CS40L50_PLL_CLK_FRQ_9600000	9600000
> +#define CS40L50_PLL_CLK_FRQ_12288000	12288000

I'm not sure these defines add greatly to legibility, indeed they make
me wonder where the translation function is when we take a directly
specified clock value in...

> +	switch (clk_src) {
> +	case CS40L50_PLL_REFCLK_BCLK:
> +		ret = cs40l50_get_clk_config(codec->sysclk_rate, &clk_cfg);
> +		if (ret)
> +			return ret;
> +		break;

We appear to have a set_sysclk() operation but this is saying the sysclk
is BCLK?  Should the driver be using the bclk_ratio() interface rather
than set_sysclk(), especially given that the device only appears to
support either 32.768kHz with no audio or 48kHz and a rather restrictive
set of multiples of that for the clock?

> +	case CS40L50_PLL_REFCLK_MCLK:
> +		clk_cfg = CS40L50_PLL_CLK_CFG_32768;
> +		break;

MCLK is always 32.768kHz?

> +static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
> +{
> +	struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component);
> +
> +	if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
> +		return -EINVAL;

Please use the modern names, the device is a clock consumer (it would be
nice if someone from Cirrus could update your drivers...).

> +static struct platform_driver cs40l50_codec_driver = {
> +	.probe = cs40l50_codec_driver_probe,
> +	.driver = {
> +		.name = "cs40l50-codec",
> +	},
> +};
> +module_platform_driver(cs40l50_codec_driver);
> +
> +MODULE_DESCRIPTION("ASoC CS40L50 driver");
> +MODULE_AUTHOR("James Ogletree <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");

There's nothing here that ensures the driver autoloads, you need to
either a MODULE_DEVICE_TABLE or a MODULE_ALIAS.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]