| Message ID | 20191029112700.14548-1-srinivas.kandagatla@linaro.org |
|---|---|
| Headers | show |
| Series | ASoC: Add support to WCD9340/WCD9341 codec | expand |
Quoting Srinivas Kandagatla (2019-10-29 04:26:58) > From: Yeleswarapu Nagaradhesh <nagaradh@codeaurora.org> > > This patch adds support to wcd934x pinctrl block found in > WCD9340/WC9341 Audio codecs. > > [Srini: multiple cleanups to the code] This goes after the author signoff and before yours. Can you add more details too? > Signed-off-by: Yeleswarapu Nagaradhesh <nagaradh@codeaurora.org> > Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> > --- > drivers/pinctrl/qcom/Kconfig | 7 + > drivers/pinctrl/qcom/Makefile | 1 + > drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c | 365 ++++++++++++++++++++ > 3 files changed, 373 insertions(+) > create mode 100644 drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c > > diff --git a/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c b/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c > new file mode 100644 > index 000000000000..1aff88d0bcb3 > --- /dev/null > +++ b/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c > @@ -0,0 +1,365 @@ > +// SPDX-License-Identifier: GPL-2.0 > +// Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. > +// Copyright (c) 2019, Linaro Limited > + > +#include <linux/module.h> > +#include <linux/gpio.h> > +#include <linux/interrupt.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/of_device.h> > +#include <linux/of_gpio.h> > + > +#include "../core.h" > +#include "../pinctrl-utils.h" > + > +#define WCD_REG_DIR_CTL_OFFSET 0x42 > +#define WCD_REG_VAL_CTL_OFFSET 0x43 > +#define WCD_GPIO_PULL_UP 1 > +#define WCD_GPIO_PULL_DOWN 2 > +#define WCD_GPIO_BIAS_DISABLE 3 > +#define WCD_GPIO_STRING_LEN 20 > +#define WCD934X_NPINS 5 > + > +/** > + * struct wcd_gpio_pad - keep current GPIO settings > + * @offset: offset of gpio. > + * @is_valid: Set to false, when GPIO in high Z state. > + * @value: value of a pin > + * @output_enabled: Set to true if GPIO is output and false if it is input > + * @pullup: Constant current which flow through GPIO output buffer. > + * @strength: Drive strength of a pin > + */ > +struct wcd_gpio_pad { > + u16 offset; > + bool is_valid; > + bool value; > + bool output_enabled; > + unsigned int pullup; > + unsigned int strength; > +}; > + > +struct wcd_gpio_priv { > + struct device *dev; > + struct regmap *map; > + struct pinctrl_dev *ctrl; > + struct gpio_chip chip; > +}; > + > +static int wcd_gpio_read(struct wcd_gpio_priv *priv_data, > + struct wcd_gpio_pad *pad, unsigned int addr) > +{ > + unsigned int val; > + int ret; > + > + ret = regmap_read(priv_data->map, addr, &val); > + if (ret < 0) > + dev_err(priv_data->dev, "%s: read 0x%x failed\n", > + __func__, addr); > + else > + ret = (val >> pad->offset); > + > + return ret; > +} > + > +static int wcd_gpio_write(struct wcd_gpio_priv *priv_data, > + struct wcd_gpio_pad *pad, unsigned int addr, > + unsigned int val) > +{ > + int ret; > + > + ret = regmap_update_bits(priv_data->map, addr, (1 << pad->offset), > + val << pad->offset); > + if (ret < 0) > + dev_err(priv_data->dev, "write 0x%x failed\n", addr); Is there value in these error messages? Also, use %#x to get '0x'. > + > + return ret; > +} [...] > + > +static int wcd_pinctrl_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct pinctrl_pin_desc *pindesc; > + struct pinctrl_desc *pctrldesc; > + struct wcd_gpio_pad *pad, *pads; > + struct wcd_gpio_priv *priv_data; > + u32 npins = WCD934X_NPINS; > + char **name; > + int i; > + > + priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL); > + if (!priv_data) > + return -ENOMEM; > + > + priv_data->dev = dev; > + priv_data->map = dev_get_regmap(dev->parent, NULL); > + if (!priv_data->map) { > + dev_err(dev, "%s: failed to get regmap\n", __func__); > + return -EINVAL; > + } > + > + pindesc = devm_kcalloc(dev, npins, sizeof(*pindesc), GFP_KERNEL); > + if (!pindesc) > + return -ENOMEM; > + > + pads = devm_kcalloc(dev, npins, sizeof(*pads), GFP_KERNEL); > + if (!pads) > + return -ENOMEM; Is it possible to put the pad struct around the pindesc struct? It's sort of sad that we have to allocate a chunk of memory twice for the pindesc and the pads when we could either use container_of() on the pindesc or just point the pindesc driver data member to the container structure for the qcom specific bits. > + > + pctrldesc = devm_kzalloc(dev, sizeof(*pctrldesc), GFP_KERNEL); > + if (!pctrldesc) > + return -ENOMEM; > + > + pctrldesc->pctlops = &wcd_pinctrl_ops; > + pctrldesc->confops = &wcd_pinconf_ops; > + pctrldesc->owner = THIS_MODULE; > + pctrldesc->name = dev_name(dev); > + pctrldesc->pins = pindesc; > + pctrldesc->npins = npins; > + > + name = devm_kcalloc(dev, npins, sizeof(char *), GFP_KERNEL); > + if (!name) > + return -ENOMEM; > + > + for (i = 0; i < npins; i++, pindesc++) { > + name[i] = devm_kzalloc(dev, sizeof(char) * WCD_GPIO_STRING_LEN, > + GFP_KERNEL); > + if (!name[i]) > + return -ENOMEM; > + > + pad = &pads[i]; > + pindesc->drv_data = pad; > + pindesc->number = i; > + snprintf(name[i], (WCD_GPIO_STRING_LEN - 1), "gpio%d", (i+1)); > + pindesc->name = name[i]; Why not use devm_kasprintf()? The 'name' array is also unnecessary? > + pad->offset = i; > + pad->is_valid = true; > + } > + > + priv_data->chip = wcd_gpio_chip; > + priv_data->chip.parent = dev; > + priv_data->chip.base = -1; > + priv_data->chip.ngpio = npins; > + priv_data->chip.label = dev_name(dev); > + priv_data->chip.of_gpio_n_cells = 2; > + priv_data->chip.can_sleep = false; > + platform_set_drvdata(pdev, priv_data); > + > + priv_data->ctrl = devm_pinctrl_register(dev, pctrldesc, priv_data); > + if (IS_ERR(priv_data->ctrl)) { > + dev_err(dev, "%s: failed to register to pinctrl\n", __func__); > + return PTR_ERR(priv_data->ctrl); > + } > + > + return gpiochip_add_data(&priv_data->chip, priv_data); WHy not use devm_gpiochip_add_data()? > +} > + > +static int wcd_pinctrl_remove(struct platform_device *pdev) > +{ > + struct wcd_gpio_priv *priv_data = platform_get_drvdata(pdev); > + > + gpiochip_remove(&priv_data->chip); > + > + return 0; And drop this function? > +} > + > +static const struct of_device_id wcd_pinctrl_of_match[] = { > + { .compatible = "qcom,wcd9340-pinctrl" }, > + { .compatible = "qcom,wcd9341-pinctrl" }, > + { }, Nitpick: Drop the comma on the sentinel. > +}; > + > +MODULE_DEVICE_TABLE(of, wcd_pinctrl_of_match); Nitpick: Drop the newline between device table and match table.
Thanks Stephen for reviewing this patch. On 30/10/2019 14:50, Stephen Boyd wrote: > Quoting Srinivas Kandagatla (2019-10-29 04:26:58) >> From: Yeleswarapu Nagaradhesh <nagaradh@codeaurora.org> >> >> This patch adds support to wcd934x pinctrl block found in >> WCD9340/WC9341 Audio codecs. >> >> [Srini: multiple cleanups to the code] > > This goes after the author signoff and before yours. Can you add more > details too? I agree, will fix this in next spin. > >> Signed-off-by: Yeleswarapu Nagaradhesh <nagaradh@codeaurora.org> >> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org> >> --- >> drivers/pinctrl/qcom/Kconfig | 7 + >> drivers/pinctrl/qcom/Makefile | 1 + >> drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c | 365 ++++++++++++++++++++ >> 3 files changed, 373 insertions(+) >> create mode 100644 drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c >> >> diff --git a/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c b/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c >> new file mode 100644 >> index 000000000000..1aff88d0bcb3 >> --- /dev/null >> +++ b/drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c >> @@ -0,0 +1,365 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +// Copyright (c) 2016-2017, The Linux Foundation. All rights reserved. >> +// Copyright (c) 2019, Linaro Limited >> + >> +#include <linux/module.h> >> +#include <linux/gpio.h> >> +#include <linux/interrupt.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/of_gpio.h> >> + >> +#include "../core.h" >> +#include "../pinctrl-utils.h" >> + >> +#define WCD_REG_DIR_CTL_OFFSET 0x42 >> +#define WCD_REG_VAL_CTL_OFFSET 0x43 >> +#define WCD_GPIO_PULL_UP 1 >> +#define WCD_GPIO_PULL_DOWN 2 >> +#define WCD_GPIO_BIAS_DISABLE 3 >> +#define WCD_GPIO_STRING_LEN 20 >> +#define WCD934X_NPINS 5 >> + >> +/** >> + * struct wcd_gpio_pad - keep current GPIO settings >> + * @offset: offset of gpio. >> + * @is_valid: Set to false, when GPIO in high Z state. >> + * @value: value of a pin >> + * @output_enabled: Set to true if GPIO is output and false if it is input >> + * @pullup: Constant current which flow through GPIO output buffer. >> + * @strength: Drive strength of a pin >> + */ >> +struct wcd_gpio_pad { >> + u16 offset; >> + bool is_valid; >> + bool value; >> + bool output_enabled; >> + unsigned int pullup; >> + unsigned int strength; >> +}; >> + >> +struct wcd_gpio_priv { >> + struct device *dev; >> + struct regmap *map; >> + struct pinctrl_dev *ctrl; >> + struct gpio_chip chip; >> +}; >> + >> +static int wcd_gpio_read(struct wcd_gpio_priv *priv_data, >> + struct wcd_gpio_pad *pad, unsigned int addr) >> +{ >> + unsigned int val; >> + int ret; >> + >> + ret = regmap_read(priv_data->map, addr, &val); >> + if (ret < 0) >> + dev_err(priv_data->dev, "%s: read 0x%x failed\n", >> + __func__, addr); >> + else >> + ret = (val >> pad->offset); >> + >> + return ret; >> +} >> + >> +static int wcd_gpio_write(struct wcd_gpio_priv *priv_data, >> + struct wcd_gpio_pad *pad, unsigned int addr, >> + unsigned int val) >> +{ >> + int ret; >> + >> + ret = regmap_update_bits(priv_data->map, addr, (1 << pad->offset), >> + val << pad->offset); >> + if (ret < 0) >> + dev_err(priv_data->dev, "write 0x%x failed\n", addr); > > Is there value in these error messages? Also, use %#x to get '0x'. I can add ret in the err message. I did not knew about "%#x".. nice, I will use this in future! > >> + >> + return ret; >> +} > [...] >> + >> +static int wcd_pinctrl_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct pinctrl_pin_desc *pindesc; >> + struct pinctrl_desc *pctrldesc; >> + struct wcd_gpio_pad *pad, *pads; >> + struct wcd_gpio_priv *priv_data; >> + u32 npins = WCD934X_NPINS; >> + char **name; >> + int i; >> + >> + priv_data = devm_kzalloc(dev, sizeof(*priv_data), GFP_KERNEL); >> + if (!priv_data) >> + return -ENOMEM; >> + >> + priv_data->dev = dev; >> + priv_data->map = dev_get_regmap(dev->parent, NULL); >> + if (!priv_data->map) { >> + dev_err(dev, "%s: failed to get regmap\n", __func__); >> + return -EINVAL; >> + } >> + >> + pindesc = devm_kcalloc(dev, npins, sizeof(*pindesc), GFP_KERNEL); >> + if (!pindesc) >> + return -ENOMEM; >> + >> + pads = devm_kcalloc(dev, npins, sizeof(*pads), GFP_KERNEL); >> + if (!pads) >> + return -ENOMEM; > > Is it possible to put the pad struct around the pindesc struct? It's > sort of sad that we have to allocate a chunk of memory twice for the > pindesc and the pads when we could either use container_of() on the > pindesc or just point the pindesc driver data member to the container > structure for the qcom specific bits. > I will give that a go in next version! >> + >> + pctrldesc = devm_kzalloc(dev, sizeof(*pctrldesc), GFP_KERNEL); >> + if (!pctrldesc) >> + return -ENOMEM; >> + >> + pctrldesc->pctlops = &wcd_pinctrl_ops; >> + pctrldesc->confops = &wcd_pinconf_ops; >> + pctrldesc->owner = THIS_MODULE; >> + pctrldesc->name = dev_name(dev); >> + pctrldesc->pins = pindesc; >> + pctrldesc->npins = npins; >> + >> + name = devm_kcalloc(dev, npins, sizeof(char *), GFP_KERNEL); >> + if (!name) >> + return -ENOMEM; >> + >> + for (i = 0; i < npins; i++, pindesc++) { >> + name[i] = devm_kzalloc(dev, sizeof(char) * WCD_GPIO_STRING_LEN, >> + GFP_KERNEL); >> + if (!name[i]) >> + return -ENOMEM; >> + >> + pad = &pads[i]; >> + pindesc->drv_data = pad; >> + pindesc->number = i; >> + snprintf(name[i], (WCD_GPIO_STRING_LEN - 1), "gpio%d", (i+1)); >> + pindesc->name = name[i]; > > Why not use devm_kasprintf()? The 'name' array is also unnecessary? Am not sure why its not used her, but I can do that change in next version. > >> + pad->offset = i; >> + pad->is_valid = true; >> + } >> + >> + priv_data->chip = wcd_gpio_chip; >> + priv_data->chip.parent = dev; >> + priv_data->chip.base = -1; >> + priv_data->chip.ngpio = npins; >> + priv_data->chip.label = dev_name(dev); >> + priv_data->chip.of_gpio_n_cells = 2; >> + priv_data->chip.can_sleep = false; >> + platform_set_drvdata(pdev, priv_data); >> + >> + priv_data->ctrl = devm_pinctrl_register(dev, pctrldesc, priv_data); >> + if (IS_ERR(priv_data->ctrl)) { >> + dev_err(dev, "%s: failed to register to pinctrl\n", __func__); >> + return PTR_ERR(priv_data->ctrl); >> + } >> + >> + return gpiochip_add_data(&priv_data->chip, priv_data); > > WHy not use devm_gpiochip_add_data()? Good idea, will do that in next spin. > >> +} >> + >> +static int wcd_pinctrl_remove(struct platform_device *pdev) >> +{ >> + struct wcd_gpio_priv *priv_data = platform_get_drvdata(pdev); >> + >> + gpiochip_remove(&priv_data->chip); >> + >> + return 0; > > And drop this function? > >> +} >> + >> +static const struct of_device_id wcd_pinctrl_of_match[] = { >> + { .compatible = "qcom,wcd9340-pinctrl" }, >> + { .compatible = "qcom,wcd9341-pinctrl" }, >> + { }, > > Nitpick: Drop the comma on the sentinel. > >> +}; >> + >> +MODULE_DEVICE_TABLE(of, wcd_pinctrl_of_match); > > Nitpick: Drop the newline between device table and match table. >
On Mon, Nov 4, 2019 at 10:35 AM Srinivas Kandagatla <srinivas.kandagatla@linaro.org> wrote: > This controller just has Output enable bits, No pin control properties. > > As you suggested I can move this to drivers/gpio in next version. OK perfect, thanks! NB: this probably also affects the compatible-string which contains "pinctrl*" right? Yours, Linus Walleij
On 05/11/2019 13:25, Linus Walleij wrote: > On Mon, Nov 4, 2019 at 10:35 AM Srinivas Kandagatla > <srinivas.kandagatla@linaro.org> wrote: > >> This controller just has Output enable bits, No pin control properties. >> >> As you suggested I can move this to drivers/gpio in next version. > OK perfect, thanks! > > NB: this probably also affects the compatible-string which contains > "pinctrl*" right? Yes, I will suffix it with "-gpio" instead. thanks, srini
On Tue, Nov 05, 2019 at 01:27:45PM +0000, Srinivas Kandagatla wrote: > > > On 05/11/2019 13:25, Linus Walleij wrote: > > On Mon, Nov 4, 2019 at 10:35 AM Srinivas Kandagatla > > <srinivas.kandagatla@linaro.org> wrote: > > > > > This controller just has Output enable bits, No pin control properties. > > > > > > As you suggested I can move this to drivers/gpio in next version. > > OK perfect, thanks! > > > > NB: this probably also affects the compatible-string which contains > > "pinctrl*" right? > Yes, I will suffix it with "-gpio" instead. Not a discussion we should be having because you should name this after what's in the chip documentation not the OS subsystem it happens to land in. Rob
This patchset adds support to Qualcomm WCD9340/WCD9341 Codec which is a standalone Hi-Fi audio codec IC. This codec supports both I2S/I2C and SLIMbus audio interfaces. On slimbus interface it supports two data lanes; 16 Tx ports and 8 Rx ports. It has Five DACs and seven dedicated interpolators, Multibutton headset control (MBHC), Active noise cancellation, Sidetone paths, MAD (mic activity detection) and codec processing engine. It supports Class-H differential earpiece out and stereo single ended headphones out. This codec also has integrated SoundWire controller. Patchset for this is already sent for review at https://patchwork.kernel.org/cover/11185769/ This patchset has been tested on SDM845 based DragonBoard DB845c and Lenovo Yoga C630 laptop with WSA881x smart speaker amplifiers via soundwire and 4 DMICs. Pin Controller patch does not have any link dependency, it can go by its own. Most of the code in this driver is rework of Qualcomm downstream drivers used in Andriod. Credits to Banajit Goswami and Patrick Lai's Team. If anyone is interested to try, here are working set of patches on top of rc3. https://git.linaro.org/people/srinivas.kandagatla/linux.git/log/?h=audio/v5.4-rc5-YOGA-C630 alsa ucm files: https://git.linaro.org/people/srinivas.kandagatla/alsa-lib.git/log/?h=DB845c Thanks, srini Changes since v2: - Updated mfd driver as suggested by Lee. - Updated bindings as suggested by Rob. - Addressed various comments by Cezary Rojewski - Cleaned up code a bit. Srinivas Kandagatla (10): ASoC: dt-bindings: add dt bindings for WCD9340/WCD9341 audio codec mfd: wcd934x: add support to wcd9340/wcd9341 codec ASoC: wcd934x: add support to wcd9340/wcd9341 codec ASoC: wcd934x: add basic controls ASoC: wcd934x: add playback dapm widgets ASoC: wcd934x: add capture dapm widgets ASoC: wcd934x: add audio routings dt-bindings: pinctrl: qcom-wcd934x: Add bindings for gpio ASoC: qcom: dt-bindings: Add compatible for DB845c and Lenovo Yoga ASoC: qcom: sdm845: add support to DB845c and Lenovo Yoga Yeleswarapu Nagaradhesh (1): pinctrl: qcom-wcd934x: Add support to wcd934x pinctrl driver. .../pinctrl/qcom,wcd934x-pinctrl.yaml | 52 + .../devicetree/bindings/sound/qcom,sdm845.txt | 5 +- .../bindings/sound/qcom,wcd934x.yaml | 162 + drivers/mfd/Kconfig | 12 + drivers/mfd/Makefile | 1 + drivers/mfd/wcd934x.c | 306 + drivers/pinctrl/qcom/Kconfig | 7 + drivers/pinctrl/qcom/Makefile | 1 + drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c | 365 ++ include/linux/mfd/wcd934x/registers.h | 529 ++ include/linux/mfd/wcd934x/wcd934x.h | 31 + sound/soc/codecs/Kconfig | 10 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/wcd934x.c | 5084 +++++++++++++++++ sound/soc/qcom/sdm845.c | 86 +- 15 files changed, 6651 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,wcd934x-pinctrl.yaml create mode 100644 Documentation/devicetree/bindings/sound/qcom,wcd934x.yaml create mode 100644 drivers/mfd/wcd934x.c create mode 100644 drivers/pinctrl/qcom/pinctrl-wcd934x-gpio.c create mode 100644 include/linux/mfd/wcd934x/registers.h create mode 100644 include/linux/mfd/wcd934x/wcd934x.h create mode 100644 sound/soc/codecs/wcd934x.c -- 2.21.0