| Message ID | 20230614231446.3687-4-andriy.shevchenko@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: aggregator: Incorporate gpio-delay functionality | expand |
On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > The aggregator mode can also handle properties of the platform, > that do not belong to the GPIO controller itself. One of such > a property is a signal delay line. Set up a parser to support it. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> I guess this is one of those instances where we actually need some OF-specific code for parsing the special case, and other HW descriptions will need other special code. It's fine for non-trivial stuff I think. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Andy, On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > The aggregator mode can also handle properties of the platform, > that do not belong to the GPIO controller itself. One of such > a property is a signal delay line. Set up a parser to support it. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> One suggestion for improvement below... > --- a/drivers/gpio/gpio-aggregator.c > +++ b/drivers/gpio/gpio-aggregator.c > @@ -525,7 +580,9 @@ static int gpio_aggregator_probe(struct platform_device *pdev) > return PTR_ERR(descs[i]); > } > > - fwd = gpiochip_fwd_create(dev, n, descs); > + delay_line = fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay"); Please do not use explicit checks for compatible values in .probe() methods. Instead, use device_get_match_data() to get the feature flag(s). This will also make it easier to scale to other external components later. > + > + fwd = gpiochip_fwd_create(dev, n, descs, delay_line); > if (IS_ERR(fwd)) > return PTR_ERR(fwd); > > @@ -534,6 +591,15 @@ static int gpio_aggregator_probe(struct platform_device *pdev) > } > > static const struct of_device_id gpio_aggregator_dt_ids[] = { > + /* > + * The GPIO delay provides a way to configure platform specific delays > + * for GPIO ramp-up or ramp-down delays. This can serve the following > + * purposes: > + * - Open-drain output using an RC filter > + */ > + { > + .compatible = "gpio-delay", .data = (void *)FWD_FEATURE_DELAY, > + }, > /* > * Add GPIO-operated devices controlled from userspace below, > * or use "driver_override" in sysfs. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Jun 15, 2023 at 9:37 AM Linus Walleij <linus.walleij@linaro.org> wrote: > On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > The aggregator mode can also handle properties of the platform, > > that do not belong to the GPIO controller itself. One of such > > a property is a signal delay line. Set up a parser to support it. > > > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > I guess this is one of those instances where we actually need some OF-specific > code for parsing the special case, and other HW descriptions will need > other special code. It's fine for non-trivial stuff I think. Actually if you look into IIO, the xlate there is fwnode specific nowadays and theoretically the specifics of OF can be parsed in ACPI as well. I would like to get rid of OF_GPIO completely, but let's see... > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Thank you!
On Thu, Jun 15, 2023 at 10:48 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Jun 15, 2023 at 1:14 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > The aggregator mode can also handle properties of the platform, > > that do not belong to the GPIO controller itself. One of such > > a property is a signal delay line. Set up a parser to support it. > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Thank you! > One suggestion for improvement below... Sure, for v3!
diff --git a/drivers/gpio/gpio-aggregator.c b/drivers/gpio/gpio-aggregator.c index a74a8d86caf3..ed11aa56bc51 100644 --- a/drivers/gpio/gpio-aggregator.c +++ b/drivers/gpio/gpio-aggregator.c @@ -18,6 +18,7 @@ #include <linux/mutex.h> #include <linux/overflow.h> #include <linux/platform_device.h> +#include <linux/property.h> #include <linux/slab.h> #include <linux/spinlock.h> #include <linux/string.h> @@ -423,6 +424,51 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset) return gpiod_to_irq(fwd->descs[offset]); } +#ifdef CONFIG_OF_GPIO +static int gpiochip_fwd_delay_of_xlate(struct gpio_chip *chip, + const struct of_phandle_args *gpiospec, + u32 *flags) +{ + struct gpiochip_fwd *fwd = gpiochip_get_data(chip); + struct gpiochip_fwd_timing *timings; + u32 line; + + if (gpiospec->args_count != chip->of_gpio_n_cells) + return -EINVAL; + + line = gpiospec->args[0]; + if (line >= chip->ngpio) + return -EINVAL; + + timings = &fwd->delay_timings[line]; + timings->ramp_up_us = gpiospec->args[1]; + timings->ramp_down_us = gpiospec->args[2]; + + return line; +} + +static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *chip, + struct gpiochip_fwd *fwd) +{ + fwd->delay_timings = devm_kcalloc(dev, chip->ngpio, + sizeof(*fwd->delay_timings), + GFP_KERNEL); + if (!fwd->delay_timings) + return -ENOMEM; + + chip->of_xlate = gpiochip_fwd_delay_of_xlate; + chip->of_gpio_n_cells = 3; + + return 0; +} +#else +static int gpiochip_fwd_setup_delay_line(struct device *dev, struct gpio_chip *chip, + struct gpiochip_fwd *fwd) +{ + return 0; +} +#endif /* !CONFIG_OF_GPIO */ + /** * gpiochip_fwd_create() - Create a new GPIO forwarder * @dev: Parent device pointer @@ -430,6 +476,7 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset) * @descs: Array containing the GPIO descriptors to forward to. * This array must contain @ngpios entries, and must not be deallocated * before the forwarder has been destroyed again. + * @delay_line: True if the pins have an external delay line. * * This function creates a new gpiochip, which forwards all GPIO operations to * the passed GPIO descriptors. @@ -439,7 +486,8 @@ static int gpio_fwd_to_irq(struct gpio_chip *chip, unsigned int offset) */ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev, unsigned int ngpios, - struct gpio_desc *descs[]) + struct gpio_desc *descs[], + bool delay_line) { const char *label = dev_name(dev); struct gpiochip_fwd *fwd; @@ -492,6 +540,12 @@ static struct gpiochip_fwd *gpiochip_fwd_create(struct device *dev, else spin_lock_init(&fwd->slock); + if (delay_line) { + error = gpiochip_fwd_setup_delay_line(dev, chip, fwd); + if (error) + return ERR_PTR(error); + } + error = devm_gpiochip_add_data(dev, chip, fwd); if (error) return ERR_PTR(error); @@ -509,6 +563,7 @@ static int gpio_aggregator_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct gpio_desc **descs; struct gpiochip_fwd *fwd; + bool delay_line; int i, n; n = gpiod_count(dev, NULL); @@ -525,7 +580,9 @@ static int gpio_aggregator_probe(struct platform_device *pdev) return PTR_ERR(descs[i]); } - fwd = gpiochip_fwd_create(dev, n, descs); + delay_line = fwnode_device_is_compatible(dev_fwnode(dev), "gpio-delay"); + + fwd = gpiochip_fwd_create(dev, n, descs, delay_line); if (IS_ERR(fwd)) return PTR_ERR(fwd); @@ -534,6 +591,15 @@ static int gpio_aggregator_probe(struct platform_device *pdev) } static const struct of_device_id gpio_aggregator_dt_ids[] = { + /* + * The GPIO delay provides a way to configure platform specific delays + * for GPIO ramp-up or ramp-down delays. This can serve the following + * purposes: + * - Open-drain output using an RC filter + */ + { + .compatible = "gpio-delay", + }, /* * Add GPIO-operated devices controlled from userspace below, * or use "driver_override" in sysfs.
The aggregator mode can also handle properties of the platform, that do not belong to the GPIO controller itself. One of such a property is a signal delay line. Set up a parser to support it. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-aggregator.c | 70 +++++++++++++++++++++++++++++++++- 1 file changed, 68 insertions(+), 2 deletions(-)