| Message ID | 20221124200007.390901-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | platform/x86: int3472/discrete: Make it work with IPU6 | expand |
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?
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
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 > >
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"; >
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 --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";
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(-)