Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1616794 > unrolled thread

[PATCH 00/16] Add support for Cirrus Logic CS47L35/L85/L90/L91 codecs

Started byRichard Fitzgerald <rf@opensource.wolfsonmicro.com>
First post2017-04-05 12:20 +0200
Last post2017-04-07 13:20 +0200
Articles 9 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/16] Add support for Cirrus Logic CS47L35/L85/L90/L91 codecs Richard Fitzgerald <rf@opensource.wolfsonmicro.com> - 2017-04-05 12:20 +0200
    [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs Richard Fitzgerald <rf@opensource.wolfsonmicro.com> - 2017-04-05 12:20 +0200
      Re: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs Linus Walleij <linus.walleij@linaro.org> - 2017-04-07 11:20 +0200
        Re: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class  codecs Richard Fitzgerald <rf@opensource.wolfsonmicro.com> - 2017-04-07 12:00 +0200
    Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus  Logic Madera codecs Linus Walleij <linus.walleij@linaro.org> - 2017-04-07 10:30 +0200
      Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus  Logic Madera codecs Linus Walleij <linus.walleij@linaro.org> - 2017-04-07 10:40 +0200
        Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus  Logic Madera codecs Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-04-07 10:50 +0200
          Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus  Logic Madera codecs Linus Walleij <linus.walleij@linaro.org> - 2017-04-07 11:20 +0200
            Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus  Logic Madera codecs Mark Brown <broonie@kernel.org> - 2017-04-07 13:20 +0200

#1616794 — [PATCH 00/16] Add support for Cirrus Logic CS47L35/L85/L90/L91 codecs

FromRichard Fitzgerald <rf@opensource.wolfsonmicro.com>
Date2017-04-05 12:20 +0200
Subject[PATCH 00/16] Add support for Cirrus Logic CS47L35/L85/L90/L91 codecs
Message-ID<tsQcF-1lq-3@gated-at.bofh.it>
The Cirrus Logic CS47L35, CS47L85, CS47L90/91 codecs are complex audio SoC
devices. In addition to the core audio capability they have onboard GPIO,
regulators, DSPs and interrupt controller and a large register map space
accessed over SPI or I2C. This family of codecs is based around common IP
blocks and they are managed by a set of common drivers referred to as "Madera".

Mayuresh Kulkarni (1):
  ASoC: wm_adsp: add support for DSP region lock

Richard Fitzgerald (15):
  mfd: madera: Add register definitions for Cirrus Logic Madera codecs
  mfd: madera: Add common support for Cirrus Logic Madera codecs
  mfd: madera: Register map tables for Cirrus Logic CS47L35
  mfd: madera: Register map tables for Cirrus Logic CS47L85
  mfd: madera: Register map tables for Cirrus Logic CS47L90/91
  regulator: madera-ldo1: LDO1 driver for Cirrus Logic Madera codecs
  regulator: madera-micsupp: Mic supply for Cirrus Logic Madera codecs
  irqchip: Add driver for Cirrus Logic Madera codecs
  pinctrl: madera: Add driver for Cirrus Logic Madera codecs
  gpio: madera: Support Cirrus Logic Madera class codecs
  ASoC: wm_adsp: Add support for ADSP2V2
  ASoC: madera: Add common support for Cirrus Logic Madera codecs
  ASoC: cs47l35: Add codec driver for Cirrus Logic CS47L35
  ASoC: cs47l85: Add codec driver for Cirrus Logic CS47L85
  ASoC: cs47l90: Add codec driver for Cirrus Logic CS47L90

 .../devicetree/bindings/gpio/gpio-madera.txt       |   24 +
 .../interrupt-controller/cirrus,madera.txt         |   31 +
 Documentation/devicetree/bindings/mfd/madera.txt   |   79 +
 .../bindings/pinctrl/cirrus,madera-pinctrl.txt     |  103 +
 .../devicetree/bindings/regulator/madera-ldo1.txt  |   29 +
 .../bindings/regulator/madera-micsupp.txt          |   27 +
 Documentation/devicetree/bindings/sound/madera.txt |   63 +
 MAINTAINERS                                        |   28 +
 drivers/gpio/Kconfig                               |    6 +
 drivers/gpio/Makefile                              |    1 +
 drivers/gpio/gpio-madera.c                         |  173 +
 drivers/irqchip/Kconfig                            |    5 +
 drivers/irqchip/Makefile                           |    1 +
 drivers/irqchip/irq-madera.c                       |  349 +
 drivers/mfd/Kconfig                                |   41 +
 drivers/mfd/Makefile                               |   13 +
 drivers/mfd/cs47l35-tables.c                       | 1688 ++++
 drivers/mfd/cs47l85-tables.c                       | 3169 +++++++
 drivers/mfd/cs47l90-tables.c                       | 2830 +++++++
 drivers/mfd/madera-core.c                          |  689 ++
 drivers/mfd/madera-i2c.c                           |  130 +
 drivers/mfd/madera-spi.c                           |  131 +
 drivers/mfd/madera.h                               |   52 +
 drivers/pinctrl/Kconfig                            |   22 +
 drivers/pinctrl/Makefile                           |    1 +
 drivers/pinctrl/pinctrl-madera.c                   | 1092 +++
 drivers/regulator/Kconfig                          |   16 +
 drivers/regulator/Makefile                         |    2 +
 drivers/regulator/madera-ldo1.c                    |  198 +
 drivers/regulator/madera-micsupp.c                 |  260 +
 include/dt-bindings/sound/madera.h                 |   18 +
 include/linux/irqchip/irq-madera-pdata.h           |   19 +
 include/linux/irqchip/irq-madera.h                 |   96 +
 include/linux/mfd/madera/core.h                    |  175 +
 include/linux/mfd/madera/pdata.h                   |   88 +
 include/linux/mfd/madera/registers.h               | 8832 ++++++++++++++++++++
 include/linux/regulator/madera-ldo1.h              |   24 +
 include/linux/regulator/madera-micsupp.h           |   21 +
 include/sound/madera-pdata.h                       |   70 +
 sound/soc/codecs/Kconfig                           |   23 +
 sound/soc/codecs/Makefile                          |    8 +
 sound/soc/codecs/cs47l35.c                         | 1747 ++++
 sound/soc/codecs/cs47l85.c                         | 2706 ++++++
 sound/soc/codecs/cs47l90.c                         | 2645 ++++++
 sound/soc/codecs/madera.c                          | 4430 ++++++++++
 sound/soc/codecs/madera.h                          |  470 ++
 sound/soc/codecs/wm_adsp.c                         |  324 +-
 sound/soc/codecs/wm_adsp.h                         |   24 +
 48 files changed, 32930 insertions(+), 43 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-madera.txt
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/cirrus,madera.txt
 create mode 100644 Documentation/devicetree/bindings/mfd/madera.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/cirrus,madera-pinctrl.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/madera-ldo1.txt
 create mode 100644 Documentation/devicetree/bindings/regulator/madera-micsupp.txt
 create mode 100644 Documentation/devicetree/bindings/sound/madera.txt
 create mode 100644 drivers/gpio/gpio-madera.c
 create mode 100644 drivers/irqchip/irq-madera.c
 create mode 100644 drivers/mfd/cs47l35-tables.c
 create mode 100644 drivers/mfd/cs47l85-tables.c
 create mode 100644 drivers/mfd/cs47l90-tables.c
 create mode 100644 drivers/mfd/madera-core.c
 create mode 100644 drivers/mfd/madera-i2c.c
 create mode 100644 drivers/mfd/madera-spi.c
 create mode 100644 drivers/mfd/madera.h
 create mode 100644 drivers/pinctrl/pinctrl-madera.c
 create mode 100644 drivers/regulator/madera-ldo1.c
 create mode 100644 drivers/regulator/madera-micsupp.c
 create mode 100644 include/dt-bindings/sound/madera.h
 create mode 100644 include/linux/irqchip/irq-madera-pdata.h
 create mode 100644 include/linux/irqchip/irq-madera.h
 create mode 100644 include/linux/mfd/madera/core.h
 create mode 100644 include/linux/mfd/madera/pdata.h
 create mode 100644 include/linux/mfd/madera/registers.h
 create mode 100644 include/linux/regulator/madera-ldo1.h
 create mode 100644 include/linux/regulator/madera-micsupp.h
 create mode 100644 include/sound/madera-pdata.h
 create mode 100644 sound/soc/codecs/cs47l35.c
 create mode 100644 sound/soc/codecs/cs47l85.c
 create mode 100644 sound/soc/codecs/cs47l90.c
 create mode 100644 sound/soc/codecs/madera.c
 create mode 100644 sound/soc/codecs/madera.h

-- 
1.9.1

[toc] | [next] | [standalone]

#1616796 — [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs

FromRichard Fitzgerald <rf@opensource.wolfsonmicro.com>
Date2017-04-05 12:20 +0200
Subject[PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs
Message-ID<tsQcH-1lq-57@gated-at.bofh.it>
In reply to#1616794
This adds support for the GPIOs on Cirrus Logic Madera class codecs.

Signed-off-by: Nariman Poushin <nariman@opensource.wolfsonmicro.com>
Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 .../devicetree/bindings/gpio/gpio-madera.txt       |  24 +++
 MAINTAINERS                                        |   2 +
 drivers/gpio/Kconfig                               |   6 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-madera.c                         | 173 +++++++++++++++++++++
 5 files changed, 206 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-madera.txt
 create mode 100644 drivers/gpio/gpio-madera.c

diff --git a/Documentation/devicetree/bindings/gpio/gpio-madera.txt b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
new file mode 100644
index 0000000..eb01c6d
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
@@ -0,0 +1,24 @@
+Cirrus Logic Madera class audio codecs gpio driver
+
+This is a subnode of the parent mfd node.
+
+See also the core bindings for the parent MFD driver:
+See Documentation/devicetree/bindings/mfd/madera.txt
+
+Required properties:
+  - compatible : must be "cirrus,madera-gpio"
+  - gpio-controller : Indicates this device is a GPIO controller.
+  - #gpio-cells : Must be 2. The first cell is the pin number. The second cell
+    is reserved for future use and must be zero
+
+Example:
+
+codec: cs47l85@0 {
+	compatible = "cirrus,cs47l85";
+
+	gpio {
+		compatible = "cirrus,madera-gpio";
+		gpio-controller;
+		#gpio-cells = <2>;
+	}
+};
diff --git a/MAINTAINERS b/MAINTAINERS
index 85af1f9..0183692 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3266,6 +3266,7 @@ L:	patches@opensource.wolfsonmicro.com
 T:	git https://github.com/CirrusLogic/linux-drivers.git
 W:	https://github.com/CirrusLogic/linux-drivers/wiki
 S:	Supported
+F:	Documentation/devicetree/bindings/gpio/gpio-madera.txt
 F:	Documentation/devicetree/bindings/interrupt-controller/cirrus,madera.txt
 F:	Documentation/devicetree/bindings/mfd/madera.txt
 F:	Documentation/devicetree/bindings/pinctrl/cirrus,madera-pinctrl.txt
@@ -3273,6 +3274,7 @@ F:	Documentation/devicetree/bindings/regulator/madera*
 F:	include/linux/irqchip/irq-madera*
 F:	include/linux/mfd/madera/*
 F:	include/linux/regulator/madera*
+F:	drivers/gpio/gpio-madera*
 F:	drivers/irqchip/irq-madera*
 F:	drivers/mfd/madera*
 F:	drivers/mfd/cs47l*
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 63ceed2..1386728 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -844,6 +844,12 @@ config GPIO_ARIZONA
 	help
 	  Support for GPIOs on Wolfson Arizona class devices.
 
+config GPIO_MADERA
+	tristate "Cirrus Logic Madera class codecs"
+	depends on MFD_MADERA
+	help
+	  Support for GPIOs on Cirrus Logic Madera class codecs.
+
 config GPIO_CRYSTAL_COVE
 	tristate "GPIO support for Crystal Cove PMIC"
 	depends on (X86 || COMPILE_TEST) && INTEL_SOC_PMIC
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 095598e..d4b6c30 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_GPIO_LPC18XX)	+= gpio-lpc18xx.o
 obj-$(CONFIG_ARCH_LPC32XX)	+= gpio-lpc32xx.o
 obj-$(CONFIG_GPIO_LP873X)	+= gpio-lp873x.o
 obj-$(CONFIG_GPIO_LYNXPOINT)	+= gpio-lynxpoint.o
+obj-$(CONFIG_GPIO_MADERA)	+= gpio-madera.o
 obj-$(CONFIG_GPIO_MAX730X)	+= gpio-max730x.o
 obj-$(CONFIG_GPIO_MAX7300)	+= gpio-max7300.o
 obj-$(CONFIG_GPIO_MAX7301)	+= gpio-max7301.o
diff --git a/drivers/gpio/gpio-madera.c b/drivers/gpio/gpio-madera.c
new file mode 100644
index 0000000..b4fa082
--- /dev/null
+++ b/drivers/gpio/gpio-madera.c
@@ -0,0 +1,173 @@
+/*
+ * gpio-madera.c - GPIO support for Cirrus Logic Madera codecs
+ *
+ * Copyright 2015-2017 Cirrus Logic
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/gpio.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/madera/core.h>
+#include <linux/mfd/madera/pdata.h>
+#include <linux/mfd/madera/registers.h>
+
+struct madera_gpio {
+	struct madera *madera;
+	struct gpio_chip gpio_chip;
+};
+
+static int madera_gpio_direction_in(struct gpio_chip *chip, unsigned int offset)
+{
+	struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
+	struct madera *madera = madera_gpio->madera;
+
+	return regmap_update_bits(madera->regmap,
+				  MADERA_GPIO1_CTRL_2 + (2 * offset),
+				  MADERA_GP1_DIR_MASK, MADERA_GP1_DIR);
+}
+
+static int madera_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
+	struct madera *madera = madera_gpio->madera;
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(madera->regmap,
+			  MADERA_GPIO1_CTRL_1 + (2 * offset), &val);
+	if (ret < 0)
+		return ret;
+
+	if (val & MADERA_GP1_LVL_MASK)
+		return 1;
+	else
+		return 0;
+}
+
+static int madera_gpio_direction_out(struct gpio_chip *chip,
+				     unsigned int offset, int value)
+{
+	struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
+	struct madera *madera = madera_gpio->madera;
+	unsigned int regval;
+	int ret;
+
+	if (value)
+		regval = MADERA_GP1_LVL;
+	else
+		regval = 0;
+
+	ret = regmap_update_bits(madera->regmap,
+				 MADERA_GPIO1_CTRL_2 + (2 * offset),
+				 MADERA_GP1_DIR_MASK, 0);
+	if (ret < 0)
+		return ret;
+
+	return regmap_update_bits(madera->regmap,
+				  MADERA_GPIO1_CTRL_1 + (2 * offset),
+				  MADERA_GP1_LVL_MASK, regval);
+}
+
+static void madera_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			    int value)
+{
+	struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
+	struct madera *madera = madera_gpio->madera;
+	unsigned int regval;
+	int ret;
+
+	if (value)
+		regval = MADERA_GP1_LVL;
+	else
+		regval = 0;
+
+	ret = regmap_update_bits(madera->regmap,
+			   MADERA_GPIO1_CTRL_1 + (2 * offset),
+			   MADERA_GP1_LVL_MASK, regval);
+	if (ret)
+		dev_warn(madera->dev, "Failed to write to 0x%x (%d)\n",
+			 MADERA_GPIO1_CTRL_1 + (2 * offset), ret);
+}
+
+static struct gpio_chip template_chip = {
+	.label			= "madera",
+	.owner			= THIS_MODULE,
+	.direction_input	= madera_gpio_direction_in,
+	.get			= madera_gpio_get,
+	.direction_output	= madera_gpio_direction_out,
+	.set			= madera_gpio_set,
+	.can_sleep		= true,
+};
+
+static int madera_gpio_probe(struct platform_device *pdev)
+{
+	struct madera *madera = dev_get_drvdata(pdev->dev.parent);
+	struct madera_pdata *pdata = dev_get_platdata(madera->dev);
+	struct madera_gpio *madera_gpio;
+	int ret;
+
+	madera_gpio = devm_kzalloc(&pdev->dev, sizeof(*madera_gpio),
+				   GFP_KERNEL);
+	if (!madera_gpio)
+		return -ENOMEM;
+
+	madera_gpio->madera = madera;
+	madera_gpio->gpio_chip = template_chip;
+	madera_gpio->gpio_chip.parent = &pdev->dev;
+
+	if (IS_ENABLED(CONFIG_OF_GPIO))
+		madera_gpio->gpio_chip.of_node = pdev->dev.of_node;
+
+	switch (madera->type) {
+	case CS47L35:
+		madera_gpio->gpio_chip.ngpio = CS47L35_NUM_GPIOS;
+		break;
+	case CS47L85:
+	case WM1840:
+		madera_gpio->gpio_chip.ngpio = CS47L85_NUM_GPIOS;
+		break;
+	case CS47L90:
+	case CS47L91:
+		madera_gpio->gpio_chip.ngpio = CS47L90_NUM_GPIOS;
+		break;
+	default:
+		dev_err(&pdev->dev, "Unknown chip variant %d\n",
+			madera->type);
+		return -EINVAL;
+	}
+
+	if (pdata && pdata->gpio_base)
+		madera_gpio->gpio_chip.base = pdata->gpio_base;
+	else
+		madera_gpio->gpio_chip.base = -1;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &madera_gpio->gpio_chip,
+				     madera_gpio);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver madera_gpio_driver = {
+	.driver.name	= "madera-gpio",
+	.driver.owner	= THIS_MODULE,
+	.probe		= madera_gpio_probe,
+};
+
+module_platform_driver(madera_gpio_driver);
+
+MODULE_DESCRIPTION("GPIO interface for Madera codecs");
+MODULE_AUTHOR("Nariman Poushin <nariman@opensource.wolfsonmicro.com>");
+MODULE_AUTHOR("Richard Fitzgerald <rf@opensource.wolfsonmicro.com>");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:madera-gpio");
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1618619 — Re: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs

FromLinus Walleij <linus.walleij@linaro.org>
Date2017-04-07 11:20 +0200
SubjectRe: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs
Message-ID<ttydI-5d4-15@gated-at.bofh.it>
In reply to#1616796
On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:

> This adds support for the GPIOs on Cirrus Logic Madera class codecs.

A bit terse commit message, could you elaborate a bit on their
specifics?

>  .../devicetree/bindings/gpio/gpio-madera.txt       |  24 +++

Again should probably be a separate patch. Again, I don't care much
as long as the DT people are happy.

> +++ b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
> @@ -0,0 +1,24 @@
> +Cirrus Logic Madera class audio codecs gpio driver
> +
> +This is a subnode of the parent mfd node.
> +
> +See also the core bindings for the parent MFD driver:
> +See Documentation/devicetree/bindings/mfd/madera.txt
> +
> +Required properties:
> +  - compatible : must be "cirrus,madera-gpio"
> +  - gpio-controller : Indicates this device is a GPIO controller.
> +  - #gpio-cells : Must be 2. The first cell is the pin number. The second cell
> +    is reserved for future use and must be zero
> +
> +Example:
> +
> +codec: cs47l85@0 {
> +       compatible = "cirrus,cs47l85";
> +
> +       gpio {
> +               compatible = "cirrus,madera-gpio";
> +               gpio-controller;
> +               #gpio-cells = <2>;
> +       }

Maybe you want to use the gpio-line-names = ; property in the example
to show how nice it is to name the lines?

> +config GPIO_MADERA
> +       tristate "Cirrus Logic Madera class codecs"
> +       depends on MFD_MADERA
> +       help
> +         Support for GPIOs on Cirrus Logic Madera class codecs.

I wonder if you should not depend on the pin controller instead.
It seems closer and also likely to act as a back-end for the
GPIOs.

> +static int madera_gpio_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
> +       struct madera *madera = madera_gpio->madera;
> +       unsigned int val;
> +       int ret;
> +
> +       ret = regmap_read(madera->regmap,
> +                         MADERA_GPIO1_CTRL_1 + (2 * offset), &val);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (val & MADERA_GP1_LVL_MASK)
> +               return 1;
> +       else
> +               return 0;

Just do this:

return !!(val & MADERA_GP1_LVL_MASK);

> +static struct gpio_chip template_chip = {
> +       .label                  = "madera",
> +       .owner                  = THIS_MODULE,
> +       .direction_input        = madera_gpio_direction_in,
> +       .get                    = madera_gpio_get,
> +       .direction_output       = madera_gpio_direction_out,
> +       .set                    = madera_gpio_set,
> +       .can_sleep              = true,
> +};

- Implement .get_direction()

Also consider implementing:

- request/free/set_config looking like this:

.request = gpiochip_generic_request,
.free = gpiochip_generic_free,
.set_config = gpiochip_generic_config,

If you also implement the corresponding
.pin_config_set in struct pinconf_ops and
.gpio_request_enable() and .gpio_disable_free()
in struct pinmux_ops, you get a pin control back-end
that will mux in the pins to GPIO mode if they are wrong
set, and also set up debounce and/or open drain for the
GPIO line using the standard GPIO callbacks with pin
control as a back-end.

If you also specify "strict" in struct pinmux_ops you block
the collisions between users of GPIO and other functions
in the pin control driver.

(Please go back and look at your pin control driver
for this.)

Example driver using pin control as GPIO back-end:
drivers/pinctrl/intel/pinctrl-intel.c

Other than this it looks fine.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1618640 — Re: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs

FromRichard Fitzgerald <rf@opensource.wolfsonmicro.com>
Date2017-04-07 12:00 +0200
SubjectRe: [PATCH 10/16] gpio: madera: Support Cirrus Logic Madera class codecs
Message-ID<ttyQq-5pF-3@gated-at.bofh.it>
In reply to#1618619
On Fri, 2017-04-07 at 11:11 +0200, Linus Walleij wrote:
> On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> <rf@opensource.wolfsonmicro.com> wrote:
> 
> > This adds support for the GPIOs on Cirrus Logic Madera class codecs.
> 
> A bit terse commit message, could you elaborate a bit on their
> specifics?
> 

Sure.

> >  .../devicetree/bindings/gpio/gpio-madera.txt       |  24 +++
> 
> Again should probably be a separate patch. Again, I don't care much
> as long as the DT people are happy.
> 
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-madera.txt
> > @@ -0,0 +1,24 @@
> > +Cirrus Logic Madera class audio codecs gpio driver
> > +
> > +This is a subnode of the parent mfd node.
> > +
> > +See also the core bindings for the parent MFD driver:
> > +See Documentation/devicetree/bindings/mfd/madera.txt
> > +
> > +Required properties:
> > +  - compatible : must be "cirrus,madera-gpio"
> > +  - gpio-controller : Indicates this device is a GPIO controller.
> > +  - #gpio-cells : Must be 2. The first cell is the pin number. The second cell
> > +    is reserved for future use and must be zero
> > +
> > +Example:
> > +
> > +codec: cs47l85@0 {
> > +       compatible = "cirrus,cs47l85";
> > +
> > +       gpio {
> > +               compatible = "cirrus,madera-gpio";
> > +               gpio-controller;
> > +               #gpio-cells = <2>;
> > +       }
> 
> Maybe you want to use the gpio-line-names = ; property in the example
> to show how nice it is to name the lines?
> 

I'll take a look at that.

> > +config GPIO_MADERA
> > +       tristate "Cirrus Logic Madera class codecs"
> > +       depends on MFD_MADERA
> > +       help
> > +         Support for GPIOs on Cirrus Logic Madera class codecs.
> 
> I wonder if you should not depend on the pin controller instead.
> It seems closer and also likely to act as a back-end for the
> GPIOs.
> 
> > +static int madera_gpio_get(struct gpio_chip *chip, unsigned int offset)
> > +{
> > +       struct madera_gpio *madera_gpio = gpiochip_get_data(chip);
> > +       struct madera *madera = madera_gpio->madera;
> > +       unsigned int val;
> > +       int ret;
> > +
> > +       ret = regmap_read(madera->regmap,
> > +                         MADERA_GPIO1_CTRL_1 + (2 * offset), &val);
> > +       if (ret < 0)
> > +               return ret;
> > +
> > +       if (val & MADERA_GP1_LVL_MASK)
> > +               return 1;
> > +       else
> > +               return 0;
> 
> Just do this:
> 
> return !!(val & MADERA_GP1_LVL_MASK);
> 

Ok. Personally I like the clarity of the more verbose version rather
than the !! but I can change it.

> > +static struct gpio_chip template_chip = {
> > +       .label                  = "madera",
> > +       .owner                  = THIS_MODULE,
> > +       .direction_input        = madera_gpio_direction_in,
> > +       .get                    = madera_gpio_get,
> > +       .direction_output       = madera_gpio_direction_out,
> > +       .set                    = madera_gpio_set,
> > +       .can_sleep              = true,
> > +};
> 
> - Implement .get_direction()
> 

Ok


> Also consider implementing:
> 
> - request/free/set_config looking like this:
> 
> .request = gpiochip_generic_request,
> .free = gpiochip_generic_free,
> .set_config = gpiochip_generic_config,
> 
> If you also implement the corresponding
> .pin_config_set in struct pinconf_ops and
> .gpio_request_enable() and .gpio_disable_free()
> in struct pinmux_ops, you get a pin control back-end
> that will mux in the pins to GPIO mode if they are wrong
> set, and also set up debounce and/or open drain for the
> GPIO line using the standard GPIO callbacks with pin
> control as a back-end.
> 
> If you also specify "strict" in struct pinmux_ops you block
> the collisions between users of GPIO and other functions
> in the pin control driver.
> 
> (Please go back and look at your pin control driver
> for this.)
> 

I'll take a look at these things.

> Example driver using pin control as GPIO back-end:
> drivers/pinctrl/intel/pinctrl-intel.c
> 
> Other than this it looks fine.
> 
> Yours,
> Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1618586 — Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

FromLinus Walleij <linus.walleij@linaro.org>
Date2017-04-07 10:30 +0200
SubjectRe: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs
Message-ID<ttxrj-4E7-17@gated-at.bofh.it>
In reply to#1616794
On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
<rf@opensource.wolfsonmicro.com> wrote:

> This patch adds a header file of register definitions for Cirrus
> Logic "Madera" class codecs. These codecs are all based off a common
> set of hardware IP so have a common register map (with a few minor
> device-to-device variations). These are complex devices with a large
> mber of features and so have a correspondingly large register set.
> The registers.h file has been auto-generated from the hardware register
> definitions, stripped down to only registers we need to access from
> the driver.
>
> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>

This:
 include/linux/mfd/madera/registers.h | 8832 ++++++++++++++++++++++++++++++++++

Get included in all subdrivers I suppose?

So you are broadcasting 8800+ lines into every subdriver across the
entire kernel.

Just the time spent in the preprocessor parsing this will affect compilation
time.

Please implement separation of concerns. Move the register definitions into
the drivers, and if they are too large, atleast make a local include file in
sound/soc for the codec parts so the GPIO subdriver does not have
to churn through all this to get its job done.

I know there are other MFD drivers doing this but it's not a good pattern.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1618596 — Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

FromLinus Walleij <linus.walleij@linaro.org>
Date2017-04-07 10:40 +0200
SubjectRe: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs
Message-ID<ttxB0-4Hw-27@gated-at.bofh.it>
In reply to#1618586
On Fri, Apr 7, 2017 at 10:27 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> <rf@opensource.wolfsonmicro.com> wrote:
>
>> This patch adds a header file of register definitions for Cirrus
>> Logic "Madera" class codecs. These codecs are all based off a common
>> set of hardware IP so have a common register map (with a few minor
>> device-to-device variations). These are complex devices with a large
>> mber of features and so have a correspondingly large register set.
>> The registers.h file has been auto-generated from the hardware register
>> definitions, stripped down to only registers we need to access from
>> the driver.
>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
>
> This:
>  include/linux/mfd/madera/registers.h | 8832 ++++++++++++++++++++++++++++++++++
>
> Get included in all subdrivers I suppose?
>
> So you are broadcasting 8800+ lines into every subdriver across the
> entire kernel.
>
> Just the time spent in the preprocessor parsing this will affect compilation
> time.

Or maybe this is a necessary sacrifice to get the regmap cache
centralized in MFD. I don't know. I feel stupid.

I guess I should focus on "my" subsystems...

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1618607 — Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

FromCharles Keepax <ckeepax@opensource.wolfsonmicro.com>
Date2017-04-07 10:50 +0200
SubjectRe: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs
Message-ID<ttxKG-4On-25@gated-at.bofh.it>
In reply to#1618596
On Fri, Apr 07, 2017 at 10:30:12AM +0200, Linus Walleij wrote:
> On Fri, Apr 7, 2017 at 10:27 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
> > <rf@opensource.wolfsonmicro.com> wrote:
> >
> >> This patch adds a header file of register definitions for Cirrus
> >> Logic "Madera" class codecs. These codecs are all based off a common
> >> set of hardware IP so have a common register map (with a few minor
> >> device-to-device variations). These are complex devices with a large
> >> mber of features and so have a correspondingly large register set.
> >> The registers.h file has been auto-generated from the hardware register
> >> definitions, stripped down to only registers we need to access from
> >> the driver.
> >>
> >> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
> >
> > This:
> >  include/linux/mfd/madera/registers.h | 8832 ++++++++++++++++++++++++++++++++++
> >
> > Get included in all subdrivers I suppose?
> >
> > So you are broadcasting 8800+ lines into every subdriver across the
> > entire kernel.
> >
> > Just the time spent in the preprocessor parsing this will affect compilation
> > time.
> 
> Or maybe this is a necessary sacrifice to get the regmap cache
> centralized in MFD. I don't know. I feel stupid.
> 
> I guess I should focus on "my" subsystems...
> 

This only gets included in files that are part of this driver, it
shouldn't affect compilation time for anyone not building the
madera driver and even then it should only affect compilation
times for the 10 or so C files that make up the driver. Also I
don't really see any other way to specify the registers for the
device.

Thanks,
Charles

[toc] | [prev] | [next] | [standalone]

#1618617 — Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

FromLinus Walleij <linus.walleij@linaro.org>
Date2017-04-07 11:20 +0200
SubjectRe: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs
Message-ID<ttydH-5d4-7@gated-at.bofh.it>
In reply to#1618607
On Fri, Apr 7, 2017 at 10:48 AM, Charles Keepax
<ckeepax@opensource.wolfsonmicro.com> wrote:
> On Fri, Apr 07, 2017 at 10:30:12AM +0200, Linus Walleij wrote:
>> On Fri, Apr 7, 2017 at 10:27 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> > On Wed, Apr 5, 2017 at 12:07 PM, Richard Fitzgerald
>> > <rf@opensource.wolfsonmicro.com> wrote:
>> >
>> >> This patch adds a header file of register definitions for Cirrus
>> >> Logic "Madera" class codecs. These codecs are all based off a common
>> >> set of hardware IP so have a common register map (with a few minor
>> >> device-to-device variations). These are complex devices with a large
>> >> mber of features and so have a correspondingly large register set.
>> >> The registers.h file has been auto-generated from the hardware register
>> >> definitions, stripped down to only registers we need to access from
>> >> the driver.
>> >>
>> >> Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com>
>> >
>> > This:
>> >  include/linux/mfd/madera/registers.h | 8832 ++++++++++++++++++++++++++++++++++
>> >
>> > Get included in all subdrivers I suppose?
>> >
>> > So you are broadcasting 8800+ lines into every subdriver across the
>> > entire kernel.
>> >
>> > Just the time spent in the preprocessor parsing this will affect compilation
>> > time.
>>
>> Or maybe this is a necessary sacrifice to get the regmap cache
>> centralized in MFD. I don't know. I feel stupid.
>>
>> I guess I should focus on "my" subsystems...
>>
>
> This only gets included in files that are part of this driver, it
> shouldn't affect compilation time for anyone not building the
> madera driver and even then it should only affect compilation
> times for the 10 or so C files that make up the driver. Also I
> don't really see any other way to specify the registers for the
> device.

No when using regmap cache this seems necessary.
I was just wrong.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1618680 — Re: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs

FromMark Brown <broonie@kernel.org>
Date2017-04-07 13:20 +0200
SubjectRe: [PATCH 01/16] mfd: madera: Add register definitions for Cirrus Logic Madera codecs
Message-ID<ttA5P-6lG-1@gated-at.bofh.it>
In reply to#1618617

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 07, 2017 at 11:12:53AM +0200, Linus Walleij wrote:
> On Fri, Apr 7, 2017 at 10:48 AM, Charles Keepax
> > On Fri, Apr 07, 2017 at 10:30:12AM +0200, Linus Walleij wrote:

> >> > Get included in all subdrivers I suppose?

> >> > So you are broadcasting 8800+ lines into every subdriver across the
> >> > entire kernel.

> >> > Just the time spent in the preprocessor parsing this will affect compilation
> >> > time.

> >> Or maybe this is a necessary sacrifice to get the regmap cache
> >> centralized in MFD. I don't know. I feel stupid.

> >> I guess I should focus on "my" subsystems...

> > This only gets included in files that are part of this driver, it
> > shouldn't affect compilation time for anyone not building the
> > madera driver and even then it should only affect compilation
> > times for the 10 or so C files that make up the driver. Also I
> > don't really see any other way to specify the registers for the
> > device.

> No when using regmap cache this seems necessary.
> I was just wrong.

Right.  The other thing to bear in mind with things like this is that
often if it's the chip vendor they've got mechanisms to generate the
register definitions directly from the chip design so if we take the
complete thing with minimal edits it's a great way of ensuring we've got
information about the chip in the upstream kernel to support future
development.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web