linux-hwmon.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/16] Add support for Kontron sl28cpld
@ 2020-04-02 20:36 Michael Walle
  2020-04-02 20:36 ` [PATCH v2 01/16] include/linux/ioport.h: add helper to define REG resource constructs Michael Walle
                   ` (15 more replies)
  0 siblings, 16 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

The Kontron sl28cpld is a board management chip providing gpio, pwm, fan
monitoring and an interrupt controller. For now this controller is used on
the Kontron SMARC-sAL28 board. But because of its flexible nature, it
might also be used on other boards in the future. The individual blocks
(like gpio, pwm, etc) are kept intentionally small. The MFD core driver
then instantiates different (or multiple of the same) blocks. It also
provides the register layout so it might be updated in the future without a
device tree change; and support other boards with a different layout or
functionalities.

See also [1] for more information.

This is my first take of a MFD driver. I don't know whether the subsystem
maintainers should only be CCed on the patches which affect the subsystem
or on all patches for this series. I've chosen the latter so you can get a
more complete picture.

[1] https://lore.kernel.org/linux-devicetree/0e3e8204ab992d75aa07fc36af7e4ab2@walle.cc/

Changes since v1:
 - use of_match_table in all drivers, needed for automatic module loading,
   when using OF_MFD_CELL()
 - add new gpio-regmap.c which adds a generic regmap gpio_chip implemention
 - new patch for reqmap_irq, so we can reuse its implementation
 - remove almost any code from gpio-sl28cpld.c, instead use gpio-regmap and
   regmap-irq
 - change the handling of the mfd core vs device tree nodes; add a new
   property "of_reg" to the mfd_cell struct which, when set, is matched to
   the unit-address of the device tree nodes.
 - fix sl28cpld watchdog when it is not initialized by the bootloader.
   Explicitly set the operation mode.
 - also add support for kontron,assert-wdt-timeout-pin in sl28cpld-wdt.

As suggested by Bartosz Golaszewski:
 - define registers as hex
 - make gpio enum uppercase
 - move parent regmap check before memory allocation
 - use device_property_read_bool() instead of the of_ version
 - mention the gpio flavors in the bindings documentation

As suggested by Guenter Roeck:
 - cleanup #includes and sort them
 - use devm_watchdog_register_device()
 - use watchdog_stop_on_reboot()
 - provide a Documentation/hwmon/sl28cpld.rst
 - cleaned up the weird tristate->bool and I2C=y issue. Instead mention
   that the MFD driver is bool because of the following intc patch
 - removed the SL28CPLD_IRQ typo

As suggested by Rob Herring:
 - combine all dt bindings docs into one patch
 - change the node name for all gpio flavors to "gpio"
 - removed the interrupts-extended rule
 - cleaned up the unit-address space, see above

Michael Walle (16):
  include/linux/ioport.h: add helper to define REG resource constructs
  mfd: mfd-core: Don't overwrite the dma_mask of the child device
  mfd: mfd-core: match device tree node against reg property
  regmap-irq: make it possible to add irq_chip do a specific device node
  dt-bindings: mfd: Add bindings for sl28cpld
  mfd: Add support for Kontron sl28cpld management controller
  irqchip: add sl28cpld interrupt controller support
  watchdog: add support for sl28cpld watchdog
  pwm: add support for sl28cpld PWM controller
  gpio: add a reusable generic gpio_chip using regmap
  gpio: add support for the sl28cpld GPIO controller
  hwmon: add support for the sl28cpld hardware monitoring controller
  arm64: dts: freescale: sl28: enable sl28cpld
  arm64: dts: freescale: sl28: map GPIOs to input events
  arm64: dts: freescale: sl28: enable LED support
  arm64: dts: freescale: sl28: enable fan support

 .../bindings/gpio/kontron,sl28cpld-gpio.yaml  |  51 +++
 .../hwmon/kontron,sl28cpld-hwmon.yaml         |  27 ++
 .../bindings/mfd/kontron,sl28cpld.yaml        | 162 +++++++++
 .../bindings/pwm/kontron,sl28cpld-pwm.yaml    |  35 ++
 .../watchdog/kontron,sl28cpld-wdt.yaml        |  35 ++
 Documentation/hwmon/sl28cpld.rst              |  36 ++
 .../fsl-ls1028a-kontron-kbox-a-230-ls.dts     |  14 +
 .../fsl-ls1028a-kontron-sl28-var3-ads2.dts    |   9 +
 .../freescale/fsl-ls1028a-kontron-sl28.dts    | 124 +++++++
 drivers/base/regmap/regmap-irq.c              |  84 ++++-
 drivers/gpio/Kconfig                          |  15 +
 drivers/gpio/Makefile                         |   2 +
 drivers/gpio/gpio-regmap.c                    | 321 ++++++++++++++++++
 drivers/gpio/gpio-sl28cpld.c                  | 187 ++++++++++
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/sl28cpld-hwmon.c                | 152 +++++++++
 drivers/irqchip/Kconfig                       |   3 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-sl28cpld.c                |  99 ++++++
 drivers/mfd/Kconfig                           |  21 ++
 drivers/mfd/Makefile                          |   2 +
 drivers/mfd/mfd-core.c                        |  31 +-
 drivers/mfd/sl28cpld.c                        | 154 +++++++++
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sl28cpld.c                    | 204 +++++++++++
 drivers/watchdog/Kconfig                      |  11 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/sl28cpld_wdt.c               | 242 +++++++++++++
 include/linux/gpio-regmap.h                   |  88 +++++
 include/linux/ioport.h                        |   5 +
 include/linux/mfd/core.h                      |  26 +-
 include/linux/regmap.h                        |  10 +
 34 files changed, 2142 insertions(+), 32 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/hwmon/kontron,sl28cpld-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/kontron,sl28cpld-pwm.yaml
 create mode 100644 Documentation/devicetree/bindings/watchdog/kontron,sl28cpld-wdt.yaml
 create mode 100644 Documentation/hwmon/sl28cpld.rst
 create mode 100644 drivers/gpio/gpio-regmap.c
 create mode 100644 drivers/gpio/gpio-sl28cpld.c
 create mode 100644 drivers/hwmon/sl28cpld-hwmon.c
 create mode 100644 drivers/irqchip/irq-sl28cpld.c
 create mode 100644 drivers/mfd/sl28cpld.c
 create mode 100644 drivers/pwm/pwm-sl28cpld.c
 create mode 100644 drivers/watchdog/sl28cpld_wdt.c
 create mode 100644 include/linux/gpio-regmap.h

-- 
2.20.1


^ permalink raw reply	[flat|nested] 38+ messages in thread

* [PATCH v2 01/16] include/linux/ioport.h: add helper to define REG resource constructs
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 02/16] mfd: mfd-core: Don't overwrite the dma_mask of the child device Michael Walle
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

Similar to the existing helpers, add one for IORESOURCE_REG which comes
in handy for most common resource declarations.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 include/linux/ioport.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index a9b9170b5dd2..cdcceeec0f86 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -165,6 +165,11 @@ enum {
 #define DEFINE_RES_MEM(_start, _size)					\
 	DEFINE_RES_MEM_NAMED((_start), (_size), NULL)
 
+#define DEFINE_RES_REG_NAMED(_start, _size, _name)			\
+	DEFINE_RES_NAMED((_start), (_size), (_name), IORESOURCE_REG)
+#define DEFINE_RES_REG(_start, _size)					\
+	DEFINE_RES_REG_NAMED((_start), (_size), NULL)
+
 #define DEFINE_RES_IRQ_NAMED(_irq, _name)				\
 	DEFINE_RES_NAMED((_irq), 1, (_name), IORESOURCE_IRQ)
 #define DEFINE_RES_IRQ(_irq)						\
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 02/16] mfd: mfd-core: Don't overwrite the dma_mask of the child device
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
  2020-04-02 20:36 ` [PATCH v2 01/16] include/linux/ioport.h: add helper to define REG resource constructs Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 03/16] mfd: mfd-core: match device tree node against reg property Michael Walle
                   ` (13 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle, Robin Murphy

Commit cdfee5623290 ("driver core: initialize a default DMA mask for
platform device") initialize the DMA of a platform device. But if the
parent doesn't have a dma_mask set, for example if it's an I2C device,
the dma_mask of the child platform device will be set to zero again.
Which leads to many "DMA mask not set" warnings, if the MFD cell has the
of_compatible property set.

[    1.877937] sl28cpld-pwm sl28cpld-pwm: DMA mask not set
[    1.883282] sl28cpld-pwm sl28cpld-pwm.0: DMA mask not set
[    1.888795] sl28cpld-gpio sl28cpld-gpio: DMA mask not set

Thus don't overwrite the dma_mask of the children. Instead set the
dma_mask of the platform device.

Signed-off-by: Michael Walle <michael@walle.cc>
Suggested-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/mfd/mfd-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index f5a73af60dd4..e735565969b3 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -138,7 +138,7 @@ static int mfd_add_device(struct device *parent, int id,
 
 	pdev->dev.parent = parent;
 	pdev->dev.type = &mfd_dev_type;
-	pdev->dev.dma_mask = parent->dma_mask;
+	pdev->platform_dma_mask = parent->dma_mask ? *parent->dma_mask : 0;
 	pdev->dev.dma_parms = parent->dma_parms;
 	pdev->dev.coherent_dma_mask = parent->coherent_dma_mask;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 03/16] mfd: mfd-core: match device tree node against reg property
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
  2020-04-02 20:36 ` [PATCH v2 01/16] include/linux/ioport.h: add helper to define REG resource constructs Michael Walle
  2020-04-02 20:36 ` [PATCH v2 02/16] mfd: mfd-core: Don't overwrite the dma_mask of the child device Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Michael Walle
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

There might be multiple children with the device tree compatible, for
example if a MFD has multiple instances of the same function. In this
case only the first is matched and the other children get a wrong
of_node reference.
Add a new option to match also against the unit address of the child
node. Additonally, a new helper OF_MFD_CELL_REG is added.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mfd/mfd-core.c   | 29 ++++++++++++++++++++---------
 include/linux/mfd/core.h | 26 ++++++++++++++++++++------
 2 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
index e735565969b3..4ecb376338f7 100644
--- a/drivers/mfd/mfd-core.c
+++ b/drivers/mfd/mfd-core.c
@@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int id,
 	struct device_node *np = NULL;
 	int ret = -ENOMEM;
 	int platform_id;
+	u32 of_reg;
 	int r;
 
 	if (id == PLATFORM_DEVID_AUTO)
@@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent, int id,
 
 	if (parent->of_node && cell->of_compatible) {
 		for_each_child_of_node(parent->of_node, np) {
-			if (of_device_is_compatible(np, cell->of_compatible)) {
-				if (!of_device_is_available(np)) {
-					/* Ignore disabled devices error free */
-					ret = 0;
-					goto fail_alias;
-				}
-				pdev->dev.of_node = np;
-				pdev->dev.fwnode = &np->fwnode;
-				break;
+			if (!of_device_is_compatible(np, cell->of_compatible))
+				continue;
+
+			/* also match the unit address if set */
+			if (cell->of_reg & MFD_OF_REG_VALID) {
+				if (of_property_read_u32(np, "reg", &of_reg))
+					continue;
+				if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg)
+					continue;
 			}
+
+			if (!of_device_is_available(np)) {
+				/* Ignore disabled devices error free */
+				ret = 0;
+				goto fail_alias;
+			}
+
+			pdev->dev.of_node = np;
+			pdev->dev.fwnode = &np->fwnode;
+			break;
 		}
 	}
 
diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
index d01d1299e49d..c2c0ad6b14f3 100644
--- a/include/linux/mfd/core.h
+++ b/include/linux/mfd/core.h
@@ -13,8 +13,11 @@
 #include <linux/platform_device.h>
 
 #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
+#define MFD_OF_REG_VALID	BIT(31)
+#define MFD_OF_REG_MASK		GENMASK(30, 0)
 
-#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, _match)\
+#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
+		     _of_reg, _match)					\
 	{								\
 		.name = (_name),					\
 		.resources = (_res),					\
@@ -22,24 +25,32 @@
 		.platform_data = (_pdata),				\
 		.pdata_size = (_pdsize),				\
 		.of_compatible = (_compat),				\
+		.of_reg = (_of_reg),					\
 		.acpi_match = (_match),					\
 		.id = (_id),						\
 	}
 
+#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat,	\
+			_of_reg)					\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
+		     ((_of_reg) | MFD_OF_REG_VALID), NULL)		\
+
 #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat)		\
-	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL)	\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,	\
+		     0, NULL)						\
 
 #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match)	\
-	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match)	\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0,	\
+		     _match)						\
 
 #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id)		\
-	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL)	\
+	MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \
 
 #define MFD_CELL_RES(_name, _res)					\
-	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL)		\
+	MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL)		\
 
 #define MFD_CELL_NAME(_name)						\
-	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL)		\
+	MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL)		\
 
 struct irq_domain;
 struct property_entry;
