| Message ID | 20230612141632.5232-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] platform/x86: int3472: discrete: Fix getting active_value | expand |
Hi, On 6/12/23 16:20, Dan Scally wrote: > Hi Hans > > On 12/06/2023 15:16, Hans de Goede wrote: >> The INT3472 discrete code assumes that the ACPI GPIO resources are >> in the same order as the pin-info _DSM entries. >> >> The returned pin-info includes the pin-number in bits 15-8. Add a check >> that this matches with the ACPI GPIO resource pin-number in case >> the assumption is not true with some ACPI tables. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> > > > That seems like a good idea to me: > > > Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> Thanks. Did you also see my: "[PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support" series ? It would be great if I can get your input on that. Personally I'm not entirely happy with patch 3/4 there, but I don't really see any other way of solving the issue on the mix 510 that 3/4 fixes. Regards, Hans > >> --- >> drivers/platform/x86/intel/int3472/discrete.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index 4ef60883154d..c1132bbbff41 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -149,8 +149,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> { >> struct int3472_discrete_device *int3472 = data; >> struct acpi_resource_gpio *agpio; >> + u8 active_value, pin, type; >> union acpi_object *obj; >> - u8 active_value, type; >> const char *err_msg; >> const char *func; >> u32 polarity; >> @@ -174,10 +174,18 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> return 1; >> } >> + /* Bits 7-0 contain the type/function of the pin */ >> type = obj->integer.value & 0xff; >> int3472_get_func_and_polarity(type, &func, &polarity); >> + /* Bits 15-8 contain the pin-number on the GPIO chip */ >> + pin = (obj->integer.value >> 8) & 0xff; >> + if (pin != agpio->pin_table[0]) >> + dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n", >> + func, agpio->resource_source.string_ptr, pin, >> + agpio->pin_table[0]); >> + >> /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ >> active_value = (obj->integer.value >> 24) & 0xff; >> if (!active_value) >
Hi Hans On 12/06/2023 16:26, Hans de Goede wrote: > Hi, > > On 6/12/23 16:20, Dan Scally wrote: >> Hi Hans >> >> On 12/06/2023 15:16, Hans de Goede wrote: >>> The INT3472 discrete code assumes that the ACPI GPIO resources are >>> in the same order as the pin-info _DSM entries. >>> >>> The returned pin-info includes the pin-number in bits 15-8. Add a check >>> that this matches with the ACPI GPIO resource pin-number in case >>> the assumption is not true with some ACPI tables. >>> >>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> >> That seems like a good idea to me: >> >> >> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> > Thanks. Did you also see my: > "[PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support" > > series ? It would be great if I can get your input on that. > > Personally I'm not entirely happy with patch 3/4 there, > but I don't really see any other way of solving > the issue on the mix 510 that 3/4 fixes. I did; I was hoping to go through the ov2680 set and that set today but I've been a bit busy; I'll try to look at and test both asap. > > Regards, > > Hans > > >>> --- >>> drivers/platform/x86/intel/int3472/discrete.c | 10 +++++++++- >>> 1 file changed, 9 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >>> index 4ef60883154d..c1132bbbff41 100644 >>> --- a/drivers/platform/x86/intel/int3472/discrete.c >>> +++ b/drivers/platform/x86/intel/int3472/discrete.c >>> @@ -149,8 +149,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>> { >>> struct int3472_discrete_device *int3472 = data; >>> struct acpi_resource_gpio *agpio; >>> + u8 active_value, pin, type; >>> union acpi_object *obj; >>> - u8 active_value, type; >>> const char *err_msg; >>> const char *func; >>> u32 polarity; >>> @@ -174,10 +174,18 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>> return 1; >>> } >>> + /* Bits 7-0 contain the type/function of the pin */ >>> type = obj->integer.value & 0xff; >>> int3472_get_func_and_polarity(type, &func, &polarity); >>> + /* Bits 15-8 contain the pin-number on the GPIO chip */ >>> + pin = (obj->integer.value >> 8) & 0xff; >>> + if (pin != agpio->pin_table[0]) >>> + dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n", >>> + func, agpio->resource_source.string_ptr, pin, >>> + agpio->pin_table[0]); >>> + >>> /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ >>> active_value = (obj->integer.value >> 24) & 0xff; >>> if (!active_value)
Hi, On 6/12/23 17:30, Dan Scally wrote: > Hi Hans > > On 12/06/2023 16:26, Hans de Goede wrote: >> Hi, >> >> On 6/12/23 16:20, Dan Scally wrote: >>> Hi Hans >>> >>> On 12/06/2023 15:16, Hans de Goede wrote: >>>> The INT3472 discrete code assumes that the ACPI GPIO resources are >>>> in the same order as the pin-info _DSM entries. >>>> >>>> The returned pin-info includes the pin-number in bits 15-8. Add a check >>>> that this matches with the ACPI GPIO resource pin-number in case >>>> the assumption is not true with some ACPI tables. >>>> >>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >>> >>> That seems like a good idea to me: >>> >>> >>> Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com> >> Thanks. Did you also see my: >> "[PATCH 0/4] platform/x86: int3472: discrete: Regulator rework / Lenovo Miix 510 support" >> >> series ? It would be great if I can get your input on that. >> >> Personally I'm not entirely happy with patch 3/4 there, >> but I don't really see any other way of solving >> the issue on the mix 510 that 3/4 fixes. > > > I did; I was hoping to go through the ov2680 set and that set today but I've been a bit busy; I'll try to look at and test both asap. Ok, thank you. Note the ov2680 set does not work with the IPU3 yet. I've tried with the ipu3-capture.sh script after first testing the script with the ov5693 and my current ov2680 set is missing the link-rate / pixel-rate v4l2 ctrls. I plan to continue working on this, adding those ctrls tomorrow. Regards, Hans >>>> --- >>>> drivers/platform/x86/intel/int3472/discrete.c | 10 +++++++++- >>>> 1 file changed, 9 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >>>> index 4ef60883154d..c1132bbbff41 100644 >>>> --- a/drivers/platform/x86/intel/int3472/discrete.c >>>> +++ b/drivers/platform/x86/intel/int3472/discrete.c >>>> @@ -149,8 +149,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>>> { >>>> struct int3472_discrete_device *int3472 = data; >>>> struct acpi_resource_gpio *agpio; >>>> + u8 active_value, pin, type; >>>> union acpi_object *obj; >>>> - u8 active_value, type; >>>> const char *err_msg; >>>> const char *func; >>>> u32 polarity; >>>> @@ -174,10 +174,18 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >>>> return 1; >>>> } >>>> + /* Bits 7-0 contain the type/function of the pin */ >>>> type = obj->integer.value & 0xff; >>>> int3472_get_func_and_polarity(type, &func, &polarity); >>>> + /* Bits 15-8 contain the pin-number on the GPIO chip */ >>>> + pin = (obj->integer.value >> 8) & 0xff; >>>> + if (pin != agpio->pin_table[0]) >>>> + dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n", >>>> + func, agpio->resource_source.string_ptr, pin, >>>> + agpio->pin_table[0]); >>>> + >>>> /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ >>>> active_value = (obj->integer.value >> 24) & 0xff; >>>> if (!active_value) >
On Mon, 12 Jun 2023, Hans de Goede wrote: > The INT3472 discrete code assumes that the ACPI GPIO resources are > in the same order as the pin-info _DSM entries. > > The returned pin-info includes the pin-number in bits 15-8. Add a check > that this matches with the ACPI GPIO resource pin-number in case > the assumption is not true with some ACPI tables. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/intel/int3472/discrete.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c > index 4ef60883154d..c1132bbbff41 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -149,8 +149,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > { > struct int3472_discrete_device *int3472 = data; > struct acpi_resource_gpio *agpio; > + u8 active_value, pin, type; > union acpi_object *obj; > - u8 active_value, type; > const char *err_msg; > const char *func; > u32 polarity; > @@ -174,10 +174,18 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, > return 1; > } > > + /* Bits 7-0 contain the type/function of the pin */ > type = obj->integer.value & 0xff; > > int3472_get_func_and_polarity(type, &func, &polarity); > > + /* Bits 15-8 contain the pin-number on the GPIO chip */ > + pin = (obj->integer.value >> 8) & 0xff; > + if (pin != agpio->pin_table[0]) > + dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n", > + func, agpio->resource_source.string_ptr, pin, > + agpio->pin_table[0]); > + > /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ > active_value = (obj->integer.value >> 24) & 0xff; > if (!active_value) > These changes made me wonder why there aren't defines for the fields? And then FIELD_GET() used to read the field. Most of those comments would be documented by the define name itself.
Hi, On 6/13/23 10:10, Ilpo Järvinen wrote: > On Mon, 12 Jun 2023, Hans de Goede wrote: > >> The INT3472 discrete code assumes that the ACPI GPIO resources are >> in the same order as the pin-info _DSM entries. >> >> The returned pin-info includes the pin-number in bits 15-8. Add a check >> that this matches with the ACPI GPIO resource pin-number in case >> the assumption is not true with some ACPI tables. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/intel/int3472/discrete.c | 10 +++++++++- >> 1 file changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c >> index 4ef60883154d..c1132bbbff41 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -149,8 +149,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> { >> struct int3472_discrete_device *int3472 = data; >> struct acpi_resource_gpio *agpio; >> + u8 active_value, pin, type; >> union acpi_object *obj; >> - u8 active_value, type; >> const char *err_msg; >> const char *func; >> u32 polarity; >> @@ -174,10 +174,18 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, >> return 1; >> } >> >> + /* Bits 7-0 contain the type/function of the pin */ >> type = obj->integer.value & 0xff; >> >> int3472_get_func_and_polarity(type, &func, &polarity); >> >> + /* Bits 15-8 contain the pin-number on the GPIO chip */ >> + pin = (obj->integer.value >> 8) & 0xff; >> + if (pin != agpio->pin_table[0]) >> + dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n", >> + func, agpio->resource_source.string_ptr, pin, >> + agpio->pin_table[0]); >> + >> /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ >> active_value = (obj->integer.value >> 24) & 0xff; >> if (!active_value) >> > > These changes made me wonder why there aren't defines for the fields? > And then FIELD_GET() used to read the field. Most of those comments > would be documented by the define name itself. That is a good idea for v2 I'll add a new 1/2 adding defines + switching the existing cases to FIELD_GET() and I'll also switch to FIELD_GET() here for v2. Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 4ef60883154d..c1132bbbff41 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -149,8 +149,8 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, { struct int3472_discrete_device *int3472 = data; struct acpi_resource_gpio *agpio; + u8 active_value, pin, type; union acpi_object *obj; - u8 active_value, type; const char *err_msg; const char *func; u32 polarity; @@ -174,10 +174,18 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, return 1; } + /* Bits 7-0 contain the type/function of the pin */ type = obj->integer.value & 0xff; int3472_get_func_and_polarity(type, &func, &polarity); + /* Bits 15-8 contain the pin-number on the GPIO chip */ + pin = (obj->integer.value >> 8) & 0xff; + if (pin != agpio->pin_table[0]) + dev_warn(int3472->dev, "%s %s pin number mismatch _DSM %d resource %d\n", + func, agpio->resource_source.string_ptr, pin, + agpio->pin_table[0]); + /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ active_value = (obj->integer.value >> 24) & 0xff; if (!active_value)
The INT3472 discrete code assumes that the ACPI GPIO resources are in the same order as the pin-info _DSM entries. The returned pin-info includes the pin-number in bits 15-8. Add a check that this matches with the ACPI GPIO resource pin-number in case the assumption is not true with some ACPI tables. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/intel/int3472/discrete.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)