| Message ID | 20221129231149.697154-7-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ov5693/int3472: Privacy LED handling changes + IPU6 compatibility | expand |
On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote: > > According to: > https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch > > Bits 31-24 of the _DSM pin entry integer value codes the active-value, > that is the actual physical signal (0 or 1) which needs to be output on > the pin to turn the sensor chip on (to make it active). > > So if bits 31-24 are 0 for a reset pin, then the actual value of the reset > pin needs to be 0 to take the chip out of reset. IOW in this case the reset > signal is active-high rather then the default active-low. > > And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk > pin needs to be 0 to enable the clk. So in this case the clk-en signal > is active-low rather then the default active-high. > > IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin > is inverted. > > Add a check for this and also propagate this new polarity to the clock > registration. I like it in this form, thanks! ... > + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); Perhaps static inline str_high_low(bool v) { return v ? "high" : "low"; } In the string_helpers.h? If you are okay with the idea, you may use my tag ahead for that patch.
Hi, On 11/30/22 11:01, Andy Shevchenko wrote: > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote: >> >> According to: >> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch >> >> Bits 31-24 of the _DSM pin entry integer value codes the active-value, >> that is the actual physical signal (0 or 1) which needs to be output on >> the pin to turn the sensor chip on (to make it active). >> >> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset >> pin needs to be 0 to take the chip out of reset. IOW in this case the reset >> signal is active-high rather then the default active-low. >> >> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk >> pin needs to be 0 to enable the clk. So in this case the clk-en signal >> is active-low rather then the default active-high. >> >> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin >> is inverted. >> >> Add a check for this and also propagate this new polarity to the clock >> registration. > > I like it in this form, thanks! > > ... > >> + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); > > Perhaps > > static inline str_high_low(bool v) > { > return v ? "high" : "low"; > } > > In the string_helpers.h? If you are okay with the idea, you may use my > tag ahead for that patch. That is an interesting idea. but IMHO best done in a follow-up series after checking for similar code in the rest of the kernel. This is based on the assumption that "selling" this new helper to the string_helpers.h maintainer will be easier if there are multiple consumers of it right away. Regards, Hans
On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote: > On 11/30/22 11:01, Andy Shevchenko wrote: > > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote: > >> > >> According to: > >> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch > >> > >> Bits 31-24 of the _DSM pin entry integer value codes the active-value, > >> that is the actual physical signal (0 or 1) which needs to be output on > >> the pin to turn the sensor chip on (to make it active). > >> > >> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset > >> pin needs to be 0 to take the chip out of reset. IOW in this case the reset > >> signal is active-high rather then the default active-low. > >> > >> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk > >> pin needs to be 0 to enable the clk. So in this case the clk-en signal > >> is active-low rather then the default active-high. > >> > >> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin > >> is inverted. > >> > >> Add a check for this and also propagate this new polarity to the clock > >> registration. > > > > I like it in this form, thanks! > > > > ... > > > >> + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); > > > > Perhaps > > > > static inline str_high_low(bool v) > > { > > return v ? "high" : "low"; > > } > > > > In the string_helpers.h? If you are okay with the idea, you may use my > > tag ahead for that patch. > > That is an interesting idea. but IMHO best done in a follow-up series > after checking for similar code in the rest of the kernel. > > This is based on the assumption that "selling" this new helper to > the string_helpers.h maintainer will be easier if there are multiple > consumers of it right away. strings_helper.h maintainer is any known kernel subsystem maintainer + me (as a reviewer / main contributor to that library). That's why I proposed the solution and my tag would be enough to route it via your tree. So, offer still stays.
On Wed, Nov 30, 2022 at 01:06:38PM +0200, Andy Shevchenko wrote: > On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote: > > On 11/30/22 11:01, Andy Shevchenko wrote: > > > On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote: ... > > >> + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); > > > > > > Perhaps > > > > > > static inline str_high_low(bool v) > > > { > > > return v ? "high" : "low"; > > > } > > > > > > In the string_helpers.h? If you are okay with the idea, you may use my > > > tag ahead for that patch. > > > > That is an interesting idea. but IMHO best done in a follow-up series > > after checking for similar code in the rest of the kernel. > > > > This is based on the assumption that "selling" this new helper to > > the string_helpers.h maintainer will be easier if there are multiple > > consumers of it right away. > > strings_helper.h maintainer is any known kernel subsystem maintainer + me > (as a reviewer / main contributor to that library). That's why I proposed > the solution and my tag would be enough to route it via your tree. > > So, offer still stays. (You may check last few commits against the header and see the tags)
Hi, On 11/30/22 12:06, Andy Shevchenko wrote: > On Wed, Nov 30, 2022 at 11:39:42AM +0100, Hans de Goede wrote: >> On 11/30/22 11:01, Andy Shevchenko wrote: >>> On Wed, Nov 30, 2022 at 1:12 AM Hans de Goede <hdegoede@redhat.com> wrote: >>>> >>>> According to: >>>> https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch >>>> >>>> Bits 31-24 of the _DSM pin entry integer value codes the active-value, >>>> that is the actual physical signal (0 or 1) which needs to be output on >>>> the pin to turn the sensor chip on (to make it active). >>>> >>>> So if bits 31-24 are 0 for a reset pin, then the actual value of the reset >>>> pin needs to be 0 to take the chip out of reset. IOW in this case the reset >>>> signal is active-high rather then the default active-low. >>>> >>>> And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk >>>> pin needs to be 0 to enable the clk. So in this case the clk-en signal >>>> is active-low rather then the default active-high. >>>> >>>> IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin >>>> is inverted. >>>> >>>> Add a check for this and also propagate this new polarity to the clock >>>> registration. >>> >>> I like it in this form, thanks! >>> >>> ... >>> >>>> + (polarity == GPIO_ACTIVE_HIGH) ? "high" : "low"); >>> >>> Perhaps >>> >>> static inline str_high_low(bool v) >>> { >>> return v ? "high" : "low"; >>> } >>> >>> In the string_helpers.h? If you are okay with the idea, you may use my >>> tag ahead for that patch. >> >> That is an interesting idea. but IMHO best done in a follow-up series >> after checking for similar code in the rest of the kernel. >> >> This is based on the assumption that "selling" this new helper to >> the string_helpers.h maintainer will be easier if there are multiple >> consumers of it right away. > > strings_helper.h maintainer is any known kernel subsystem maintainer + me > (as a reviewer / main contributor to that library). That's why I proposed > the solution and my tag would be enough to route it via your tree. > > So, offer still stays. Ok I'll add your patch adding the str_high_low() helper to the next version of this series (or when I merge it, depending on how things go) and then move this over to use the str_high_low() helper. Regards, Hans
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c index dc4ab7821b56..d56c56287414 100644 --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c @@ -87,7 +87,7 @@ static const struct clk_ops skl_int3472_clock_ops = { }; int skl_int3472_register_clock(struct int3472_discrete_device *int3472, - struct acpi_resource_gpio *agpio) + struct acpi_resource_gpio *agpio, u32 polarity) { char *path = agpio->resource_source.string_ptr; struct clk_init_data init = { @@ -103,6 +103,9 @@ int skl_int3472_register_clock(struct int3472_discrete_device *int3472, return dev_err_probe(int3472->dev, ret, "getting regulator GPIO\n"); } + if (polarity == GPIO_ACTIVE_LOW) + gpiod_toggle_active_low(int3472->clock.ena_gpio); + /* Ensure the pin is in output mode */ gpiod_direction_output(int3472->clock.ena_gpio, 0); diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h index 434591313762..fb02fbcc18b7 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -112,7 +112,7 @@ int skl_int3472_get_sensor_adev_and_name(struct device *dev, const char **name_ret); int skl_int3472_register_clock(struct int3472_discrete_device *int3472, - struct acpi_resource_gpio *agpio); + struct acpi_resource_gpio *agpio, u32 polarity); void skl_int3472_unregister_clock(struct int3472_discrete_device *int3472); int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index 6f5fef11cfb7..321551ef4519 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -219,11 +219,11 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, struct int3472_discrete_device *int3472 = data; struct acpi_resource_gpio *agpio; union acpi_object *obj; + u8 active_value, type; const char *err_msg; const char *func; u32 polarity; int ret; - u8 type; if (!acpi_gpio_get_io_resource(ares, &agpio)) return 1; @@ -247,6 +247,15 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, int3472_get_func_and_polarity(type, &func, &polarity); + /* If bits 31-24 of the _DSM entry are all 0 then the signal is inverted */ + active_value = obj->integer.value >> 24; + if (!active_value) + polarity ^= GPIO_ACTIVE_LOW; + + 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: @@ -257,7 +266,7 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, break; case INT3472_GPIO_TYPE_CLK_ENABLE: - ret = skl_int3472_register_clock(int3472, agpio); + ret = skl_int3472_register_clock(int3472, agpio, polarity); if (ret) err_msg = "Failed to register clock\n";
According to: https://github.com/intel/ipu6-drivers/blob/master/patch/int3472-support-independent-clock-and-LED-gpios-5.17%2B.patch Bits 31-24 of the _DSM pin entry integer value codes the active-value, that is the actual physical signal (0 or 1) which needs to be output on the pin to turn the sensor chip on (to make it active). So if bits 31-24 are 0 for a reset pin, then the actual value of the reset pin needs to be 0 to take the chip out of reset. IOW in this case the reset signal is active-high rather then the default active-low. And if bits 31-24 are 0 for a clk-en pin then the actual value of the clk pin needs to be 0 to enable the clk. So in this case the clk-en signal is active-low rather then the default active-high. IOW if bits 31-24 are 0 for a pin, then the default polarity of the pin is inverted. Add a check for this and also propagate this new polarity to the clock registration. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- This patch is deliberately the last patch in the series, because this patch could do with some more testing. By making it last it can be applied later then the rest of the series. --- .../platform/x86/intel/int3472/clk_and_regulator.c | 5 ++++- drivers/platform/x86/intel/int3472/common.h | 2 +- drivers/platform/x86/intel/int3472/discrete.c | 13 +++++++++++-- 3 files changed, 16 insertions(+), 4 deletions(-)