@@ -78,6 +89,9 @@ struct mfd_cell {
 	 */
 	const char		*of_compatible;
 
+	/* matching the reg property if set */
+	unsigned int		of_reg;
+
 	/* Matches ACPI */
 	const struct mfd_cell_acpi_match	*acpi_match;
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (2 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 03/16] mfd: mfd-core: match device tree node against reg property Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-14 15:37   ` Applied "regmap-irq: make it possible to add irq_chip do a specific device node" to the regmap tree Mark Brown
  2020-04-14 17:12   ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Mark Brown
  2020-04-02 20:36 ` [PATCH v2 05/16] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
                   ` (11 subsequent siblings)
  15 siblings, 2 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

Add a new function regmap_add_irq_chip_np() with its corresponding
devm_regmap_add_irq_chip_np() variant. Sometimes one want to register
the IRQ domain on a different device node that the one of the regmap
node. For example when using a MFD where there are different interrupt
controllers and particularly for the generic regmap gpio_chip/irq_chip
driver. In this case it is not desireable to have the IRQ domain on
the parent node.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/base/regmap/regmap-irq.c | 84 ++++++++++++++++++++++++++------
 include/linux/regmap.h           | 10 ++++
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 3d64c9331a82..4340e1d268b6 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -541,8 +541,9 @@ static const struct irq_domain_ops regmap_domain_ops = {
 };
 
 /**
- * regmap_add_irq_chip() - Use standard regmap IRQ controller handling
+ * regmap_add_irq_chip_np() - Use standard regmap IRQ controller handling
  *
+ * @np: The device_node where the IRQ domain should be added to.
  * @map: The regmap for the device.
  * @irq: The IRQ the device uses to signal interrupts.
  * @irq_flags: The IRQF_ flags to use for the primary interrupt.
@@ -556,9 +557,10 @@ static const struct irq_domain_ops regmap_domain_ops = {
  * register cache.  The chip driver is responsible for restoring the
  * register values used by the IRQ controller over suspend and resume.
  */
-int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
-			int irq_base, const struct regmap_irq_chip *chip,
-			struct regmap_irq_chip_data **data)
+int regmap_add_irq_chip_np(struct device_node *np, struct regmap *map, int irq,
+			   int irq_flags, int irq_base,
+			   const struct regmap_irq_chip *chip,
+			   struct regmap_irq_chip_data **data)
 {
 	struct regmap_irq_chip_data *d;
 	int i;
@@ -769,12 +771,10 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	}
 
 	if (irq_base)
-		d->domain = irq_domain_add_legacy(map->dev->of_node,
-						  chip->num_irqs, irq_base, 0,
-						  &regmap_domain_ops, d);
+		d->domain = irq_domain_add_legacy(np, chip->num_irqs, irq_base,
+						  0, &regmap_domain_ops, d);
 	else
-		d->domain = irq_domain_add_linear(map->dev->of_node,
-						  chip->num_irqs,
+		d->domain = irq_domain_add_linear(np, chip->num_irqs,
 						  &regmap_domain_ops, d);
 	if (!d->domain) {
 		dev_err(map->dev, "Failed to create IRQ domain\n");
@@ -808,6 +808,30 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	kfree(d);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(regmap_add_irq_chip_np);
+
+/**
+ * regmap_add_irq_chip() - Use standard regmap IRQ controller handling
+ *
+ * @map: The regmap for the device.
+ * @irq: The IRQ the device uses to signal interrupts.
+ * @irq_flags: The IRQF_ flags to use for the primary interrupt.
+ * @irq_base: Allocate at specific IRQ number if irq_base > 0.
+ * @chip: Configuration for the interrupt controller.
+ * @data: Runtime data structure for the controller, allocated on success.
+ *
+ * Returns 0 on success or an errno on failure.
+ *
+ * This is the same as regmap_add_irq_chip_np, except that the device
+ * node of the regmap is used.
+ */
+int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
+			int irq_base, const struct regmap_irq_chip *chip,
+			struct regmap_irq_chip_data **data)
+{
+	return regmap_add_irq_chip_np(map->dev->of_node, map, irq, irq_flags,
+				      irq_base, chip, data);
+}
 EXPORT_SYMBOL_GPL(regmap_add_irq_chip);
 
 /**
@@ -875,9 +899,10 @@ static int devm_regmap_irq_chip_match(struct device *dev, void *res, void *data)
 }
 
 /**
- * devm_regmap_add_irq_chip() - Resource manager regmap_add_irq_chip()
+ * devm_regmap_add_irq_chip_np() - Resource manager regmap_add_irq_chip_np()
  *
  * @dev: The device pointer on which irq_chip belongs to.
+ * @np: The device_node where the IRQ domain should be added to.
  * @map: The regmap for the device.
  * @irq: The IRQ the device uses to signal interrupts
  * @irq_flags: The IRQF_ flags to use for the primary interrupt.
@@ -890,10 +915,11 @@ static int devm_regmap_irq_chip_match(struct device *dev, void *res, void *data)
  * The &regmap_irq_chip_data will be automatically released when the device is
  * unbound.
  */
-int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
-			     int irq_flags, int irq_base,
-			     const struct regmap_irq_chip *chip,
-			     struct regmap_irq_chip_data **data)
+int devm_regmap_add_irq_chip_np(struct device *dev, struct device_node *np,
+				struct regmap *map, int irq, int irq_flags,
+				int irq_base,
+				const struct regmap_irq_chip *chip,
+				struct regmap_irq_chip_data **data)
 {
 	struct regmap_irq_chip_data **ptr, *d;
 	int ret;
@@ -903,8 +929,8 @@ int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = regmap_add_irq_chip(map, irq, irq_flags, irq_base,
-				  chip, &d);
+	ret = regmap_add_irq_chip_np(np, map, irq, irq_flags, irq_base,
+				     chip, &d);
 	if (ret < 0) {
 		devres_free(ptr);
 		return ret;
@@ -915,6 +941,32 @@ int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 	*data = d;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(devm_regmap_add_irq_chip_np);
+
+/**
+ * devm_regmap_add_irq_chip() - Resource manager regmap_add_irq_chip()
+ *
+ * @dev: The device pointer on which irq_chip belongs to.
+ * @map: The regmap for the device.
+ * @irq: The IRQ the device uses to signal interrupts
+ * @irq_flags: The IRQF_ flags to use for the primary interrupt.
+ * @irq_base: Allocate at specific IRQ number if irq_base > 0.
+ * @chip: Configuration for the interrupt controller.
+ * @data: Runtime data structure for the controller, allocated on success
+ *
+ * Returns 0 on success or an errno on failure.
+ *
+ * The &regmap_irq_chip_data will be automatically released when the device is
+ * unbound.
+ */
+int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
+			     int irq_flags, int irq_base,
+			     const struct regmap_irq_chip *chip,
+			     struct regmap_irq_chip_data **data)
+{
+	return devm_regmap_add_irq_chip_np(dev, map->dev->of_node, map, irq,
+					   irq_flags, irq_base, chip, data);
+}
 EXPORT_SYMBOL_GPL(devm_regmap_add_irq_chip);
 
 /**
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 40b07168fd8e..ae5034b2d7c3 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -21,6 +21,7 @@
 struct module;
 struct clk;
 struct device;
+struct device_node;
 struct i2c_client;
 struct i3c_device;
 struct irq_domain;
@@ -1310,12 +1311,21 @@ struct regmap_irq_chip_data;
 int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 			int irq_base, const struct regmap_irq_chip *chip,
 			struct regmap_irq_chip_data **data);
+int regmap_add_irq_chip_np(struct device_node *np, struct regmap *map, int irq,
+			   int irq_flags, int irq_base,
+			   const struct regmap_irq_chip *chip,
+			   struct regmap_irq_chip_data **data);
 void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *data);
 
 int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 			     int irq_flags, int irq_base,
 			     const struct regmap_irq_chip *chip,
 			     struct regmap_irq_chip_data **data);
+int devm_regmap_add_irq_chip_np(struct device *dev, struct device_node *np,
+				struct regmap *map, int irq, int irq_flags,
+				int irq_base,
+				const struct regmap_irq_chip *chip,
+				struct regmap_irq_chip_data **data);
 void devm_regmap_del_irq_chip(struct device *dev, int irq,
 			      struct regmap_irq_chip_data *data);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 05/16] dt-bindings: mfd: Add bindings for sl28cpld
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (3 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 06/16] mfd: Add support for Kontron sl28cpld management controller Michael Walle
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This adds device tree bindings for the board management controller found
on the Kontron SMARC-sAL28 board.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../bindings/gpio/kontron,sl28cpld-gpio.yaml  |  51 ++++++
 .../hwmon/kontron,sl28cpld-hwmon.yaml         |  27 +++
 .../bindings/mfd/kontron,sl28cpld.yaml        | 162 ++++++++++++++++++
 .../bindings/pwm/kontron,sl28cpld-pwm.yaml    |  35 ++++
 .../watchdog/kontron,sl28cpld-wdt.yaml        |  35 ++++
 5 files changed, 310 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/hwmon/kontron,sl28cpld-hwmon.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml
 create mode 100644 Documentation/devicetree/bindings/pwm/kontron,sl28cpld-pwm.yaml
 create mode 100644 Documentation/devicetree/bindings/watchdog/kontron,sl28cpld-wdt.yaml

diff --git a/Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml b/Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml
new file mode 100644
index 000000000000..291ca116743d
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/kontron,sl28cpld-gpio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO driver for the sl28cpld board management controller
+
+maintainers:
+  - Michael Walle <michael@walle.cc>
+
+description: |
+  This module is part of the sl28cpld multi-function device. For more
+  details see Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml.
+
+  There are three flavors of the GPIO controller, one full featured
+  input/output with interrupt support (kontron,sl28cpld-gpio), one
+  output-only (kontron,sl28-gpo) and one input-only (kontron,sl28-gpi).
+
+  Each controller supports 8 GPIO lines.
+
+properties:
+  compatible:
+    enum:
+      - kontron,sl28cpld-gpio
+      - kontron,sl28cpld-gpi
+      - kontron,sl28cpld-gpo
+
+  reg:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupt-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+  gpio-controller: true
+
+  gpio-line-names:
+      minItems: 1
+      maxItems: 8
+
+required:
+  - compatible
+  - "#gpio-cells"
+  - gpio-controller
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/hwmon/kontron,sl28cpld-hwmon.yaml b/Documentation/devicetree/bindings/hwmon/kontron,sl28cpld-hwmon.yaml
new file mode 100644
index 000000000000..1cebd61c6c32
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/kontron,sl28cpld-hwmon.yaml
@@ -0,0 +1,27 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/kontron,sl28cpld-hwmon.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Hardware monitoring driver for the sl28cpld board management controller
+
+maintainers:
+  - Michael Walle <michael@walle.cc>
+
+description: |
+  This module is part of the sl28cpld multi-function device. For more
+  details see Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml.
+
+properties:
+  compatible:
+    enum:
+      - kontron,sl28cpld-fan
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml b/Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml
new file mode 100644
index 000000000000..e155e3035513
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml
@@ -0,0 +1,162 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/kontron,sl28cpld.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Kontron's sl28cpld board management controller
+
+maintainers:
+  - Michael Walle <michael@walle.cc>
+
+description: |
+  The board management controller may contain different IP blocks like
+  watchdog, fan monitoring, PWM controller, interrupt controller and a
+  GPIO controller.
+
+  Currently the MFD exports the following functions at the given
+  unit-addresses.
+
+  === =========== =======================
+   ua name        compatible
+  === =========== =======================
+   0  watchdog    kontron,sl28cpld-wdt
+   1  hwmon       kontron,sl28cpld-fan
+   2  pwm         kontron,sl28cpld-pwm
+   3  pwm         kontron,sl28cpld-pwm
+   4  gpio        kontron,sl28cpld-gpio
+   5  gpio        kontron,sl28cpld-gpio
+   6  gpio        kontron,sl28cpld-gpo
+   7  gpio        kontron,sl28cpld-gpi
+  === =========== =======================
+
+  The MFD driver will match the child nodes according to the unit-address
+  (eg. the reg property) and the compatible string.
+
+properties:
+  compatible:
+    const: kontron,sl28cpld
+
+  reg:
+    description:
+      I2C device address.
+    maxItems: 1
+
+  "#address-cells":
+    const: 1
+
+  "#size-cells":
+    const: 0
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupts:
+    maxItems: 1
+
+  interrupt-controller: true
+
+patternProperties:
+  "^gpio(@[0-9]+)?$":
+    $ref: ../gpio/kontron,sl28cpld-gpio.yaml
+
+  "^hwmon(@[0-9]+)?$":
+    $ref: ../hwmon/kontron,sl28cpld-hwmon.yaml
+
+  "^pwm(@[0-9]+)?$":
+    $ref: ../pwm/kontron,sl28cpld-pwm.yaml
+
+  "^watchdog(@[0-9]+)?$":
+    $ref: ../watchdog/kontron,sl28cpld-wdt.yaml
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+  - compatible
+  - reg
+  - interrupts
+  - "#interrupt-cells"
+  - interrupt-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        sl28cpld@4a {
+            #address-cells = <1>;
+            #size-cells = <0>;
+            compatible = "kontron,sl28cpld";
+            reg = <0x4a>;
+            interrupts-extended = <&gpio2 6 IRQ_TYPE_EDGE_FALLING>;
+
+            #interrupt-cells = <2>;
+            interrupt-controller;
+
+            watchdog@0 {
+                compatible = "kontron,sl28cpld-wdt";
+                reg = <0>;
+                kontron,assert-wdt-timeout-pin;
+            };
+
+            hwmon@1 {
+                compatible = "kontron,sl28cpld-fan";
+                reg = <1>;
+            };
+
+            pwm@2 {
+                compatible = "kontron,sl28cpld-pwm";
+                reg = <2>;
+                #pwm-cells = <2>;
+            };
+
+            pwm@3 {
+                compatible = "kontron,sl28cpld-pwm";
+                reg = <3>;
+                #pwm-cells = <2>;
+            };
+
+            gpio@4 {
+                compatible = "kontron,sl28cpld-gpio";
+                reg = <4>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+
+                interrupt-controller;
+                #interrupt-cells = <2>;
+            };
+
+            gpio@5 {
+                compatible = "kontron,sl28cpld-gpio";
+                reg = <5>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+
+                interrupt-controller;
+                #interrupt-cells = <2>;
+            };
+
+            gpio@6 {
+                compatible = "kontron,sl28cpld-gpo";
+                reg = <6>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+                gpio-line-names = "a", "b", "c";
+            };
+
+            gpio@7 {
+                compatible = "kontron,sl28cpld-gpi";
+                reg = <7>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+        };
+    };
diff --git a/Documentation/devicetree/bindings/pwm/kontron,sl28cpld-pwm.yaml b/Documentation/devicetree/bindings/pwm/kontron,sl28cpld-pwm.yaml
new file mode 100644
index 000000000000..02fe88c30233
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/kontron,sl28cpld-pwm.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pwm/kontron,sl28cpld-pwm.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: PWM driver for the sl28cpld board management controller
+
+maintainers:
+  - Michael Walle <michael@walle.cc>
+
+description: |
+  This module is part of the sl28cpld multi-function device. For more
+  details see Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml.
+
+  The controller supports one PWM channel and supports only four distinct
+  frequencies (250Hz, 500Hz, 1kHz, 2kHz).
+
+allOf:
+  - $ref: pwm.yaml#
+
+properties:
+  compatible:
+    const: kontron,sl28cpld-pwm
+
+  reg:
+    maxItems: 1
+
+  "#pwm-cells":
+    const: 2
+
+required:
+  - compatible
+
+additionalProperties: false
diff --git a/Documentation/devicetree/bindings/watchdog/kontron,sl28cpld-wdt.yaml b/Documentation/devicetree/bindings/watchdog/kontron,sl28cpld-wdt.yaml
new file mode 100644
index 000000000000..dd6559f2973a
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/kontron,sl28cpld-wdt.yaml
@@ -0,0 +1,35 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/kontron,sl28cpld-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Watchdog driver for the sl28cpld board management controller
+
+maintainers:
+  - Michael Walle <michael@walle.cc>
+
+description: |
+  This module is part of the sl28cpld multi-function device. For more
+  details see Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml.
+
+allOf:
+  - $ref: watchdog.yaml#
+
+properties:
+  compatible:
+    const: kontron,sl28cpld-wdt
+
+  reg:
+    maxItems: 1
+
+  kontron,assert-wdt-timeout-pin:
+    description: The SMARC standard defines a WDT_TIME_OUT# pin. If this
+      property is set, this output will be pulsed when the watchdog bites
+      and the system resets.
+    type: boolean
+
+required:
+  - compatible
+
+additionalProperties: false
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 06/16] mfd: Add support for Kontron sl28cpld management controller
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (4 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 05/16] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 07/16] irqchip: add sl28cpld interrupt controller support Michael Walle
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This patch adds core support for the board management controller found
on the SMARC-sAL28 board. It consists of the following functions:
 - watchdog
 - GPIO controller
 - PWM controller
 - fan sensor
 - interrupt controller

At the moment, this controller is used on the Kontron SMARC-sAL28 board.

Please note that the MFD driver is defined as bool in the Kconfig
because the next patch will add interrupt support.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/mfd/Kconfig    |  19 +++++
 drivers/mfd/Makefile   |   2 +
 drivers/mfd/sl28cpld.c | 153 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 174 insertions(+)
 create mode 100644 drivers/mfd/sl28cpld.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3c547ed575e6..7c6761161f7a 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2059,5 +2059,24 @@ config SGI_MFD_IOC3
 	  If you have an SGI Origin, Octane, or a PCI IOC3 card,
 	  then say Y. Otherwise say N.
 
+config MFD_SL28CPLD
+	bool "Kontron sl28 core driver"
+	depends on I2C=y
+	depends on OF
+	select REGMAP_I2C
+	select MFD_CORE
+	help
+	  This option enables support for the board management controller
+	  found on the Kontron sl28 CPLD. You have to select individual
+	  functions, such as watchdog, GPIO, etc, under the corresponding menus
+	  in order to enable them.
+
+	  Currently supported boards are:
+
+		Kontron SMARC-sAL28
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called sl28cpld.
+
 endmenu
 endif
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index f935d10cbf0f..9bc38863b9c7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -259,3 +259,5 @@ obj-$(CONFIG_MFD_ROHM_BD718XX)	+= rohm-bd718x7.o
 obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
+
+obj-$(CONFIG_MFD_SL28CPLD)	+= sl28cpld.o
diff --git a/drivers/mfd/sl28cpld.c b/drivers/mfd/sl28cpld.c
new file mode 100644
index 000000000000..1e5860cc7ffc
--- /dev/null
+++ b/drivers/mfd/sl28cpld.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * MFD core for the sl28cpld.
+ *
+ * Copyright 2019 Kontron Europe GmbH
+ */
+
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+
+#define SL28CPLD_VERSION	0x03
+#define SL28CPLD_WATCHDOG_BASE	0x04
+#define SL28CPLD_HWMON_FAN_BASE	0x0b
+#define SL28CPLD_PWM0_BASE	0x0c
+#define SL28CPLD_PWM1_BASE	0x0e
+#define SL28CPLD_GPIO0_BASE	0x10
+#define SL28CPLD_GPIO1_BASE	0x15
+#define SL28CPLD_GPO_BASE	0x1a
+#define SL28CPLD_GPI_BASE	0x1b
+#define SL28CPLD_INTC_BASE	0x1c
+
+/* all subdevices share the same IRQ */
+#define SL28CPLD_IRQ 0
+
+#define SL28CPLD_MIN_REQ_VERSION 14
+
+struct sl28cpld {
+	struct device *dev;
+	struct regmap *regmap;
+};
+
+static const struct regmap_config sl28cpld_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+};
+
+static struct resource sl28cpld_watchdog_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_WATCHDOG_BASE, 1),
+};
+
+static struct resource sl28cpld_hwmon_fan_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_HWMON_FAN_BASE, 1),
+};
+
+static struct resource sl28cpld_pwm0_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_PWM0_BASE, 1),
+};
+
+static struct resource sl28cpld_pwm1_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_PWM1_BASE, 1),
+};
+
+static struct resource sl28cpld_gpio0_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_GPIO0_BASE, 1),
+	DEFINE_RES_IRQ(SL28CPLD_IRQ),
+};
+
+static struct resource sl28cpld_gpio1_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_GPIO1_BASE, 1),
+	DEFINE_RES_IRQ(SL28CPLD_IRQ),
+};
+
+static struct resource sl28cpld_gpo_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_GPO_BASE, 1),
+};
+
+static struct resource sl28cpld_gpi_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_GPI_BASE, 1),
+};
+
+static struct resource sl28cpld_intc_resources[] = {
+	DEFINE_RES_REG(SL28CPLD_INTC_BASE, 1),
+	DEFINE_RES_IRQ(SL28CPLD_IRQ),
+};
+
+static const struct mfd_cell sl28cpld_devs[] = {
+	OF_MFD_CELL_REG("sl28cpld-wdt", sl28cpld_watchdog_resources,
+			NULL, 0, 0, "kontron,sl28cpld-wdt", 0),
+	OF_MFD_CELL_REG("sl28cpld-fan", sl28cpld_hwmon_fan_resources,
+			NULL, 0, 0, "kontron,sl28cpld-fan", 1),
+	OF_MFD_CELL_REG("sl28cpld-pwm", sl28cpld_pwm0_resources,
+			NULL, 0, 0, "kontron,sl28cpld-pwm", 2),
+	OF_MFD_CELL_REG("sl28cpld-pwm", sl28cpld_pwm1_resources,
+			NULL, 0, 1, "kontron,sl28cpld-pwm", 3),
+	OF_MFD_CELL_REG("sl28cpld-gpio", sl28cpld_gpio0_resources,
+			NULL, 0, 0, "kontron,sl28cpld-gpio", 4),
+	OF_MFD_CELL_REG("sl28cpld-gpio", sl28cpld_gpio1_resources,
+			NULL, 0, 1, "kontron,sl28cpld-gpio", 5),
+	OF_MFD_CELL_REG("sl28cpld-gpo", sl28cpld_gpo_resources,
+			NULL, 0, 0, "kontron,sl28cpld-gpo", 6),
+	OF_MFD_CELL_REG("sl28cpld-gpi", sl28cpld_gpi_resources,
+			NULL, 0, 0, "kontron,sl28cpld-gpi", 7),
+	MFD_CELL_RES("sl28cpld-intc", sl28cpld_intc_resources),
+};
+
+static int sl28cpld_probe(struct i2c_client *i2c)
+{
+	struct sl28cpld *sl28cpld;
+	struct device *dev = &i2c->dev;
+	unsigned int cpld_version;
+	int ret;
+
+	sl28cpld = devm_kzalloc(dev, sizeof(*sl28cpld), GFP_KERNEL);
+	if (!sl28cpld)
+		return -ENOMEM;
+
+	sl28cpld->regmap = devm_regmap_init_i2c(i2c, &sl28cpld_regmap_config);
+	if (IS_ERR(sl28cpld->regmap))
+		return PTR_ERR(sl28cpld->regmap);
+
+	ret = regmap_read(sl28cpld->regmap, SL28CPLD_VERSION, &cpld_version);
+	if (ret)
+		return ret;
+
+	if (cpld_version < SL28CPLD_MIN_REQ_VERSION) {
+		dev_err(dev, "unsupported CPLD version %d\n", cpld_version);
+		return -ENODEV;
+	}
+
+	sl28cpld->dev = dev;
+	i2c_set_clientdata(i2c, sl28cpld);
+
+	dev_info(dev, "successfully probed. CPLD version %d\n", cpld_version);
+
+	return devm_mfd_add_devices(dev, -1, sl28cpld_devs,
+				    ARRAY_SIZE(sl28cpld_devs), NULL,
+				    i2c->irq, NULL);
+}
+
+static const struct of_device_id sl28cpld_of_match[] = {
+	{ .compatible = "kontron,sl28cpld", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_of_match);
+
+static struct i2c_driver sl28cpld_driver = {
+	.probe_new = sl28cpld_probe,
+	.driver = {
+		.name = "sl28cpld",
+		.of_match_table = of_match_ptr(sl28cpld_of_match),
+	},
+};
+module_i2c_driver(sl28cpld_driver);
+
+MODULE_DESCRIPTION("sl28cpld MFD Core Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 07/16] irqchip: add sl28cpld interrupt controller support
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (5 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 06/16] mfd: Add support for Kontron sl28cpld management controller Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog Michael Walle
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This patch adds support for the interrupt controller inside the sl28
CPLD management controller.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/irqchip/Kconfig        |  3 ++
 drivers/irqchip/Makefile       |  1 +
 drivers/irqchip/irq-sl28cpld.c | 97 ++++++++++++++++++++++++++++++++++
 drivers/mfd/Kconfig            |  2 +
 4 files changed, 103 insertions(+)
 create mode 100644 drivers/irqchip/irq-sl28cpld.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index a85aada04a64..234db932e7cf 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -246,6 +246,9 @@ config RENESAS_RZA1_IRQC
 	  Enable support for the Renesas RZ/A1 Interrupt Controller, to use up
 	  to 8 external interrupts with configurable sense select.
 
+config SL28CPLD_INTC
+	bool
+
 config ST_IRQCHIP
 	bool
 	select REGMAP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 37bbe39bf909..e3c6b94f7b0a 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -107,3 +107,4 @@ obj-$(CONFIG_TI_SCI_INTR_IRQCHIP)	+= irq-ti-sci-intr.o
 obj-$(CONFIG_TI_SCI_INTA_IRQCHIP)	+= irq-ti-sci-inta.o
 obj-$(CONFIG_LOONGSON_LIOINTC)		+= irq-loongson-liointc.o
 obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
+obj-$(CONFIG_SL28CPLD_INTC)		+= irq-sl28cpld.o
diff --git a/drivers/irqchip/irq-sl28cpld.c b/drivers/irqchip/irq-sl28cpld.c
new file mode 100644
index 000000000000..88de71d32b09
--- /dev/null
+++ b/drivers/irqchip/irq-sl28cpld.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld interrupt controller driver.
+ *
+ * Copyright 2019 Kontron Europe GmbH
+ */
+
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define INTC_IE 0x00
+#define INTC_IP 0x01
+
+static const struct regmap_irq sl28cpld_irqs[] = {
+	REGMAP_IRQ_REG_LINE(0, 8),
+	REGMAP_IRQ_REG_LINE(1, 8),
+	REGMAP_IRQ_REG_LINE(2, 8),
+	REGMAP_IRQ_REG_LINE(3, 8),
+	REGMAP_IRQ_REG_LINE(4, 8),
+	REGMAP_IRQ_REG_LINE(5, 8),
+	REGMAP_IRQ_REG_LINE(6, 8),
+	REGMAP_IRQ_REG_LINE(7, 8),
+};
+
+struct sl28cpld_intc {
+	struct regmap *regmap;
+	struct regmap_irq_chip chip;
+	struct regmap_irq_chip_data *irq_data;
+};
+
+static int sl28cpld_intc_probe(struct platform_device *pdev)
+{
+	struct sl28cpld_intc *irqchip;
+	struct resource *res;
+	unsigned int irq;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	irqchip = devm_kzalloc(&pdev->dev, sizeof(*irqchip), GFP_KERNEL);
+	if (!irqchip)
+		return -ENOMEM;
+
+	irqchip->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!irqchip->regmap)
+		return -ENODEV;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (!res)
+		return -EINVAL;
+
+	irqchip->chip.name = "sl28cpld-intc";
+	irqchip->chip.irqs = sl28cpld_irqs;
+	irqchip->chip.num_irqs = ARRAY_SIZE(sl28cpld_irqs);
+	irqchip->chip.num_regs = 1;
+	irqchip->chip.status_base = res->start + INTC_IP;
+	irqchip->chip.mask_base = res->start + INTC_IE;
+	irqchip->chip.mask_invert = true,
+	irqchip->chip.ack_base = res->start + INTC_IP;
+
+	ret = devm_regmap_add_irq_chip(&pdev->dev, irqchip->regmap, irq,
+				       IRQF_SHARED | IRQF_ONESHOT, 0,
+				       &irqchip->chip, &irqchip->irq_data);
+	if (ret)
+		return ret;
+	dev_info(&pdev->dev, "registered IRQ %d\n", irq);
+
+	return 0;
+}
+
+static const struct platform_device_id sl28cpld_intc_id_table[] = {
+	{ "sl28cpld-intc" },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, sl28cpld_intc_id_table);
+
+static struct platform_driver sl28cpld_intc_driver = {
+	.probe	= sl28cpld_intc_probe,
+	.id_table = sl28cpld_intc_id_table,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	}
+};
+module_platform_driver(sl28cpld_intc_driver);
+
+MODULE_DESCRIPTION("sl28cpld Interrupt Controller Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 7c6761161f7a..4f741d640705 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -2064,6 +2064,8 @@ config MFD_SL28CPLD
 	depends on I2C=y
 	depends on OF
 	select REGMAP_I2C
+	select REGMAP_IRQ
+	select SL28CPLD_INTC
 	select MFD_CORE
 	help
 	  This option enables support for the board management controller
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (6 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 07/16] irqchip: add sl28cpld interrupt controller support Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-03  6:25   ` Guenter Roeck
  2020-04-02 20:36 ` [PATCH v2 09/16] pwm: add support for sl28cpld PWM controller Michael Walle
                   ` (7 subsequent siblings)
  15 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This adds support for the watchdog of the sl28cpld board management
controller. This is part of a multi-function device driver.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/watchdog/Kconfig        |  11 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/sl28cpld_wdt.c | 242 ++++++++++++++++++++++++++++++++
 3 files changed, 254 insertions(+)
 create mode 100644 drivers/watchdog/sl28cpld_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0663c604bd64..6c53c1d0f348 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -340,6 +340,17 @@ config MLX_WDT
 	  To compile this driver as a module, choose M here: the
 	  module will be called mlx-wdt.
 
+config SL28CPLD_WATCHDOG
+	tristate "Kontron sl28 watchdog"
+	depends on MFD_SL28CPLD
+	select WATCHDOG_CORE
+	help
+	  Say Y here to include support for the watchdog timer
+	  on the Kontron sl28 CPLD.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called sl28cpld_wdt.
+
 # ALPHA Architecture
 
 # ARM Architecture
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6de2e4ceef19..b9ecdf2d7347 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -224,3 +224,4 @@ obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
 obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
 obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
 obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
+obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
diff --git a/drivers/watchdog/sl28cpld_wdt.c b/drivers/watchdog/sl28cpld_wdt.c
new file mode 100644
index 000000000000..79a7e36217a6
--- /dev/null
+++ b/drivers/watchdog/sl28cpld_wdt.c
@@ -0,0 +1,242 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld watchdog driver.
+ *
+ * Copyright 2019 Kontron Europe GmbH
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/watchdog.h>
+
+/*
+ * Watchdog timer block registers.
+ */
+#define WDT_CTRL			0x00
+#define  WDT_CTRL_EN			BIT(0)
+#define  WDT_CTRL_LOCK			BIT(2)
+#define  WDT_CTRL_ASSERT_SYS_RESET	BIT(6)
+#define  WDT_CTRL_ASSERT_WDT_TIMEOUT	BIT(7)
+#define WDT_TIMEOUT			0x01
+#define WDT_KICK			0x02
+#define  WDT_KICK_VALUE			0x6b
+#define WDT_COUNT			0x03
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static int timeout;
+module_param(timeout, int, 0);
+MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds");
+
+struct sl28cpld_wdt {
+	struct watchdog_device wdd;
+	struct regmap *regmap;
+	u32 offset;
+	bool assert_wdt_timeout;
+};
+
+static int sl28cpld_wdt_ping(struct watchdog_device *wdd)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	return regmap_write(wdt->regmap, wdt->offset + WDT_KICK,
+			    WDT_KICK_VALUE);
+}
+
+static int sl28cpld_wdt_start(struct watchdog_device *wdd)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+	unsigned int val;
+
+	val = WDT_CTRL_EN | WDT_CTRL_ASSERT_SYS_RESET;
+	if (wdt->assert_wdt_timeout)
+		val |= WDT_CTRL_ASSERT_WDT_TIMEOUT;
+	if (nowayout)
+		val |= WDT_CTRL_LOCK;
+
+	return regmap_update_bits(wdt->regmap, wdt->offset + WDT_CTRL,
+				  val, val);
+}
+
+static int sl28cpld_wdt_stop(struct watchdog_device *wdd)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	return regmap_update_bits(wdt->regmap, wdt->offset + WDT_CTRL,
+				  WDT_CTRL_EN, 0);
+}
+
+static unsigned int sl28cpld_wdt_status(struct watchdog_device *wdd)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+	unsigned int status;
+	int ret;
+
+	ret = regmap_read(wdt->regmap, wdt->offset + WDT_CTRL, &status);
+	if (ret < 0)
+		return 0;
+
+	/* is the watchdog timer running? */
+	return (status & WDT_CTRL_EN) << WDOG_ACTIVE;
+}
+
+static unsigned int sl28cpld_wdt_get_timeleft(struct watchdog_device *wdd)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(wdt->regmap, wdt->offset + WDT_COUNT, &val);
+	if (ret < 0)
+		return 0;
+
+	return val;
+}
+
+static int sl28cpld_wdt_set_timeout(struct watchdog_device *wdd,
+				  unsigned int timeout)
+{
+	int ret;
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+
+	ret = regmap_write(wdt->regmap, wdt->offset + WDT_TIMEOUT, timeout);
+	if (ret == 0)
+		wdd->timeout = timeout;
+
+	return ret;
+}
+
+static const struct watchdog_info sl28cpld_wdt_info = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "SMARC-sAL28 CPLD watchdog",
+};
+
+static struct watchdog_ops sl28cpld_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = sl28cpld_wdt_start,
+	.stop = sl28cpld_wdt_stop,
+	.status = sl28cpld_wdt_status,
+	.ping = sl28cpld_wdt_ping,
+	.set_timeout = sl28cpld_wdt_set_timeout,
+	.get_timeleft = sl28cpld_wdt_get_timeleft,
+};
+
+static int sl28cpld_wdt_locked(struct sl28cpld_wdt *wdt)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(wdt->regmap, wdt->offset + WDT_CTRL, &val);
+	if (ret < 0)
+		return ret;
+
+	return val & WDT_CTRL_LOCK;
+}
+
+static int sl28cpld_wdt_probe(struct platform_device *pdev)
+{
+	struct sl28cpld_wdt *wdt;
+	struct watchdog_device *wdd;
+	struct resource *res;
+	unsigned int val;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
+	if (!wdt)
+		return -ENOMEM;
+
+	wdt->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!wdt->regmap)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (res == NULL)
+		return -EINVAL;
+	wdt->offset = res->start;
+
+	if (device_property_read_bool(&pdev->dev,
+				      "kontron,assert-wdt-timeout-pin"))
+		wdt->assert_wdt_timeout = true;
+
+	/* initialize struct watchdog_device */
+	wdd = &wdt->wdd;
+	wdd->parent = &pdev->dev;
+	wdd->info = &sl28cpld_wdt_info;
+	wdd->ops = &sl28cpld_wdt_ops;
+	wdd->min_timeout = 1;
+	wdd->max_timeout = 255;
+
+	watchdog_set_drvdata(wdd, wdt);
+
+	/* if the watchdog is locked, we set nowayout to true */
+	ret = sl28cpld_wdt_locked(wdt);
+	if (ret < 0)
+		return ret;
+	if (ret)
+		nowayout = true;
+	watchdog_set_nowayout(wdd, nowayout);
+
+	/*
+	 * Initial timeout value, can either be set by kernel parameter or by
+	 * the device tree. If both are not given the current value is used.
+	 */
+	watchdog_init_timeout(wdd, timeout, &pdev->dev);
+	if (wdd->timeout) {
+		sl28cpld_wdt_set_timeout(wdd, wdd->timeout);
+	} else {
+		ret = regmap_read(wdt->regmap, wdt->offset + WDT_TIMEOUT,
+				  &val);
+		if (ret < 0)
+			return ret;
+		wdd->timeout = val;
+	}
+
+	watchdog_stop_on_reboot(wdd);
+	ret = devm_watchdog_register_device(&pdev->dev, wdd);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register watchdog device\n");
+		return ret;
+	}
+
+	platform_set_drvdata(pdev, wdt);
+
+	dev_info(&pdev->dev, "CPLD watchdog: initial timeout %d sec%s\n",
+		wdd->timeout, nowayout ? ", nowayout" : "");
+
+	return 0;
+}
+
+static const struct of_device_id sl28cpld_wdt_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-wdt" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_wdt_of_match);
+
+static const struct platform_device_id sl28cpld_wdt_id_table[] = {
+	{ "sl28cpld-wdt" },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, sl28cpld_wdt_id_table);
+
+static struct platform_driver sl28cpld_wdt_driver = {
+	.probe = sl28cpld_wdt_probe,
+	.id_table = sl28cpld_wdt_id_table,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = sl28cpld_wdt_of_match,
+	},
+};
+module_platform_driver(sl28cpld_wdt_driver);
+
+MODULE_DESCRIPTION("sl28cpld Watchdog Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 09/16] pwm: add support for sl28cpld PWM controller
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (7 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap Michael Walle
                   ` (6 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This adds support for the PWM controller of the sl28cpld board
management controller. This is part of a multi-function device driver.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-sl28cpld.c | 203 +++++++++++++++++++++++++++++++++++++
 3 files changed, 214 insertions(+)
 create mode 100644 drivers/pwm/pwm-sl28cpld.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index eebbc917ac97..5673e5e8b0c3 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -427,6 +427,16 @@ config PWM_SIFIVE
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-sifive.
 
+config PWM_SL28CPLD
+	tristate "Kontron sl28 PWM support"
+	depends on MFD_SL28CPLD
+	help
+	  Generic PWM framework driver for board management controller
+	  found on the Kontron sl28 CPLD.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-sl28cpld.
+
 config PWM_SPEAR
 	tristate "STMicroelectronics SPEAr PWM support"
 	depends on PLAT_SPEAR || COMPILE_TEST
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index 9a475073dafc..2c2b569dcde9 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -40,6 +40,7 @@ obj-$(CONFIG_PWM_RENESAS_TPU)	+= pwm-renesas-tpu.o
 obj-$(CONFIG_PWM_ROCKCHIP)	+= pwm-rockchip.o
 obj-$(CONFIG_PWM_SAMSUNG)	+= pwm-samsung.o
 obj-$(CONFIG_PWM_SIFIVE)	+= pwm-sifive.o
+obj-$(CONFIG_PWM_SL28CPLD)	+= pwm-sl28cpld.o
 obj-$(CONFIG_PWM_SPEAR)		+= pwm-spear.o
 obj-$(CONFIG_PWM_SPRD)		+= pwm-sprd.o
 obj-$(CONFIG_PWM_STI)		+= pwm-sti.o
diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
new file mode 100644
index 000000000000..c6b372bf45fa
--- /dev/null
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -0,0 +1,203 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld PWM driver.
+ *
+ * Copyright 2019 Kontron Europe GmbH
+ */
+
+#include <linux/bitfield.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+
+/*
+ * PWM timer block registers.
+ */
+#define PWM_CTRL		0x00
+#define   PWM_ENABLE		BIT(7)
+#define   PWM_MODE_250HZ	0
+#define   PWM_MODE_500HZ	1
+#define   PWM_MODE_1KHZ		2
+#define   PWM_MODE_2KHZ		3
+#define   PWM_MODE_MASK		GENMASK(1, 0)
+#define PWM_CYCLE		0x01
+#define   PWM_CYCLE_MAX		0x7f
+
+struct sl28cpld_pwm {
+	struct pwm_chip pwm_chip;
+	struct regmap *regmap;
+	u32 offset;
+};
+
+struct sl28cpld_pwm_periods {
+	u8 ctrl;
+	unsigned long duty_cycle;
+};
+
+struct sl28cpld_pwm_config {
+	unsigned long period_ns;
+	u8 max_duty_cycle;
+};
+
+static struct sl28cpld_pwm_config sl28cpld_pwm_config[] = {
+	[PWM_MODE_250HZ] = { .period_ns = 4000000, .max_duty_cycle = 0x80 },
+	[PWM_MODE_500HZ] = { .period_ns = 2000000, .max_duty_cycle = 0x40 },
+	[PWM_MODE_1KHZ] = { .period_ns = 1000000, .max_duty_cycle = 0x20 },
+	[PWM_MODE_2KHZ] = { .period_ns =  500000, .max_duty_cycle = 0x10 },
+};
+
+static inline struct sl28cpld_pwm *to_sl28cpld_pwm(struct pwm_chip *chip)
+{
+	return container_of(chip, struct sl28cpld_pwm, pwm_chip);
+}
+
+static void sl28cpld_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	struct sl28cpld_pwm *spc = to_sl28cpld_pwm(chip);
+	static struct sl28cpld_pwm_config *config;
+	unsigned int reg;
+	unsigned long cycle;
+	unsigned int mode;
+
+	regmap_read(spc->regmap, spc->offset + PWM_CTRL, &reg);
+
+	state->enabled = reg & PWM_ENABLE;
+
+	mode = FIELD_GET(PWM_MODE_MASK, reg);
+	config = &sl28cpld_pwm_config[mode];
+	state->period = config->period_ns;
+
+	regmap_read(spc->regmap, spc->offset + PWM_CYCLE, &reg);
+	cycle = reg * config->period_ns;
+	state->duty_cycle = DIV_ROUND_CLOSEST_ULL(cycle,
+						  config->max_duty_cycle);
+}
+
+static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
+			      const struct pwm_state *state)
+{
+	struct sl28cpld_pwm *spc = to_sl28cpld_pwm(chip);
+	struct sl28cpld_pwm_config *config;
+	unsigned long long cycle;
+	int ret;
+	int mode;
+	u8 ctrl;
+
+	/* update config, first search best matching period */
+	for (mode = 0; mode < ARRAY_SIZE(sl28cpld_pwm_config); mode++) {
+		config = &sl28cpld_pwm_config[mode];
+		if (state->period == config->period_ns)
+			break;
+	}
+
+	if (mode == ARRAY_SIZE(sl28cpld_pwm_config))
+		return -EINVAL;
+
+	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
+	if (state->enabled)
+		ctrl |= PWM_ENABLE;
+
+	cycle = state->duty_cycle * config->max_duty_cycle;
+	do_div(cycle, state->period);
+
+	/*
+	 * The hardware doesn't allow to set max_duty_cycle if the
+	 * 250Hz mode is enabled. But since this is "all-high" output
+	 * just use the 500Hz mode with the duty cycle to max value.
+	 */
+	if (cycle == config->max_duty_cycle) {
+		ctrl &= ~PWM_MODE_MASK;
+		ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ);
+		cycle = PWM_CYCLE_MAX;
+	}
+
+	ret = regmap_write(spc->regmap, spc->offset + PWM_CTRL, ctrl);
+	if (ret)
+		return ret;
+
+	return regmap_write(spc->regmap, spc->offset + PWM_CYCLE, (u8)cycle);
+}
+
+static const struct pwm_ops sl28cpld_pwm_ops = {
+	.apply = sl28cpld_pwm_apply,
+	.get_state = sl28cpld_pwm_get_state,
+	.owner = THIS_MODULE,
+};
+
+static int sl28cpld_pwm_probe(struct platform_device *pdev)
+{
+	struct sl28cpld_pwm *pwm;
+	struct pwm_chip *chip;
+	struct resource *res;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!pwm->regmap)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (!res)
+		return -EINVAL;
+	pwm->offset = res->start;
+
+	/* initialize struct gpio_chip */
+	chip = &pwm->pwm_chip;
+	chip->dev = &pdev->dev;
+	chip->ops = &sl28cpld_pwm_ops;
+	chip->base = -1;
+	chip->npwm = 1;
+
+	ret = pwmchip_add(&pwm->pwm_chip);
+	if (ret < 0)
+		return ret;
+
+	platform_set_drvdata(pdev, pwm);
+
+	return 0;
+}
+
+static int sl28cpld_pwm_remove(struct platform_device *pdev)
+{
+	struct sl28cpld_pwm *pwm = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&pwm->pwm_chip);
+}
+
+static const struct of_device_id sl28cpld_pwm_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-pwm" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_pwm_of_match);
+
+static const struct platform_device_id sl28cpld_pwm_id_table[] = {
+	{"sl28cpld-gpio"},
+	{},
+};
+MODULE_DEVICE_TABLE(platform, sl28cpld_pwm_id_table);
+
+static struct platform_driver sl28cpld_pwm_driver = {
+	.probe = sl28cpld_pwm_probe,
+	.remove	= sl28cpld_pwm_remove,
+	.id_table = sl28cpld_pwm_id_table,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = sl28cpld_pwm_of_match,
+	},
+};
+module_platform_driver(sl28cpld_pwm_driver);
+
+MODULE_DESCRIPTION("sl28cpld PWM Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (8 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 09/16] pwm: add support for sl28cpld PWM controller Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-06  7:47   ` Bartosz Golaszewski
  2020-04-16  9:27   ` Linus Walleij
  2020-04-02 20:36 ` [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller Michael Walle
                   ` (5 subsequent siblings)
  15 siblings, 2 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

There are quite a lot simple GPIO controller which are using regmap to
access the hardware. This driver tries to be a base to unify existing
code into one place. This won't cover everything but it should be a good
starting point.

It does not implement its own irq_chip because there is already a
generic one for regmap based devices. Instead, the irq_chip will be
instanciated in the parent driver and its irq domain will be associate
to this driver.

For now it consists of the usual registers, like set (and an optional
clear) data register, an input register and direction registers.
Out-of-the-box, it supports consecutive register mappings and mappings
where the registers have gaps between them with a linear mapping between
GPIO offset and bit position. For weirder mappings the user can register
its own .xlate().

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/gpio/Kconfig        |   4 +
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-regmap.c  | 320 ++++++++++++++++++++++++++++++++++++
 include/linux/gpio-regmap.h |  88 ++++++++++
 4 files changed, 413 insertions(+)
 create mode 100644 drivers/gpio/gpio-regmap.c
 create mode 100644 include/linux/gpio-regmap.h

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1b96169d84f7..a8e148f4b2e0 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -73,6 +73,10 @@ config GPIO_GENERIC
 	depends on HAS_IOMEM # Only for IOMEM drivers
 	tristate
 
+config GPIO_REGMAP
+	depends on REGMAP
+	tristate
+
 # put drivers in the right section, in alphabetical order
 
 # This symbol is selected by both I2C and SPI expanders
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index b2cfc21a97f3..93e139fdfa57 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_SYSFS)	+= gpiolib-sysfs.o
 obj-$(CONFIG_GPIO_ACPI)		+= gpiolib-acpi.o
 
 # Device drivers. Generally keep list sorted alphabetically
+obj-$(CONFIG_GPIO_REGMAP)	+= gpio-regmap.o
 obj-$(CONFIG_GPIO_GENERIC)	+= gpio-generic.o
 
 # directly supported by gpio-generic
diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
new file mode 100644
index 000000000000..cc4437dc0521
--- /dev/null
+++ b/drivers/gpio/gpio-regmap.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * regmap based generic GPIO driver
+ *
+ * Copyright 2019 Michael Walle <michael@walle.cc>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/gpio-regmap.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+struct gpio_regmap_data {
+	struct gpio_chip gpio_chip;
+	struct gpio_regmap *gpio;
+};
+
+/**
+ * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
+ *
+ * Use a simple linear mapping to translate the offset to the bitmask.
+ */
+int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base,
+			     unsigned int offset,
+			     unsigned int *reg, unsigned int *mask)
+{
+	unsigned int line = offset % gpio->ngpio_per_reg;
+	unsigned int stride = offset / gpio->ngpio_per_reg;
+
+	*reg = base + stride * gpio->reg_stride;
+	*mask = BIT(line);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate);
+
+static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_regmap_data *data = gpiochip_get_data(chip);
+	struct gpio_regmap *gpio = data->gpio;
+	unsigned int base;
+	unsigned int val, reg, mask;
+	int ret;
+
+	/* we might not have an output register if we are input only */
+	if (gpio->reg_dat_base.valid)
+		base = gpio->reg_dat_base.addr;
+	else
+		base = gpio->reg_set_base.addr;
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(gpio->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	return (val & mask) ? 1 : 0;
+}
+
+static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
+			    int val)
+{
+	struct gpio_regmap_data *data = gpiochip_get_data(chip);
+	struct gpio_regmap *gpio = data->gpio;
+	unsigned int base = gpio->reg_set_base.addr;
+	unsigned int reg, mask;
+
+	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (val)
+		regmap_update_bits(gpio->regmap, reg, mask, mask);
+	else
+		regmap_update_bits(gpio->regmap, reg, mask, 0);
+}
+
+static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
+				       unsigned int offset, int val)
+{
+	struct gpio_regmap_data *data = gpiochip_get_data(chip);
+	struct gpio_regmap *gpio = data->gpio;
+	unsigned int base;
+	unsigned int reg, mask;
+
+	if (val)
+		base = gpio->reg_set_base.addr;
+	else
+		base = gpio->reg_clr_base.addr;
+
+	gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	regmap_write(gpio->regmap, reg, mask);
+}
+
+static int gpio_regmap_get_direction(struct gpio_chip *chip,
+				     unsigned int offset)
+{
+	struct gpio_regmap_data *data = gpiochip_get_data(chip);
+	struct gpio_regmap *gpio = data->gpio;
+	unsigned int val, reg, mask;
+	unsigned int base;
+	int invert;
+	int ret;
+
+	if (gpio->reg_dir_out_base.valid) {
+		base = gpio->reg_dir_out_base.addr;
+		invert = 0;
+	} else if (gpio->reg_dir_in_base.valid) {
+		base = gpio->reg_dir_in_base.addr;
+		invert = 1;
+	} else {
+		return GPIO_LINE_DIRECTION_IN;
+	}
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(gpio->regmap, reg, &val);
+	if (ret)
+		return ret;
+
+	if (!!(val & mask) ^ invert)
+		return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_IN;
+}
+
+static int gpio_regmap_set_direction(struct gpio_chip *chip,
+				     unsigned int offset, bool output)
+{
+	struct gpio_regmap_data *data = gpiochip_get_data(chip);
+	struct gpio_regmap *gpio = data->gpio;
+	unsigned int val, reg, mask;
+	unsigned int base;
+	int invert;
+	int ret;
+
+	if (gpio->reg_dir_out_base.valid) {
+		base = gpio->reg_dir_out_base.addr;
+		invert = 0;
+	} else if (gpio->reg_dir_in_base.valid) {
+		base = gpio->reg_dir_in_base.addr;
+		invert = 1;
+	} else {
+		return 0;
+	}
+
+	ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
+	if (ret)
+		return ret;
+
+	if (!invert)
+		val = (output) ? mask : 0;
+	else
+		val = (output) ? 0 : mask;
+
+	return regmap_update_bits(gpio->regmap, reg, mask, val);
+}
+
+static int gpio_regmap_direction_input(struct gpio_chip *chip,
+				       unsigned int offset)
+{
+	return gpio_regmap_set_direction(chip, offset, false);
+}
+
+static int gpio_regmap_direction_output(struct gpio_chip *chip,
+					unsigned int offset, int value)
+{
+	gpio_regmap_set(chip, offset, value);
+	return gpio_regmap_set_direction(chip, offset, true);
+}
+
+static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct gpio_regmap_data *data = gpiochip_get_data(chip);
+	struct gpio_regmap *gpio = data->gpio;
+
+	/* the user might have its own .to_irq callback */
+	if (gpio->to_irq)
+		return gpio->to_irq(gpio, offset);
+
+	return irq_create_mapping(gpio->irq_domain, offset);
+}
+
+/**
+ * gpio_regmap_register() - Register a generic regmap GPIO controller
+ *
+ * @gpio: gpio_regmap device to register
+ *
+ * Returns 0 on success or an errno on failure.
+ */
+int gpio_regmap_register(struct gpio_regmap *gpio)
+{
+	struct gpio_regmap_data *d;
+	struct gpio_chip *chip;
+	int ret;
+
+	if (!gpio->parent)
+		return -EINVAL;
+
+	if (!gpio->ngpio)
+		return -EINVAL;
+
+	/* we need at least one */
+	if (!gpio->reg_dat_base.valid && !gpio->reg_set_base.valid)
+		return -EINVAL;
+
+	/* we don't support having both registers simulaniously for now */
+	if (gpio->reg_dir_out_base.valid && gpio->reg_dir_in_base.valid)
+		return -EINVAL;
+
+	/* if not set, assume they are consecutive */
+	if (!gpio->reg_stride)
+		gpio->reg_stride = 1;
+
+	/* if not set, assume there is only one register */
+	if (!gpio->ngpio_per_reg)
+		gpio->ngpio_per_reg = gpio->ngpio;
+
+	if (!gpio->reg_mask_xlate)
+		gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
+
+	d = kzalloc(sizeof(*d), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	gpio->data = d;
+	d->gpio = gpio;
+
+	chip = &d->gpio_chip;
+	chip->parent = gpio->parent;
+	chip->label = gpio->label;
+	chip->base = -1;
+	chip->ngpio = gpio->ngpio;
+	chip->can_sleep = true;
+	chip->get = gpio_regmap_get;
+
+	if (!chip->label)
+		chip->label = dev_name(gpio->parent);
+
+	if (gpio->reg_set_base.valid && gpio->reg_clr_base.valid)
+		chip->set = gpio_regmap_set_with_clear;
+	else if (gpio->reg_set_base.valid)
+		chip->set = gpio_regmap_set;
+
+	if (gpio->reg_dir_in_base.valid || gpio->reg_dir_out_base.valid) {
+		chip->get_direction = gpio_regmap_get_direction;
+		chip->direction_input = gpio_regmap_direction_input;
+		chip->direction_output = gpio_regmap_direction_output;
+	}
+
+	if (gpio->irq_domain)
+		chip->to_irq = gpio_regmap_to_irq;
+
+	ret = gpiochip_add_data(chip, d);
+	if (ret < 0)
+		goto err_alloc;
+
+	return 0;
+
+err_alloc:
+	kfree(d);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_register);
+
+/**
+ * gpio_regmap_unregister() - Unregister a generic regmap GPIO controller
+ *
+ * @gpio: gpio_regmap device to unregister
+ */
+void gpio_regmap_unregister(struct gpio_regmap *gpio)
+{
+	gpiochip_remove(&gpio->data->gpio_chip);
+	kfree(gpio->data);
+}
+EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
+
+static void devm_gpio_regmap_unregister(struct device *dev, void *res)
+{
+	gpio_regmap_unregister(*(struct gpio_regmap **)res);
+}
+
+/**
+ * devm_gpio_regmap_register() - resource managed gpio_regmap_register()
+ *
+ * @dev: device that is registering this GPIO device
+ * @gpio: gpio_regmap device to register
+ *
+ * Managed gpio_regmap_register(). For generic regmap GPIO device registered by
+ * this function, gpio_regmap_unregister() is automatically called on driver
+ * detach. See gpio_regmap_register() for more information.
+ */
+int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio)
+{
+	struct gpio_regmap **ptr;
+	int ret;
+
+	ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
+			   GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = gpio_regmap_register(gpio);
+	if (ret) {
+		devres_free(ptr);
+		return ret;
+	}
+
+	*ptr = gpio;
+	devres_add(dev, ptr);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_gpio_regmap_register);
+
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_DESCRIPTION("GPIO generic regmap driver core");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
new file mode 100644
index 000000000000..ad63955e0e43
--- /dev/null
+++ b/include/linux/gpio-regmap.h
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _LINUX_GPIO_REGMAP_H
+#define _LINUX_GPIO_REGMAP_H
+
+struct gpio_regmap_addr {
+	unsigned int addr;
+	bool valid;
+};
+#define GPIO_REGMAP_ADDR(_addr) \
+	((struct gpio_regmap_addr) { .addr = _addr, .valid = true })
+
+/**
+ * struct gpio_regmap - Description of a generic regmap gpio_chip.
+ *
+ * @parent:		The parent device
+ * @regmap:		The regmap use to access the registers
+ *			given, the name of the device is used
+ * @label:		(Optional) Descriptive name for GPIO controller.
+ *			If not given, the name of the device is used.
+ * @ngpio:		Number of GPIOs
+ * @reg_dat_base:	(Optional) (in) register base address
+ * @reg_set_base:	(Optional) set register base address
+ * @reg_clr_base:	(Optional) clear register base address
+ * @reg_dir_in_base:	(Optional) out setting register base address
+ * @reg_dir_out_base:	(Optional) in setting register base address
+ * @reg_stride:		(Optional) May be set if the registers (of the
+ *			same type, dat, set, etc) are not consecutive.
+ * @ngpio_per_reg:	Number of GPIOs per register
+ * @irq_domain:		(Optional) IRQ domain if the controller is
+ *			interrupt-capable
+ * @reg_mask_xlate:     (Optional) Translates base address and GPIO
+ *			offset to a register/bitmask pair. If not
+ *			given the default gpio_regmap_simple_xlate()
+ *			is used.
+ * @to_irq:		(Optional) Maps GPIO offset to a irq number.
+ *			By default assumes a linear mapping of the
+ *			given irq_domain.
+ * @driver_data:	Pointer to the drivers private data. Not used by
+ *			gpio-regmap.
+ *
+ * The reg_mask_xlate translates a given base address and GPIO offset to
+ * register and mask pair. The base address is one of the given reg_*_base.
+ */
+struct gpio_regmap {
+	struct device *parent;
+	struct regmap *regmap;
+	struct gpio_regmap_data *data;
+
+	const char *label;
+	int ngpio;
+
+	struct gpio_regmap_addr reg_dat_base;
+	struct gpio_regmap_addr reg_set_base;
+	struct gpio_regmap_addr reg_clr_base;
+	struct gpio_regmap_addr reg_dir_in_base;
+	struct gpio_regmap_addr reg_dir_out_base;
+	int reg_stride;
+	int ngpio_per_reg;
+	struct irq_domain *irq_domain;
+
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
+			      unsigned int offset, unsigned int *reg,
+			      unsigned int *mask);
+	int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset);
+
+	void *driver_data;
+};
+
+static inline void gpio_regmap_set_drvdata(struct gpio_regmap *gpio,
+					   void *data)
+{
+	gpio->driver_data = data;
+}
+
+static inline void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
+{
+	return gpio->driver_data;
+}
+
+int gpio_regmap_register(struct gpio_regmap *gpio);
+void gpio_regmap_unregister(struct gpio_regmap *gpio);
+int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio);
+int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base,
+			     unsigned int offset,
+			     unsigned int *reg, unsigned int *mask);
+
+#endif /* _LINUX_GPIO_REGMAP_H */
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (9 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-16  8:34   ` Linus Walleij
  2020-04-02 20:36 ` [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
                   ` (4 subsequent siblings)
  15 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This adds support for the GPIO controller of the sl28 board management
controller. This driver is part of a multi-function device.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 drivers/gpio/Kconfig         |  11 +++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-sl28cpld.c | 186 +++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/gpio/gpio-sl28cpld.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index a8e148f4b2e0..d6d54cb9558e 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1215,6 +1215,17 @@ config GPIO_RC5T583
 	  This driver provides the support for driving/reading the gpio pins
 	  of RC5T583 device through standard gpio library.
 
+config GPIO_SL28CPLD
+	tristate "Kontron sl28 GPIO"
+	depends on MFD_SL28CPLD
+	select GPIO_REGMAP
+	select GPIOLIB_IRQCHIP
+	help
+	  This enables support for the GPIOs found on the Kontron sl28 CPLD.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-sl28cpld.
+
 config GPIO_STMPE
 	bool "STMPE GPIOs"
 	depends on MFD_STMPE
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 93e139fdfa57..f50ccf13f5eb 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -129,6 +129,7 @@ obj-$(CONFIG_GPIO_SCH311X)		+= gpio-sch311x.o
 obj-$(CONFIG_GPIO_SCH)			+= gpio-sch.o
 obj-$(CONFIG_GPIO_SIFIVE)		+= gpio-sifive.o
 obj-$(CONFIG_GPIO_SIOX)			+= gpio-siox.o
+obj-$(CONFIG_GPIO_SL28CPLD)		+= gpio-sl28cpld.o
 obj-$(CONFIG_GPIO_SODAVILLE)		+= gpio-sodaville.o
 obj-$(CONFIG_GPIO_SPEAR_SPICS)		+= gpio-spear-spics.o
 obj-$(CONFIG_GPIO_SPRD)			+= gpio-sprd.o
diff --git a/drivers/gpio/gpio-sl28cpld.c b/drivers/gpio/gpio-sl28cpld.c
new file mode 100644
index 000000000000..1d199517e2e0
--- /dev/null
+++ b/drivers/gpio/gpio-sl28cpld.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld GPIO driver.
+ *
+ * Copyright 2019 Michael Walle <michael@walle.cc>
+ */
+
+#include <linux/device.h>
+#include <linux/gpio-regmap.h>
+#include <linux/gpio/driver.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+/* GPIO flavor */
+#define GPIO_REG_DIR	0x00
+#define GPIO_REG_OUT	0x01
+#define GPIO_REG_IN	0x02
+#define GPIO_REG_IE	0x03
+#define GPIO_REG_IP	0x04
+
+/* input-only flavor */
+#define GPI_REG_IN	0x00
+
+/* output-only flavor */
+#define GPO_REG_OUT	0x00
+
+enum sl28cpld_gpio_type {
+	SL28CPLD_GPIO = 1,
+	SL28CPLD_GPI,
+	SL28CPLD_GPO,
+};
+
+struct sl28cpld_gpio {
+	struct gpio_regmap gpio_chip;
+	struct regmap_irq_chip irq_chip;
+	struct regmap_irq_chip_data *irq_data;
+};
+
+static const struct regmap_irq sl28cpld_gpio_irqs[] = {
+	REGMAP_IRQ_REG_LINE(0, 8),
+	REGMAP_IRQ_REG_LINE(1, 8),
+	REGMAP_IRQ_REG_LINE(2, 8),
+	REGMAP_IRQ_REG_LINE(3, 8),
+	REGMAP_IRQ_REG_LINE(4, 8),
+	REGMAP_IRQ_REG_LINE(5, 8),
+	REGMAP_IRQ_REG_LINE(6, 8),
+	REGMAP_IRQ_REG_LINE(7, 8),
+};
+
+static int sl28cpld_gpio_irq_init(struct device *dev,
+				   struct sl28cpld_gpio *gpio,
+				   struct regmap *regmap, unsigned int base,
+				   int irq)
+{
+	struct regmap_irq_chip *irq_chip = &gpio->irq_chip;
+
+	irq_chip->name = "sl28cpld-gpio-irq",
+	irq_chip->irqs = sl28cpld_gpio_irqs;
+	irq_chip->num_irqs = ARRAY_SIZE(sl28cpld_gpio_irqs);
+	irq_chip->num_regs = 1;
+	irq_chip->status_base = base + GPIO_REG_IP;
+	irq_chip->mask_base = base + GPIO_REG_IE;
+	irq_chip->mask_invert = true,
+	irq_chip->ack_base = base + GPIO_REG_IP;
+
+	return devm_regmap_add_irq_chip_np(dev, dev_of_node(dev), regmap,
+					   irq, IRQF_SHARED | IRQF_ONESHOT, 0,
+					   irq_chip, &gpio->irq_data);
+}
+
+static int sl28cpld_gpio_probe(struct platform_device *pdev)
+{
+	const struct platform_device_id *dev_id;
+	enum sl28cpld_gpio_type type;
+	struct sl28cpld_gpio *gpio;
+	struct gpio_regmap *gpio_chip;
+	struct regmap *regmap;
+	struct resource *res;
+	bool irq_support = false;
+	unsigned int base;
+	int ret;
+	int irq;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	dev_id = platform_get_device_id(pdev);
+	if (dev_id)
+		type = dev_id->driver_data;
+	else
+		type = (uintptr_t)of_device_get_match_data(&pdev->dev);
+	if (!type)
+		return -ENODEV;
+
+	gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (!res)
+		return -EINVAL;
+	base = res->start;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	gpio_chip = &gpio->gpio_chip;
+	gpio_chip->regmap = regmap;
+	gpio_chip->parent = &pdev->dev;
+	gpio_chip->ngpio = 8;
+
+	switch (type) {
+	case SL28CPLD_GPIO:
+		gpio_chip->reg_dat_base = GPIO_REGMAP_ADDR(base + GPIO_REG_IN);
+		gpio_chip->reg_set_base =
+				GPIO_REGMAP_ADDR(base + GPIO_REG_OUT);
+		gpio_chip->reg_dir_out_base =
+				GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
+		irq_support = true;
+		break;
+	case SL28CPLD_GPO:
+		gpio_chip->reg_set_base = GPIO_REGMAP_ADDR(base + GPO_REG_OUT);
+		break;
+	case SL28CPLD_GPI:
+		gpio_chip->reg_dat_base = GPIO_REGMAP_ADDR(base + GPI_REG_IN);
+		break;
+	default:
+		dev_err(&pdev->dev, "unknown type %d\n", type);
+		return -ENODEV;
+	}
+
+	if (irq_support &&
+	    device_property_read_bool(&pdev->dev, "interrupt-controller")) {
+		irq = platform_get_irq(pdev, 0);
+		if (irq < 0)
+			return irq;
+
+		ret = sl28cpld_gpio_irq_init(&pdev->dev, gpio, regmap,
+					     base, irq);
+		if (ret)
+			return ret;
+
+		gpio_chip->irq_domain = regmap_irq_get_domain(gpio->irq_data);
+		dev_info(&pdev->dev, "registered IRQ %d\n", irq);
+	}
+
+	return devm_gpio_regmap_register(&pdev->dev, gpio_chip);
+}
+
+static const struct of_device_id sl28cpld_gpio_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-gpio",
+	  .data = (void *)SL28CPLD_GPIO },
+	{ .compatible = "kontron,sl28cpld-gpi",
+	  .data = (void *)SL28CPLD_GPI },
+	{ .compatible = "kontron,sl28cpld-gpo",
+	  .data = (void *)SL28CPLD_GPO },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_gpio_of_match);
+
+static const struct platform_device_id sl28cpld_gpio_id_table[] = {
+	{ "sl28cpld-gpio", SL28CPLD_GPIO },
+	{ "sl28cpld-gpi", SL28CPLD_GPI },
+	{ "sl28cpld-gpo", SL28CPLD_GPO },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, sl28cpld_gpio_id_table);
+
+static struct platform_driver sl28cpld_gpio_driver = {
+	.probe = sl28cpld_gpio_probe,
+	.id_table = sl28cpld_gpio_id_table,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = sl28cpld_gpio_of_match,
+	},
+};
+module_platform_driver(sl28cpld_gpio_driver);
+
+MODULE_DESCRIPTION("sl28cpld GPIO Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (10 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 21:30   ` Guenter Roeck
  2020-04-02 20:36 ` [PATCH v2 13/16] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
                   ` (3 subsequent siblings)
  15 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

This adds support for the hardware monitoring controller of the sl28cpld
board management controller. This driver is part of a multi-function
device.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 Documentation/hwmon/sl28cpld.rst |  36 ++++++++
 drivers/hwmon/Kconfig            |  10 ++
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/sl28cpld-hwmon.c   | 151 +++++++++++++++++++++++++++++++
 4 files changed, 198 insertions(+)
 create mode 100644 Documentation/hwmon/sl28cpld.rst
 create mode 100644 drivers/hwmon/sl28cpld-hwmon.c

diff --git a/Documentation/hwmon/sl28cpld.rst b/Documentation/hwmon/sl28cpld.rst
new file mode 100644
index 000000000000..7ed65f78250c
--- /dev/null
+++ b/Documentation/hwmon/sl28cpld.rst
@@ -0,0 +1,36 @@
+.. SPDX-License-Identifier: GPL-2.0-only
+
+Kernel driver sl28cpld
+======================
+
+Supported chips:
+
+   * Kontron sl28cpld
+
+     Prefix: 'sl28cpld'
+
+     Datasheet: not available
+
+Authors: Michael Walle <michael@walle.cc>
+
+Description
+-----------
+
+The sl28cpld is a board management controller which also exposes a hardware
+monitoring controller. At the moment this controller supports a single fan
+supervisor. In the future there might be other flavours and additional
+hardware monitoring might be supported.
+
+The fan supervisor has a 7 bit counter register and a counter period of 1
+second. If the 7 bit counter overflows, the supervisor will automatically
+switch to x8 mode to support a wider input range at the loss of
+granularity.
+
+Sysfs entries
+-------------
+
+The following attributes are supported.
+
+======================= ========================================================
+fan1_input		Fan RPM. Assuming 2 pulses per revolution.
+======================= ========================================================
diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 05a30832c6ba..c98716f78cfa 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1412,6 +1412,16 @@ config SENSORS_RASPBERRYPI_HWMON
 	  This driver can also be built as a module. If so, the module
 	  will be called raspberrypi-hwmon.
 
+config SENSORS_SL28CPLD
+	tristate "Kontron's SMARC-sAL28 hardware monitoring driver"
+	depends on MFD_SL28CPLD
+	help
+	  If you say yes here you get support for a fan connected to the
+	  input of the SMARC connector of Kontron's SMARC-sAL28 module.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called sl28cpld-hwmon.
+
 config SENSORS_SHT15
 	tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
 	depends on GPIOLIB || COMPILE_TEST
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index b0b9c8e57176..dfb0f8cda2dd 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -155,6 +155,7 @@ obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
 obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
 obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
 obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
+obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
 obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
 obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
 obj-$(CONFIG_SENSORS_SHT3x)	+= sht3x.o
diff --git a/drivers/hwmon/sl28cpld-hwmon.c b/drivers/hwmon/sl28cpld-hwmon.c
new file mode 100644
index 000000000000..c79bdfed8332
--- /dev/null
+++ b/drivers/hwmon/sl28cpld-hwmon.c
@@ -0,0 +1,151 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld hardware monitoring driver.
+ *
+ * Copyright 2019 Kontron Europe GmbH
+ */
+
+#include <linux/bitfield.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define FAN_INPUT		0x00
+#define   FAN_SCALE_X8		BIT(7)
+#define   FAN_VALUE_MASK	GENMASK(6, 0)
+
+struct sl28cpld_hwmon {
+	struct regmap *regmap;
+	u32 offset;
+};
+
+static umode_t sl28cpld_hwmon_is_visible(const void *data,
+					 enum hwmon_sensor_types type,
+					 u32 attr, int channel)
+{
+	return 0444;
+}
+
+static int sl28cpld_hwmon_read(struct device *dev,
+			       enum hwmon_sensor_types type, u32 attr,
+			       int channel, long *input)
+{
+	struct sl28cpld_hwmon *hwmon = dev_get_drvdata(dev);
+	unsigned int value;
+	int ret;
+
+	switch (attr) {
+	case hwmon_fan_input:
+		ret = regmap_read(hwmon->regmap, hwmon->offset + FAN_INPUT,
+				  &value);
+		if (ret)
+			return ret;
+		/*
+		 * The register has a 7 bit value and 1 bit which indicates the
+		 * scale. If the MSB is set, then the lower 7 bit has to be
+		 * multiplied by 8, to get the correct reading.
+		 */
+		if (value & FAN_SCALE_X8)
+			value = FIELD_GET(FAN_VALUE_MASK, value) << 3;
+
+		/*
+		 * The counter period is 1000ms and the sysfs specification
+		 * says we should asssume 2 pulses per revolution.
+		 */
+		value *= 60 / 2;
+
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	*input = value;
+	return 0;
+}
+
+static const u32 sl28cpld_hwmon_fan_config[] = {
+	HWMON_F_INPUT,
+	0
+};
+
+static const struct hwmon_channel_info sl28cpld_hwmon_fan = {
+	.type = hwmon_fan,
+	.config = sl28cpld_hwmon_fan_config,
+};
+
+static const struct hwmon_channel_info *sl28cpld_hwmon_info[] = {
+	&sl28cpld_hwmon_fan,
+	NULL
+};
+
+static const struct hwmon_ops sl28cpld_hwmon_ops = {
+	.is_visible = sl28cpld_hwmon_is_visible,
+	.read = sl28cpld_hwmon_read,
+};
+
+static const struct hwmon_chip_info sl28cpld_hwmon_chip_info = {
+	.ops = &sl28cpld_hwmon_ops,
+	.info = sl28cpld_hwmon_info,
+};
+
+static int sl28cpld_hwmon_probe(struct platform_device *pdev)
+{
+	struct device *hwmon_dev;
+	struct sl28cpld_hwmon *hwmon;
+	struct resource *res;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
+	if (!hwmon)
+		return -ENOMEM;
+
+	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!hwmon->regmap)
+		return -ENODEV;
+
+	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
+	if (!res)
+		return -EINVAL;
+	hwmon->offset = res->start;
+
+	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
+				"sl28cpld_hwmon", hwmon,
+				&sl28cpld_hwmon_chip_info, NULL);
+	if (IS_ERR(hwmon_dev)) {
+		dev_err(&pdev->dev, "failed to register as hwmon device");
+		return PTR_ERR(hwmon_dev);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id sl28cpld_hwmon_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-fan" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_hwmon_of_match);
+
+static const struct platform_device_id sl28cpld_hwmon_id_table[] = {
+	{ "sl28cpld-fan" },
+	{}
+};
+MODULE_DEVICE_TABLE(platform, sl28cpld_hwmon_id_table);
+
+static struct platform_driver sl28cpld_hwmon_driver = {
+	.probe = sl28cpld_hwmon_probe,
+	.id_table = sl28cpld_hwmon_id_table,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = sl28cpld_hwmon_of_match,
+	},
+};
+module_platform_driver(sl28cpld_hwmon_driver);
+
+MODULE_DESCRIPTION("sl28cpld Hardware Monitoring Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 13/16] arm64: dts: freescale: sl28: enable sl28cpld
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (11 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 14/16] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

Add the board management controller node.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../freescale/fsl-ls1028a-kontron-sl28.dts    | 92 +++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
index 41ba38adc906..b73794d57db4 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
@@ -8,6 +8,7 @@
 
 /dts-v1/;
 #include "fsl-ls1028a.dtsi"
+#include <dt-bindings/interrupt-controller/irq.h>
 
 / {
 	model = "Kontron SMARC-sAL28";
@@ -174,6 +175,97 @@
 		reg = <0x32>;
 	};
 
+	sl28cpld: sl28cpld@4a {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "kontron,sl28cpld";
+		reg = <0x4a>;
+		interrupts-extended = <&gpio2 6 IRQ_TYPE_EDGE_FALLING>;
+
+		interrupt-controller;
+		#interrupt-cells = <2>;
+
+		watchdog@0 {
+			compatible = "kontron,sl28cpld-wdt";
+			reg = <0>;
+			kontron,assert-wdt-timeout-pin;
+		};
+
+		hwmon@4 {
+			compatible = "kontron,sl28cpld-fan";
+			reg = <1>;
+		};
+
+		sl28cpld_pwm0: pwm@5 {
+			#pwm-cells = <2>;
+			compatible = "kontron,sl28cpld-pwm";
+			reg = <2>;
+		};
+
+		sl28cpld_pwm1: pwm@6 {
+			#pwm-cells = <2>;
+			compatible = "kontron,sl28cpld-pwm";
+			reg = <3>;
+		};
+
+		sl28cpld_gpio0: gpio@0 {
+			compatible = "kontron,sl28cpld-gpio";
+			reg = <4>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-line-names =
+				"GPIO0_CAM0_PWR_N", "GPIO1_CAM1_PWR_N",
+				"GPIO2_CAM0_RST_N", "GPIO3_CAM1_RST_N",
+				"GPIO4_HDA_RST_N", "GPIO5_PWM_OUT",
+				"GPIO6_TACHIN", "GPIO7";
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		sl28cpld_gpio1: gpio@1 {
+			compatible = "kontron,sl28cpld-gpio";
+			reg = <5>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-line-names =
+				"GPIO8", "GPIO9", "GPIO10", "GPIO11",
+				"", "", "", "";
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		sl28cpld_gpio2: gpio@2 {
+			compatible = "kontron,sl28cpld-gpo";
+			reg = <6>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-line-names =
+				"LCD0 voltage enable",
+				"LCD0 backlight enable",
+				"eMMC reset", "LVDS bridge reset",
+				"LVDS bridge power-down",
+				"SDIO power enable",
+				"", "";
+		};
+
+		sl28cpld_gpio3: gpio@3 {
+			compatible = "kontron,sl28cpld-gpi";
+			reg = <7>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-line-names =
+				"Power button", "Force recovery", "Sleep",
+				"Battery low", "Lid state", "Charging",
+				"Charger present", "";
+		};
+	};
+
 	eeprom@50 {
 		compatible = "atmel,24c32";
 		reg = <0x50>;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 14/16] arm64: dts: freescale: sl28: map GPIOs to input events
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (12 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 13/16] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 15/16] arm64: dts: freescale: sl28: enable LED support Michael Walle
  2020-04-02 20:36 ` [PATCH v2 16/16] arm64: dts: freescale: sl28: enable fan support Michael Walle
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

Now that we have support for GPIO lines of the SMARC connector, map the
sleep, power and lid switch signals to the corresponding keys using the
gpio-keys and gpio-keys-polled drivers. The power and sleep signals have
dedicated interrupts, thus we use these ones. The lid switch is just
mapped to a GPIO input and needs polling.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../freescale/fsl-ls1028a-kontron-sl28.dts    | 32 +++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
index b73794d57db4..263ce50b0b79 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
@@ -9,6 +9,8 @@
 /dts-v1/;
 #include "fsl-ls1028a.dtsi"
 #include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
 
 / {
 	model = "Kontron SMARC-sAL28";
@@ -23,6 +25,36 @@
 		spi1 = &dspi2;
 	};
 
+	buttons0 {
+		compatible = "gpio-keys";
+
+		power-button {
+			interrupts-extended = <&sl28cpld
+					       4 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_POWER>;
+			label = "Power";
+		};
+
+		sleep-button {
+			interrupts-extended = <&sl28cpld
+					       5 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_SLEEP>;
+			label = "Sleep";
+		};
+	};
+
+	buttons1 {
+		compatible = "gpio-keys-polled";
+		poll-interval = <200>;
+
+		lid-switch {
+			linux,input-type = <EV_SW>;
+			linux,code = <SW_LID>;
+			gpios = <&sl28cpld_gpio3 4 GPIO_ACTIVE_LOW>;
+			label = "Lid";
+		};
+	};
+
 	chosen {
 		stdout-path = "serial0:115200n8";
 	};
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 15/16] arm64: dts: freescale: sl28: enable LED support
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (13 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 14/16] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  2020-04-02 20:36 ` [PATCH v2 16/16] arm64: dts: freescale: sl28: enable fan support Michael Walle
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

Now that we have support for GPIO lines of the SMARC connector, enable
LED support on the KBox A-230-LS. There are two LEDs without fixed
functions, one is yellow and one is green. Unfortunately, it is just one
multi-color LED, thus while it is possible to enable both at the same
time it is hard to tell the difference between "yellow only" and "yellow
and green".

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../fsl-ls1028a-kontron-kbox-a-230-ls.dts          | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
index 4b4cc6a1573d..49cf4fe05c80 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
@@ -16,6 +16,20 @@
 	model = "Kontron KBox A-230-LS";
 	compatible = "kontron,kbox-a-230-ls", "kontron,sl28-var4",
 		     "kontron,sl28", "fsl,ls1028a";
+
+	leds {
+		compatible = "gpio-leds";
+
+		user_yellow {
+			label = "s1914:yellow:user";
+			gpios = <&sl28cpld_gpio0 0 0>;
+		};
+
+		user_green {
+			label = "s1914:green:user";
+			gpios = <&sl28cpld_gpio1 3 0>;
+		};
+	};
 };
 
 &enetc_mdio_pf3 {
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* [PATCH v2 16/16] arm64: dts: freescale: sl28: enable fan support
  2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
                   ` (14 preceding siblings ...)
  2020-04-02 20:36 ` [PATCH v2 15/16] arm64: dts: freescale: sl28: enable LED support Michael Walle
@ 2020-04-02 20:36 ` Michael Walle
  15 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-02 20:36 UTC (permalink / raw)
  To: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel
  Cc: Linus Walleij, Bartosz Golaszewski, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Michael Walle

Add a pwm-fan mapped to the PWM channel 0 which is connected to the
fan connector of the carrier.

Signed-off-by: Michael Walle <michael@walle.cc>
---
 .../dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts
index 0973a6a45217..c45d7b40e374 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var3-ads2.dts
@@ -15,6 +15,15 @@
 	compatible = "kontron,sl28-var3-ads2", "kontron,sl28-var3",
 		     "kontron,sl28", "fsl,ls1028a";
 
+	pwm-fan {
+		compatible = "pwm-fan";
+		cooling-min-state = <0>;
+		cooling-max-state = <3>;
+		#cooling-cells = <2>;
+		pwms = <&sl28cpld_pwm0 0 4000000>;
+		cooling-levels = <1 128 192 255>;
+	};
+
 	sound {
 		#address-cells = <1>;
 		#size-cells = <0>;
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller
  2020-04-02 20:36 ` [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
@ 2020-04-02 21:30   ` Guenter Roeck
  0 siblings, 0 replies; 38+ messages in thread
From: Guenter Roeck @ 2020-04-02 21:30 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Lee Jones,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown, Greg Kroah-Hartman

On Thu, Apr 02, 2020 at 10:36:52PM +0200, Michael Walle wrote:
> This adds support for the hardware monitoring controller of the sl28cpld
> board management controller. This driver is part of a multi-function
> device.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  Documentation/hwmon/sl28cpld.rst |  36 ++++++++

Needs to be added to Documentation.hwmon/index.rst.
Otherwise looks good. With that fixed, please feel free to add

Acked-by: Guenter Roeck <linux@roeck-us.net>

in the next version.

Guenter

>  drivers/hwmon/Kconfig            |  10 ++
>  drivers/hwmon/Makefile           |   1 +
>  drivers/hwmon/sl28cpld-hwmon.c   | 151 +++++++++++++++++++++++++++++++
>  4 files changed, 198 insertions(+)
>  create mode 100644 Documentation/hwmon/sl28cpld.rst
>  create mode 100644 drivers/hwmon/sl28cpld-hwmon.c
> 
> diff --git a/Documentation/hwmon/sl28cpld.rst b/Documentation/hwmon/sl28cpld.rst
> new file mode 100644
> index 000000000000..7ed65f78250c
> --- /dev/null
> +++ b/Documentation/hwmon/sl28cpld.rst
> @@ -0,0 +1,36 @@
> +.. SPDX-License-Identifier: GPL-2.0-only
> +
> +Kernel driver sl28cpld
> +======================
> +
> +Supported chips:
> +
> +   * Kontron sl28cpld
> +
> +     Prefix: 'sl28cpld'
> +
> +     Datasheet: not available
> +
> +Authors: Michael Walle <michael@walle.cc>
> +
> +Description
> +-----------
> +
> +The sl28cpld is a board management controller which also exposes a hardware
> +monitoring controller. At the moment this controller supports a single fan
> +supervisor. In the future there might be other flavours and additional
> +hardware monitoring might be supported.
> +
> +The fan supervisor has a 7 bit counter register and a counter period of 1
> +second. If the 7 bit counter overflows, the supervisor will automatically
> +switch to x8 mode to support a wider input range at the loss of
> +granularity.
> +
> +Sysfs entries
> +-------------
> +
> +The following attributes are supported.
> +
> +======================= ========================================================
> +fan1_input		Fan RPM. Assuming 2 pulses per revolution.
> +======================= ========================================================
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 05a30832c6ba..c98716f78cfa 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1412,6 +1412,16 @@ config SENSORS_RASPBERRYPI_HWMON
>  	  This driver can also be built as a module. If so, the module
>  	  will be called raspberrypi-hwmon.
>  
> +config SENSORS_SL28CPLD
> +	tristate "Kontron's SMARC-sAL28 hardware monitoring driver"
> +	depends on MFD_SL28CPLD
> +	help
> +	  If you say yes here you get support for a fan connected to the
> +	  input of the SMARC connector of Kontron's SMARC-sAL28 module.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called sl28cpld-hwmon.
> +
>  config SENSORS_SHT15
>  	tristate "Sensiron humidity and temperature sensors. SHT15 and compat."
>  	depends on GPIOLIB || COMPILE_TEST
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index b0b9c8e57176..dfb0f8cda2dd 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -155,6 +155,7 @@ obj-$(CONFIG_SENSORS_S3C)	+= s3c-hwmon.o
>  obj-$(CONFIG_SENSORS_SCH56XX_COMMON)+= sch56xx-common.o
>  obj-$(CONFIG_SENSORS_SCH5627)	+= sch5627.o
>  obj-$(CONFIG_SENSORS_SCH5636)	+= sch5636.o
> +obj-$(CONFIG_SENSORS_SL28CPLD)	+= sl28cpld-hwmon.o
>  obj-$(CONFIG_SENSORS_SHT15)	+= sht15.o
>  obj-$(CONFIG_SENSORS_SHT21)	+= sht21.o
>  obj-$(CONFIG_SENSORS_SHT3x)	+= sht3x.o
> diff --git a/drivers/hwmon/sl28cpld-hwmon.c b/drivers/hwmon/sl28cpld-hwmon.c
> new file mode 100644
> index 000000000000..c79bdfed8332
> --- /dev/null
> +++ b/drivers/hwmon/sl28cpld-hwmon.c
> @@ -0,0 +1,151 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld hardware monitoring driver.
> + *
> + * Copyright 2019 Kontron Europe GmbH
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/hwmon.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +
> +#define FAN_INPUT		0x00
> +#define   FAN_SCALE_X8		BIT(7)
> +#define   FAN_VALUE_MASK	GENMASK(6, 0)
> +
> +struct sl28cpld_hwmon {
> +	struct regmap *regmap;
> +	u32 offset;
> +};
> +
> +static umode_t sl28cpld_hwmon_is_visible(const void *data,
> +					 enum hwmon_sensor_types type,
> +					 u32 attr, int channel)
> +{
> +	return 0444;
> +}
> +
> +static int sl28cpld_hwmon_read(struct device *dev,
> +			       enum hwmon_sensor_types type, u32 attr,
> +			       int channel, long *input)
> +{
> +	struct sl28cpld_hwmon *hwmon = dev_get_drvdata(dev);
> +	unsigned int value;
> +	int ret;
> +
> +	switch (attr) {
> +	case hwmon_fan_input:
> +		ret = regmap_read(hwmon->regmap, hwmon->offset + FAN_INPUT,
> +				  &value);
> +		if (ret)
> +			return ret;
> +		/*
> +		 * The register has a 7 bit value and 1 bit which indicates the
> +		 * scale. If the MSB is set, then the lower 7 bit has to be
> +		 * multiplied by 8, to get the correct reading.
> +		 */
> +		if (value & FAN_SCALE_X8)
> +			value = FIELD_GET(FAN_VALUE_MASK, value) << 3;
> +
> +		/*
> +		 * The counter period is 1000ms and the sysfs specification
> +		 * says we should asssume 2 pulses per revolution.
> +		 */
> +		value *= 60 / 2;
> +
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	*input = value;
> +	return 0;
> +}
> +
> +static const u32 sl28cpld_hwmon_fan_config[] = {
> +	HWMON_F_INPUT,
> +	0
> +};
> +
> +static const struct hwmon_channel_info sl28cpld_hwmon_fan = {
> +	.type = hwmon_fan,
> +	.config = sl28cpld_hwmon_fan_config,
> +};
> +
> +static const struct hwmon_channel_info *sl28cpld_hwmon_info[] = {
> +	&sl28cpld_hwmon_fan,
> +	NULL
> +};
> +
> +static const struct hwmon_ops sl28cpld_hwmon_ops = {
> +	.is_visible = sl28cpld_hwmon_is_visible,
> +	.read = sl28cpld_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info sl28cpld_hwmon_chip_info = {
> +	.ops = &sl28cpld_hwmon_ops,
> +	.info = sl28cpld_hwmon_info,
> +};
> +
> +static int sl28cpld_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct device *hwmon_dev;
> +	struct sl28cpld_hwmon *hwmon;
> +	struct resource *res;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +
> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
> +	if (!hwmon)
> +		return -ENOMEM;
> +
> +	hwmon->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!hwmon->regmap)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> +	if (!res)
> +		return -EINVAL;
> +	hwmon->offset = res->start;
> +
> +	hwmon_dev = devm_hwmon_device_register_with_info(&pdev->dev,
> +				"sl28cpld_hwmon", hwmon,
> +				&sl28cpld_hwmon_chip_info, NULL);
> +	if (IS_ERR(hwmon_dev)) {
> +		dev_err(&pdev->dev, "failed to register as hwmon device");
> +		return PTR_ERR(hwmon_dev);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sl28cpld_hwmon_of_match[] = {
> +	{ .compatible = "kontron,sl28cpld-fan" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sl28cpld_hwmon_of_match);
> +
> +static const struct platform_device_id sl28cpld_hwmon_id_table[] = {
> +	{ "sl28cpld-fan" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(platform, sl28cpld_hwmon_id_table);
> +
> +static struct platform_driver sl28cpld_hwmon_driver = {
> +	.probe = sl28cpld_hwmon_probe,
> +	.id_table = sl28cpld_hwmon_id_table,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = sl28cpld_hwmon_of_match,
> +	},
> +};
> +module_platform_driver(sl28cpld_hwmon_driver);
> +
> +MODULE_DESCRIPTION("sl28cpld Hardware Monitoring Driver");
> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> +MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog
  2020-04-02 20:36 ` [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog Michael Walle
@ 2020-04-03  6:25   ` Guenter Roeck
  0 siblings, 0 replies; 38+ messages in thread
From: Guenter Roeck @ 2020-04-03  6:25 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Lee Jones,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown, Greg Kroah-Hartman

On Thu, Apr 02, 2020 at 10:36:48PM +0200, Michael Walle wrote:
> This adds support for the watchdog of the sl28cpld board management
> controller. This is part of a multi-function device driver.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
>  drivers/watchdog/Kconfig        |  11 ++
>  drivers/watchdog/Makefile       |   1 +
>  drivers/watchdog/sl28cpld_wdt.c | 242 ++++++++++++++++++++++++++++++++
>  3 files changed, 254 insertions(+)
>  create mode 100644 drivers/watchdog/sl28cpld_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0663c604bd64..6c53c1d0f348 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -340,6 +340,17 @@ config MLX_WDT
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called mlx-wdt.
>  
> +config SL28CPLD_WATCHDOG
> +	tristate "Kontron sl28 watchdog"
> +	depends on MFD_SL28CPLD
> +	select WATCHDOG_CORE
> +	help
> +	  Say Y here to include support for the watchdog timer
> +	  on the Kontron sl28 CPLD.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called sl28cpld_wdt.
> +
>  # ALPHA Architecture
>  
>  # ARM Architecture
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 6de2e4ceef19..b9ecdf2d7347 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -224,3 +224,4 @@ obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o
>  obj-$(CONFIG_MENZ069_WATCHDOG) += menz69_wdt.o
>  obj-$(CONFIG_RAVE_SP_WATCHDOG) += rave-sp-wdt.o
>  obj-$(CONFIG_STPMIC1_WATCHDOG) += stpmic1_wdt.o
> +obj-$(CONFIG_SL28CPLD_WATCHDOG) += sl28cpld_wdt.o
> diff --git a/drivers/watchdog/sl28cpld_wdt.c b/drivers/watchdog/sl28cpld_wdt.c
> new file mode 100644
> index 000000000000..79a7e36217a6
> --- /dev/null
> +++ b/drivers/watchdog/sl28cpld_wdt.c
> @@ -0,0 +1,242 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld watchdog driver.
> + *
> + * Copyright 2019 Kontron Europe GmbH
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/watchdog.h>
> +
> +/*
> + * Watchdog timer block registers.
> + */
> +#define WDT_CTRL			0x00
> +#define  WDT_CTRL_EN			BIT(0)
> +#define  WDT_CTRL_LOCK			BIT(2)
> +#define  WDT_CTRL_ASSERT_SYS_RESET	BIT(6)
> +#define  WDT_CTRL_ASSERT_WDT_TIMEOUT	BIT(7)
> +#define WDT_TIMEOUT			0x01
> +#define WDT_KICK			0x02
> +#define  WDT_KICK_VALUE			0x6b
> +#define WDT_COUNT			0x03
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +				__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static int timeout;
> +module_param(timeout, int, 0);
> +MODULE_PARM_DESC(timeout, "Initial watchdog timeout in seconds");
> +
> +struct sl28cpld_wdt {
> +	struct watchdog_device wdd;
> +	struct regmap *regmap;
> +	u32 offset;
> +	bool assert_wdt_timeout;
> +};
> +
> +static int sl28cpld_wdt_ping(struct watchdog_device *wdd)
> +{
> +	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	return regmap_write(wdt->regmap, wdt->offset + WDT_KICK,
> +			    WDT_KICK_VALUE);
> +}
> +
> +static int sl28cpld_wdt_start(struct watchdog_device *wdd)
> +{
> +	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
> +	unsigned int val;
> +
> +	val = WDT_CTRL_EN | WDT_CTRL_ASSERT_SYS_RESET;
> +	if (wdt->assert_wdt_timeout)
> +		val |= WDT_CTRL_ASSERT_WDT_TIMEOUT;
> +	if (nowayout)
> +		val |= WDT_CTRL_LOCK;
> +
> +	return regmap_update_bits(wdt->regmap, wdt->offset + WDT_CTRL,
> +				  val, val);
> +}
> +
> +static int sl28cpld_wdt_stop(struct watchdog_device *wdd)
> +{
> +	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
> +
> +	return regmap_update_bits(wdt->regmap, wdt->offset + WDT_CTRL,
> +				  WDT_CTRL_EN, 0);
> +}
> +
> +static unsigned int sl28cpld_wdt_status(struct watchdog_device *wdd)
> +{
> +	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
> +	unsigned int status;
> +	int ret;
> +
> +	ret = regmap_read(wdt->regmap, wdt->offset + WDT_CTRL, &status);
> +	if (ret < 0)
> +		return 0;
> +
> +	/* is the watchdog timer running? */
> +	return (status & WDT_CTRL_EN) << WDOG_ACTIVE;

This is really bad coding style. It uses the fact that WDT_CTRL_EN is
at bit position 0 and sets WDOG_ACTIVE accordingly.

But that it is wrong, not even considering the coding style problem.
The status function is supposed to return WDIOF_ bits. What it returns
if the watchdog is running is WDOG_ACTIVE, or BIT(0), which is then
reported to userspace as WDIOF_OVERHEAT.

> +}
> +
> +static unsigned int sl28cpld_wdt_get_timeleft(struct watchdog_device *wdd)
> +{
> +	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(wdt->regmap, wdt->offset + WDT_COUNT, &val);
> +	if (ret < 0)
> +		return 0;
> +
> +	return val;
> +}
> +
> +static int sl28cpld_wdt_set_timeout(struct watchdog_device *wdd,
> +				  unsigned int timeout)
> +{
> +	int ret;
> +	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);

Nit: Reverse christmas tree order looks a bit nicer.

> +
> +	ret = regmap_write(wdt->regmap, wdt->offset + WDT_TIMEOUT, timeout);
> +	if (ret == 0)

Please run checkpatch --strict and fix this and the reported alignment
problem.

> +		wdd->timeout = timeout;
> +
> +	return ret;
> +}
> +
> +static const struct watchdog_info sl28cpld_wdt_info = {
> +	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.identity = "SMARC-sAL28 CPLD watchdog",
> +};
> +
> +static struct watchdog_ops sl28cpld_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = sl28cpld_wdt_start,
> +	.stop = sl28cpld_wdt_stop,
> +	.status = sl28cpld_wdt_status,
> +	.ping = sl28cpld_wdt_ping,
> +	.set_timeout = sl28cpld_wdt_set_timeout,
> +	.get_timeleft = sl28cpld_wdt_get_timeleft,
> +};
> +
> +static int sl28cpld_wdt_locked(struct sl28cpld_wdt *wdt)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(wdt->regmap, wdt->offset + WDT_CTRL, &val);
> +	if (ret < 0)
> +		return ret;
> +
> +	return val & WDT_CTRL_LOCK;
> +}
> +
> +static int sl28cpld_wdt_probe(struct platform_device *pdev)
> +{
> +	struct sl28cpld_wdt *wdt;
> +	struct watchdog_device *wdd;
> +	struct resource *res;
> +	unsigned int val;
> +	int ret;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +
> +	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> +	if (!wdt)
> +		return -ENOMEM;
> +
> +	wdt->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!wdt->regmap)
> +		return -ENODEV;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_REG, 0);
> +	if (res == NULL)
> +		return -EINVAL;
> +	wdt->offset = res->start;
> +
> +	if (device_property_read_bool(&pdev->dev,
> +				      "kontron,assert-wdt-timeout-pin"))
> +		wdt->assert_wdt_timeout = true;

This might be simpler written as
	wdt->assert_wdt_timeout = device_property_read_bool(...);

> +
> +	/* initialize struct watchdog_device */
> +	wdd = &wdt->wdd;
> +	wdd->parent = &pdev->dev;
> +	wdd->info = &sl28cpld_wdt_info;
> +	wdd->ops = &sl28cpld_wdt_ops;
> +	wdd->min_timeout = 1;
> +	wdd->max_timeout = 255;
> +
> +	watchdog_set_drvdata(wdd, wdt);
> +
> +	/* if the watchdog is locked, we set nowayout to true */
> +	ret = sl28cpld_wdt_locked(wdt);
> +	if (ret < 0)
> +		return ret;
> +	if (ret)
> +		nowayout = true;
> +	watchdog_set_nowayout(wdd, nowayout);
> +
> +	/*
> +	 * Initial timeout value, can either be set by kernel parameter or by
> +	 * the device tree. If both are not given the current value is used.
> +	 */
> +	watchdog_init_timeout(wdd, timeout, &pdev->dev);
> +	if (wdd->timeout) {
> +		sl28cpld_wdt_set_timeout(wdd, wdd->timeout);
> +	} else {
> +		ret = regmap_read(wdt->regmap, wdt->offset + WDT_TIMEOUT,
> +				  &val);
> +		if (ret < 0)
> +			return ret;
> +		wdd->timeout = val;

Oddly enough that can result in a timeout of 0 if that is what the chip
reports. Are you sure that is acceptable ?

> +	}
> +
> +	watchdog_stop_on_reboot(wdd);
> +	ret = devm_watchdog_register_device(&pdev->dev, wdd);

This does not inform the watchdog core if the watchdog is already active,
even though that is clearly supported. You might want to consider setting
WDOG_HW_RUNNING in that case.

> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "failed to register watchdog device\n");
> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, wdt);
> +

I don't see where this is used.

> +	dev_info(&pdev->dev, "CPLD watchdog: initial timeout %d sec%s\n",
> +		wdd->timeout, nowayout ? ", nowayout" : "");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id sl28cpld_wdt_of_match[] = {
> +	{ .compatible = "kontron,sl28cpld-wdt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, sl28cpld_wdt_of_match);
> +
> +static const struct platform_device_id sl28cpld_wdt_id_table[] = {
> +	{ "sl28cpld-wdt" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, sl28cpld_wdt_id_table);
> +
> +static struct platform_driver sl28cpld_wdt_driver = {
> +	.probe = sl28cpld_wdt_probe,
> +	.id_table = sl28cpld_wdt_id_table,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +		.of_match_table = sl28cpld_wdt_of_match,
> +	},
> +};
> +module_platform_driver(sl28cpld_wdt_driver);
> +
> +MODULE_DESCRIPTION("sl28cpld Watchdog Driver");
> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> +MODULE_LICENSE("GPL");

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-02 20:36 ` [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap Michael Walle
@ 2020-04-06  7:47   ` Bartosz Golaszewski
  2020-04-06 10:10     ` Michael Walle
  2020-04-16  9:27   ` Linus Walleij
  1 sibling, 1 reply; 38+ messages in thread
From: Bartosz Golaszewski @ 2020-04-06  7:47 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a):
>
> There are quite a lot simple GPIO controller which are using regmap to
> access the hardware. This driver tries to be a base to unify existing
> code into one place. This won't cover everything but it should be a good
> starting point.
>
> It does not implement its own irq_chip because there is already a
> generic one for regmap based devices. Instead, the irq_chip will be
> instanciated in the parent driver and its irq domain will be associate
> to this driver.
>
> For now it consists of the usual registers, like set (and an optional
> clear) data register, an input register and direction registers.
> Out-of-the-box, it supports consecutive register mappings and mappings
> where the registers have gaps between them with a linear mapping between
> GPIO offset and bit position. For weirder mappings the user can register
> its own .xlate().
>
> Signed-off-by: Michael Walle <michael@walle.cc>

Hi Michael,

Thanks for doing this! When looking at other generic drivers:
gpio-mmio and gpio-reg I can see there are some corner-cases and more
specific configuration options we could add but it's not a blocker,
we'll probably be extending this one as we convert more drivers to
using it. Personally I'd love to see gpio-mmio and gpio-reg removed
and replaced by a single, generic regmap interface eventually.

> ---
>  drivers/gpio/Kconfig        |   4 +
>  drivers/gpio/Makefile       |   1 +
>  drivers/gpio/gpio-regmap.c  | 320 ++++++++++++++++++++++++++++++++++++
>  include/linux/gpio-regmap.h |  88 ++++++++++
>  4 files changed, 413 insertions(+)
>  create mode 100644 drivers/gpio/gpio-regmap.c
>  create mode 100644 include/linux/gpio-regmap.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 1b96169d84f7..a8e148f4b2e0 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -73,6 +73,10 @@ config GPIO_GENERIC
>         depends on HAS_IOMEM # Only for IOMEM drivers
>         tristate
>
> +config GPIO_REGMAP
> +       depends on REGMAP
> +       tristate
> +
>  # put drivers in the right section, in alphabetical order
>
>  # This symbol is selected by both I2C and SPI expanders
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index b2cfc21a97f3..93e139fdfa57 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_SYSFS)      += gpiolib-sysfs.o
>  obj-$(CONFIG_GPIO_ACPI)                += gpiolib-acpi.o
>
>  # Device drivers. Generally keep list sorted alphabetically
> +obj-$(CONFIG_GPIO_REGMAP)      += gpio-regmap.o
>  obj-$(CONFIG_GPIO_GENERIC)     += gpio-generic.o
>
>  # directly supported by gpio-generic
> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
> new file mode 100644
> index 000000000000..cc4437dc0521
> --- /dev/null
> +++ b/drivers/gpio/gpio-regmap.c
> @@ -0,0 +1,320 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * regmap based generic GPIO driver
> + *
> + * Copyright 2019 Michael Walle <michael@walle.cc>
> + */
> +
> +#include <linux/gpio/driver.h>
> +#include <linux/gpio-regmap.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +struct gpio_regmap_data {
> +       struct gpio_chip gpio_chip;
> +       struct gpio_regmap *gpio;
> +};
> +
> +/**
> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
> + *
> + * Use a simple linear mapping to translate the offset to the bitmask.
> + */
> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base,
> +                            unsigned int offset,
> +                            unsigned int *reg, unsigned int *mask)
> +{
> +       unsigned int line = offset % gpio->ngpio_per_reg;
> +       unsigned int stride = offset / gpio->ngpio_per_reg;
> +
> +       *reg = base + stride * gpio->reg_stride;
> +       *mask = BIT(line);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate);

Why does this need to be exported?

> +
> +static int gpio_regmap_get(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +       unsigned int base;
> +       unsigned int val, reg, mask;

This can fit on a single line with base. Same elsewhere.

> +       int ret;
> +
> +       /* we might not have an output register if we are input only */
> +       if (gpio->reg_dat_base.valid)
> +               base = gpio->reg_dat_base.addr;
> +       else
> +               base = gpio->reg_set_base.addr;
> +
> +       ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_read(gpio->regmap, reg, &val);
> +       if (ret)
> +               return ret;
> +
> +       return (val & mask) ? 1 : 0;
> +}
> +
> +static void gpio_regmap_set(struct gpio_chip *chip, unsigned int offset,
> +                           int val)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +       unsigned int base = gpio->reg_set_base.addr;
> +       unsigned int reg, mask;
> +
> +       gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +       if (val)
> +               regmap_update_bits(gpio->regmap, reg, mask, mask);
> +       else
> +               regmap_update_bits(gpio->regmap, reg, mask, 0);
> +}
> +
> +static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
> +                                      unsigned int offset, int val)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +       unsigned int base;
> +       unsigned int reg, mask;
> +
> +       if (val)
> +               base = gpio->reg_set_base.addr;
> +       else
> +               base = gpio->reg_clr_base.addr;
> +
> +       gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +       regmap_write(gpio->regmap, reg, mask);
> +}
> +
> +static int gpio_regmap_get_direction(struct gpio_chip *chip,
> +                                    unsigned int offset)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +       unsigned int val, reg, mask;
> +       unsigned int base;
> +       int invert;
> +       int ret;
> +
> +       if (gpio->reg_dir_out_base.valid) {
> +               base = gpio->reg_dir_out_base.addr;
> +               invert = 0;
> +       } else if (gpio->reg_dir_in_base.valid) {
> +               base = gpio->reg_dir_in_base.addr;
> +               invert = 1;
> +       } else {
> +               return GPIO_LINE_DIRECTION_IN;
> +       }
> +
> +       ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +       if (ret)
> +               return ret;
> +
> +       ret = regmap_read(gpio->regmap, reg, &val);
> +       if (ret)
> +               return ret;
> +
> +       if (!!(val & mask) ^ invert)
> +               return GPIO_LINE_DIRECTION_OUT;
> +       else
> +               return GPIO_LINE_DIRECTION_IN;
> +}
> +
> +static int gpio_regmap_set_direction(struct gpio_chip *chip,
> +                                    unsigned int offset, bool output)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +       unsigned int val, reg, mask;
> +       unsigned int base;
> +       int invert;
> +       int ret;
> +
> +       if (gpio->reg_dir_out_base.valid) {
> +               base = gpio->reg_dir_out_base.addr;
> +               invert = 0;
> +       } else if (gpio->reg_dir_in_base.valid) {
> +               base = gpio->reg_dir_in_base.addr;
> +               invert = 1;
> +       } else {
> +               return 0;
> +       }
> +
> +       ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
> +       if (ret)
> +               return ret;
> +
> +       if (!invert)
> +               val = (output) ? mask : 0;
> +       else
> +               val = (output) ? 0 : mask;
> +
> +       return regmap_update_bits(gpio->regmap, reg, mask, val);
> +}
> +
> +static int gpio_regmap_direction_input(struct gpio_chip *chip,
> +                                      unsigned int offset)
> +{
> +       return gpio_regmap_set_direction(chip, offset, false);
> +}
> +
> +static int gpio_regmap_direction_output(struct gpio_chip *chip,
> +                                       unsigned int offset, int value)
> +{
> +       gpio_regmap_set(chip, offset, value);
> +       return gpio_regmap_set_direction(chip, offset, true);
> +}
> +
> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +
> +       /* the user might have its own .to_irq callback */
> +       if (gpio->to_irq)
> +               return gpio->to_irq(gpio, offset);
> +
> +       return irq_create_mapping(gpio->irq_domain, offset);
> +}
> +
> +/**
> + * gpio_regmap_register() - Register a generic regmap GPIO controller
> + *
> + * @gpio: gpio_regmap device to register
> + *
> + * Returns 0 on success or an errno on failure.
> + */
> +int gpio_regmap_register(struct gpio_regmap *gpio)
> +{
> +       struct gpio_regmap_data *d;
> +       struct gpio_chip *chip;
> +       int ret;
> +
> +       if (!gpio->parent)
> +               return -EINVAL;
> +
> +       if (!gpio->ngpio)
> +               return -EINVAL;
> +
> +       /* we need at least one */
> +       if (!gpio->reg_dat_base.valid && !gpio->reg_set_base.valid)
> +               return -EINVAL;
> +
> +       /* we don't support having both registers simulaniously for now */
> +       if (gpio->reg_dir_out_base.valid && gpio->reg_dir_in_base.valid)
> +               return -EINVAL;
> +
> +       /* if not set, assume they are consecutive */
> +       if (!gpio->reg_stride)
> +               gpio->reg_stride = 1;
> +
> +       /* if not set, assume there is only one register */
> +       if (!gpio->ngpio_per_reg)
> +               gpio->ngpio_per_reg = gpio->ngpio;
> +
> +       if (!gpio->reg_mask_xlate)
> +               gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
> +
> +       d = kzalloc(sizeof(*d), GFP_KERNEL);
> +       if (!d)
> +               return -ENOMEM;
> +
> +       gpio->data = d;
> +       d->gpio = gpio;
> +
> +       chip = &d->gpio_chip;
> +       chip->parent = gpio->parent;
> +       chip->label = gpio->label;
> +       chip->base = -1;
> +       chip->ngpio = gpio->ngpio;
> +       chip->can_sleep = true;
> +       chip->get = gpio_regmap_get;
> +
> +       if (!chip->label)
> +               chip->label = dev_name(gpio->parent);
> +
> +       if (gpio->reg_set_base.valid && gpio->reg_clr_base.valid)
> +               chip->set = gpio_regmap_set_with_clear;
> +       else if (gpio->reg_set_base.valid)
> +               chip->set = gpio_regmap_set;
> +
> +       if (gpio->reg_dir_in_base.valid || gpio->reg_dir_out_base.valid) {
> +               chip->get_direction = gpio_regmap_get_direction;
> +               chip->direction_input = gpio_regmap_direction_input;
> +               chip->direction_output = gpio_regmap_direction_output;
> +       }
> +
> +       if (gpio->irq_domain)
> +               chip->to_irq = gpio_regmap_to_irq;
> +
> +       ret = gpiochip_add_data(chip, d);
> +       if (ret < 0)
> +               goto err_alloc;
> +
> +       return 0;
> +
> +err_alloc:
> +       kfree(d);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(gpio_regmap_register);
> +
> +/**
> + * gpio_regmap_unregister() - Unregister a generic regmap GPIO controller
> + *
> + * @gpio: gpio_regmap device to unregister
> + */
> +void gpio_regmap_unregister(struct gpio_regmap *gpio)
> +{
> +       gpiochip_remove(&gpio->data->gpio_chip);
> +       kfree(gpio->data);
> +}
> +EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
> +
> +static void devm_gpio_regmap_unregister(struct device *dev, void *res)
> +{
> +       gpio_regmap_unregister(*(struct gpio_regmap **)res);
> +}
> +
> +/**
> + * devm_gpio_regmap_register() - resource managed gpio_regmap_register()
> + *
> + * @dev: device that is registering this GPIO device
> + * @gpio: gpio_regmap device to register
> + *
> + * Managed gpio_regmap_register(). For generic regmap GPIO device registered by
> + * this function, gpio_regmap_unregister() is automatically called on driver
> + * detach. See gpio_regmap_register() for more information.
> + */
> +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio)
> +{
> +       struct gpio_regmap **ptr;
> +       int ret;
> +
> +       ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
> +                          GFP_KERNEL);
> +       if (!ptr)
> +               return -ENOMEM;
> +
> +       ret = gpio_regmap_register(gpio);
> +       if (ret) {
> +               devres_free(ptr);
> +               return ret;
> +       }
> +
> +       *ptr = gpio;
> +       devres_add(dev, ptr);
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_gpio_regmap_register);
> +
> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> +MODULE_DESCRIPTION("GPIO generic regmap driver core");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
> new file mode 100644
> index 000000000000..ad63955e0e43
> --- /dev/null
> +++ b/include/linux/gpio-regmap.h
> @@ -0,0 +1,88 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef _LINUX_GPIO_REGMAP_H
> +#define _LINUX_GPIO_REGMAP_H
> +
> +struct gpio_regmap_addr {
> +       unsigned int addr;
> +       bool valid;
> +};

I'm not quite sure what the meaning behind the valid field here is.
When would we potentially set it to false?

> +#define GPIO_REGMAP_ADDR(_addr) \
> +       ((struct gpio_regmap_addr) { .addr = _addr, .valid = true })
> +
> +/**
> + * struct gpio_regmap - Description of a generic regmap gpio_chip.
> + *
> + * @parent:            The parent device
> + * @regmap:            The regmap use to access the registers

s/use/used/

> + *                     given, the name of the device is used
> + * @label:             (Optional) Descriptive name for GPIO controller.
> + *                     If not given, the name of the device is used.
> + * @ngpio:             Number of GPIOs
> + * @reg_dat_base:      (Optional) (in) register base address
> + * @reg_set_base:      (Optional) set register base address
> + * @reg_clr_base:      (Optional) clear register base address
> + * @reg_dir_in_base:   (Optional) out setting register base address
> + * @reg_dir_out_base:  (Optional) in setting register base address
> + * @reg_stride:                (Optional) May be set if the registers (of the
> + *                     same type, dat, set, etc) are not consecutive.
> + * @ngpio_per_reg:     Number of GPIOs per register
> + * @irq_domain:                (Optional) IRQ domain if the controller is
> + *                     interrupt-capable
> + * @reg_mask_xlate:     (Optional) Translates base address and GPIO
> + *                     offset to a register/bitmask pair. If not
> + *                     given the default gpio_regmap_simple_xlate()
> + *                     is used.
> + * @to_irq:            (Optional) Maps GPIO offset to a irq number.
> + *                     By default assumes a linear mapping of the
> + *                     given irq_domain.
> + * @driver_data:       Pointer to the drivers private data. Not used by
> + *                     gpio-regmap.
> + *
> + * The reg_mask_xlate translates a given base address and GPIO offset to
> + * register and mask pair. The base address is one of the given reg_*_base.
> + */
> +struct gpio_regmap {

I'd prefer to follow a pattern seen in other such APIs of calling this
structure gpio_regmap_config and creating another private structure
called gpio_regmap used in callbacks that would only contain necessary
fields.

> +       struct device *parent;
> +       struct regmap *regmap;
> +       struct gpio_regmap_data *data;
> +
> +       const char *label;
> +       int ngpio;
> +
> +       struct gpio_regmap_addr reg_dat_base;
> +       struct gpio_regmap_addr reg_set_base;
> +       struct gpio_regmap_addr reg_clr_base;
> +       struct gpio_regmap_addr reg_dir_in_base;
> +       struct gpio_regmap_addr reg_dir_out_base;
> +       int reg_stride;
> +       int ngpio_per_reg;
> +       struct irq_domain *irq_domain;
> +
> +       int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
> +                             unsigned int offset, unsigned int *reg,
> +                             unsigned int *mask);
> +       int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset);
> +
> +       void *driver_data;
> +};
> +
> +static inline void gpio_regmap_set_drvdata(struct gpio_regmap *gpio,
> +                                          void *data)
> +{
> +       gpio->driver_data = data;
> +}
> +
> +static inline void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
> +{
> +       return gpio->driver_data;
> +}
> +
> +int gpio_regmap_register(struct gpio_regmap *gpio);
> +void gpio_regmap_unregister(struct gpio_regmap *gpio);
> +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap *gpio);
> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int base,
> +                            unsigned int offset,
> +                            unsigned int *reg, unsigned int *mask);
> +
> +#endif /* _LINUX_GPIO_REGMAP_H */
> --
> 2.20.1
>

Overall looks really nice. I'm happy we'll have it in v5.8.

Best regards,
Bartosz Golaszewski

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-06  7:47   ` Bartosz Golaszewski
@ 2020-04-06 10:10     ` Michael Walle
  2020-04-14  9:50       ` Bartosz Golaszewski
  0 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-06 10:10 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman


Hi Bartosz, Hi Mark Brown,

Am 2020-04-06 09:47, schrieb Bartosz Golaszewski:
> czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a):
>> 
>> There are quite a lot simple GPIO controller which are using regmap to
>> access the hardware. This driver tries to be a base to unify existing
>> code into one place. This won't cover everything but it should be a 
>> good
>> starting point.
>> 
>> It does not implement its own irq_chip because there is already a
>> generic one for regmap based devices. Instead, the irq_chip will be
>> instanciated in the parent driver and its irq domain will be associate
>> to this driver.
>> 
>> For now it consists of the usual registers, like set (and an optional
>> clear) data register, an input register and direction registers.
>> Out-of-the-box, it supports consecutive register mappings and mappings
>> where the registers have gaps between them with a linear mapping 
>> between
>> GPIO offset and bit position. For weirder mappings the user can 
>> register
>> its own .xlate().
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> Hi Michael,
> 
> Thanks for doing this! When looking at other generic drivers:
> gpio-mmio and gpio-reg I can see there are some corner-cases and more
> specific configuration options we could add

I didn't want to copy every bit without being able to test it.

> but it's not a blocker,
> we'll probably be extending this one as we convert more drivers to
> using it.

correct, that was also my plan.

> Personally I'd love to see gpio-mmio and gpio-reg removed
> and replaced by a single, generic regmap interface eventually.

agreed.


>> ---
>>  drivers/gpio/Kconfig        |   4 +
>>  drivers/gpio/Makefile       |   1 +
>>  drivers/gpio/gpio-regmap.c  | 320 
>> ++++++++++++++++++++++++++++++++++++
>>  include/linux/gpio-regmap.h |  88 ++++++++++
>>  4 files changed, 413 insertions(+)
>>  create mode 100644 drivers/gpio/gpio-regmap.c
>>  create mode 100644 include/linux/gpio-regmap.h
>> 
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
>> index 1b96169d84f7..a8e148f4b2e0 100644
>> --- a/drivers/gpio/Kconfig
>> +++ b/drivers/gpio/Kconfig
>> @@ -73,6 +73,10 @@ config GPIO_GENERIC
>>         depends on HAS_IOMEM # Only for IOMEM drivers
>>         tristate
>> 
>> +config GPIO_REGMAP
>> +       depends on REGMAP
>> +       tristate
>> +
>>  # put drivers in the right section, in alphabetical order
>> 
>>  # This symbol is selected by both I2C and SPI expanders
>> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
>> index b2cfc21a97f3..93e139fdfa57 100644
>> --- a/drivers/gpio/Makefile
>> +++ b/drivers/gpio/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_GPIO_SYSFS)      += gpiolib-sysfs.o
>>  obj-$(CONFIG_GPIO_ACPI)                += gpiolib-acpi.o
>> 
>>  # Device drivers. Generally keep list sorted alphabetically
>> +obj-$(CONFIG_GPIO_REGMAP)      += gpio-regmap.o
>>  obj-$(CONFIG_GPIO_GENERIC)     += gpio-generic.o
>> 
>>  # directly supported by gpio-generic
>> diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c
>> new file mode 100644
>> index 000000000000..cc4437dc0521
>> --- /dev/null
>> +++ b/drivers/gpio/gpio-regmap.c
>> @@ -0,0 +1,320 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * regmap based generic GPIO driver
>> + *
>> + * Copyright 2019 Michael Walle <michael@walle.cc>
>> + */
>> +
>> +#include <linux/gpio/driver.h>
>> +#include <linux/gpio-regmap.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +
>> +struct gpio_regmap_data {
>> +       struct gpio_chip gpio_chip;
>> +       struct gpio_regmap *gpio;
>> +};
>> +
>> +/**
>> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
>> + *
>> + * Use a simple linear mapping to translate the offset to the 
>> bitmask.
>> + */
>> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int 
>> base,
>> +                            unsigned int offset,
>> +                            unsigned int *reg, unsigned int *mask)
>> +{
>> +       unsigned int line = offset % gpio->ngpio_per_reg;
>> +       unsigned int stride = offset / gpio->ngpio_per_reg;
>> +
>> +       *reg = base + stride * gpio->reg_stride;
>> +       *mask = BIT(line);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate);
> 
> Why does this need to be exported?

Mh, the idea was that a user could also set this xlate() by himself (for
whatever reason). But since it is the default, it is not really 
necessary.
That being said, I don't care if its only local to this module.

>> +
>> +static int gpio_regmap_get(struct gpio_chip *chip, unsigned int 
>> offset)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +       unsigned int base;
>> +       unsigned int val, reg, mask;
> 
> This can fit on a single line with base. Same elsewhere.
> 
>> +       int ret;
>> +
>> +       /* we might not have an output register if we are input only 
>> */
>> +       if (gpio->reg_dat_base.valid)
>> +               base = gpio->reg_dat_base.addr;
>> +       else
>> +               base = gpio->reg_set_base.addr;
>> +
>> +       ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = regmap_read(gpio->regmap, reg, &val);
>> +       if (ret)
>> +               return ret;
>> +
>> +       return (val & mask) ? 1 : 0;
>> +}
>> +
>> +static void gpio_regmap_set(struct gpio_chip *chip, unsigned int 
>> offset,
>> +                           int val)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +       unsigned int base = gpio->reg_set_base.addr;
>> +       unsigned int reg, mask;
>> +
>> +       gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>> +       if (val)
>> +               regmap_update_bits(gpio->regmap, reg, mask, mask);
>> +       else
>> +               regmap_update_bits(gpio->regmap, reg, mask, 0);
>> +}
>> +
>> +static void gpio_regmap_set_with_clear(struct gpio_chip *chip,
>> +                                      unsigned int offset, int val)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +       unsigned int base;
>> +       unsigned int reg, mask;
>> +
>> +       if (val)
>> +               base = gpio->reg_set_base.addr;
>> +       else
>> +               base = gpio->reg_clr_base.addr;
>> +
>> +       gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>> +       regmap_write(gpio->regmap, reg, mask);
>> +}
>> +
>> +static int gpio_regmap_get_direction(struct gpio_chip *chip,
>> +                                    unsigned int offset)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +       unsigned int val, reg, mask;
>> +       unsigned int base;
>> +       int invert;
>> +       int ret;
>> +
>> +       if (gpio->reg_dir_out_base.valid) {
>> +               base = gpio->reg_dir_out_base.addr;
>> +               invert = 0;
>> +       } else if (gpio->reg_dir_in_base.valid) {
>> +               base = gpio->reg_dir_in_base.addr;
>> +               invert = 1;
>> +       } else {
>> +               return GPIO_LINE_DIRECTION_IN;
>> +       }
>> +
>> +       ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = regmap_read(gpio->regmap, reg, &val);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (!!(val & mask) ^ invert)
>> +               return GPIO_LINE_DIRECTION_OUT;
>> +       else
>> +               return GPIO_LINE_DIRECTION_IN;
>> +}
>> +
>> +static int gpio_regmap_set_direction(struct gpio_chip *chip,
>> +                                    unsigned int offset, bool output)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +       unsigned int val, reg, mask;
>> +       unsigned int base;
>> +       int invert;
>> +       int ret;
>> +
>> +       if (gpio->reg_dir_out_base.valid) {
>> +               base = gpio->reg_dir_out_base.addr;
>> +               invert = 0;
>> +       } else if (gpio->reg_dir_in_base.valid) {
>> +               base = gpio->reg_dir_in_base.addr;
>> +               invert = 1;
>> +       } else {
>> +               return 0;
>> +       }
>> +
>> +       ret = gpio->reg_mask_xlate(gpio, base, offset, &reg, &mask);
>> +       if (ret)
>> +               return ret;
>> +
>> +       if (!invert)
>> +               val = (output) ? mask : 0;
>> +       else
>> +               val = (output) ? 0 : mask;
>> +
>> +       return regmap_update_bits(gpio->regmap, reg, mask, val);
>> +}
>> +
>> +static int gpio_regmap_direction_input(struct gpio_chip *chip,
>> +                                      unsigned int offset)
>> +{
>> +       return gpio_regmap_set_direction(chip, offset, false);
>> +}
>> +
>> +static int gpio_regmap_direction_output(struct gpio_chip *chip,
>> +                                       unsigned int offset, int 
>> value)
>> +{
>> +       gpio_regmap_set(chip, offset, value);
>> +       return gpio_regmap_set_direction(chip, offset, true);
>> +}
>> +
>> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int 
>> offset)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +
>> +       /* the user might have its own .to_irq callback */
>> +       if (gpio->to_irq)
>> +               return gpio->to_irq(gpio, offset);
>> +
>> +       return irq_create_mapping(gpio->irq_domain, offset);
>> +}
>> +
>> +/**
>> + * gpio_regmap_register() - Register a generic regmap GPIO controller
>> + *
>> + * @gpio: gpio_regmap device to register
>> + *
>> + * Returns 0 on success or an errno on failure.
>> + */
>> +int gpio_regmap_register(struct gpio_regmap *gpio)
>> +{
>> +       struct gpio_regmap_data *d;
>> +       struct gpio_chip *chip;
>> +       int ret;
>> +
>> +       if (!gpio->parent)
>> +               return -EINVAL;
>> +
>> +       if (!gpio->ngpio)
>> +               return -EINVAL;
>> +
>> +       /* we need at least one */
>> +       if (!gpio->reg_dat_base.valid && !gpio->reg_set_base.valid)
>> +               return -EINVAL;
>> +
>> +       /* we don't support having both registers simulaniously for 
>> now */
>> +       if (gpio->reg_dir_out_base.valid && 
>> gpio->reg_dir_in_base.valid)
>> +               return -EINVAL;
>> +
>> +       /* if not set, assume they are consecutive */
>> +       if (!gpio->reg_stride)
>> +               gpio->reg_stride = 1;
>> +
>> +       /* if not set, assume there is only one register */
>> +       if (!gpio->ngpio_per_reg)
>> +               gpio->ngpio_per_reg = gpio->ngpio;
>> +
>> +       if (!gpio->reg_mask_xlate)
>> +               gpio->reg_mask_xlate = gpio_regmap_simple_xlate;
>> +
>> +       d = kzalloc(sizeof(*d), GFP_KERNEL);
>> +       if (!d)
>> +               return -ENOMEM;
>> +
>> +       gpio->data = d;
>> +       d->gpio = gpio;
>> +
>> +       chip = &d->gpio_chip;
>> +       chip->parent = gpio->parent;
>> +       chip->label = gpio->label;
>> +       chip->base = -1;
>> +       chip->ngpio = gpio->ngpio;
>> +       chip->can_sleep = true;
>> +       chip->get = gpio_regmap_get;
>> +
>> +       if (!chip->label)
>> +               chip->label = dev_name(gpio->parent);
>> +
>> +       if (gpio->reg_set_base.valid && gpio->reg_clr_base.valid)
>> +               chip->set = gpio_regmap_set_with_clear;
>> +       else if (gpio->reg_set_base.valid)
>> +               chip->set = gpio_regmap_set;
>> +
>> +       if (gpio->reg_dir_in_base.valid || 
>> gpio->reg_dir_out_base.valid) {
>> +               chip->get_direction = gpio_regmap_get_direction;
>> +               chip->direction_input = gpio_regmap_direction_input;
>> +               chip->direction_output = gpio_regmap_direction_output;
>> +       }
>> +
>> +       if (gpio->irq_domain)
>> +               chip->to_irq = gpio_regmap_to_irq;
>> +
>> +       ret = gpiochip_add_data(chip, d);
>> +       if (ret < 0)
>> +               goto err_alloc;
>> +
>> +       return 0;
>> +
>> +err_alloc:
>> +       kfree(d);
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(gpio_regmap_register);
>> +
>> +/**
>> + * gpio_regmap_unregister() - Unregister a generic regmap GPIO 
>> controller
>> + *
>> + * @gpio: gpio_regmap device to unregister
>> + */
>> +void gpio_regmap_unregister(struct gpio_regmap *gpio)
>> +{
>> +       gpiochip_remove(&gpio->data->gpio_chip);
>> +       kfree(gpio->data);
>> +}
>> +EXPORT_SYMBOL_GPL(gpio_regmap_unregister);
>> +
>> +static void devm_gpio_regmap_unregister(struct device *dev, void 
>> *res)
>> +{
>> +       gpio_regmap_unregister(*(struct gpio_regmap **)res);
>> +}
>> +
>> +/**
>> + * devm_gpio_regmap_register() - resource managed 
>> gpio_regmap_register()
>> + *
>> + * @dev: device that is registering this GPIO device
>> + * @gpio: gpio_regmap device to register
>> + *
>> + * Managed gpio_regmap_register(). For generic regmap GPIO device 
>> registered by
>> + * this function, gpio_regmap_unregister() is automatically called on 
>> driver
>> + * detach. See gpio_regmap_register() for more information.
>> + */
>> +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap 
>> *gpio)
>> +{
>> +       struct gpio_regmap **ptr;
>> +       int ret;
>> +
>> +       ptr = devres_alloc(devm_gpio_regmap_unregister, sizeof(*ptr),
>> +                          GFP_KERNEL);
>> +       if (!ptr)
>> +               return -ENOMEM;
>> +
>> +       ret = gpio_regmap_register(gpio);
>> +       if (ret) {
>> +               devres_free(ptr);
>> +               return ret;
>> +       }
>> +
>> +       *ptr = gpio;
>> +       devres_add(dev, ptr);
>> +
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_gpio_regmap_register);
>> +
>> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
>> +MODULE_DESCRIPTION("GPIO generic regmap driver core");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
>> new file mode 100644
>> index 000000000000..ad63955e0e43
>> --- /dev/null
>> +++ b/include/linux/gpio-regmap.h
>> @@ -0,0 +1,88 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +
>> +#ifndef _LINUX_GPIO_REGMAP_H
>> +#define _LINUX_GPIO_REGMAP_H
>> +
>> +struct gpio_regmap_addr {
>> +       unsigned int addr;
>> +       bool valid;
>> +};
> 
> I'm not quite sure what the meaning behind the valid field here is.
> When would we potentially set it to false?

Some base addresses are optional, but on the other hand, a base address
of 0 could also be valid. So I cannot use 0 as an indicator whether a
base address is set or not. The generic mmio driver has some special
case for the ack base, where there is a use_ack flag which forces to
use the ack register even if its zero. So I've had a look at the kernel
if there is a better idiom for that, but I haven't found anything.

So the best from a user perspective I've could come up with was:

   ->base_reg = GPIO_REGMAP_ADDR(addr);

I'm open for suggestions.

> 
>> +#define GPIO_REGMAP_ADDR(_addr) \
>> +       ((struct gpio_regmap_addr) { .addr = _addr, .valid = true })
>> +
>> +/**
>> + * struct gpio_regmap - Description of a generic regmap gpio_chip.
>> + *
>> + * @parent:            The parent device
>> + * @regmap:            The regmap use to access the registers
> 
> s/use/used/
> 
>> + *                     given, the name of the device is used
>> + * @label:             (Optional) Descriptive name for GPIO 
>> controller.
>> + *                     If not given, the name of the device is used.
>> + * @ngpio:             Number of GPIOs
>> + * @reg_dat_base:      (Optional) (in) register base address
>> + * @reg_set_base:      (Optional) set register base address
>> + * @reg_clr_base:      (Optional) clear register base address
>> + * @reg_dir_in_base:   (Optional) out setting register base address
>> + * @reg_dir_out_base:  (Optional) in setting register base address
>> + * @reg_stride:                (Optional) May be set if the registers 
>> (of the
>> + *                     same type, dat, set, etc) are not consecutive.
>> + * @ngpio_per_reg:     Number of GPIOs per register
>> + * @irq_domain:                (Optional) IRQ domain if the 
>> controller is
>> + *                     interrupt-capable
>> + * @reg_mask_xlate:     (Optional) Translates base address and GPIO
>> + *                     offset to a register/bitmask pair. If not
>> + *                     given the default gpio_regmap_simple_xlate()
>> + *                     is used.
>> + * @to_irq:            (Optional) Maps GPIO offset to a irq number.
>> + *                     By default assumes a linear mapping of the
>> + *                     given irq_domain.
>> + * @driver_data:       Pointer to the drivers private data. Not used 
>> by
>> + *                     gpio-regmap.
>> + *
>> + * The reg_mask_xlate translates a given base address and GPIO offset 
>> to
>> + * register and mask pair. The base address is one of the given 
>> reg_*_base.
>> + */
>> +struct gpio_regmap {
> 
> I'd prefer to follow a pattern seen in other such APIs of calling this
> structure gpio_regmap_config and creating another private structure
> called gpio_regmap used in callbacks that would only contain necessary
> fields.

something like the following?

struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *)

