Re: [alsa-devel] [PATCH v2 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec

On Fri, 18 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

drivers/mfd/Kconfig | 8 + drivers/mfd/Makefile | 1 + drivers/mfd/wcd934x.c | 330 ++++++++++++++++ include/linux/mfd/wcd934x/registers.h | 529 ++++++++++++++++++++++++++ include/linux/mfd/wcd934x/wcd934x.h | 24 ++ 5 files changed, 892 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

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig index ae24d3ea68ea..ab09862b5996 100644 --- a/drivers/mfd/Kconfig +++ b/drivers/mfd/Kconfig @@ -1967,6 +1967,14 @@ 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

No help?

menu "Multimedia Capabilities Port drivers" depends on ARCH_SA1100

diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile index c1067ea46204..8059a9c36188 100644 --- a/drivers/mfd/Makefile +++ b/drivers/mfd/Makefile @@ -58,6 +58,7 @@ endif ifeq ($(CONFIG_MFD_CS47L24),y) obj-$(CONFIG_MFD_ARIZONA) += cs47l24-tables.o endif +obj-$(CONFIG_MFD_WCD934X) += wcd934x.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/wcd934x.c b/drivers/mfd/wcd934x.c new file mode 100644 index 000000000000..bb4d2a6c89bc --- /dev/null +++ b/drivers/mfd/wcd934x.c @@ -0,0 +1,330 @@ +// SPDX-License-Identifier: GPL-2.0 +// Copyright (c) 2019, Linaro Limited

+#include <linux/clk.h> +#include <linux/gpio.h> +#include <linux/interrupt.h> +#include <linux/kernel.h> +#include <linux/mfd/core.h> +#include <linux/mfd/wcd934x/registers.h> +#include <linux/mfd/wcd934x/wcd934x.h> +#include <linux/module.h> +#include <linux/of_gpio.h> +#include <linux/of.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/slimbus.h>

+static const struct mfd_cell wcd934x_devices[] = {

• { /* Audio Codec */

These comments aren't required. The .names are good enough.

• .name = "wcd934x-codec",
  

• }, { /* Pin controller */

• .name = "wcd934x-pinctrl",
  

• .of_compatible = "qcom,wcd9340-pinctrl",
  

• }, { /* Soundwire Controller */

• .name = "wcd934x-soundwire",
  

• .of_compatible = "qcom,soundwire-v1.3.0",
  

• },

+};

+static const struct regmap_irq wcd934x_irqs[] = {

• /* INTR_REG 0 */

Again, doesn't really add or explain anything additional.

• [WCD934X_IRQ_SLIMBUS] = {

• .reg_offset = 0,
  

• .mask = BIT(0),
  

• .type = {
  

• 	.type_reg_offset = 0,
  

• 	.types_supported = IRQ_TYPE_EDGE_BOTH,
  

• 	.type_reg_mask  = BIT(0),
  

• 	.type_level_low_val = BIT(0),
  

• 	.type_level_high_val = BIT(0),
  

• 	.type_falling_val = 0,
  

• 	.type_rising_val = 0,
  

• },
  

• },
• [WCD934X_IRQ_SOUNDWIRE] = {

• .reg_offset = 2,
  

• .mask = BIT(4),
  

• .type = {
  

• 	.type_reg_offset = 2,
  

• 	.types_supported = IRQ_TYPE_EDGE_BOTH,
  

• 	.type_reg_mask  = BIT(4),
  

• 	.type_level_low_val = BIT(4),
  

• 	.type_level_high_val = BIT(4),
  

• 	.type_falling_val = 0,
  

• 	.type_rising_val = 0,
  

• },
  

• },

+};

+static const struct regmap_irq_chip wcd934x_regmap_irq_chip = {

• .name = "wcd934x_irq",
• .status_base = WCD934X_INTR_PIN1_STATUS0,
• .mask_base = WCD934X_INTR_PIN1_MASK0,
• .ack_base = WCD934X_INTR_PIN1_CLEAR0,
• .type_base = WCD934X_INTR_LEVEL0,
• .num_type_reg = 4,
• .type_in_mask = false,
• .num_regs = 4,
• .irqs = wcd934x_irqs,
• .num_irqs = ARRAY_SIZE(wcd934x_irqs),

+};

+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;
  

• }

+};

+static const struct regmap_range_cfg wcd934x_ranges[] = {

• { .name = "WCD934X",

• .range_min =  0x0,
  

• .range_max =  WCD934X_MAX_REGISTER,
  

• .selector_reg = WCD934X_SEL_REGISTER,
  

• .selector_mask = WCD934X_SEL_MASK,
  

• .selector_shift = WCD934X_SEL_SHIFT,
  

• .window_start = WCD934X_WINDOW_START,
  

• .window_len = WCD934X_WINDOW_LENGTH,
  

• },

+};

