Re: [PATCH v2 02/11] mfd: wcd934x: add support to wcd9340/wcd9341 codec
From: Srinivas Kandagatla
Date: Mon Oct 21 2019 - 07:25:44 EST
Thanks Lee for taking time to review.
I agree with most of the style related comments, will fix them in next
version. For others I have replied it inline.
On 21/10/2019 11:46, Lee Jones wrote:
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.
[...]
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 int wcd934x_bring_up(struct wcd934x_data *wcd)
+{
+ struct regmap *rm = wcd->regmap;
It's much more common to use 'regmap' or 'map'.
Only reason to make it short here is to save some lines!
If you prefer regmap, I will add that in next version!
+ 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.
SLIMBus device will not be in a state to communicate before enumeration
(at probe), so all the device initialization is done in status callback
where it is ready for communication.
This is same with SoundWire Bus as well!
+ 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);
[...]
+ 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?
Good point, will add missing regulator disable in next version.
+ mfd_remove_devices(&sdev->dev);
+ kfree(wcd);
+}
+
[...]
+#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>
[...]
+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'.
slim_device instance (sdev) is required by audio codec driver to
allocate stream runtime.
+ struct regmap_irq_chip_data *irq_data;
+ struct regulator_bulk_data supplies[WCD934X_MAX_SUPPLY];
+};
+
+#endif /* __WCD934X_H__ */
+