but if that structure is private, how can a callback access individual
elements? Or do you mean private in "local to the gpio drivers"?

Also I was unsure about the naming, eg. some use
stuff_register()/stuff_unregister() and some stuff_add()/stuff_remove().

> 
>> +       struct device *parent;
>> +       struct regmap *regmap;
>> +       struct gpio_regmap_data *data;
>> +
>> +       const char *label;
>> +       int ngpio;
>> +
>> +       struct gpio_regmap_addr reg_dat_base;
>> +       struct gpio_regmap_addr reg_set_base;
>> +       struct gpio_regmap_addr reg_clr_base;
>> +       struct gpio_regmap_addr reg_dir_in_base;
>> +       struct gpio_regmap_addr reg_dir_out_base;
>> +       int reg_stride;
>> +       int ngpio_per_reg;
>> +       struct irq_domain *irq_domain;
>> +
>> +       int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int 
>> base,
>> +                             unsigned int offset, unsigned int *reg,
>> +                             unsigned int *mask);
>> +       int (*to_irq)(struct gpio_regmap *gpio, unsigned int offset);
>> +
>> +       void *driver_data;
>> +};
>> +
>> +static inline void gpio_regmap_set_drvdata(struct gpio_regmap *gpio,
>> +                                          void *data)
>> +{
>> +       gpio->driver_data = data;
>> +}
>> +
>> +static inline void *gpio_regmap_get_drvdata(struct gpio_regmap *gpio)
>> +{
>> +       return gpio->driver_data;
>> +}
>> +
>> +int gpio_regmap_register(struct gpio_regmap *gpio);
>> +void gpio_regmap_unregister(struct gpio_regmap *gpio);
>> +int devm_gpio_regmap_register(struct device *dev, struct gpio_regmap 
>> *gpio);
>> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int 
>> base,
>> +                            unsigned int offset,
>> +                            unsigned int *reg, unsigned int *mask);
>> +
>> +#endif /* _LINUX_GPIO_REGMAP_H */
>> --
>> 2.20.1
>> 
> 
> Overall looks really nice. I'm happy we'll have it in v5.8.

