Re: [PATCH v2 1/2] platform/x86: int3472: Add board data for Intel nvl

[johannes . goede]

Hi,

On 1-Mar-26 15:37, Sakari Ailus wrote:
> Hi Arun,
> 
> Thanks for the update. Please avoid using --in-reply-to argument for git
> send-email when sending a new version of the patchset on the next time.
> 
> Could you cc me to my @linux.intel.com address on the next time?
> 
> Also cc other maintainers, see what
> 
> 	$ scripts/get_maintainer.pl drivers/platform/x86/intel/int3472/
> 
> prints.
> 
> On Fri, Feb 27, 2026 at 07:05:41PM +0530, Arun T wrote:
>> The Intel Nvl O13b10 sensor with the Intel IPU8 ISP.
>> The sensor is powered by a TPS68470 PMIC, and so we
>> need some board data to describe how to configure the GPIOs and
>> regulators to run the sensor.
> 
> This can be rewrapped and fits to three lines; also see
> Documentation/process/submitting-patches.rst . Most editors can do that
> without too much manual work.
> 
>>
>> Signed-off-by: Arun T <arun.t@xxxxxxxxx>
>> ---
>>  .../x86/intel/int3472/tps68470_board_data.c   | 150 ++++++++++++++++++
>>  1 file changed, 150 insertions(+)
>>
>> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>> index 71357a036292..71dc0940a94b 100644
>> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
>> @@ -143,6 +143,34 @@ static struct regulator_consumer_supply int3479_aux2_consumer_supplies[] = {
>>  	REGULATOR_SUPPLY("dovdd", "i2c-INT3479:00"),
>>  };
>>  
>> +/* Settings for Intel NVL platform */
>> +
>> +static struct regulator_consumer_supply int3472_core_consumer_supplies[] = {
>> +	REGULATOR_SUPPLY("dvdd", "i2c-OVTI13B1:00"),
>> +};
>> +
>> +static struct regulator_consumer_supply int3472_ana_consumer_supplies[] = {
>> +	REGULATOR_SUPPLY("ana", "i2c-OVTI13B1:00"),
>> +};
>> +
>> +static struct regulator_consumer_supply int3472_vcm_consumer_supplies[] = {
>> +	REGULATOR_SUPPLY("vdd", "i2c-OVTI13B1:00"),
>> +};
>> +
>> +static struct regulator_consumer_supply int3472_vsio_consumer_supplies[] = {
>> +	REGULATOR_SUPPLY("dovdd", "i2c-OVTI13B1:00"),
>> +	REGULATOR_SUPPLY("vsio", "i2c-OVTI13B1:00"),
>> +	REGULATOR_SUPPLY("vddd", "i2c-OVTI13B1:00"),
>> +};
>> +
>> +static struct regulator_consumer_supply int3472_aux1_consumer_supplies[] = {
>> +	REGULATOR_SUPPLY("vdda", "i2c-OVTI13B1:00"),
>> +};
>> +
>> +static struct regulator_consumer_supply int3472_aux2_consumer_supplies[] = {
>> +	REGULATOR_SUPPLY("vdddo", "i2c-OVTI13B1:00"),
>> +};
> 
> That's a lot of regulators. The sensor driver appears to use only three,
> even after the second patch.

Also the names still do not match up with the sensor driver, the sensor
driver (after patch 2/2) has:

	"dovdd",        /* Digital I/O power */
	"avdd",         /* Analog power */
	"dvdd",         /* Digital core power */

Where as above there is no "avdd" only "vdda" and "ana" which are
both candidates to actually be "avdd" I guess you want to change
the "ana" one but please double check.

Note all other regulator_consumer_supply maps except for
the 3 actually used by the sensor-driver are *wrong* and should
be dropped.

Even if there is a VCM, then the second part of the mapping
to "i2c-OVTI13B1:00" is wrong. The "i2c-OVTI13B1:00" consumer
will only consume "dovdd", "avdd" and "dvdd" supplies so
for that consumer there should only be those 3 maps.

Regards,

Hans




