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

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

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

No changelog?

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

[...]

> +static bool wcd934x_is_volatile_register(struct device *dev, unsigned int reg)
> +{
> +	switch (reg) {
> +	case WCD934X_INTR_PIN1_STATUS0...WCD934X_INTR_PIN2_CLEAR3:
> +	case WCD934X_SWR_AHB_BRIDGE_RD_DATA_0:
> +	case WCD934X_SWR_AHB_BRIDGE_RD_DATA_1:
> +	case WCD934X_SWR_AHB_BRIDGE_RD_DATA_2:
> +	case WCD934X_SWR_AHB_BRIDGE_RD_DATA_3:
> +	case WCD934X_SWR_AHB_BRIDGE_ACCESS_STATUS:
> +	case WCD934X_ANA_MBHC_RESULT_3:
> +	case WCD934X_ANA_MBHC_RESULT_2:
> +	case WCD934X_ANA_MBHC_RESULT_1:
> +	case WCD934X_ANA_MBHC_MECH:
> +	case WCD934X_ANA_MBHC_ELECT:
> +	case WCD934X_ANA_MBHC_ZDET:
> +	case WCD934X_ANA_MICB2:
> +	case WCD934X_ANA_RCO:
> +	case WCD934X_ANA_BIAS:
> +		return true;
> +	default:
> +		return false;
> +	}
> +

Nit: Please remove the superfluous empty line.

> +};

[...]

> +static int wcd934x_slim_probe(struct slim_device *sdev)
> +{
> +	struct device *dev = &sdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct wcd934x_ddata *ddata;
> +	int reset_gpio, ret;
> +
> +	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
> +	if (!ddata)
> +		return	-ENOMEM;
> +
> +	ddata->irq = of_irq_get(np, 0);
> +	if (ddata->irq < 0) {
> +		if (ddata->irq != -EPROBE_DEFER)
> +			dev_err(ddata->dev, "Failed to get IRQ: err = %d\n",
> +				ddata->irq);
> +		return ddata->irq;
> +	}
> +
> +	reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
> +	if (reset_gpio < 0) {
> +		dev_err(dev, "Failed to get reset gpio: err = %d\n",
> +			reset_gpio);
> +		return reset_gpio;
> +	}
> +
> +	ddata->extclk = devm_clk_get(dev, "extclk");
> +	if (IS_ERR(ddata->extclk)) {
> +		dev_err(dev, "Failed to get extclk");
> +		return PTR_ERR(ddata->extclk);
> +	}
> +
> +	ddata->supplies[0].supply = "vdd-buck";
> +	ddata->supplies[1].supply = "vdd-buck-sido";
> +	ddata->supplies[2].supply = "vdd-tx";
> +	ddata->supplies[3].supply = "vdd-rx";
> +	ddata->supplies[4].supply = "vdd-io";
> +
> +	ret = regulator_bulk_get(dev, WCD934X_MAX_SUPPLY, ddata->supplies);
> +	if (ret != 0) {

Nit: "if (ret)"

> +		dev_err(dev, "Failed to get supplies: err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_bulk_enable(WCD934X_MAX_SUPPLY, ddata->supplies);
> +	if (ret != 0) {

Nit: "if (ret)"

> +		dev_err(dev, "Failed to enable supplies: err = %d\n", ret);
> +		return ret;
> +	}
> +
> +	/*
> +	 * For WCD934X, 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(reset_gpio, 0);
> +	msleep(20);
> +	gpio_set_value(reset_gpio, 1);
> +	msleep(20);
> +
> +	ddata->dev = dev;
> +	dev_set_drvdata(dev, ddata);
> +
> +	return 0;
> +}
> +
> +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.

> +	{}
> +};

[...]

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