Re: [PATCH 0/6] ov5693/int3472: Privacy LED handling changes + IPU6 compatibility

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

Hi All,

On 11/30/22 00:11, Hans de Goede wrote:
> Hi All,
> 
> The out of tree IPU6 driver has moved to using the in kernel INT3472
> code for doing power-ctrl rather then doing their own thing (good!).
> 
> Some of the IPU6 devices with a discrete INT3472 ACPI device have a
> privacy-led GPIO. but no clk-enable GPIO. To make this work this series
> moves the privacy LED control from being integrated with the clk-provider
> to modelling the privacy LED as a separate GPIO. This also brings the
> discrete INT3472 ACPI device privacy LED handling inline with the privacy
> LED handling for INT3472 TPS68470 PMIC devices which I posted here:
> 
> https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/
> 
> This obsoletes my previous "[PATCH 0/3] platform/x86: int3472/discrete:
> Make it work with IPU6" series:
> 
> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
> 
> Mauro since laptops with IPU6 cameras are becoming more and more
> popular I would like to get this merged for 6.2 so that with 6.2
> users will be able to build the out of tree IPU6 driver without
> requiring patching their main kernel. I realize we are a bit
> late in the cycle, but can you please still take the ov5693 patch
> for 6.2 ? It is quite small / straight-forward and since it used
> gpiod_get_optional() it is a no-op without the rest of this series.
> 
> This series has been tested on:
> 
> - Lenovo ThinkPad X1 Yoga gen 7, IPU6, front: ov2740 with privacy LED
> - Dell Latitude 9420, IPU 6 with privacy LED on front
> - Mirosoft Surface Go, IPU3, front: ov5693 with privacy LED,
>                               back: ov8865 with privacy LED

There has once again been push-back against the concept using
plain GPIOs for the privacy LED controls rather then wrapping
this in a LED class device. This time in the related series
adding support for the privacy LED on the back of Surface Go
devices:

https://lore.kernel.org/platform-driver-x86/20221128214408.165726-1-hdegoede@redhat.com/

Given all the comments / requests to use the LED class for this
I'm going to attempt to do that, see the above thread for some
challenges which I already encountered while exploring LED class
usage for this + proposed solution for those (adding a lookup
table mechanism to the LED class code similar to the existing
GPIO lookup table support).

This will result in a partial rewrite of this series, so self
NACK for this version of the series.

Andy this also means that I will not be using your new str_high_low()
helper function. The code which could use this will likely stay
around, but given that I need to do a rewrite and then get ne
reviews, it would IMHO be better to just get your series starting with:

[PATCH v1 1/3] lib/string_helpers: Add missing header files to MAINTAINERS database

upstream independently and then later my code can be moved over
to the helper (or if the helper lands first maybe use it from
day one), either way it seems best to decouple the merging
of these 2 series from each other.

Regards,

Hans












> Hans de Goede (6):
>   media: ov5693: Add support for a privacy-led GPIO
>   platform/x86: int3472/discrete: Refactor GPIO to sensor mapping
>   platform/x86: int3472/discrete: Treat privacy LED as regular GPIO
>   platform/x86: int3472/discrete: Move GPIO request to
>     skl_int3472_register_clock()
>   platform/x86: int3472/discrete: Ensure the clk/power enable pins are
>     in output mode
>   platform/x86: int3472/discrete: Get the polarity from the _DSM entry
> 
>  drivers/media/i2c/ov5693.c                    | 10 ++
>  .../x86/intel/int3472/clk_and_regulator.c     | 35 +++++--
>  drivers/platform/x86/intel/int3472/common.h   |  4 +-
>  drivers/platform/x86/intel/int3472/discrete.c | 95 ++++++++-----------
>  4 files changed, 80 insertions(+), 64 deletions(-)
>