Re: [PATCH v1 07/10] ASoC: codecs: add wsa881x-i2c amplifier codec driver

["Alexey Klimov" <alexey.klimov@xxxxxxxxxx>]

On Fri Nov 1, 2024 at 8:12 AM GMT, Krzysztof Kozlowski wrote:
> On Fri, Nov 01, 2024 at 05:31:51AM +0000, Alexey Klimov wrote:
> > Add support to analog mode of WSA8810/WSA8815 Class-D Smart Speaker
> > family of amplifiers. Such amplifiers is primarily interfaced with
> > SoundWire but they also support analog mode which is configurable
> > by setting one of the pins to high/low. In such case the WSA881X
> > amplifier is configurable only using i2c.
> > 
> > To have stereo two WSA881X amplifiers are required but mono
> > configurations are also possible.
> > 
> > Cc: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > Signed-off-by: Alexey Klimov <alexey.klimov@xxxxxxxxxx>
> > ---
> >  sound/soc/codecs/Kconfig          |   11 +
> >  sound/soc/codecs/Makefile         |    2 +
> >  sound/soc/codecs/wsa881x-common.h |   19 +
> >  sound/soc/codecs/wsa881x-i2c.c    | 1454 +++++++++++++++++++++++++++++
> >  4 files changed, 1486 insertions(+)
> >  create mode 100644 sound/soc/codecs/wsa881x-i2c.c

> > +++ b/sound/soc/codecs/wsa881x-i2c.c

[...]

> > +struct reg_default wsa881x_ana_reg_defaults[] = {
>
> Not const?
>
> Same question everywhere further.

Here it doesn't work:

sound/soc/codecs/wsa881x-i2c.c: In function ‘wsa881x_update_reg_defaults_2_0’:
sound/soc/codecs/wsa881x-i2c.c:421:65: error: assignment of member ‘def’ in read-only object
  421 |                                 wsa881x_ana_reg_defaults[j].def =
      |                                                                 ^
sound/soc/codecs/wsa881x-i2c.c:428:65: error: assignment of member ‘def’ in read-only object
  428 |                                 wsa881x_ana_reg_defaults[j].def =
      |                                                                 ^

but I updated it other places.

> > +	{WSA881X_CHIP_ID0, 0x00},
> > +	{WSA881X_CHIP_ID1, 0x00},

[...]

> > +static void wsa881x_clk_ctrl(struct snd_soc_component *component, bool enable)
> > +{
> > +	struct wsa881x_priv *wsa881x =
> > +				snd_soc_component_get_drvdata(component);
> > +
> > +	dev_dbg(component->dev, "%s:ss enable:%d\n", __func__, enable);
>
> Please drop all tracing-like debugs from final code.

With a pleasure.

> > +static int wsa881x_i2c_probe(struct i2c_client *client)
> > +{
> > +	struct device *dev = &client->dev;
> > +	struct wsa881x_priv *wsa881x;
> > +	int wsa881x_index = 0;
> > +	int ret;
> > +
> > +	ret = wsa881x_probe_common(&wsa881x, dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = wsa881x_i2c_get_client_index(client, &wsa881x_index);
> > +	if (ret) {
> > +		dev_err(dev, "get codec I2C client failed\n");
> > +		return ret;
> > +	}
> > +	wsa881x->index = wsa881x_index;
>
> I cannot find how this is used. Your entire I2C address detection seems
> odd and not used at all. None of the I2C drivers are supposed to do
> this.

This is used to differentiate between two amplifiers, mostly in sound
component names. I found another way to implement this and it will be
present in version 2.

[..]

> > +	wsa881x->driver = devm_kzalloc(dev, sizeof(*wsa881x->driver),
> > +				       GFP_KERNEL);
> > +	if (!wsa881x->driver)
> > +		return -ENOMEM;
> > +
> > +	memcpy(wsa881x->driver, &soc_codec_dev_wsa881x,
> > +	       sizeof(*wsa881x->driver));
>
> Why not devm_kmemdump?

Because it doesn't yet exist in kernel.
But there is another nice thingy -- devm_kmemdup that can be used. Thanks!

> > +	wsa881x->dai_driver = devm_kzalloc(dev,

[...]

> Why this is just not module_i2c_driver?

Thank you for the review. I was aware that initialisation is not perfect,
it is inherited from downstream code.
I reworked a lot of these parts of wsa881x-i2c.c driver and will share it
in version 2.

Best regards,
Alexey