Re: [PATCH v3 2/5] platform/x86: int3472: tps68470: fix clock consumer registration for Dell Latitude 5285

[Hans de Goede]

Hi,

On 17-Apr-26 18:32, Thierry Chatard wrote:
> The BIOS on the Dell Latitude 5285 leaves GNVS field C0TP at zero.
> With C0TP=0 the ACPI _DEP method on INT3479 (OV5670, front camera)
> resolves to PCI0 instead of the INT3472 (TPS68470 PMIC) device.
> 
> Because for_each_acpi_consumer_dev() walks the _DEP reverse-mapping,
> INT3479 is invisible to it: the clock consumer lookup entry for the
> front camera is never registered with the tps68470-clk driver, and
> the OV5670 sensor driver cannot acquire its MCLK.
> 
> Fix this without touching ACPI tables by adding optional static clock
> consumer fields to struct int3472_tps68470_board_data:
> 
>   unsigned int n_clk_consumers;
>   const struct tps68470_clk_consumer *clk_consumers;
> 
> When board data is present and n_clk_consumers is non-zero, probe uses
> the static list instead of for_each_acpi_consumer_dev() to populate
> tps68470-clk platform data.  Platforms that do not set these fields
> continue to use the existing ACPI traversal path unchanged.
> 
> The board_data lookup is moved before the clock-pdata allocation so
> that it is available for both the static and dynamic paths.
> 
> Signed-off-by: Thierry Chatard <tchatard@xxxxxxxxx>

Thank you, this is much better then the AML parsing + GNVS patching.

Patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@xxxxxxxxxxxxxxxx>

Regards,

Hans




> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 29 ++++++++++++++++---
>  drivers/platform/x86/intel/int3472/tps68470.h | 10 +++++++
>  2 files changed, 35 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index a496075c0..3364ef428 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -155,9 +155,31 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  	if (!adev)
>  		return -ENODEV;
>  
> -	n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
> -	if (n_consumers < 0)
> -		return n_consumers;
> +	/*
> +	 * Look up board data before building clock platform data.  On platforms
> +	 * where a sensor's ACPI _DEP does not list the INT3472 device,
> +	 * for_each_acpi_consumer_dev() misses that sensor and its clock consumer
> +	 * entry is never registered.  Board data can supply a static consumer
> +	 * list to use instead, bypassing the broken _DEP traversal.
> +	 */
> +	board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
> +
> +	if (board_data && board_data->n_clk_consumers) {
> +		clk_pdata = devm_kzalloc(&client->dev,
> +					 struct_size(clk_pdata, consumers,
> +						     board_data->n_clk_consumers),
> +					 GFP_KERNEL);
> +		if (!clk_pdata)
> +			return -ENOMEM;
> +		clk_pdata->n_consumers = board_data->n_clk_consumers;
> +		for (i = 0; i < (int)board_data->n_clk_consumers; i++)
> +			clk_pdata->consumers[i] = board_data->clk_consumers[i];
> +		n_consumers = board_data->n_clk_consumers;
> +	} else {
> +		n_consumers = skl_int3472_fill_clk_pdata(&client->dev, &clk_pdata);
> +		if (n_consumers < 0)
> +			return n_consumers;
> +	}
>  
>  	regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
>  	if (IS_ERR(regmap)) {
> @@ -176,7 +198,6 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  	device_type = skl_int3472_tps68470_calc_type(adev);
>  	switch (device_type) {
>  	case DESIGNED_FOR_WINDOWS:
> -		board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
>  		if (!board_data)
>  			return dev_err_probe(&client->dev, -ENODEV, "No board-data found for this model\n");
>  
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
> index 35915e701..4aefb728e 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.h
> +++ b/drivers/platform/x86/intel/int3472/tps68470.h
> @@ -12,11 +12,21 @@
>  #define _INTEL_SKL_INT3472_TPS68470_H
>  
>  struct gpiod_lookup_table;
> +struct tps68470_clk_consumer;
>  struct tps68470_regulator_platform_data;
>  
>  struct int3472_tps68470_board_data {
>  	const char *dev_name;
>  	const struct tps68470_regulator_platform_data *tps68470_regulator_pdata;
> +	/*
> +	 * Static clock consumers.  When n_clk_consumers is non-zero these are
> +	 * used in place of for_each_acpi_consumer_dev() to build the tps68470-clk
> +	 * platform data.  Needed on platforms where a sensor's ACPI _DEP does not
> +	 * list the INT3472 device, causing that sensor to be missed by the ACPI
> +	 * dependency traversal.
> +	 */
> +	unsigned int n_clk_consumers;
> +	const struct tps68470_clk_consumer *clk_consumers;
>  	unsigned int n_gpiod_lookups;
>  	struct gpiod_lookup_table *tps68470_gpio_lookup_tables[];
>  };