Re: [PATCH v3 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec

[Srinivas Kandagatla]

On 11/11/2019 11:18, Lee Jones wrote:
> On Tue, 29 Oct 2019, Srinivas Kandagatla wrote:
>
> > Qualcomm WCD9340/WCD9341 Codec is a standalone Hi-Fi audio codec IC.
> >
> > This codec has integrated SoundWire controller, pin controller and
> > interrupt controller.
> >
> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > ---
>
> No changelog?

I have done that in cover letter.
If you prefer it here, I can add that in next version.

> >   drivers/mfd/Kconfig                   |  12 +
> >   drivers/mfd/Makefile                  |   1 +
> >   drivers/mfd/wcd934x.c                 | 306 +++++++++++++++
> >   include/linux/mfd/wcd934x/registers.h | 529 ++++++++++++++++++++++++++
> >   include/linux/mfd/wcd934x/wcd934x.h   |  31 ++
> >   5 files changed, 879 insertions(+)
> >   create mode 100644 drivers/mfd/wcd934x.c
> >   create mode 100644 include/linux/mfd/wcd934x/registers.h
> >   create mode 100644 include/linux/mfd/wcd934x/wcd934x.h
>
> This driver reads much better now. Thanks for making the changes.
>
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index ae24d3ea68ea..9fe7e54b13bf 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -1967,6 +1967,18 @@ config MFD_STMFX
> >   	  additional drivers must be enabled in order to use the functionality
> >   	  of the device.
> >   
> > +config MFD_WCD934X
> > +	tristate "Support for WCD9340/WCD9341 Codec"
> > +	depends on SLIMBUS
> > +	select REGMAP
> > +	select REGMAP_SLIMBUS
> > +	select REGMAP_IRQ
> > +	select MFD_CORE
> > +	help
> > +	  Support for the Qualcomm WCD9340/WCD9341 Codec.
> > +	  This driver provides common support wcd934x audio codec and its
> > +	  associated Pin Controller, Soundwire Controller and Audio codec.
>
> Your capitalisation of devices is all over the place in both your help
> section and in the commit message. Either capitalise them all or none
> of them. Personally I would prefer all, rather than none. What ever
> you choose, please be consistent.
>
> Same for "wcd934x", this should read "WCD934x" in all comments and the
> help.

I agree, will fix it along with other Nits you suggested.


[...]
> > +static void wcd934x_slim_remove(struct slim_device *sdev)
> > +{
> > +	struct wcd934x_ddata *ddata = dev_get_drvdata(&sdev->dev);
> > +
> > +	regulator_bulk_disable(WCD934X_MAX_SUPPLY, ddata->supplies);
> > +	mfd_remove_devices(&sdev->dev);
> > +	kfree(ddata);
> > +}
> > +
> > +static const struct slim_device_id wcd934x_slim_id[] = {
> > +	{ SLIM_MANF_ID_QCOM, SLIM_PROD_CODE_WCD9340, 0x1, 0x0 },
>
> What do the last parameters mean? Might be better to define them.

This is Instance ID and Device ID of SLIMBus enumeration address.


> > +	{}
> > +};
>
> [...]