Message ID | 20221124200007.390901-1-hdegoede@redhat.com (mailing list archive) |
---|---|
Headers |
Received: from vger.kernel.org ([23.128.96.18]) by www.linuxtv.org with esmtp (Exim 4.92) (envelope-from <linux-media-owner@vger.kernel.org>) id 1oyIPO-00Bhr2-56; Thu, 24 Nov 2022 20:01:18 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229536AbiKXUBP (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Thu, 24 Nov 2022 15:01:15 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:60202 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229507AbiKXUBP (ORCPT <rfc822;linux-media@vger.kernel.org>); Thu, 24 Nov 2022 15:01:15 -0500 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0A7492197 for <linux-media@vger.kernel.org>; Thu, 24 Nov 2022 12:00:14 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1669320014; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding; bh=+7VjSpm55Uc6sFsUphFIMlvG+vQLARNqOyyl0AiFriE=; b=Whkpx0s21SRdz8F01EH3G1CkkjBzNLmDwKIoop65Lac+NqPEP5SZyrk4A/19uuShpsa1Mn mnHFiX5zfj9GV1qKiZinTxRi4XV3E+IGxZMoyAN6Yn90rzPAlPIC7I01pfjrseHPuGuXI3 6lXcyYkQe9Mz2Y0vaEX4xX38ZKqFNkI= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-615-FQ_ZYcUJNj6VEAVKbHnd3Q-1; Thu, 24 Nov 2022 15:00:12 -0500 X-MC-Unique: FQ_ZYcUJNj6VEAVKbHnd3Q-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 77040801585; Thu, 24 Nov 2022 20:00:12 +0000 (UTC) Received: from shalem.redhat.com (unknown [10.39.195.152]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8ACF01415114; Thu, 24 Nov 2022 20:00:10 +0000 (UTC) From: Hans de Goede <hdegoede@redhat.com> To: Mark Gross <markgross@kernel.org>, Andy Shevchenko <andy@kernel.org>, Daniel Scally <djrscally@gmail.com> Cc: Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org, Sakari Ailus <sakari.ailus@linux.intel.com>, Kate Hsuan <hpa@redhat.com>, linux-media@vger.kernel.org Subject: [PATCH 0/3] platform/x86: int3472/discrete: Make it work with IPU6 Date: Thu, 24 Nov 2022 21:00:04 +0100 Message-Id: <20221124200007.390901-1-hdegoede@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-LSpam-Score: -2.5 (--) X-LSpam-Report: No, score=-2.5 required=5.0 tests=BAYES_00=-1.9,DKIMWL_WL_HIGH=0.001,DKIM_SIGNED=0.1,DKIM_VALID=-0.1,DKIM_VALID_AU=-0.1,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1 autolearn=ham autolearn_force=no |
Series |
platform/x86: int3472/discrete: Make it work with IPU6
|
|
Message
Hans de Goede
Nov. 24, 2022, 8 p.m. UTC
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). 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. 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(-)
Comments
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. > > 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(-) >
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 ? > > 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(-)
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
On 25/11/2022 10: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 ? I think that's what led_sysfs_disable() is for, though I haven't gotten far enough down that route to know that for sure. >>> 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(-)
On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote: > On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote: ... > Can the LED framework be used without having the LED exposed to > userspace ? I believe the correct question here is "can the states of some leds be read-only from user perspective" (this way any changes into led subsystems looks less intrusive, esp. taking into account that subsystem is de facto unmaintained).
On 25/11/2022 11:06, Andy Shevchenko wrote: > On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote: >> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote: > ... > >> Can the LED framework be used without having the LED exposed to >> userspace ? > I believe the correct question here is "can the states of some leds be > read-only from user perspective" (this way any changes into led subsystems > looks less intrusive, esp. taking into account that subsystem is de facto > unmaintained). > I think the answer to that is yes: https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47
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(-) >
Hi, On 11/25/22 12:11, Dan Scally wrote: > > On 25/11/2022 11:06, Andy Shevchenko wrote: >> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote: >>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote: >> ... >> >>> Can the LED framework be used without having the LED exposed to >>> userspace ? >> I believe the correct question here is "can the states of some leds be >> read-only from user perspective" (this way any changes into led subsystems >> looks less intrusive, esp. taking into account that subsystem is de facto >> unmaintained). >> > > I think the answer to that is yes: > > > https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47 Interesting, I did not know that. But what is the added value of using the LED subsytem then for a simple only on/off LED driven by a GPIO? One of the challenges with using LED triggers for the privacy led, is that we need at least 2 triggers: "camera-front" and "camera-back" and then somehow to let what ever code sets the triggers know if it is dealing with the front or back sensor. Where as with GPIO-s we *bind* them to the sensor i2c_client so if we just have the sensor-driver look for an optional GPIO called "privacy-led" then we don't have this how to we bind the LED to the sensor problem; and if we drop the sysfs interface I fail to see the value in using the LED subsystem for GPIO a driven LED. Also see my other reply for a proposal to be able to share the code dealing with this between sensor drivers (and also remove some other gpio/clk/regulator boilerplate from sensor drivers). Regards, Hans
Hi On 25/11/2022 11:23, Hans de Goede wrote: > Hi, > > On 11/25/22 12:11, Dan Scally wrote: >> On 25/11/2022 11:06, Andy Shevchenko wrote: >>> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote: >>>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote: >>> ... >>> >>>> Can the LED framework be used without having the LED exposed to >>>> userspace ? >>> I believe the correct question here is "can the states of some leds be >>> read-only from user perspective" (this way any changes into led subsystems >>> looks less intrusive, esp. taking into account that subsystem is de facto >>> unmaintained). >>> >> I think the answer to that is yes: >> >> >> https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47 > Interesting, I did not know that. But what is the added value of > using the LED subsytem then for a simple only on/off LED driven > by a GPIO? Well I suppose it depends on the LED. In the flash case the v4l2 framework disables the sysfs interface for the LED whilst it holds the flash subdev open, which should mean that no nefarious program could turn off the LED whilst it was running the camera but because the sysfs is enabled whilst the v4l2 subdev is closed [1] you could still use that LED as a torch outside of camera streaming. That's probably not a situation that's likely to occur with a privacy LED given they're likely to be much less bright though I suppose, and maybe it's right to treat them differently. [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-flash-led-class.c#L632 > One of the challenges with using LED triggers for the privacy led, > is that we need at least 2 triggers: "camera-front" and "camera-back" > and then somehow to let what ever code sets the triggers know if > it is dealing with the front or back sensor. Yes, that is a problem, my plan was to connect them with fwnode and ancillary links, in the same way for example we connected the VCM to the cameras. I think that the int3472-discrete driver would have to do that. > > Where as with GPIO-s we *bind* them to the sensor i2c_client so if > we just have the sensor-driver look for an optional GPIO called > "privacy-led" then we don't have this how to we bind the LED to > the sensor problem; and if we drop the sysfs interface I fail to > see the value in using the LED subsystem for GPIO a driven LED. > > Also see my other reply for a proposal to be able to share the > code dealing with this between sensor drivers (and also remove > some other gpio/clk/regulator boilerplate from sensor drivers). Yes I certainly find that idea appealing, there is of lot of boilerplate that could be reduced with that idea. > > Regards, > > Hans > >
Hi, On 11/25/22 12:42, Dan Scally wrote: > Hi > > On 25/11/2022 11:23, Hans de Goede wrote: >> Hi, >> >> On 11/25/22 12:11, Dan Scally wrote: >>> On 25/11/2022 11:06, Andy Shevchenko wrote: >>>> On Fri, Nov 25, 2022 at 12:58:46PM +0200, Laurent Pinchart wrote: >>>>> On Fri, Nov 25, 2022 at 10:17:17AM +0000, Dan Scally wrote: >>>> ... >>>> >>>>> Can the LED framework be used without having the LED exposed to >>>>> userspace ? >>>> I believe the correct question here is "can the states of some leds be >>>> read-only from user perspective" (this way any changes into led subsystems >>>> looks less intrusive, esp. taking into account that subsystem is de facto >>>> unmaintained). >>>> >>> I think the answer to that is yes: >>> >>> >>> https://elixir.bootlin.com/linux/latest/source/drivers/leds/led-class.c#L47 >> Interesting, I did not know that. But what is the added value of >> using the LED subsytem then for a simple only on/off LED driven >> by a GPIO? > > > Well I suppose it depends on the LED. In the flash case the v4l2 framework disables the sysfs interface for the LED whilst it holds the flash subdev open, which should mean that no nefarious program could turn off the LED whilst it was running the camera but because the sysfs is enabled whilst the v4l2 subdev is closed [1] you could still use that LED as a torch outside of camera streaming. That's probably not a situation that's likely to occur with a privacy LED given they're likely to be much less bright though I suppose, and maybe it's right to treat them differently. > > > [1] https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-flash-led-class.c#L632 If we only disable the sysfs access temporarily then spy-ware in userspace can still just clear the trigger, so we would need to permanently disable userspace access (or decide to not disable userspace access at all). >> One of the challenges with using LED triggers for the privacy led, >> is that we need at least 2 triggers: "camera-front" and "camera-back" >> and then somehow to let what ever code sets the triggers know if >> it is dealing with the front or back sensor. > > > Yes, that is a problem, my plan was to connect them with fwnode and ancillary links, in the same way for example we connected the VCM to the cameras. I think that the int3472-discrete driver would have to do that. Which would involve a bunch of non trivial code. Where as if we just model the LED as a GPIO for the sensor-driver to consume we get this for free. My conclusion here is: 1. We don't want userspace access because we don't want to make things easier for spy-ware. 2. Without userspace access there is no added value in using the LED subsystem and just modelling this as a GPIO is easier / more KISS. >> Where as with GPIO-s we *bind* them to the sensor i2c_client so if >> we just have the sensor-driver look for an optional GPIO called >> "privacy-led" then we don't have this how to we bind the LED to >> the sensor problem; and if we drop the sysfs interface I fail to >> see the value in using the LED subsystem for GPIO a driven LED. >> >> Also see my other reply for a proposal to be able to share the >> code dealing with this between sensor drivers (and also remove >> some other gpio/clk/regulator boilerplate from sensor drivers). > > > Yes I certainly find that idea appealing, there is of lot of boilerplate that could be reduced with that idea. I glad you like the idea. Any suggestions for a better name for the helper lib / namespace ? Regards, Hans
On Thu, Nov 24, 2022 at 09:00:04PM +0100, 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. How can we be sure that a given sensor model will always be wired to the same GPIOs on all platforms that integrate it with an IPU6 (or IPU3) ? > 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). Can we turn it on at the same time as the power then ? > 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. > > 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(-)
Hi Hans, On Fri, Nov 25, 2022 at 12:15:57PM +0100, Hans de Goede wrote: > 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... ? One use case for turning it off is avoiding reflection in glasses. I however think this is outweighted by the privacy concerns. If the privacy LED can be controlled from userspace, at least by default, then it could as well be dropped completely. > 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. To make it secure it should be controlled by the hardware, yes. > 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 :( I also wish we could get both privacy and flexibility :-( I'm not necessarily opposed to making it controllable by userspace, but that shouldn't be the default. It could be controlled by a kernel command line argument for instance. I doubt that would be worth it though. > 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) I fully agree that camera sensor helpers would be good to have. > 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 */); There are all kind of constraints on the power on/off sequences, I don't think we would be able to model this in a generic way without making it so complicated that it would outweight the benefits. What I think could help is moving all camera sensor drivers to runtime PM, and having helpers to properly enable runtime PM in probe() in a way that works on both ACPI and DT systems, with or without CONFIG_PM enabled. It's way more complicated than it sounds. > // 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. > > >>> And likewise (eventually) completely drop the "clken" GPIO this > >>> patch series introduces (with some sensors) and instead always model > >>> this through the clk-framework. > >>> > >>> 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(-)
Hi, On 11/25/22 15:40, Laurent Pinchart wrote: > On Thu, Nov 24, 2022 at 09:00:04PM +0100, 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. > > How can we be sure that a given sensor model will always be wired to the > same GPIOs on all platforms that integrate it with an IPU6 (or IPU3) ? This is not about which GPIOs are actually there, this is about what the driver expects. Specifically about if the driver expects the clock to be modelled with the clk framework or as a clk-en GPIO which is a property of the driver, not of the board design. But as already mentioned I agree with Dan and you that modelling it through the clk framework is correct and what needs to happen here is to patch the IPU6 sensor drivers to move them to the clk framework. so this is all mute. Regards, Hans
Hi Laurent, On 11/25/22 15:46, Laurent Pinchart wrote: <snip> >> 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) > > I fully agree that camera sensor helpers would be good to have. > >> 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 */); > > There are all kind of constraints on the power on/off sequences, I don't > think we would be able to model this in a generic way without making it > so complicated that it would outweight the benefits. I know that for some ICs the power sequence can be quite complicated, but I think that for most this order should work fine: 0. Force enable/reset GPIOs to disabled / reset-asserted (do this at GPIO request time ?) 1. Enable clk(s) 2. Enable regulators (using the bulk API, with supply-names passed in by the sensor drivers, 3. Set enable/reset GPIOs to enabled / reset de-asserted I guess on some models we may need to swap 1 and 2, there could be a flag for that. Anything more complicated should just be coded out in the driver, but I think just supporting this common pattern will already save us quite a bit of code duplication. > What I think could help is moving all camera sensor drivers to runtime > PM, and having helpers to properly enable runtime PM in probe() in a way > that works on both ACPI and DT systems, with or without CONFIG_PM > enabled. It's way more complicated than it sounds. I agree that we should move to runtime-pm and put the power-sequence in the suspend/resume callback. This will be necessary for any sensors used on atomisp2 devices, where there are actually ACPI _PS0 and _PS3 methods and/or ACPI power-resources doing the PM for us. Note for some reason the current staging atomisp driver does not use this, likely because it was developed for Android boards with broken ACPI tables. But after having sampled the ACPI tables of a bunch of atomisp windows devices I believe this should work fine for those. Regards, Hans > >> // 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. >> >>>>> And likewise (eventually) completely drop the "clken" GPIO this >>>>> patch series introduces (with some sensors) and instead always model >>>>> this through the clk-framework. >>>>> >>>>> 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(-) >
Hi Hans, On Mon, Nov 28, 2022 at 05:11:52PM +0100, Hans de Goede wrote: > On 11/25/22 15:46, Laurent Pinchart wrote: > > <snip> > > >> 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) > > > > I fully agree that camera sensor helpers would be good to have. > > > >> 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 */); > > > > There are all kind of constraints on the power on/off sequences, I don't > > think we would be able to model this in a generic way without making it > > so complicated that it would outweight the benefits. > > I know that for some ICs the power sequence can be quite complicated, > but I think that for most this order should work fine: > > 0. Force enable/reset GPIOs to disabled / reset-asserted (do this at GPIO request time ?) > 1. Enable clk(s) > 2. Enable regulators (using the bulk API, with supply-names passed > in by the sensor drivers, > 3. Set enable/reset GPIOs to enabled / reset de-asserted > > I guess on some models we may need to swap 1 and 2, there could be > a flag for that. There are also various delays that may be needed between the different steps, including between bringing up (and down) the different power rails. > Anything more complicated should just be coded out in the driver, but > I think just supporting this common pattern will already save us > quite a bit of code duplication. There was an old attempt to code generic power sequences in DT which didn't lead anywhere. I'm not quite sure doing so in a camera sensor helper will have a much better fate. We can of course give it a try, but as mentioned before, I think effort would be better focussed on first moving sensor drivers to runtime PM (and runtime PM autosuspend). > > What I think could help is moving all camera sensor drivers to runtime > > PM, and having helpers to properly enable runtime PM in probe() in a way > > that works on both ACPI and DT systems, with or without CONFIG_PM > > enabled. It's way more complicated than it sounds. > > I agree that we should move to runtime-pm and put the power-sequence > in the suspend/resume callback. This will be necessary for any sensors > used on atomisp2 devices, where there are actually ACPI _PS0 and _PS3 > methods and/or ACPI power-resources doing the PM for us. > > Note for some reason the current staging atomisp driver does not use this, > likely because it was developed for Android boards with broken ACPI > tables. But after having sampled the ACPI tables of a bunch of atomisp > windows devices I believe this should work fine for those. > > >> // 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. > >> > >>>>> And likewise (eventually) completely drop the "clken" GPIO this > >>>>> patch series introduces (with some sensors) and instead always model > >>>>> this through the clk-framework. > >>>>> > >>>>> 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(-)