| Message ID | 20230322160926.948687-9-dan.scally@ideasonboard.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add WLED support to TPS68470 LED driver | expand |
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
Hi Hans On 22/03/2023 17:34, Hans de Goede wrote: > 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 ? Oh interesting; on IPU3 devices with the discrete INT3472 the IR cameras don't seem to have an LED GPIO in _DSM so we're not sure how to turn them on yet. I also have a Pro7 which is an IPU4 device, but that has the same problem as on the IPU3 ones - there's no privacy-led GPIO defined in _DSM and the _ON method for the camera's _PR0 resource just prints "PR Not Supported"...so we don't know how to use those yet. So interesting that the IPU6 ones work differently. > > Regardless I think it would be best to turn on the front privacy LED when the IR camera is used too, right ? That does make a certain amount of sense yes - My only thought would be that this would be difficult to replicate to platforms that use _only_ discrete versions of the INT3472, because each sensor depends on a separate INT3472, so there wouldn't be an obvious way to automatically assign the privacy LED for the user facing camera to the IR camera since we couldn't use the board data method below. It might be surmountable using the location information in DSDT to decide whether it's on the same face as another camera which DOES have a privacy LED maybe... > > IMHO this should look like this (with either the media-core or the driver consuming "ir-led"): The general principle of moving the IR led away from being treated as a privacy LED is ok by me - I'll work on that. > > 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 > >
Hi, On 3/23/23 11:31, Dan Scally wrote: > Hi Hans > > On 22/03/2023 17:34, Hans de Goede wrote: >> 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 ? > > > Oh interesting; on IPU3 devices with the discrete INT3472 the IR cameras don't seem to have an LED GPIO in _DSM so we're not sure how to turn them on yet. I also have a Pro7 which is an IPU4 device, but that has the same problem as on the IPU3 ones - there's no privacy-led GPIO defined in _DSM and the _ON method for the camera's _PR0 resource just prints "PR Not Supported"...so we don't know how to use those yet. So interesting that the IPU6 ones work differently. > > >> >> Regardless I think it would be best to turn on the front privacy LED when the IR camera is used too, right ? > > > That does make a certain amount of sense yes - My only thought would be that this would be difficult to replicate to platforms that use _only_ discrete versions of the INT3472, because each sensor depends on a separate INT3472, so there wouldn't be an obvious way to automatically assign the privacy LED for the user facing camera to the IR camera since we couldn't use the board data method below. It might be surmountable using the location information in DSDT to decide whether it's on the same face as another camera which DOES have a privacy LED maybe... Right, I realize turning on the front privacy LED when the front ir-sensor is on is not always going to be (easily) doable. But IMHO we should at least do it on platforms where we can do this. >> IMHO this should look like this (with either the media-core or the driver consuming "ir-led"): > > > The general principle of moving the IR led away from being treated as a privacy LED is ok by me - I'll work on that. Thanks. Regards, Hans > >> >> 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 >> >> >
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", + }, + }, +}; + static const struct int3472_tps68470_board_data surface_go_tps68470_board_data = { .dev_name = "i2c-INT3472:05", .tps68470_regulator_pdata = &surface_go_tps68470_pdata, .tps68470_led_pdata = &surface_go_tps68470_led_pdata, + .led_lookups = &surface_go_tps68470_led_lookups, .n_gpiod_lookups = 2, .tps68470_gpio_lookup_tables = { &surface_go_int347a_gpios, @@ -172,6 +189,7 @@ static const struct int3472_tps68470_board_data surface_go3_tps68470_board_data .dev_name = "i2c-INT3472:01", .tps68470_regulator_pdata = &surface_go_tps68470_pdata, .tps68470_led_pdata = &surface_go_tps68470_led_pdata, + .led_lookups = &surface_go_tps68470_led_lookups, .n_gpiod_lookups = 2, .tps68470_gpio_lookup_tables = { &surface_go_int347a_gpios,
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(+)