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