Re: [PATCH 3/3] platform/x86: int3472/discrete: Add support for sensor-drivers which expect clken + pled GPIOs

[Hans de Goede <hdegoede@redhat.com>]

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