Skip to content

Commit ca38625

Browse files
amboarpavelmachek
authored andcommitted
leds: pca955x: Make the gpiochip always expose all pins
The devicetree binding allows specifying which pins are GPIO vs LED. Limiting the instantiated gpiochip to just these pins as the driver currently does requires an arbitrary mapping between pins and GPIOs, but such a mapping is not implemented by the driver. As a result, specifying GPIOs in such a way that they don't map 1-to-1 to pin indexes does not function as expected. Establishing such a mapping is more complex than not and even if we did, doing so leads to a slightly hairy userspace experience as the behaviour of the PCA955x gpiochip would depend on how the pins are assigned in the devicetree. Instead, always expose all pins via the gpiochip to provide a stable interface and track which pins are in use. Specifying a pin as `type = <PCA955X_TYPE_GPIO>;` in the devicetree becomes a no-op. I've assessed the impact of this change by looking through all of the affected devicetrees as of the tag leds-5.15-rc1: ``` $ git grep -l 'pca955[0123]' $(find . -name dts -type d) arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts arch/arm/boot/dts/aspeed-bmc-opp-mihawk.dts arch/arm/boot/dts/aspeed-bmc-opp-mowgli.dts arch/arm/boot/dts/aspeed-bmc-opp-swift.dts arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts ``` These are all IBM-associated platforms. I've analysed both the devicetrees and schematics where necessary to determine whether any systems hit the hazard of the current broken behaviour. For the most part, the systems specify the pins as either all LEDs or all GPIOs, or at least do so in a way such that the broken behaviour isn't exposed. The main counter-point to this observation is the Everest system whose devicetree describes a large number of PCA955x devices and in some cases has pin assignments that hit the hazard. However, there does not seem to be any use of the affected GPIOs in the userspace associated with Everest. Regardless, any use of the hazardous GPIOs in Everest is already broken, so let's fix the interface and then fix any already broken userspace with it. Signed-off-by: Andrew Jeffery <andrew@aj.id.au> Acked-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Cédric Le Goater <clg@kaod.org> Signed-off-by: Pavel Machek <pavel@ucw.cz>
1 parent 8b43ef0 commit ca38625

File tree

1 file changed

+34
-31
lines changed

1 file changed

+34
-31
lines changed

drivers/leds/leds-pca955x.c

Lines changed: 34 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@
3737
* bits the chip supports.
3838
*/
3939

