| Message ID | 20250409-mdb-max7360-support-v6-3-7a2535876e39@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for MAX7360 | expand |
On Wed Apr 9, 2025 at 4:55 PM CEST, 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. > > Signed-off-by: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > ... > diff --git a/drivers/pinctrl/pinctrl-max7360.c b/drivers/pinctrl/pinctrl-max7360.c > ... > +static int max7360_pinctrl_probe(struct platform_device *pdev) > +{ > + struct regmap *regmap; > + struct pinctrl_desc *pd; > + struct max7360_pinctrl *chip; > + struct device *dev = &pdev->dev; > + > + regmap = dev_get_regmap(dev->parent, NULL); > + if (!regmap) > + dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n"); > + > + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > + if (!chip) > + return -ENOMEM; > + > + pd = &chip->pinctrl_desc; > + > + pd->pctlops = &max7360_pinctrl_ops; > + pd->pmxops = &max7360_pmxops; > + pd->name = dev_name(dev); > + pd->pins = max7360_pins; > + pd->npins = MAX7360_MAX_GPIO; > + pd->owner = THIS_MODULE; > + > + device_set_of_node_from_dev(dev, dev->parent); Ok, so this goes a bit against what I said I was going to do on my previous series, let me explain why. Same reasoning applies for both uses, in PWM and pinctrl drivers. With my previous experiments, I came to the conclusion that: - Either we should use device_set_of_node_from_dev() as I do here. - Or we should add more subnodes in the device tree binding. - Also, copying the fwnode with device_set_node() was not possible, as the kernel would then try to apply pinctrl on both the parent and child device. I previously said the second solution was probably the way to go, but I changed my mind for two reasons. First having more subnodes in the device tree was already rejected in the past in the reviews of the dt-bindings patch. This do makes sense as it would be describing device internals (which should not be made in DT), just to ease one specific software implementation (which should also be avoided). So I believe this change would again be rejected. https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/ But the the second reason is, doing 'git grep "device_set_of_node_from_dev.*parent"', I found several drivers using device_set_of_node_from_dev() for a similar need. Some of these uses are also for MFD child devices: - gpio-adp5585.c / pwm-adp5585.c, - pwm-ntxec.c, - max77620-regulator.c / max77620_thermal.c. So, based on this, I believe using device_set_of_node_from_dev() in these two drivers is the way to go. > + chip->pctldev = devm_pinctrl_register(dev, pd, chip); > + if (IS_ERR(chip->pctldev)) > + return dev_err_probe(dev, PTR_ERR(chip->pctldev), "can't register controller\n"); > + > + return 0; > +} > +
On Wed, Apr 09, 2025 at 05:03:02PM +0200, Mathieu Dubois-Briand wrote: > On Wed Apr 9, 2025 at 4:55 PM CEST, 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. ... The all the rest of the driver LGTM, but the below. > > + device_set_of_node_from_dev(dev, dev->parent); > > Ok, so this goes a bit against what I said I was going to do on my > previous series, let me explain why. Same reasoning applies for both > uses, in PWM and pinctrl drivers. > > With my previous experiments, I came to the conclusion that: > - Either we should use device_set_of_node_from_dev() as I do here. > - Or we should add more subnodes in the device tree binding. > - Also, copying the fwnode with device_set_node() was not possible, as > the kernel would then try to apply pinctrl on both the parent and > child device. Hmm... I need to refresh my memory with the old discussions. Can you point out to the problem statement with that approach? > I previously said the second solution was probably the way to go, but I > changed my mind for two reasons. > > First having more subnodes in the device tree was already rejected in > the past in the reviews of the dt-bindings patch. This do makes sense as > it would be describing device internals (which should not be made in > DT), just to ease one specific software implementation (which should > also be avoided). So I believe this change would again be rejected. > https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/ > > But the the second reason is, doing > 'git grep "device_set_of_node_from_dev.*parent"', I found several > drivers using device_set_of_node_from_dev() for a similar need. Some of > these uses are also for MFD child devices: > - gpio-adp5585.c / pwm-adp5585.c, > - pwm-ntxec.c, > - max77620-regulator.c / max77620_thermal.c. > > So, based on this, I believe using device_set_of_node_from_dev() in > these two drivers is the way to go. The problem with this solution is that, It's OF-centric. Which shouldn't be done in a new code (and I don't see impediments to avoid it). Yes, it does the right thing for the case, but only on OF systems. Note, fwnode is a list of maximum of two entries (yeah, designed like that right now), can you utilise that somehow?
On Wed Apr 9, 2025 at 6:32 PM CEST, Andy Shevchenko wrote: > On Wed, Apr 09, 2025 at 05:03:02PM +0200, Mathieu Dubois-Briand wrote: >> On Wed Apr 9, 2025 at 4:55 PM CEST, 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. > > ... > > The all the rest of the driver LGTM, but the below. > >> > + device_set_of_node_from_dev(dev, dev->parent); >> >> Ok, so this goes a bit against what I said I was going to do on my >> previous series, let me explain why. Same reasoning applies for both >> uses, in PWM and pinctrl drivers. >> >> With my previous experiments, I came to the conclusion that: >> - Either we should use device_set_of_node_from_dev() as I do here. >> - Or we should add more subnodes in the device tree binding. > >> - Also, copying the fwnode with device_set_node() was not possible, as >> the kernel would then try to apply pinctrl on both the parent and >> child device. > > Hmm... I need to refresh my memory with the old discussions. Can you point out > to the problem statement with that approach? > I mentioned here briefly in my previous series: https://lore.kernel.org/lkml/D8R4B2PKIWSU.2LWTN50YP7SMX@bootlin.com/ So the issue is, if I copy the parent fwnode using device_set_node(), the kernel is trying to apply any pinctrl defined on the node with pinctrl- properties on both the parent and the child node. Of course, only the first one will succeed, as two devices cannot request the same pins at the same time. >> I previously said the second solution was probably the way to go, but I >> changed my mind for two reasons. >> >> First having more subnodes in the device tree was already rejected in >> the past in the reviews of the dt-bindings patch. This do makes sense as >> it would be describing device internals (which should not be made in >> DT), just to ease one specific software implementation (which should >> also be avoided). So I believe this change would again be rejected. >> https://lore.kernel.org/lkml/58c80c2a-2532-4bc5-9c9f-52480b3af52a@kernel.org/ >> >> But the the second reason is, doing >> 'git grep "device_set_of_node_from_dev.*parent"', I found several >> drivers using device_set_of_node_from_dev() for a similar need. Some of >> these uses are also for MFD child devices: >> - gpio-adp5585.c / pwm-adp5585.c, >> - pwm-ntxec.c, >> - max77620-regulator.c / max77620_thermal.c. >> >> So, based on this, I believe using device_set_of_node_from_dev() in >> these two drivers is the way to go. > > The problem with this solution is that, It's OF-centric. Which shouldn't be > done in a new code (and I don't see impediments to avoid it). Yes, it does > the right thing for the case, but only on OF systems. Note, fwnode is a list > of maximum of two entries (yeah, designed like that right now), can you utilise > that somehow? Looking at MFD code, I believe ACPI MFD child devices already get the parent fwnode, except if a fwnode exists for them. https://elixir.bootlin.com/linux/v6.13.7/source/drivers/mfd/mfd-core.c#L90 Thanks for your review.
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig index 464cc9aca157..276695c7a92e 100644 --- a/drivers/pinctrl/Kconfig +++ b/drivers/pinctrl/Kconfig @@ -348,6 +348,17 @@ config PINCTRL_LPC18XX help Pinctrl driver for NXP LPC18xx/43xx System Control Unit (SCU). +config PINCTRL_MAX7360 + tristate "MAX7360 Pincontrol support" + depends on MFD_MAX7360 + select PINMUX + select GENERIC_PINCONF + help + Say Y here to enable pin control support for Maxim MAX7360 keypad + controller. + This keypad controller has 8 GPIO pins that may work as GPIO, or PWM, + or rotary encoder alternate modes. + config PINCTRL_MAX77620 tristate "MAX77620/MAX20024 Pincontrol support" depends on MFD_MAX77620 && OF diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile index ac27e88677d1..974a179b5593 100644 --- a/drivers/pinctrl/Makefile +++ b/drivers/pinctrl/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_PINCTRL_FALCON) += pinctrl-falcon.o obj-$(CONFIG_PINCTRL_LOONGSON2) += pinctrl-loongson2.o obj-$(CONFIG_PINCTRL_XWAY) += pinctrl-xway.o obj-$(CONFIG_PINCTRL_LPC18XX) += pinctrl-lpc18xx.o +obj-$(CONFIG_PINCTRL_MAX7360) += pinctrl-max7360.o obj-$(CONFIG_PINCTRL_MAX77620) += pinctrl-max77620.o obj-$(CONFIG_PINCTRL_MCP23S08_I2C) += pinctrl-mcp23s08_i2c.o obj-$(CONFIG_PINCTRL_MCP23S08_SPI) += pinctrl-mcp23s08_spi.o diff --git a/drivers/pinctrl/pinctrl-max7360.c b/drivers/pinctrl/pinctrl-max7360.c new file mode 100644 index 000000000000..ebd98666cd39 --- /dev/null +++ b/drivers/pinctrl/pinctrl-max7360.c @@ -0,0 +1,208 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright 2025 Bootlin + * + * Author: Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com> + */ + +#include <linux/array_size.h> +#include <linux/dev_printk.h> +#include <linux/device.h> +#include <linux/device/devres.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/mfd/max7360.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/slab.h> + +#include <linux/pinctrl/pinctrl.h> +#include <linux/pinctrl/pinconf-generic.h> +#include <linux/pinctrl/pinmux.h> + +#include "core.h" +#include "pinmux.h" + +struct max7360_pinctrl { + struct pinctrl_dev *pctldev; + struct pinctrl_desc pinctrl_desc; +}; + +static const struct pinctrl_pin_desc max7360_pins[] = { + PINCTRL_PIN(0, "PORT0"), + PINCTRL_PIN(1, "PORT1"), + PINCTRL_PIN(2, "PORT2"), + PINCTRL_PIN(3, "PORT3"), + PINCTRL_PIN(4, "PORT4"), + PINCTRL_PIN(5, "PORT5"), + PINCTRL_PIN(6, "PORT6"), + PINCTRL_PIN(7, "PORT7"), +}; + +static const unsigned int port0_pins[] = {0}; +static const unsigned int port1_pins[] = {1}; +static const unsigned int port2_pins[] = {2}; +static const unsigned int port3_pins[] = {3}; +static const unsigned int port4_pins[] = {4}; +static const unsigned int port5_pins[] = {5}; +static const unsigned int port6_pins[] = {6}; +static const unsigned int port7_pins[] = {7}; +static const unsigned int rotary_pins[] = {6, 7}; + +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)), +}; + +static int max7360_pinctrl_get_groups_count(struct pinctrl_dev *pctldev) +{ + return ARRAY_SIZE(max7360_groups); +} + +static const char *max7360_pinctrl_get_group_name(struct pinctrl_dev *pctldev, + unsigned int group) +{ + return max7360_groups[group].name; +} + +static int max7360_pinctrl_get_group_pins(struct pinctrl_dev *pctldev, + unsigned int group, + const unsigned int **pins, + unsigned int *num_pins) +{ + *pins = max7360_groups[group].pins; + *num_pins = max7360_groups[group].npins; + return 0; +} + +static const struct pinctrl_ops max7360_pinctrl_ops = { + .get_groups_count = max7360_pinctrl_get_groups_count, + .get_group_name = max7360_pinctrl_get_group_name, + .get_group_pins = max7360_pinctrl_get_group_pins, +#ifdef CONFIG_OF + .dt_node_to_map = pinconf_generic_dt_node_to_map_pin, + .dt_free_map = pinconf_generic_dt_free_map, +#endif +}; + +static const char * const simple_groups[] = { + "PORT0", "PORT1", "PORT2", "PORT3", + "PORT4", "PORT5", "PORT6", "PORT7", +}; + +static const char * const rotary_groups[] = { "ROTARY" }; + +#define MAX7360_PINCTRL_FN_GPIO 0 +#define MAX7360_PINCTRL_FN_PWM 1 +#define MAX7360_PINCTRL_FN_ROTARY 2 +static const struct pinfunction max7360_functions[] = { + [MAX7360_PINCTRL_FN_GPIO] = PINCTRL_PINFUNCTION("gpio", simple_groups, + ARRAY_SIZE(simple_groups)), + [MAX7360_PINCTRL_FN_PWM] = PINCTRL_PINFUNCTION("pwm", simple_groups, + ARRAY_SIZE(simple_groups)), + [MAX7360_PINCTRL_FN_ROTARY] = PINCTRL_PINFUNCTION("rotary", rotary_groups, + ARRAY_SIZE(rotary_groups)), +}; + +static int max7360_get_functions_count(struct pinctrl_dev *pctldev) +{ + return ARRAY_SIZE(max7360_functions); +} + +static const char *max7360_get_function_name(struct pinctrl_dev *pctldev, unsigned int selector) +{ + return max7360_functions[selector].name; +} + +static int max7360_get_function_groups(struct pinctrl_dev *pctldev, unsigned int selector, + const char * const **groups, + unsigned int * const num_groups) +{ + *groups = max7360_functions[selector].groups; + *num_groups = max7360_functions[selector].ngroups; + + return 0; +} + +static int max7360_set_mux(struct pinctrl_dev *pctldev, unsigned int selector, + unsigned int group) +{ + struct regmap *regmap; + 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->parent, NULL); + return regmap_write_bits(regmap, MAX7360_REG_GPIOCFG, MAX7360_GPIO_CFG_RTR_EN, val); + } + + return 0; +} + +static const struct pinmux_ops max7360_pmxops = { + .get_functions_count = max7360_get_functions_count, + .get_function_name = max7360_get_function_name, + .get_function_groups = max7360_get_function_groups, + .set_mux = max7360_set_mux, + .strict = true, +}; + +static int max7360_pinctrl_probe(struct platform_device *pdev) +{ + struct regmap *regmap; + struct pinctrl_desc *pd; + struct max7360_pinctrl *chip; + struct device *dev = &pdev->dev; + + regmap = dev_get_regmap(dev->parent, NULL); + if (!regmap) + dev_err_probe(dev, -ENODEV, "Could not get parent regmap\n"); + + chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); + if (!chip) + return -ENOMEM; + + pd = &chip->pinctrl_desc; + + pd->pctlops = &max7360_pinctrl_ops; + pd->pmxops = &max7360_pmxops; + pd->name = dev_name(dev); + pd->pins = max7360_pins; + pd->npins = MAX7360_MAX_GPIO; + pd->owner = THIS_MODULE; + + device_set_of_node_from_dev(dev, dev->parent); + chip->pctldev = devm_pinctrl_register(dev, pd, chip); + if (IS_ERR(chip->pctldev)) + return dev_err_probe(dev, PTR_ERR(chip->pctldev), "can't register controller\n"); + + return 0; +} + +static struct platform_driver max7360_pinctrl_driver = { + .driver = { + .name = "max7360-pinctrl", + }, + .probe = max7360_pinctrl_probe, +}; +module_platform_driver(max7360_pinctrl_driver); + +MODULE_DESCRIPTION("MAX7360 pinctrl driver"); +MODULE_AUTHOR("Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>"); +MODULE_LICENSE("GPL");