| Message ID | 20221216113013.126881-9-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:10PM +0100, Hans de Goede wrote: > On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad > X1 Nano gen 2 there is no clock-enable pin, triggering the: > "No clk GPIO. The privacy LED won't work" warning and causing the privacy > LED to not work. > > Fix this by modeling the privacy LED as a LED class device rather then > integrating it with the registered clock. > > Note this relies on media subsys changes to actually turn the LED on/off > when the sensor's v4l2_subdev's s_stream() operand gets called. ... > + struct int3472_pled { > + char name[INT3472_LED_MAX_NAME_LEN]; > + struct led_lookup_data lookup; > + struct led_classdev classdev; Why not putting this as a first member in the struct, so any container_of() against it become no-op at compile time? > + struct gpio_desc *gpio; > + } pled; ... > + if (IS_ERR(int3472->pled.gpio)) { > + ret = PTR_ERR(int3472->pled.gpio); > + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); return dev_err_probe(...); > + } ... > + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ > + snprintf(int3472->pled.name, sizeof(int3472->pled.name), > + "%s::privacy_led", acpi_dev_name(int3472->sensor)); > + for (i = 0; int3472->pled.name[i]; i++) { > + if (int3472->pled.name[i] == ':') { > + int3472->pled.name[i] = '_'; > + break; > + } > + } NIH strreplace(). ... > +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) > +{ > + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) > + return; This dups the check inside the _unregister() below, right? > + led_remove_lookup(&int3472->pled.lookup); With list_del_init() I believe the above check can be droped. > + led_classdev_unregister(&int3472->pled.classdev); > + gpiod_put(int3472->pled.gpio); > +}
Hi, On 12/16/22 15:16, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote: >> On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad >> X1 Nano gen 2 there is no clock-enable pin, triggering the: >> "No clk GPIO. The privacy LED won't work" warning and causing the privacy >> LED to not work. >> >> Fix this by modeling the privacy LED as a LED class device rather then >> integrating it with the registered clock. >> >> Note this relies on media subsys changes to actually turn the LED on/off >> when the sensor's v4l2_subdev's s_stream() operand gets called. > > ... > >> + struct int3472_pled { >> + char name[INT3472_LED_MAX_NAME_LEN]; >> + struct led_lookup_data lookup; > >> + struct led_classdev classdev; > > Why not putting this as a first member in the struct, so any container_of() > against it become no-op at compile time? Ack will fix for v4. > >> + struct gpio_desc *gpio; >> + } pled; > > ... > >> + if (IS_ERR(int3472->pled.gpio)) { >> + ret = PTR_ERR(int3472->pled.gpio); >> + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); > > return dev_err_probe(...); That goes over 100 chars. > >> + } > > ... > >> + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >> + snprintf(int3472->pled.name, sizeof(int3472->pled.name), >> + "%s::privacy_led", acpi_dev_name(int3472->sensor)); > >> + for (i = 0; int3472->pled.name[i]; i++) { >> + if (int3472->pled.name[i] == ':') { >> + int3472->pled.name[i] = '_'; >> + break; >> + } >> + } > > NIH strreplace(). Please look more careful, quoting from the strreplace() docs: * strreplace - Replace all occurrences of character in string. Notice the *all* and we only want to replace the first ':' here, because the ':' char has a special meaning in LED class-device-names. > > ... > >> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) >> +{ >> + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) >> + return; > > This dups the check inside the _unregister() below, right? Right. > >> + led_remove_lookup(&int3472->pled.lookup); > > With list_del_init() I believe the above check can be droped. No it cannot, list_del_init() inside led_remove_lookup() would protect against double led_remove_lookup() calls. But here we may have a completely uninitialized list_head on devices without an INT3472 privacy-led, which will trigger either __list_del_entry_valid() errors or lead to NULL pointer derefs. > >> + led_classdev_unregister(&int3472->pled.classdev); >> + gpiod_put(int3472->pled.gpio); >> +} > Regards. Hans
On Fri, Dec 16, 2022 at 05:29:13PM +0100, Hans de Goede wrote: > On 12/16/22 15:16, Andy Shevchenko wrote: > > On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote: ... > >> + if (IS_ERR(int3472->pled.gpio)) { > >> + ret = PTR_ERR(int3472->pled.gpio); > >> + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); > > > > return dev_err_probe(...); > > That goes over 100 chars. The point is you don't need ret to be initialized. Moreover, no-one prevents you to split the line to two. > >> + } ... > >> + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ > >> + snprintf(int3472->pled.name, sizeof(int3472->pled.name), > >> + "%s::privacy_led", acpi_dev_name(int3472->sensor)); > > > >> + for (i = 0; int3472->pled.name[i]; i++) { > >> + if (int3472->pled.name[i] == ':') { > >> + int3472->pled.name[i] = '_'; > >> + break; > >> + } > >> + } > > > > NIH strreplace(). > > Please look more careful, quoting from the strreplace() docs: > > * strreplace - Replace all occurrences of character in string. > > Notice the *all* and we only want to replace the first ':' here, > because the ':' char has a special meaning in LED class-device-names. It's still possible to use that, but anyway, the above is still something NIH. char *p; p = strchr(name, ':'); *p = '_'; But either code has an issue if by some reason you need to check if : is ever present in acpi_dev_name(). The more robust is either to copy acpi_dev_name(), call strreplace(), so you will be sure that _all_ : from ACPI device name will be covered and then attach the rest. ... > >> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) > >> +{ > >> + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) > >> + return; > > > > This dups the check inside the _unregister() below, right? > > Right. > > >> + led_remove_lookup(&int3472->pled.lookup); > > > > With list_del_init() I believe the above check can be droped. > > No it cannot, list_del_init() inside led_remove_lookup() would > protect against double led_remove_lookup() calls. > > But here we may have a completely uninitialized list_head on > devices without an INT3472 privacy-led, which will trigger > either __list_del_entry_valid() errors or lead to NULL > pointer derefs. But we can initialize that as well... > >> + led_classdev_unregister(&int3472->pled.classdev); > >> + gpiod_put(int3472->pled.gpio); > >> +}
Hi, On 12/16/22 18:14, Andy Shevchenko wrote: > On Fri, Dec 16, 2022 at 05:29:13PM +0100, Hans de Goede wrote: >> On 12/16/22 15:16, Andy Shevchenko wrote: >>> On Fri, Dec 16, 2022 at 12:30:10PM +0100, Hans de Goede wrote: > > ... > >>>> + if (IS_ERR(int3472->pled.gpio)) { >>>> + ret = PTR_ERR(int3472->pled.gpio); >>>> + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); >>> >>> return dev_err_probe(...); >> >> That goes over 100 chars. > > The point is you don't need ret to be initialized. Moreover, no-one prevents > you to split the line to two. The compiler is perfectly capable of optimizing away the store in ret if that is not necessary; and splitting the line instead of doing it above will just make the code harder to read. Also this really is bikeshedding... > >>>> + } > > ... > >>>> + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ >>>> + snprintf(int3472->pled.name, sizeof(int3472->pled.name), >>>> + "%s::privacy_led", acpi_dev_name(int3472->sensor)); >>> >>>> + for (i = 0; int3472->pled.name[i]; i++) { >>>> + if (int3472->pled.name[i] == ':') { >>>> + int3472->pled.name[i] = '_'; >>>> + break; >>>> + } >>>> + } >>> >>> NIH strreplace(). >> >> Please look more careful, quoting from the strreplace() docs: >> >> * strreplace - Replace all occurrences of character in string. >> >> Notice the *all* and we only want to replace the first ':' here, >> because the ':' char has a special meaning in LED class-device-names. > > It's still possible to use that, but anyway, the above is still > something NIH. > > char *p; > > p = strchr(name, ':'); > *p = '_'; Ok, In will switch to this for the next version. > But either code has an issue if by some reason you need to check if : is ever > present in acpi_dev_name(). acpi device names are set by this code: result = ida_alloc(instance_ida, GFP_KERNEL); if (result < 0) return result; device->pnp.instance_no = result; dev_set_name(&device->dev, "%s:%02x", acpi_device_bus_id->bus_id, result); And the bus_id cannot have a : in it, so there always is a single :. > > The more robust is either to copy acpi_dev_name(), call strreplace(), so you > will be sure that _all_ : from ACPI device name will be covered and then attach > the rest. > > ... > >>>> +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) >>>> +{ >>>> + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) >>>> + return; >>> >>> This dups the check inside the _unregister() below, right? >> >> Right. >> >>>> + led_remove_lookup(&int3472->pled.lookup); >>> >>> With list_del_init() I believe the above check can be droped. >> >> No it cannot, list_del_init() inside led_remove_lookup() would >> protect against double led_remove_lookup() calls. >> >> But here we may have a completely uninitialized list_head on >> devices without an INT3472 privacy-led, which will trigger >> either __list_del_entry_valid() errors or lead to NULL >> pointer derefs. > > But we can initialize that as well... The standard pattern in the kernel is that INIT_LIST_HEAD() is only used for list_head-s which are actually used as the head of the list. list_head-s used to track members of the list are usually not initialized until they are added to the list. Doing multiple list-init-s in multiple cases, including one in *subsystem core code* just to drop an if here seems counter productive. Also checking that we can move forward with the unregister is a good idea regardless of all the called functions being able to run safely if the register never happened, because future changes to the unregister function might end up doing something which is unsafe when the LED was never registered in the first place. Regards, Hans > >>>> + led_classdev_unregister(&int3472->pled.classdev); >>>> + gpiod_put(int3472->pled.gpio); >>>> +} >
diff --git a/drivers/platform/x86/intel/int3472/Makefile b/drivers/platform/x86/intel/int3472/Makefile index cfec7784c5c9..9f16cb514397 100644 --- a/drivers/platform/x86/intel/int3472/Makefile +++ b/drivers/platform/x86/intel/int3472/Makefile @@ -1,4 +1,4 @@ obj-$(CONFIG_INTEL_SKL_INT3472) += intel_skl_int3472_discrete.o \ intel_skl_int3472_tps68470.o -intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o common.o +intel_skl_int3472_discrete-y := discrete.o clk_and_regulator.o led.o common.o intel_skl_int3472_tps68470-y := tps68470.o tps68470_board_data.o common.o diff --git a/drivers/platform/x86/intel/int3472/clk_and_regulator.c b/drivers/platform/x86/intel/int3472/clk_and_regulator.c index 1cf958983e86..e61119b17677 100644 --- a/drivers/platform/x86/intel/int3472/clk_and_regulator.c +++ b/drivers/platform/x86/intel/int3472/clk_and_regulator.c @@ -23,8 +23,6 @@ static int skl_int3472_clk_prepare(struct clk_hw *hw) struct int3472_gpio_clock *clk = to_int3472_clk(hw); gpiod_set_value_cansleep(clk->ena_gpio, 1); - gpiod_set_value_cansleep(clk->led_gpio, 1); - return 0; } @@ -33,7 +31,6 @@ static void skl_int3472_clk_unprepare(struct clk_hw *hw) struct int3472_gpio_clock *clk = to_int3472_clk(hw); gpiod_set_value_cansleep(clk->ena_gpio, 0); - gpiod_set_value_cansleep(clk->led_gpio, 0); } static int skl_int3472_clk_enable(struct clk_hw *hw) diff --git a/drivers/platform/x86/intel/int3472/common.h b/drivers/platform/x86/intel/int3472/common.h index 53270d19c73a..e106bbfe8ffa 100644 --- a/drivers/platform/x86/intel/int3472/common.h +++ b/drivers/platform/x86/intel/int3472/common.h @@ -6,6 +6,7 @@ #include <linux/clk-provider.h> #include <linux/gpio/machine.h> +#include <linux/leds.h> #include <linux/regulator/driver.h> #include <linux/regulator/machine.h> #include <linux/types.h> @@ -28,6 +29,8 @@ #define GPIO_REGULATOR_NAME_LENGTH 21 #define GPIO_REGULATOR_SUPPLY_NAME_LENGTH 9 +#define INT3472_LED_MAX_NAME_LEN 32 + #define CIO2_SENSOR_SSDB_MCLKSPEED_OFFSET 86 #define INT3472_REGULATOR(_name, _supply, _ops) \ @@ -96,10 +99,16 @@ struct int3472_discrete_device { struct clk_hw clk_hw; struct clk_lookup *cl; struct gpio_desc *ena_gpio; - struct gpio_desc *led_gpio; u32 frequency; } clock; + struct int3472_pled { + char name[INT3472_LED_MAX_NAME_LEN]; + struct led_lookup_data lookup; + struct led_classdev classdev; + struct gpio_desc *gpio; + } pled; + unsigned int ngpios; /* how many GPIOs have we seen */ unsigned int n_sensor_gpios; /* how many have we mapped to sensor */ struct gpiod_lookup_table gpios; @@ -119,4 +128,8 @@ int skl_int3472_register_regulator(struct int3472_discrete_device *int3472, struct acpi_resource_gpio *agpio); void skl_int3472_unregister_regulator(struct int3472_discrete_device *int3472); +int skl_int3472_register_pled(struct int3472_discrete_device *int3472, + struct acpi_resource_gpio *agpio, u32 polarity); +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472); + #endif diff --git a/drivers/platform/x86/intel/int3472/discrete.c b/drivers/platform/x86/intel/int3472/discrete.c index bd3797ce64bf..1a1e0b196bfa 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -155,33 +155,19 @@ static int skl_int3472_map_gpio_to_sensor(struct int3472_discrete_device *int347 } static int skl_int3472_map_gpio_to_clk(struct int3472_discrete_device *int3472, - struct acpi_resource_gpio *agpio, u8 type) + struct acpi_resource_gpio *agpio) { char *path = agpio->resource_source.string_ptr; u16 pin = agpio->pin_table[0]; struct gpio_desc *gpio; - switch (type) { - case INT3472_GPIO_TYPE_CLK_ENABLE: - gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable"); - if (IS_ERR(gpio)) - return (PTR_ERR(gpio)); - - int3472->clock.ena_gpio = gpio; - break; - case INT3472_GPIO_TYPE_PRIVACY_LED: - gpio = acpi_get_and_request_gpiod(path, pin, "int3472,privacy-led"); - if (IS_ERR(gpio)) - return (PTR_ERR(gpio)); + gpio = acpi_get_and_request_gpiod(path, pin, "int3472,clk-enable"); + if (IS_ERR(gpio)) + return (PTR_ERR(gpio)); - int3472->clock.led_gpio = gpio; - break; - default: - dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type); - break; - } + int3472->clock.ena_gpio = gpio; - return 0; + return skl_int3472_register_clock(int3472); } static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) @@ -289,11 +275,16 @@ static int skl_int3472_handle_gpio_resources(struct acpi_resource *ares, break; case INT3472_GPIO_TYPE_CLK_ENABLE: - case INT3472_GPIO_TYPE_PRIVACY_LED: - ret = skl_int3472_map_gpio_to_clk(int3472, agpio, type); + ret = skl_int3472_map_gpio_to_clk(int3472, agpio); if (ret) err_msg = "Failed to map GPIO to clock\n"; + break; + case INT3472_GPIO_TYPE_PRIVACY_LED: + ret = skl_int3472_register_pled(int3472, agpio, polarity); + if (ret) + err_msg = "Failed to register LED\n"; + break; case INT3472_GPIO_TYPE_POWER_ENABLE: ret = skl_int3472_register_regulator(int3472, agpio); @@ -337,21 +328,6 @@ static int skl_int3472_parse_crs(struct int3472_discrete_device *int3472) acpi_dev_free_resource_list(&resource_list); - /* - * If we find no clock enable GPIO pin then the privacy LED won't work. - * We've never seen that situation, but it's possible. Warn the user so - * it's clear what's happened. - */ - if (int3472->clock.ena_gpio) { - ret = skl_int3472_register_clock(int3472); - if (ret) - return ret; - } else { - if (int3472->clock.led_gpio) - dev_warn(int3472->dev, - "No clk GPIO. The privacy LED won't work\n"); - } - int3472->gpios.dev_id = int3472->sensor_name; gpiod_add_lookup_table(&int3472->gpios); @@ -368,8 +344,8 @@ static int skl_int3472_discrete_remove(struct platform_device *pdev) skl_int3472_unregister_clock(int3472); gpiod_put(int3472->clock.ena_gpio); - gpiod_put(int3472->clock.led_gpio); + skl_int3472_unregister_pled(int3472); skl_int3472_unregister_regulator(int3472); return 0; diff --git a/drivers/platform/x86/intel/int3472/led.c b/drivers/platform/x86/intel/int3472/led.c new file mode 100644 index 000000000000..05c58ba23570 --- /dev/null +++ b/drivers/platform/x86/intel/int3472/led.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Author: Hans de Goede <hdegoede@redhat.com> */ + +#include <linux/acpi.h> +#include <linux/gpio/consumer.h> +#include <linux/leds.h> +#include "common.h" + +static int int3472_register_pled_set(struct led_classdev *led_cdev, + enum led_brightness brightness) +{ + struct int3472_discrete_device *int3472 = + container_of(led_cdev, struct int3472_discrete_device, pled.classdev); + + gpiod_set_value_cansleep(int3472->pled.gpio, brightness); + return 0; +} + +int skl_int3472_register_pled(struct int3472_discrete_device *int3472, + struct acpi_resource_gpio *agpio, u32 polarity) +{ + char *path = agpio->resource_source.string_ptr; + int i, ret; + + if (int3472->pled.classdev.dev) + return -EBUSY; + + int3472->pled.gpio = acpi_get_and_request_gpiod(path, agpio->pin_table[0], + "int3472,privacy-led"); + if (IS_ERR(int3472->pled.gpio)) { + ret = PTR_ERR(int3472->pled.gpio); + return dev_err_probe(int3472->dev, ret, "getting privacy LED GPIO\n"); + } + + if (polarity == GPIO_ACTIVE_LOW) + gpiod_toggle_active_low(int3472->pled.gpio); + + /* Ensure the pin is in output mode and non-active state */ + gpiod_direction_output(int3472->pled.gpio, 0); + + /* Generate the name, replacing the ':' in the ACPI devname with '_' */ + snprintf(int3472->pled.name, sizeof(int3472->pled.name), + "%s::privacy_led", acpi_dev_name(int3472->sensor)); + for (i = 0; int3472->pled.name[i]; i++) { + if (int3472->pled.name[i] == ':') { + int3472->pled.name[i] = '_'; + break; + } + } + + int3472->pled.classdev.name = int3472->pled.name; + int3472->pled.classdev.max_brightness = 1; + int3472->pled.classdev.brightness_set_blocking = int3472_register_pled_set; + + ret = led_classdev_register(int3472->dev, &int3472->pled.classdev); + if (ret) + goto err_free_gpio; + + int3472->pled.lookup.led_name = int3472->pled.name; + int3472->pled.lookup.consumer_dev_name = int3472->sensor_name; + int3472->pled.lookup.consumer_function = "privacy-led"; + led_add_lookup(&int3472->pled.lookup); + + return 0; + +err_free_gpio: + gpiod_put(int3472->pled.gpio); + return ret; +} + +void skl_int3472_unregister_pled(struct int3472_discrete_device *int3472) +{ + if (IS_ERR_OR_NULL(int3472->pled.classdev.dev)) + return; + + led_remove_lookup(&int3472->pled.lookup); + led_classdev_unregister(&int3472->pled.classdev); + gpiod_put(int3472->pled.gpio); +}
On some systems, e.g. the Lenovo ThinkPad X1 Yoga gen 7 and the ThinkPad X1 Nano gen 2 there is no clock-enable pin, triggering the: "No clk GPIO. The privacy LED won't work" warning and causing the privacy LED to not work. Fix this by modeling the privacy LED as a LED class device rather then integrating it with the registered clock. Note this relies on media subsys changes to actually turn the LED on/off when the sensor's v4l2_subdev's s_stream() operand gets called. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/intel/int3472/Makefile | 2 +- .../x86/intel/int3472/clk_and_regulator.c | 3 - drivers/platform/x86/intel/int3472/common.h | 15 +++- drivers/platform/x86/intel/int3472/discrete.c | 52 ++++-------- drivers/platform/x86/intel/int3472/led.c | 79 +++++++++++++++++++ 5 files changed, 108 insertions(+), 43 deletions(-) create mode 100644 drivers/platform/x86/intel/int3472/led.c