40+
#include <linux/bitops.h>
4041
#include <linux/ctype.h>
4142
#include <linux/delay.h>
4243
#include <linux/err.h>
@@ -118,6 +119,7 @@ struct pca955x {
118119
struct pca955x_led *leds;
119120
struct pca955x_chipdef *chipdef;
120121
struct i2c_client *client;
122+
unsigned long active_pins;
121123
#ifdef CONFIG_LEDS_PCA955X_GPIO
122124
struct gpio_chip gpio;
123125
#endif
@@ -360,12 +362,15 @@ static int pca955x_read_input(struct i2c_client *client, int n, u8 *val)
360362
static int pca955x_gpio_request_pin(struct gpio_chip *gc, unsigned int offset)
361363
{
362364
struct pca955x *pca955x = gpiochip_get_data(gc);
363-
struct pca955x_led *led = &pca955x->leds[offset];
364365

365-
if (led->type == PCA955X_TYPE_GPIO)
366-
return 0;
366+
return test_and_set_bit(offset, &pca955x->active_pins) ? -EBUSY : 0;
367+
}
368+
369+
static void pca955x_gpio_free_pin(struct gpio_chip *gc, unsigned int offset)
370+
{
371+
struct pca955x *pca955x = gpiochip_get_data(gc);
367372

368-
return -EBUSY;
373+
clear_bit(offset, &pca955x->active_pins);
369374
}
370375

371376
static int pca955x_set_value(struct gpio_chip *gc, unsigned int offset,
@@ -489,7 +494,6 @@ static int pca955x_probe(struct i2c_client *client)
489494
struct i2c_adapter *adapter;
490495
int i, err;
491496
struct pca955x_platform_data *pdata;
492-
int ngpios = 0;
493497
bool set_default_label = false;
494498
bool keep_pwm = false;
495499
char default_label[8];
@@ -567,9 +571,7 @@ static int pca955x_probe(struct i2c_client *client)
567571

568572
switch (pca955x_led->type) {
569573
case PCA955X_TYPE_NONE:
570-
break;
571574
case PCA955X_TYPE_GPIO:
572-
ngpios++;
573575
break;
574576
case PCA955X_TYPE_LED:
575577
led = &pca955x_led->led_cdev;
@@ -613,6 +615,8 @@ static int pca955x_probe(struct i2c_client *client)
613615
if (err)
614616
return err;
615617

618+
set_bit(i, &pca955x->active_pins);
619+
616620
/*
617621
* For default-state == "keep", let the core update the
618622
* brightness from the hardware, then check the
@@ -650,31 +654,30 @@ static int pca955x_probe(struct i2c_client *client)
650654
return err;
651655

652656
#ifdef CONFIG_LEDS_PCA955X_GPIO
653-
if (ngpios) {
654-
pca955x->gpio.label = "gpio-pca955x";
655-
pca955x->gpio.direction_input = pca955x_gpio_direction_input;
656-
pca955x->gpio.direction_output = pca955x_gpio_direction_output;
657-
pca955x->gpio.set = pca955x_gpio_set_value;
658-
pca955x->gpio.get = pca955x_gpio_get_value;
659-
pca955x->gpio.request = pca955x_gpio_request_pin;
660-
pca955x->gpio.can_sleep = 1;
661-
pca955x->gpio.base = -1;
662-
pca955x->gpio.ngpio = ngpios;
663-
pca955x->gpio.parent = &client->dev;
664-
pca955x->gpio.owner = THIS_MODULE;
665-
666-
err = devm_gpiochip_add_data(&client->dev, &pca955x->gpio,
667-
pca955x);
668-
if (err) {
669-
/* Use data->gpio.dev as a flag for freeing gpiochip */
670-
pca955x->gpio.parent = NULL;
671-
dev_warn(&client->dev, "could not add gpiochip\n");
672-
return err;
673-
}
674-
dev_info(&client->dev, "gpios %i...%i\n",
675-
pca955x->gpio.base, pca955x->gpio.base +
676-
pca955x->gpio.ngpio - 1);
657+
pca955x->gpio.label = "gpio-pca955x";
658+
pca955x->gpio.direction_input = pca955x_gpio_direction_input;
659+
pca955x->gpio.direction_output = pca955x_gpio_direction_output;
660+
pca955x->gpio.set = pca955x_gpio_set_value;
661+
pca955x->gpio.get = pca955x_gpio_get_value;
662+
pca955x->gpio.request = pca955x_gpio_request_pin;
663+
pca955x->gpio.free = pca955x_gpio_free_pin;
664+
pca955x->gpio.can_sleep = 1;
665+
pca955x->gpio.base = -1;
666+
pca955x->gpio.ngpio = chip->bits;
667+
pca955x->gpio.parent = &client->dev;
668+
pca955x->gpio.owner = THIS_MODULE;
669+
670+
err = devm_gpiochip_add_data(&client->dev, &pca955x->gpio,
671+
pca955x);
672+
if (err) {
673+
/* Use data->gpio.dev as a flag for freeing gpiochip */
674+
pca955x->gpio.parent = NULL;
675+
dev_warn(&client->dev, "could not add gpiochip\n");
676+
return err;
677677
}
678+
dev_info(&client->dev, "gpios %i...%i\n",
679+
pca955x->gpio.base, pca955x->gpio.base +
680+
pca955x->gpio.ngpio - 1);
678681
#endif
679682

680683
return 0;

0 commit comments

Comments
 (0)