diff mbox series

[2/3] platform/x86: int3472/discrete: Get the polarity from the _DSM entry

Message ID 20221124200007.390901-3-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 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.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/discrete.c | 28 +++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Andy Shevchenko Nov. 24, 2022, 8:13 p.m. UTC | #1 
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?
Hans de Goede Nov. 24, 2022, 8:26 p.m. UTC | #2 
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
Dan Scally Nov. 25, 2022, 10:42 a.m. UTC | #3 
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
>
>
Dan Scally Nov. 25, 2022, 4:01 p.m. UTC | #4 
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";
>
Hans de Goede Nov. 29, 2022, 9:56 p.m. UTC | #5 
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";
>
diff mbox series

Patch

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