Date:   Wed, 8 Feb 2023 08:10:48 +0100
From:   Herve Codina <herve.codina@bootlin.com>
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>, alsa-devel@alsa-project.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-gpio@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Subject: Re: [PATCH 2/3] ASoC: codecs: Add support for the Infineon PEB2466
 codec
Message-ID: <20230208081048.0708037f@bootlin.com>
In-Reply-To: <fd3ccda3-f964-6904-6056-f93c43b85a0f@wanadoo.fr>
References: <20230206144904.91078-1-herve.codina@bootlin.com>
        <20230206144904.91078-3-herve.codina@bootlin.com>
        <fd3ccda3-f964-6904-6056-f93c43b85a0f@wanadoo.fr>
Organization: Bootlin
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, 7 Feb 2023 22:17:39 +0100
Hi Christophe,

Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:

> Le 06/02/2023 =C3=A0 15:49, Herve Codina a =C3=A9crit=C2=A0:
> > The Infineon PEB2466 codec is a programmable DSP-based four channels
> > codec with filters capabilities.
> > It also provides signals as GPIOs.
> >=20
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >   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
> >  =20
>=20
> [...]
>=20
> > +static int peb2466_spi_probe(struct spi_device *spi)
> > +{
> > +	struct peb2466 *peb2466;
> > +	unsigned long mclk_rate;
> > +	int ret;
> > +	u8 xr5;
> > +
> > +	spi->bits_per_word =3D 8;
> > +	ret =3D spi_setup(spi);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	peb2466 =3D devm_kzalloc(&spi->dev, sizeof(*peb2466), GFP_KERNEL);
> > +	if (!peb2466)
> > +		return -ENOMEM;
> > +
> > +	peb2466->spi =3D spi;
> > +
> > +	peb2466->regmap =3D devm_regmap_init(&peb2466->spi->dev, NULL, peb246=
6,
> > +					   &peb2466_regmap_config);
> > +	if (IS_ERR(peb2466->regmap))
> > +		return PTR_ERR(peb2466->regmap);
> > +
> > +	peb2466->reset_gpio =3D 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 =3D devm_clk_get(&peb2466->spi->dev, "mclk"); =20
>=20
> Hi,
>=20
> Up to you to decide if it is a good idea or not, but using=20
> devm_clk_get_enabled() would save the 'mclk' field in peb2466 ...
>=20
> > +	if (IS_ERR(peb2466->mclk))
> > +		return PTR_ERR(peb2466->mclk);
> > +	ret =3D clk_prepare_enable(peb2466->mclk);
> > +	if (ret)
> > +		return ret; =20
>=20
> ... these 3 lines ...
>=20
> > +
> > +	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); =20
>=20
> ... this spi_set_drvdata() call ...
>=20
> > +
> > +	mclk_rate =3D clk_get_rate(peb2466->mclk);
> > +	switch (mclk_rate) {
> > +	case 1536000:
> > +		xr5 =3D PEB2466_XR5_MCLK_1536;
> > +		break;
> > +	case 2048000:
> > +		xr5 =3D PEB2466_XR5_MCLK_2048;
> > +		break;
> > +	case 4096000:
> > +		xr5 =3D PEB2466_XR5_MCLK_4096;
> > +		break;
> > +	case 8192000:
> > +		xr5 =3D PEB2466_XR5_MCLK_8192;
> > +		break;
> > +	default:
> > +		dev_err(&peb2466->spi->dev, "Unsupported clock rate %lu\n",
> > +			mclk_rate);
> > +		ret =3D -EINVAL;
> > +		goto failed;
> > +	}
> > +	ret =3D regmap_write(peb2466->regmap, PEB2466_XR5, xr5);
> > +	if (ret) {
> > +		dev_err(&peb2466->spi->dev, "Setting MCLK failed (%d)\n", ret);
> > +		goto failed;
> > +	}
> > +
> > +	ret =3D devm_snd_soc_register_component(&spi->dev, &peb2466_component=
_driver,
> > +					      &peb2466_dai_driver, 1);
> > +	if (ret)
> > +		goto failed;
> > +
> > +	if (IS_ENABLED(CONFIG_GPIOLIB)) {
> > +		ret =3D peb2466_gpio_init(peb2466);
> > +		if (ret)
> > +			goto failed;
> > +	}
> > +
> > +	return 0;
> > +
> > +failed:
> > +	clk_disable_unprepare(peb2466->mclk);
> > +	return ret; =20
>=20
> ... this error handling path ...
>=20
> > +}
> > +
> > +static void peb2466_spi_remove(struct spi_device *spi)
> > +{
> > +	struct peb2466 *peb2466 =3D spi_get_drvdata(spi);
> > +
> > +	clk_disable_unprepare(peb2466->mclk);
> > +} =20
>=20
> ... and the remove function.
>=20
> CJ
>=20

Thanks for pointing this.
I will use devm_clk_get_enabled() in the next series iteration as suggested.

Best regards,
Herv=C3=A9

--=20
Herv=C3=A9 Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com