+static struct regmap_config wcd934x_regmap_config = {

• .reg_bits = 16,
• .val_bits = 8,
• .cache_type = REGCACHE_RBTREE,
• .max_register = 0xffff,
• .can_multi_write = true,
• .ranges = wcd934x_ranges,
• .num_ranges = ARRAY_SIZE(wcd934x_ranges),
• .volatile_reg = wcd934x_is_volatile_register,

+};

+static int wcd934x_parse_resources(struct wcd934x_data *wcd) +{

• struct device *dev = wcd->dev;

Since most things stem from the device, it's more common to pass that instead. You can then use dev_get_drvdata() to obtain the device data.

• struct device_node *np = dev->of_node;
• int ret;
• wcd->irq = of_irq_get(wcd->dev->of_node, 0);
• if (wcd->irq < 0) {

• if (wcd->irq != -EPROBE_DEFER)
  

• 	dev_err(wcd->dev, "Failed to get IRQ: err = %d\n",
  

• 		wcd->irq);
  

• return wcd->irq;
  

• }
• wcd->reset_gpio = of_get_named_gpio(np, "reset-gpios", 0);
• if (wcd->reset_gpio < 0) {

• dev_err(dev, "Failed to get reset gpio: err = %d\n",
  

• 	wcd->reset_gpio);
  

• return wcd->reset_gpio;
  

• }
• wcd->extclk = devm_clk_get(dev, "extclk");
• if (IS_ERR(wcd->extclk)) {

• dev_err(dev, "Failed to get extclk");
  

• return PTR_ERR(wcd->extclk);
  

• }
• 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, WCD934X_MAX_SUPPLY, wcd->supplies);
• if (ret != 0) {

• dev_err(dev, "Failed to get supplies: err = %d\n", ret);
  

• return ret;
  

• }

More of a nit-pick really, but I don't see any reason for all of this not to be in probe.

• return 0;

+}

+static int wcd934x_power_on_reset(struct wcd934x_data *wcd) +{

• int ret;
• ret = regulator_bulk_enable(WCD934X_MAX_SUPPLY, wcd->supplies);
• if (ret != 0) {

• dev_err(wcd->dev, "Failed to get supplies: err = %d\n", ret);
  

• return ret;
  

• }

By doing regulator_bulk_{get,enable}() in separate functions, you put an unnecessary burden on 'struct wcd934x_data'. Unless of course you're using 'supplies' to disable and put as well. Where does that happen? Surely you should do that in .remove()?

• /*

• * 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(wcd->reset_gpio, 0);
• msleep(20);
• gpio_set_value(wcd->reset_gpio, 1);
• msleep(20);
• return 0;

+}

+static int wcd934x_slim_probe(struct slim_device *sdev)

I'd expect to find the .probe() closer to the bottom of the file.

+{

• struct device *dev = &sdev->dev;
• struct wcd934x_data *wcd;

This is actually "ddata". It's more common to do:

struct wcd934x_ddata *ddata;

• int ret = 0;

Why does this need pre-initialisation?

• wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL);
• if (!wcd)

• return	-ENOMEM;
  

• wcd->dev = dev;

'\n' here.

• ret = wcd934x_parse_resources(wcd);
• if (ret) {

• dev_err(dev, "Error parsing DT: err = %d\n", ret);
  

You've already printed an error at this point. No need for another.

• return ret;
  

• }
• ret = wcd934x_power_on_reset(wcd);
• if (ret) {

• dev_err(dev, "Error Power resetting device: err = %d\n", ret);
  

You've already printed an error at this point. No need for another.

• return ret;
  

• }
• wcd->sdev = sdev;
• dev_set_drvdata(dev, wcd);

Do this first, then you can use dev_get_drvdata() above.

• return 0;

+}

+static int wcd934x_bring_up(struct wcd934x_data *wcd) +{

• struct regmap *rm = wcd->regmap;

It's much more common to use 'regmap' or 'map'.

• u16 id_minor, id_major;
• int ret;
• ret = regmap_bulk_read(rm, WCD934X_CHIP_TIER_CTRL_CHIP_ID_BYTE0,

• 	       (u8 *)&id_minor, sizeof(u16));
  

• if (ret)

• return -EINVAL;
  

• ret = regmap_bulk_read(rm, WCD934X_CHIP_TIER_CTRL_CHIP_ID_BYTE2,

• 	       (u8 *)&id_major, sizeof(u16));
  

• if (ret)

• return -EINVAL;
  

• dev_info(wcd->dev, "wcd934x chip id major 0x%x, minor 0x%x\n",

•  id_major, id_minor);
  

• regmap_write(rm, WCD934X_CODEC_RPM_RST_CTL, 0x01);
• regmap_write(rm, WCD934X_SIDO_NEW_VOUT_A_STARTUP, 0x19);
• regmap_write(rm, WCD934X_SIDO_NEW_VOUT_D_STARTUP, 0x15);
• /* Add 1msec delay for VOUT to settle */
• usleep_range(1000, 1100);
• regmap_write(rm, WCD934X_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x5);
• regmap_write(rm, WCD934X_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x7);
• regmap_write(rm, WCD934X_CODEC_RPM_RST_CTL, 0x3);
• regmap_write(rm, WCD934X_CODEC_RPM_RST_CTL, 0x7);
• regmap_write(rm, WCD934X_CODEC_RPM_PWR_CDC_DIG_HM_CTL, 0x3);
• return 0;

Superfluous '\n'.

+}