Thanks, one thing I'm uncertain about is the regmap_irq change and if 
that
is acceptable. So Mark would need to comment on that.

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-06 10:10     ` Michael Walle
@ 2020-04-14  9:50       ` Bartosz Golaszewski
  2020-04-14 10:07         ` Michael Walle
  0 siblings, 1 reply; 38+ messages in thread
From: Bartosz Golaszewski @ 2020-04-14  9:50 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

pon., 6 kwi 2020 o 12:10 Michael Walle <michael@walle.cc> napisał(a):
>
>
> Hi Bartosz, Hi Mark Brown,
>
> Am 2020-04-06 09:47, schrieb Bartosz Golaszewski:
> > czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a):
> >>
> >> There are quite a lot simple GPIO controller which are using regmap to
> >> access the hardware. This driver tries to be a base to unify existing
> >> code into one place. This won't cover everything but it should be a
> >> good
> >> starting point.
> >>
> >> It does not implement its own irq_chip because there is already a
> >> generic one for regmap based devices. Instead, the irq_chip will be
> >> instanciated in the parent driver and its irq domain will be associate
> >> to this driver.
> >>
> >> For now it consists of the usual registers, like set (and an optional
> >> clear) data register, an input register and direction registers.
> >> Out-of-the-box, it supports consecutive register mappings and mappings
> >> where the registers have gaps between them with a linear mapping
> >> between
> >> GPIO offset and bit position. For weirder mappings the user can
> >> register
> >> its own .xlate().
> >>
> >> Signed-off-by: Michael Walle <michael@walle.cc>
> >
> > Hi Michael,
> >
> > Thanks for doing this! When looking at other generic drivers:
> > gpio-mmio and gpio-reg I can see there are some corner-cases and more
> > specific configuration options we could add
>
> I didn't want to copy every bit without being able to test it.
>

Sure, I didn't mean we need to do it now - just set it as the future goal.

> > but it's not a blocker,
> > we'll probably be extending this one as we convert more drivers to
> > using it.
>
> correct, that was also my plan.
>
> > Personally I'd love to see gpio-mmio and gpio-reg removed
> > and replaced by a single, generic regmap interface eventually.
>
> agreed.
>
>

[snip!]

> >> +
> >> +/**
> >> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
> >> + *
> >> + * Use a simple linear mapping to translate the offset to the
> >> bitmask.
> >> + */
> >> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int
> >> base,
> >> +                            unsigned int offset,
> >> +                            unsigned int *reg, unsigned int *mask)
> >> +{
> >> +       unsigned int line = offset % gpio->ngpio_per_reg;
> >> +       unsigned int stride = offset / gpio->ngpio_per_reg;
> >> +
> >> +       *reg = base + stride * gpio->reg_stride;
> >> +       *mask = BIT(line);
> >> +
> >> +       return 0;
> >> +}
> >> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate);
> >
> > Why does this need to be exported?
>
> Mh, the idea was that a user could also set this xlate() by himself (for
> whatever reason). But since it is the default, it is not really
> necessary.
> That being said, I don't care if its only local to this module.
>

Let's only export symbols that have external users then.

[snip!]

> >> +
> >> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
> >> +MODULE_DESCRIPTION("GPIO generic regmap driver core");
> >> +MODULE_LICENSE("GPL");
> >> diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
> >> new file mode 100644
> >> index 000000000000..ad63955e0e43
> >> --- /dev/null
> >> +++ b/include/linux/gpio-regmap.h
> >> @@ -0,0 +1,88 @@
> >> +/* SPDX-License-Identifier: GPL-2.0-only */
> >> +
> >> +#ifndef _LINUX_GPIO_REGMAP_H
> >> +#define _LINUX_GPIO_REGMAP_H
> >> +
> >> +struct gpio_regmap_addr {
> >> +       unsigned int addr;
> >> +       bool valid;
> >> +};
> >
> > I'm not quite sure what the meaning behind the valid field here is.
> > When would we potentially set it to false?
>
> Some base addresses are optional, but on the other hand, a base address
> of 0 could also be valid. So I cannot use 0 as an indicator whether a
> base address is set or not. The generic mmio driver has some special
> case for the ack base, where there is a use_ack flag which forces to
> use the ack register even if its zero. So I've had a look at the kernel
> if there is a better idiom for that, but I haven't found anything.
>
> So the best from a user perspective I've could come up with was:
>
>    ->base_reg = GPIO_REGMAP_ADDR(addr);
>
> I'm open for suggestions.
>

Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
IS_ERR() returning true?

> >
> >> +#define GPIO_REGMAP_ADDR(_addr) \
> >> +       ((struct gpio_regmap_addr) { .addr = _addr, .valid = true })
> >> +
> >> +/**
> >> + * struct gpio_regmap - Description of a generic regmap gpio_chip.
> >> + *
> >> + * @parent:            The parent device
> >> + * @regmap:            The regmap use to access the registers
> >
> > s/use/used/
> >
> >> + *                     given, the name of the device is used
> >> + * @label:             (Optional) Descriptive name for GPIO
> >> controller.
> >> + *                     If not given, the name of the device is used.
> >> + * @ngpio:             Number of GPIOs
> >> + * @reg_dat_base:      (Optional) (in) register base address
> >> + * @reg_set_base:      (Optional) set register base address
> >> + * @reg_clr_base:      (Optional) clear register base address
> >> + * @reg_dir_in_base:   (Optional) out setting register base address
> >> + * @reg_dir_out_base:  (Optional) in setting register base address
> >> + * @reg_stride:                (Optional) May be set if the registers
> >> (of the
> >> + *                     same type, dat, set, etc) are not consecutive.
> >> + * @ngpio_per_reg:     Number of GPIOs per register
> >> + * @irq_domain:                (Optional) IRQ domain if the
> >> controller is
> >> + *                     interrupt-capable
> >> + * @reg_mask_xlate:     (Optional) Translates base address and GPIO
> >> + *                     offset to a register/bitmask pair. If not
> >> + *                     given the default gpio_regmap_simple_xlate()
> >> + *                     is used.
> >> + * @to_irq:            (Optional) Maps GPIO offset to a irq number.
> >> + *                     By default assumes a linear mapping of the
> >> + *                     given irq_domain.
> >> + * @driver_data:       Pointer to the drivers private data. Not used
> >> by
> >> + *                     gpio-regmap.
> >> + *
> >> + * The reg_mask_xlate translates a given base address and GPIO offset
> >> to
> >> + * register and mask pair. The base address is one of the given
> >> reg_*_base.
> >> + */
> >> +struct gpio_regmap {
> >
> > I'd prefer to follow a pattern seen in other such APIs of calling this
> > structure gpio_regmap_config and creating another private structure
> > called gpio_regmap used in callbacks that would only contain necessary
> > fields.
>
> something like the following?
>
> struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *)
>
> but if that structure is private, how can a callback access individual
> elements? Or do you mean private in "local to the gpio drivers"?
>

Either making the structure local to drivers/gpio or making it
entirely opaque and providing accessor functions. Depending on how
much of the structure one may want to access.

> Also I was unsure about the naming, eg. some use
> stuff_register()/stuff_unregister() and some stuff_add()/stuff_remove().
>

register/unregister is fine with me.

Bart

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14  9:50       ` Bartosz Golaszewski
@ 2020-04-14 10:07         ` Michael Walle
  2020-04-14 17:00           ` Bartosz Golaszewski
  2020-04-14 17:21           ` Mark Brown
  0 siblings, 2 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-14 10:07 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

Am 2020-04-14 11:50, schrieb Bartosz Golaszewski:
> pon., 6 kwi 2020 o 12:10 Michael Walle <michael@walle.cc> napisał(a):
>> 
>> 
>> Hi Bartosz, Hi Mark Brown,
>> 
>> Am 2020-04-06 09:47, schrieb Bartosz Golaszewski:
>> > czw., 2 kwi 2020 o 22:37 Michael Walle <michael@walle.cc> napisał(a):
>> >>
>> >> There are quite a lot simple GPIO controller which are using regmap to
>> >> access the hardware. This driver tries to be a base to unify existing
>> >> code into one place. This won't cover everything but it should be a
>> >> good
>> >> starting point.
>> >>
>> >> It does not implement its own irq_chip because there is already a
>> >> generic one for regmap based devices. Instead, the irq_chip will be
>> >> instanciated in the parent driver and its irq domain will be associate
>> >> to this driver.
>> >>
>> >> For now it consists of the usual registers, like set (and an optional
>> >> clear) data register, an input register and direction registers.
>> >> Out-of-the-box, it supports consecutive register mappings and mappings
>> >> where the registers have gaps between them with a linear mapping
>> >> between
>> >> GPIO offset and bit position. For weirder mappings the user can
>> >> register
>> >> its own .xlate().
>> >>
>> >> Signed-off-by: Michael Walle <michael@walle.cc>
>> >
>> > Hi Michael,
>> >
>> > Thanks for doing this! When looking at other generic drivers:
>> > gpio-mmio and gpio-reg I can see there are some corner-cases and more
>> > specific configuration options we could add
>> 
>> I didn't want to copy every bit without being able to test it.
>> 
> 
> Sure, I didn't mean we need to do it now - just set it as the future 
> goal.
> 
>> > but it's not a blocker,
>> > we'll probably be extending this one as we convert more drivers to
>> > using it.
>> 
>> correct, that was also my plan.
>> 
>> > Personally I'd love to see gpio-mmio and gpio-reg removed
>> > and replaced by a single, generic regmap interface eventually.
>> 
>> agreed.
>> 
>> 
> 
> [snip!]
> 
>> >> +
>> >> +/**
>> >> + * gpio_regmap_simple_xlate() - translate base/offset to reg/mask
>> >> + *
>> >> + * Use a simple linear mapping to translate the offset to the
>> >> bitmask.
>> >> + */
>> >> +int gpio_regmap_simple_xlate(struct gpio_regmap *gpio, unsigned int
>> >> base,
>> >> +                            unsigned int offset,
>> >> +                            unsigned int *reg, unsigned int *mask)
>> >> +{
>> >> +       unsigned int line = offset % gpio->ngpio_per_reg;
>> >> +       unsigned int stride = offset / gpio->ngpio_per_reg;
>> >> +
>> >> +       *reg = base + stride * gpio->reg_stride;
>> >> +       *mask = BIT(line);
>> >> +
>> >> +       return 0;
>> >> +}
>> >> +EXPORT_SYMBOL_GPL(gpio_regmap_simple_xlate);
>> >
>> > Why does this need to be exported?
>> 
>> Mh, the idea was that a user could also set this xlate() by himself 
>> (for
>> whatever reason). But since it is the default, it is not really
>> necessary.
>> That being said, I don't care if its only local to this module.
>> 
> 
> Let's only export symbols that have external users then.
> 
> [snip!]
> 
>> >> +
>> >> +MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
>> >> +MODULE_DESCRIPTION("GPIO generic regmap driver core");
>> >> +MODULE_LICENSE("GPL");
>> >> diff --git a/include/linux/gpio-regmap.h b/include/linux/gpio-regmap.h
>> >> new file mode 100644
>> >> index 000000000000..ad63955e0e43
>> >> --- /dev/null
>> >> +++ b/include/linux/gpio-regmap.h
>> >> @@ -0,0 +1,88 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0-only */
>> >> +
>> >> +#ifndef _LINUX_GPIO_REGMAP_H
>> >> +#define _LINUX_GPIO_REGMAP_H
>> >> +
>> >> +struct gpio_regmap_addr {
>> >> +       unsigned int addr;
>> >> +       bool valid;
>> >> +};
>> >
>> > I'm not quite sure what the meaning behind the valid field here is.
>> > When would we potentially set it to false?
>> 
>> Some base addresses are optional, but on the other hand, a base 
>> address
>> of 0 could also be valid. So I cannot use 0 as an indicator whether a
>> base address is set or not. The generic mmio driver has some special
>> case for the ack base, where there is a use_ack flag which forces to
>> use the ack register even if its zero. So I've had a look at the 
>> kernel
>> if there is a better idiom for that, but I haven't found anything.
>> 
>> So the best from a user perspective I've could come up with was:
>> 
>>    ->base_reg = GPIO_REGMAP_ADDR(addr);
>> 
>> I'm open for suggestions.
>> 
> 
> Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
> IS_ERR() returning true?

Unfortunatly, its not a pointer, but only a regular unsigned int (ie
the type the regmap API has for its "reg" property). It could be a
pointer of course but then the user would have to allocate additional
memory.

-michael

> 
>> >
>> >> +#define GPIO_REGMAP_ADDR(_addr) \
>> >> +       ((struct gpio_regmap_addr) { .addr = _addr, .valid = true })
>> >> +
>> >> +/**
>> >> + * struct gpio_regmap - Description of a generic regmap gpio_chip.
>> >> + *
>> >> + * @parent:            The parent device
>> >> + * @regmap:            The regmap use to access the registers
>> >
>> > s/use/used/
>> >
>> >> + *                     given, the name of the device is used
>> >> + * @label:             (Optional) Descriptive name for GPIO
>> >> controller.
>> >> + *                     If not given, the name of the device is used.
>> >> + * @ngpio:             Number of GPIOs
>> >> + * @reg_dat_base:      (Optional) (in) register base address
>> >> + * @reg_set_base:      (Optional) set register base address
>> >> + * @reg_clr_base:      (Optional) clear register base address
>> >> + * @reg_dir_in_base:   (Optional) out setting register base address
>> >> + * @reg_dir_out_base:  (Optional) in setting register base address
>> >> + * @reg_stride:                (Optional) May be set if the registers
>> >> (of the
>> >> + *                     same type, dat, set, etc) are not consecutive.
>> >> + * @ngpio_per_reg:     Number of GPIOs per register
>> >> + * @irq_domain:                (Optional) IRQ domain if the
>> >> controller is
>> >> + *                     interrupt-capable
>> >> + * @reg_mask_xlate:     (Optional) Translates base address and GPIO
>> >> + *                     offset to a register/bitmask pair. If not
>> >> + *                     given the default gpio_regmap_simple_xlate()
>> >> + *                     is used.
>> >> + * @to_irq:            (Optional) Maps GPIO offset to a irq number.
>> >> + *                     By default assumes a linear mapping of the
>> >> + *                     given irq_domain.
>> >> + * @driver_data:       Pointer to the drivers private data. Not used
>> >> by
>> >> + *                     gpio-regmap.
>> >> + *
>> >> + * The reg_mask_xlate translates a given base address and GPIO offset
>> >> to
>> >> + * register and mask pair. The base address is one of the given
>> >> reg_*_base.
>> >> + */
>> >> +struct gpio_regmap {
>> >
>> > I'd prefer to follow a pattern seen in other such APIs of calling this
>> > structure gpio_regmap_config and creating another private structure
>> > called gpio_regmap used in callbacks that would only contain necessary
>> > fields.
>> 
>> something like the following?
>> 
>> struct gpio_regmap *gpio_regmap_register(struct gpio_regmap_config *)
>> 
>> but if that structure is private, how can a callback access individual
>> elements? Or do you mean private in "local to the gpio drivers"?
>> 
> 
> Either making the structure local to drivers/gpio or making it
> entirely opaque and providing accessor functions. Depending on how
> much of the structure one may want to access.
> 
>> Also I was unsure about the naming, eg. some use
>> stuff_register()/stuff_unregister() and some 
>> stuff_add()/stuff_remove().
>> 
> 
> register/unregister is fine with me.
> 
> Bart

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Applied "regmap-irq: make it possible to add irq_chip do a specific device node" to the regmap tree
  2020-04-02 20:36 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Michael Walle
@ 2020-04-14 15:37   ` Mark Brown
  2020-04-14 17:12   ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Mark Brown
  1 sibling, 0 replies; 38+ messages in thread
