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@xxxxxxxxxx>");
> +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:
signature.asc
Description: PGP signature
