On Thu, 02 Aug 2018, Srinivas Kandagatla wrote:

On 02/08/18 09:33, Lee Jones wrote:

quoted

On Fri, 27 Jul 2018, Srinivas Kandagatla wrote:

quoted

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 <redacted>
---
  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.

[...]

quoted

quoted

+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 ... "

quoted

   gpio_direction_output(wcd->reset_gpio, 1);

... and drop the next 2 lines?

[...]

quoted

quoted

+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" :)

quoted

By the way, one line entries should be placed on one line.

quoted

+	{ .name = "wcd9335-codec" },

[...]

quoted

quoted

+static int wcd9335_slim_status(struct slim_device *sdev,
+				 enum slim_device_status s)

's' is not a good variable name.  Suggest 'status'.

Agreed!

quoted

quoted

+{
+	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
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel