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

[Lee Jones <lee.jones@linaro.org>]

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@linaro.org>
> ---
>  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/

> +	Qualcomm Technologies, Inc. (QTI) multimedia solutions, including
> +	the MSM8996, MSM8976, and MSM8956 chipsets. It has inbuild

s/inbuild/in-built/

> +	Soundwire controller, interrupt mux. It supports both I2S/I2C and
> +	SLIMbus audio interfaces. This option selects SLIMbus audio interface.

The help should be indented.

>  config MFD_STW481X
>  	tristate "Support for ST Microelectronics STw481x"
>  	depends on I2C && (ARCH_NOMADIK || COMPILE_TEST)
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 2852a6042ecf..a4697370640b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -56,6 +56,10 @@ endif
>  ifeq ($(CONFIG_MFD_CS47L24),y)
>  obj-$(CONFIG_MFD_ARIZONA)	+= cs47l24-tables.o
>  endif
> +
> +obj-$(CONFIG_MFD_WCD9335)	+= wcd9335.o
> +wcd9335-objs			:= wcd9335-core.o
> +
>  obj-$(CONFIG_MFD_WM8400)	+= wm8400-core.o
>  wm831x-objs			:= wm831x-core.o wm831x-irq.o wm831x-otp.o
>  wm831x-objs			+= wm831x-auxadc.o
> diff --git a/drivers/mfd/wcd9335-core.c b/drivers/mfd/wcd9335-core.c
> new file mode 100644
> index 000000000000..8f746901f4e9
> --- /dev/null
> +++ b/drivers/mfd/wcd9335-core.c
> @@ -0,0 +1,268 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018, Linaro Limited
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slimbus.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
> +#include <linux/mfd/core.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/mfd/wcd9335/wcd9335.h>
> +#include <linux/mfd/wcd9335/registers.h>

Alphabetical.