From: Mark Brown @ 2020-04-14 15:37 UTC (permalink / raw)
  To: Michael Walle
  Cc: Bartosz Golaszewski, devicetree, Greg Kroah-Hartman,
	Guenter Roeck, Jason Cooper, Jean Delvare, Lee Jones,
	Linus Walleij, linux-arm-kernel, linux-gpio, linux-hwmon,
	linux-kernel, linux-pwm, linux-watchdog, Li Yang, Marc Zyngier,
	Mark Brown, Rob Herring, Shawn Guo, Thierry Reding,
	Thomas Gleixner, Uwe Kleine-König, Wim Van Sebroeck

The patch

   regmap-irq: make it possible to add irq_chip do a specific device node

has been applied to the regmap tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 12479382877dcf6623af4676caa8d3c647469a1b Mon Sep 17 00:00:00 2001
From: Michael Walle <michael@walle.cc>
Date: Thu, 2 Apr 2020 22:36:44 +0200
Subject: [PATCH] regmap-irq: make it possible to add irq_chip do a specific
 device node

Add a new function regmap_add_irq_chip_np() with its corresponding
devm_regmap_add_irq_chip_np() variant. Sometimes one want to register
the IRQ domain on a different device node that the one of the regmap
node. For example when using a MFD where there are different interrupt
controllers and particularly for the generic regmap gpio_chip/irq_chip
driver. In this case it is not desireable to have the IRQ domain on
the parent node.

