Re: [PATCH 0/3] platform/x86: int3472/discrete: Make it work with IPU6

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

Hi,

On 11/25/22 11:17, Dan Scally wrote:
> Morning Hans - thanks for the set
> 
> On 24/11/2022 20:00, Hans de Goede wrote:
>> Hi All,
>>
>> Here is a small set of patches to make the int3472/discrete code
>> work with the sensor drivers bundled with the (unfortunately out of tree)
>> IPU6 driver.
>>
>> There are parts of the out of tree IPU6 code, like the sensor drivers,
>> which can be moved to the mainline and I do plan to work on this at some
>> point and then some of this might need to change. But for now the goal is
>> to make the out of tree driver work with standard mainline distro kernels
>> through e.g. dkms. Otherwise users need to run a patched kernel just for
>> a couple of small differences.
>>
>> This is basically a rewrite of this patch:
>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch
>>
>> Wich users who want to use the IPU6 driver so far have had to manually
>> apply to their kernels which is quite inconvenient.
>>
>> This rewrite makes 2 significant changes:
>>
>> 1. Don't break things on IPU3 platforms
>>
>> 2. Instead of extending the int3472_sensor_configs[] quirks table for each
>> model which needs "clken" and "pled" GPIOs, do this based on matching
>> the ACPI HID of the ACPI device describing the sensor.
>>
>> The need for these GPIOs is a property of the specific sensor driver which
>> binds using this same HID, so by using this we avoid having to extend the
>> int3472_sensor_configs[] quirks table all the time.
>>
>> This allows roling back the behavior to at least use a clk-framework
>> clk instead of clken GPIO on a per sensor(-driver) basis as we mainline
>> the sensor drivers, assuming that the drivers are switched over to the
>> clk framework as part of their mainlining.
>>
>> A bigger question is what to do with the privacy-led GPIO on IPU3
>> we so far have turned the LED on/off at the same as te clock,
>> but at least on some IPU6 models this won't work, because they only
>> have a privacy-led GPIO and no clk_en GPIO (there is no sensor
>> clk-control at all on some models).
> 
> 
> Ah how annoying, we hadn't come across any situations for IPU3 with a privacy LED but no clock GPIO
> 
>>
>> I think we should maybe move all models, including IPU3 based
>> models over to using a normal GPIO for controlling the privacy-led
>> to make things consistent.
> 
> 
> I think they probably should be represented as LED devices then, and have the media subsytem call some framework to find associated LEDs and cycle them at power on time in the sensor drivers. I know there's the v4l2_flash structure at the moment, but not sure if a privacy one exists.

That is actually a pretty good idea, the LED subsystem has the notion
of triggers, which are identified simply with a string.

So we could simple add a LED class device which sets it default trigger
to "camera-front" and then have either the sensor-drivers start-stream
or maybe even something more generic, so in the media subsystem code
somewhere set the led trigger.

See e.g. the LED class device in drivers/hid/hid-lenovo.c and how
it sets data->led_micmute.default_trigger = "audio-micmute",
combined with the drivers/leds/trigger/ledtrig-audio.c code,
where the ALSA kernel code just does, e.g.:

ledtrig_audio_set(LED_AUDIO_MUTE, 1);
or:
ledtrig_audio_set(LED_AUDIO_MUTE, 0);

We would then probably need to do something like rename
the drivers/leds/trigger/ledtrig-audio.c code and extend the
enum led_audio type with front + back cameras. That or copy the
code, but just renaming it seems better then adding a copy.

And then all need is to have something call:

ledtrig_multimedia_set(LED_CAMERA_FRONT, 0);
ledtrig_multimedia_set(LED_CAMERA_FRONT, 1);

And we are done.

I think the biggest question here is where to put those
ledtrig_multimedia_set() calls ?

These calls will be no-ops when no LED has its trigger
set to "camera-front", so there is no need to worry
about making them conditional (other then them being
conditional (or stubbed out?) when the LED subsystem
is disabled).

Note I would like to move forward with this patch set as is,
to unblock distros wanting to package the out of tree
IPU6 driver for now and then we can convert things to this
model later.

Please let me know if there are any objections with going
with this patch-set as an intermediate solution.

Regards,

Hans