+static int wcd934x_slim_status(struct slim_device *sdev,

• 	       enum slim_device_status status)
  

+{

• struct device *dev = &sdev->dev;
• struct wcd934x_data *wcd;
• int ret;

This is semantically odd! Why are you doing most of the initialisation and bring-up in 'status' and not 'probe'. Seems broken to me.

• wcd = dev_get_drvdata(dev);
• switch (status) {
• case SLIM_DEVICE_STATUS_UP:

• wcd->regmap = regmap_init_slimbus(sdev, &wcd934x_regmap_config);
  

• if (IS_ERR(wcd->regmap)) {
  

• 	dev_err(dev, "Error allocating slim regmap\n");
  

• 	return PTR_ERR(wcd->regmap);
  

• }
  

• ret = wcd934x_bring_up(wcd);
  

• if (ret) {
  

• 	dev_err(dev, "Error WCD934X bringup: err = %d\n", ret);
  

Please use well formed English.

"Failed to bring up WCD934X".

• 	return ret;
  

• }
  

• ret = devm_regmap_add_irq_chip(wcd->dev, wcd->regmap, wcd->irq,
  

• 			       IRQF_TRIGGER_HIGH, 0,
  

• 			       &wcd934x_regmap_irq_chip,
  

• 			       &wcd->irq_data);
  

• if (ret) {
  

• 	dev_err(wcd->dev, "Error adding irq_chip: err = %d\n",
  

"Failed to add IRQ chip".

• 		ret);
  

• 	return ret;
  

• }
  

• ret = mfd_add_devices(wcd->dev, 0, wcd934x_devices,
  

What do you mean by 0 here?

• 		      ARRAY_SIZE(wcd934x_devices),
  

• 		      NULL, 0, NULL);
  

• if (ret) {
  

• 	dev_err(wcd->dev, "Error add mfd devices: err = %d\n",
  

"Failed to add child devices".

• 		ret);
  

• 	return ret;
  

• }
  

• break;
  

• case SLIM_DEVICE_STATUS_DOWN:

• mfd_remove_devices(wcd->dev);
  

• break;
  

• default:

• return -EINVAL;
  

• }
• return 0;

+}

+static void wcd934x_slim_remove(struct slim_device *sdev) +{

• struct wcd934x_data *wcd = dev_get_drvdata(&sdev->dev);

No more clean-up? Aren't the regulators still enabled?

• mfd_remove_devices(&sdev->dev);
• kfree(wcd);

+}

+static const struct slim_device_id wcd934x_slim_id[] = {

• {SLIM_MANF_ID_QCOM, SLIM_PROD_CODE_WCD9340, 0x1, 0x0},

' 's before and after first and last chars please.

• {}

+};

+static struct slim_driver wcd934x_slim_driver = {

• .driver = {

• .name = "wcd934x-slim",
  

• },
• .probe = wcd934x_slim_probe,
• .remove = wcd934x_slim_remove,
• .device_status = wcd934x_slim_status,
• .id_table = wcd934x_slim_id,

+};

+module_slim_driver(wcd934x_slim_driver); +MODULE_DESCRIPTION("WCD934X slim driver"); +MODULE_LICENSE("GPL v2"); +MODULE_ALIAS("slim:217:250:*");

MODULE_AUTHOR?

[...]

+#endif diff --git a/include/linux/mfd/wcd934x/wcd934x.h b/include/linux/mfd/wcd934x/wcd934x.h new file mode 100644 index 000000000000..d102e211948c --- /dev/null +++ b/include/linux/mfd/wcd934x/wcd934x.h @@ -0,0 +1,24 @@ +/* SPDX-License-Identifier: GPL-2.0 */

+#ifndef __WCD934X_H__ +#define __WCD934X_H__ +#include <linux/clk.h> +#include <linux/regulator/consumer.h> +#include <linux/regmap.h> +#include <linux/slimbus.h>

+#define WCD934X_MAX_SUPPLY 5

Kernel doc?

+struct wcd934x_data {

• int reset_gpio;
• int irq;

I'd prefer to see the more complex 'struct's at the top.

• struct device *dev;
• struct clk *extclk;
• struct regmap *regmap;
• struct slim_device *sdev;

You don't need 'sdev' and 'dev'.

• struct regmap_irq_chip_data *irq_data;
• struct regulator_bulk_data supplies[WCD934X_MAX_SUPPLY];

+};

+#endif /* __WCD934X_H__ */