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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
