| Message ID | 20240212173111.771107-1-jogletre@opensource.cirrus.com |
|---|---|
| Headers | show |
| Series | Add support for CS40L50 | expand |
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.
On Mon, Feb 12, 2024 at 05:31:07PM +0000, James Ogletree wrote: > A write sequencer is a sequence of register addresses > and values executed by some Cirrus DSPs following > power state transitions. > > Add support for Cirrus drivers to update or add to a > write sequencer present in firmware. > > Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com> > --- > + if (!update) { > + if (wseq->ctl->len - op_end->offset < new_op_size) { > + cs_dsp_err(dsp, "Not enough memory in write sequence for entry\n"); > + ret = -ENOMEM; > + goto op_new_free; > + } > + > + op_end->offset += new_op_size; > + > + ret = cs_dsp_coeff_write_ctrl(wseq->ctl, op_end->offset / sizeof(u32), > + &op_end->data, sizeof(u32)); > + if (ret) > + goto op_new_free; > + > + list_add_tail(&op_new->list, &wseq->ops); This means the new write will be after the terminator in the list. Whilst I think the code will work this way it would be much cleaner if we kept the list ordered to match the writes, by inserting the new element just before op_end. Thanks, Charles
Hi Mark, Acknowledged on anything unmentioned. > On Feb 13, 2024, at 1:55 PM, Mark Brown <broonie@kernel.org> wrote: > > On Mon, Feb 12, 2024 at 05:31:11PM +0000, James Ogletree wrote: > >> + 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? Yes, I will use the set_bclk_ratio callback in the next version. > >> + case CS40L50_PLL_REFCLK_MCLK: >> + clk_cfg = CS40L50_PLL_CLK_CFG_32768; >> + break; > > MCLK is always 32.768kHz? When we leave audio mode, we need to go back to a constant 32.768kHz clock. Best, James