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@xxxxxxxxxx> > --- > 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__ */ > + -- 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@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel