diff mbox series

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

Message ID 20221124200007.390901-4-hdegoede@redhat.com (mailing list archive)
State Not Applicable
Headers
Series platform/x86: int3472/discrete: Make it work with IPU6 |

Commit Message

Hans de Goede Nov. 24, 2022, 8 p.m. UTC 
  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(-)

Comments

Laurent Pinchart Nov. 25, 2022, 2:36 p.m. UTC | #1 
Hi Hans,

Thank you for the patch.

On Thu, Nov 24, 2022 at 09:00:07PM +0100, 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.

I don't like this idea much. I see this as opening the door to other
hacks in mainline just for the purpose of supporting out-of-tree
drivers. That's not how we should operate upstream.

Why can't we patch the out-of-tree drivers to use the clock framework,
given that's what it will need to do in mainline ? That shouldn't be a
too intrusive change.

> 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.

To fix the privacy LED flickering problem correctly we need to avoid
powering up sensor at probe time, as there are hardware designs that
wire the privacy LED to the sensor power rails without any way to
disable it in software.

> 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.
> + */
> +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);
Dan Scally Nov. 25, 2022, 4:07 p.m. UTC | #2 
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?


Thanks

Dan

> +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);
Hans de Goede Nov. 25, 2022, 6:38 p.m. UTC | #3 
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);
>
Dan Scally Nov. 28, 2022, 7:39 a.m. UTC | #4 
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);
Hans de Goede Nov. 28, 2022, 10:04 a.m. UTC | #5 
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);
>
diff mbox series

Patch

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.
+ */
+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);