Re: [PATCH v7 RESEND 3/4] leds: pca955x: Optimize probe led selection

[Lee Jones <lee@kernel.org>]

On Thu, 30 Jan 2025, Eddie James wrote:

> Previously, the probe function might do up to 32 reads and writes
> to the same 4 registers to program the led selection. Reduce this to
> a maximum of 5 operations by accumulating the changes to the led
> selection and comparing with the previous value to write the
> selection if different.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> ---
>  drivers/leds/leds-pca955x.c | 38 +++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
> index 4990f8aff6d16..8bdebc14ea2e6 100644
> --- a/drivers/leds/leds-pca955x.c
> +++ b/drivers/leds/leds-pca955x.c
> @@ -446,7 +446,9 @@ static int pca955x_probe(struct i2c_client *client)
>  	struct led_classdev *led;
>  	struct led_init_data init_data;
>  	struct i2c_adapter *adapter;
> -	int i, err;
> +	int i, bit, err, nls, reg;
> +	u8 ls1[4];
> +	u8 ls2[4];
>  	struct pca955x_platform_data *pdata;
>  	bool set_default_label = false;
>  	bool keep_pwm = false;
> @@ -504,6 +506,17 @@ static int pca955x_probe(struct i2c_client *client)
>  	init_data.devname_mandatory = false;
>  	init_data.devicename = "pca955x";
>  
> +	nls = pca955x_num_led_regs(chip->bits);
> +	/* use auto-increment feature to read all the led selectors at once */

Sentences / comment should start with an upper-case char.

And acronyms should be capitalised too, so it's "LED".

> +	err = i2c_smbus_read_i2c_block_data(client,
> +					    0x10 | (pca955x_num_input_regs(chip->bits) + 4), nls,
> +					    ls1);
> +	if (err < 0)
> +		return err;
> +
> +	for (i = 0; i < nls; ++i)

Why the need to pre-increment?  Can you rejig this to be more consistent
with the way we usually format for loops please?

> +		ls2[i] = ls1[i];
> +
>  	for (i = 0; i < chip->bits; i++) {
>  		pca955x_led = &pca955x->leds[i];
>  		pca955x_led->led_num = i;
> @@ -515,19 +528,16 @@ static int pca955x_probe(struct i2c_client *client)
>  		case PCA955X_TYPE_GPIO:
>  			break;
>  		case PCA955X_TYPE_LED:
> +			bit = i % 4;
> +			reg = i / 4;
>  			led = &pca955x_led->led_cdev;
>  			led->brightness_set_blocking = pca955x_led_set;
>  			led->brightness_get = pca955x_led_get;
>  
> -			if (pdata->leds[i].default_state == LEDS_DEFSTATE_OFF) {
> -				err = pca955x_led_set(led, LED_OFF);
> -				if (err)
> -					return err;
> -			} else if (pdata->leds[i].default_state == LEDS_DEFSTATE_ON) {
> -				err = pca955x_led_set(led, LED_FULL);
> -				if (err)
> -					return err;
> -			}
> +			if (pdata->leds[i].default_state == LEDS_DEFSTATE_OFF)
> +				ls2[reg] = pca955x_ledsel(ls2[reg], bit, PCA955X_LS_LED_OFF);
> +			else if (pdata->leds[i].default_state == LEDS_DEFSTATE_ON)
> +				ls2[reg] = pca955x_ledsel(ls2[reg], bit, PCA955X_LS_LED_ON);
>  
>  			init_data.fwnode = pdata->leds[i].fwnode;
>  
> @@ -571,6 +581,14 @@ static int pca955x_probe(struct i2c_client *client)
>  		}
>  	}
>  
> +	for (i = 0; i < nls; ++i) {

As above.

> +		if (ls1[i] != ls2[i]) {
> +			err = pca955x_write_ls(pca955x, i, ls2[i]);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
>  	/* PWM0 is used for half brightness or 50% duty cycle */
>  	err = pca955x_write_pwm(pca955x, 0, 255 - LED_HALF);
>  	if (err)
> -- 
> 2.43.5
> 

-- 
Lee Jones [李琼斯]