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
