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

["Rivera-Matos, Ricardo" <rriveram@opensource.cirrus.com>]

On 4/8/24 10:32 AM, James Ogletree wrote:
> Introduce support for Cirrus Logic Device CS40L50: a
> haptic driver with waveform memory, integrated DSP,
> and closed-loop algorithms.
>
> The ASoC driver enables I2S streaming to the device.
>
> Reviewed-by: David Rhodes <drhodes@opensource.cirrus.com>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
>   MAINTAINERS                      |   1 +
>   sound/soc/codecs/Kconfig         |  11 ++
>   sound/soc/codecs/Makefile        |   2 +
>   sound/soc/codecs/cs40l50-codec.c | 308 +++++++++++++++++++++++++++++++
>   4 files changed, 322 insertions(+)
>   create mode 100644 sound/soc/codecs/cs40l50-codec.c
>
<cut>
> diff --git a/sound/soc/codecs/cs40l50-codec.c b/sound/soc/codecs/cs40l50-codec.c
> new file mode 100644
> index 000000000000..6d4a0970b219
> --- /dev/null
> +++ b/sound/soc/codecs/cs40l50-codec.c
> @@ -0,0 +1,308 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// CS40L50 Advanced Haptic Driver with waveform memory,
> +// integrated DSP, and closed-loop algorithms
> +//
> +// Copyright 2024 Cirrus Logic, Inc.
> +//
> +// Author: James Ogletree <james.ogletree@cirrus.com>
> +
> +#include <linux/bitfield.h>
> +#include <linux/mfd/cs40l50.h>
> +#include <linux/pm_runtime.h>
Is pm_runtime.h being used in the context of the codec driver? If not, 
you should drop it.
> +#include <sound/pcm_params.h>
> +#include <sound/soc.h>
> +
<cut>
> +
> +static const struct cs40l50_pll_config cs40l50_pll_cfg[] = {
> +	{ 32768, 0x00 },
> +	{ 1536000, 0x1B },
> +	{ 3072000, 0x21 },
> +	{ 6144000, 0x28 },
> +	{ 9600000, 0x30 },
> +	{ 12288000, 0x33 },
> +};
> +
> +static int cs40l50_get_clk_config(unsigned int freq, unsigned int *cfg)
You could constify freq.
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(cs40l50_pll_cfg); i++) {
> +		if (cs40l50_pll_cfg[i].freq == freq) {
> +			*cfg = cs40l50_pll_cfg[i].cfg;
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int cs40l50_swap_ext_clk(struct cs40l50_codec *codec, unsigned int clk_src)
You could constify clk_src.
> +{
> +	unsigned int cfg;
> +	int ret;
> +
> +	switch (clk_src) {
> +	case CS40L50_PLL_REFCLK_BCLK:
> +		ret = cs40l50_get_clk_config(codec->bclk_ratio * codec->rate, &cfg);
> +		if (ret)
> +			return ret;
> +		break;
> +	case CS40L50_PLL_REFCLK_MCLK:
> +		cfg = CS40L50_PLL_REEFCLK_MCLK_CFG;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
> +				 CS40L50_PLL_REFCLK_LOOP_MASK,
> +				 CS40L50_PLL_REFCLK_OPEN_LOOP <<
> +				 CS40L50_PLL_REFCLK_LOOP_SHIFT);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
> +				 CS40L50_PLL_REFCLK_FREQ_MASK |
> +				 CS40L50_PLL_REFCLK_SEL_MASK,
> +				 (cfg << CS40L50_PLL_REFCLK_FREQ_SHIFT) | clk_src);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(codec->regmap, CS40L50_REFCLK_INPUT,
> +				  CS40L50_PLL_REFCLK_LOOP_MASK,
> +				  CS40L50_PLL_REFCLK_CLOSED_LOOP <<
> +				  CS40L50_PLL_REFCLK_LOOP_SHIFT);
> +}
> +
<cut>
> +
> +MODULE_DESCRIPTION("ASoC CS40L50 driver");
> +MODULE_AUTHOR("James Ogletree <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");

This gets my Reviewed-by pending these edits.

Ricardo