On Tue, 7 Feb 2023 22:17:39 +0100
Hi Christophe,
Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
> Le 06/02/2023 à 15:49, Herve Codina a écrit :
> > The Infineon PEB2466 codec is a programmable DSP-based four channels
> > codec with filters capabilities.
> > It also provides signals as GPIOs.
> >
> > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
> > ---
> > sound/soc/codecs/Kconfig | 12 +
> > sound/soc/codecs/Makefile | 2 +
> > sound/soc/codecs/peb2466.c | 2071 ++++++++++++++++++++++++++++++++++++
> > 3 files changed, 2085 insertions(+)
> > create mode 100644 sound/soc/codecs/peb2466.c
> >
>
> [...]
>
> > +static int peb2466_spi_probe(struct spi_device *spi)
> > +{
> > + struct peb2466 *peb2466;
> > + unsigned long mclk_rate;
> > + int ret;
> > + u8 xr5;
> > +
> > + spi->bits_per_word = 8;
> > + ret = spi_setup(spi);
> > + if (ret < 0)
> > + return ret;
> > +
> > + peb2466 = devm_kzalloc(&spi->dev, sizeof(*peb2466), GFP_KERNEL);
> > + if (!peb2466)
> > + return -ENOMEM;
> > +
> > + peb2466->spi = spi;
> > +
> > + peb2466->regmap = devm_regmap_init(&peb2466->spi->dev, NULL, peb2466,
> > + &peb2466_regmap_config);
> > + if (IS_ERR(peb2466->regmap))
> > + return PTR_ERR(peb2466->regmap);
> > +
> > + peb2466->reset_gpio = devm_gpiod_get_optional(&peb2466->spi->dev,
> > + "reset", GPIOD_OUT_LOW);
> > + if (IS_ERR(peb2466->reset_gpio))
> > + return PTR_ERR(peb2466->reset_gpio);
> > +
> > + peb2466->mclk = devm_clk_get(&peb2466->spi->dev, "mclk");
>
> Hi,
>
> Up to you to decide if it is a good idea or not, but using
> devm_clk_get_enabled() would save the 'mclk' field in peb2466 ...
>
> > + if (IS_ERR(peb2466->mclk))
> > + return PTR_ERR(peb2466->mclk);
> > + ret = clk_prepare_enable(peb2466->mclk);
> > + if (ret)
> > + return ret;
>
> ... these 3 lines ...
>
> > +
> > + if (peb2466->reset_gpio) {
> > + gpiod_set_value_cansleep(peb2466->reset_gpio, 1);
> > + udelay(4);
> > + gpiod_set_value_cansleep(peb2466->reset_gpio, 0);
> > + udelay(4);
> > + }
> > +
> > + spi_set_drvdata(spi, peb2466);
>
> ... this spi_set_drvdata() call ...
>
> > +
> > + mclk_rate = clk_get_rate(peb2466->mclk);
> > + switch (mclk_rate) {
> > + case 1536000:
> > + xr5 = PEB2466_XR5_MCLK_1536;
> > + break;
> > + case 2048000:
> > + xr5 = PEB2466_XR5_MCLK_2048;
> > + break;
> > + case 4096000:
> > + xr5 = PEB2466_XR5_MCLK_4096;
> > + break;
> > + case 8192000:
> > + xr5 = PEB2466_XR5_MCLK_8192;
> > + break;
> > + default:
> > + dev_err(&peb2466->spi->dev, "Unsupported clock rate %lu\n",
> > + mclk_rate);
> > + ret = -EINVAL;
> > + goto failed;
> > + }
> > + ret = regmap_write(peb2466->regmap, PEB2466_XR5, xr5);
> > + if (ret) {
> > + dev_err(&peb2466->spi->dev, "Setting MCLK failed (%d)\n", ret);
> > + goto failed;
> > + }
> > +
> > + ret = devm_snd_soc_register_component(&spi->dev, &peb2466_component_driver,
> > + &peb2466_dai_driver, 1);
> > + if (ret)
> > + goto failed;
> > +
> > + if (IS_ENABLED(CONFIG_GPIOLIB)) {
> > + ret = peb2466_gpio_init(peb2466);
> > + if (ret)
> > + goto failed;
> > + }
> > +
> > + return 0;
> > +
> > +failed:
> > + clk_disable_unprepare(peb2466->mclk);
> > + return ret;
>
> ... this error handling path ...
>
> > +}
> > +
> > +static void peb2466_spi_remove(struct spi_device *spi)
> > +{
> > + struct peb2466 *peb2466 = spi_get_drvdata(spi);
> > +
> > + clk_disable_unprepare(peb2466->mclk);
> > +}
>
> ... and the remove function.
>
> CJ
>
Thanks for pointing this.
I will use devm_clk_get_enabled() in the next series iteration as suggested.
Best regards,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
