Re: [PATCH 1/8] platform/x86: int3472: Add platform data for LEDs

[Hans de Goede <hdegoede@redhat.com>]

Hi Daniel,

Thank you for your work in this!

On 3/22/23 17:09, Daniel Scally wrote:
> Some of the LEDs that can be provided by the TPS68470 PMIC come with
> various configuration registers that must be set to appropriate values.
> Add a platform data struct so that those data can be defined and
> passed to the tps68470-led platform device.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c |  2 ++
>  drivers/platform/x86/intel/int3472/tps68470.h |  2 ++
>  include/linux/platform_data/tps68470.h        | 11 +++++++++++
>  3 files changed, 15 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 82ef022f8916..53b0459f278a 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -194,6 +194,8 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  		cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
>  		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
>  		cells[2].name = "tps68470-led";
> +		cells[2].platform_data = (void *)board_data->tps68470_led_pdata;
> +		cells[2].pdata_size = sizeof(struct tps68470_led_platform_data);
>  		cells[3].name = "tps68470-gpio";
>  
>  		for (i = 0; i < board_data->n_gpiod_lookups; i++)
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
> index 35915e701593..ce50687db6fb 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.h
> +++ b/drivers/platform/x86/intel/int3472/tps68470.h
> @@ -13,10 +13,12 @@
>  
>  struct gpiod_lookup_table;
>  struct tps68470_regulator_platform_data;
> +struct tps68470_led_platform_data;
>  
>  struct int3472_tps68470_board_data {
>  	const char *dev_name;
>  	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
> +	const struct tps68470_led_platform_data *tps68470_led_pdata;
>  	unsigned int n_gpiod_lookups;
>  	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
>  };


> diff --git a/include/linux/platform_data/tps68470.h b/include/linux/platform_data/tps68470.h
> index e605a2cab07f..5d55ad5c17ed 100644
> --- a/include/linux/platform_data/tps68470.h
> +++ b/include/linux/platform_data/tps68470.h
> @@ -37,4 +37,15 @@ struct tps68470_clk_platform_data {
>  	struct tps68470_clk_consumer consumers[];
>  };
>  
> +struct tps68470_led_platform_data {
> +	u8 iledctl_ctrlb;
> +	u8 wledmaxf;
> +	u8 wledto;
> +	u8 wledc1;
> +	u8 wledc2;
> +	u8 wledctl_mode;
> +	bool wledctl_disled1;
> +	bool wledctl_disled2;
> +};
> +
>  #endif


It seems to me that these include/linux/platform_data/tps68470.h
changes should be squashed to:

[PATCH 2/8] platform/x86: int3472: Init LED registers using platform data

Which BTW has the wrong subsys prefix, it should be named:

[PATCH 2/8] leds: tps68470: Init LED registers using platform data

or even better:

[PATCH 2/8] leds: tps68470: Init WLED registers using platform data


And then squash the rest of this patch into:

[PATCH 3/8] platform/x86: int3472: Add TPS68470 LED Board Data

please and drop this now empty patch from the set.

Regards,

Hans