> 
> Is there a VCM there, for instance?
> 
>> +
>>  static const struct regulator_init_data dell_7212_tps68470_core_reg_init_data = {
>>  	.constraints = {
>>  		.min_uV = 1200000,
>> @@ -220,6 +248,82 @@ static const struct regulator_init_data dell_7212_tps68470_aux2_reg_init_data =
>>  	.consumer_supplies = int3479_aux2_consumer_supplies,
>>  };
>>  
>> +static const struct regulator_init_data intel_nvl_tps68470_core_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 1200000,
>> +		.max_uV = 1200000,
>> +		.apply_uV = true,
>> +		.always_on = true,
> 
> Setting always_on to true shouldn't be necessary here.
> 
>> +	},
>> +	.num_consumer_supplies = ARRAY_SIZE(int3472_core_consumer_supplies),
>> +	.consumer_supplies = int3472_core_consumer_supplies,
>> +};
>> +
>> +static const struct regulator_init_data intel_nvl_tps68470_ana_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 2815200,
>> +		.max_uV = 2815200,
>> +		.apply_uV = true,
>> +		.always_on = true,
>> +	},
>> +	.num_consumer_supplies = ARRAY_SIZE(int3472_ana_consumer_supplies),
>> +	.consumer_supplies = int3472_ana_consumer_supplies,
>> +};
>> +static const struct regulator_init_data intel_nvl_tps68470_vcm_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 2815200,
>> +		.max_uV = 2815200,
>> +		.apply_uV = true,
>> +		.always_on = true,
>> +	},
>> +	.num_consumer_supplies = ARRAY_SIZE(int3472_vcm_consumer_supplies),
>> +	.consumer_supplies = int3472_vcm_consumer_supplies,
>> +};
>> +
>> +/* Ensure the always-on VIO regulator has the same voltage as VSIO */
>> +static const struct regulator_init_data intel_nvl_tps68470_vio_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 1800600,
>> +		.max_uV = 1800600,
>> +		.apply_uV = true,
>> +		.always_on = true,
>> +	},
>> +};
>> +
>> +static const struct regulator_init_data intel_nvl_tps68470_vsio_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 1800600,
>> +		.max_uV = 1800600,
>> +		.apply_uV = true,
>> +		.always_on = true,
>> +	},
>> +	.num_consumer_supplies = ARRAY_SIZE(int3472_vsio_consumer_supplies),
>> +	.consumer_supplies = int3472_vsio_consumer_supplies,
>> +};
>> +
>> +static const struct regulator_init_data intel_nvl_tps68470_aux1_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 2815200,
>> +		.max_uV = 2815200,
>> +		.apply_uV = 1,
>> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
>> +	},
>> +	.num_consumer_supplies = ARRAY_SIZE(int3472_aux1_consumer_supplies),
>> +	.consumer_supplies = int3472_aux1_consumer_supplies,
>> +};
>> +
>> +static const struct regulator_init_data intel_nvl_tps68470_aux2_reg_init_data = {
>> +	.constraints = {
>> +		.min_uV = 1800600,
>> +		.max_uV = 1800600,
>> +		.apply_uV = 1,
>> +		.valid_ops_mask = REGULATOR_CHANGE_STATUS,
>> +	},
>> +	.num_consumer_supplies = ARRAY_SIZE(int3472_aux2_consumer_supplies),
>> +	.consumer_supplies = int3472_aux2_consumer_supplies,
>> +};
>> +
>> +
>>  static const struct tps68470_regulator_platform_data dell_7212_tps68470_pdata = {
>>  	.reg_init_data = {
>>  		[TPS68470_CORE] = &dell_7212_tps68470_core_reg_init_data,
>> @@ -232,6 +336,18 @@ static const struct tps68470_regulator_platform_data dell_7212_tps68470_pdata =
>>  	},
>>  };
>>  
>> +static const struct tps68470_regulator_platform_data intel_nvl_tps68470_pdata = {
>> +	.reg_init_data = {
>> +		[TPS68470_CORE] = &intel_nvl_tps68470_core_reg_init_data,
>> +		[TPS68470_ANA]  = &intel_nvl_tps68470_ana_reg_init_data,
>> +		[TPS68470_VCM]  = &intel_nvl_tps68470_vcm_reg_init_data,
>> +		[TPS68470_VIO] = &intel_nvl_tps68470_vio_reg_init_data,
>> +		[TPS68470_VSIO] = &intel_nvl_tps68470_vsio_reg_init_data,
>> +		[TPS68470_AUX1] = &intel_nvl_tps68470_aux1_reg_init_data,
>> +		[TPS68470_AUX2] = &intel_nvl_tps68470_aux2_reg_init_data,
>> +	},
>> +};
>> +
>>  static struct gpiod_lookup_table surface_go_int347a_gpios = {
>>  	.dev_id = "i2c-INT347A:00",
>>  	.table = {
>> @@ -258,6 +374,23 @@ static struct gpiod_lookup_table dell_7212_int3479_gpios = {
>>  	}
>>  };
>>  
>> +static struct gpiod_lookup_table intel_nvl_int347a_gpios = {
>> +	.dev_id = "i2c-OVTI13B1:01",
>> +	.table = {
>> +		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
>> +		GPIO_LOOKUP("tps68470-gpio", 8, "s_idle", GPIO_ACTIVE_LOW),
>> +		{ }
>> +	}
>> +};
>> +
>> +static struct gpiod_lookup_table intel_nvl_int347e_gpios = {
>> +	.dev_id = "i2c-OVTI13B1:01",
>> +	.table = {
>> +	GPIO_LOOKUP("tps68470-gpio", 7, "s_enable", GPIO_ACTIVE_LOW),
>> +		{ }
>> +	}
>> +};
>> +
>>  static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = {
>>  	.dev_name = "i2c-INT3472:05",
>>  	.tps68470_regulator_pdata = &surface_go_tps68470_pdata,
>> @@ -287,6 +420,16 @@ static const struct int3472_tps68470_board_data dell_7212_tps68470_board_data =
>>  	},
>>  };
>>  
>> +static const struct int3472_tps68470_board_data intel_nvl_tps68470_board_data = {
>> +	.dev_name = "i2c-INT3472:04",
>> +	.tps68470_regulator_pdata = &intel_nvl_tps68470_pdata,
>> +	.n_gpiod_lookups = 2,
>> +	.tps68470_gpio_lookup_tables = {
>> +		&intel_nvl_int347a_gpios,
>> +		&intel_nvl_int347e_gpios,
>> +	},
>> +};
>> +
>>  static const struct dmi_system_id int3472_tps68470_board_data_table[] = {
>>  	{
>>  		.matches = {
>> @@ -316,6 +459,13 @@ static const struct dmi_system_id int3472_tps68470_board_data_table[] = {
>>  		},
>>  		.driver_data = (void *)&dell_7212_tps68470_board_data,
>>  	},
>> +	{
>> +		.matches = {
>> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Intel Corporation"),
>> +			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Nova Lake Client Platform"),
>> +		},
>> +	       .driver_data = (void *)&intel_nvl_tps68470_board_data,
>> +	},
>>  	{ }
>>  };
>>  
>