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