[alsa-devel] [PATCH v2 02/10] mfd: wcd9335: add support to wcd9335 core
Srinivas Kandagatla
srinivas.kandagatla at linaro.org
Thu Aug 2 11:26:13 CEST 2018
Thanks for the review,
On 02/08/18 09:33, Lee Jones wrote:
> 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 at 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/
Yep.
>
>> + Qualcomm Technologies, Inc. (QTI) multimedia solutions, including
>> + the MSM8996, MSM8976, and MSM8956 chipsets. It has inbuild
>
> s/inbuild/in-built/
Sure!
>
>> + Soundwire controller, interrupt mux. It supports both I2S/I2C and
>> + SLIMbus audio interfaces. This option selects SLIMbus audio interface.
>
> The help should be indented.
Sure!
>
>> 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.
>
Sure will do that in next version.
>> +static const struct regmap_range_cfg wcd9335_ranges[] = {
>> + { .name = "WCD9335",
>
> What is that? New line please.
Opps, will fix it in next version.
>
>> + .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.
>
Yep.
>> + .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/
Yep!
>
>> + 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?
>
No, Will change it to clock in next version.
>> + 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.
Yes, I agree, will fix such instances.
>
>> + 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:
>
This is just making sure that reset line is low before actual the chip
is taken out of reset.
> 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/ ?
>
sure!
>> + 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.
>
Yep.
>> + ret = -EINVAL;
>
> Why not just
>
> return -EINVAL;
>
> Then you can just return 0 and avoid pre-initialising ret.
Agreed!
>
>> + }
>> +
>> + 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?
looks like over done!
>
>> + /* Interface device */
>> + if (slim->e_addr.dev_index == 0)
>
> Is 0 the parent? I think 0 needs defining for clarity.
Sure, Its interface device index, I will try to make this clear by
defining ir.
>
>> + return 0;
>> +
>> + wcd = devm_kzalloc(dev, sizeof(*wcd), GFP_KERNEL);
>> + if (!wcd)
>> + return -ENOMEM;
>> +
>> + wcd->dev = dev;
>
> '\n' here.
>
Yep.
>> + 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
Okay I will keep a close eye on such things before sending next version.
>
>> + 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.
>
Yep!
>> + 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'.
>
Okay.
>> + 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?
>
We are trying to sort of hw setup to test other devices like soundwire
controller, will add these once we are in a position to test them.
> 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'.
>
Agreed!
>> +{
>> + struct device_node *ifd_np;
>
> What's 'ifd'?
>
SLIMbus Interface device.
>> + struct wcd9335 *wcd;
>> + struct device *dev;
>> + int ret;
>> +
>> + /* Interface device */
>
> As previously suggested just define the device ID and drop the
> comment.
>
Yep.
>> + 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.
>
Will cleanup such instances!
>> + 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?
No, Its my bad!
>
>> + return mfd_add_devices(wcd->dev, 0, wcd9335_devices,
>> + ARRAY_SIZE(wcd9335_devices), NULL, 0, NULL);
>
> No error message if it were to fail?
Will fix this in next version.
>
>> +}
>> +
>> +static const struct slim_device_id wcd9335_slim_id[] = {
>> + {0x217, 0x1a0, 0x1, 0x0},
>
> Well that's just horrific. Can't these be defined?
>
Possibly, This is 48bit enumeration address of SLIMbus device, These
correspond to Manufacture ID, Product-ID, and Device-ID and Instance IDs.
>> + {}
>> +};
>> +
>> +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
>
More information about the Alsa-devel
mailing list