Signed-off-by: Michael Walle <michael@walle.cc>
Link: https://lore.kernel.org/r/20200402203656.27047-5-michael@walle.cc
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/base/regmap/regmap-irq.c | 84 ++++++++++++++++++++++++++------
 include/linux/regmap.h           | 10 ++++
 2 files changed, 78 insertions(+), 16 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 3d64c9331a82..4340e1d268b6 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -541,8 +541,9 @@ static const struct irq_domain_ops regmap_domain_ops = {
 };
 
 /**
- * regmap_add_irq_chip() - Use standard regmap IRQ controller handling
+ * regmap_add_irq_chip_np() - Use standard regmap IRQ controller handling
  *
+ * @np: The device_node where the IRQ domain should be added to.
  * @map: The regmap for the device.
  * @irq: The IRQ the device uses to signal interrupts.
  * @irq_flags: The IRQF_ flags to use for the primary interrupt.
@@ -556,9 +557,10 @@ static const struct irq_domain_ops regmap_domain_ops = {
  * register cache.  The chip driver is responsible for restoring the
  * register values used by the IRQ controller over suspend and resume.
  */
-int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
-			int irq_base, const struct regmap_irq_chip *chip,
-			struct regmap_irq_chip_data **data)
+int regmap_add_irq_chip_np(struct device_node *np, struct regmap *map, int irq,
+			   int irq_flags, int irq_base,
+			   const struct regmap_irq_chip *chip,
+			   struct regmap_irq_chip_data **data)
 {
 	struct regmap_irq_chip_data *d;
 	int i;
@@ -769,12 +771,10 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	}
 
 	if (irq_base)
-		d->domain = irq_domain_add_legacy(map->dev->of_node,
-						  chip->num_irqs, irq_base, 0,
-						  &regmap_domain_ops, d);
+		d->domain = irq_domain_add_legacy(np, chip->num_irqs, irq_base,
+						  0, &regmap_domain_ops, d);
 	else
-		d->domain = irq_domain_add_linear(map->dev->of_node,
-						  chip->num_irqs,
+		d->domain = irq_domain_add_linear(np, chip->num_irqs,
 						  &regmap_domain_ops, d);
 	if (!d->domain) {
 		dev_err(map->dev, "Failed to create IRQ domain\n");
@@ -808,6 +808,30 @@ int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 	kfree(d);
 	return ret;
 }
+EXPORT_SYMBOL_GPL(regmap_add_irq_chip_np);
+
+/**
+ * regmap_add_irq_chip() - Use standard regmap IRQ controller handling
+ *
+ * @map: The regmap for the device.
+ * @irq: The IRQ the device uses to signal interrupts.
+ * @irq_flags: The IRQF_ flags to use for the primary interrupt.
+ * @irq_base: Allocate at specific IRQ number if irq_base > 0.
+ * @chip: Configuration for the interrupt controller.
+ * @data: Runtime data structure for the controller, allocated on success.
+ *
+ * Returns 0 on success or an errno on failure.
+ *
+ * This is the same as regmap_add_irq_chip_np, except that the device
+ * node of the regmap is used.
+ */
+int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
+			int irq_base, const struct regmap_irq_chip *chip,
+			struct regmap_irq_chip_data **data)
+{
+	return regmap_add_irq_chip_np(map->dev->of_node, map, irq, irq_flags,
+				      irq_base, chip, data);
+}
 EXPORT_SYMBOL_GPL(regmap_add_irq_chip);
 
 /**
@@ -875,9 +899,10 @@ static int devm_regmap_irq_chip_match(struct device *dev, void *res, void *data)
 }
 
 /**
- * devm_regmap_add_irq_chip() - Resource manager regmap_add_irq_chip()
+ * devm_regmap_add_irq_chip_np() - Resource manager regmap_add_irq_chip_np()
  *
  * @dev: The device pointer on which irq_chip belongs to.
+ * @np: The device_node where the IRQ domain should be added to.
  * @map: The regmap for the device.
  * @irq: The IRQ the device uses to signal interrupts
  * @irq_flags: The IRQF_ flags to use for the primary interrupt.
@@ -890,10 +915,11 @@ static int devm_regmap_irq_chip_match(struct device *dev, void *res, void *data)
  * The &regmap_irq_chip_data will be automatically released when the device is
  * unbound.
  */
