Hi, On 11/25/22 11:58, Laurent Pinchart wrote: > On Fri, Nov 25, 2022 at 10:17:17AM +0000, 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. > > The whole point of a privacy LED is to be controlled automatically (and > ideally without software intervention, but that's a different story). > Can the LED framework be used without having the LED exposed to > userspace ? AFAIK using the LED framework will automatically expose the LED to userspace; and using triggers as I mentioned in my other email will also allow the user to unset the trigger or even use a different trigger. I understand where you are coming from, but I was actually seeing this (exposed to userspace) as a feature. Users may want to repurpose the LED, maybe make it blink when the camera is on for extra obviousness the camera is on. Maybe always have it off because it is too annoying, etc... ? My vision here is that ideally the LED should be hardwired to go on together with some enable pin or power-supply of the sensor. But if it is actually just a GPIO, then there is something to be said for giving the user full-control. OTOH this would make writing spy-ware where the LED never goes on a lot easier... Typing this out I'm afraid that I have to agree with you and that the spyware argument likely wins over how giving the user more control would be nice :( Which would bring us back to just making it a GPIO, which would then need to be turned on+off by the sensor driver I guess. There seems to be a bunch of GPIO/clk/regulator boilerplate duplicated in all the sensor drivers. I think a little helper-library for this might be in order. E.g. Something like this (in the .h file) struct camera_sensor_pwr_helper { // bunch of stuff here, this should be fixed size so that the // sensor drivers can embed it into their driver-data struct }; int camera_sensor_pwr_helper_init(struct camera_sensor_pwr_helper *helper, const char *supply_names, int supply_count, const char* clk_name. /* other stuff which I'm probably forgetting right now */); // turn_on_privacy_led should be false when called from probe(), must be true when // called on stream_on(). int camera_sensor_pwr_helper_on(struct camera_sensor_pwr_helper *helper, bool turn_on_privacy_led); int camera_sensor_pwr_helper_off(struct camera_sensor_pwr_helper *helper); // maybe, or make everything devm managed? : int camera_sensor_pwr_helper_exit(struct camera_sensor_pwr_helper *helper); Just is just a really really quick n dirty design. For one I could use suggestions for a better name for the thing :) I think something like this will be helpfull to reduce a whole bunch of boilerplate code related to powering on/off the sensor in all the drivers; and it would give us a central place to drive an (optional) privacy-led GPIO. Regards, Hans > >>> And likewise (eventually) completely drop the "clken" GPIO this >>> patch series introduces (with some sensors) and instead always model >>> this through the clk-framework. >>> >>> Regards, >>> >>> Hans >>> >>> >>> Hans de Goede (3): >>> platform/x86: int3472/discrete: Refactor GPIO to sensor mapping >>> platform/x86: int3472/discrete: Get the polarity from the _DSM entry >>> platform/x86: int3472/discrete: Add support for sensor-drivers which >>> expect clken + pled GPIOs >>> >>> drivers/platform/x86/intel/int3472/common.h | 2 +- >>> drivers/platform/x86/intel/int3472/discrete.c | 92 ++++++++++++++++--- >>> 2 files changed, 78 insertions(+), 16 deletions(-) >
