mbox series

[v3,00/11] ASoC: Add support to WCD9340/WCD9341 codec

Message ID 20191029112700.14548-1-srinivas.kandagatla@linaro.org
Headers show
Series ASoC: Add support to WCD9340/WCD9341 codec | expand

Message

Srinivas Kandagatla Oct. 29, 2019, 11:26 a.m. UTC
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

Comments

Stephen Boyd Oct. 30, 2019, 2:50 p.m. UTC | #1
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.
Srinivas Kandagatla Oct. 31, 2019, 10:31 a.m. UTC | #2
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.

>
Linus Walleij Nov. 5, 2019, 1:25 p.m. UTC | #3
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
Srinivas Kandagatla Nov. 5, 2019, 1:27 p.m. UTC | #4
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
Rob Herring Nov. 5, 2019, 6:49 p.m. UTC | #5
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