-int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
-			     int irq_flags, int irq_base,
-			     const struct regmap_irq_chip *chip,
-			     struct regmap_irq_chip_data **data)
+int devm_regmap_add_irq_chip_np(struct device *dev, struct device_node *np,
+				struct regmap *map, int irq, int irq_flags,
+				int irq_base,
+				const struct regmap_irq_chip *chip,
+				struct regmap_irq_chip_data **data)
 {
 	struct regmap_irq_chip_data **ptr, *d;
 	int ret;
@@ -903,8 +929,8 @@ int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = regmap_add_irq_chip(map, irq, irq_flags, irq_base,
-				  chip, &d);
+	ret = regmap_add_irq_chip_np(np, map, irq, irq_flags, irq_base,
+				     chip, &d);
 	if (ret < 0) {
 		devres_free(ptr);
 		return ret;
@@ -915,6 +941,32 @@ int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 	*data = d;
 	return 0;
 }
+EXPORT_SYMBOL_GPL(devm_regmap_add_irq_chip_np);
+
+/**
+ * devm_regmap_add_irq_chip() - Resource manager regmap_add_irq_chip()
+ *
+ * @dev: The device pointer on which irq_chip belongs to.
+ * @map: The regmap for the device.
+ * @irq: The IRQ the device uses to signal interrupts
+ * @irq_flags: The IRQF_ flags to use for the primary interrupt.
+ * @irq_base: Allocate at specific IRQ number if irq_base > 0.
+ * @chip: Configuration for the interrupt controller.
+ * @data: Runtime data structure for the controller, allocated on success
+ *
+ * Returns 0 on success or an errno on failure.
+ *
+ * The &regmap_irq_chip_data will be automatically released when the device is
+ * unbound.
+ */
+int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
+			     int irq_flags, int irq_base,
+			     const struct regmap_irq_chip *chip,
+			     struct regmap_irq_chip_data **data)
+{
+	return devm_regmap_add_irq_chip_np(dev, map->dev->of_node, map, irq,
+					   irq_flags, irq_base, chip, data);
+}
 EXPORT_SYMBOL_GPL(devm_regmap_add_irq_chip);
 
 /**
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 40b07168fd8e..ae5034b2d7c3 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -21,6 +21,7 @@
 struct module;
 struct clk;
 struct device;
+struct device_node;
 struct i2c_client;
 struct i3c_device;
 struct irq_domain;
@@ -1310,12 +1311,21 @@ struct regmap_irq_chip_data;
 int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 			int irq_base, const struct regmap_irq_chip *chip,
 			struct regmap_irq_chip_data **data);
+int regmap_add_irq_chip_np(struct device_node *np, struct regmap *map, int irq,
+			   int irq_flags, int irq_base,
+			   const struct regmap_irq_chip *chip,
+			   struct regmap_irq_chip_data **data);
 void regmap_del_irq_chip(int irq, struct regmap_irq_chip_data *data);
 
 int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 			     int irq_flags, int irq_base,
 			     const struct regmap_irq_chip *chip,
 			     struct regmap_irq_chip_data **data);
+int devm_regmap_add_irq_chip_np(struct device *dev, struct device_node *np,
+				struct regmap *map, int irq, int irq_flags,
+				int irq_base,
+				const struct regmap_irq_chip *chip,
+				struct regmap_irq_chip_data **data);
 void devm_regmap_del_irq_chip(struct device *dev, int irq,
 			      struct regmap_irq_chip_data *data);
 
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 10:07         ` Michael Walle
@ 2020-04-14 17:00           ` Bartosz Golaszewski
  2020-04-14 18:41             ` Michael Walle
  2020-04-14 17:21           ` Mark Brown
  1 sibling, 1 reply; 38+ messages in thread
From: Bartosz Golaszewski @ 2020-04-14 17:00 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

wt., 14 kwi 2020 o 12:07 Michael Walle <michael@walle.cc> napisał(a):
> >>
> >> So the best from a user perspective I've could come up with was:
> >>
> >>    ->base_reg = GPIO_REGMAP_ADDR(addr);
> >>
> >> I'm open for suggestions.
> >>
> >
> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
> > IS_ERR() returning true?
>
> Unfortunatly, its not a pointer, but only a regular unsigned int (ie
> the type the regmap API has for its "reg" property). It could be a
> pointer of course but then the user would have to allocate additional
> memory.
>
> -michael
>

Eek, of course it's not a pointer. If possible I'd like to avoid this
GPIO_REGMAP_ADDR() macro, so how about having some separate field for
invalid offsets making every offset 'valid' by default?

Linus: do you have a better idea?

Bart

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node
  2020-04-02 20:36 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Michael Walle
  2020-04-14 15:37   ` Applied "regmap-irq: make it possible to add irq_chip do a specific device node" to the regmap tree Mark Brown
@ 2020-04-14 17:12   ` Mark Brown
  1 sibling, 0 replies; 38+ messages in thread
From: Mark Brown @ 2020-04-14 17:12 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio, devicetree, linux-kernel, linux-hwmon, linux-pwm,
	linux-watchdog, linux-arm-kernel, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Greg Kroah-Hartman

[-- Attachment #1: Type: text/plain, Size: 1172 bytes --]

On Thu, Apr 02, 2020 at 10:36:44PM +0200, Michael Walle wrote:
> Add a new function regmap_add_irq_chip_np() with its corresponding
> devm_regmap_add_irq_chip_np() variant. Sometimes one want to register
> the IRQ domain on a different device node that the one of the regmap

The following changes since commit 8f3d9f354286745c751374f5f1fcafee6b3f3136:

  Linux 5.7-rc1 (2020-04-12 12:35:55 -0700)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git tags/regmap-irq-np

for you to fetch changes up to 12479382877dcf6623af4676caa8d3c647469a1b:

  regmap-irq: make it possible to add irq_chip do a specific device node (2020-04-14 16:21:37 +0100)

----------------------------------------------------------------
regmap: Allow an irqchip to be created for a specific DT node

----------------------------------------------------------------
Michael Walle (1):
      regmap-irq: make it possible to add irq_chip do a specific device node

 drivers/base/regmap/regmap-irq.c | 84 ++++++++++++++++++++++++++++++++--------
 include/linux/regmap.h           | 10 +++++
 2 files changed, 78 insertions(+), 16 deletions(-)

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 10:07         ` Michael Walle
  2020-04-14 17:00           ` Bartosz Golaszewski
@ 2020-04-14 17:21           ` Mark Brown
  2020-04-14 18:36             ` Michael Walle
  1 sibling, 1 reply; 38+ messages in thread
From: Mark Brown @ 2020-04-14 17:21 UTC (permalink / raw)
  To: Michael Walle
  Cc: Bartosz Golaszewski, linux-gpio, linux-devicetree, LKML,
	linux-hwmon, linux-pwm, LINUXWATCHDOG, arm-soc, Linus Walleij,
	Rob Herring, Jean Delvare, Guenter Roeck, Lee Jones,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Greg Kroah-Hartman

[-- Attachment #1: Type: text/plain, Size: 1026 bytes --]

On Tue, Apr 14, 2020 at 12:07:01PM +0200, Michael Walle wrote:
> Am 2020-04-14 11:50, schrieb Bartosz Golaszewski:

> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
> > IS_ERR() returning true?

> Unfortunatly, its not a pointer, but only a regular unsigned int (ie
> the type the regmap API has for its "reg" property). It could be a
> pointer of course but then the user would have to allocate additional
> memory.

You could define REGMAP_INVALID_ADDR to be (unsigned int)(-1) or some
other suitably implausible address and use that as a value.  It's
possible that there might be a collision with a real address on some
device but it should be sufficiently unlikely to be useful, especially
if it's not something regmap in general goes and evaluates.  For extra
safety we could have an API for allowing users to query the register
validity information regmap has (or can be given) and gpiolib could then
use that to figure out if the value was actually a dummy value but
that's probably overdoing it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 17:21           ` Mark Brown
@ 2020-04-14 18:36             ` Michael Walle
  2020-04-14 18:39               ` Mark Brown
  0 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-14 18:36 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bartosz Golaszewski, linux-gpio, linux-devicetree, LKML,
	linux-hwmon, linux-pwm, LINUXWATCHDOG, arm-soc, Linus Walleij,
	Rob Herring, Jean Delvare, Guenter Roeck, Lee Jones,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Greg Kroah-Hartman

Am 2020-04-14 19:21, schrieb Mark Brown:
> On Tue, Apr 14, 2020 at 12:07:01PM +0200, Michael Walle wrote:
>> Am 2020-04-14 11:50, schrieb Bartosz Golaszewski:
> 
>> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
>> > IS_ERR() returning true?
> 
>> Unfortunatly, its not a pointer, but only a regular unsigned int (ie
>> the type the regmap API has for its "reg" property). It could be a
>> pointer of course but then the user would have to allocate additional
>> memory.
> 
> You could define REGMAP_INVALID_ADDR to be (unsigned int)(-1) or some
> other suitably implausible address and use that as a value.  It's
> possible that there might be a collision with a real address on some
> device but it should be sufficiently unlikely to be useful, especially
> if it's not something regmap in general goes and evaluates.  For extra
> safety we could have an API for allowing users to query the register
> validity information regmap has (or can be given) and gpiolib could 
> then
> use that to figure out if the value was actually a dummy value but
> that's probably overdoing it.

If possible, I'd like to have the opposite logic. That is, if it is not
set it should be invalid. If we have a magic macro like
REGMAP_INVALID_ADDR, we must assign it to all the unused addresses. Thus
every driver would have to assign all addresses and if in the future
there will be some added, we'd have to touch all the drivers which use
gpio_regmap.

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 18:36             ` Michael Walle
@ 2020-04-14 18:39               ` Mark Brown
  0 siblings, 0 replies; 38+ messages in thread
From: Mark Brown @ 2020-04-14 18:39 UTC (permalink / raw)
  To: Michael Walle
  Cc: Bartosz Golaszewski, linux-gpio, linux-devicetree, LKML,
	linux-hwmon, linux-pwm, LINUXWATCHDOG, arm-soc, Linus Walleij,
	Rob Herring, Jean Delvare, Guenter Roeck, Lee Jones,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Greg Kroah-Hartman

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

On Tue, Apr 14, 2020 at 08:36:23PM +0200, Michael Walle wrote:
> Am 2020-04-14 19:21, schrieb Mark Brown:

> > You could define REGMAP_INVALID_ADDR to be (unsigned int)(-1) or some
> > other suitably implausible address and use that as a value.  It's
> > possible that there might be a collision with a real address on some
> > device but it should be sufficiently unlikely to be useful, especially
> > if it's not something regmap in general goes and evaluates.  For extra
> > safety we could have an API for allowing users to query the register
> > validity information regmap has (or can be given) and gpiolib could then
> > use that to figure out if the value was actually a dummy value but
> > that's probably overdoing it.

> If possible, I'd like to have the opposite logic. That is, if it is not
> set it should be invalid. If we have a magic macro like
> REGMAP_INVALID_ADDR, we must assign it to all the unused addresses. Thus
> every driver would have to assign all addresses and if in the future
> there will be some added, we'd have to touch all the drivers which use
> gpio_regmap.

Sure, for that you'd need a separate flag since zero is such a commonly
valid address.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 17:00           ` Bartosz Golaszewski
@ 2020-04-14 18:41             ` Michael Walle
  2020-04-14 19:57               ` Michael Walle
  0 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-14 18:41 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

Am 2020-04-14 19:00, schrieb Bartosz Golaszewski:
> wt., 14 kwi 2020 o 12:07 Michael Walle <michael@walle.cc> napisał(a):
>> >>
>> >> So the best from a user perspective I've could come up with was:
>> >>
>> >>    ->base_reg = GPIO_REGMAP_ADDR(addr);
>> >>
>> >> I'm open for suggestions.
>> >>
>> >
>> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
>> > IS_ERR() returning true?
>> 
>> Unfortunatly, its not a pointer, but only a regular unsigned int (ie
>> the type the regmap API has for its "reg" property). It could be a
>> pointer of course but then the user would have to allocate additional
>> memory.
>> 
>> -michael
>> 
> 
> Eek, of course it's not a pointer. If possible I'd like to avoid this
> GPIO_REGMAP_ADDR() macro, so how about having some separate field for
> invalid offsets making every offset 'valid' by default?

IMHO this has the same problems as mentioned in the response to Mark's
idea. Normally, the user sets only some addresses, thus he has to mark
all other as invalid. And if you add another address, you have to touch
all the drivers to mark it as invalid.

We could add some force bits like the "use_ack" flag in the bgpio 
driver,
where you can force the use of the value 0. But I'd really like to find
a better way..

-michael

> 
> Linus: do you have a better idea?
> 
> Bart

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 18:41             ` Michael Walle
@ 2020-04-14 19:57               ` Michael Walle
  2020-04-16  9:20                 ` Linus Walleij
  0 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-14 19:57 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-gpio, linux-devicetree, LKML, linux-hwmon, linux-pwm,
	LINUXWATCHDOG, arm-soc, Linus Walleij, Rob Herring, Jean Delvare,
	Guenter Roeck, Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

Hi Mark, Hi Bartosz, Hi Linus,

Am 2020-04-14 20:41, schrieb Michael Walle:
> Am 2020-04-14 19:00, schrieb Bartosz Golaszewski:
>> wt., 14 kwi 2020 o 12:07 Michael Walle <michael@walle.cc> napisał(a):
>>> >>
>>> >> So the best from a user perspective I've could come up with was:
>>> >>
>>> >>    ->base_reg = GPIO_REGMAP_ADDR(addr);
>>> >>
>>> >> I'm open for suggestions.
>>> >>
>>> >
>>> > Maybe setting the pointer to ERR_PTR(-ENOENT) which will result in
>>> > IS_ERR() returning true?
>>> 
>>> Unfortunatly, its not a pointer, but only a regular unsigned int (ie
>>> the type the regmap API has for its "reg" property). It could be a
>>> pointer of course but then the user would have to allocate additional
>>> memory.
>>> 
>>> -michael
>>> 
>> 
>> Eek, of course it's not a pointer. If possible I'd like to avoid this
>> GPIO_REGMAP_ADDR() macro, so how about having some separate field for
>> invalid offsets making every offset 'valid' by default?
> 
> IMHO this has the same problems as mentioned in the response to Mark's
> idea. Normally, the user sets only some addresses, thus he has to mark
> all other as invalid. And if you add another address, you have to touch
> all the drivers to mark it as invalid.
> 
> We could add some force bits like the "use_ack" flag in the bgpio 
> driver,
> where you can force the use of the value 0. But I'd really like to find
> a better way..

So what about the following:

#define GPIO_REGMAP_ADDR_ZERO (unsigned int)(-1)

So this way the user might assign the base addresses the normal way
except when he wants to use zero, in that case he has to use

   ->base_adr = GPIO_REGMAP_ADDR_ZERO;

gpio-regmap.c could use then:

if (base_addr)
   something_useful(gpio_regmap_addr(base_addr));

unsigned int gpio_regmap_addr(unsigned int addr)
{
   return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr;
}

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller
  2020-04-02 20:36 ` [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller Michael Walle
@ 2020-04-16  8:34   ` Linus Walleij
  2020-04-16  8:55     ` Michael Walle
  0 siblings, 1 reply; 38+ messages in thread
From: Linus Walleij @ 2020-04-16  8:34 UTC (permalink / raw)
  To: Michael Walle
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, linux-hwmon, linux-pwm, LINUXWATCHDOG, Linux ARM,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

Hi Michael,

this is looking good provided we can get the generic GPIO regmap
helper reviewed and merged. Thanks!

On Thu, Apr 2, 2020 at 10:37 PM Michael Walle <michael@walle.cc> wrote:

> This adds support for the GPIO controller of the sl28 board management
> controller. This driver is part of a multi-function device.
>
> Signed-off-by: Michael Walle <michael@walle.cc>

> +       depends on MFD_SL28CPLD

Apart from this depends it seems the patch is compile-time
independent of the other patches so I'd suggest we just merge
the generic regmap driver and this driver to the GPIO tree once
we feel finished with them, optimistically assuming that the MFD
driver will land and that we will not need any fundamental
changes in the GPIO driver.

Worst case we have to revert the driver and that is no disaster.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller
  2020-04-16  8:34   ` Linus Walleij
@ 2020-04-16  8:55     ` Michael Walle
  0 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-16  8:55 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, linux-hwmon, linux-pwm, LINUXWATCHDOG, Linux ARM,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman


Hi Linus,

Am 2020-04-16 10:34, schrieb Linus Walleij:
> Hi Michael,
> 
> this is looking good provided we can get the generic GPIO regmap
> helper reviewed and merged. Thanks!
> 
> On Thu, Apr 2, 2020 at 10:37 PM Michael Walle <michael@walle.cc> wrote:
> 
>> This adds support for the GPIO controller of the sl28 board management
>> controller. This driver is part of a multi-function device.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
>> +       depends on MFD_SL28CPLD
> 
> Apart from this depends it seems the patch is compile-time
> independent of the other patches

correct. There are no common mfd headers or something like that.

> so I'd suggest we just merge
> the generic regmap driver and this driver to the GPIO tree once
> we feel finished with them, optimistically assuming that the MFD
> driver will land and that we will not need any fundamental
> changes in the GPIO driver.
> 
> Worst case we have to revert the driver and that is no disaster.

Sure. One major thing I'm waiting for is the decision/new ideas on
how to handle the "register is not set or zero" problem, see the
other thread on the generic regmap gpio. Then I'd respin an update
of this whole series.

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-14 19:57               ` Michael Walle
@ 2020-04-16  9:20                 ` Linus Walleij
  2020-04-16  9:34                   ` Michael Walle
  0 siblings, 1 reply; 38+ messages in thread
From: Linus Walleij @ 2020-04-16  9:20 UTC (permalink / raw)
  To: Michael Walle
  Cc: Bartosz Golaszewski, linux-gpio, linux-devicetree, LKML,
	linux-hwmon, linux-pwm, LINUXWATCHDOG, arm-soc, Rob Herring,
	Jean Delvare, Guenter Roeck, Lee Jones, Thierry Reding,
	Uwe Kleine-König, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

On Tue, Apr 14, 2020 at 9:57 PM Michael Walle <michael@walle.cc> wrote:

> So what about the following:
>
> #define GPIO_REGMAP_ADDR_ZERO (unsigned int)(-1)

Yeah with regmap explicitly using int I guess we can't use
S32_MAX, so that is fair.

> So this way the user might assign the base addresses the normal way
> except when he wants to use zero, in that case he has to use
>
>    ->base_adr = GPIO_REGMAP_ADDR_ZERO;
>
> gpio-regmap.c could use then:
>
> if (base_addr)
>    something_useful(gpio_regmap_addr(base_addr));
>
> unsigned int gpio_regmap_addr(unsigned int addr)
> {
>    return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr;
> }

That's reasonably clean.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-02 20:36 ` [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap Michael Walle
  2020-04-06  7:47   ` Bartosz Golaszewski
@ 2020-04-16  9:27   ` Linus Walleij
  2020-04-17  6:34     ` Michael Walle
  1 sibling, 1 reply; 38+ messages in thread
From: Linus Walleij @ 2020-04-16  9:27 UTC (permalink / raw)
  To: Michael Walle
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, linux-hwmon, linux-pwm, LINUXWATCHDOG, Linux ARM,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

On Thu, Apr 2, 2020 at 10:37 PM Michael Walle <michael@walle.cc> wrote:

> There are quite a lot simple GPIO controller which are using regmap to
> access the hardware. This driver tries to be a base to unify existing
> code into one place. This won't cover everything but it should be a good
> starting point.
>
> It does not implement its own irq_chip because there is already a
> generic one for regmap based devices. Instead, the irq_chip will be
> instanciated in the parent driver and its irq domain will be associate
> to this driver.
>
> For now it consists of the usual registers, like set (and an optional
> clear) data register, an input register and direction registers.
> Out-of-the-box, it supports consecutive register mappings and mappings
> where the registers have gaps between them with a linear mapping between
> GPIO offset and bit position. For weirder mappings the user can register
> its own .xlate().
>
> Signed-off-by: Michael Walle <michael@walle.cc>

Overall I really like this driver and I think we should merge is as soon
as it is in reasonable shape and then improve on top so we can start
migrating drivers to it.

> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
> +       struct gpio_regmap *gpio = data->gpio;
> +
> +       /* the user might have its own .to_irq callback */
> +       if (gpio->to_irq)
> +               return gpio->to_irq(gpio, offset);
> +
> +       return irq_create_mapping(gpio->irq_domain, offset);

I think that should at least be irq_find_mapping(), the mapping should
definately not be created by the .to_irq() callback since that is just
a convenience function.

> +       if (gpio->irq_domain)
> +               chip->to_irq = gpio_regmap_to_irq;

I don't know about this.
(...)
> + * @irq_domain:                (Optional) IRQ domain if the controller is
> + *                     interrupt-capable
(...)
> +       struct irq_domain *irq_domain;

I don't think this is a good storage place for the irqdomain, we already have
gpio_irq_chip inside gpio_chip and that has an irqdomain, we should
strive to reuse that infrastructure also for regmap GPIO I think, for now
I would just leave .to_irq() out of this and let the driver deal with any
irqs.

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-16  9:20                 ` Linus Walleij
@ 2020-04-16  9:34                   ` Michael Walle
  0 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-16  9:34 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bartosz Golaszewski, linux-gpio, linux-devicetree, LKML,
	linux-hwmon, linux-pwm, LINUXWATCHDOG, arm-soc, Rob Herring,
	Jean Delvare, Guenter Roeck, Lee Jones, Thierry Reding,
	Uwe Kleine-König, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

Am 2020-04-16 11:20, schrieb Linus Walleij:
> On Tue, Apr 14, 2020 at 9:57 PM Michael Walle <michael@walle.cc> wrote:
> 
>> So what about the following:
>> 
>> #define GPIO_REGMAP_ADDR_ZERO (unsigned int)(-1)
> 
> Yeah with regmap explicitly using int I guess we can't use
> S32_MAX, so that is fair.
> 
>> So this way the user might assign the base addresses the normal way
>> except when he wants to use zero, in that case he has to use
>> 
>>    ->base_adr = GPIO_REGMAP_ADDR_ZERO;
>> 
>> gpio-regmap.c could use then:
>> 
>> if (base_addr)
>>    something_useful(gpio_regmap_addr(base_addr));
>> 
>> unsigned int gpio_regmap_addr(unsigned int addr)
>> {
>>    return (addr == GPIO_REGMAP_ADDR_ZERO) ? 0 : addr;
>> }
> 
> That's reasonably clean.

Ok, at least on that side. For my sl28 gpio driver I then have
the problem that depending on 'base' I might have to use
GPIO_REGMAP_ADDR_ZERO:

   #define GPIO_REG_DIR 0
   config.reg_dir_out_base = base + GPIO_REG_DIR;

So there is still a convenience macro:
   #define GPIO_REGMAP_ADDR(addr) ((addr) ? addr : GPIO_REGMAP_ADDR_ZERO)

which you can use if you can't be sure that the address is not non-zero.
So the code in my sl28 gpio driver looks like:

  config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);

I'll respin the patch with the current remarks.

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-16  9:27   ` Linus Walleij
@ 2020-04-17  6:34     ` Michael Walle
  2020-04-21 10:50       ` Michael Walle
  0 siblings, 1 reply; 38+ messages in thread
From: Michael Walle @ 2020-04-17  6:34 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, linux-hwmon, linux-pwm, LINUXWATCHDOG, Linux ARM,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

Hi Linus,

Am 2020-04-16 11:27, schrieb Linus Walleij:
> On Thu, Apr 2, 2020 at 10:37 PM Michael Walle <michael@walle.cc> wrote:
> 
>> There are quite a lot simple GPIO controller which are using regmap to
>> access the hardware. This driver tries to be a base to unify existing
>> code into one place. This won't cover everything but it should be a 
>> good
>> starting point.
>> 
>> It does not implement its own irq_chip because there is already a
>> generic one for regmap based devices. Instead, the irq_chip will be
>> instanciated in the parent driver and its irq domain will be associate
>> to this driver.
>> 
>> For now it consists of the usual registers, like set (and an optional
>> clear) data register, an input register and direction registers.
>> Out-of-the-box, it supports consecutive register mappings and mappings
>> where the registers have gaps between them with a linear mapping 
>> between
>> GPIO offset and bit position. For weirder mappings the user can 
>> register
>> its own .xlate().
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
> 
> Overall I really like this driver and I think we should merge is as 
> soon
> as it is in reasonable shape and then improve on top so we can start
> migrating drivers to it.
> 
>> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int 
>> offset)
>> +{
>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>> +       struct gpio_regmap *gpio = data->gpio;
>> +
>> +       /* the user might have its own .to_irq callback */
>> +       if (gpio->to_irq)
>> +               return gpio->to_irq(gpio, offset);
>> +
>> +       return irq_create_mapping(gpio->irq_domain, offset);
> 
> I think that should at least be irq_find_mapping(), the mapping should
> definately not be created by the .to_irq() callback since that is just
> a convenience function.

what do you mean by conenience function? are there other ways? if you 
use
irq_find_mapping() who will create the mappings? most gpio drivers use a
similar function like gpio_regmap_to_irq().

> 
>> +       if (gpio->irq_domain)
>> +               chip->to_irq = gpio_regmap_to_irq;
> 
> I don't know about this.
> (...)
>> + * @irq_domain:                (Optional) IRQ domain if the 
>> controller is
>> + *                     interrupt-capable
> (...)
>> +       struct irq_domain *irq_domain;
> 
> I don't think this is a good storage place for the irqdomain, we 
> already have
> gpio_irq_chip inside gpio_chip and that has an irqdomain, we should
> strive to reuse that infrastructure also for regmap GPIO I think, for 
> now
> I would just leave .to_irq() out of this and let the driver deal with 
> any
> irqs.

How would a driver attach the to_irq callback then? At the moment, the
gpio_regmap doesn't expose the gpio_chip. So either we have to do that 
or
the config still have to have a .to_irq property.

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

* Re: [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap
  2020-04-17  6:34     ` Michael Walle
@ 2020-04-21 10:50       ` Michael Walle
  0 siblings, 0 replies; 38+ messages in thread
From: Michael Walle @ 2020-04-21 10:50 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, linux-hwmon, linux-pwm, LINUXWATCHDOG, Linux ARM,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Uwe Kleine-König,
	Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman

Hi Linus,

Am 2020-04-17 08:34, schrieb Michael Walle:
> Hi Linus,
> 
> Am 2020-04-16 11:27, schrieb Linus Walleij:
>> On Thu, Apr 2, 2020 at 10:37 PM Michael Walle <michael@walle.cc> 
>> wrote:
>> 
>>> There are quite a lot simple GPIO controller which are using regmap 
>>> to
>>> access the hardware. This driver tries to be a base to unify existing
>>> code into one place. This won't cover everything but it should be a 
>>> good
>>> starting point.
>>> 
>>> It does not implement its own irq_chip because there is already a
>>> generic one for regmap based devices. Instead, the irq_chip will be
>>> instanciated in the parent driver and its irq domain will be 
>>> associate
>>> to this driver.
>>> 
>>> For now it consists of the usual registers, like set (and an optional
>>> clear) data register, an input register and direction registers.
>>> Out-of-the-box, it supports consecutive register mappings and 
>>> mappings
>>> where the registers have gaps between them with a linear mapping 
>>> between
>>> GPIO offset and bit position. For weirder mappings the user can 
>>> register
>>> its own .xlate().
>>> 
>>> Signed-off-by: Michael Walle <michael@walle.cc>
>> 
>> Overall I really like this driver and I think we should merge is as 
>> soon
>> as it is in reasonable shape and then improve on top so we can start
>> migrating drivers to it.
>> 
>>> +static int gpio_regmap_to_irq(struct gpio_chip *chip, unsigned int 
>>> offset)
>>> +{
>>> +       struct gpio_regmap_data *data = gpiochip_get_data(chip);
>>> +       struct gpio_regmap *gpio = data->gpio;
>>> +
>>> +       /* the user might have its own .to_irq callback */
>>> +       if (gpio->to_irq)
>>> +               return gpio->to_irq(gpio, offset);
>>> +
>>> +       return irq_create_mapping(gpio->irq_domain, offset);
>> 
>> I think that should at least be irq_find_mapping(), the mapping should
>> definately not be created by the .to_irq() callback since that is just
>> a convenience function.
> 
> what do you mean by conenience function? are there other ways? if you 
> use
> irq_find_mapping() who will create the mappings? most gpio drivers use 
> a
> similar function like gpio_regmap_to_irq().
> 
>> 
>>> +       if (gpio->irq_domain)
>>> +               chip->to_irq = gpio_regmap_to_irq;
>> 
>> I don't know about this.
>> (...)
>>> + * @irq_domain:                (Optional) IRQ domain if the 
>>> controller is
>>> + *                     interrupt-capable
>> (...)
>>> +       struct irq_domain *irq_domain;
>> 
>> I don't think this is a good storage place for the irqdomain, we 
>> already have
>> gpio_irq_chip inside gpio_chip and that has an irqdomain, we should
>> strive to reuse that infrastructure also for regmap GPIO I think, for 
>> now
>> I would just leave .to_irq() out of this and let the driver deal with 
>> any
>> irqs.
> 
> How would a driver attach the to_irq callback then? At the moment, the
> gpio_regmap doesn't expose the gpio_chip. So either we have to do that 
> or
> the config still have to have a .to_irq property.

Also, if I move the interrupt hanling completely out of the gpio-regmap, 
the
driver would have to deal with "struct gpio_chip" which I would like to 
avoid
if possible and keep it private to gpio-regmap.

Unfortunately, I don't have much experience how a good API for the 
interrupt
handling and the gpio-regmap might look like. And there seems to be some
overlap between regmap-irq and the interrupt stuff in gpiolib. For 
example,
both provide and set the irq_domain_ops. Thus handing the domain over to
gpio-regmap looked like a good idea to me. I get you point, that there 
is
already a irqdomain in gpiolib and also a _to_irq() which is the same as
the current implementation in gpio-regmap. Maybe it makes sense to just
have a new function

int gpiolib_add_irqdomain(struct gpio_chip *gc, struct irq_domain 
domain)
{
   gc->irq.domain = domain;
   gc->to_irq = gpiochip_to_irq;
}

which is called by gpio_regmap_register() if a config->irq_domain is 
given.

-michael

^ permalink raw reply	[flat|nested] 38+ messages in thread

end of thread, other threads:[~2020-04-21 10:51 UTC | newest]

Thread overview: 38+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-02 20:36 [PATCH v2 00/16] Add support for Kontron sl28cpld Michael Walle
2020-04-02 20:36 ` [PATCH v2 01/16] include/linux/ioport.h: add helper to define REG resource constructs Michael Walle
2020-04-02 20:36 ` [PATCH v2 02/16] mfd: mfd-core: Don't overwrite the dma_mask of the child device Michael Walle
2020-04-02 20:36 ` [PATCH v2 03/16] mfd: mfd-core: match device tree node against reg property Michael Walle
2020-04-02 20:36 ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Michael Walle
2020-04-14 15:37   ` Applied "regmap-irq: make it possible to add irq_chip do a specific device node" to the regmap tree Mark Brown
2020-04-14 17:12   ` [PATCH v2 04/16] regmap-irq: make it possible to add irq_chip do a specific device node Mark Brown
2020-04-02 20:36 ` [PATCH v2 05/16] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-04-02 20:36 ` [PATCH v2 06/16] mfd: Add support for Kontron sl28cpld management controller Michael Walle
2020-04-02 20:36 ` [PATCH v2 07/16] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-04-02 20:36 ` [PATCH v2 08/16] watchdog: add support for sl28cpld watchdog Michael Walle
2020-04-03  6:25   ` Guenter Roeck
2020-04-02 20:36 ` [PATCH v2 09/16] pwm: add support for sl28cpld PWM controller Michael Walle
2020-04-02 20:36 ` [PATCH v2 10/16] gpio: add a reusable generic gpio_chip using regmap Michael Walle
2020-04-06  7:47   ` Bartosz Golaszewski
2020-04-06 10:10     ` Michael Walle
2020-04-14  9:50       ` Bartosz Golaszewski
2020-04-14 10:07         ` Michael Walle
2020-04-14 17:00           ` Bartosz Golaszewski
2020-04-14 18:41             ` Michael Walle
2020-04-14 19:57               ` Michael Walle
2020-04-16  9:20                 ` Linus Walleij
2020-04-16  9:34                   ` Michael Walle
2020-04-14 17:21           ` Mark Brown
2020-04-14 18:36             ` Michael Walle
2020-04-14 18:39               ` Mark Brown
2020-04-16  9:27   ` Linus Walleij
2020-04-17  6:34     ` Michael Walle
2020-04-21 10:50       ` Michael Walle
2020-04-02 20:36 ` [PATCH v2 11/16] gpio: add support for the sl28cpld GPIO controller Michael Walle
2020-04-16  8:34   ` Linus Walleij
2020-04-16  8:55     ` Michael Walle
2020-04-02 20:36 ` [PATCH v2 12/16] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-04-02 21:30   ` Guenter Roeck
2020-04-02 20:36 ` [PATCH v2 13/16] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-04-02 20:36 ` [PATCH v2 14/16] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-04-02 20:36 ` [PATCH v2 15/16] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-04-02 20:36 ` [PATCH v2 16/16] arm64: dts: freescale: sl28: enable fan support Michael Walle

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).