| Message ID | 20250318-mdb-max7360-support-v5-0-fb20baf97da0@bootlin.com |
|---|---|
| Headers | show |
| Series | Add support for MAX7360 | expand |
Hi, > GPIO controller often have support for IRQ: allow to easily allocate > both gpio-regmap and regmap-irq in one operation. > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > --- > drivers/gpio/gpio-regmap.c | 23 +++++++++++++++++++++-- > include/linux/gpio/regmap.h | 15 +++++++++++++++ > 2 files changed, 36 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpio/gpio-regmap.c b/drivers/gpio/gpio-regmap.c > index 05f8781b5204..61d5f48b445d 100644 > --- a/drivers/gpio/gpio-regmap.c > +++ b/drivers/gpio/gpio-regmap.c > @@ -203,6 +203,7 @@ EXPORT_SYMBOL_GPL(gpio_regmap_get_drvdata); > */ > struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config) > { > + struct irq_domain *irq_domain; > struct gpio_regmap *gpio; > struct gpio_chip *chip; > int ret; > @@ -280,8 +281,26 @@ struct gpio_regmap *gpio_regmap_register(const struct gpio_regmap_config *config > if (ret < 0) > goto err_free_gpio; > > - if (config->irq_domain) { > - ret = gpiochip_irqchip_add_domain(chip, config->irq_domain); > + irq_domain = config->irq_domain; > +#ifdef CONFIG_GPIOLIB_IRQCHIP Why do we need this ifdef? > + if (config->regmap_irq_chip) { > + struct regmap_irq_chip_data *irq_chip_data; > + > + ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent), > + config->regmap, config->regmap_irq_irqno, > + config->regmap_irq_flags, 0, > + config->regmap_irq_chip, &irq_chip_data); > + if (ret) > + goto err_free_gpio; > + > + irq_domain = regmap_irq_get_domain(irq_chip_data); > + if (config->regmap_irq_chip_data) > + *config->regmap_irq_chip_data = irq_chip_data; I'm not a fan of misusing the config to return any data. Can we have a normal gpio_regmap_get_...()? Usually, the config is on the stack of the caller, what if you need to get irq_chip_data afterwards? Then your caller has to save it somewhere. Also, what is the advantage of this? Your caller doesn't have to call devm_regmap_add_irq_chip_fwnode(), but on the flip side you have to cram all its parameters in the gpio_regmap config. I'd like to keep that small and simple (but still extensible!). IMHO just setting the irq_domain is enough to achieve that. -michael > + } > +#endif > + > + if (irq_domain) { > + ret = gpiochip_irqchip_add_domain(chip, irq_domain); > if (ret) > goto err_remove_gpiochip; > } > diff --git a/include/linux/gpio/regmap.h b/include/linux/gpio/regmap.h > index a9f7b7faf57b..55df2527b982 100644 > --- a/include/linux/gpio/regmap.h > +++ b/include/linux/gpio/regmap.h > @@ -40,6 +40,14 @@ struct regmap; > * @drvdata: (Optional) Pointer to driver specific data which is > * not used by gpio-remap but is provided "as is" to the > * driver callback(s). > + * @regmap_irq_chip: (Optional) Pointer on an regmap_irq_chip structure. If > + * set, a regmap-irq device will be created and the IRQ > + * domain will be set accordingly. > + * @regmap_irq_chip_data: (Optional) Pointer on an regmap_irq_chip_data > + * structure pointer. If set, it will be populated with a > + * pointer on allocated regmap_irq data. > + * @regmap_irq_irqno (Optional) The IRQ the device uses to signal interrupts. > + * @regmap_irq_flags (Optional) The IRQF_ flags to use for the interrupt. > * > * The ->reg_mask_xlate translates a given base address and GPIO offset to > * register and mask pair. The base address is one of the given register > @@ -78,6 +86,13 @@ struct gpio_regmap_config { > int ngpio_per_reg; > struct irq_domain *irq_domain; > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > + struct regmap_irq_chip *regmap_irq_chip; > + struct regmap_irq_chip_data **regmap_irq_chip_data; > + int regmap_irq_irqno; > + unsigned long regmap_irq_flags; > +#endif > + > int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base, > unsigned int offset, unsigned int *reg, > unsigned int *mask);
Hi Mathieu, thanks for your patch! On Tue, Mar 18, 2025 at 5:26 PM Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> wrote: > Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins > can be used either for GPIO, PWM or rotary encoder functionalities. > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> Overall it's a clean and simple pin control driver, so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to > 8 independent PWM outputs. ... > +#include <linux/bits.h> > +#include <linux/dev_printk.h> > +#include <linux/err.h> > +#include <linux/math64.h> > +#include <linux/mfd/max7360.h> > +#include <linux/minmax.h> > +#include <linux/mod_devicetable.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pwm.h> > +#include <linux/regmap.h> > +#include <linux/time.h> > +#include <linux/types.h> ... > +static void max7360_pwm_free(struct pwm_chip *chip, struct pwm_device *pwm) > +{ > + struct regmap *regmap; > + struct device *dev; > + > + regmap = pwmchip_get_drvdata(chip); > + dev = regmap_get_device(regmap); Huh?! > +} ... > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const struct pwm_waveform *wf, > + void *_wfhw) I would expect other way around, i.e. naming with leading underscore(s) to be private / local. Ditto for all similar cases. ... > +static int max7360_pwm_write_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + const void *_wfhw) > +{ > + const struct max7360_pwm_waveform *wfhw = _wfhw; > + struct regmap *regmap; > + unsigned int val; > + int ret; > + > + regmap = pwmchip_get_drvdata(chip); > + val = (wfhw->enabled) ? BIT(pwm->hwpwm) : 0; Redundant parentheses. > + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCTRL, BIT(pwm->hwpwm), val); > + if (ret) > + return ret; > + > + if (wfhw->duty_steps) > + return regmap_write(regmap, MAX7360_REG_PWM(pwm->hwpwm), wfhw->duty_steps); > + > + return 0; > +} ... > +static int max7360_pwm_read_waveform(struct pwm_chip *chip, > + struct pwm_device *pwm, > + void *_wfhw) > +{ > + struct max7360_pwm_waveform *wfhw = _wfhw; > + struct regmap *regmap; > + unsigned int val; > + int ret; > + > + regmap = pwmchip_get_drvdata(chip); > + > + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val); > + if (ret) > + return ret; > + > + if (val & BIT(pwm->hwpwm)) { > + wfhw->enabled = true; Also can be (but up to you) wfhw->enabled = val & BIT(pwm->hwpwm); if (wfhw->enabled) { And also see below. Perhaps it is not a good suggestion after all. > + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val); > + wfhw->duty_steps = val; Set to a garbage in case of error, why? > + } else { > + wfhw->enabled = false; > + wfhw->duty_steps = 0; > + } > + > + return ret; > +} ... > +static int max7360_pwm_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct pwm_chip *chip; > + struct regmap *regmap; > + int ret; > + > + if (!dev->parent) > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); Why? Code most likely will fail on the regmap retrieval. Just do that first. > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); This is quite worrying. The devm_ to parent makes a lot of assumptions that may not be realised. If you really need this, it has to have a very good comment explaining why and object lifetimes. > + if (IS_ERR(chip)) > + return PTR_ERR(chip); > + chip->ops = &max7360_pwm_ops; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n"); > + > + pwmchip_set_drvdata(chip, regmap); > + > + ret = devm_pwmchip_add(dev, chip); > + if (ret) > + return dev_err_probe(dev, ret, "failed to add PWM chip\n"); > + > + return 0; > +}
On Tue, Mar 18, 2025 at 05:26:19PM +0100, Mathieu Dubois-Briand wrote: > Add driver for Maxim Integrated MAX7360 pinctrl on the PORT pins. Pins > can be used either for GPIO, PWM or rotary encoder functionalities. ... > + help > + Say Y here to enable Pin control support for Maxim MAX7360 keypad s/Pin/pin/ > + controller. > + This keypad controller has 8 GPIO pins that work as GPIO as well as "...that may work as GPIO, or PWM, or..." > + PWM or rotary encoder alternate modes. ... + array_size.h + dev_printk.h + device/devres.h // currently only in Linux Next + err.h > +#include <linux/init.h> > +#include <linux/mfd/max7360.h> > +#include <linux/module.h> > +#include <linux/platform_device.h> > +#include <linux/pinctrl/pinctrl.h> > +#include <linux/pinctrl/pinconf-generic.h> > +#include <linux/pinctrl/pinmux.h> We usually move this group of inclusions... > +#include <linux/regmap.h> > +#include <linux/slab.h> ...to somewhere here. > +#include "core.h" > +#include "pinmux.h" ... > +static const struct pingroup max7360_groups[] = { > + PINCTRL_PINGROUP("PORT0", port0_pins, ARRAY_SIZE(port0_pins)), > + PINCTRL_PINGROUP("PORT1", port1_pins, ARRAY_SIZE(port1_pins)), > + PINCTRL_PINGROUP("PORT2", port2_pins, ARRAY_SIZE(port2_pins)), > + PINCTRL_PINGROUP("PORT3", port3_pins, ARRAY_SIZE(port3_pins)), > + PINCTRL_PINGROUP("PORT4", port4_pins, ARRAY_SIZE(port4_pins)), > + PINCTRL_PINGROUP("PORT5", port5_pins, ARRAY_SIZE(port5_pins)), > + PINCTRL_PINGROUP("PORT6", port6_pins, ARRAY_SIZE(port6_pins)), > + PINCTRL_PINGROUP("PORT7", port7_pins, ARRAY_SIZE(port7_pins)), > + PINCTRL_PINGROUP("ROTARY", rotary_pins, ARRAY_SIZE(rotary_pins)) Leave trailing comma. Helps in the future in case of expansion. > +}; ... > +static const char * const simple_groups[] = {"PORT0", "PORT1", "PORT2", "PORT3", > + "PORT4", "PORT5", "PORT6", "PORT7"}; It's better to read when split as static const char * const simple_groups[] = { "PORT0", "PORT1", "PORT2", "PORT3", "PORT4", "PORT5", "PORT6", "PORT7", }; (also note the trailing comma). … > +static const char * const rotary_groups[] = {"ROTARY"}; Add spaces inside {}. ... > +#define MAX7360_PINCTRL_FN_ROTARY 2 > +static const struct pinfunction max7360_functions[] = { > + PINCTRL_PINFUNCTION("gpio", simple_groups, ARRAY_SIZE(simple_groups)), > + PINCTRL_PINFUNCTION("pwm", simple_groups, ARRAY_SIZE(simple_groups)), > + [MAX7360_PINCTRL_FN_ROTARY] = PINCTRL_PINFUNCTION("rotary", rotary_groups, > + ARRAY_SIZE(rotary_groups)), Please make them all look the same, if indexed, than add indices to all. > +}; ... > +static int max7360_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, > + unsigned int group) > +{ > + struct regmap *regmap; > + int ret = 0; Variable is not needed, just return directly. > + int val; > + > + /* > + * GPIO and PWM functions are the same: we only need to handle the > + * rotary encoder function, on pins 6 and 7. > + */ > + if (max7360_groups[group].pins[0] >= 6) { > + if (selector == MAX7360_PINCTRL_FN_ROTARY) > + val = MAX7360_GPIO_CFG_RTR_EN; > + else > + val = 0; > + > + regmap = dev_get_regmap(pctldev->dev, NULL); > + ret = regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, MAX7360_GPIO_CFG_RTR_EN, val); > + } > + > + return ret; > +} ... > +static int max7360_pinctrl_probe(struct platform_device *pdev) > +{ With struct device *dev = &pdev->dev; the below will look better. > + struct regmap *regmap; > + struct pinctrl_desc *pd; > + struct max7360_pinctrl *chip; > + > + chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!regmap) > + dev_err_probe(&pdev->dev, -ENODEV, "Could not get parent regmap\n"); Make it first check, in such a case you don't even need to allocate memory for peanuts. > + pd = &chip->pinctrl_desc; > + > + pd->pctlops = &max7360_pinctrl_ops; > + pd->pmxops = &max7360_pmxops; > + pd->name = dev_name(&pdev->dev); > + pd->pins = max7360_pins; > + pd->npins = MAX7360_MAX_GPIO; > + pd->owner = THIS_MODULE; > + > + chip->pctldev = devm_pinctrl_register(pdev->dev.parent, pd, chip); > + if (IS_ERR(chip->pctldev)) > + return dev_err_probe(&pdev->dev, PTR_ERR(chip->pctldev), > + "can't register controller\n"); > + > + return 0; > +}
On Tue, Mar 18, 2025 at 05:26:25PM +0100, Mathieu Dubois-Briand wrote: > Add driver for Maxim Integrated MAX7360 keypad controller, providing > support for up to 64 keys, with a matrix of 8 columns and 8 rows. ... > + help > + If you say yes here you get support for the keypad controller on the > + Maxim MAX7360 I/O Expander. > + > + To compile this driver as a module, choose M here: the > + module will be called max7360_keypad. One paragraph is wrapped way too late or too early, can you make them approx. the same in terms of a line width? ... + bitfield.h + bitops.h + dev_printk.h + device/devres.h + err.h > +#include <linux/init.h> > +#include <linux/input.h> > +#include <linux/input/matrix_keypad.h> > +#include <linux/interrupt.h> > +#include <linux/mfd/max7360.h> + mod_devicetable.h > +#include <linux/module.h> > +#include <linux/property.h> > +#include <linux/platform_device.h> > +#include <linux/pm_wakeirq.h> > +#include <linux/regmap.h> > +#include <linux/slab.h> IS it used? I think it's device/devres.h that covers it. ... > +static int max7360_keypad_open(struct input_dev *pdev) > +{ > + struct max7360_keypad *max7360_keypad = input_get_drvdata(pdev); > + int ret; > + > + /* > + * Somebody is using the device: get out of sleep. > + */ > + ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, > + MAX7360_CFG_SLEEP, MAX7360_CFG_SLEEP); > + if (ret) { > + dev_err(&max7360_keypad->input->dev, > + "Failed to write max7360 configuration\n"); > + return ret; > + } > + > + return 0; Just return ret; ? > +} ... > + /* > + * Nobody is using the device anymore: go to sleep. > + */ The comment message can take only a line. > + ret = regmap_write_bits(max7360_keypad->regmap, MAX7360_REG_CONFIG, MAX7360_CFG_SLEEP, 0); > + if (ret) > + dev_err(&max7360_keypad->input->dev, > + "Failed to write max7360 configuration\n"); > +} ... > +static int max7360_keypad_parse_dt(struct platform_device *pdev, s/dt/fw > + struct max7360_keypad *max7360_keypad, > + bool *autorepeat) > +{ struct device *dev = &pdev>dev; but why not supply struct device to begin with? How is the platform part used here? > + int ret; > + > + ret = matrix_keypad_parse_properties(pdev->dev.parent, &max7360_keypad->rows, > + &max7360_keypad->cols); > + if (ret) > + return ret; > + > + if (!max7360_keypad->rows || !max7360_keypad->cols || > + max7360_keypad->rows > MAX7360_MAX_KEY_ROWS || > + max7360_keypad->cols > MAX7360_MAX_KEY_COLS) { See also below comment. > + dev_err(&pdev->dev, > + "Invalid number of columns or rows (%ux%u)\n", > + max7360_keypad->cols, max7360_keypad->rows); > + return -EINVAL; > + } > + > + *autorepeat = device_property_read_bool(pdev->dev.parent, "autorepeat"); > + > + max7360_keypad->debounce_ms = MAX7360_DEBOUNCE_MIN; > + ret = device_property_read_u32(pdev->dev.parent, "keypad-debounce-delay-ms", > + &max7360_keypad->debounce_ms); > + if (ret == -EINVAL) { > + dev_info(&pdev->dev, "Using default keypad-debounce-delay-ms: %u\n", > + max7360_keypad->debounce_ms); > + } else if (ret < 0) { > + dev_err(&pdev->dev, > + "Failed to read keypad-debounce-delay-ms property\n"); > + return ret; > + } else if (max7360_keypad->debounce_ms < MAX7360_DEBOUNCE_MIN || Redundant 'else'. > + max7360_keypad->debounce_ms > MAX7360_DEBOUNCE_MAX) { Maybe in_range()? But up to you, it takes start:len and not start:end. > + dev_err(&pdev->dev, > + "Invalid keypad-debounce-delay-ms: %u, should be between %u and %u.\n", > + max7360_keypad->debounce_ms, MAX7360_DEBOUNCE_MIN, MAX7360_DEBOUNCE_MAX); > + return -EINVAL; > + } > + > + return 0; > +} ... > +static int max7360_keypad_probe(struct platform_device *pdev) > +{ > + struct max7360_keypad *max7360_keypad; struct device *dev = &pdev>dev; > + struct input_dev *input; > + bool autorepeat; > + int ret; > + int irq; > + if (!pdev->dev.parent) > + return dev_err_probe(&pdev->dev, -ENODEV, "No parent device\n"); Just do like in the rest, i.e. local variable for regmap and its validness will be the one that indicates the wrong enumeration path. > + irq = platform_get_irq_byname(to_platform_device(pdev->dev.parent), "intk"); > + if (irq < 0) > + return irq; > + > + max7360_keypad = devm_kzalloc(&pdev->dev, sizeof(*max7360_keypad), GFP_KERNEL); > + if (!max7360_keypad) > + return -ENOMEM; > + > + max7360_keypad->regmap = dev_get_regmap(pdev->dev.parent, NULL); > + if (!max7360_keypad->regmap) > + return dev_err_probe(&pdev->dev, -ENODEV, "Could not get parent regmap\n"); > + > + ret = max7360_keypad_parse_dt(pdev, max7360_keypad, &autorepeat); > + if (ret) > + return ret; > + > + input = devm_input_allocate_device(pdev->dev.parent); > + if (!input) > + return -ENOMEM; > + > + max7360_keypad->input = input; > + > + input->id.bustype = BUS_I2C; > + input->name = pdev->name; > + input->open = max7360_keypad_open; > + input->close = max7360_keypad_close; > + > + ret = matrix_keypad_build_keymap(NULL, NULL, MAX7360_MAX_KEY_ROWS, MAX7360_MAX_KEY_COLS, > + max7360_keypad->keycodes, input); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, > + "Failed to build keymap\n"); One line. > + > + input_set_capability(input, EV_MSC, MSC_SCAN); > + if (autorepeat) > + __set_bit(EV_REP, input->evbit); > + > + input_set_drvdata(input, max7360_keypad); > + > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max7360_keypad_irq, > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, What's wrong with the interrupt flags provided by firmware description? > + "max7360-keypad", max7360_keypad); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "Failed to register interrupt\n"); > + > + ret = input_register_device(input); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "Could not register input device\n"); > + platform_set_drvdata(pdev, max7360_keypad); Is it used? > + ret = max7360_keypad_hw_init(max7360_keypad); > + if (ret) > + return dev_err_probe(&pdev->dev, ret, "Failed to initialize max7360 keypad\n"); > + > + device_init_wakeup(&pdev->dev, true); > + ret = dev_pm_set_wake_irq(&pdev->dev, irq); > + if (ret) > + dev_warn(&pdev->dev, "Failed to set up wakeup irq: %d\n", ret); > + > + return 0; > +}
On Tue, Mar 18, 2025 at 05:26:16PM +0100, Mathieu Dubois-Briand wrote: > This series implements a set of drivers allowing to support the Maxim > Integrated MAX7360 device. > > The MAX7360 is an I2C key-switch and led controller, with following > functionalities: > - Keypad controller for a key matrix of up to 8 rows and 8 columns. > - Rotary encoder support, for a single rotary encoder. > - Up to 8 PWM outputs. > - Up to 8 GPIOs with support for interrupts and 6 GPOs. > > Chipset pins are shared between all functionalities, so all cannot be > used at the same time. Thanks! Skeleton more or less looks at it's stable phase, there are tons of the style and small amendments that may be made. I would expect one or two at most new versions of this series.
On Tue Mar 18, 2025 at 6:39 PM CET, Rob Herring wrote: > On Tue, Mar 18, 2025 at 05:26:17PM +0100, Mathieu Dubois-Briand wrote: > > Add device tree bindings for Maxim Integrated MAX7360 device with > > support for keypad, rotary, gpios and pwm functionalities. > > > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > > --- > > .../bindings/gpio/maxim,max7360-gpio.yaml | 83 ++++++++++ > > .../devicetree/bindings/mfd/maxim,max7360.yaml | 170 +++++++++++++++++++++ > > 2 files changed, 253 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml > > new file mode 100644 > > index 000000000000..21d603d9504c > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/gpio/maxim,max7360-gpio.yaml > > @@ -0,0 +1,83 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) ... > > + > > + keypad-debounce-delay-ms: > > The existing debounce-delay-ms or poll-interval properties don't work > for you? > The issue is this node also describes the rotary encoder (just below), so I feel using only debounce-delay-ms is a bit misleading. > > + description: Keypad debounce delay in ms > > + minimum: 9 > > + maximum: 40 > > + default: 9 > > + > > + rotary-debounce-delay-ms: > > + description: Rotary encoder debounce delay in ms > > + minimum: 0 > > + maximum: 15 > > + default: 0 > > + > > + linux,axis: > > + description: The input subsystem axis to map to this rotary encoder. > > You should have a $ref to rotary-encoder.yaml too. None of the other > properties in it are needed? Makes sense, thanks! And no, I believe this is the only property we need. > > > + > > + "#pwm-cells": > > + const: 3 > > + > > + gpio: > > + $ref: /schemas/gpio/maxim,max7360-gpio.yaml# > > + description: > > + PORT0 to PORT7 general purpose input/output pins configuration. > > + > > + gpo: > > + $ref: /schemas/gpio/maxim,max7360-gpio.yaml# > > + description: > > > + COL2 to COL7 general purpose output pins configuration. > > + Allows to use unused keypad columns as outputs. > > Are these paragraphs? If so, add a blank line between paragraphs. If > not, re-wrap the lines. > OK > > + The MAX7360 has 8 column lines and 6 of them can be used as GPOs. GPIOs > > + numbers used for this gpio-controller node do correspond to the column > > + numbers: values 0 and 1 are never valid, values from 2 to 7 might be valid > > + depending on the value of the keypad,num-column property. > > + > > +patternProperties: > > + '-pins$': > > + type: object > > + description: > > + Pinctrl node's client devices use subnodes for desired pin configuration. > > + Client device subnodes use below standard properties. > > + $ref: /schemas/pinctrl/pincfg-node.yaml > > + > > + properties: > > + pins: > > + description: > > + List of gpio pins affected by the properties specified in this > > + subnode. > > + items: > > + pattern: '^PORT[0-7]|ROTARY$' > > Don't you need ()?: > > ^(PORT[0-7]|ROTARY)$' > Yes! Thanks for your review. Mathieu
On Tue Mar 18, 2025 at 5:52 PM CET, Andy Shevchenko wrote: > On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote: > > GPIO controller often have support for IRQ: allow to easily allocate > > both gpio-regmap and regmap-irq in one operation. > > ... > > > - if (config->irq_domain) { > > - ret = gpiochip_irqchip_add_domain(chip, config->irq_domain); > > > + irq_domain = config->irq_domain; > > Better to move it into #else, so we avoid double assignment (see below). > OK > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > + if (config->regmap_irq_chip) { > > + struct regmap_irq_chip_data *irq_chip_data; > > + > > + ret = devm_regmap_add_irq_chip_fwnode(config->parent, dev_fwnode(config->parent), > > + config->regmap, config->regmap_irq_irqno, > > + config->regmap_irq_flags, 0, > > + config->regmap_irq_chip, &irq_chip_data); > > + if (ret) > > + goto err_free_gpio; > > + > > + irq_domain = regmap_irq_get_domain(irq_chip_data); > > + if (config->regmap_irq_chip_data) > > + *config->regmap_irq_chip_data = irq_chip_data; > > Hmm... I was under impression that we don't need this to be returned. > Do we have any user of it right now? If not, no need to export until > it is needed. > Right, I will remove it. > > + } > > } else > > > +#endif > > irq_domain = config->irq_domain; > > > + > > + if (irq_domain) { > > + ret = gpiochip_irqchip_add_domain(chip, irq_domain); > > if (ret) > > goto err_remove_gpiochip; > > } > > ... > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > + struct regmap_irq_chip *regmap_irq_chip; > > + struct regmap_irq_chip_data **regmap_irq_chip_data; > > But why double pointer? > I believe this has to be a double pointer, as it is going to be assigned a pointer value: data buffer is allocated inside of devm_regmap_add_irq_chip_fwnode(). But as you said, it's better to remove it and add it later if there is an use case. > > + int regmap_irq_irqno; > > + unsigned long regmap_irq_flags; > > +#endif Thanks for your review.
On Thu, Mar 20, 2025 at 08:55:57AM +0100, Mathieu Dubois-Briand wrote: > On Tue Mar 18, 2025 at 5:52 PM CET, Andy Shevchenko wrote: > > On Tue, Mar 18, 2025 at 05:26:22PM +0100, Mathieu Dubois-Briand wrote: ... > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > > + struct regmap_irq_chip *regmap_irq_chip; > > > + struct regmap_irq_chip_data **regmap_irq_chip_data; > > > > But why double pointer? > > I believe this has to be a double pointer, as it is going to be assigned > a pointer value: data buffer is allocated inside of > devm_regmap_add_irq_chip_fwnode(). Yes, but it doesn't need to be a double one in the data structrure, right? > But as you said, it's better to remove it and add it later if there is > an use case. This would be even better for now, thanks!
On Thu, Mar 20, 2025 at 09:35:15AM +0100, Mathieu Dubois-Briand wrote: > On Wed Mar 19, 2025 at 8:15 AM CET, Michael Walle wrote: ... > > Also, what is the advantage of this? Your caller doesn't have to > > call devm_regmap_add_irq_chip_fwnode(), but on the flip side you > > have to cram all its parameters in the gpio_regmap config. I'd like > > to keep that small and simple (but still extensible!). IMHO just > > setting the irq_domain is enough to achieve that. > > This was a request from Andy on my previous series. The benefit is deduplication of a lot of code. You may consider it the same as GPIO library does with IRQ chip. This is just the same on a different level. Besides the driver in this series, I would think of other GPIO drivers that are not (yet) converted to regmap (partially because of this is being absent) or existing drivers, if any, that may utilise it.
On Wed Mar 19, 2025 at 12:18 PM CET, Andy Shevchenko wrote: > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > From: Kamel Bouhara <kamel.bouhara@bootlin.com> > > > > Add driver for Maxim Integrated MAX7360 PWM controller, supporting up to > > 8 independent PWM outputs. > > ... > > > > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + const struct pwm_waveform *wf, > > + void *_wfhw) > > I would expect other way around, i.e. naming with leading underscore(s) to be > private / local. Ditto for all similar cases. I get the point, but the 2 existing drivers based on pwm_ops structure name it that way: drivers/pwm/pwm-axi-pwmgen.c and drivers/pwm/pwm-stm32.c. Also, the parameter is mostly unusable as-is, as it is a void*, so I believe it also makes sense to have no underscore for the correctly casted one, that we will be using in the function body (wfhw). > > ... > > > +static int max7360_pwm_read_waveform(struct pwm_chip *chip, > > + struct pwm_device *pwm, > > + void *_wfhw) > > +{ > > + struct max7360_pwm_waveform *wfhw = _wfhw; > > + struct regmap *regmap; > > + unsigned int val; > > + int ret; > > + > > + regmap = pwmchip_get_drvdata(chip); > > + > > + ret = regmap_read(regmap, MAX7360_REG_GPIOCTRL, &val); > > + if (ret) > > + return ret; > > + > > + if (val & BIT(pwm->hwpwm)) { > > + wfhw->enabled = true; > > Also can be (but up to you) > > wfhw->enabled = val & BIT(pwm->hwpwm); > if (wfhw->enabled) { > > And also see below. Perhaps it is not a good suggestion after all. > > > + ret = regmap_read(regmap, MAX7360_REG_PWM(pwm->hwpwm), &val); > > + wfhw->duty_steps = val; > > Set to a garbage in case of error, why? > Ok, I'm fixing the whole block of code. > > + } else { > > + wfhw->enabled = false; > > + wfhw->duty_steps = 0; > > + } > > + > > + return ret; > > +} > > ... > > > +static int max7360_pwm_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct pwm_chip *chip; > > + struct regmap *regmap; > > + int ret; > > + > > + if (!dev->parent) > > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); > > Why? Code most likely will fail on the regmap retrieval. Just do that first. > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > not be realised. If you really need this, it has to have a very good comment > explaining why and object lifetimes. > Thanks, I'm fixing this in this driver and similar code in keypad, rotary and pinctrl. More details in the child mail. Thanks for your review! Mathieu
On Tue, Mar 25, 2025 at 03:29:18PM +0100, Mathieu Dubois-Briand wrote: > On Wed Mar 19, 2025 at 12:18 PM CET, Andy Shevchenko wrote: > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: ... > > > +static int max7360_pwm_round_waveform_tohw(struct pwm_chip *chip, > > > + struct pwm_device *pwm, > > > + const struct pwm_waveform *wf, > > > + void *_wfhw) > > > > I would expect other way around, i.e. naming with leading underscore(s) to be > > private / local. Ditto for all similar cases. > > I get the point, but the 2 existing drivers based on pwm_ops structure > name it that way: drivers/pwm/pwm-axi-pwmgen.c and > drivers/pwm/pwm-stm32.c. > > Also, the parameter is mostly unusable as-is, as it is a void*, so I > believe it also makes sense to have no underscore for the correctly > casted one, that we will be using in the function body (wfhw). It's all up to PWM maintainers, but I find this style a bit weird, sorry. I only saw this in the macros, where it's kinda okay. In functions it's something that needs an additional thinking and understanding the semantics of the underscore. ... > > > +static int max7360_pwm_probe(struct platform_device *pdev) > > > +{ > > > + struct device *dev = &pdev->dev; > > > + struct pwm_chip *chip; > > > + struct regmap *regmap; > > > + int ret; > > > + > > > + if (!dev->parent) > > > + return dev_err_probe(dev, -ENODEV, "no parent device\n"); > > > > Why? Code most likely will fail on the regmap retrieval. Just do that first. > > > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > not be realised. If you really need this, it has to have a very good comment > > explaining why and object lifetimes. > > Thanks, I'm fixing this in this driver and similar code in keypad, > rotary and pinctrl. More details in the child mail. Sure, thanks!
On Tue, Mar 25, 2025 at 03:57:01PM +0100, Mathieu Dubois-Briand wrote: > On Wed Mar 19, 2025 at 1:02 PM CET, Andy Shevchenko wrote: > > On Tue, Mar 18, 2025 at 05:26:25PM +0100, Mathieu Dubois-Briand wrote: ... > > > + ret = devm_request_threaded_irq(&pdev->dev, irq, NULL, max7360_keypad_irq, > > > + IRQF_TRIGGER_LOW | IRQF_ONESHOT, > > > > What's wrong with the interrupt flags provided by firmware description? > > So same question as for the GPIO driver: IRQF_TRIGGER_LOW from the > firmware, but IRQF_ONESHOT from the driver? Or should everything come > from the firmware? The same answer, yes, the Linux stuff (e.g., ONESHOT) should be given explicitly here.
On Tue Mar 25, 2025 at 8:50 AM CET, Michael Walle wrote: > > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > > > > > Why do we need this ifdef? > > > > > > > Hum yes, on second thought we probably need to depend on > > CONFIG_REGMAP_IRQ here. > > But then, you'd also require the regmap_irq support for chips that > don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be > missing a stub version. > Sorry, maybe my previous message was not clear, when I said "depend", what I meant is having an "#ifdef CONFIG_REGMAP_IRQ" here in place of "#ifdef CONFIG_GPIOLIB_IRQCHIP" If CONFIG_REGMAP_IRQ is enabled, drivers/base/regmap/regmap-irq.c is built, so we do have both devm_regmap_add_irq_chip_fwnode() and regmap_irq_get_domain(). So this code block should compile and link correctly. I did some build tests with and without CONFIG_GPIOLIB_IRQCHIP and I believe this is fine. Or am I missing something?
On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote: > On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote: > > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote: > > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote: > > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote: > > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > ... > > > > > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > > > > not be realised. If you really need this, it has to have a very good comment > > > > > explaining why and object lifetimes. > > > > > > > > Pretty sure this is broken. This results for example in the device link > > > > being created on the parent. So if the pwm devices goes away a consumer > > > > might not notice (at least in the usual way). I guess this was done to > > > > ensure that #pwm-cells is parsed from the right dt node? If so, that > > > > needs a different adaption. That will probably involve calling > > > > device_set_of_node_from_dev(). > > > > > > It's an MFD based driver, and MFD core cares about propagating fwnode by > > > default. I believe it should just work if we drop that '->parent' part. > > > > Are you sure about that? > > Yes and no. If your DT looks like (pseudo code as I don't know > DTS syntax by heart): > > device: { > parent-property = value; > child0: > ... > child1: > ... > } > > the parent-property value is automatically accessible via fwnode API, > but I don't know what will happen to the cases when each of the children > has its own compatible string. This might be your case, but again, > I'm not an expert in DT. > On my side: - Some MFD child do have a child node in the device tree, with an associated compatible value. No problem for these, they do get correct of_node/fwnode values pointing on the child device tree node. - Some MFD child do not have any node in the device tree, and for these, they have to use properties from the parent (MFD) device tree node. And here we do have some problems. > > On my side it does not work if I just drop the '->parent', this is why I > > ended whit this (bad) pattern. > > > Now it does work if I do call device_set_of_node_from_dev() manually, > > AFAICT, this is wrong API to be called in the children. Are you talking about > parent code? > I believe I cannot do it in the parent code, as I would need to do it after the call to devm_mfd_add_devices(), and so it might happen after the probe. I still tried to see how it behaved, and it looks like PWM core really did not expect to get an of_node assigned to the device after adding the PWM device. So either I can do something in MFD core or in sub devices probe(), or I need to come with a different way to do things. > > so it's definitely better. But I believe the MFD core is not propagating > > OF data, and I did not find where it would do that in the code. Yet it > > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we > > do something bad in our MFD driver? > > ...or MFD needs something to have... Dunno. I have something working with a very simple change in mfd-core.c, but I'm really not confident it won't break anything else. I wish I could get some insights from an MFD expert. @@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id, if (!pdev->dev.of_node) pr_warn("%s: Failed to locate of_node [id: %d]\n", cell->name, platform_id); + } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) { + device_set_of_node_from_dev(&pdev->dev, parent); }
On Wed, Mar 26, 2025 at 05:49:07PM +0200, Andy Shevchenko wrote: > On Wed, Mar 26, 2025 at 03:44:28PM +0100, Mathieu Dubois-Briand wrote: > > On Tue Mar 25, 2025 at 4:56 PM CET, Andy Shevchenko wrote: > > > On Tue, Mar 25, 2025 at 03:37:29PM +0100, Mathieu Dubois-Briand wrote: > > > > On Thu Mar 20, 2025 at 11:48 AM CET, Andy Shevchenko wrote: > > > > > On Thu, Mar 20, 2025 at 08:50:00AM +0100, Uwe Kleine-König wrote: > > > > > > On Wed, Mar 19, 2025 at 01:18:50PM +0200, Andy Shevchenko wrote: > > > > > > > On Tue, Mar 18, 2025 at 05:26:20PM +0100, mathieu.dubois-briand@bootlin.com wrote: > > ... > > > > > > > > > + chip = devm_pwmchip_alloc(dev->parent, MAX7360_NUM_PWMS, 0); > > > > > > > > > > > > > > This is quite worrying. The devm_ to parent makes a lot of assumptions that may > > > > > > > not be realised. If you really need this, it has to have a very good comment > > > > > > > explaining why and object lifetimes. > > > > > > > > > > > > Pretty sure this is broken. This results for example in the device link > > > > > > being created on the parent. So if the pwm devices goes away a consumer > > > > > > might not notice (at least in the usual way). I guess this was done to > > > > > > ensure that #pwm-cells is parsed from the right dt node? If so, that > > > > > > needs a different adaption. That will probably involve calling > > > > > > device_set_of_node_from_dev(). > > > > > > > > > > It's an MFD based driver, and MFD core cares about propagating fwnode by > > > > > default. I believe it should just work if we drop that '->parent' part. > > > > > > > > Are you sure about that? > > > > > > Yes and no. If your DT looks like (pseudo code as I don't know > > > DTS syntax by heart): > > > > > > device: { > > > parent-property = value; > > > child0: > > > ... > > > child1: > > > ... > > > } > > > > > > the parent-property value is automatically accessible via fwnode API, > > > but I don't know what will happen to the cases when each of the children > > > has its own compatible string. This might be your case, but again, > > > I'm not an expert in DT. > > > > > > > On my side: > > - Some MFD child do have a child node in the device tree, with an > > associated compatible value. No problem for these, they do get correct > > of_node/fwnode values pointing on the child device tree node. > > - Some MFD child do not have any node in the device tree, and for these, > > they have to use properties from the parent (MFD) device tree node. > > And here we do have some problems. > > > > > > On my side it does not work if I just drop the '->parent', this is why I > > > > ended whit this (bad) pattern. > > > > > > > Now it does work if I do call device_set_of_node_from_dev() manually, > > > > > > AFAICT, this is wrong API to be called in the children. Are you talking about > > > parent code? > > > > > > > I believe I cannot do it in the parent code, as I would need to do it > > after the call to devm_mfd_add_devices(), and so it might happen after > > the probe. I still tried to see how it behaved, and it looks like PWM > > core really did not expect to get an of_node assigned to the device > > after adding the PWM device. > > > > So either I can do something in MFD core or in sub devices probe(), or I > > need to come with a different way to do things. > > > > > > so it's definitely better. But I believe the MFD core is not propagating > > > > OF data, and I did not find where it would do that in the code. Yet it > > > > does something like this for ACPI in mfd_acpi_add_device(). Or maybe we > > > > do something bad in our MFD driver? > > > > > > ...or MFD needs something to have... Dunno. > > > > I have something working with a very simple change in mfd-core.c, but > > I'm really not confident it won't break anything else. I wish I could > > get some insights from an MFD expert. > > > > @@ -210,6 +210,8 @@ static int mfd_add_device(struct device *parent, int id, > > if (!pdev->dev.of_node) > > pr_warn("%s: Failed to locate of_node [id: %d]\n", > > cell->name, platform_id); > > + } else if (IS_ENABLED(CONFIG_OF) && parent->of_node) { > > + device_set_of_node_from_dev(&pdev->dev, parent); > > The use of this API is inappropriate here AFAICT. It drops the parent refcount > and on the second call to it you will have a warning from refcount library. device_set_of_node_from_dev() does: of_node_put(pdev->dev->of_node); pdev->dev->of_node = of_node_get(parent->of_node); pdev->dev->of_node_reused = true; Note that pdev isn't the platform device associated with the parent device but the just allocated one representing the subdevice so pdev->dev->of_node is NULL and the parent refcount isn't dropped. But I'm unsure if this is the right place to call it or if device_set_node() is indeed enough (also I wonder if device_set_of_node_from_dev() should care for fwnode). I'll keep that question for someone else. Best regards Uwe
On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote: > The use of this API is inappropriate here AFAICT. It drops the parent refcount > and on the second call to it you will have a warning from refcount library. > > It should be as simple as device_set_node(). > > > } > > With that, the conditional becomes > > } else if (is_of_node(fwnode)) { > device_set_node(&pdev->dev, fwnode); > } > > where fwnode is something like > > struct fwnode_handle *fwnode = dev_fwnode(parent); Hi, I tried to use device_set_node(), but then I got some other issue: as we now have several devices with the same firmware node, they all share the same properties. In particular, if we do use pinctrl- properties to apply some pinmmuxing, all devices will try to apply this pinmuxing and of course all but one will fail. And this makes me think again about the whole thing, maybe copying the fwnode or of_node from the parent is not the way to go. So today we rely on the parent node for four drivers: - keypad and rotary, just to ease a bit the parsing of some properties, such as the keymap with matrix_keypad_build_keymap(). I can easily do it another way. - PWM and pinctrl drivers, are a bit more complicated, as in both case the device tree node associated with the device is used internally. In one case to find the correct PWM device for PWM clients listed in the device tree, in the other case to find the pinctrl device when applying pinctrl described in the device tree. So maybe I have to find a better way for have this association. One way would be to modify the device tree bindings to add a PWM and a pinctrl node, with their own compatible, so they are associated to the corresponding device. But maybe there is a better way to do it.
On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote: > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote: > > The use of this API is inappropriate here AFAICT. It drops the parent refcount > > and on the second call to it you will have a warning from refcount library. > > > > It should be as simple as device_set_node(). > > > > > } > > > > With that, the conditional becomes > > > > } else if (is_of_node(fwnode)) { > > device_set_node(&pdev->dev, fwnode); > > } > > > > where fwnode is something like > > > > struct fwnode_handle *fwnode = dev_fwnode(parent); > > I tried to use device_set_node(), but then I got some other issue: as we > now have several devices with the same firmware node, they all share the > same properties. In particular, if we do use pinctrl- properties to > apply some pinmmuxing, all devices will try to apply this pinmuxing and > of course all but one will fail. > > And this makes me think again about the whole thing, maybe copying the > fwnode or of_node from the parent is not the way to go. > > So today we rely on the parent node for four drivers: > - keypad and rotary, just to ease a bit the parsing of some properties, > such as the keymap with matrix_keypad_build_keymap(). I can easily do > it another way. > - PWM and pinctrl drivers, are a bit more complicated, as in both case > the device tree node associated with the device is used internally. In > one case to find the correct PWM device for PWM clients listed in the > device tree, in the other case to find the pinctrl device when > applying pinctrl described in the device tree. > > So maybe I have to find a better way for have this association. One way > would be to modify the device tree bindings to add a PWM and a pinctrl > node, with their own compatible, so they are associated to the > corresponding device. But maybe there is a better way to do it. Okay, so the main question now, why do the device share their properties to begin with? It can be done via fwnode graph or similar APIs (in case it is _really_ needed).
On Thu Mar 27, 2025 at 6:50 PM CET, Andy Shevchenko wrote: > On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote: > > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote: > > > > The use of this API is inappropriate here AFAICT. It drops the parent refcount > > > and on the second call to it you will have a warning from refcount library. > > > > > > It should be as simple as device_set_node(). > > > > > > > } > > > > > > With that, the conditional becomes > > > > > > } else if (is_of_node(fwnode)) { > > > device_set_node(&pdev->dev, fwnode); > > > } > > > > > > where fwnode is something like > > > > > > struct fwnode_handle *fwnode = dev_fwnode(parent); > > > > I tried to use device_set_node(), but then I got some other issue: as we > > now have several devices with the same firmware node, they all share the > > same properties. In particular, if we do use pinctrl- properties to > > apply some pinmmuxing, all devices will try to apply this pinmuxing and > > of course all but one will fail. > > > > And this makes me think again about the whole thing, maybe copying the > > fwnode or of_node from the parent is not the way to go. > > > > So today we rely on the parent node for four drivers: > > - keypad and rotary, just to ease a bit the parsing of some properties, > > such as the keymap with matrix_keypad_build_keymap(). I can easily do > > it another way. > > - PWM and pinctrl drivers, are a bit more complicated, as in both case > > the device tree node associated with the device is used internally. In > > one case to find the correct PWM device for PWM clients listed in the > > device tree, in the other case to find the pinctrl device when > > applying pinctrl described in the device tree. > > > > So maybe I have to find a better way for have this association. One way > > would be to modify the device tree bindings to add a PWM and a pinctrl > > node, with their own compatible, so they are associated to the > > corresponding device. But maybe there is a better way to do it. > > Okay, so the main question now, why do the device share their properties > to begin with? It can be done via fwnode graph or similar APIs (in case > it is _really_ needed). I wouldn't say the properties are shared: we have a single node in the device tree as this is just one device. But as we create several (software) devices in the MFD driver, we now have several devices linked with a single device tree node. One solution would be to create more subnodes in the device tree, one for pinctrl and one for PWM, but this feels a bit like describing our software implementation in the device tree instead of describing the hardware.
On Wed Mar 26, 2025 at 12:00 PM CET, Mathieu Dubois-Briand wrote: > On Tue Mar 25, 2025 at 8:50 AM CET, Michael Walle wrote: > > > > > +#ifdef CONFIG_GPIOLIB_IRQCHIP > > > > > > > > Why do we need this ifdef? > > > > > > > > > > Hum yes, on second thought we probably need to depend on > > > CONFIG_REGMAP_IRQ here. > > > > But then, you'd also require the regmap_irq support for chips that > > don't support IRQs at all. devm_regmap_add_irq_fwnode() seems to be > > missing a stub version. > > > > Sorry, maybe my previous message was not clear, when I said "depend", > what I meant is having an "#ifdef CONFIG_REGMAP_IRQ" here in place of > "#ifdef CONFIG_GPIOLIB_IRQCHIP" > > If CONFIG_REGMAP_IRQ is enabled, drivers/base/regmap/regmap-irq.c is > built, so we do have both devm_regmap_add_irq_chip_fwnode() and > regmap_irq_get_domain(). So this code block should compile and link > correctly. Yes. > I did some build tests with and without CONFIG_GPIOLIB_IRQCHIP and I > believe this is fine. > > Or am I missing something? I'd like to avoid the ifdef macros if possible. Thus you'd need stubs for devm_regmap_add_irq_chip_fwnode() and regmap_irq_get_domain() if CONFIG_REGMAP_IRQ is not defined. Not sure if broonie agrees though (?). -michael
On Fri, Mar 28, 2025 at 09:13:12AM +0100, Mathieu Dubois-Briand wrote: > On Thu Mar 27, 2025 at 6:50 PM CET, Andy Shevchenko wrote: > > On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote: > > > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote: ... > > > > The use of this API is inappropriate here AFAICT. It drops the parent refcount > > > > and on the second call to it you will have a warning from refcount library. > > > > > > > > It should be as simple as device_set_node(). > > > > > > > > > } > > > > > > > > With that, the conditional becomes > > > > > > > > } else if (is_of_node(fwnode)) { > > > > device_set_node(&pdev->dev, fwnode); > > > > } > > > > > > > > where fwnode is something like > > > > > > > > struct fwnode_handle *fwnode = dev_fwnode(parent); > > > > > > I tried to use device_set_node(), but then I got some other issue: as we > > > now have several devices with the same firmware node, they all share the > > > same properties. In particular, if we do use pinctrl- properties to > > > apply some pinmmuxing, all devices will try to apply this pinmuxing and > > > of course all but one will fail. > > > > > > And this makes me think again about the whole thing, maybe copying the > > > fwnode or of_node from the parent is not the way to go. > > > > > > So today we rely on the parent node for four drivers: > > > - keypad and rotary, just to ease a bit the parsing of some properties, > > > such as the keymap with matrix_keypad_build_keymap(). I can easily do > > > it another way. > > > - PWM and pinctrl drivers, are a bit more complicated, as in both case > > > the device tree node associated with the device is used internally. In > > > one case to find the correct PWM device for PWM clients listed in the > > > device tree, in the other case to find the pinctrl device when > > > applying pinctrl described in the device tree. > > > > > > So maybe I have to find a better way for have this association. One way > > > would be to modify the device tree bindings to add a PWM and a pinctrl > > > node, with their own compatible, so they are associated to the > > > corresponding device. But maybe there is a better way to do it. > > > > Okay, so the main question now, why do the device share their properties > > to begin with? It can be done via fwnode graph or similar APIs (in case > > it is _really_ needed). > > I wouldn't say the properties are shared: we have a single node in the > device tree as this is just one device. But as we create several > (software) devices in the MFD driver, we now have several devices linked > with a single device tree node. > > One solution would be to create more subnodes in the device tree, one > for pinctrl and one for PWM, but this feels a bit like describing our > software implementation in the device tree instead of describing the > hardware. I see. From my point of view the above is the correct approach, but you need to ask DT experts, I'm not one of them.
This series implements a set of drivers allowing to support the Maxim Integrated MAX7360 device. The MAX7360 is an I2C key-switch and led controller, with following functionalities: - Keypad controller for a key matrix of up to 8 rows and 8 columns. - Rotary encoder support, for a single rotary encoder. - Up to 8 PWM outputs. - Up to 8 GPIOs with support for interrupts and 6 GPOs. Chipset pins are shared between all functionalities, so all cannot be used at the same time. Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> --- Changes in v5: - Add pinctrl driver to replace the previous use of request()/free() callbacks for PORT pins. - Remove ngpios property from GPIO device tree bindings. - Use GPIO valid_mask to mark unusable keypad columns GPOs, instead of changing ngpios. - Drop patches adding support for request()/free() callbacks in GPIO regmap and gpio_regmap_get_ngpio(). - Allow gpio_regmap_register() to create the associated regmap IRQ. - Various fixes in MFD, PWM, GPIO and KEYPAD drivers. - Link to v4: https://lore.kernel.org/r/20250214-mdb-max7360-support-v4-0-8a35c6dbb966@bootlin.com Changes in v4: - Modified the GPIO driver to use gpio-regmap and regmap-irq. - Add support for request()/free() callbacks in gpio-regmap. - Add support for status_is_level in regmap-irq. - Switched the PWM driver to waveform callbacks. - Various small fixes in MFD, PWM, GPIO drivers and dt bindings. - Rebased on v6.14-rc2. - Link to v3: https://lore.kernel.org/r/20250113-mdb-max7360-support-v3-0-9519b4acb0b1@bootlin.com Changes in v3: - Fix MFD device tree binding to add gpio child nodes. - Fix various small issues in device tree bindings. - Add missing line returns in error messages. - Use dev_err_probe() when possible. - Link to v2: https://lore.kernel.org/r/20241223-mdb-max7360-support-v2-0-37a8d22c36ed@bootlin.com Changes in v2: - Removing device tree subnodes for keypad, rotary encoder and pwm functionalities. - Fixed dt-bindings syntax and naming. - Fixed missing handling of requested period in PWM driver. - Cleanup of the code - Link to v1: https://lore.kernel.org/r/20241219-mdb-max7360-support-v1-0-8e8317584121@bootlin.com --- Kamel Bouhara (2): mfd: Add max7360 support pwm: max7360: Add MAX7360 PWM support Mathieu Dubois-Briand (9): dt-bindings: mfd: gpio: Add MAX7360 pinctrl: Add MAX7360 pinctrl driver regmap: irq: Add support for chips without separate IRQ status gpio: regmap: Allow to allocate regmap-irq device gpio: regmap: Allow to provide init_valid_mask callback gpio: max7360: Add MAX7360 gpio support input: keyboard: Add support for MAX7360 keypad input: misc: Add support for MAX7360 rotary MAINTAINERS: Add entry on MAX7360 driver .../bindings/gpio/maxim,max7360-gpio.yaml | 83 +++++++ .../devicetree/bindings/mfd/maxim,max7360.yaml | 170 +++++++++++++ MAINTAINERS | 13 + drivers/base/regmap/regmap-irq.c | 97 +++++--- drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-max7360.c | 246 +++++++++++++++++++ drivers/gpio/gpio-regmap.c | 26 +- drivers/input/keyboard/Kconfig | 12 + drivers/input/keyboard/Makefile | 1 + drivers/input/keyboard/max7360-keypad.c | 264 +++++++++++++++++++++ drivers/input/misc/Kconfig | 11 + drivers/input/misc/Makefile | 1 + drivers/input/misc/max7360-rotary.c | 161 +++++++++++++ drivers/mfd/Kconfig | 14 ++ drivers/mfd/Makefile | 1 + drivers/mfd/max7360.c | 185 +++++++++++++++ drivers/pinctrl/Kconfig | 11 + drivers/pinctrl/Makefile | 1 + drivers/pinctrl/pinctrl-max7360.c | 195 +++++++++++++++ drivers/pwm/Kconfig | 11 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-max7360.c | 194 +++++++++++++++ include/linux/gpio/regmap.h | 22 ++ include/linux/mfd/max7360.h | 112 +++++++++ include/linux/regmap.h | 3 + 26 files changed, 1813 insertions(+), 35 deletions(-) --- base-commit: a64dcfb451e254085a7daee5fe51bf22959d52d3 change-id: 20241219-mdb-max7360-support-223a8ce45ba3 Best regards,