Message ID | 20221124200007.390901-1-hdegoede@redhat.com |
---|---|
Headers | show |
Series | platform/x86: int3472/discrete: Make it work with IPU6 | expand |
On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote: > > The out of tree IPU6 driver has moved to also using the in kernel INT3472 > code for doing power-ctrl rather then doing their own thing (good!). > > On IPU6 the polarity is encoded in the _DSM entry rather then being > hardcoded to GPIO_ACTIVE_LOW. > > Using the _DSM entry for this on IPU3 leads to regressions, so only > use the _DSM entry for this on non IPU3 devices. > > Note there is a whole bunch of PCI-ids for the IPU6 which is why > the check is for the IPU3-CIO2, because the CIO2 there has a unique > PCI-id which can be used for this. ... > +/* IPU3 vs IPU6 needs to be handled differently */ > +#define IPU3_CIO2_PCI_ID 0x9d32 If it makes more than a single driver, perhaps pci_ids.h is a good place to keep it there? ... > + dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func, > + agpio->resource_source.string_ptr, agpio->pin_table[0], > + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); Parentheses are not needed, right?
Hi, On 11/24/22 21:13, Andy Shevchenko wrote: > On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> The out of tree IPU6 driver has moved to also using the in kernel INT3472 >> code for doing power-ctrl rather then doing their own thing (good!). >> >> On IPU6 the polarity is encoded in the _DSM entry rather then being >> hardcoded to GPIO_ACTIVE_LOW. >> >> Using the _DSM entry for this on IPU3 leads to regressions, so only >> use the _DSM entry for this on non IPU3 devices. >> >> Note there is a whole bunch of PCI-ids for the IPU6 which is why >> the check is for the IPU3-CIO2, because the CIO2 there has a unique >> PCI-id which can be used for this. > > ... > >> +/* IPU3 vs IPU6 needs to be handled differently */ >> +#define IPU3_CIO2_PCI_ID 0x9d32 > > If it makes more than a single driver, perhaps pci_ids.h is a good > place to keep it there? Yes, I've added a note to my TODO to clean this up in a follow-up patch (the pci-ids.h maintainers want to see multiple users of an id before accepting new ids in there). > > ... > >> + dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func, >> + agpio->resource_source.string_ptr, agpio->pin_table[0], > >> + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); > > Parentheses are not needed, right? Right, but I prefer to use them in conditional statements like these, because I personally find that they make things easier to read. Regards, Hans
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(-) >
Hello On 24/11/2022 20:26, Hans de Goede wrote: > Hi, > > On 11/24/22 21:13, Andy Shevchenko wrote: >> On Thu, Nov 24, 2022 at 10:00 PM Hans de Goede <hdegoede@redhat.com> wrote: >>> The out of tree IPU6 driver has moved to also using the in kernel INT3472 >>> code for doing power-ctrl rather then doing their own thing (good!). >>> >>> On IPU6 the polarity is encoded in the _DSM entry rather then being >>> hardcoded to GPIO_ACTIVE_LOW. >>> >>> Using the _DSM entry for this on IPU3 leads to regressions, so only >>> use the _DSM entry for this on non IPU3 devices. >>> >>> Note there is a whole bunch of PCI-ids for the IPU6 which is why >>> the check is for the IPU3-CIO2, because the CIO2 there has a unique >>> PCI-id which can be used for this. >> ... >> >>> +/* IPU3 vs IPU6 needs to be handled differently */ >>> +#define IPU3_CIO2_PCI_ID 0x9d32 >> If it makes more than a single driver, perhaps pci_ids.h is a good >> place to keep it there? > Yes, I've added a note to my TODO to clean this up in a follow-up > patch (the pci-ids.h maintainers want to see multiple users of > an id before accepting new ids in there). fwiw this in drivers/media/pci/intel/ipu3/ipu3-cio2.h already as CIO2_PCI_ID, so it will have multiple users with this change. >> ... >> >>> + dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func, >>> + agpio->resource_source.string_ptr, agpio->pin_table[0], >>> + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); >> Parentheses are not needed, right? > Right, but I prefer to use them in conditional statements like these, > because I personally find that they make things easier to read. Seconded. > Regards, > > Hans > >
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 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: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
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 Hans On 24/11/2022 20:00, Hans de Goede wrote: > The out of tree IPU6 driver has moved to also using the in kernel INT3472 > code for doing power-ctrl rather then doing their own thing (good!). Neat! > > On IPU6 the polarity is encoded in the _DSM entry rather then being > hardcoded to GPIO_ACTIVE_LOW. > > Using the _DSM entry for this on IPU3 leads to regressions, so only > use the _DSM entry for this on non IPU3 devices. Shame; some consistency might have been nice > Note there is a whole bunch of PCI-ids for the IPU6 which is why > the check is for the IPU3-CIO2, because the CIO2 there has a unique > PCI-id which can be used for this. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> Tested-by: Daniel Scally <dan.scally@ideasonboard.com> > drivers/platform/x86/intel/int3472/discrete.c | 28 +++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index bc6c62f3f3bf..9159291be28a 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -11,6 +11,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/overflow.h> > +#include <linux/pci.h> > #include <linux/platform_device.h> > #include <linux/uuid.h> > > @@ -36,6 +37,19 @@ static const guid_t cio2_sensor_module_guid = > GUID_INIT(0x822ace8f, 0x2814, 0x4174, > 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); > > +/* IPU3 vs IPU6 needs to be handled differently */ > +#define IPU3_CIO2_PCI_ID 0x9d32 > + > +static const struct pci_device_id ipu3_cio2_pci_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, IPU3_CIO2_PCI_ID) }, > + { } > +}; > + > +static int ipu3_present(void) > +{ > + return pci_dev_present(ipu3_cio2_pci_id_table); > +} > + > /* > * Here follows platform specific mapping information that we can pass to > * the functions mapping resources to the sensors. Where the sensors have > @@ -242,6 +256,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > union acpi_object *obj; > const char *err_msg; > const char *func; > + u32 polarity; > int ret; > u8 type; > > @@ -265,13 +280,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > > type = obj->integer.value & 0xff; > > + /* IPU3 always uses active-low, IPU6 polarity is encoded in the _DSM entry. */ > + if (ipu3_present()) > + polarity = GPIO_ACTIVE_LOW; > + else > + polarity = ((obj->integer.value >> 24) & 0xff) ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW; > + > func = int3472_dsm_type_to_func(type); > > + dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func, > + agpio->resource_source.string_ptr, agpio->pin_table[0], > + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); > + > switch (type) { > case INT3472_GPIO_TYPE_RESET: > case INT3472_GPIO_TYPE_POWERDOWN: > - ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, > - GPIO_ACTIVE_LOW); > + ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity); > if (ret) > err_msg = "Failed to map GPIO pin to sensor\n"; >
Hi, On 11/25/22 17:07, Dan Scally wrote: > Hi Hans > > On 24/11/2022 20:00, Hans de Goede wrote: >> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 >> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather >> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver >> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then >> it being turned on/off at the same time as the clk. >> >> Adjust how we handle the GPIOs on these sensors accordingly, for now at >> least, so that the out of tree driver can work with standard distro kernels >> through e.g. dkms. Otherwise users need to run a patched kernel just for >> this small difference. >> >> This of course needs to be revisited when we mainline these sensor drivers, >> I can imagine the drivers getting clk-framework support when they are >> mainlined and then at that same time their acpi HID can be dropped from >> the use_gpio_for_clk_acpi_ids[] array. >> >> Note there already is a mainline driver for the ov2740, but that is not >> impacted by this change since atm it uses neither the clk framework nor >> a "clken" GPIO. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Maybe we should patch the sensor drivers for sensors supported with >> the IPU3 to also expect the privacy-led to always be a separate GPIO? >> >> This way we can also avoid the camera LED briefly going on at boot, >> when the driver is powering things up to read the sensor's ID register. >> >> And I have also put looking at making the mainline ov2740 driver suitable >> for use with the (out of tree) IPU6 driver on my TODO list. >> --- >> drivers/platform/x86/intel/int3472/common.h | 2 +- >> drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++---- >> 2 files changed, 31 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h >> index 53270d19c73a..58647d3084b9 100644 >> --- a/drivers/platform/x86/intel/int3472/common.h >> +++ b/drivers/platform/x86/intel/int3472/common.h >> @@ -23,7 +23,7 @@ >> #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d >> #define INT3472_PDEV_MAX_NAME_LEN 23 >> -#define INT3472_MAX_SENSOR_GPIOS 3 >> +#define INT3472_MAX_SENSOR_GPIOS 4 >> #define GPIO_REGULATOR_NAME_LENGTH 21 >> #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index 9159291be28a..bfcf8184db16 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type) >> return "unknown"; >> } >> +/* >> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver, >> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and >> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled >> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk. >> + * >> + * Note there also is a mainline driver for the ov2740, but that does not use >> + * the clk framework atm either. >> + * >> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least. >> + * This needs to be revisited when we mainline these sensor drivers / when we merge >> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6. >> + */ > > > I'm not really sure about this one though; wouldn't it be better to alter those sensor drivers to use the clock framework properly? Yes, Laurent more or less said the same thing; and I was already starting to think in this direction myself when typing the cover letter. So yes I agree with you and Laurent. That still leaves the question of what to do with devices with just a privacy LED without a clk-en though. Dan, do you have a list of sensors which currently are known to work / be used together with the IPU3 (and the int3472 discrete code) ? I know I will need to modifi the ov5693 code, but I wonder what other drivers I will need to modify ? I think I might just move those sensor-drivers over to using a GPIO for the privacy LED and just always register a GPIO for the privacy LED pin, does that sound like a good idea to you ? Anyways it is weekend now and I've already worked too many hours this week, so I'll take a look at this on Monday. Regards, Hans >> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = { >> + { "HIMX11B1" }, /* hm11b1 */ >> + { "OVTI01AS" }, /* ov01a1s */ >> + { "INT3474" }, /* ov2740 */ >> + {} >> +}; >> + >> /** >> * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor >> * @ares: A pointer to a &struct acpi_resource >> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); >> switch (type) { >> + case INT3472_GPIO_TYPE_CLK_ENABLE: >> + case INT3472_GPIO_TYPE_PRIVACY_LED: >> + if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) { >> + ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); >> + if (ret) >> + err_msg = "Failed to map GPIO to clock\n"; >> + >> + break; >> + } >> + fallthrough; >> case INT3472_GPIO_TYPE_RESET: >> case INT3472_GPIO_TYPE_POWERDOWN: >> ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity); >> if (ret) >> err_msg = "Failed to map GPIO pin to sensor\n"; >> - break; >> - case INT3472_GPIO_TYPE_CLK_ENABLE: >> - case INT3472_GPIO_TYPE_PRIVACY_LED: >> - ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); >> - if (ret) >> - err_msg = "Failed to map GPIO to clock\n"; >> - >> break; >> case INT3472_GPIO_TYPE_POWER_ENABLE: >> ret = skl_int3472_register_regulator(int3472, agpio); >
Morning Hans On 25/11/2022 18:38, Hans de Goede wrote: > Hi, > > On 11/25/22 17:07, Dan Scally wrote: >> Hi Hans >> >> On 24/11/2022 20:00, Hans de Goede wrote: >>> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 >>> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather >>> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver >>> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then >>> it being turned on/off at the same time as the clk. >>> >>> Adjust how we handle the GPIOs on these sensors accordingly, for now at >>> least, so that the out of tree driver can work with standard distro kernels >>> through e.g. dkms. Otherwise users need to run a patched kernel just for >>> this small difference. >>> >>> This of course needs to be revisited when we mainline these sensor drivers, >>> I can imagine the drivers getting clk-framework support when they are >>> mainlined and then at that same time their acpi HID can be dropped from >>> the use_gpio_for_clk_acpi_ids[] array. >>> >>> Note there already is a mainline driver for the ov2740, but that is not >>> impacted by this change since atm it uses neither the clk framework nor >>> a "clken" GPIO. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> --- >>> Maybe we should patch the sensor drivers for sensors supported with >>> the IPU3 to also expect the privacy-led to always be a separate GPIO? >>> >>> This way we can also avoid the camera LED briefly going on at boot, >>> when the driver is powering things up to read the sensor's ID register. >>> >>> And I have also put looking at making the mainline ov2740 driver suitable >>> for use with the (out of tree) IPU6 driver on my TODO list. >>> --- >>> drivers/platform/x86/intel/int3472/common.h | 2 +- >>> drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++---- >>> 2 files changed, 31 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h >>> index 53270d19c73a..58647d3084b9 100644 >>> --- a/drivers/platform/x86/intel/int3472/common.h >>> +++ b/drivers/platform/x86/intel/int3472/common.h >>> @@ -23,7 +23,7 @@ >>> #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d >>> #define INT3472_PDEV_MAX_NAME_LEN 23 >>> -#define INT3472_MAX_SENSOR_GPIOS 3 >>> +#define INT3472_MAX_SENSOR_GPIOS 4 >>> #define GPIO_REGULATOR_NAME_LENGTH 21 >>> #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 >>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >>> index 9159291be28a..bfcf8184db16 100644 >>> --- a/drivers/platform/x86/intel/int3472/discrete.c >>> +++ b/drivers/platform/x86/intel/int3472/discrete.c >>> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type) >>> return "unknown"; >>> } >>> +/* >>> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver, >>> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and >>> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled >>> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk. >>> + * >>> + * Note there also is a mainline driver for the ov2740, but that does not use >>> + * the clk framework atm either. >>> + * >>> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least. >>> + * This needs to be revisited when we mainline these sensor drivers / when we merge >>> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6. >>> + */ >> >> I'm not really sure about this one though; wouldn't it be better to alter those sensor drivers to use the clock framework properly? > Yes, Laurent more or less said the same thing; and I was already starting > to think in this direction myself when typing the cover letter. > > So yes I agree with you and Laurent. That still leaves the question of what to do > with devices with just a privacy LED without a clk-en though. > > Dan, do you have a list of sensors which currently are known to work / be used > together with the IPU3 (and the int3472 discrete code) ? > > I know I will need to modifi the ov5693 code, but I wonder what other drivers > I will need to modify ? The ov5693, ov8865 and ov7251 are the upstream working ones. There's a couple more that need changes upstreaming, but I can handle those during that process. > > I think I might just move those sensor-drivers over to using a GPIO > for the privacy LED and just always register a GPIO for the privacy LED > pin, does that sound like a good idea to you ? Well if we can't handle it during the int3472 code then yes - I certainly don't have a better idea. > Anyways it is weekend now and I've already worked too many hours this week, > so I'll take a look at this on Monday. > > Regards, > > Hans > > > >>> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = { >>> + { "HIMX11B1" }, /* hm11b1 */ >>> + { "OVTI01AS" }, /* ov01a1s */ >>> + { "INT3474" }, /* ov2740 */ >>> + {} >>> +}; >>> + >>> /** >>> * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor >>> * @ares: A pointer to a &struct acpi_resource >>> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>> (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); >>> switch (type) { >>> + case INT3472_GPIO_TYPE_CLK_ENABLE: >>> + case INT3472_GPIO_TYPE_PRIVACY_LED: >>> + if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) { >>> + ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); >>> + if (ret) >>> + err_msg = "Failed to map GPIO to clock\n"; >>> + >>> + break; >>> + } >>> + fallthrough; >>> case INT3472_GPIO_TYPE_RESET: >>> case INT3472_GPIO_TYPE_POWERDOWN: >>> ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity); >>> if (ret) >>> err_msg = "Failed to map GPIO pin to sensor\n"; >>> - break; >>> - case INT3472_GPIO_TYPE_CLK_ENABLE: >>> - case INT3472_GPIO_TYPE_PRIVACY_LED: >>> - ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); >>> - if (ret) >>> - err_msg = "Failed to map GPIO to clock\n"; >>> - >>> break; >>> case INT3472_GPIO_TYPE_POWER_ENABLE: >>> ret = skl_int3472_register_regulator(int3472, agpio);
Hi, On 11/28/22 08:39, Dan Scally wrote: > Morning Hans > > On 25/11/2022 18:38, Hans de Goede wrote: >> Hi, >> >> On 11/25/22 17:07, Dan Scally wrote: >>> Hi Hans >>> >>> On 24/11/2022 20:00, Hans de Goede wrote: >>>> The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 >>>> driver, expect the clk_en GPIO to be modelled as a "clken" GPIO rather >>>> then using the clk framework; and the hm11b1, ov01a1s and ov2740 driver >>>> all 3 expect the privacy-led to be modelled as a "pled" GPIO, rather then >>>> it being turned on/off at the same time as the clk. >>>> >>>> Adjust how we handle the GPIOs on these sensors accordingly, for now at >>>> least, so that the out of tree driver can work with standard distro kernels >>>> through e.g. dkms. Otherwise users need to run a patched kernel just for >>>> this small difference. >>>> >>>> This of course needs to be revisited when we mainline these sensor drivers, >>>> I can imagine the drivers getting clk-framework support when they are >>>> mainlined and then at that same time their acpi HID can be dropped from >>>> the use_gpio_for_clk_acpi_ids[] array. >>>> >>>> Note there already is a mainline driver for the ov2740, but that is not >>>> impacted by this change since atm it uses neither the clk framework nor >>>> a "clken" GPIO. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>>> --- >>>> Maybe we should patch the sensor drivers for sensors supported with >>>> the IPU3 to also expect the privacy-led to always be a separate GPIO? >>>> >>>> This way we can also avoid the camera LED briefly going on at boot, >>>> when the driver is powering things up to read the sensor's ID register. >>>> >>>> And I have also put looking at making the mainline ov2740 driver suitable >>>> for use with the (out of tree) IPU6 driver on my TODO list. >>>> --- >>>> drivers/platform/x86/intel/int3472/common.h | 2 +- >>>> drivers/platform/x86/intel/int3472/discrete.c | 37 +++++++++++++++---- >>>> 2 files changed, 31 insertions(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h >>>> index 53270d19c73a..58647d3084b9 100644 >>>> --- a/drivers/platform/x86/intel/int3472/common.h >>>> +++ b/drivers/platform/x86/intel/int3472/common.h >>>> @@ -23,7 +23,7 @@ >>>> #define INT3472_GPIO_TYPE_PRIVACY_LED 0x0d >>>> #define INT3472_PDEV_MAX_NAME_LEN 23 >>>> -#define INT3472_MAX_SENSOR_GPIOS 3 >>>> +#define INT3472_MAX_SENSOR_GPIOS 4 >>>> #define GPIO_REGULATOR_NAME_LENGTH 21 >>>> #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 >>>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >>>> index 9159291be28a..bfcf8184db16 100644 >>>> --- a/drivers/platform/x86/intel/int3472/discrete.c >>>> +++ b/drivers/platform/x86/intel/int3472/discrete.c >>>> @@ -216,6 +216,26 @@ static const char *int3472_dsm_type_to_func(u8 type) >>>> return "unknown"; >>>> } >>>> +/* >>>> + * The hm11b1 and ov01a1s sensor drivers shipped with the out of tree IPU6 driver, >>>> + * expect the clk_en GPIO to be modelled as a "clken" GPIO rather then as a clk and >>>> + * the hm11b1, ov01a1s and ov2740 driver all 3 expect the privacy-led to be modelled >>>> + * as a "pled" GPIO, rather then it being turned on/off at the same time as the clk. >>>> + * >>>> + * Note there also is a mainline driver for the ov2740, but that does not use >>>> + * the clk framework atm either. >>>> + * >>>> + * Adjust how we handle the GPIOs on these sensors accordingly, for now at least. >>>> + * This needs to be revisited when we mainline these sensor drivers / when we merge >>>> + * the necessary changes in the ov2740 sensor driver so that it can work on the IPU6. >>>> + */ >>> >>> I'm not really sure about this one though; wouldn't it be better to alter those sensor drivers to use the clock framework properly? >> Yes, Laurent more or less said the same thing; and I was already starting >> to think in this direction myself when typing the cover letter. >> >> So yes I agree with you and Laurent. That still leaves the question of what to do >> with devices with just a privacy LED without a clk-en though. >> >> Dan, do you have a list of sensors which currently are known to work / be used >> together with the IPU3 (and the int3472 discrete code) ? >> >> I know I will need to modifi the ov5693 code, but I wonder what other drivers >> I will need to modify ? > > > The ov5693, ov8865 and ov7251 are the upstream working ones. There's a couple more that need changes upstreaming, but I can handle those during that process. Ok thanks. >> I think I might just move those sensor-drivers over to using a GPIO >> for the privacy LED and just always register a GPIO for the privacy LED >> pin, does that sound like a good idea to you ? > > > Well if we can't handle it during the int3472 code then yes - I certainly don't have a better idea. I will patch the 3 sensors listed above to take an optional privacy LED GPIO then. I do plan to work on the sensor_power helper library which I discussed soon-ish since that should make things a lot easier, but for now I'll just do this the quick and dirty way, also to make backporting easier for distros (so that they can have a kernel which works with both IPU3 and the out-of-tree IPU6 stuff). Regards, Hans > >> Anyways it is weekend now and I've already worked too many hours this week, >> so I'll take a look at this on Monday. >> >> Regards, >> >> Hans >> >> >> >>>> +static const struct acpi_device_id use_gpio_for_clk_acpi_ids[] = { >>>> + { "HIMX11B1" }, /* hm11b1 */ >>>> + { "OVTI01AS" }, /* ov01a1s */ >>>> + { "INT3474" }, /* ov2740 */ >>>> + {} >>>> +}; >>>> + >>>> /** >>>> * skl_int3472_handle_gpio_resources: Map PMIC resources to consuming sensor >>>> * @ares: A pointer to a &struct acpi_resource >>>> @@ -293,19 +313,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>>> (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); >>>> switch (type) { >>>> + case INT3472_GPIO_TYPE_CLK_ENABLE: >>>> + case INT3472_GPIO_TYPE_PRIVACY_LED: >>>> + if (!acpi_match_device_ids(int3472->adev, use_gpio_for_clk_acpi_ids)) { >>>> + ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); >>>> + if (ret) >>>> + err_msg = "Failed to map GPIO to clock\n"; >>>> + >>>> + break; >>>> + } >>>> + fallthrough; >>>> case INT3472_GPIO_TYPE_RESET: >>>> case INT3472_GPIO_TYPE_POWERDOWN: >>>> ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity); >>>> if (ret) >>>> err_msg = "Failed to map GPIO pin to sensor\n"; >>>> - break; >>>> - case INT3472_GPIO_TYPE_CLK_ENABLE: >>>> - case INT3472_GPIO_TYPE_PRIVACY_LED: >>>> - ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); >>>> - if (ret) >>>> - err_msg = "Failed to map GPIO to clock\n"; >>>> - >>>> break; >>>> case INT3472_GPIO_TYPE_POWER_ENABLE: >>>> ret = skl_int3472_register_regulator(int3472, agpio); >
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(-)
Hi All, On 11/24/22 21:00, Hans de Goede wrote: > The out of tree IPU6 driver has moved to also using the in kernel INT3472 > code for doing power-ctrl rather then doing their own thing (good!). > > On IPU6 the polarity is encoded in the _DSM entry rather then being > hardcoded to GPIO_ACTIVE_LOW. > > Using the _DSM entry for this on IPU3 leads to regressions, so only > use the _DSM entry for this on non IPU3 devices. So it turns out that the reason why this does not work on IPU3 is because looking at this as polarity = (bits 31-24) ? high:low is not correct. The correct way of looking at this really is: polarity = default-polarity-for-pin; if ((bits 31-24) == 0) polarity = !polarity; The: "polarity = (bits 31-24) ? high:low" thing did work with IPU6 because the out of tree bundled drivers set reset and poweroff to 1 on power-on and to 0 on power-off. IOW they apply the default active-low-ness of these pins at the sensor driver level rather then letting the GPIO core handle this. Which is actually the wrong thing to do... For the new series replacing this one I'm going to go with the: if ((bits 31-24) == 0) polarity = !polarity; Approach which works on both IPU3 and IPU6. I'll also make this the last patch in the series and I'll probably merge it later then the rest of the series so that it can get some extra testing. Regards, Hans > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/intel/int3472/discrete.c | 28 +++++++++++++++++-- > 1 file changed, 26 insertions(+), 2 deletions(-) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index bc6c62f3f3bf..9159291be28a 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -11,6 +11,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/overflow.h> > +#include <linux/pci.h> > #include <linux/platform_device.h> > #include <linux/uuid.h> > > @@ -36,6 +37,19 @@ static const guid_t cio2_sensor_module_guid = > GUID_INIT(0x822ace8f, 0x2814, 0x4174, > 0xa5, 0x6b, 0x5f, 0x02, 0x9f, 0xe0, 0x79, 0xee); > > +/* IPU3 vs IPU6 needs to be handled differently */ > +#define IPU3_CIO2_PCI_ID 0x9d32 > + > +static const struct pci_device_id ipu3_cio2_pci_id_table[] = { > + { PCI_DEVICE(PCI_VENDOR_ID_INTEL, IPU3_CIO2_PCI_ID) }, > + { } > +}; > + > +static int ipu3_present(void) > +{ > + return pci_dev_present(ipu3_cio2_pci_id_table); > +} > + > /* > * Here follows platform specific mapping information that we can pass to > * the functions mapping resources to the sensors. Where the sensors have > @@ -242,6 +256,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > union acpi_object *obj; > const char *err_msg; > const char *func; > + u32 polarity; > int ret; > u8 type; > > @@ -265,13 +280,22 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > > type = obj->integer.value & 0xff; > > + /* IPU3 always uses active-low, IPU6 polarity is encoded in the _DSM entry. */ > + if (ipu3_present()) > + polarity = GPIO_ACTIVE_LOW; > + else > + polarity = ((obj->integer.value >> 24) & 0xff) ? GPIO_ACTIVE_HIGH : GPIO_ACTIVE_LOW; > + > func = int3472_dsm_type_to_func(type); > > + dev_dbg(int3472->dev, "%s %s pin %d active-%s\n", func, > + agpio->resource_source.string_ptr, agpio->pin_table[0], > + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); > + > switch (type) { > case INT3472_GPIO_TYPE_RESET: > case INT3472_GPIO_TYPE_POWERDOWN: > - ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, > - GPIO_ACTIVE_LOW); > + ret = skl_int3472_map_gpio_to_sensor(int3472, agpio, func, polarity); > if (ret) > err_msg = "Failed to map GPIO pin to sensor\n"; >