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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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(-)

-- 
Regards,

Laurent Pinchart