Re: [PATCH 4/8] platform/x86: int3472: Add tps68470-led as clock consumer

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

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> Some of the LEDs provided by the TPS68470 require the clock that it
> provides to be active in order to function. Add the platform driver
> for the leds as a consumer of the clock so that the led driver can
> discover it during .probe()
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 21 ++++++++++++-------
>  1 file changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index 53b0459f278a..818f2fc5bf2a 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -105,25 +105,30 @@ skl_int3472_fill_clk_pdata(struct device *dev, struct tps68470_clk_platform_data
>  {
>  	struct acpi_device *adev = ACPI_COMPANION(dev);
>  	struct acpi_device *consumer;
> -	unsigned int n_consumers = 0;
> +	unsigned int n_consumers = 1;
>  	const char *sensor_name;
> -	unsigned int i = 0;
> +	const char *led_name;
> +	unsigned int i = 1;

Nitpick: please just don't init i here at all.

>  
>  	for_each_acpi_consumer_dev(adev, consumer)
>  		n_consumers++;
>  
> -	if (!n_consumers) {
> -		dev_err(dev, "INT3472 seems to have no dependents\n");
> -		return -ENODEV;
> -	}
> -
>  	*clk_pdata = devm_kzalloc(dev, struct_size(*clk_pdata, consumers, n_consumers),
>  				  GFP_KERNEL);
>  	if (!*clk_pdata)
>  		return -ENOMEM;
>  
>  	(*clk_pdata)->n_consumers = n_consumers;
> -	i = 0;
> +
> +	/*
> +	 * The TPS68470 includes an LED driver which requires the clock be active
> +	 * to function. Add the led platform device as a consumer of the clock.
> +	 */
> +	led_name = devm_kstrdup(dev, "tps68470-led", GFP_KERNEL);
> +	if (!led_name)
> +		return -ENOMEM;
> +
> +	(*clk_pdata)->consumers[0].consumer_dev_name = led_name;
>  

And add a:

	i = 1;

here, to make it clear that after setting consumers[0] we now
start adding more consumers at index 1.

>  	for_each_acpi_consumer_dev(adev, consumer) {
>  		sensor_name = devm_kasprintf(dev, GFP_KERNEL, I2C_DEV_NAME_FORMAT,

With that fixed:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans