| Message ID | 20210921043936.468001-1-andrew@aj.id.au (mailing list archive) |
|---|---|
| Headers | show |
| Series | leds: pca955x: Expose GPIOs for all pins | expand |
On Tue, Sep 21, 2021 at 7:39 AM Andrew Jeffery <andrew@aj.id.au> wrote: > > Hello, > > This is a rework of a Rube Goldberg-inspired RFC I posted previously: > > https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/ > > This time around there's a lot less Rube - the series: > > 1. Contains no (ab)use of pinctrl > 2. Always exposes all pins as GPIOs > 3. Internally tracks the active pins Thanks for the rework! Briefly looking it looks very nice to me, hence, FWIW, Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Without these patches the driver limits the number of pins exposed on > the gpiochip to the number of pins specified as GPIO in the devicetree, > but doesn't map between the GPIO and pin number spaces. The result is > that specifying offset or interleaved GPIOs in the devicetree gives > unexpected behaviour in userspace. > > By always exposing all pins as GPIOs the patches resolve the lack of > mapping between GPIO offsets and pins on the package in the driver by > ensuring we always have a 1-to-1 mapping. > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it > possible to not expose any pins as LEDs (and therefore make them all > accessible as GPIOs). This has a follow-on effect of allowing the driver > to bind to a device instantiated at runtime without requiring a > description in the devicetree. > > I've tested the series under qemu to inspect the various interactions > between LEDs vs GPIOs as well as conflicting GPIO requests. > > Please review! > > Andrew > > Andrew Jeffery (2): > leds: pca955x: Make the gpiochip always expose all pins > leds: pca955x: Allow zero LEDs to be specified > > drivers/leds/leds-pca955x.c | 65 +++++++++++++++++++------------------ > 1 file changed, 34 insertions(+), 31 deletions(-) > > > base-commit: 239f32b4f161c1584cd4b386d6ab8766432a6ede > -- > 2.30.2 >
On Tue, Sep 21, 2021 at 6:39 AM Andrew Jeffery <andrew@aj.id.au> wrote: > 1. Contains no (ab)use of pinctrl > 2. Always exposes all pins as GPIOs > 3. Internally tracks the active pins Looks good to me! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, 21 Sept 2021 at 04:39, Andrew Jeffery <andrew@aj.id.au> wrote: > > Hello, > > This is a rework of a Rube Goldberg-inspired RFC I posted previously: > > https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/ > > This time around there's a lot less Rube - the series: > > 1. Contains no (ab)use of pinctrl > 2. Always exposes all pins as GPIOs > 3. Internally tracks the active pins > > Without these patches the driver limits the number of pins exposed on > the gpiochip to the number of pins specified as GPIO in the devicetree, > but doesn't map between the GPIO and pin number spaces. The result is > that specifying offset or interleaved GPIOs in the devicetree gives > unexpected behaviour in userspace. > > By always exposing all pins as GPIOs the patches resolve the lack of > mapping between GPIO offsets and pins on the package in the driver by > ensuring we always have a 1-to-1 mapping. > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it > possible to not expose any pins as LEDs (and therefore make them all > accessible as GPIOs). This has a follow-on effect of allowing the driver > to bind to a device instantiated at runtime without requiring a > description in the devicetree. > > I've tested the series under qemu to inspect the various interactions > between LEDs vs GPIOs as well as conflicting GPIO requests. > > Please review! Reviewed-by: Joel Stanley <joel@jms.id.au> Cheers, Joel
On 9/21/21 06:39, Andrew Jeffery wrote: > Hello, > > This is a rework of a Rube Goldberg-inspired RFC I posted previously: > > https://lore.kernel.org/lkml/20210723075858.376378-1-andrew@aj.id.au/ > > This time around there's a lot less Rube - the series: > > 1. Contains no (ab)use of pinctrl > 2. Always exposes all pins as GPIOs > 3. Internally tracks the active pins > > Without these patches the driver limits the number of pins exposed on > the gpiochip to the number of pins specified as GPIO in the devicetree, > but doesn't map between the GPIO and pin number spaces. The result is > that specifying offset or interleaved GPIOs in the devicetree gives > unexpected behaviour in userspace. > > By always exposing all pins as GPIOs the patches resolve the lack of > mapping between GPIO offsets and pins on the package in the driver by > ensuring we always have a 1-to-1 mapping. > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it > possible to not expose any pins as LEDs (and therefore make them all > accessible as GPIOs). This has a follow-on effect of allowing the driver > to bind to a device instantiated at runtime without requiring a > description in the devicetree. > > I've tested the series under qemu to inspect the various interactions > between LEDs vs GPIOs as well as conflicting GPIO requests. I was wondering what kind of tests you did. Did you cold plug 'pca995x' devices in a QEMU Aspeed EVB* machine running with a custom DTB adding different layouts of the pca955x ? > Please review! This is simpler than the 'ngpio' business we had before. Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > Andrew > > Andrew Jeffery (2): > leds: pca955x: Make the gpiochip always expose all pins > leds: pca955x: Allow zero LEDs to be specified > > drivers/leds/leds-pca955x.c | 65 +++++++++++++++++++------------------ > 1 file changed, 34 insertions(+), 31 deletions(-) > > > base-commit: 239f32b4f161c1584cd4b386d6ab8766432a6ede >
Hello Pavel, On Fri, 24 Sept 2021 at 06:41, Cédric Le Goater <clg@kaod.org> wrote: > > On 9/21/21 06:39, Andrew Jeffery wrote: > > Without these patches the driver limits the number of pins exposed on > > the gpiochip to the number of pins specified as GPIO in the devicetree, > > but doesn't map between the GPIO and pin number spaces. The result is > > that specifying offset or interleaved GPIOs in the devicetree gives > > unexpected behaviour in userspace. > > > > By always exposing all pins as GPIOs the patches resolve the lack of > > mapping between GPIO offsets and pins on the package in the driver by > > ensuring we always have a 1-to-1 mapping. > > > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it > > possible to not expose any pins as LEDs (and therefore make them all > > accessible as GPIOs). This has a follow-on effect of allowing the driver > > to bind to a device instantiated at runtime without requiring a > > description in the devicetree. > > > > I've tested the series under qemu to inspect the various interactions > > between LEDs vs GPIOs as well as conflicting GPIO requests. > > Please review! > > This is simpler than the 'ngpio' business we had before. > > Reviewed-by: Cédric Le Goater <clg@kaod.org> I saw that you recently merged some LED patches. I was wondering if you could consider this series for v5.18. It still applies cleanly, and we've been running it for a while now, so it's very well tested. Cheers, Joel
Hi! > > > Without these patches the driver limits the number of pins exposed on > > > the gpiochip to the number of pins specified as GPIO in the devicetree, > > > but doesn't map between the GPIO and pin number spaces. The result is > > > that specifying offset or interleaved GPIOs in the devicetree gives > > > unexpected behaviour in userspace. > > > > > > By always exposing all pins as GPIOs the patches resolve the lack of > > > mapping between GPIO offsets and pins on the package in the driver by > > > ensuring we always have a 1-to-1 mapping. > > > > > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it > > > possible to not expose any pins as LEDs (and therefore make them all > > > accessible as GPIOs). This has a follow-on effect of allowing the driver > > > to bind to a device instantiated at runtime without requiring a > > > description in the devicetree. > > > > > > I've tested the series under qemu to inspect the various interactions > > > between LEDs vs GPIOs as well as conflicting GPIO requests. > > > > Please review! > > > > This is simpler than the 'ngpio' business we had before. > > > > Reviewed-by: Cédric Le Goater <clg@kaod.org> > > I saw that you recently merged some LED patches. I was wondering if > you could consider this series for v5.18. It still applies cleanly, > and we've been running it for a while now, so it's very well tested. Thanks, applied. I must say this is really ninja-mutant driver, but I see no better way. +++ b/drivers/leds/leds-pca955x.c @@ -429,7 +429,7 @@ pca955x_get_pdata(struct i2c_client *client, struct pca955x_chipdef *chip) int count; This really should be unsigned. Care to fix/submit a patch? Best regards, Pavel
On Wed, 2 Mar 2022, at 19:24, Pavel Machek wrote: > Hi! > >> > > Without these patches the driver limits the number of pins exposed on >> > > the gpiochip to the number of pins specified as GPIO in the devicetree, >> > > but doesn't map between the GPIO and pin number spaces. The result is >> > > that specifying offset or interleaved GPIOs in the devicetree gives >> > > unexpected behaviour in userspace. >> > > >> > > By always exposing all pins as GPIOs the patches resolve the lack of >> > > mapping between GPIO offsets and pins on the package in the driver by >> > > ensuring we always have a 1-to-1 mapping. >> > > >> > > The issue is primarily addressed by patch 1/2. Patch 2/2 makes it >> > > possible to not expose any pins as LEDs (and therefore make them all >> > > accessible as GPIOs). This has a follow-on effect of allowing the driver >> > > to bind to a device instantiated at runtime without requiring a >> > > description in the devicetree. >> > > >> > > I've tested the series under qemu to inspect the various interactions >> > > between LEDs vs GPIOs as well as conflicting GPIO requests. >> >> > > Please review! >> > >> > This is simpler than the 'ngpio' business we had before. >> > >> > Reviewed-by: Cédric Le Goater <clg@kaod.org> >> >> I saw that you recently merged some LED patches. I was wondering if >> you could consider this series for v5.18. It still applies cleanly, >> and we've been running it for a while now, so it's very well tested. > > Thanks, applied. I must say this is really ninja-mutant driver, but I > see no better way. > > +++ b/drivers/leds/leds-pca955x.c > @@ -429,7 +429,7 @@ pca955x_get_pdata(struct i2c_client *client, struct > pca955x_chipdef *chip) > int count; > > This really should be unsigned. Care to fix/submit a patch? I'll send a fix. Andrew