| Message ID | 20221216113013.126881-12-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | leds: lookup-table support + int3472/media privacy LED support | expand |
On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede 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, Here and in the code you actually can refer to it as 3rd byte. > 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. ... > + /* 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) Not sure why you need a temporary variable for this. Just use GENMASK()/GENMASK_ULL()? if (obj->integer.value & GENMASK(31, 24)); In this case you even don't need to repeat bit numbers in the comment. > + 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"); Yet another high/low :-)
On Fri, Dec 16, 2022 at 04:57:02PM +0200, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote: > > + /* 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) > > Not sure why you need a temporary variable for this. Just use > GENMASK()/GENMASK_ULL()? > > if (obj->integer.value & GENMASK(31, 24)); Of course should be if (!(obj->integer.value & GENMASK(31, 24))) > In this case you even don't need to repeat bit numbers in the comment. > > > + polarity ^= GPIO_ACTIVE_LOW;
Hi, On 12/16/22 15:57, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede 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, > > Here and in the code you actually can refer to it as 3rd byte. 3th byte or 4th byte? There is no universal convention for numbering bytes in a word, so just using Bits 31-24 is unambiguous. > >> 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. > > ... > >> + /* 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) > > Not sure why you need a temporary variable for this. Just use > GENMASK()/GENMASK_ULL()? > > if (obj->integer.value & GENMASK(31, 24)); > > In this case you even don't need to repeat bit numbers in the comment. These bits contain the value to which the pin should be set when the sensor is active (on), the active_value helper variable IMHO makes this a lot more clear then directly checking the mask. > >> + 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"); > > Yet another high/low :-) Nope, this is the same patch as last time (when you were fine with the other bits...). Regards, Hans
On Fri, Dec 16, 2022 at 05:42:08PM +0100, Hans de Goede wrote: > On 12/16/22 15:57, Andy Shevchenko wrote: > > On Fri, Dec 16, 2022 at 12:30:13PM +0100, Hans de Goede wrote: ... > >> + /* 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) > > > > Not sure why you need a temporary variable for this. Just use > > GENMASK()/GENMASK_ULL()? > > > > if (obj->integer.value & GENMASK(31, 24)); > > > > In this case you even don't need to repeat bit numbers in the comment. > > These bits contain the value to which the pin should be set when the > sensor is active (on), the active_value helper variable IMHO makes this > a lot more clear then directly checking the mask. Mask makes much more clear to understand what bits you are really use without looking at the type of a temporary variable. (IIUC the value is u64.) Maybe active_value = (obj->integer.value & GENMASK(31, 24)) >> 24; But yes, this is on the edge of bikeshedding. > >> + polarity ^= GPIO_ACTIVE_LOW;
diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c index 15a8bff645f7..2b81066b024e 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 = { @@ -106,6 +106,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 and non-active state */ 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 5ec89038ae07..4317c77e53d1 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -122,7 +122,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 b7752c2b798d..96963e30ab6c 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -220,11 +220,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; @@ -248,6 +248,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> --- .../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(-)