[PATCH linux dev-4.10 2/2] leds: pca955x: check for I2C errors
Andrew Jeffery
andrew at aj.id.au
Mon Aug 28 12:40:51 AEST 2017
Hi Cédric,
On Sun, 2017-08-27 at 18:45 +0200, Cédric Le Goater wrote:
> This should also allow probing to fail when a pca955x chip is not
> found on a I2C bus.
>
> > Signed-off-by: Cédric Le Goater <clg at kaod.org>
> ---
> drivers/leds/leds-pca955x.c | 93 +++++++++++++++++++++++++++++++++++----------
> 1 file changed, 72 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index f062d1e7640f..5f12d55550a4 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -165,13 +165,18 @@ static inline u8 pca955x_ledsel(u8 oldval, int led_num, int state)
> * Write to frequency prescaler register, used to program the
> * period of the PWM output. period = (PSCx + 1) / 38
> */
> -static void pca955x_write_psc(struct i2c_client *client, int n, u8 val)
> +static int pca955x_write_psc(struct i2c_client *client, int n, u8 val)
> {
> > struct pca955x *pca955x = i2c_get_clientdata(client);
> > + int ret;
>
> > - i2c_smbus_write_byte_data(client,
> > + ret = i2c_smbus_write_byte_data(client,
> > pca95xx_num_input_regs(pca955x->chipdef->bits) + 2*n,
> > val);
> > + if (ret < 0)
> > + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
> > + __func__, n, val, ret);
> > + return ret;
> }
>
> /*
> @@ -181,38 +186,57 @@ static void pca955x_write_psc(struct i2c_client *client, int n, u8 val)
> *
> * Duty cycle is (256 - PWMx) / 256
> */
> -static void pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
> +static int pca955x_write_pwm(struct i2c_client *client, int n, u8 val)
> {
> > struct pca955x *pca955x = i2c_get_clientdata(client);
> > + int ret;
>
> > - i2c_smbus_write_byte_data(client,
> > + ret = i2c_smbus_write_byte_data(client,
> > pca95xx_num_input_regs(pca955x->chipdef->bits) + 1 + 2*n,
> > val);
> > + if (ret < 0)
> > + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
> > + __func__, n, val, ret);
> > + return ret;
> }
>
> +
> /*
> * Write to LED selector register, which determines the source that
> * drives the LED output.
> */
> -static void pca955x_write_ls(struct i2c_client *client, int n, u8 val)
> +static int pca955x_write_ls(struct i2c_client *client, int n, u8 val)
> {
> > struct pca955x *pca955x = i2c_get_clientdata(client);
> > + int ret;
>
> > - i2c_smbus_write_byte_data(client,
> > + ret = i2c_smbus_write_byte_data(client,
> > pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n,
> > val);
> > + if (ret < 0)
> > + dev_err(&client->dev, "%s: reg 0x%x, val 0x%x, err %d\n",
> > + __func__, n, val, ret);
> > + return ret;
> }
>
> /*
> * Read the LED selector register, which determines the source that
> * drives the LED output.
> */
> -static u8 pca955x_read_ls(struct i2c_client *client, int n)
> +static int pca955x_read_ls(struct i2c_client *client, int n, u8 *val)
> {
> > struct pca955x *pca955x = i2c_get_clientdata(client);
> > + int ret;
>
> > - return (u8) i2c_smbus_read_byte_data(client,
> > + ret = i2c_smbus_read_byte_data(client,
> > pca95xx_num_input_regs(pca955x->chipdef->bits) + 4 + n);
> > + if (ret < 0) {
> > + dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
> > + __func__, n, ret);
> > + return ret;
> > + }
> > + *val = (u8) ret;
> > + return 0;
> }
>
> static int pca955x_led_set(struct led_classdev *led_cdev,
> @@ -223,6 +247,7 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
> > u8 ls;
> > > int chip_ls; /* which LSx to use (0-3 potentially) */
> > > int ls_led; /* which set of bits within LSx to use (0-3) */
> > + int ret;
>
> > pca955x_led = container_of(led_cdev, struct pca955x_led, led_cdev);
> > pca955x = pca955x_led->pca955x;
> @@ -232,7 +257,9 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
>
> > mutex_lock(&pca955x->lock);
>
> > - ls = pca955x_read_ls(pca955x->client, chip_ls);
> > + ret = pca955x_read_ls(pca955x->client, chip_ls, &ls);
> > + if (ret)
> > + goto out;
>
> > switch (value) {
> > case LED_FULL:
> @@ -252,26 +279,37 @@ static int pca955x_led_set(struct led_classdev *led_cdev,
> > * OFF, HALF, or FULL. But, this is probably better than
> > * just turning off for all other values.
> > */
> > - pca955x_write_pwm(pca955x->client, 1,
> > - 255 - value);
> > + ret = pca955x_write_pwm(pca955x->client, 1, 255 - value);
> > + if (ret)
> > + goto out;
> > ls = pca955x_ledsel(ls, ls_led, PCA955X_LS_BLINK1);
> > break;
> > }
>
> > - pca955x_write_ls(pca955x->client, chip_ls, ls);
> > + ret = pca955x_write_ls(pca955x->client, chip_ls, ls);
>
> +out:
> > mutex_unlock(&pca955x->lock);
>
> > - return 0;
> > + return ret;
> }
>
> #ifdef CONFIG_LEDS_PCA955X_GPIO
> /*
> * Read the INPUT register, which contains the state of LEDs.
> */
> -static u8 pca955x_read_input(struct i2c_client *client, int n)
> +static int pca955x_read_input(struct i2c_client *client, int n, u8 *val)
> {
> > - return (u8)i2c_smbus_read_byte_data(client, n);
> > + int ret = i2c_smbus_read_byte_data(client, n);
> +
> > + if (ret < 0) {
> > + dev_err(&client->dev, "%s: reg 0x%x, err %d\n",
> > + __func__, n, ret);
> > + return ret;
> > + }
> > + *val = (u8) ret;
> > + return 0;
> +
> }
>
> static int pca955x_gpio_request_pin(struct gpio_chip *gc, unsigned int offset)
> @@ -301,7 +339,10 @@ static int pca955x_gpio_get_value(struct gpio_chip *gc, unsigned int offset)
I don't see a corresponding change to pca955x_gpio_set_value(), though given
it's hooked up as the gpiochip set() callback we would have to reimplement that
interface. We should do that and return the error code rather than swallow it.
This flows on to pca955x_gpio_direction_input() and
pca955x_gpio_direction_output() where we currently just blindly return 0.
I think that's it though, looks good otherwise.
Andrew
> {
> > struct pca955x *pca955x = gpiochip_get_data(gc);
> > struct pca955x_led *led = &pca955x->leds[offset];
> > - u8 reg = pca955x_read_input(pca955x->client, led->led_num / 8);
> > + u8 reg = 0;
> +
> > + /* There is nothing we can do about errors */
> > + pca955x_read_input(pca955x->client, led->led_num / 8, ®);
>
> > return !!(reg & (1 << (led->led_num % 8)));
> }
> @@ -491,19 +532,29 @@ static int pca955x_probe(struct i2c_client *client,
> > return err;
>
> > /* Turn off LED */
> > - pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
> > + err = pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);
> > + if (err)
> > + return err;
> > }
> > }
>
> > /* PWM0 is used for half brightness or 50% duty cycle */
> > - pca955x_write_pwm(client, 0, 255-LED_HALF);
> > + err = pca955x_write_pwm(client, 0, 255-LED_HALF);
> > + if (err)
> > + return err;
>
> > /* PWM1 is used for variable brightness, default to OFF */
> > - pca955x_write_pwm(client, 1, 0);
> > + err = pca955x_write_pwm(client, 1, 0);
> > + if (err)
> > + return err;
>
> > /* Set to fast frequency so we do not see flashing */
> > - pca955x_write_psc(client, 0, 0);
> > - pca955x_write_psc(client, 1, 0);
> > + err = pca955x_write_psc(client, 0, 0);
> > + if (err)
> > + return err;
> > + err = pca955x_write_psc(client, 1, 0);
> > + if (err)
> > + return err;
>
> #ifdef CONFIG_LEDS_PCA955X_GPIO
> > if (ngpios) {
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170828/8cc277ea/attachment.sig>
More information about the openbmc
mailing list