> +static const struct regmap_range_cfg wcd9335_ranges[] = {
> +	{	.name = "WCD9335",

What is that?  New line please.

> +		.range_min =  0x0,
> +		.range_max =  WCD9335_MAX_REGISTER,
> +		.selector_reg = WCD9335_REG(0x0, 0),
> +		.selector_mask = 0xff,
> +		.selector_shift = 0,
> +		.window_start = 0x0,
> +		.window_len = 0x1000,
> +	},
> +};
> +
> +static struct regmap_config wcd9335_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.cache_type = REGCACHE_RBTREE,
> +	.max_register = WCD9335_MAX_REGISTER,
> +	.can_multi_write = true,
> +	.ranges = wcd9335_ranges,
> +	.num_ranges = ARRAY_SIZE(wcd9335_ranges),
> +};
> +
> +static const struct regmap_range_cfg wcd9335_ifd_ranges[] = {
> +	{	.name = "WCD9335-IFD",

As above.

> +		.range_min =  0x0,
> +		.range_max = WCD9335_REG(0, 0x7ff),
> +		.selector_reg = WCD9335_REG(0, 0x0),
> +		.selector_mask = 0xff,
> +		.selector_shift = 0,
> +		.window_start = 0x0,
> +		.window_len = 0x1000,
> +	},
> +};
> +
> +static struct regmap_config wcd9335_ifd_regmap_config = {
> +	.reg_bits = 16,
> +	.val_bits = 8,
> +	.can_multi_write = true,
> +	.max_register = WCD9335_REG(0, 0x7FF),
> +	.ranges = wcd9335_ifd_ranges,
> +	.num_ranges = ARRAY_SIZE(wcd9335_ifd_ranges),
> +};
> +
> +static int wcd9335_parse_dt(struct wcd9335 *wcd)
> +{
> +	struct device *dev = wcd->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	wcd->reset_gpio = of_get_named_gpio(np,	"reset-gpio", 0);
> +	if (wcd->reset_gpio < 0) {
> +		dev_err(dev, "Reset gpio missing in DT\n");

s/gpio/GPIO/

s/missing in DT/missing from DT/

> +		return wcd->reset_gpio;
> +	}
> +
> +	wcd->mclk = devm_clk_get(dev, "mclk");
> +	if (IS_ERR(wcd->mclk)) {
> +		dev_err(dev, "mclk not found\n");
> +		return PTR_ERR(wcd->mclk);
> +	}
> +
> +	wcd->native_clk = devm_clk_get(dev, "slimbus");
> +	if (IS_ERR(wcd->native_clk)) {
> +		dev_err(dev, "slimbus clk not found\n");

Any reason for abbreviating 'clock' in the error message?

> +		return PTR_ERR(wcd->native_clk);
> +	}
> +
> +	wcd->supplies[0].supply = "vdd-buck";
> +	wcd->supplies[1].supply = "vdd-buck-sido";
> +	wcd->supplies[2].supply = "vdd-tx";
> +	wcd->supplies[3].supply = "vdd-rx";
> +	wcd->supplies[4].supply = "vdd-io";
> +
> +	ret = regulator_bulk_get(dev, WCD9335_MAX_SUPPLY, wcd->supplies);
> +	if (ret != 0) {

"if (ret)"

Same for all.  I won't comment on the others.

> +		dev_err(dev, "Failed to get supplies: err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +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:

  gpio_direction_output(wcd->reset_gpio, 1);

... and drop the next 2 lines?

> +	gpio_set_value(wcd->reset_gpio, 1);
> +	msleep(20);
> +
> +	return 0;
> +}
> +
> +static int wcd9335_bring_up(struct wcd9335 *wcd)
> +{
> +	struct regmap *rm = wcd->regmap;
> +	int val, byte0;
> +	int ret = 0;
> +
> +	regmap_read(rm, WCD9335_CHIP_TIER_CTRL_EFUSE_VAL_OUT0, &val);
> +	regmap_read(rm, WCD9335_CHIP_TIER_CTRL_CHIP_ID_BYTE0, &byte0);
> +
> +	if ((val < 0) || (byte0 < 0)) {
> +		dev_err(wcd->dev, "wcd9335 codec version detection fail!\n");

s/wcd9335 codec/WCD9335 CODEC/ ?

> +		return -EINVAL;
> +	}
> +
> +	if (byte0 == 0x1) {
> +		dev_info(wcd->dev, "wcd9335 codec version is v2.0\n");
> +		wcd->version = WCD9335_VERSION_2_0;
> +		regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x01);
> +		regmap_write(rm, WCD9335_SIDO_SIDO_TEST_2, 0x00);
> +		regmap_write(rm, WCD9335_SIDO_SIDO_CCL_8, 0x6F);
> +		regmap_write(rm, WCD9335_BIAS_VBG_FINE_ADJ, 0x65);
> +		regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x5);
> +		regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x7);
> +		regmap_write(rm, WCD9335_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x3);
> +		regmap_write(rm, WCD9335_CODEC_RPM_RST_CTL, 0x3);
> +	} else {
> +		dev_err(wcd->dev, "wcd9335 codec version not supported\n");

As above.

> +		ret = -EINVAL;

Why not just

  return -EINVAL;

Then you can just return 0 and avoid pre-initialising ret.

> +	}
> +
> +	return ret;
> +}
> +
> +static int wcd9335_slim_probe(struct slim_device *slim)
> +{
> +	struct device *dev = &slim->dev;
> +	struct wcd9335 *wcd;
> +	int ret = 0;

Why pre-initialise?

> +	/* Interface device */
> +	if (slim->e_addr.dev_index == 0)

Is 0 the parent?  I think 0 needs defining for clarity.

> +		return 0;
> +
> +	wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL);
> +	if (!wcd)
> +		return	-ENOMEM;
> +
> +	wcd->dev = dev;

'\n' here.

