Re: [PATCH v2 02/10] mfd: wcd9335: add support to wcd9335 core

[Lee Jones]

On Thu, 02 Aug 2018, Srinivas Kandagatla wrote:
> On 02/08/18 09:33, Lee Jones wrote:
> > On Fri, 27 Jul 2018, Srinivas Kandagatla wrote:
> > 
> > > Qualcomm WCD9335 Codec is a standalone Hi-Fi audio codec IC,
> > > It has mulitple blocks like Soundwire controller, codec,
> > > Codec processing engine, ClassH controller, interrupt mux.
> > > It supports both I2S/I2C and SLIMbus audio interfaces.
> > > 
> > > This patch adds support to SLIMbus audio interface.
> > > 
> > > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
> > > ---
> > >   drivers/mfd/Kconfig                   |  18 ++
> > >   drivers/mfd/Makefile                  |   4 +
> > >   drivers/mfd/wcd9335-core.c            | 268 ++++++++++++++++
> > >   include/linux/mfd/wcd9335/registers.h | 580 ++++++++++++++++++++++++++++++++++
> > >   include/linux/mfd/wcd9335/wcd9335.h   |  42 +++
> > >   5 files changed, 912 insertions(+)
> > >   create mode 100644 drivers/mfd/wcd9335-core.c
> > >   create mode 100644 include/linux/mfd/wcd9335/registers.h
> > >   create mode 100644 include/linux/mfd/wcd9335/wcd9335.h
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index f3fa516011ec..6e5b5f3cfe20 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1807,6 +1807,24 @@ config MFD_WM97xx
> > >   	  support for the WM97xx, in order to use the actual functionaltiy of
> > >   	  the device other drivers must be enabled.
> > > +config MFD_WCD9335
> > > +	tristate
> > > +	select MFD_CORE
> > > +	select REGMAP
> > > +	select REGMAP_IRQ
> > > +
> > > +config MFD_WCD9335_SLIM
> > > +	tristate "Qualcomm WCD9335 with SLIMbus"
> > > +	select MFD_WCD9335
> > > +	select REGMAP_SLIMBUS
> > > +	depends on SLIMBUS
> > > +	help
> > > +	The WCD9335 is a standalone Hi-Fi audio codec IC, supports
> > 
> > s/codec/CODEC/
> Yep.

If you agree with something, best just to snip it.

No need to reply at all if you agree with everything.

[...]

> > > +static int wcd9335_power_on_reset(struct wcd9335 *wcd)
> > > +{
> > > +	struct device *dev = wcd->dev;
> > > +	int ret;
> > > +
> > > +	ret = regulator_bulk_enable(WCD9335_MAX_SUPPLY, wcd->supplies);
> > > +	if (ret != 0) {
> > > +		dev_err(dev, "Failed to get supplies: err = %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	/*
> > > +	 * For WCD9335, it takes about 600us for the Vout_A and
> > > +	 * Vout_D to be ready after BUCK_SIDO is powered up.
> > > +	 * SYS_RST_N shouldn't be pulled high during this time
> > > +	 */
> > > +	usleep_range(600, 650);
> > > +
> > > +	gpio_direction_output(wcd->reset_gpio, 0);
> > > +	msleep(20);
> > 
> > What's this for?  Why can't you just:
> > 
> This is just making sure that reset line is low before actual the chip is
> taken out of reset.

If it confused me, it might confuse others.

Best to add a comment to the tune of:

  "Toggle reset line off/on to ensure ... "

> >    gpio_direction_output(wcd->reset_gpio, 1);
> > 
> > ... and drop the next 2 lines?

[...]

> > > +static const struct mfd_cell wcd9335_devices[] = {
> > > +	{
> > > +		.name = "wcd9335-codec",
> > > +	},
> > > +};
> > 
> > When are you going to add the other devices?
> > 
> We are trying to sort of hw setup to test other devices like soundwire
> controller, will add these once we are in a position to test them.

Ensure that you do, or I'll revert the whole driver as "not an MFD" :)

> > By the way, one line entries should be placed on one line.
> > 
> > > +	{ .name = "wcd9335-codec" },

[...]

> > > +static int wcd9335_slim_status(struct slim_device *sdev,
> > > +				 enum slim_device_status s)
> > 
> > 's' is not a good variable name.  Suggest 'status'.
> > 
> Agreed!
> > > +{
> > > +	struct device_node *ifd_np;
> > 
> > What's 'ifd'?
> > 
> SLIMbus Interface device.

That's a terrible variable name. :)

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog