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
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, ®map_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, ®map_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 ®map_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
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
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
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
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
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
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, ®); + + 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, ®); + 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
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
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
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
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
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
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
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
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
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
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...
[-- 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 --]
On Mon, 6 Jul 2020 19:53:40 +0200, Michael Walle wrote: > 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. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git for-next Thanks! [1/1] regmap-irq: use fwnode instead of device node in add_irq_chip() commit: 5cc2013bfeee756a1ee6da9bfbe42e52b4695035 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
[-- 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, ®); > + > + 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, ®); > + 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 --]
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, ®); >> + >> + 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, ®); >> + 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
[-- 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, ®); > > > + > > > + 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, ®); > > > + 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 --]
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>
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, ®); >> > > + >> > > + 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, ®); >> > > + 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
[-- 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 --]
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
[-- 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 --]
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
[-- 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 --]
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
[-- 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 --]
[-- 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 --]
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
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
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
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
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
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
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