Linux-PWM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5 00/13] Add support for Kontron sl28cpld
@ 2020-07-06 17:53 Michael Walle
  2020-07-06 17:53 ` [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip() Michael Walle
                   ` (10 more replies)
  0 siblings, 11 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, 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 v4:
 - see individual patches, suggested by Lee.

Changes since v3:
 - use of_platform_populate() to populate internal devices using the
   internal register offsets as unit-addresses
 - because we don't use mfd_cells anymore, we cannot use IORESOURCE_REG,
   but instead parse the reg property in each individual driver
 - dropped the following patches because they were already merged:
     gpiolib: Introduce gpiochip_irqchip_add_domain()
     gpio: add a reusable generic gpio_chip using regmap
 - dropped the following patches because they are no longer needed:
     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
 - rephrase commit messages, as suggested by Thomas Gleixner

Changes since v2:
As suggested by Guenter Roeck:
 - added sl28cpld.rst to index.rst
 - removed sl28cpld_wdt_status()
 - reverse christmas tree local variable ordering
 - assign device_property_read_bool() retval directly
 - introduce WDT_DEFAULT_TIMEOUT and use it if the hardware reports
   0 as timeout.
 - set WDOG_HW_RUNNING if applicable
 - remove platform_set_drvdata() leftover

As suggested by Bartosz Golaszewski:
 - don't export gpio_regmap_simple_xlate()
 - combine local variable declaration of the same type
 - drop the "struct gpio_regmap_addr", instead use -1 to force an address
   offset of zero
 - fix typo
 - use "struct gpio_regmap_config" pattern, keep "struct gpio_regmap"
   private. this also means we need a getter/setter for the driver_data
   element.

As suggested by Linus Walleij:
 - don't store irq_domain in gpio-regmap. drop to_irq() altogether for now.
   Instead there is now a new patch which lets us set the irqdomain of the
   gpiochip_irqchip and use its .to_irq() function. This way we don't have
   to expose the gpio_chip inside the gpio-regmap to the user.

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
   implementation
 - 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 (13):
  regmap-irq: use fwnode instead of device node in add_irq_chip()
  mfd: add simple regmap based I2C driver
  dt-bindings: mfd: Add bindings for sl28cpld
  mfd: simple-mfd-i2c: add sl28cpld support
  irqchip: add sl28cpld interrupt controller support
  watchdog: add support for sl28cpld watchdog
  pwm: add support for sl28cpld PWM controller
  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  |  54 +++++
 .../hwmon/kontron,sl28cpld-hwmon.yaml         |  27 +++
 .../kontron,sl28cpld-intc.yaml                |  54 +++++
 .../bindings/mfd/kontron,sl28cpld.yaml        | 153 ++++++++++++
 .../bindings/pwm/kontron,sl28cpld-pwm.yaml    |  35 +++
 .../watchdog/kontron,sl28cpld-wdt.yaml        |  35 +++
 Documentation/hwmon/index.rst                 |   1 +
 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    | 134 ++++++++++
 drivers/base/regmap/regmap-irq.c              |  53 ++--
 drivers/gpio/Kconfig                          |  11 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-sl28cpld.c                  | 161 ++++++++++++
 drivers/hwmon/Kconfig                         |  10 +
 drivers/hwmon/Makefile                        |   1 +
 drivers/hwmon/sl28cpld-hwmon.c                | 149 ++++++++++++
 drivers/irqchip/Kconfig                       |   8 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-sl28cpld.c                |  96 ++++++++
 drivers/mfd/Kconfig                           |   9 +
 drivers/mfd/Makefile                          |   1 +
 drivers/mfd/simple-mfd-i2c.c                  |  51 ++++
 drivers/pwm/Kconfig                           |  10 +
 drivers/pwm/Makefile                          |   1 +
 drivers/pwm/pwm-sl28cpld.c                    | 187 ++++++++++++++
 drivers/watchdog/Kconfig                      |  11 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/sl28cpld_wdt.c               | 229 ++++++++++++++++++
 include/linux/regmap.h                        |  21 +-
 31 files changed, 1531 insertions(+), 33 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/interrupt-controller/kontron,sl28cpld-intc.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-sl28cpld.c
 create mode 100644 drivers/hwmon/sl28cpld-hwmon.c
 create mode 100644 drivers/irqchip/irq-sl28cpld.c
 create mode 100644 drivers/mfd/simple-mfd-i2c.c
 create mode 100644 drivers/pwm/pwm-sl28cpld.c
 create mode 100644 drivers/watchdog/sl28cpld_wdt.c

-- 
2.20.1

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

* [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip()
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-08 10:22   ` Mark Brown
       [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
                   ` (9 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

Convert the argument to the newer fwnode_handle instead a device tree
node. Fortunately, there are no users for now. So this is an easy
change.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Mark, after this patch is reviewed, could already pick it, so it is
less likely, others will use the old regmap_add_irq_chip_np() variant?

Changes since v4:
 - new patch, suggested by Andy

 drivers/base/regmap/regmap-irq.c | 53 +++++++++++++++++---------------
 include/linux/regmap.h           | 21 +++++++------
 2 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 4340e1d268b6..369a57e6f89d 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -541,9 +541,9 @@ static const struct irq_domain_ops regmap_domain_ops = {
 };
 
 /**
- * regmap_add_irq_chip_np() - Use standard regmap IRQ controller handling
+ * regmap_add_irq_chip_fwnode() - Use standard regmap IRQ controller handling
  *
- * @np: The device_node where the IRQ domain should be added to.
+ * @fwnode: The firmware 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.
@@ -557,10 +557,11 @@ 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_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)
+int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
+			       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;
@@ -771,10 +772,12 @@ int regmap_add_irq_chip_np(struct device_node *np, struct regmap *map, int irq,
 	}
 
 	if (irq_base)
-		d->domain = irq_domain_add_legacy(np, chip->num_irqs, irq_base,
+		d->domain = irq_domain_add_legacy(to_of_node(fwnode),
+						  chip->num_irqs, irq_base,
 						  0, &regmap_domain_ops, d);
 	else
-		d->domain = irq_domain_add_linear(np, chip->num_irqs,
+		d->domain = irq_domain_add_linear(to_of_node(fwnode),
+						  chip->num_irqs,
 						  &regmap_domain_ops, d);
 	if (!d->domain) {
 		dev_err(map->dev, "Failed to create IRQ domain\n");
@@ -808,7 +811,7 @@ int regmap_add_irq_chip_np(struct device_node *np, struct regmap *map, int irq,
 	kfree(d);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(regmap_add_irq_chip_np);
+EXPORT_SYMBOL_GPL(regmap_add_irq_chip_fwnode);
 
 /**
  * regmap_add_irq_chip() - Use standard regmap IRQ controller handling
@@ -822,15 +825,15 @@ EXPORT_SYMBOL_GPL(regmap_add_irq_chip_np);
  *
  * Returns 0 on success or an errno on failure.
  *
- * This is the same as regmap_add_irq_chip_np, except that the device
+ * This is the same as regmap_add_irq_chip_fwnode, except that the firmware
  * 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);
+	return regmap_add_irq_chip_fwnode(dev_fwnode(map->dev), map, irq,
+					  irq_flags, irq_base, chip, data);
 }
 EXPORT_SYMBOL_GPL(regmap_add_irq_chip);
 
@@ -899,10 +902,10 @@ static int devm_regmap_irq_chip_match(struct device *dev, void *res, void *data)
 }
 
 /**
- * devm_regmap_add_irq_chip_np() - Resource manager regmap_add_irq_chip_np()
+ * devm_regmap_add_irq_chip_fwnode() - Resource managed regmap_add_irq_chip_fwnode()
  *
  * @dev: The device pointer on which irq_chip belongs to.
- * @np: The device_node where the IRQ domain should be added to.
+ * @fwnode: The firmware 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.
@@ -915,11 +918,12 @@ 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_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)
+int devm_regmap_add_irq_chip_fwnode(struct device *dev,
+				    struct fwnode_handle *fwnode,
+				    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;
@@ -929,8 +933,8 @@ int devm_regmap_add_irq_chip_np(struct device *dev, struct device_node *np,
 	if (!ptr)
 		return -ENOMEM;
 
-	ret = regmap_add_irq_chip_np(np, map, irq, irq_flags, irq_base,
-				     chip, &d);
+	ret = regmap_add_irq_chip_fwnode(fwnode, map, irq, irq_flags, irq_base,
+					 chip, &d);
 	if (ret < 0) {
 		devres_free(ptr);
 		return ret;
@@ -941,7 +945,7 @@ int devm_regmap_add_irq_chip_np(struct device *dev, struct device_node *np,
 	*data = d;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(devm_regmap_add_irq_chip_np);
+EXPORT_SYMBOL_GPL(devm_regmap_add_irq_chip_fwnode);
 
 /**
  * devm_regmap_add_irq_chip() - Resource manager regmap_add_irq_chip()
@@ -964,8 +968,9 @@ int devm_regmap_add_irq_chip(struct device *dev, struct regmap *map, int irq,
 			     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);
+	return devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(map->dev), 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 f4917efed5c3..e3817c097791 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -18,6 +18,7 @@
 #include <linux/bug.h>
 #include <linux/lockdep.h>
 #include <linux/iopoll.h>
+#include <linux/fwnode.h>
 
 struct module;
 struct clk;
@@ -1376,21 +1377,23 @@ 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);
+int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
+			       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);
+int devm_regmap_add_irq_chip_fwnode(struct device *dev,
+				    struct fwnode_handle *fwnode,
+				    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	[flat|nested] 39+ messages in thread

* [PATCH v5 02/13] mfd: add simple regmap based I2C driver
       [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-06 17:53   ` Michael Walle
       [not found]     ` <20200706175353.16404-3-michael-QKn5cuLxLXY@public.gmane.org>
  2020-07-17  9:06     ` Lee Jones
  2020-07-06 17:53   ` [PATCH v5 06/13] watchdog: add support for sl28cpld watchdog Michael Walle
  2020-07-06 17:53   ` [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
  2 siblings, 2 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 UTC (permalink / raw)
  To: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  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,
	Andy Shevchenko, Michael Walle

There are I2C devices which contain several different functions but
doesn't require any special access functions. For these kind of drivers
an I2C regmap should be enough.

Create an I2C driver which creates an I2C regmap and enumerates its
children. If a device wants to use this as its MFD core driver, it has
to add an individual compatible string. It may provide its own regmap
configuration.

Subdevices can use dev_get_regmap() on the parent to get their regmap
instance.

Signed-off-by: Michael Walle <michael-QKn5cuLxLXY@public.gmane.org>
---
Changes since v4:
 - new patch. Lee, please bear with me. I didn't want to delay the
   new version (where a lot of remarks on the other patches were
   addressed) even more, just because we haven't figured out how
   to deal with the MFD part. So for now, I've included this one.

 drivers/mfd/Kconfig          |  9 +++++++
 drivers/mfd/Makefile         |  1 +
 drivers/mfd/simple-mfd-i2c.c | 50 ++++++++++++++++++++++++++++++++++++
 3 files changed, 60 insertions(+)
 create mode 100644 drivers/mfd/simple-mfd-i2c.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 33df0837ab41..f1536a710aca 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
 	  To compile this driver as a module, choose M here: the
 	  module will be called si476x-core.
 
+config MFD_SIMPLE_MFD_I2C
+	tristate "Simple regmap based I2C devices"
+	depends on I2C
+	select MFD_CORE
+	select REGMAP_I2C
+	help
+	  This is a consolidated driver for all MFD devices which are
+	  basically just a regmap bus driver.
+
 config MFD_SM501
 	tristate "Silicon Motion SM501"
 	depends on HAS_DMA
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a60e5f835283..78d24a3e7c9e 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -264,3 +264,4 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
 obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
 
 obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
+obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
new file mode 100644
index 000000000000..1fdca89964b1
--- /dev/null
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -0,0 +1,49 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+
+struct simple_mfd_i2c_config {
+	const struct regmap_config *regmap_config;
+};
+
+static const struct regmap_config simple_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+};
+
+static int simple_mfd_i2c_probe(struct i2c_client *i2c)
+{
+	const struct regmap_config *regmap_config = &simple_regmap_config;
+	const struct simple_mfd_i2c_config *config;
+	struct regmap *regmap;
+
+	config = device_get_match_data(&i2c->dev);
+
+	if (config && config->regmap_config)
+		regmap_config = config->regmap_config;
+
+	regmap = devm_regmap_init_i2c(i2c, regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	return devm_of_platform_populate(&i2c->dev);
+}
+
+static const struct of_device_id simple_mfd_i2c_of_match[] = {
+	{}
+};
+
+static struct i2c_driver simple_mfd_i2c_driver = {
+	.probe_new = simple_mfd_i2c_probe,
+	.driver = {
+		.name = "simple-mfd-i2c",
+		.of_match_table = simple_mfd_i2c_of_match,
+	},
+};
+builtin_i2c_driver(simple_mfd_i2c_driver);
-- 
2.20.1

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

* [PATCH v5 03/13] dt-bindings: mfd: Add bindings for sl28cpld
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
  2020-07-06 17:53 ` [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip() Michael Walle
       [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-13 16:04   ` Rob Herring
  2020-07-06 17:53 ` [PATCH v5 04/13] mfd: simple-mfd-i2c: add sl28cpld support Michael Walle
                   ` (7 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

Add a device tree bindings for the board management controller found on
the Kontron SMARC-sAL28 board.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v4:
 - fix the regex of the unit-address

Changes since v3:
 - see cover letter

 .../bindings/gpio/kontron,sl28cpld-gpio.yaml  |  54 +++++++
 .../hwmon/kontron,sl28cpld-hwmon.yaml         |  27 ++++
 .../kontron,sl28cpld-intc.yaml                |  54 +++++++
 .../bindings/mfd/kontron,sl28cpld.yaml        | 153 ++++++++++++++++++
 .../bindings/pwm/kontron,sl28cpld-pwm.yaml    |  35 ++++
 .../watchdog/kontron,sl28cpld-wdt.yaml        |  35 ++++
 6 files changed, 358 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/interrupt-controller/kontron,sl28cpld-intc.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..9a63a158a796
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/kontron,sl28cpld-gpio.yaml
@@ -0,0 +1,54 @@
+# 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
+
+  interrupts:
+    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/interrupt-controller/kontron,sl28cpld-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/kontron,sl28cpld-intc.yaml
new file mode 100644
index 000000000000..4c39e9ff9aea
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/kontron,sl28cpld-intc.yaml
@@ -0,0 +1,54 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/kontron,sl28cpld-intc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Interrupt controller 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 following interrupts are available. All types and levels are fixed
+  and handled by the board management controller.
+
+  ==== ============= ==================================
+   IRQ line/device   description
+  ==== ============= ==================================
+    0  RTC_INT#      Interrupt line from on-board RTC
+    1  SMB_ALERT#    Event on SMB_ALERT# line (P1)
+    2  ESPI_ALERT0#  Event on ESPI_ALERT0# line (S43)
+    3  ESPI_ALERT1#  Event on ESPI_ALERT1# line (S44)
+    4  PWR_BTN#      Event on PWR_BTN# line (P128)
+    5  SLEEP#        Event on SLEEP# line (S149)
+    6  watchdog      Interrupt of the internal watchdog
+    7  n/a           not used
+  ==== ============= ==================================
+
+properties:
+  compatible:
+    enum:
+      - kontron,sl28cpld-intc
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  "#interrupt-cells":
+    const: 2
+
+  interrupt-controller: true
+
+required:
+  - compatible
+  - interrupts
+  - "#interrupt-cells"
+  - interrupt-controller
+
+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..e3a62db678e7
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/kontron,sl28cpld.yaml
@@ -0,0 +1,153 @@
+# 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.
+
+properties:
+  compatible:
+    const: kontron,sl28cpld-r1
+
+  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-9a-f]+)?$":
+    $ref: ../gpio/kontron,sl28cpld-gpio.yaml
+
+  "^hwmon(@[0-9a-f]+)?$":
+    $ref: ../hwmon/kontron,sl28cpld-hwmon.yaml
+
+  "^interrupt-controller(@[0-9a-f]+)?$":
+    $ref: ../interrupt-controller/kontron,sl28cpld-intc.yaml
+
+  "^pwm(@[0-9a-f]+)?$":
+    $ref: ../pwm/kontron,sl28cpld-pwm.yaml
+
+  "^watchdog(@[0-9a-f]+)?$":
+    $ref: ../watchdog/kontron,sl28cpld-wdt.yaml
+
+required:
+  - "#address-cells"
+  - "#size-cells"
+  - compatible
+  - reg
+
+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-r1";
+            reg = <0x4a>;
+
+            watchdog@4 {
+                compatible = "kontron,sl28cpld-wdt";
+                reg = <0x4>;
+                kontron,assert-wdt-timeout-pin;
+            };
+
+            hwmon@b {
+                compatible = "kontron,sl28cpld-fan";
+                reg = <0xb>;
+            };
+
+            pwm@c {
+                #pwm-cells = <2>;
+                compatible = "kontron,sl28cpld-pwm";
+                reg = <0xc>;
+            };
+
+            pwm@e {
+                #pwm-cells = <2>;
+                compatible = "kontron,sl28cpld-pwm";
+                reg = <0xe>;
+            };
+
+            gpio@10 {
+                compatible = "kontron,sl28cpld-gpio";
+                reg = <0x10>;
+                interrupts-extended = <&gpio2 6
+                               IRQ_TYPE_EDGE_FALLING>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+                gpio-line-names = "a", "b", "c";
+
+                interrupt-controller;
+                #interrupt-cells = <2>;
+            };
+
+            gpio@15 {
+                compatible = "kontron,sl28cpld-gpio";
+                reg = <0x15>;
+                interrupts-extended = <&gpio2 6
+                               IRQ_TYPE_EDGE_FALLING>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+
+                interrupt-controller;
+                #interrupt-cells = <2>;
+            };
+
+            gpio@1a {
+                compatible = "kontron,sl28cpld-gpo";
+                reg = <0x1a>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            gpio@1b {
+                compatible = "kontron,sl28cpld-gpi";
+                reg = <0x1b>;
+
+                gpio-controller;
+                #gpio-cells = <2>;
+            };
+
+            interrupt-controller@1c {
+                compatible = "kontron,sl28cpld-intc";
+                reg = <0x1c>;
+                interrupts-extended = <&gpio2 6
+                               IRQ_TYPE_EDGE_FALLING>;
+
+                interrupt-controller;
+                #interrupt-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	[flat|nested] 39+ messages in thread

* [PATCH v5 04/13] mfd: simple-mfd-i2c: add sl28cpld support
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (2 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 03/13] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-06 17:53 ` [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support Michael Walle
                   ` (6 subsequent siblings)
  10 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

Add the core support for the board management controller found on the
SMARC-sAL28 board.

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

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v4:
 - new patch

 drivers/mfd/simple-mfd-i2c.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
index 1fdca89964b1..c3556c88aa13 100644
--- a/drivers/mfd/simple-mfd-i2c.c
+++ b/drivers/mfd/simple-mfd-i2c.c
@@ -37,6 +37,7 @@ static int simple_mfd_i2c_probe(struct i2c_client *i2c)
 }
 
 static const struct of_device_id simple_mfd_i2c_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-r1" },
 	{}
 };
 
-- 
2.20.1

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

* [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (3 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 04/13] mfd: simple-mfd-i2c: add sl28cpld support Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-08  9:37   ` Marc Zyngier
  2020-07-06 17:53 ` [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller Michael Walle
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

Add support for the interrupt controller inside the sl28 CPLD management
controller.

The interrupt controller can handle at most 8 interrupts and is really
simplistic and consists only of an interrupt mask and an interrupt
pending register.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v4:
 - update copyright year
 - don't use "int irq" instead of "unsigne int irq", because
   platform_get_irq() might return a negative error code. Found by "kernel
   test robot <lkp@intel.com>
 - remove comma in terminator line of the compatible strings list,
   suggested by Andy
 - use newer devm_regmap_add_irq_chip_fwnode()
 - don't use KBUID_MODNAME, suggested by Andy
 - remove the platform device table

Changes since v3:
 - see cover letter

 drivers/irqchip/Kconfig        |  8 +++
 drivers/irqchip/Makefile       |  1 +
 drivers/irqchip/irq-sl28cpld.c | 96 ++++++++++++++++++++++++++++++++++
 3 files changed, 105 insertions(+)
 create mode 100644 drivers/irqchip/irq-sl28cpld.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 216b3b8392b5..b23d23eed3a5 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -246,6 +246,14 @@ 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 "Kontron sl28cpld IRQ controller"
+	select MFD_SIMPLE_MFD_I2C
+	select REGMAP_IRQ
+	help
+	  Interrupt controller driver for the board management controller
+	  found on the Kontron sl28 CPLD.
+
 config ST_IRQCHIP
 	bool
 	select REGMAP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 133f9c45744a..39af7d89204d 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -111,3 +111,4 @@ obj-$(CONFIG_LOONGSON_HTPIC)		+= irq-loongson-htpic.o
 obj-$(CONFIG_LOONGSON_HTVEC)		+= irq-loongson-htvec.o
 obj-$(CONFIG_LOONGSON_PCH_PIC)		+= irq-loongson-pch-pic.o
 obj-$(CONFIG_LOONGSON_PCH_MSI)		+= irq-loongson-pch-msi.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..0aa50d025ef6
--- /dev/null
+++ b/drivers/irqchip/irq-sl28cpld.c
@@ -0,0 +1,96 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld interrupt controller driver
+ *
+ * Copyright 2020 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/property.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 device *dev = &pdev->dev;
+	struct sl28cpld_intc *irqchip;
+	int irq;
+	u32 base;
+	int ret;
+
+	if (!dev->parent)
+		return -ENODEV;
+
+	irqchip = devm_kzalloc(dev, sizeof(*irqchip), GFP_KERNEL);
+	if (!irqchip)
+		return -ENOMEM;
+
+	irqchip->regmap = dev_get_regmap(dev->parent, NULL);
+	if (!irqchip->regmap)
+		return -ENODEV;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &base);
+	if (ret)
+		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 = base + INTC_IP;
+	irqchip->chip.mask_base = base + INTC_IE;
+	irqchip->chip.mask_invert = true,
+	irqchip->chip.ack_base = base + INTC_IP;
+
+	return devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev),
+					       irqchip->regmap, irq,
+					       IRQF_SHARED | IRQF_ONESHOT, 0,
+					       &irqchip->chip,
+					       &irqchip->irq_data);
+}
+
+static const struct of_device_id sl28cpld_intc_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-intc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_intc_of_match);
+
+static struct platform_driver sl28cpld_intc_driver = {
+	.probe = sl28cpld_intc_probe,
+	.driver = {
+		.name = "sl28cpld-intc",
+		.of_match_table = sl28cpld_intc_of_match,
+	}
+};
+module_platform_driver(sl28cpld_intc_driver);
+
+MODULE_DESCRIPTION("sl28cpld Interrupt Controller Driver");
+MODULE_AUTHOR("Michael Walle <michael@walle.cc>");
+MODULE_LICENSE("GPL");
-- 
2.20.1

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

* [PATCH v5 06/13] watchdog: add support for sl28cpld watchdog
       [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
  2020-07-06 17:53   ` [PATCH v5 02/13] mfd: add simple regmap based I2C driver Michael Walle
@ 2020-07-06 17:53   ` Michael Walle
  2020-07-06 17:53   ` [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
  2 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 UTC (permalink / raw)
  To: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  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,
	Andy Shevchenko, Michael Walle

Add 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-QKn5cuLxLXY@public.gmane.org>
Acked-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
Changes since v4:
 - update copyright year
 - remove #include <linux/of_device.h>, suggested by Andy.
 - slightly reworked the error code handling (only style), suggested by
   Andy.
 - Don't use "if (ret < 0)", but only "if (ret)", suggested by Andy.
 - remove comma in terminator line of the compatible strings list
 - don't use KBUID_MODNAME
 - remove the platform device table

Changes since v3:
 - see cover letter

 drivers/watchdog/Kconfig        |  11 ++
 drivers/watchdog/Makefile       |   1 +
 drivers/watchdog/sl28cpld_wdt.c | 229 ++++++++++++++++++++++++++++++++
 3 files changed, 241 insertions(+)
 create mode 100644 drivers/watchdog/sl28cpld_wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4f4687c46d38..c4115ea0d3f8 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 sl28cpld Watchdog"
+	select MFD_SIMPLE_MFD_I2C
+	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 97bed1d3d97c..aa6e41126901 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -225,3 +225,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..a45047d8d9ab
--- /dev/null
+++ b/drivers/watchdog/sl28cpld_wdt.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld watchdog driver
+ *
+ * Copyright 2020 Kontron Europe GmbH
+ */
+
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.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
+
+#define WDT_DEFAULT_TIMEOUT		10
+
+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_get_timeleft(struct watchdog_device *wdd)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(wdt->regmap, wdt->offset + WDT_COUNT, &val);
+	if (ret)
+		return 0;
+
+	return val;
+}
+
+static int sl28cpld_wdt_set_timeout(struct watchdog_device *wdd,
+				    unsigned int timeout)
+{
+	struct sl28cpld_wdt *wdt = watchdog_get_drvdata(wdd);
+	int ret;
+
+	ret = regmap_write(wdt->regmap, wdt->offset + WDT_TIMEOUT, timeout);
+	if (ret)
+		return ret;
+
+	wdd->timeout = timeout;
+
+	return 0;
+}
+
+static const struct watchdog_info sl28cpld_wdt_info = {
+	.options = WDIOF_MAGICCLOSE | WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity = "sl28cpld watchdog",
+};
+
+static struct watchdog_ops sl28cpld_wdt_ops = {
+	.owner = THIS_MODULE,
+	.start = sl28cpld_wdt_start,
+	.stop = sl28cpld_wdt_stop,
+	.ping = sl28cpld_wdt_ping,
+	.set_timeout = sl28cpld_wdt_set_timeout,
+	.get_timeleft = sl28cpld_wdt_get_timeleft,
+};
+
+static int sl28cpld_wdt_probe(struct platform_device *pdev)
+{
+	struct watchdog_device *wdd;
+	struct sl28cpld_wdt *wdt;
+	unsigned int status;
+	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;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &wdt->offset);
+	if (ret)
+		return -EINVAL;
+
+	wdt->assert_wdt_timeout = device_property_read_bool(&pdev->dev,
+							    "kontron,assert-wdt-timeout-pin");
+
+	/* 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);
+	watchdog_stop_on_reboot(wdd);
+
+	/*
+	 * Read the status early, in case of an error, we haven't modified the
+	 * hardware.
+	 */
+	ret = regmap_read(wdt->regmap, wdt->offset + WDT_CTRL, &status);
+	if (ret)
+		return ret;
+
+	/*
+	 * Initial timeout value, may be overwritten by device tree or module
+	 * parmeter in watchdog_init_timeout().
+	 *
+	 * Reading a zero here means that either the hardware has a default
+	 * value of zero (which is very unlikely and definitely a hardware
+	 * bug) or the bootloader set it to zero. In any case, we handle
+	 * this case gracefully and set out own timeout.
+	 */
+	ret = regmap_read(wdt->regmap, wdt->offset + WDT_TIMEOUT, &val);
+	if (ret)
+		return ret;
+
+	if (val)
+		wdd->timeout = val;
+	else
+		wdd->timeout = WDT_DEFAULT_TIMEOUT;
+
+	watchdog_init_timeout(wdd, timeout, &pdev->dev);
+	sl28cpld_wdt_set_timeout(wdd, wdd->timeout);
+
+	/* if the watchdog is locked, we set nowayout */
+	if (status & WDT_CTRL_LOCK)
+		nowayout = true;
+	watchdog_set_nowayout(wdd, nowayout);
+
+	/*
+	 * If watchdog is already running, keep it enabled, but make
+	 * sure its mode is set correctly.
+	 */
+	if (status & WDT_CTRL_EN) {
+		sl28cpld_wdt_start(wdd);
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+	}
+
+	ret = devm_watchdog_register_device(&pdev->dev, wdd);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed to register watchdog device\n");
+		return ret;
+	}
+
+	dev_info(&pdev->dev, "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 struct platform_driver sl28cpld_wdt_driver = {
+	.probe = sl28cpld_wdt_probe,
+	.driver = {
+		.name = "sl28cpld-wdt",
+		.of_match_table = sl28cpld_wdt_of_match,
+	},
+};
+module_platform_driver(sl28cpld_wdt_driver);
+
+MODULE_DESCRIPTION("sl28cpld Watchdog Driver");
+MODULE_AUTHOR("Michael Walle <michael-QKn5cuLxLXY@public.gmane.org>");
+MODULE_LICENSE("GPL");
-- 
2.20.1

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

* [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (4 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-09  8:50   ` Uwe Kleine-König
  2020-07-06 17:53 ` [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
                   ` (4 subsequent siblings)
  10 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

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

The controller has one PWM channel and can just generate four distinct
frequencies.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v4:
 - update copyright year
 - remove #include <linux/of_device.h>, suggested by Andy.
 - make the pwm mode table look nicer, suggested by Lee.
 - use dev_get_drvdata(chip->dev) instead of container_of(), suggested by
   Lee.
 - use whole sentence in comments, suggested by Lee.
 - renamed the local "struct sl28cpld_pwm" variable to "priv" everywhere,
   suggested by Lee.
 - use pwm_{get,set}_relative_duty_cycle(), suggested by Andy.
 - make the comment about the 250Hz hardware limitation clearer
 - don't use "if (ret < 0)", but only "if (ret)", suggested by Andy.
 - don't use KBUID_MODNAME
 - remove comma in terminator line of the compatible strings list
 - remove the platform device table

Changes since v3:
 - see cover letter

 drivers/pwm/Kconfig        |  10 ++
 drivers/pwm/Makefile       |   1 +
 drivers/pwm/pwm-sl28cpld.c | 187 +++++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+)
 create mode 100644 drivers/pwm/pwm-sl28cpld.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index 7dbcf6973d33..a0d50d70c3b9 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -428,6 +428,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 sl28cpld PWM support"
+	select MFD_SIMPLE_MFD_I2C
+	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 2c2ba0a03557..cbdcd55d69ee 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..8ee286b605bf
--- /dev/null
+++ b/drivers/pwm/pwm-sl28cpld.c
@@ -0,0 +1,187 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld PWM driver
+ *
+ * Copyright 2020 Kontron Europe GmbH
+ */
+
+#include <linux/bitfield.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.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 void sl28cpld_pwm_get_state(struct pwm_chip *chip,
+				   struct pwm_device *pwm,
+				   struct pwm_state *state)
+{
+	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
+	static struct sl28cpld_pwm_config *config;
+	unsigned int reg;
+	unsigned int mode;
+
+	regmap_read(priv->regmap, priv->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(priv->regmap, priv->offset + PWM_CYCLE, &reg);
+	pwm_set_relative_duty_cycle(state, reg, 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 *priv = dev_get_drvdata(chip->dev);
+	struct sl28cpld_pwm_config *config;
+	unsigned int cycle;
+	int ret;
+	int mode;
+	u8 ctrl;
+
+	/* Get the configuration by comparing the 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 = pwm_get_relative_duty_cycle(state, config->max_duty_cycle);
+
+	/*
+	 * The hardware doesn't allow to set max_duty_cycle if the
+	 * 250Hz mode is enabled, thus we have to trap that here.
+	 * But because a 100% duty cycle is equal on all modes, i.e.
+	 * it is just a "all-high" output, we trap any case with a
+	 * 100% duty cycle and use the 500Hz mode.
+	 */
+	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(priv->regmap, priv->offset + PWM_CTRL, ctrl);
+	if (ret)
+		return ret;
+
+	return regmap_write(priv->regmap, priv->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 *priv;
+	struct pwm_chip *chip;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!priv->regmap)
+		return -ENODEV;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset);
+	if (ret)
+		return -EINVAL;
+
+	/* Initialize the pwm_chip structure */
+	chip = &priv->pwm_chip;
+	chip->dev = &pdev->dev;
+	chip->ops = &sl28cpld_pwm_ops;
+	chip->base = -1;
+	chip->npwm = 1;
+
+	ret = pwmchip_add(&priv->pwm_chip);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, priv);
+
+	return 0;
+}
+
+static int sl28cpld_pwm_remove(struct platform_device *pdev)
+{
+	struct sl28cpld_pwm *priv = platform_get_drvdata(pdev);
+
+	return pwmchip_remove(&priv->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 struct platform_driver sl28cpld_pwm_driver = {
+	.probe = sl28cpld_pwm_probe,
+	.remove	= sl28cpld_pwm_remove,
+	.driver = {
+		.name = "sl28cpld-pwm",
+		.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	[flat|nested] 39+ messages in thread

* [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (5 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
       [not found]   ` <20200706175353.16404-9-michael-QKn5cuLxLXY@public.gmane.org>
  2020-07-17  0:25   ` kernel test robot
  2020-07-06 17:53 ` [PATCH v5 09/13] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
                   ` (3 subsequent siblings)
  10 siblings, 2 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

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

A controller has 8 lines. There are three different flavors:
full-featured GPIO with interrupt support, input-only and output-only.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v4:
 - update copyright year
 - remove #include <linux/of_device.h>, suggested by Andy.
 - use device_get_match_data(), suggested by Andy.
 - drop the irq_support variable, instead call _init_irq() directly,
   suggested by Andy.
 - also move the irq code a bit around to make it look nicer
 - drop "struct sl28cpld_gpio". We don't need to actually store the
   irq_chip and irq_chip_data, everything is device resource managed.
 - use 100 chars line limit, suggested by Andy.
 - remove the platform device table
 - don't use KBUID_MODNAME

Changes since v3:
 - see cover letter

 drivers/gpio/Kconfig         |  11 +++
 drivers/gpio/Makefile        |   1 +
 drivers/gpio/gpio-sl28cpld.c | 161 +++++++++++++++++++++++++++++++++++
 3 files changed, 173 insertions(+)
 create mode 100644 drivers/gpio/gpio-sl28cpld.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 05e0801c6a78..f67df2b02de4 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 sl28cpld GPIO support"
+	select MFD_SIMPLE_MFD_I2C
+	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 ef666cfef9d0..cb8ed4463265 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -131,6 +131,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..889b8f5622c2
--- /dev/null
+++ b/drivers/gpio/gpio-sl28cpld.c
@@ -0,0 +1,161 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld GPIO driver
+ *
+ * Copyright 2020 Michael Walle <michael@walle.cc>
+ */
+
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/gpio/regmap.h>
+#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>
+
+/* 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,
+};
+
+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 platform_device *pdev,
+				  unsigned int base,
+				  struct gpio_regmap_config *config)
+{
+	struct regmap_irq_chip_data *irq_data;
+	struct regmap_irq_chip *irq_chip;
+	struct device *dev = &pdev->dev;
+	int irq, ret;
+
+	if (!device_property_read_bool(dev, "interrupt-controller"))
+		return 0;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	irq_chip = devm_kzalloc(dev, sizeof(*irq_chip), GFP_KERNEL);
+	if (!irq_chip)
+		return -ENOMEM;
+
+	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;
+
+	ret = devm_regmap_add_irq_chip_fwnode(dev, dev_fwnode(dev),
+					      config->regmap, irq,
+					      IRQF_SHARED | IRQF_ONESHOT,
+					      0, irq_chip, &irq_data);
+	if (ret)
+		return ret;
+
+	config->irq_domain = regmap_irq_get_domain(irq_data);
+
+	return 0;
+}
+
+static int sl28cpld_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_regmap_config config = {0};
+	enum sl28cpld_gpio_type type;
+	struct regmap *regmap;
+	u32 base;
+	int ret;
+
+	if (!pdev->dev.parent)
+		return -ENODEV;
+
+	type = (uintptr_t)device_get_match_data(&pdev->dev);
+	if (!type)
+		return -ENODEV;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &base);
+	if (ret)
+		return -EINVAL;
+
+	regmap = dev_get_regmap(pdev->dev.parent, NULL);
+	if (!regmap)
+		return -ENODEV;
+
+	config.regmap = regmap;
+	config.parent = &pdev->dev;
+	config.ngpio = 8;
+
+	switch (type) {
+	case SL28CPLD_GPIO:
+		config.reg_dat_base = base + GPIO_REG_IN;
+		config.reg_set_base = base + GPIO_REG_OUT;
+		/* reg_dir_out_base might be zero */
+		config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
+
+		/* This type supports interrupts */
+		ret = sl28cpld_gpio_irq_init(pdev, base, &config);
+		if (ret)
+			return ret;
+		break;
+	case SL28CPLD_GPO:
+		config.reg_set_base = base + GPO_REG_OUT;
+		break;
+	case SL28CPLD_GPI:
+		config.reg_dat_base = base + GPI_REG_IN;
+		break;
+	default:
+		dev_err(&pdev->dev, "unknown type %d\n", type);
+		return -ENODEV;
+	}
+
+	return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
+}
+
+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 struct platform_driver sl28cpld_gpio_driver = {
+	.probe = sl28cpld_gpio_probe,
+	.driver = {
+		.name = "sl28cpld-gpio",
+		.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	[flat|nested] 39+ messages in thread

* [PATCH v5 09/13] hwmon: add support for the sl28cpld hardware monitoring controller
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (6 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-06 17:53 ` [PATCH v5 10/13] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

Add 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>
Acked-by: Guenter Roeck <linux@roeck-us.net>
---
Changes since v4:
 - update copyright year
 - remove #include <linux/of_device.h>, suggested by Andy.
 - use PTR_ERR_OR_ZERO(), suggested by Andy.
 - remove the platform device table
 - don't use KBUID_MODNAME

Changes since v3:
 - see cover letter

 Documentation/hwmon/index.rst    |   1 +
 Documentation/hwmon/sl28cpld.rst |  36 ++++++++
 drivers/hwmon/Kconfig            |  10 +++
 drivers/hwmon/Makefile           |   1 +
 drivers/hwmon/sl28cpld-hwmon.c   | 142 +++++++++++++++++++++++++++++++
 5 files changed, 190 insertions(+)
 create mode 100644 Documentation/hwmon/sl28cpld.rst
 create mode 100644 drivers/hwmon/sl28cpld-hwmon.c

diff --git a/Documentation/hwmon/index.rst b/Documentation/hwmon/index.rst
index 55ff4b7c5349..1f4beb7449c7 100644
--- a/Documentation/hwmon/index.rst
+++ b/Documentation/hwmon/index.rst
@@ -153,6 +153,7 @@ Hardware Monitoring Kernel Drivers
    sht3x
    shtc1
    sis5595
+   sl28cpld
    smm665
    smsc47b397
    smsc47m192
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 288ae9f63588..af2823f09bd1 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1459,6 +1459,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 sl28cpld hardware monitoring driver"
+	select MFD_SIMPLE_MFD_I2C
+	help
+	  If you say yes here you get support for the fan supervisor of the
+	  sl28cpld board management controller.
+
+	  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 3e32c21f5efe..03822f6bf970 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -158,6 +158,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..e48f58ec5b9c
--- /dev/null
+++ b/drivers/hwmon/sl28cpld-hwmon.c
@@ -0,0 +1,142 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * sl28cpld hardware monitoring driver
+ *
+ * Copyright 2020 Kontron Europe GmbH
+ */
+
+#include <linux/bitfield.h>
+#include <linux/hwmon.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/property.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 sl28cpld_hwmon *hwmon;
+	struct device *hwmon_dev;
+	int ret;
+
+	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;
+
+	ret = device_property_read_u32(&pdev->dev, "reg", &hwmon->offset);
+	if (ret)
+		return -EINVAL;
+
+	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_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id sl28cpld_hwmon_of_match[] = {
+	{ .compatible = "kontron,sl28cpld-fan" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, sl28cpld_hwmon_of_match);
+
+static struct platform_driver sl28cpld_hwmon_driver = {
+	.probe = sl28cpld_hwmon_probe,
+	.driver = {
+		.name = "sl28cpld-fan",
+		.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	[flat|nested] 39+ messages in thread

* [PATCH v5 10/13] arm64: dts: freescale: sl28: enable sl28cpld
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (7 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 09/13] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-06 17:53 ` [PATCH v5 11/13] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
  2020-07-06 17:53 ` [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
  10 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, Michael Walle

Add the board management controller node.

Signed-off-by: Michael Walle <michael@walle.cc>
---
Changes since v4:
 - none

Changes since v3:
 - see cover letter

 .../freescale/fsl-ls1028a-kontron-sl28.dts    | 102 ++++++++++++++++++
 1 file changed, 102 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 360b3a168c10..8712fe82727b 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";
@@ -170,6 +171,107 @@
 		reg = <0x32>;
 	};
 
+	sl28cpld@4a {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "kontron,sl28cpld-r1";
+		reg = <0x4a>;
+
+		watchdog@4 {
+			compatible = "kontron,sl28cpld-wdt";
+			reg = <0x4>;
+			kontron,assert-wdt-timeout-pin;
+		};
+
+		hwmon@b {
+			compatible = "kontron,sl28cpld-fan";
+			reg = <0xb>;
+		};
+
+		sl28cpld_pwm0: pwm@c {
+			#pwm-cells = <2>;
+			compatible = "kontron,sl28cpld-pwm";
+			reg = <0xc>;
+		};
+
+		sl28cpld_pwm1: pwm@e {
+			#pwm-cells = <2>;
+			compatible = "kontron,sl28cpld-pwm";
+			reg = <0xe>;
+		};
+
+		sl28cpld_gpio0: gpio@10 {
+			compatible = "kontron,sl28cpld-gpio";
+			reg = <0x10>;
+			interrupts-extended = <&gpio2 6
+					       IRQ_TYPE_EDGE_FALLING>;
+
+			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@15 {
+			compatible = "kontron,sl28cpld-gpio";
+			reg = <0x15>;
+			interrupts-extended = <&gpio2 6
+					       IRQ_TYPE_EDGE_FALLING>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-line-names =
+				"GPIO8", "GPIO9", "GPIO10", "GPIO11",
+				"", "", "", "";
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+
+		sl28cpld_gpio2: gpio@1a {
+			compatible = "kontron,sl28cpld-gpo";
+			reg = <0x1a>;
+
+			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@1b {
+			compatible = "kontron,sl28cpld-gpi";
+			reg = <0x1b>;
+
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-line-names =
+				"Power button", "Force recovery", "Sleep",
+				"Battery low", "Lid state", "Charging",
+				"Charger present", "";
+		};
+
+		sl28cpld_intc: interrupt-controller@1c {
+			compatible = "kontron,sl28cpld-intc";
+			reg = <0x1c>;
+			interrupts-extended = <&gpio2 6
+					       IRQ_TYPE_EDGE_FALLING>;
+
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
+
 	eeprom@50 {
 		compatible = "atmel,24c32";
 		reg = <0x50>;
-- 
2.20.1

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

* [PATCH v5 11/13] arm64: dts: freescale: sl28: map GPIOs to input events
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (8 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 10/13] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
  2020-07-06 17:53 ` [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
  10 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, 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>
---
Changes since v4:
 - none

Changes since v3:
 - see cover letter

 .../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 8712fe82727b..c4fd99efdbba 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_intc
+					       4 IRQ_TYPE_EDGE_BOTH>;
+			linux,code = <KEY_POWER>;
+			label = "Power";
+		};
+
+		sleep-button {
+			interrupts-extended = <&sl28cpld_intc
+					       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	[flat|nested] 39+ messages in thread

* [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support
       [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
  2020-07-06 17:53   ` [PATCH v5 02/13] mfd: add simple regmap based I2C driver Michael Walle
  2020-07-06 17:53   ` [PATCH v5 06/13] watchdog: add support for sl28cpld watchdog Michael Walle
@ 2020-07-06 17:53   ` Michael Walle
  2020-07-17  8:36     ` Pavel Machek
  2 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 UTC (permalink / raw)
  To: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r
  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,
	Andy Shevchenko, 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-QKn5cuLxLXY@public.gmane.org>
---
Changes since v4:
 - none

Changes since v3:
 - see cover letter

 .../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	[flat|nested] 39+ messages in thread

* [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support
  2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
                   ` (9 preceding siblings ...)
  2020-07-06 17:53 ` [PATCH v5 11/13] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
@ 2020-07-06 17:53 ` Michael Walle
       [not found]   ` <20200706175353.16404-14-michael-QKn5cuLxLXY@public.gmane.org>
  10 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-06 17:53 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,
	Andy Shevchenko, 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>
---
Changes since v4:
 - none

Changes since v3:
 - see cover letter

 .../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	[flat|nested] 39+ messages in thread

* Re: [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller
       [not found]   ` <20200706175353.16404-9-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-08  7:35     ` Linus Walleij
  0 siblings, 0 replies; 39+ messages in thread
From: Linus Walleij @ 2020-07-08  7:35 UTC (permalink / raw)
  To: Michael Walle
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA, 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

On Mon, Jul 6, 2020 at 7:57 PM Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:

> Add support for the GPIO controller of the sl28 board management
> controller. This driver is part of a multi-function device.
>
> A controller has 8 lines. There are three different flavors:
> full-featured GPIO with interrupt support, input-only and output-only.
>
> Signed-off-by: Michael Walle <michael-QKn5cuLxLXY@public.gmane.org>
> ---
> Changes since v4:

This is awesomely elegant now.
Reviewed-by: Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

I suppose it needs merging through MFD with the rest.

Yours,
Linus Walleij

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

* Re: [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support
       [not found]   ` <20200706175353.16404-14-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-08  7:39     ` Linus Walleij
       [not found]       ` <CACRpkdaPO7CGNrxmjL5QH1cxP5wqku1oMtQaQgJfeKiKqiGAOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 39+ messages in thread
From: Linus Walleij @ 2020-07-08  7:39 UTC (permalink / raw)
  To: Michael Walle
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA, 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

Hi Michael,

just a drive-by-comment:

On Mon, Jul 6, 2020 at 7:57 PM Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:

> 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-QKn5cuLxLXY@public.gmane.org>

If you have a cooling device like this, do you also have a temperature
sensor? In that case it makes sense to add a thermal zone and a
policy, such as I did for a device in
6e97f0aaca4ca778905dd1dc667cbf379f4cae15

Yours,
Linus Walleij

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

* Re: [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support
       [not found]       ` <CACRpkdaPO7CGNrxmjL5QH1cxP5wqku1oMtQaQgJfeKiKqiGAOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2020-07-08  7:56         ` Michael Walle
  0 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-08  7:56 UTC (permalink / raw)
  To: Linus Walleij
  Cc: open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA, 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

Hi Linus,

Am 2020-07-08 09:39, schrieb Linus Walleij:
> just a drive-by-comment:
> 
> On Mon, Jul 6, 2020 at 7:57 PM Michael Walle <michael-QKn5cuLxLXY@public.gmane.org> wrote:
> 
>> 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-QKn5cuLxLXY@public.gmane.org>
> 
> If you have a cooling device like this, do you also have a temperature
> sensor? In that case it makes sense to add a thermal zone and a
> policy, such as I did for a device in
> 6e97f0aaca4ca778905dd1dc667cbf379f4cae15

Yep, the CPU and DDR controller have temperatur sensors and there are
already thermal zones for them. We have the fan linked to the policies
in our vendor DTS overlay. For now I didn't want to include that here,
mainly because there are no labels in the fsl-ls1028a.dtsi for the
thermal zone/cooling maps/trips. But this is still on my TODO, when
this series finally make it into the kernel ;)

-michael

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

* Re: [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support
  2020-07-06 17:53 ` [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support Michael Walle
@ 2020-07-08  9:37   ` Marc Zyngier
  0 siblings, 0 replies; 39+ messages in thread
From: Marc Zyngier @ 2020-07-08  9:37 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, Mark Brown

On 2020-07-06 18:53, Michael Walle wrote:
> Add support for the interrupt controller inside the sl28 CPLD 
> management
> controller.
> 
> The interrupt controller can handle at most 8 interrupts and is really
> simplistic and consists only of an interrupt mask and an interrupt
> pending register.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Changes since v4:
>  - update copyright year
>  - don't use "int irq" instead of "unsigne int irq", because
>    platform_get_irq() might return a negative error code. Found by 
> "kernel
>    test robot <lkp@intel.com>
>  - remove comma in terminator line of the compatible strings list,
>    suggested by Andy
>  - use newer devm_regmap_add_irq_chip_fwnode()
>  - don't use KBUID_MODNAME, suggested by Andy
>  - remove the platform device table
> 
> Changes since v3:
>  - see cover letter
> 
>  drivers/irqchip/Kconfig        |  8 +++
>  drivers/irqchip/Makefile       |  1 +
>  drivers/irqchip/irq-sl28cpld.c | 96 ++++++++++++++++++++++++++++++++++
>  3 files changed, 105 insertions(+)
>  create mode 100644 drivers/irqchip/irq-sl28cpld.c

Acked-by: Marc Zyngier <maz@kernel.org>

Given the dependency on the MFD patches, I assume this will
be routed to that subsystem. Please let me know if you want
it to be handled differently.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

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

* Re: [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip()
  2020-07-06 17:53 ` [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip() Michael Walle
@ 2020-07-08 10:22   ` Mark Brown
  0 siblings, 0 replies; 39+ messages in thread
From: Mark Brown @ 2020-07-08 10:22 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


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

On Mon, Jul 06, 2020 at 07:53:41PM +0200, Michael Walle wrote:
> Convert the argument to the newer fwnode_handle instead a device tree
> node. Fortunately, there are no users for now. So this is an easy
> change.

The following changes since commit b3a9e3b9622ae10064826dccb4f7a52bd88c7407:

  Linux 5.8-rc1 (2020-06-14 12:45:04 -0700)

are available in the Git repository at:

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

for you to fetch changes up to 5cc2013bfeee756a1ee6da9bfbe42e52b4695035:

  regmap-irq: use fwnode instead of device node in add_irq_chip() (2020-07-08 11:15:12 +0100)

----------------------------------------------------------------
regmap: Change node pointer to fwnode in new IRQ API

----------------------------------------------------------------
Michael Walle (1):
      regmap-irq: use fwnode instead of device node in add_irq_chip()

 drivers/base/regmap/regmap-irq.c | 53 ++++++++++++++++++++++------------------
 include/linux/regmap.h           | 21 +++++++++-------
 2 files changed, 41 insertions(+), 33 deletions(-)

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

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-06 17:53 ` [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller Michael Walle
@ 2020-07-09  8:50   ` Uwe Kleine-König
       [not found]     ` <20200709085006.b54ype3p4yu64upl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-07-09  8:50 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman


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

Hello Michael,

On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote:
> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> new file mode 100644
> index 000000000000..8ee286b605bf
> --- /dev/null
> +++ b/drivers/pwm/pwm-sl28cpld.c
> @@ -0,0 +1,187 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * sl28cpld PWM driver
> + *
> + * Copyright 2020 Kontron Europe GmbH
> + */

Is there publically available documenation available? If so please add a
link here.

> +
> +#include <linux/bitfield.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.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

Please use a less generic prefix for your defines. Also I like having
the defines for field names include register name. Something like:

	#define PWM_SL28CPLD_CTRL		0x00
	#define PWM_SL28CPLD_CTRL_ENABLE		BIT(7)
	#define PWM_SL28CPLD_CTRL_MODE_MASK		GENMASK(1, 0)
	#define PWM_SL28CPLD_CTRL_MODE_250HZ		FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK, 0)

> +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[] = {

const ? (Or drop as the values can be easily computed, see below.)

> +	[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 void sl28cpld_pwm_get_state(struct pwm_chip *chip,
> +				   struct pwm_device *pwm,
> +				   struct pwm_state *state)
> +{
> +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> +	static struct sl28cpld_pwm_config *config;
> +	unsigned int reg;
> +	unsigned int mode;
> +
> +	regmap_read(priv->regmap, priv->offset + PWM_CTRL, &reg);
> +
> +	state->enabled = reg & PWM_ENABLE;

Would it be more consisted to use FIELD_GET here, too?

> +
> +	mode = FIELD_GET(PWM_MODE_MASK, reg);
> +	config = &sl28cpld_pwm_config[mode];
> +	state->period = config->period_ns;

I wonder if this could be done more effectively without the above table.
Something like:

	state->period = 4000000 >> mode.
	
(with a #define for 4000000 of course).

> +	regmap_read(priv->regmap, priv->offset + PWM_CYCLE, &reg);
> +	pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle);

Oh, what a creative idea to use pwm_set_relative_duty_cycle here.
Unfortunately it's using the wrong rounding strategy. Please enable
PWM_DEBUG which should diagnose these problems (given enough testing).

(Hmm, on second thought I'm not sure that rounding is relevant with the
numbers of this hardware. Still it's wrong in general and I don't want
to have others copy this.)

> +}
> +
> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
> +			      const struct pwm_state *state)
> +{
> +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> +	struct sl28cpld_pwm_config *config;
> +	unsigned int cycle;
> +	int ret;
> +	int mode;
> +	u8 ctrl;
> +
> +	/* Get the configuration by comparing the 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;

You're supposed to pick the biggest period that isn't bigger than the
requested period. So something like:

	switch(period) {
	case 4000000 ... UINT_MAX:
		mode = 0;
		break;
	case 2000000 ... 3999999:
		mode = 1;
		break;
	...
	}

(or:

	if period >= 4000000:
		mode = 0
	else:
		// I think ... please double-check
		mode = ilog2(4000000 / (period + 1)) + 1

	if mode > 3:
		return -ERANGE;
)

	real_period = 4000000 >> mode;

> +	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
> +	if (state->enabled)
> +		ctrl |= PWM_ENABLE;
> +
> +	cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle);

Again the rounding is wrong. You need need to round down the requested
duty_cycle to the next possible value. So something like:

	duty_cycle = min(real_period, state->duty_cycle);

	cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode);

which can be further simplified to

	cycle = duty_cycle / 31250

.

> +	/*
> +	 * The hardware doesn't allow to set max_duty_cycle if the
> +	 * 250Hz mode is enabled, thus we have to trap that here.
> +	 * But because a 100% duty cycle is equal on all modes, i.e.

It depends on how picky you are if you can agree here. Please document
this in a Limitations paragraph at the top of the driver similar to
drivers/pwm/pwm-rcar.c and others.

> +	 * it is just a "all-high" output, we trap any case with a
> +	 * 100% duty cycle and use the 500Hz mode.

Please only trap on 250Hz mode. (Can be done using: if (cycle == 0x80) I
think)

> +	 */
> +	if (cycle == config->max_duty_cycle) {
> +		ctrl &= ~PWM_MODE_MASK;
> +		ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ);
> +		cycle = PWM_CYCLE_MAX;
	
I would have expected 0x40 here instead of 0x7f?

> +	}
> +
> +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, (u8)cycle);

I assume this can result in broken output? Consider the hardware runs
with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
0x42: Can this result in a period that has mode = 0 & cycle = 0x23?

If this cannot be avoided, please document this in the Limitations
paragraph.

> +}
> +
> +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 *priv;
> +	struct pwm_chip *chip;
> +	int ret;
> +
> +	if (!pdev->dev.parent)
> +		return -ENODEV;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> +	if (!priv->regmap)
> +		return -ENODEV;
> +
> +	ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset);
> +	if (ret)
> +		return -EINVAL;
> +
> +	/* Initialize the pwm_chip structure */
> +	chip = &priv->pwm_chip;
> +	chip->dev = &pdev->dev;
> +	chip->ops = &sl28cpld_pwm_ops;
> +	chip->base = -1;
> +	chip->npwm = 1;
> +
> +	ret = pwmchip_add(&priv->pwm_chip);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, priv);
> +
> +	return 0;
> +}

Please add error messages with some details for the error paths
(preferable using %pe to indicate the error code).

> +static int sl28cpld_pwm_remove(struct platform_device *pdev)
> +{
> +	struct sl28cpld_pwm *priv = platform_get_drvdata(pdev);
> +
> +	return pwmchip_remove(&priv->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 struct platform_driver sl28cpld_pwm_driver = {
> +	.probe = sl28cpld_pwm_probe,
> +	.remove	= sl28cpld_pwm_remove,
> +	.driver = {
> +		.name = "sl28cpld-pwm",
> +		.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");

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
       [not found]     ` <20200709085006.b54ype3p4yu64upl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
@ 2020-07-11 17:28       ` Michael Walle
  2020-07-13  8:47         ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-11 17:28 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Lee Jones, Thierry Reding, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

Hi Uwe,

first of all, thank you for that thorough review.

Am 2020-07-09 10:50, schrieb Uwe Kleine-König:
> On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote:
>> diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
>> new file mode 100644
>> index 000000000000..8ee286b605bf
>> --- /dev/null
>> +++ b/drivers/pwm/pwm-sl28cpld.c
>> @@ -0,0 +1,187 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * sl28cpld PWM driver
>> + *
>> + * Copyright 2020 Kontron Europe GmbH
>> + */
> 
> Is there publically available documenation available? If so please add 
> a
> link here.

Unfortunately not. But it should be easy enough and I'll describe it
briefly in the header.

>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.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
> 
> Please use a less generic prefix for your defines. Also I like having
> the defines for field names include register name. Something like:
> 
> 	#define PWM_SL28CPLD_CTRL		0x00
> 	#define PWM_SL28CPLD_CTRL_ENABLE		BIT(7)
> 	#define PWM_SL28CPLD_CTRL_MODE_MASK		GENMASK(1, 0)

Ok.

> 	#define
> PWM_SL28CPLD_CTRL_MODE_250HZ		FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK,
> 0)

Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the 
code,
so you can actually use the normalized enumeration values, too?

Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is
more accurate.

>> +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[] = {
> 
> const ? (Or drop as the values can be easily computed, see below.)
> 
>> +	[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 void sl28cpld_pwm_get_state(struct pwm_chip *chip,
>> +				   struct pwm_device *pwm,
>> +				   struct pwm_state *state)
>> +{
>> +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
>> +	static struct sl28cpld_pwm_config *config;
>> +	unsigned int reg;
>> +	unsigned int mode;
>> +
>> +	regmap_read(priv->regmap, priv->offset + PWM_CTRL, &reg);
>> +
>> +	state->enabled = reg & PWM_ENABLE;
> 
> Would it be more consisted to use FIELD_GET here, too?

I had used FIELD_GET only for bit-fields with more than one bit,
i.e. no flags. But that is just a matter of taste, I guess. I'd
prefer to keep the simple "reg & PWM_ENABLE". If you insist on
the FIELD_GET() I'll change it ;)

>> +
>> +	mode = FIELD_GET(PWM_MODE_MASK, reg);
>> +	config = &sl28cpld_pwm_config[mode];
>> +	state->period = config->period_ns;
> 
> I wonder if this could be done more effectively without the above 
> table.
> Something like:
> 
> 	state->period = 4000000 >> mode.

The reason I introduced a lookup table here was that I need a
list of the supported modes; I wasn't aware of the rounding.
See also below.

> (with a #define for 4000000 of course).
> 
>> +	regmap_read(priv->regmap, priv->offset + PWM_CYCLE, &reg);
>> +	pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle);
> 
> Oh, what a creative idea to use pwm_set_relative_duty_cycle here.

What is that helper for then? The former versions did the same
calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But
I guess then it was also rounding the wrong way.

> Unfortunately it's using the wrong rounding strategy. Please enable
> PWM_DEBUG which should diagnose these problems (given enough testing).

Is there any written documentation on how to round, i.e. up or down?
I had a look Documentation/driver-api/pwm.rst again. But couldn't find
anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few
drivers use it, so I did the same ;)

> (Hmm, on second thought I'm not sure that rounding is relevant with the
> numbers of this hardware. Still it's wrong in general and I don't want
> to have others copy this.)
> 
>> +}
>> +
>> +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct 
>> pwm_device *pwm,
>> +			      const struct pwm_state *state)
>> +{
>> +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
>> +	struct sl28cpld_pwm_config *config;
>> +	unsigned int cycle;
>> +	int ret;
>> +	int mode;
>> +	u8 ctrl;
>> +
>> +	/* Get the configuration by comparing the 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;
> 
> You're supposed to pick the biggest period that isn't bigger than the
> requested period. So something like:
> 
> 	switch(period) {
> 	case 4000000 ... UINT_MAX:
> 		mode = 0;
> 		break;
> 	case 2000000 ... 3999999:
> 		mode = 1;
> 		break;
> 	...
> 	}
> 
> (or:
> 
> 	if period >= 4000000:
> 		mode = 0
> 	else:
> 		// I think ... please double-check
> 		mode = ilog2(4000000 / (period + 1)) + 1
> 
> 	if mode > 3:
> 		return -ERANGE;
> )

I see. In this case I can of course drop the table. But the rounding
will be then very coarse for this driver. And there is no way to get
the value which is actually set, right? You can just read the cached
value. So that value might be far off the actual one set in the
hardware.

During testing I've also found the following problem: Assume we set
a period of 5000000ns; this will be rounded to 4000000ns and written
to the hardware. But the usable duty cycle is still 0..5000000ns. The
driver will translate this input in the following manner:
  - 0..4000000 -> 0%..100%
  - >4000000 -> 100%
Is this behavior intended? Even for PWM hardware which supports finer
grained frequencies there will be some upper and lower limits. Is
the user of the PWM supposed to know these?

> 
> 	real_period = 4000000 >> mode;
> 
>> +	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
>> +	if (state->enabled)
>> +		ctrl |= PWM_ENABLE;
>> +
>> +	cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle);
> 
> Again the rounding is wrong. You need need to round down the requested
> duty_cycle to the next possible value. So something like:
> 
> 	duty_cycle = min(real_period, state->duty_cycle);
> 
> 	cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode);
> 
> which can be further simplified to
> 
> 	cycle = duty_cycle / 31250

Mh, this made me think where that "magic" number is coming from. Turns
out this is the NSECS_PE_SEC / base clock of the PWM.

I guess I'll rework the get_state() and apply() to just use this
base frequency, dropping the table etc.

Btw what about the polarity. Do I have to support it or can I
return an error code if its != PWM_POLARITY_NORMAL? If so, which
error code? EINVAL? I know I could just invert the duty cycle in
software, but shouldn't this be done in the core for any controller
which doesn't support changing the polarity in hardware?

> 
> .
> 
>> +	/*
>> +	 * The hardware doesn't allow to set max_duty_cycle if the
>> +	 * 250Hz mode is enabled, thus we have to trap that here.
>> +	 * But because a 100% duty cycle is equal on all modes, i.e.
> 
> It depends on how picky you are if you can agree here.

why is that? The only drawback is that the mode is changed without
the user seeing it. But the PWM subsystem returns the cached state,
right? get_state() is called only on device request (and during
debug it seems). Actually, enabling PWM_DEBUG might choke on this
workaround (".apply didn't pick the best available period"). Is
this ok?

> Please document
> this in a Limitations paragraph at the top of the driver similar to
> drivers/pwm/pwm-rcar.c and others.

sure.

> 
>> +	 * it is just a "all-high" output, we trap any case with a
>> +	 * 100% duty cycle and use the 500Hz mode.
> 
> Please only trap on 250Hz mode. (Can be done using: if (cycle == 0x80) 
> I
> think)

you are correct.

> 
>> +	 */
>> +	if (cycle == config->max_duty_cycle) {
>> +		ctrl &= ~PWM_MODE_MASK;
>> +		ctrl |= FIELD_PREP(PWM_MODE_MASK, PWM_MODE_500HZ);
>> +		cycle = PWM_CYCLE_MAX;
> 
> I would have expected 0x40 here instead of 0x7f?

Yes, but technically, any value above 0x40 will do it. But you
are correct, that is wrong and misleading.

>> +	}
>> +
>> +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE, 
>> (u8)cycle);
> 
> I assume this can result in broken output? Consider the hardware runs
> with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
> 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?

Isn't that always the case if a write may fail and there are more than
one register to configure? For example, have a look at pwm-iqs620a.c.
Btw. the get_state might also fail, but there is no return value to
return the error.

> If this cannot be avoided, please document this in the Limitations
> paragraph.

Sure. There might be (or most likely are) gliches when you change the
mode.

> 
>> +}
>> +
>> +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 *priv;
>> +	struct pwm_chip *chip;
>> +	int ret;
>> +
>> +	if (!pdev->dev.parent)
>> +		return -ENODEV;
>> +
>> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	priv->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	if (!priv->regmap)
>> +		return -ENODEV;
>> +
>> +	ret = device_property_read_u32(&pdev->dev, "reg", &priv->offset);
>> +	if (ret)
>> +		return -EINVAL;
>> +
>> +	/* Initialize the pwm_chip structure */
>> +	chip = &priv->pwm_chip;
>> +	chip->dev = &pdev->dev;
>> +	chip->ops = &sl28cpld_pwm_ops;
>> +	chip->base = -1;
>> +	chip->npwm = 1;
>> +
>> +	ret = pwmchip_add(&priv->pwm_chip);
>> +	if (ret)
>> +		return ret;
>> +
>> +	platform_set_drvdata(pdev, priv);
>> +
>> +	return 0;
>> +}
> 
> Please add error messages with some details for the error paths
> (preferable using %pe to indicate the error code).

Ok.

> 
>> +static int sl28cpld_pwm_remove(struct platform_device *pdev)
>> +{
>> +	struct sl28cpld_pwm *priv = platform_get_drvdata(pdev);
>> +
>> +	return pwmchip_remove(&priv->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 struct platform_driver sl28cpld_pwm_driver = {
>> +	.probe = sl28cpld_pwm_probe,
>> +	.remove	= sl28cpld_pwm_remove,
>> +	.driver = {
>> +		.name = "sl28cpld-pwm",
>> +		.of_match_table = sl28cpld_pwm_of_match,
>> +	},
>> +};
>> +module_platform_driver(sl28cpld_pwm_driver);
>> +
>> +MODULE_DESCRIPTION("sl28cpld PWM Driver");
>> +MODULE_AUTHOR("Michael Walle <michael-QKn5cuLxLXY@public.gmane.org>");
>> +MODULE_LICENSE("GPL");

-michael

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-11 17:28       ` Michael Walle
@ 2020-07-13  8:47         ` Uwe Kleine-König
  2020-07-14 11:31           ` Michael Walle
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-07-13  8:47 UTC (permalink / raw)
  To: Michael Walle, Thierry Reding
  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, Wim Van Sebroeck, Shawn Guo, Li Yang, Thomas Gleixner,
	Jason Cooper, Marc Zyngier, Mark Brown, Greg Kroah-Hartman,
	Andy Shevchenko


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

Hello Michael,

On Sat, Jul 11, 2020 at 07:28:05PM +0200, Michael Walle wrote:
> Am 2020-07-09 10:50, schrieb Uwe Kleine-König:
> > On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote:
> > > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
> > > new file mode 100644
> > > index 000000000000..8ee286b605bf
> > > --- /dev/null
> > > +++ b/drivers/pwm/pwm-sl28cpld.c
> > > @@ -0,0 +1,187 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * sl28cpld PWM driver
> > > + *
> > > + * Copyright 2020 Kontron Europe GmbH
> > > + */
> > 
> > Is there publically available documenation available? If so please add a
> > link here.
> 
> Unfortunately not. But it should be easy enough and I'll describe it
> briefly in the header.

That's fine.

> > > +#include <linux/bitfield.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.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
> > 
> > Please use a less generic prefix for your defines. Also I like having
> > the defines for field names include register name. Something like:
> > 
> > 	#define PWM_SL28CPLD_CTRL		0x00
> > 	#define PWM_SL28CPLD_CTRL_ENABLE		BIT(7)
> > 	#define PWM_SL28CPLD_CTRL_MODE_MASK		GENMASK(1, 0)
> 
> Ok.
> 
> > 	#define
> > PWM_SL28CPLD_CTRL_MODE_250HZ		FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK,
> > 0)
> 
> Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the code,
> so you can actually use the normalized enumeration values, too?

yeah, looks sane.

> Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is
> more accurate.

Whatever suits you and is consistent is fine for me.

> > > +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[] = {
> > 
> > const ? (Or drop as the values can be easily computed, see below.)
> > 
> > > +	[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 void sl28cpld_pwm_get_state(struct pwm_chip *chip,
> > > +				   struct pwm_device *pwm,
> > > +				   struct pwm_state *state)
> > > +{
> > > +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> > > +	static struct sl28cpld_pwm_config *config;
> > > +	unsigned int reg;
> > > +	unsigned int mode;
> > > +
> > > +	regmap_read(priv->regmap, priv->offset + PWM_CTRL, &reg);
> > > +
> > > +	state->enabled = reg & PWM_ENABLE;
> > 
> > Would it be more consisted to use FIELD_GET here, too?
> 
> I had used FIELD_GET only for bit-fields with more than one bit,
> i.e. no flags. But that is just a matter of taste, I guess. I'd
> prefer to keep the simple "reg & PWM_ENABLE". If you insist on
> the FIELD_GET() I'll change it ;)

I think using FIELD_GET is more consistent, but I won't insist.

> > > +	mode = FIELD_GET(PWM_MODE_MASK, reg);
> > > +	config = &sl28cpld_pwm_config[mode];
> > > +	state->period = config->period_ns;
> > 
> > I wonder if this could be done more effectively without the above table.
> > Something like:
> > 
> > 	state->period = 4000000 >> mode.
> 
> The reason I introduced a lookup table here was that I need a
> list of the supported modes; I wasn't aware of the rounding.

List of supported modes = [0, 1, 2, 3], isn't it?

> See also below.
> 
> > (with a #define for 4000000 of course).
> > 
> > > +	regmap_read(priv->regmap, priv->offset + PWM_CYCLE, &reg);
> > > +	pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle);
> > 
> > Oh, what a creative idea to use pwm_set_relative_duty_cycle here.
> 
> What is that helper for then? The former versions did the same
> calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But
> I guess then it was also rounding the wrong way.

Yes. In my book pwm_set_relative_duty_cycle is for consumers. And if
DIV_ROUND_CLOSEST_ULL is the right thing for them depends on their use
case.

> > Unfortunately it's using the wrong rounding strategy. Please enable
> > PWM_DEBUG which should diagnose these problems (given enough testing).
> 
> Is there any written documentation on how to round, i.e. up or down?

There are the checks implemented for PWM_DEBUG. I started to work on the
documentation
(https://patchwork.ozlabs.org/project/linux-pwm/patch/20191209213233.29574-2-u.kleine-koenig@pengutronix.de/)
but didn't get feedback yet. (And the rounding rules are not included
there.)

> I had a look Documentation/driver-api/pwm.rst again. But couldn't find
> anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few
> drivers use it, so I did the same ;)

Yes, the rounding requirement is new and many driver's are not
conforming (yet).

> > (Hmm, on second thought I'm not sure that rounding is relevant with the
> > numbers of this hardware. Still it's wrong in general and I don't want
> > to have others copy this.)
> > 
> > > +}
> > > +
> > > +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct
> > > pwm_device *pwm,
> > > +			      const struct pwm_state *state)
> > > +{
> > > +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
> > > +	struct sl28cpld_pwm_config *config;
> > > +	unsigned int cycle;
> > > +	int ret;
> > > +	int mode;
> > > +	u8 ctrl;
> > > +
> > > +	/* Get the configuration by comparing the 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;
> > 
> > You're supposed to pick the biggest period that isn't bigger than the
> > requested period. So something like:
> > 
> > 	switch(period) {
> > 	case 4000000 ... UINT_MAX:
> > 		mode = 0;
> > 		break;
> > 	case 2000000 ... 3999999:
> > 		mode = 1;
> > 		break;
> > 	...
> > 	}
> > 
> > (or:
> > 
> > 	if period >= 4000000:
> > 		mode = 0
> > 	else:
> > 		// I think ... please double-check
> > 		mode = ilog2(4000000 / (period + 1)) + 1
> > 
> > 	if mode > 3:
> > 		return -ERANGE;
> > )
> 
> I see. In this case I can of course drop the table. But the rounding
> will be then very coarse for this driver. And there is no way to get
> the value which is actually set, right? You can just read the cached
> value. So that value might be far off the actual one set in the
> hardware.

Yes, we once changed pwm_get_rate to return the actually implemented
setting, but this broke some stuff; see commit
40a6b9a00930fd6b59aa2eb6135abc2efe5440c3.

I already thought about proposing pwm_get_rate_hw(), but for now there
is (AFAICT) no user who would need it. And it's hard to know which
variant is actually preferred by consumers. My expectation is that most
don't even care.

I also have a pwm_round_rate() function in mind that will give you the
actual rate without applying it. This can then be used by consumers who
care. But also there is no user who would need it today.

> During testing I've also found the following problem: Assume we set
> a period of 5000000ns; this will be rounded to 4000000ns and written
> to the hardware. But the usable duty cycle is still 0..5000000ns. The
> driver will translate this input in the following manner:
>  - 0..4000000 -> 0%..100%
>  - >4000000 -> 100%
> Is this behavior intended?

It's expected.

> Even for PWM hardware which supports finer
> grained frequencies there will be some upper and lower limits. Is
> the user of the PWM supposed to know these?

There is nothing we can do on hardware imposed limits. In practise it
doesn't seem to matter. Also note that most consumers get a proposed
period length.

> > 	real_period = 4000000 >> mode;
> > 
> > > +	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
> > > +	if (state->enabled)
> > > +		ctrl |= PWM_ENABLE;
> > > +
> > > +	cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle);
> > 
> > Again the rounding is wrong. You need need to round down the requested
> > duty_cycle to the next possible value. So something like:
> > 
> > 	duty_cycle = min(real_period, state->duty_cycle);
> > 
> > 	cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode);
> > 
> > which can be further simplified to
> > 
> > 	cycle = duty_cycle / 31250
> 
> Mh, this made me think where that "magic" number is coming from. Turns
> out this is the NSECS_PE_SEC / base clock of the PWM.

It's a simplification of the line above, so it is 4000000 / 0x80. (But
it's not by chance this matches NSECS_PER_SEC / base clock of course.)

> I guess I'll rework the get_state() and apply() to just use this
> base frequency, dropping the table etc.
> 
> Btw what about the polarity. Do I have to support it or can I
> return an error code if its != PWM_POLARITY_NORMAL? If so, which
> error code? EINVAL?

..ooOO(Did I really miss that during review? Bummer)

If your hardware only support normal polarity, only implement this and
return -EINVAL for inverted polarity requests.

> I know I could just invert the duty cycle in
> software, but shouldn't this be done in the core for any controller
> which doesn't support changing the polarity in hardware?

Please don't. This should indeed be done at the framework level. (But
I'm not convinced doing this unconditionally is a good idea.)

> > > +	/*
> > > +	 * The hardware doesn't allow to set max_duty_cycle if the
> > > +	 * 250Hz mode is enabled, thus we have to trap that here.
> > > +	 * But because a 100% duty cycle is equal on all modes, i.e.
> > 
> > It depends on how picky you are if you can agree here.
> 
> why is that? The only drawback is that the mode is changed without
> the user seeing it.

Ideally periods are completed before they change. So if a user requests
.period = .duty_cycle = 100ms with having the PWM disabled before and
afterwards, the expectation is that it is active for (an integer
multiple of) 100 ms. But honestly there are not many hardware
implementation + driver combos that get this right.

> But the PWM subsystem returns the cached state,
> right? get_state() is called only on device request (and during
> debug it seems). Actually, enabling PWM_DEBUG might choke on this
> workaround (".apply didn't pick the best available period"). Is
> this ok?

hmm, I didn't consider this when writing the checks for PWM_DEBUG.
According to the currently checked rules the expected configuration is
to pick the 250Hz mode and use cycle = 0x7f. Hmm, I have to think about
this. Maybe we should weaken the check to the cases with
0 < duty_cycle < period. Thierry, what do you think?

Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems
to be far from reality. (Is it?) 

> > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
> > > (u8)cycle);
> > 
> > I assume this can result in broken output? Consider the hardware runs
> > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
> > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
> 
> Isn't that always the case if a write may fail and there are more than
> one register to configure?

Depending on hardware capabilities you might not be able to prevent
this yes. Unfortunately this is quite common.

But there are hardware implementations that are not prone to such
failures. (E.g. the registers written can be only shadow values that are
latched into hardware only when the last value is written.)

> For example, have a look at pwm-iqs620a.c.
> Btw. the get_state might also fail, but there is no return value to
> return the error.

Yes, changing this is on my todo list.

> > If this cannot be avoided, please document this in the Limitations
> > paragraph.
> 
> Sure. There might be (or most likely are) gliches when you change the
> mode.

If you change only cycle but not mode, does the hardware complete the
currently running period? What about disable()?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v5 03/13] dt-bindings: mfd: Add bindings for sl28cpld
  2020-07-06 17:53 ` [PATCH v5 03/13] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
@ 2020-07-13 16:04   ` Rob Herring
  0 siblings, 0 replies; 39+ messages in thread
From: Rob Herring @ 2020-07-13 16:04 UTC (permalink / raw)
  To: Michael Walle
  Cc: Li Yang, Greg Kroah-Hartman, linux-pwm, Marc Zyngier, devicetree,
	Thierry Reding, Linus Walleij, Shawn Guo, Guenter Roeck,
	Jean Delvare, linux-kernel, linux-hwmon, Uwe Kleine-König,
	Andy Shevchenko, Mark Brown, Bartosz Golaszewski, Rob Herring,
	Lee Jones, linux-watchdog, linux-arm-kernel

On Mon, 06 Jul 2020 19:53:43 +0200, Michael Walle wrote:
> Add a device tree bindings for the board management controller found on
> the Kontron SMARC-sAL28 board.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Changes since v4:
>  - fix the regex of the unit-address
> 
> Changes since v3:
>  - see cover letter
> 
>  .../bindings/gpio/kontron,sl28cpld-gpio.yaml  |  54 +++++++
>  .../hwmon/kontron,sl28cpld-hwmon.yaml         |  27 ++++
>  .../kontron,sl28cpld-intc.yaml                |  54 +++++++
>  .../bindings/mfd/kontron,sl28cpld.yaml        | 153 ++++++++++++++++++
>  .../bindings/pwm/kontron,sl28cpld-pwm.yaml    |  35 ++++
>  .../watchdog/kontron,sl28cpld-wdt.yaml        |  35 ++++
>  6 files changed, 358 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/interrupt-controller/kontron,sl28cpld-intc.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
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-13  8:47         ` Uwe Kleine-König
@ 2020-07-14 11:31           ` Michael Walle
  2020-07-14 16:08             ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-14 11:31 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

Hi Uwe,

Am 2020-07-13 10:47, schrieb Uwe Kleine-König:
> On Sat, Jul 11, 2020 at 07:28:05PM +0200, Michael Walle wrote:
>> Am 2020-07-09 10:50, schrieb Uwe Kleine-König:
>> > On Mon, Jul 06, 2020 at 07:53:47PM +0200, Michael Walle wrote:
>> > > diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c
>> > > new file mode 100644
>> > > index 000000000000..8ee286b605bf
>> > > --- /dev/null
>> > > +++ b/drivers/pwm/pwm-sl28cpld.c
>> > > @@ -0,0 +1,187 @@
>> > > +// SPDX-License-Identifier: GPL-2.0-only
>> > > +/*
>> > > + * sl28cpld PWM driver
>> > > + *
>> > > + * Copyright 2020 Kontron Europe GmbH
>> > > + */
>> >
>> > Is there publically available documenation available? If so please add a
>> > link here.
>> 
>> Unfortunately not. But it should be easy enough and I'll describe it
>> briefly in the header.
> 
> That's fine.
> 
>> > > +#include <linux/bitfield.h>
>> > > +#include <linux/kernel.h>
>> > > +#include <linux/mod_devicetable.h>
>> > > +#include <linux/module.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
>> >
>> > Please use a less generic prefix for your defines. Also I like having
>> > the defines for field names include register name. Something like:
>> >
>> > 	#define PWM_SL28CPLD_CTRL		0x00
>> > 	#define PWM_SL28CPLD_CTRL_ENABLE		BIT(7)
>> > 	#define PWM_SL28CPLD_CTRL_MODE_MASK		GENMASK(1, 0)
>> 
>> Ok.
>> 
>> > 	#define
>> > PWM_SL28CPLD_CTRL_MODE_250HZ		FIELD_PREP(PWM_SL28CPLD_CTRL_MODE_MASK,
>> > 0)
>> 
>> Shouldn't we just "#define ..MODE_250HZ 1" use FIELD_PREP inside the 
>> code,
>> so you can actually use the normalized enumeration values, too?
> 
> yeah, looks sane.
> 
>> Actually, I'll rename the PWM_MODE to PWM_PRESCALER, because that is
>> more accurate.
> 
> Whatever suits you and is consistent is fine for me.
> 
>> > > +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[] = {
>> >
>> > const ? (Or drop as the values can be easily computed, see below.)
>> >
>> > > +	[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 void sl28cpld_pwm_get_state(struct pwm_chip *chip,
>> > > +				   struct pwm_device *pwm,
>> > > +				   struct pwm_state *state)
>> > > +{
>> > > +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
>> > > +	static struct sl28cpld_pwm_config *config;
>> > > +	unsigned int reg;
>> > > +	unsigned int mode;
>> > > +
>> > > +	regmap_read(priv->regmap, priv->offset + PWM_CTRL, &reg);
>> > > +
>> > > +	state->enabled = reg & PWM_ENABLE;
>> >
>> > Would it be more consisted to use FIELD_GET here, too?
>> 
>> I had used FIELD_GET only for bit-fields with more than one bit,
>> i.e. no flags. But that is just a matter of taste, I guess. I'd
>> prefer to keep the simple "reg & PWM_ENABLE". If you insist on
>> the FIELD_GET() I'll change it ;)
> 
> I think using FIELD_GET is more consistent, but I won't insist.
> 
>> > > +	mode = FIELD_GET(PWM_MODE_MASK, reg);
>> > > +	config = &sl28cpld_pwm_config[mode];
>> > > +	state->period = config->period_ns;
>> >
>> > I wonder if this could be done more effectively without the above table.
>> > Something like:
>> >
>> > 	state->period = 4000000 >> mode.
>> 
>> The reason I introduced a lookup table here was that I need a
>> list of the supported modes; I wasn't aware of the rounding.
> 
> List of supported modes = [0, 1, 2, 3], isn't it?

Back then it was the list of supported periods. But because we do
the rounding now, we won't need it anymore.

>> See also below.
>> 
>> > (with a #define for 4000000 of course).
>> >
>> > > +	regmap_read(priv->regmap, priv->offset + PWM_CYCLE, &reg);
>> > > +	pwm_set_relative_duty_cycle(state, reg, config->max_duty_cycle);
>> >
>> > Oh, what a creative idea to use pwm_set_relative_duty_cycle here.
>> 
>> What is that helper for then? The former versions did the same
>> calculations (i.e. DIV_ROUND_CLOSEST_ULL()) just open coded. But
>> I guess then it was also rounding the wrong way.
> 
> Yes. In my book pwm_set_relative_duty_cycle is for consumers. And if
> DIV_ROUND_CLOSEST_ULL is the right thing for them depends on their use
> case.
> 
>> > Unfortunately it's using the wrong rounding strategy. Please enable
>> > PWM_DEBUG which should diagnose these problems (given enough testing).
>> 
>> Is there any written documentation on how to round, i.e. up or down?
> 
> There are the checks implemented for PWM_DEBUG. I started to work on 
> the
> documentation
> (https://patchwork.ozlabs.org/project/linux-pwm/patch/20191209213233.29574-2-u.kleine-koenig@pengutronix.de/)
> but didn't get feedback yet. (And the rounding rules are not included
> there.)
> 
>> I had a look Documentation/driver-api/pwm.rst again. But couldn't find
>> anything. A grep DIV_ROUND_CLOSEST_ULL() turns out that quite a few
>> drivers use it, so I did the same ;)
> 
> Yes, the rounding requirement is new and many driver's are not
> conforming (yet).

ok, I'll then compute everything then and drop the table.

>> > (Hmm, on second thought I'm not sure that rounding is relevant with the
>> > numbers of this hardware. Still it's wrong in general and I don't want
>> > to have others copy this.)
>> >
>> > > +}
>> > > +
>> > > +static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct
>> > > pwm_device *pwm,
>> > > +			      const struct pwm_state *state)
>> > > +{
>> > > +	struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev);
>> > > +	struct sl28cpld_pwm_config *config;
>> > > +	unsigned int cycle;
>> > > +	int ret;
>> > > +	int mode;
>> > > +	u8 ctrl;
>> > > +
>> > > +	/* Get the configuration by comparing the 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;
>> >
>> > You're supposed to pick the biggest period that isn't bigger than the
>> > requested period. So something like:
>> >
>> > 	switch(period) {
>> > 	case 4000000 ... UINT_MAX:
>> > 		mode = 0;
>> > 		break;
>> > 	case 2000000 ... 3999999:
>> > 		mode = 1;
>> > 		break;
>> > 	...
>> > 	}
>> >
>> > (or:
>> >
>> > 	if period >= 4000000:
>> > 		mode = 0
>> > 	else:
>> > 		// I think ... please double-check
>> > 		mode = ilog2(4000000 / (period + 1)) + 1
>> >
>> > 	if mode > 3:
>> > 		return -ERANGE;
>> > )
>> 
>> I see. In this case I can of course drop the table. But the rounding
>> will be then very coarse for this driver. And there is no way to get
>> the value which is actually set, right? You can just read the cached
>> value. So that value might be far off the actual one set in the
>> hardware.
> 
> Yes, we once changed pwm_get_rate to return the actually implemented
> setting, but this broke some stuff; see commit
> 40a6b9a00930fd6b59aa2eb6135abc2efe5440c3.
> 
> I already thought about proposing pwm_get_rate_hw(), but for now there
> is (AFAICT) no user who would need it. And it's hard to know which
> variant is actually preferred by consumers. My expectation is that most
> don't even care.
> 
> I also have a pwm_round_rate() function in mind that will give you the
> actual rate without applying it. This can then be used by consumers who
> care. But also there is no user who would need it today.

Ok. I take it that all such improvements are still in the making ;)

>> During testing I've also found the following problem: Assume we set
>> a period of 5000000ns; this will be rounded to 4000000ns and written
>> to the hardware. But the usable duty cycle is still 0..5000000ns. The
>> driver will translate this input in the following manner:
>>  - 0..4000000 -> 0%..100%
>>  - >4000000 -> 100%
>> Is this behavior intended?
> 
> It's expected.
ok

>> Even for PWM hardware which supports finer
>> grained frequencies there will be some upper and lower limits. Is
>> the user of the PWM supposed to know these?
> 
> There is nothing we can do on hardware imposed limits. In practise it
> doesn't seem to matter. Also note that most consumers get a proposed
> period length.
> 
>> > 	real_period = 4000000 >> mode;
>> >
>> > > +	ctrl = FIELD_PREP(PWM_MODE_MASK, mode);
>> > > +	if (state->enabled)
>> > > +		ctrl |= PWM_ENABLE;
>> > > +
>> > > +	cycle = pwm_get_relative_duty_cycle(state, config->max_duty_cycle);
>> >
>> > Again the rounding is wrong. You need need to round down the requested
>> > duty_cycle to the next possible value. So something like:
>> >
>> > 	duty_cycle = min(real_period, state->duty_cycle);
>> >
>> > 	cycle = duty_cycle * (0x80 >> mode) / (4000000 >> mode);
>> >
>> > which can be further simplified to
>> >
>> > 	cycle = duty_cycle / 31250
>> 
>> Mh, this made me think where that "magic" number is coming from. Turns
>> out this is the NSECS_PE_SEC / base clock of the PWM.
> 
> It's a simplification of the line above, so it is 4000000 / 0x80. (But
> it's not by chance this matches NSECS_PER_SEC / base clock of course.)
> 
>> I guess I'll rework the get_state() and apply() to just use this
>> base frequency, dropping the table etc.
>> 
>> Btw what about the polarity. Do I have to support it or can I
>> return an error code if its != PWM_POLARITY_NORMAL? If so, which
>> error code? EINVAL?
> 
> ..ooOO(Did I really miss that during review? Bummer)
> 
> If your hardware only support normal polarity, only implement this and
> return -EINVAL for inverted polarity requests.

ok

>> I know I could just invert the duty cycle in
>> software, but shouldn't this be done in the core for any controller
>> which doesn't support changing the polarity in hardware?
> 
> Please don't. This should indeed be done at the framework level. (But
> I'm not convinced doing this unconditionally is a good idea.)
> 
>> > > +	/*
>> > > +	 * The hardware doesn't allow to set max_duty_cycle if the
>> > > +	 * 250Hz mode is enabled, thus we have to trap that here.
>> > > +	 * But because a 100% duty cycle is equal on all modes, i.e.
>> >
>> > It depends on how picky you are if you can agree here.
>> 
>> why is that? The only drawback is that the mode is changed without
>> the user seeing it.
> 
> Ideally periods are completed before they change. So if a user requests
> .period = .duty_cycle = 100ms with having the PWM disabled before and
> afterwards, the expectation is that it is active for (an integer
> multiple of) 100 ms. But honestly there are not many hardware
> implementation + driver combos that get this right.
> 
>> But the PWM subsystem returns the cached state,
>> right? get_state() is called only on device request (and during
>> debug it seems). Actually, enabling PWM_DEBUG might choke on this
>> workaround (".apply didn't pick the best available period"). Is
>> this ok?
> 
> hmm, I didn't consider this when writing the checks for PWM_DEBUG.
> According to the currently checked rules the expected configuration is
> to pick the 250Hz mode and use cycle = 0x7f.

Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 
500Hz
mode.

> Hmm, I have to think about
> this. Maybe we should weaken the check to the cases with
> 0 < duty_cycle < period. Thierry, what do you think?
> 
> Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f 
> seems
> to be far from reality. (Is it?)

If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because
the user wants an all-high line, but in the end it would be a toggling
line. It wouldn't be that bad for anything in between 0% and 100% but
IMHO its bad for exactly 0% and 100%.

You could also ask the driver about known quirks, like special 0% and
100% handling and exclude it from the tests accordingly.


>> > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
>> > > +	if (ret)
>> > > +		return ret;
>> > > +
>> > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
>> > > (u8)cycle);
>> >
>> > I assume this can result in broken output? Consider the hardware runs
>> > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
>> > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
>> 
>> Isn't that always the case if a write may fail and there are more than
>> one register to configure?
> 
> Depending on hardware capabilities you might not be able to prevent
> this yes. Unfortunately this is quite common.
> 
> But there are hardware implementations that are not prone to such
> failures. (E.g. the registers written can be only shadow values that 
> are
> latched into hardware only when the last value is written.)

Maybe this could be improved in the future.

> 
>> For example, have a look at pwm-iqs620a.c.
>> Btw. the get_state might also fail, but there is no return value to
>> return the error.
> 
> Yes, changing this is on my todo list.
> 
>> > If this cannot be avoided, please document this in the Limitations
>> > paragraph.
>> 
>> Sure. There might be (or most likely are) gliches when you change the
>> mode.
> 
> If you change only cycle but not mode, does the hardware complete the
> currently running period?

No it does not.

> What about disable()?

Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
fix that (in hardware), not much we can do in the driver here. We are
_very_ constraint in size, therefore all that little edge cases fall off
the table.

I'll post a new version soon.

-michael

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-14 11:31           ` Michael Walle
@ 2020-07-14 16:08             ` Uwe Kleine-König
  2020-07-14 21:09               ` Michael Walle
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-07-14 16:08 UTC (permalink / raw)
  To: Michael Walle
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman


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

Hello Michael,

On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote:
> Am 2020-07-13 10:47, schrieb Uwe Kleine-König:
> > I already thought about proposing pwm_get_rate_hw(), but for now there
> > is (AFAICT) no user who would need it. And it's hard to know which
> > variant is actually preferred by consumers. My expectation is that most
> > don't even care.
> > 
> > I also have a pwm_round_rate() function in mind that will give you the
> > actual rate without applying it. This can then be used by consumers who
> > care. But also there is no user who would need it today.
> 
> Ok. I take it that all such improvements are still in the making ;)

If you have a real use case, present it, then I give it a boost on my
todo list.

> > > But the PWM subsystem returns the cached state,
> > > right? get_state() is called only on device request (and during
> > > debug it seems). Actually, enabling PWM_DEBUG might choke on this
> > > workaround (".apply didn't pick the best available period"). Is
> > > this ok?
> > 
> > hmm, I didn't consider this when writing the checks for PWM_DEBUG.
> > According to the currently checked rules the expected configuration is
> > to pick the 250Hz mode and use cycle = 0x7f.
> 
> Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 500Hz
> mode.

No, when I thought about a sane set of rules (and so checks for
PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for
all otherwise available period lengths. I'm still amazed sometimes how
different the capabilities of different implementations for something so
seemingly easy like a PWM are.

> > Hmm, I have to think about
> > this. Maybe we should weaken the check to the cases with
> > 0 < duty_cycle < period. Thierry, what do you think?
> > 
> > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems
> > to be far from reality. (Is it?)
> 
> If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because
> the user wants an all-high line, but in the end it would be a toggling
> line. It wouldn't be that bad for anything in between 0% and 100% but
> IMHO its bad for exactly 0% and 100%.
> 
> You could also ask the driver about known quirks, like special 0% and
> 100% handling and exclude it from the tests accordingly.

Do you care enough to propose a patch? You're in the situation to test
it.

> > > > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
> > > > > +	if (ret)
> > > > > +		return ret;
> > > > > +
> > > > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
> > > > > (u8)cycle);
> > > >
> > > > I assume this can result in broken output? Consider the hardware runs
> > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
> > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
> > > 
> > > Isn't that always the case if a write may fail and there are more than
> > > one register to configure?
> > 
> > Depending on hardware capabilities you might not be able to prevent
> > this yes. Unfortunately this is quite common.
> > 
> > But there are hardware implementations that are not prone to such
> > failures. (E.g. the registers written can be only shadow values that are
> > latched into hardware only when the last value is written.)
> 
> Maybe this could be improved in the future.

We should somewhere describe, what an ideal PWM can do. 
My wishlist (just as it comes to my mind, so no guarantee of
completeness):

 - can do 0% duty cycle for all supported period lengths
 - can do 100% duty cycle for all supported period lengths
 - supports both polarities
 - supports immediate change of configuration and after completion of
   the currently running period
 - atomic update (i.e. if you go from configuration A to configuration B
   the hardware guarantees to only emit periods of type A and then type
   B. (Depending on the item above, the last A period might be cut off.)
 - emits an irq when configuration changes

> > If you change only cycle but not mode, does the hardware complete the
> > currently running period?
> 
> No it does not.

Please document this as a Limitation.
 
> > What about disable()?
> 
> Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
> fix that (in hardware), not much we can do in the driver here. We are
> _very_ constraint in size, therefore all that little edge cases fall off
> the table.

You're saying that on disable the hardware emits a constant high level
for one cycle? I hope not ...

I never programmed a CPLD to emulate a hardware PWM, but I wonder if
these are really edge cases that increase the size of the binary?!

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-14 16:08             ` Uwe Kleine-König
@ 2020-07-14 21:09               ` Michael Walle
  2020-07-15 16:36                 ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-14 21:09 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

Hi Uwe,

Am 2020-07-14 18:08, schrieb Uwe Kleine-König:
> On Tue, Jul 14, 2020 at 01:31:13PM +0200, Michael Walle wrote:
>> Am 2020-07-13 10:47, schrieb Uwe Kleine-König:
>> > I already thought about proposing pwm_get_rate_hw(), but for now there
>> > is (AFAICT) no user who would need it. And it's hard to know which
>> > variant is actually preferred by consumers. My expectation is that most
>> > don't even care.
>> >
>> > I also have a pwm_round_rate() function in mind that will give you the
>> > actual rate without applying it. This can then be used by consumers who
>> > care. But also there is no user who would need it today.
>> 
>> Ok. I take it that all such improvements are still in the making ;)
> 
> If you have a real use case, present it, then I give it a boost on my
> todo list.

Not really.

>> > > But the PWM subsystem returns the cached state,
>> > > right? get_state() is called only on device request (and during
>> > > debug it seems). Actually, enabling PWM_DEBUG might choke on this
>> > > workaround (".apply didn't pick the best available period"). Is
>> > > this ok?
>> >
>> > hmm, I didn't consider this when writing the checks for PWM_DEBUG.
>> > According to the currently checked rules the expected configuration is
>> > to pick the 250Hz mode and use cycle = 0x7f.
>> 
>> Not to use 0x80, which is the max_duty_cycle? Like its 0x40 for the 
>> 500Hz
>> mode.
> 
> No, when I thought about a sane set of rules (and so checks for
> PWM_DEBUG) I didn't consider a PWM that can implement 100% but not for
> all otherwise available period lengths. I'm still amazed sometimes how
> different the capabilities of different implementations for something 
> so
> seemingly easy like a PWM are.
> 
>> > Hmm, I have to think about
>> > this. Maybe we should weaken the check to the cases with
>> > 0 < duty_cycle < period. Thierry, what do you think?
>> >
>> > Special casing 0% and 100% is annoying, but insisting 250Hz + 0x7f seems
>> > to be far from reality. (Is it?)
>> 
>> If you mean by insisting to clip at 0x7f, yeah thats bad IMHO, because
>> the user wants an all-high line, but in the end it would be a toggling
>> line. It wouldn't be that bad for anything in between 0% and 100% but
>> IMHO its bad for exactly 0% and 100%.
>> 
>> You could also ask the driver about known quirks, like special 0% and
>> 100% handling and exclude it from the tests accordingly.
> 
> Do you care enough to propose a patch? You're in the situation to test
> it.

Ok. I'll come up with something outside of this patch series.

>> > > > > +	ret = regmap_write(priv->regmap, priv->offset + PWM_CTRL, ctrl);
>> > > > > +	if (ret)
>> > > > > +		return ret;
>> > > > > +
>> > > > > +	return regmap_write(priv->regmap, priv->offset + PWM_CYCLE,
>> > > > > (u8)cycle);
>> > > >
>> > > > I assume this can result in broken output? Consider the hardware runs
>> > > > with mode = 1 & cycle = 0x23 and you want to go to mode = 0 & cycle =
>> > > > 0x42: Can this result in a period that has mode = 0 & cycle = 0x23?
>> > >
>> > > Isn't that always the case if a write may fail and there are more than
>> > > one register to configure?
>> >
>> > Depending on hardware capabilities you might not be able to prevent
>> > this yes. Unfortunately this is quite common.
>> >
>> > But there are hardware implementations that are not prone to such
>> > failures. (E.g. the registers written can be only shadow values that are
>> > latched into hardware only when the last value is written.)
>> 
>> Maybe this could be improved in the future.
> 
> We should somewhere describe, what an ideal PWM can do.
> My wishlist (just as it comes to my mind, so no guarantee of
> completeness):
> 
>  - can do 0% duty cycle for all supported period lengths
>  - can do 100% duty cycle for all supported period lengths
>  - supports both polarities
>  - supports immediate change of configuration and after completion of
>    the currently running period
>  - atomic update (i.e. if you go from configuration A to configuration 
> B
>    the hardware guarantees to only emit periods of type A and then type
>    B. (Depending on the item above, the last A period might be cut 
> off.)

We actually discussed this, because the implementation would be easier. 
But
if the change takes place immediately you might end up with a longer 
duty
cycle. Assume the PWM runs at 80% duty cycle and starts with the 
on-period.
If you now change that to 50% you might end up with one successive duty
cycle of "130%". Eg. the 80% of the old and right after that you switch 
to
the new 50% and then you'd have a high output which corresponds to a 
130%
cycle. I don't know if that is acceptable for all applications.

>  - emits an irq when configuration changes

Why would you need the interrupt?

> 
>> > If you change only cycle but not mode, does the hardware complete the
>> > currently running period?
>> 
>> No it does not.
> 
> Please document this as a Limitation.

I've discussed this internally, for now its a limitation. In the worst
case you'd do one 100% duty cycle. Maybe we can fix the hardware. I
acknowledge that this is a severe limitation, esp. if you use the PWM
for controlling stuff (for now its only LCD backlight.. so thats ok).

>> > What about disable()?
>> 
>> Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
>> fix that (in hardware), not much we can do in the driver here. We are
>> _very_ constraint in size, therefore all that little edge cases fall 
>> off
>> the table.
> 
> You're saying that on disable the hardware emits a constant high level
> for one cycle? I hope not ...

Mh, I was mistaken, disabling the PWM will turn it off immediately, but
one 100% duty cycle may happen if you change from a higher to a lower
duty cycle setting. See above.

> I never programmed a CPLD to emulate a hardware PWM, but I wonder if
> these are really edge cases that increase the size of the binary?!

At the moment there is only one 8bit register which stores the value
which is used for matching. If you want to change that setting after
a whole cycle, you'd use another 8bit register to cache the new value.
So this would at least needs 8 additional flip-flops. This doesn't
sound much, but we are already near 100% usage of the CPLD. So its
hard to convince people why this is really necessary.

-michael

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-14 21:09               ` Michael Walle
@ 2020-07-15 16:36                 ` Uwe Kleine-König
  2020-07-15 17:45                   ` Michael Walle
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-07-15 16:36 UTC (permalink / raw)
  To: Michael Walle
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman


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

Hello Michael,

On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote:
> > My wishlist (just as it comes to my mind, so no guarantee of
> > completeness):
> > 
> >  - can do 0% duty cycle for all supported period lengths
> >  - can do 100% duty cycle for all supported period lengths
> >  - supports both polarities
> >  - supports immediate change of configuration and after completion of
> >    the currently running period
> >  - atomic update (i.e. if you go from configuration A to configuration B
> >    the hardware guarantees to only emit periods of type A and then type
> >    B. (Depending on the item above, the last A period might be cut off.)
> 
> We actually discussed this, because the implementation would be easier. But
> if the change takes place immediately you might end up with a longer duty
> cycle. Assume the PWM runs at 80% duty cycle and starts with the on-period.
> If you now change that to 50% you might end up with one successive duty
> cycle of "130%". Eg. the 80% of the old and right after that you switch to
> the new 50% and then you'd have a high output which corresponds to a 130%
> cycle. I don't know if that is acceptable for all applications.

I thought this is a "change takes place immediately" implementation?! So
these problems are actually real here. (And this not happening is exactly
my wish here. Is there a mis-understanding?)

> >  - emits an irq when configuration changes
> 
> Why would you need the interrupt?

To know that the new setting is active. Currently Thierry's ideal PWM
implementation blocks in pwm_apply_state() until the new setting is
active. So some signaling is nice. 

> > > > If you change only cycle but not mode, does the hardware complete the
> > > > currently running period?
> > > 
> > > No it does not.
> > 
> > Please document this as a Limitation.
> 
> I've discussed this internally, for now its a limitation. In the worst
> case you'd do one 100% duty cycle. Maybe we can fix the hardware. I
> acknowledge that this is a severe limitation, esp. if you use the PWM
> for controlling stuff (for now its only LCD backlight.. so thats ok).

That happens if you reduce the duty cycle from A to B and the counter is
already bigger than B but smaller than A, right? The fix would be to
compare for counter >= match instead of counter = match. (Which then
would result in a period with a duty cycle bigger than B but smaller
than A. Also not ideal, but probably better.)

> > > > What about disable()?
> > > 
> > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
> > > fix that (in hardware), not much we can do in the driver here. We are
> > > _very_ constraint in size, therefore all that little edge cases fall
> > > off
> > > the table.
> > 
> > You're saying that on disable the hardware emits a constant high level
> > for one cycle? I hope not ...
> 
> Mh, I was mistaken, disabling the PWM will turn it off immediately, but

And does turn off mean, the output gets inactive?
If so you might also disable the hardware if a 0% duty cycle is
configured assuming this saves some energy without modifying the
resulting wave form.

> one 100% duty cycle may happen if you change from a higher to a lower
> duty cycle setting. See above.
> 
> > I never programmed a CPLD to emulate a hardware PWM, but I wonder if
> > these are really edge cases that increase the size of the binary?!
> 
> At the moment there is only one 8bit register which stores the value
> which is used for matching. If you want to change that setting after
> a whole cycle, you'd use another 8bit register to cache the new value.
> So this would at least needs 8 additional flip-flops. This doesn't
> sound much, but we are already near 100% usage of the CPLD. So its
> hard to convince people why this is really necessary.

OK. (Maybe there is enough space to allow implementing 100% for mode 0?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-15 16:36                 ` Uwe Kleine-König
@ 2020-07-15 17:45                   ` Michael Walle
  2020-07-15 18:18                     ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-15 17:45 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

Hi Uwe,

Am 2020-07-15 18:36, schrieb Uwe Kleine-König:
> On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote:
>> > My wishlist (just as it comes to my mind, so no guarantee of
>> > completeness):
>> >
>> >  - can do 0% duty cycle for all supported period lengths
>> >  - can do 100% duty cycle for all supported period lengths
>> >  - supports both polarities
>> >  - supports immediate change of configuration and after completion of
>> >    the currently running period
>> >  - atomic update (i.e. if you go from configuration A to configuration B
>> >    the hardware guarantees to only emit periods of type A and then type
>> >    B. (Depending on the item above, the last A period might be cut off.)
>> 
>> We actually discussed this, because the implementation would be 
>> easier. But
>> if the change takes place immediately you might end up with a longer 
>> duty
>> cycle. Assume the PWM runs at 80% duty cycle and starts with the 
>> on-period.
>> If you now change that to 50% you might end up with one successive 
>> duty
>> cycle of "130%". Eg. the 80% of the old and right after that you 
>> switch to
>> the new 50% and then you'd have a high output which corresponds to a 
>> 130%
>> cycle. I don't know if that is acceptable for all applications.
> 
> I thought this is a "change takes place immediately" implementation?! 
> So
> these problems are actually real here. (And this not happening is 
> exactly
> my wish here. Is there a mis-understanding?)

I wasn't talking about the sl28cpld btw. What is the difference between
your proposed "change take place immediately" and "after the cycle".
I understand how the after the cycle should work. But how would the
immediate change work in your ideal PWM?

>> > > > If you change only cycle but not mode, does the hardware complete the
>> > > > currently running period?
>> > >
>> > > No it does not.
>> >
>> > Please document this as a Limitation.
>> 
>> I've discussed this internally, for now its a limitation. In the worst
>> case you'd do one 100% duty cycle. Maybe we can fix the hardware. I
>> acknowledge that this is a severe limitation, esp. if you use the PWM
>> for controlling stuff (for now its only LCD backlight.. so thats ok).
> 
> That happens if you reduce the duty cycle from A to B and the counter 
> is
> already bigger than B but smaller than A, right?
That is correct.

> The fix would be to
> compare for counter >= match instead of counter = match. (Which then
> would result in a period with a duty cycle bigger than B but smaller
> than A. Also not ideal, but probably better.)

This would actually work. I could imagine that comparing "less than" 
will
take up more space again. But it would be worth a try; see also below.

>> > > > What about disable()?
>> > >
>> > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
>> > > fix that (in hardware), not much we can do in the driver here. We are
>> > > _very_ constraint in size, therefore all that little edge cases fall
>> > > off
>> > > the table.
>> >
>> > You're saying that on disable the hardware emits a constant high level
>> > for one cycle? I hope not ...
>> 
>> Mh, I was mistaken, disabling the PWM will turn it off immediately, 
>> but
> 
> And does turn off mean, the output gets inactive?
> If so you might also disable the hardware if a 0% duty cycle is
> configured assuming this saves some energy without modifying the
> resulting wave form.

Disabling it has some side effects like switching to another function
for this multi function pin. So I'd rather keep it on ;)

>> one 100% duty cycle may happen if you change from a higher to a lower
>> duty cycle setting. See above.
>> 
>> > I never programmed a CPLD to emulate a hardware PWM, but I wonder if
>> > these are really edge cases that increase the size of the binary?!
>> 
>> At the moment there is only one 8bit register which stores the value
>> which is used for matching. If you want to change that setting after
>> a whole cycle, you'd use another 8bit register to cache the new value.
>> So this would at least needs 8 additional flip-flops. This doesn't
>> sound much, but we are already near 100% usage of the CPLD. So its
>> hard to convince people why this is really necessary.
> 
> OK. (Maybe there is enough space to allow implementing 100% for mode 
> 0?)

Little bit here a little bit there ;) TBH there are some more critical
bugs which would need to be fixed first. So this would need to be a
limitation for now.

-michael

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-15 17:45                   ` Michael Walle
@ 2020-07-15 18:18                     ` Uwe Kleine-König
  2020-07-15 20:41                       ` Michael Walle
  0 siblings, 1 reply; 39+ messages in thread
From: Uwe Kleine-König @ 2020-07-15 18:18 UTC (permalink / raw)
  To: Michael Walle
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman


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

On Wed, Jul 15, 2020 at 07:45:10PM +0200, Michael Walle wrote:
> Hi Uwe,
> 
> Am 2020-07-15 18:36, schrieb Uwe Kleine-König:
> > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote:
> > > > My wishlist (just as it comes to my mind, so no guarantee of
> > > > completeness):
> > > >
> > > >  - can do 0% duty cycle for all supported period lengths
> > > >  - can do 100% duty cycle for all supported period lengths
> > > >  - supports both polarities
> > > >  - supports immediate change of configuration and after completion of
> > > >    the currently running period
> > > >  - atomic update (i.e. if you go from configuration A to configuration B
> > > >    the hardware guarantees to only emit periods of type A and then type
> > > >    B. (Depending on the item above, the last A period might be cut off.)
> > > 
> > > We actually discussed this, because the implementation would be
> > > easier. But
> > > if the change takes place immediately you might end up with a longer
> > > duty
> > > cycle. Assume the PWM runs at 80% duty cycle and starts with the
> > > on-period.
> > > If you now change that to 50% you might end up with one successive
> > > duty
> > > cycle of "130%". Eg. the 80% of the old and right after that you
> > > switch to
> > > the new 50% and then you'd have a high output which corresponds to a
> > > 130%
> > > cycle. I don't know if that is acceptable for all applications.
> > 
> > I thought this is a "change takes place immediately" implementation?! So
> > these problems are actually real here. (And this not happening is
> > exactly
> > my wish here. Is there a mis-understanding?)
> 
> I wasn't talking about the sl28cpld btw. What is the difference between
> your proposed "change take place immediately" and "after the cycle".
> I understand how the after the cycle should work. But how would the
> immediate change work in your ideal PWM?

If the PWM is running at 1/3 duty cycle and reconfigured for 2/3, then
the two scenarios are (the * marks the moment where pwm_apply_state() is
called, ^ marks the start of a period):

immediately:

  __       __    _____    _____
 /  \_____/  \__/     \__/
 ^        ^     ^        ^
                *

after the cycle
  __       __       _____    _____
 /  \_____/  \_____/     \__/
 ^        ^        ^        ^
                *

and with my ideal PWM I can choose which of the two behaviours I want.

> > > > > > What about disable()?
> > > > >
> > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
> > > > > fix that (in hardware), not much we can do in the driver here. We are
> > > > > _very_ constraint in size, therefore all that little edge cases fall
> > > > > off
> > > > > the table.
> > > >
> > > > You're saying that on disable the hardware emits a constant high level
> > > > for one cycle? I hope not ...
> > > 
> > > Mh, I was mistaken, disabling the PWM will turn it off immediately,
> > > but
> > 
> > And does turn off mean, the output gets inactive?
> > If so you might also disable the hardware if a 0% duty cycle is
> > configured assuming this saves some energy without modifying the
> > resulting wave form.
> 
> Disabling it has some side effects like switching to another function
> for this multi function pin. So I'd rather keep it on ;)

So IMHO you should also keep it on when pwm_apply_state is called with
state.enabled = false to ensure a low output.

> > > one 100% duty cycle may happen if you change from a higher to a lower
> > > duty cycle setting. See above.
> > > 
> > > > I never programmed a CPLD to emulate a hardware PWM, but I wonder if
> > > > these are really edge cases that increase the size of the binary?!
> > > 
> > > At the moment there is only one 8bit register which stores the value
> > > which is used for matching. If you want to change that setting after
> > > a whole cycle, you'd use another 8bit register to cache the new value.
> > > So this would at least needs 8 additional flip-flops. This doesn't
> > > sound much, but we are already near 100% usage of the CPLD. So its
> > > hard to convince people why this is really necessary.
> > 
> > OK. (Maybe there is enough space to allow implementing 100% for mode 0?)
> 
> Little bit here a little bit there ;) TBH there are some more critical
> bugs which would need to be fixed first. So this would need to be a
> limitation for now.

Ok for me.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-15 18:18                     ` Uwe Kleine-König
@ 2020-07-15 20:41                       ` Michael Walle
  2020-07-16  6:10                         ` Uwe Kleine-König
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-15 20:41 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman

Hi Uwe,

Am 2020-07-15 20:18, schrieb Uwe Kleine-König:
> On Wed, Jul 15, 2020 at 07:45:10PM +0200, Michael Walle wrote:
>> 
>> Am 2020-07-15 18:36, schrieb Uwe Kleine-König:
>> > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote:
>> > > > My wishlist (just as it comes to my mind, so no guarantee of
>> > > > completeness):
>> > > >
>> > > >  - can do 0% duty cycle for all supported period lengths
>> > > >  - can do 100% duty cycle for all supported period lengths
>> > > >  - supports both polarities
>> > > >  - supports immediate change of configuration and after completion of
>> > > >    the currently running period
>> > > >  - atomic update (i.e. if you go from configuration A to configuration B
>> > > >    the hardware guarantees to only emit periods of type A and then type
>> > > >    B. (Depending on the item above, the last A period might be cut off.)
>> > >
>> > > We actually discussed this, because the implementation would be
>> > > easier. But
>> > > if the change takes place immediately you might end up with a longer
>> > > duty
>> > > cycle. Assume the PWM runs at 80% duty cycle and starts with the
>> > > on-period.
>> > > If you now change that to 50% you might end up with one successive
>> > > duty
>> > > cycle of "130%". Eg. the 80% of the old and right after that you
>> > > switch to
>> > > the new 50% and then you'd have a high output which corresponds to a
>> > > 130%
>> > > cycle. I don't know if that is acceptable for all applications.
>> >
>> > I thought this is a "change takes place immediately" implementation?! So
>> > these problems are actually real here. (And this not happening is
>> > exactly
>> > my wish here. Is there a mis-understanding?)
>> 
>> I wasn't talking about the sl28cpld btw. What is the difference 
>> between
>> your proposed "change take place immediately" and "after the cycle".
>> I understand how the after the cycle should work. But how would the
>> immediate change work in your ideal PWM?
> 
> If the PWM is running at 1/3 duty cycle and reconfigured for 2/3, then
> the two scenarios are (the * marks the moment where pwm_apply_state() 
> is
> called, ^ marks the start of a period):
> 
> immediately:
> 
>   __       __    _____    _____
>  /  \_____/  \__/     \__/
>  ^        ^     ^        ^
>                 *

Ok lets assume 2/3 and change it to 1/3:

    ____     ______      __
   /    \___/      \____/  \____
   ^        ^   ^       ^
                *
This will then have a longer on period than any of the settings.

> and with my ideal PWM I can choose which of the two behaviours I want.

Ahh, that I've missed.

>> > > > > > What about disable()?
>> > > > >
>> > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
>> > > > > fix that (in hardware), not much we can do in the driver here. We are
>> > > > > _very_ constraint in size, therefore all that little edge cases fall
>> > > > > off
>> > > > > the table.
>> > > >
>> > > > You're saying that on disable the hardware emits a constant high level
>> > > > for one cycle? I hope not ...
>> > >
>> > > Mh, I was mistaken, disabling the PWM will turn it off immediately,
>> > > but
>> >
>> > And does turn off mean, the output gets inactive?
>> > If so you might also disable the hardware if a 0% duty cycle is
>> > configured assuming this saves some energy without modifying the
>> > resulting wave form.
>> 
>> Disabling it has some side effects like switching to another function
>> for this multi function pin. So I'd rather keep it on ;)
> 
> So IMHO you should also keep it on when pwm_apply_state is called with
> state.enabled = false to ensure a low output.

That won't work either, because that is how you would turn on that multi
function. Ie. it is GPIO (default input) as long as the PWM is not 
enabled,
otherwise its PWM.

-michael

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

* Re: [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller
  2020-07-15 20:41                       ` Michael Walle
@ 2020-07-16  6:10                         ` Uwe Kleine-König
  0 siblings, 0 replies; 39+ messages in thread
From: Uwe Kleine-König @ 2020-07-16  6:10 UTC (permalink / raw)
  To: Michael Walle
  Cc: Thierry Reding, 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, Wim Van Sebroeck, Shawn Guo, Li Yang,
	Thomas Gleixner, Jason Cooper, Marc Zyngier, Mark Brown,
	Greg Kroah-Hartman


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

Hello Michael,

On Wed, Jul 15, 2020 at 10:41:25PM +0200, Michael Walle wrote:
> Am 2020-07-15 20:18, schrieb Uwe Kleine-König:
> > On Wed, Jul 15, 2020 at 07:45:10PM +0200, Michael Walle wrote:
> > > 
> > > Am 2020-07-15 18:36, schrieb Uwe Kleine-König:
> > > > On Tue, Jul 14, 2020 at 11:09:28PM +0200, Michael Walle wrote:
> > > > > > My wishlist (just as it comes to my mind, so no guarantee of
> > > > > > completeness):
> > > > > >
> > > > > >  - can do 0% duty cycle for all supported period lengths
> > > > > >  - can do 100% duty cycle for all supported period lengths
> > > > > >  - supports both polarities
> > > > > >  - supports immediate change of configuration and after completion of
> > > > > >    the currently running period
> > > > > >  - atomic update (i.e. if you go from configuration A to configuration B
> > > > > >    the hardware guarantees to only emit periods of type A and then type
> > > > > >    B. (Depending on the item above, the last A period might be cut off.)
> > > > >
> > > > > We actually discussed this, because the implementation would be
> > > > > easier. But
> > > > > if the change takes place immediately you might end up with a longer
> > > > > duty
> > > > > cycle. Assume the PWM runs at 80% duty cycle and starts with the
> > > > > on-period.
> > > > > If you now change that to 50% you might end up with one successive
> > > > > duty
> > > > > cycle of "130%". Eg. the 80% of the old and right after that you
> > > > > switch to
> > > > > the new 50% and then you'd have a high output which corresponds to a
> > > > > 130%
> > > > > cycle. I don't know if that is acceptable for all applications.
> > > >
> > > > I thought this is a "change takes place immediately" implementation?! So
> > > > these problems are actually real here. (And this not happening is
> > > > exactly
> > > > my wish here. Is there a mis-understanding?)
> > > 
> > > I wasn't talking about the sl28cpld btw. What is the difference
> > > between
> > > your proposed "change take place immediately" and "after the cycle".
> > > I understand how the after the cycle should work. But how would the
> > > immediate change work in your ideal PWM?
> > 
> > If the PWM is running at 1/3 duty cycle and reconfigured for 2/3, then
> > the two scenarios are (the * marks the moment where pwm_apply_state() is
> > called, ^ marks the start of a period):
> > 
> > immediately:
> > 
> >   __       __    _____    _____
> >  /  \_____/  \__/     \__/
> >  ^        ^     ^        ^
> >                 *
> 
> Ok lets assume 2/3 and change it to 1/3:
> 
>    ____     ______      __
>   /    \___/      \____/  \____
>   ^        ^   ^       ^
>                *
> This will then have a longer on period than any of the settings.

I think we agree here. With an immediate change to the new setting both
too long and too short signals can heppen. How bad this is depends on
the use. The consumers currently in the kernel probably don't care too
much.

> > > > > > > > What about disable()?
> > > > > > >
> > > > > > > Mhh well, it would do one 100% cycle.. mhh ;) Lets see if there we can
> > > > > > > fix that (in hardware), not much we can do in the driver here. We are
> > > > > > > _very_ constraint in size, therefore all that little edge cases fall
> > > > > > > off
> > > > > > > the table.
> > > > > >
> > > > > > You're saying that on disable the hardware emits a constant high level
> > > > > > for one cycle? I hope not ...
> > > > >
> > > > > Mh, I was mistaken, disabling the PWM will turn it off immediately,
> > > > > but
> > > >
> > > > And does turn off mean, the output gets inactive?
> > > > If so you might also disable the hardware if a 0% duty cycle is
> > > > configured assuming this saves some energy without modifying the
> > > > resulting wave form.
> > > 
> > > Disabling it has some side effects like switching to another function
> > > for this multi function pin. So I'd rather keep it on ;)
> > 
> > So IMHO you should also keep it on when pwm_apply_state is called with
> > state.enabled = false to ensure a low output.
> 
> That won't work either, because that is how you would turn on that multi
> function. Ie. it is GPIO (default input) as long as the PWM is not enabled,
> otherwise its PWM.

I think you misunderstood what I wrote. The intended behaviour for a
disabled PWM (as in: pwm_apply_state() was called with state.enabled =
false) is that the output is a constant low (assuming a normal
polarity). If disabling your hardware results in something else, don't
disable the hardware. That's another item in the Limitations paragraph.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

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

* Re: [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller
  2020-07-06 17:53 ` [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
       [not found]   ` <20200706175353.16404-9-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-17  0:25   ` kernel test robot
  1 sibling, 0 replies; 39+ messages in thread
From: kernel test robot @ 2020-07-17  0:25 UTC (permalink / raw)
  To: Michael Walle, linux-gpio, devicetree, linux-kernel, linux-hwmon,
	linux-pwm, linux-watchdog, linux-arm-kernel
  Cc: kbuild-all, clang-built-linux, Linus Walleij,
	Bartosz Golaszewski, Rob Herring


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

Hi Michael,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ljones-mfd/for-mfd-next]
[also build test WARNING on shawnguo/for-next v5.8-rc5]
[cannot apply to gpio/for-next hwmon/hwmon-next next-20200716]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Michael-Walle/Add-support-for-Kontron-sl28cpld/20200707-020034
base:   https://git.kernel.org/pub/scm/linux/kernel/git/lee/mfd.git for-mfd-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project ed6b578040a85977026c93bf4188f996148f3218)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpio/gpio-sl28cpld.c:121:29: warning: implicit conversion from 'unsigned long' to 'unsigned int' changes value from 18446744073709551615 to 4294967295 [-Wconstant-conversion]
                   config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
                                           ~ ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/gpio/regmap.h:12:44: note: expanded from macro 'GPIO_REGMAP_ADDR'
   #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
                                              ^~~~~~~~~~~~~~~~~~~~~
   include/linux/gpio/regmap.h:11:32: note: expanded from macro 'GPIO_REGMAP_ADDR_ZERO'
   #define GPIO_REGMAP_ADDR_ZERO ((unsigned long)(-1))
                                  ^~~~~~~~~~~~~~~~~~~
   1 warning generated.

vim +121 drivers/gpio/gpio-sl28cpld.c

    88	
    89	static int sl28cpld_gpio_probe(struct platform_device *pdev)
    90	{
    91		struct gpio_regmap_config config = {0};
    92		enum sl28cpld_gpio_type type;
    93		struct regmap *regmap;
    94		u32 base;
    95		int ret;
    96	
    97		if (!pdev->dev.parent)
    98			return -ENODEV;
    99	
   100		type = (uintptr_t)device_get_match_data(&pdev->dev);
   101		if (!type)
   102			return -ENODEV;
   103	
   104		ret = device_property_read_u32(&pdev->dev, "reg", &base);
   105		if (ret)
   106			return -EINVAL;
   107	
   108		regmap = dev_get_regmap(pdev->dev.parent, NULL);
   109		if (!regmap)
   110			return -ENODEV;
   111	
   112		config.regmap = regmap;
   113		config.parent = &pdev->dev;
   114		config.ngpio = 8;
   115	
   116		switch (type) {
   117		case SL28CPLD_GPIO:
   118			config.reg_dat_base = base + GPIO_REG_IN;
   119			config.reg_set_base = base + GPIO_REG_OUT;
   120			/* reg_dir_out_base might be zero */
 > 121			config.reg_dir_out_base = GPIO_REGMAP_ADDR(base + GPIO_REG_DIR);
   122	
   123			/* This type supports interrupts */
   124			ret = sl28cpld_gpio_irq_init(pdev, base, &config);
   125			if (ret)
   126				return ret;
   127			break;
   128		case SL28CPLD_GPO:
   129			config.reg_set_base = base + GPO_REG_OUT;
   130			break;
   131		case SL28CPLD_GPI:
   132			config.reg_dat_base = base + GPI_REG_IN;
   133			break;
   134		default:
   135			dev_err(&pdev->dev, "unknown type %d\n", type);
   136			return -ENODEV;
   137		}
   138	
   139		return PTR_ERR_OR_ZERO(devm_gpio_regmap_register(&pdev->dev, &config));
   140	}
   141	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 75399 bytes --]

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

* Re: [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support
  2020-07-06 17:53   ` [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
@ 2020-07-17  8:36     ` Pavel Machek
  2020-07-17 21:54       ` Michael Walle
  0 siblings, 1 reply; 39+ messages in thread
From: Pavel Machek @ 2020-07-17  8:36 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,
	Mark Brown

Hi!

> 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".

> +		user_yellow {
> +			label = "s1914:yellow:user";
> +			gpios = <&sl28cpld_gpio0 0 0>;
> +		};
> +
> +		user_green {
> +			label = "s1914:green:user";
> +			gpios = <&sl28cpld_gpio1 3 0>;
> +		};

This is not suitable label for such LEDs... there's zero chance userland will
know what to do with these.

Do they have some kind of "usual" function?

Best regards,
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

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

* Re: [PATCH v5 02/13] mfd: add simple regmap based I2C driver
       [not found]     ` <20200706175353.16404-3-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-17  9:04       ` Lee Jones
  2020-07-19 22:25         ` Michael Walle
  0 siblings, 1 reply; 39+ messages in thread
From: Lee Jones @ 2020-07-17  9:04 UTC (permalink / raw)
  To: Michael Walle
  Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown

On Mon, 06 Jul 2020, Michael Walle wrote:

> There are I2C devices which contain several different functions but
> doesn't require any special access functions. For these kind of drivers
> an I2C regmap should be enough.
> 
> Create an I2C driver which creates an I2C regmap and enumerates its
> children. If a device wants to use this as its MFD core driver, it has
> to add an individual compatible string. It may provide its own regmap
> configuration.
> 
> Subdevices can use dev_get_regmap() on the parent to get their regmap
> instance.
> 
> Signed-off-by: Michael Walle <michael-QKn5cuLxLXY@public.gmane.org>
> ---
> Changes since v4:
>  - new patch. Lee, please bear with me. I didn't want to delay the
>    new version (where a lot of remarks on the other patches were
>    addressed) even more, just because we haven't figured out how
>    to deal with the MFD part. So for now, I've included this one.
> 
>  drivers/mfd/Kconfig          |  9 +++++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/simple-mfd-i2c.c | 50 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+)
>  create mode 100644 drivers/mfd/simple-mfd-i2c.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df0837ab41..f1536a710aca 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called si476x-core.
>  
> +config MFD_SIMPLE_MFD_I2C
> +	tristate "Simple regmap based I2C devices"
> +	depends on I2C
> +	select MFD_CORE
> +	select REGMAP_I2C
> +	help
> +	  This is a consolidated driver for all MFD devices which are
> +	  basically just a regmap bus driver.
> +
>  config MFD_SM501
>  	tristate "Silicon Motion SM501"
>  	depends on HAS_DMA
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index a60e5f835283..78d24a3e7c9e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -264,3 +264,4 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
>  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>  
>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
> +obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
> diff --git a/drivers/mfd/simple-mfd-i2c.c b/drivers/mfd/simple-mfd-i2c.c
> new file mode 100644
> index 000000000000..1fdca89964b1
> --- /dev/null
> +++ b/drivers/mfd/simple-mfd-i2c.c
> @@ -0,0 +1,49 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/regmap.h>

I'm pretty sure you do not require all of these headers.

> +struct simple_mfd_i2c_config {
> +	const struct regmap_config *regmap_config;
> +};

No need for this yet I feel.

Let's keep it as simple as possible.

> +static const struct regmap_config simple_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +};
> +
> +static int simple_mfd_i2c_probe(struct i2c_client *i2c)
> +{
> +	const struct regmap_config *regmap_config = &simple_regmap_config;
> +	const struct simple_mfd_i2c_config *config;
> +	struct regmap *regmap;
> +
> +	config = device_get_match_data(&i2c->dev);

Have this return regmap_config.

> +	if (config && config->regmap_config)
> +		regmap_config = config->regmap_config;
> +
> +	regmap = devm_regmap_init_i2c(i2c, regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	return devm_of_platform_populate(&i2c->dev);
> +}
> +
> +static const struct of_device_id simple_mfd_i2c_of_match[] = {
> +	{}
> +};
> +
> +static struct i2c_driver simple_mfd_i2c_driver = {
> +	.probe_new = simple_mfd_i2c_probe,
> +	.driver = {
> +		.name = "simple-mfd-i2c",
> +		.of_match_table = simple_mfd_i2c_of_match,
> +	},
> +};
> +builtin_i2c_driver(simple_mfd_i2c_driver);

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

* Re: [PATCH v5 02/13] mfd: add simple regmap based I2C driver
  2020-07-06 17:53   ` [PATCH v5 02/13] mfd: add simple regmap based I2C driver Michael Walle
       [not found]     ` <20200706175353.16404-3-michael-QKn5cuLxLXY@public.gmane.org>
@ 2020-07-17  9:06     ` Lee Jones
  2020-07-19 22:31       ` Michael Walle
  1 sibling, 1 reply; 39+ messages in thread
From: Lee Jones @ 2020-07-17  9:06 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,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown

On Mon, 06 Jul 2020, Michael Walle wrote:

> There are I2C devices which contain several different functions but
> doesn't require any special access functions. For these kind of drivers
> an I2C regmap should be enough.
> 
> Create an I2C driver which creates an I2C regmap and enumerates its
> children. If a device wants to use this as its MFD core driver, it has
> to add an individual compatible string. It may provide its own regmap
> configuration.
> 
> Subdevices can use dev_get_regmap() on the parent to get their regmap
> instance.
> 
> Signed-off-by: Michael Walle <michael@walle.cc>
> ---
> Changes since v4:
>  - new patch. Lee, please bear with me. I didn't want to delay the
>    new version (where a lot of remarks on the other patches were
>    addressed) even more, just because we haven't figured out how
>    to deal with the MFD part. So for now, I've included this one.
> 
>  drivers/mfd/Kconfig          |  9 +++++++
>  drivers/mfd/Makefile         |  1 +
>  drivers/mfd/simple-mfd-i2c.c | 50 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 60 insertions(+)
>  create mode 100644 drivers/mfd/simple-mfd-i2c.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 33df0837ab41..f1536a710aca 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called si476x-core.
>  
> +config MFD_SIMPLE_MFD_I2C
> +	tristate "Simple regmap based I2C devices"

Doesn't look like tristate to me.

Haven't you made this builtin only?

> +	depends on I2C
> +	select MFD_CORE

Why?

> +	select REGMAP_I2C
> +	help
> +	  This is a consolidated driver for all MFD devices which are
> +	  basically just a regmap bus driver.

Please expand on this.  I think it deserves greater explanation.

>  config MFD_SM501
>  	tristate "Silicon Motion SM501"
>  	depends on HAS_DMA

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

* Re: [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support
  2020-07-17  8:36     ` Pavel Machek
@ 2020-07-17 21:54       ` Michael Walle
  0 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-17 21:54 UTC (permalink / raw)
  To: Pavel Machek
  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,
	Mark Brown

Hi Pavel,

Am 2020-07-17 10:36, schrieb Pavel Machek:
> Hi!
> 
>> 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".
> 
>> +		user_yellow {
>> +			label = "s1914:yellow:user";
>> +			gpios = <&sl28cpld_gpio0 0 0>;
>> +		};
>> +
>> +		user_green {
>> +			label = "s1914:green:user";
>> +			gpios = <&sl28cpld_gpio1 3 0>;
>> +		};
> 
> This is not suitable label for such LEDs... there's zero chance 
> userland will
> know what to do with these.
> 
> Do they have some kind of "usual" function?

Unfortunately, they do not. I guess the green one could be something 
like
"application ready" and the yellow one could be an indication that some
fault occurred.

Do you have any suggestions?

-michael

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

* Re: [PATCH v5 02/13] mfd: add simple regmap based I2C driver
  2020-07-17  9:04       ` Lee Jones
@ 2020-07-19 22:25         ` Michael Walle
  0 siblings, 0 replies; 39+ messages in thread
From: Michael Walle @ 2020-07-19 22:25 UTC (permalink / raw)
  To: Lee Jones
  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,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown

Am 2020-07-17 11:04, schrieb Lee Jones:
> On Mon, 06 Jul 2020, Michael Walle wrote:
> 
>> There are I2C devices which contain several different functions but
>> doesn't require any special access functions. For these kind of 
>> drivers
>> an I2C regmap should be enough.
>> 
>> Create an I2C driver which creates an I2C regmap and enumerates its
>> children. If a device wants to use this as its MFD core driver, it has
>> to add an individual compatible string. It may provide its own regmap
>> configuration.
>> 
>> Subdevices can use dev_get_regmap() on the parent to get their regmap
>> instance.
>> 
>> Signed-off-by: Michael Walle <michael@walle.cc>
>> ---
>> Changes since v4:
>>  - new patch. Lee, please bear with me. I didn't want to delay the
>>    new version (where a lot of remarks on the other patches were
>>    addressed) even more, just because we haven't figured out how
>>    to deal with the MFD part. So for now, I've included this one.
>> 
>>  drivers/mfd/Kconfig          |  9 +++++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/simple-mfd-i2c.c | 50 
>> ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 60 insertions(+)
>>  create mode 100644 drivers/mfd/simple-mfd-i2c.c
>> 
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 33df0837ab41..f1536a710aca 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called si476x-core.
>> 
>> +config MFD_SIMPLE_MFD_I2C
>> +	tristate "Simple regmap based I2C devices"
>> +	depends on I2C
>> +	select MFD_CORE
>> +	select REGMAP_I2C
>> +	help
>> +	  This is a consolidated driver for all MFD devices which are
>> +	  basically just a regmap bus driver.
>> +
>>  config MFD_SM501
>>  	tristate "Silicon Motion SM501"
>>  	depends on HAS_DMA
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index a60e5f835283..78d24a3e7c9e 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -264,3 +264,4 @@ obj-$(CONFIG_MFD_STMFX) 	+= stmfx.o
>>  obj-$(CONFIG_MFD_KHADAS_MCU) 	+= khadas-mcu.o
>> 
>>  obj-$(CONFIG_SGI_MFD_IOC3)	+= ioc3.o
>> +obj-$(CONFIG_MFD_SIMPLE_MFD_I2C)	+= simple-mfd-i2c.o
>> diff --git a/drivers/mfd/simple-mfd-i2c.c 
>> b/drivers/mfd/simple-mfd-i2c.c
>> new file mode 100644
>> index 000000000000..1fdca89964b1
>> --- /dev/null
>> +++ b/drivers/mfd/simple-mfd-i2c.c
>> @@ -0,0 +1,49 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +#include <linux/i2c.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/regmap.h>
> 
> I'm pretty sure you do not require all of these headers.

Shot, I'll clean that up.

>> +struct simple_mfd_i2c_config {
>> +	const struct regmap_config *regmap_config;
>> +};
> 
> No need for this yet I feel.
> 
> Let's keep it as simple as possible.

ok

>> +static const struct regmap_config simple_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +};
>> +
>> +static int simple_mfd_i2c_probe(struct i2c_client *i2c)
>> +{
>> +	const struct regmap_config *regmap_config = &simple_regmap_config;
>> +	const struct simple_mfd_i2c_config *config;
>> +	struct regmap *regmap;
>> +
>> +	config = device_get_match_data(&i2c->dev);
> 
> Have this return regmap_config.

ok

>> +	if (config && config->regmap_config)
>> +		regmap_config = config->regmap_config;
>> +
>> +	regmap = devm_regmap_init_i2c(i2c, regmap_config);
>> +	if (IS_ERR(regmap))
>> +		return PTR_ERR(regmap);
>> +
>> +	return devm_of_platform_populate(&i2c->dev);
>> +}
>> +
>> +static const struct of_device_id simple_mfd_i2c_of_match[] = {
>> +	{}
>> +};
>> +
>> +static struct i2c_driver simple_mfd_i2c_driver = {
>> +	.probe_new = simple_mfd_i2c_probe,
>> +	.driver = {
>> +		.name = "simple-mfd-i2c",
>> +		.of_match_table = simple_mfd_i2c_of_match,
>> +	},
>> +};
>> +builtin_i2c_driver(simple_mfd_i2c_driver);

-- 
-michael

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

* Re: [PATCH v5 02/13] mfd: add simple regmap based I2C driver
  2020-07-17  9:06     ` Lee Jones
@ 2020-07-19 22:31       ` Michael Walle
  2020-07-20  7:43         ` Lee Jones
  0 siblings, 1 reply; 39+ messages in thread
From: Michael Walle @ 2020-07-19 22:31 UTC (permalink / raw)
  To: Lee Jones
  Cc: linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-pwm-u79uwXL29TY76Z2rM5mHXA,
	linux-watchdog-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Linus Walleij,
	Bartosz Golaszewski, Rob Herring, Jean Delvare, Guenter Roeck,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown

Am 2020-07-17 11:06, schrieb Lee Jones:
> On Mon, 06 Jul 2020, Michael Walle wrote:
> 
>> There are I2C devices which contain several different functions but
>> doesn't require any special access functions. For these kind of 
>> drivers
>> an I2C regmap should be enough.
>> 
>> Create an I2C driver which creates an I2C regmap and enumerates its
>> children. If a device wants to use this as its MFD core driver, it has
>> to add an individual compatible string. It may provide its own regmap
>> configuration.
>> 
>> Subdevices can use dev_get_regmap() on the parent to get their regmap
>> instance.
>> 
>> Signed-off-by: Michael Walle <michael-QKn5cuLxLXY@public.gmane.org>
>> ---
>> Changes since v4:
>>  - new patch. Lee, please bear with me. I didn't want to delay the
>>    new version (where a lot of remarks on the other patches were
>>    addressed) even more, just because we haven't figured out how
>>    to deal with the MFD part. So for now, I've included this one.
>> 
>>  drivers/mfd/Kconfig          |  9 +++++++
>>  drivers/mfd/Makefile         |  1 +
>>  drivers/mfd/simple-mfd-i2c.c | 50 
>> ++++++++++++++++++++++++++++++++++++
>>  3 files changed, 60 insertions(+)
>>  create mode 100644 drivers/mfd/simple-mfd-i2c.c
>> 
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 33df0837ab41..f1536a710aca 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called si476x-core.
>> 
>> +config MFD_SIMPLE_MFD_I2C
>> +	tristate "Simple regmap based I2C devices"
> 
> Doesn't look like tristate to me.
> 
> Haven't you made this builtin only?

Mh yeah, I forgot to change it to module in the driver. I don't
know whats better though, have it tristate or just offer a boolean
option because it should be small anyway. What do you think?
My interrupt driver will force it to boolean anyway.

> 
>> +	depends on I2C
>> +	select MFD_CORE
> 
> Why?

leftover :( I'll remove it.


>> +	select REGMAP_I2C
>> +	help
>> +	  This is a consolidated driver for all MFD devices which are
>> +	  basically just a regmap bus driver.
> 
> Please expand on this.  I think it deserves greater explanation.

ok.

> 
>>  config MFD_SM501
>>  	tristate "Silicon Motion SM501"
>>  	depends on HAS_DMA

-- 
-michael

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

* Re: [PATCH v5 02/13] mfd: add simple regmap based I2C driver
  2020-07-19 22:31       ` Michael Walle
@ 2020-07-20  7:43         ` Lee Jones
  0 siblings, 0 replies; 39+ messages in thread
From: Lee Jones @ 2020-07-20  7:43 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,
	Thierry Reding, Uwe Kleine-König, Wim Van Sebroeck,
	Shawn Guo, Li Yang, Thomas Gleixner, Jason Cooper, Marc Zyngier,
	Mark Brown

On Mon, 20 Jul 2020, Michael Walle wrote:

> Am 2020-07-17 11:06, schrieb Lee Jones:
> > On Mon, 06 Jul 2020, Michael Walle wrote:
> > 
> > > There are I2C devices which contain several different functions but
> > > doesn't require any special access functions. For these kind of
> > > drivers
> > > an I2C regmap should be enough.
> > > 
> > > Create an I2C driver which creates an I2C regmap and enumerates its
> > > children. If a device wants to use this as its MFD core driver, it has
> > > to add an individual compatible string. It may provide its own regmap
> > > configuration.
> > > 
> > > Subdevices can use dev_get_regmap() on the parent to get their regmap
> > > instance.
> > > 
> > > Signed-off-by: Michael Walle <michael@walle.cc>
> > > ---
> > > Changes since v4:
> > >  - new patch. Lee, please bear with me. I didn't want to delay the
> > >    new version (where a lot of remarks on the other patches were
> > >    addressed) even more, just because we haven't figured out how
> > >    to deal with the MFD part. So for now, I've included this one.
> > > 
> > >  drivers/mfd/Kconfig          |  9 +++++++
> > >  drivers/mfd/Makefile         |  1 +
> > >  drivers/mfd/simple-mfd-i2c.c | 50
> > > ++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 60 insertions(+)
> > >  create mode 100644 drivers/mfd/simple-mfd-i2c.c
> > > 
> > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > index 33df0837ab41..f1536a710aca 100644
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -1162,6 +1162,15 @@ config MFD_SI476X_CORE
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called si476x-core.
> > > 
> > > +config MFD_SIMPLE_MFD_I2C
> > > +	tristate "Simple regmap based I2C devices"
> > 
> > Doesn't look like tristate to me.
> > 
> > Haven't you made this builtin only?
> 
> Mh yeah, I forgot to change it to module in the driver. I don't
> know whats better though, have it tristate or just offer a boolean
> option because it should be small anyway. What do you think?
> My interrupt driver will force it to boolean anyway.

Better to give consumers the choice I think.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

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

end of thread, back to index

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-06 17:53 [PATCH v5 00/13] Add support for Kontron sl28cpld Michael Walle
2020-07-06 17:53 ` [PATCH v5 01/13] regmap-irq: use fwnode instead of device node in add_irq_chip() Michael Walle
2020-07-08 10:22   ` Mark Brown
     [not found] ` <20200706175353.16404-1-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-06 17:53   ` [PATCH v5 02/13] mfd: add simple regmap based I2C driver Michael Walle
     [not found]     ` <20200706175353.16404-3-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-17  9:04       ` Lee Jones
2020-07-19 22:25         ` Michael Walle
2020-07-17  9:06     ` Lee Jones
2020-07-19 22:31       ` Michael Walle
2020-07-20  7:43         ` Lee Jones
2020-07-06 17:53   ` [PATCH v5 06/13] watchdog: add support for sl28cpld watchdog Michael Walle
2020-07-06 17:53   ` [PATCH v5 12/13] arm64: dts: freescale: sl28: enable LED support Michael Walle
2020-07-17  8:36     ` Pavel Machek
2020-07-17 21:54       ` Michael Walle
2020-07-06 17:53 ` [PATCH v5 03/13] dt-bindings: mfd: Add bindings for sl28cpld Michael Walle
2020-07-13 16:04   ` Rob Herring
2020-07-06 17:53 ` [PATCH v5 04/13] mfd: simple-mfd-i2c: add sl28cpld support Michael Walle
2020-07-06 17:53 ` [PATCH v5 05/13] irqchip: add sl28cpld interrupt controller support Michael Walle
2020-07-08  9:37   ` Marc Zyngier
2020-07-06 17:53 ` [PATCH v5 07/13] pwm: add support for sl28cpld PWM controller Michael Walle
2020-07-09  8:50   ` Uwe Kleine-König
     [not found]     ` <20200709085006.b54ype3p4yu64upl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
2020-07-11 17:28       ` Michael Walle
2020-07-13  8:47         ` Uwe Kleine-König
2020-07-14 11:31           ` Michael Walle
2020-07-14 16:08             ` Uwe Kleine-König
2020-07-14 21:09               ` Michael Walle
2020-07-15 16:36                 ` Uwe Kleine-König
2020-07-15 17:45                   ` Michael Walle
2020-07-15 18:18                     ` Uwe Kleine-König
2020-07-15 20:41                       ` Michael Walle
2020-07-16  6:10                         ` Uwe Kleine-König
2020-07-06 17:53 ` [PATCH v5 08/13] gpio: add support for the sl28cpld GPIO controller Michael Walle
     [not found]   ` <20200706175353.16404-9-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-08  7:35     ` Linus Walleij
2020-07-17  0:25   ` kernel test robot
2020-07-06 17:53 ` [PATCH v5 09/13] hwmon: add support for the sl28cpld hardware monitoring controller Michael Walle
2020-07-06 17:53 ` [PATCH v5 10/13] arm64: dts: freescale: sl28: enable sl28cpld Michael Walle
2020-07-06 17:53 ` [PATCH v5 11/13] arm64: dts: freescale: sl28: map GPIOs to input events Michael Walle
2020-07-06 17:53 ` [PATCH v5 13/13] arm64: dts: freescale: sl28: enable fan support Michael Walle
     [not found]   ` <20200706175353.16404-14-michael-QKn5cuLxLXY@public.gmane.org>
2020-07-08  7:39     ` Linus Walleij
     [not found]       ` <CACRpkdaPO7CGNrxmjL5QH1cxP5wqku1oMtQaQgJfeKiKqiGAOg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2020-07-08  7:56         ` Michael Walle

Linux-PWM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pwm/0 linux-pwm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pwm linux-pwm/ https://lore.kernel.org/linux-pwm \
		linux-pwm@vger.kernel.org
	public-inbox-index linux-pwm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pwm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git