> +	ret = wcd9335_parse_dt(wcd);
> +	if (ret) {
> +		dev_err(dev, "Error parsing DT (%d)\n", ret);

This format changes from message to message.

Please pick one and stick with it.

I personally like: "<MESSAGE>: %d", ret

> +		return ret;
> +	}
> +
> +	ret = wcd9335_power_on_reset(wcd);
> +	if (ret) {
> +		dev_err(dev, "Error Powering\n");

I think this is superflouous since you already printed a message.

> +		return ret;
> +	}
> +
> +	wcd->regmap = regmap_init_slimbus(slim, &wcd9335_regmap_config);
> +	if (IS_ERR(wcd->regmap)) {
> +		ret = PTR_ERR(wcd->regmap);
> +		dev_err(dev, "Failed to allocate register map:%d\n", ret);

A different format again.  Ensure there is a ' ' after the ':'.

> +		return ret;
> +	}
> +
> +	dev_set_drvdata(dev, wcd);

Probably nicer to do this at the very end - after a '\n'.

> +	wcd->slim = slim;
> +	wcd->intf_type = WCD9335_INTERFACE_TYPE_SLIMBUS;
> +
> +	return 0;
> +}
> +
> +static const struct mfd_cell wcd9335_devices[] = {
> +	{
> +		.name = "wcd9335-codec",
> +	},
> +};

When are you going to add the other devices?

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

> +{
> +	struct device_node *ifd_np;

What's 'ifd'?

> +	struct wcd9335 *wcd;
> +	struct device *dev;
> +	int ret;
> +
> +	/* Interface device */

As previously suggested just define the device ID and drop the
comment.

> +	if (sdev->e_addr.dev_index == 0)
> +		return 0;
> +
> +	wcd = dev_get_drvdata(&sdev->dev);
> +	dev = wcd->dev;

Odd.  You do to the effort of assigning this and use 'wcd->dev' at
most call sites anyway.  I'd drop 'dev' and just use it from 'wcd'
everywhere.

> +	ifd_np = of_parse_phandle(wcd->dev->of_node, "qcom,ifd", 0);
> +	if (!ifd_np) {
> +		dev_err(wcd->dev, "No Interface device found\n");
> +		return -EINVAL;
> +	}
> +
> +	wcd->slim_ifd = of_slim_get_device(sdev->ctrl, ifd_np);
> +	if (!wcd->slim_ifd) {
> +		dev_err(wcd->dev, "Unable to get SLIM Interface device\n");
> +		return -EINVAL;
> +	}
> +
> +	wcd->ifd_regmap = regmap_init_slimbus(wcd->slim_ifd,
> +					      &wcd9335_ifd_regmap_config);
> +	if (IS_ERR(wcd->ifd_regmap)) {
> +		dev_err(dev, "Failed to allocate register map\n");
> +		return PTR_ERR(wcd->ifd_regmap);
> +	}
> +
> +	ret = wcd9335_bring_up(wcd);
> +	if (ret) {
> +		dev_err(dev, "Failed to bringup WCD9335\n");
> +		return ret;
> +	}
> +
> +	wcd->slim_ifd = wcd->slim_ifd;

Am I missing something?

> +	return mfd_add_devices(wcd->dev, 0, wcd9335_devices,
> +			       ARRAY_SIZE(wcd9335_devices), NULL, 0, NULL);

No error message if it were to fail?

> +}
> +
> +static const struct slim_device_id wcd9335_slim_id[] = {
> +	{0x217, 0x1a0, 0x1, 0x0},

Well that's just horrific.  Can't these be defined?

> +	{}
> +};
> +
> +static struct slim_driver wcd9335_slim_driver = {
> +	.driver = {
> +		.name = "wcd9335-slim",
> +	},
> +	.probe = wcd9335_slim_probe,
> +	.device_status = wcd9335_slim_status,
> +	.id_table = wcd9335_slim_id,
> +};
> +
> +module_slim_driver(wcd9335_slim_driver);
> +MODULE_DESCRIPTION("WCD9335 slim driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/wcd9335/registers.h b/include/linux/mfd/wcd9335/registers.h

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog