Re: [PATCH 8/8] platform/x86: int3472: Define LED lookup data for MS Surface Go

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

Hi,

On 3/22/23 17:09, Daniel Scally wrote:
> Add LED lookup data to tps68470_board_data.c for the Microsoft
> Surface Go line of devices.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  .../x86/intel/int3472/tps68470_board_data.c    | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> index 0d46a238b630..e2c53319e112 100644
> --- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> +++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
> @@ -157,10 +157,27 @@ static const struct tps68470_led_platform_data surface_go_tps68470_led_pdata = {
>  	.wledctl_disled2 = false,
>  };
>  
> +static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
> +	.n_lookups = 2,
> +	.lookup_table = {
> +		{
> +			.provider = "tps68470-iled_a::indicator",
> +			.dev_id = "i2c-INT347A:00",
> +			.con_id = "privacy-led",
> +		},
> +		{
> +			.provider = "tps68470-wled::indicator",
> +			.dev_id = "i2c-INT347E:00",
> +			.con_id = "privacy-led",
> +		},

So this feels wrong to me in 2 ways:

1. It is abusing .con_id = "privacy-led" to enable the WLED

2. You are not activating the front privacy LED for the IR projector. I have noticed on IPU6 devices that the _DSM listing GPIOs for the discrete INT3472 device lists a privacy-LED GPIO for the IR sensor too, which I so far have been guessing activates the actual privacy-LED. As I'm typing this I'm wondering if instead this is doing the same hack as you are doing here ?  

Regardless I think it would be best to turn on the front privacy LED when the IR camera is used too, right ?

IMHO this should look like this (with either the media-core or the driver consuming "ir-led"):

static struct tps68470_led_lookups surface_go_tps68470_led_lookups = {
	.n_lookups = 3,
	.lookup_table = {
		{
			.provider = "tps68470-iled_a::indicator",
			.dev_id = "i2c-INT347A:00",
			.con_id = "privacy-led",
		},
		{
			/* Use regular front-sensor privacy LED for ir-sensor too */
			.provider = "INT33BE_00::privacy_led",
			.dev_id = "i2c-INT347E:00",
			.con_id = "privacy-led",
		},
		{
			.provider = "tps68470-wled::indicator",
			.dev_id = "i2c-INT347E:00",
			.con_id = "ir-led",
		},
	}

Regards,

Hans