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

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Thierry,

Thanks for the update.

One question regarding the set: is
Documentation/process/coding-assistants.rst relevant for this?

On Sat, Apr 25, 2026 at 09:31:42AM -0700, 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@gmail.com>
> ---
>  drivers/platform/x86/intel/int3472/tps68470.c | 113 +++++++++++-------
>  drivers/platform/x86/intel/int3472/tps68470.h |  10 ++
>  2 files changed, 79 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.c b/drivers/platform/x86/intel/int3472/tps68470.c
> index a496075c0..92da2ba80 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470.c
> @@ -147,17 +147,41 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  	struct tps68470_clk_platform_data *clk_pdata;
>  	struct mfd_cell *cells;
>  	struct regmap *regmap;
> +	struct gpiod_lookup_table * const *tables;
>  	int n_consumers;
>  	int device_type;
>  	int ret;
> -	int i;
> +	unsigned int i;
>  
>  	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 < 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;
> +	}

board_data is only relevant for the Windows case and is tested after the
switch anyway. Please move this chunk after the switch.

>  
>  	regmap = devm_regmap_init_i2c(client, &tps68470_regmap_config);
>  	if (IS_ERR(regmap)) {
> @@ -176,52 +200,53 @@ 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");
> -
> -		cells = kzalloc_objs(*cells, TPS68470_WIN_MFD_CELL_COUNT);
> -		if (!cells)
> -			return -ENOMEM;
> -
> -		/*
> -		 * The order of the cells matters here! The clk must be first
> -		 * because the regulator depends on it. The gpios must be last,
> -		 * acpi_gpiochip_add() calls acpi_dev_clear_dependencies() and
> -		 * the clk + regulators must be ready when this happens.
> -		 */
> -		cells[0].name = "tps68470-clk";
> -		cells[0].platform_data = clk_pdata;
> -		cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
> -		cells[1].name = "tps68470-regulator";
> -		cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
> -		cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
> -		cells[2].name = "tps68470-gpio";
> -
> -		for (i = 0; i < board_data->n_gpiod_lookups; i++)
> -			gpiod_add_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
> -
> -		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> -					   cells, TPS68470_WIN_MFD_CELL_COUNT,
> -					   NULL, 0, NULL);
> -		kfree(cells);
> -
> -		if (ret) {
> -			for (i = 0; i < board_data->n_gpiod_lookups; i++)
> -				gpiod_remove_lookup_table(board_data->tps68470_gpio_lookup_tables[i]);
> -		}
> -
>  		break;
>  	case DESIGNED_FOR_CHROMEOS:
> -		ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> -					   tps68470_cros, ARRAY_SIZE(tps68470_cros),
> -					   NULL, 0, NULL);
> -		break;
> +		return devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> +					    tps68470_cros,
> +					    ARRAY_SIZE(tps68470_cros),
> +					    NULL, 0, NULL);
>  	default:
>  		dev_err(&client->dev, "Failed to add MFD devices\n");
>  		return device_type;
>  	}
>  
> +	if (!board_data)
> +		return dev_err_probe(&client->dev, -ENODEV,
> +				     "No board-data found for this model\n");
> +
> +	cells = kzalloc_objs(*cells, TPS68470_WIN_MFD_CELL_COUNT);
> +	if (!cells)
> +		return -ENOMEM;
> +
> +	/*
> +	 * The order of the cells matters here! The clk must be first
> +	 * because the regulator depends on it. The gpios must be last,
> +	 * acpi_gpiochip_add() calls acpi_dev_clear_dependencies() and
> +	 * the clk + regulators must be ready when this happens.
> +	 */
> +	cells[0].name = "tps68470-clk";
> +	cells[0].platform_data = clk_pdata;
> +	cells[0].pdata_size = struct_size(clk_pdata, consumers, n_consumers);
> +	cells[1].name = "tps68470-regulator";
> +	cells[1].platform_data = (void *)board_data->tps68470_regulator_pdata;
> +	cells[1].pdata_size = sizeof(struct tps68470_regulator_platform_data);
> +	cells[2].name = "tps68470-gpio";
> +
> +	tables = board_data->tps68470_gpio_lookup_tables;
> +	for (i = 0; i < board_data->n_gpiod_lookups; i++)
> +		gpiod_add_lookup_table(tables[i]);
> +
> +	ret = devm_mfd_add_devices(&client->dev, PLATFORM_DEVID_NONE,
> +				   cells, TPS68470_WIN_MFD_CELL_COUNT,
> +				   NULL, 0, NULL);
> +	kfree(cells);
> +
> +	if (ret) {
> +		for (i = 0; i < board_data->n_gpiod_lookups; i++)
> +			gpiod_remove_lookup_table(tables[i]);
> +	}
> +
>  	/*
>  	 * No acpi_dev_clear_dependencies() here, since the acpi_gpiochip_add()
>  	 * for the GPIO cell already does this.
> @@ -233,7 +258,7 @@ static int skl_int3472_tps68470_probe(struct i2c_client *client)
>  static void skl_int3472_tps68470_remove(struct i2c_client *client)
>  {
>  	const struct int3472_tps68470_board_data *board_data;
> -	int i;
> +	unsigned int i;
>  
>  	board_data = int3472_tps68470_get_board_data(dev_name(&client->dev));
>  	if (board_data) {
> diff --git a/drivers/platform/x86/intel/int3472/tps68470.h b/drivers/platform/x86/intel/int3472/tps68470.h
> index 35915e701..1d3d67459 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[];
>  };

-- 
Regards,

Sakari Ailus