| Message ID | 20230127203729.10205-4-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | 5ae20a8050d08a51fef9769cd53237551f259dbe |
| Headers | show |
| Series | int3472/media privacy LED support | expand |
Hi Hans,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc5]
[cannot apply to media-tree/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link: https://lore.kernel.org/r/20230127203729.10205-4-hdegoede%40redhat.com
patch subject: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
config: i386-randconfig-r004-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281537.fKVHsgf4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
# https://github.com/intel-lab-lkp/linux/commit/d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
git checkout d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/platform/x86/intel/int3472/discrete.c:17:
>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type
107 | struct led_lookup_data lookup;
| ^~~~~~
--
In file included from drivers/platform/x86/intel/int3472/led.c:7:
>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type
107 | struct led_lookup_data lookup;
| ^~~~~~
drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_register_pled':
>> drivers/platform/x86/intel/int3472/led.c:57:9: error: implicit declaration of function 'led_add_lookup'; did you mean 'd_can_lookup'? [-Werror=implicit-function-declaration]
57 | led_add_lookup(&int3472->pled.lookup);
| ^~~~~~~~~~~~~~
| d_can_lookup
drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_unregister_pled':
>> drivers/platform/x86/intel/int3472/led.c:71:9: error: implicit declaration of function 'led_remove_lookup' [-Werror=implicit-function-declaration]
71 | led_remove_lookup(&int3472->pled.lookup);
| ^~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
vim +/lookup +107 drivers/platform/x86/intel/int3472/common.h
80
81 struct int3472_discrete_device {
82 struct acpi_device *adev;
83 struct device *dev;
84 struct acpi_device *sensor;
85 const char *sensor_name;
86
87 const struct int3472_sensor_config *sensor_config;
88
89 struct int3472_gpio_regulator {
90 char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
91 char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
92 struct gpio_desc *gpio;
93 struct regulator_dev *rdev;
94 struct regulator_desc rdesc;
95 } regulator;
96
97 struct int3472_gpio_clock {
98 struct clk *clk;
99 struct clk_hw clk_hw;
100 struct clk_lookup *cl;
101 struct gpio_desc *ena_gpio;
102 u32 frequency;
103 } clock;
104
105 struct int3472_pled {
106 struct led_classdev classdev;
> 107 struct led_lookup_data lookup;
108 char name[INT3472_LED_MAX_NAME_LEN];
109 struct gpio_desc *gpio;
110 } pled;
111
112 unsigned int ngpios; /* how many GPIOs have we seen */
113 unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
114 struct gpiod_lookup_table gpios;
115 };
116
Hi, On 1/28/23 08:24, kernel test robot wrote: > Hi Hans, > > I love your patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [also build test ERROR on v6.2-rc5] > [cannot apply to media-tree/master] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch#_base_tree_information] > > url: https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233 > patch link: https://lore.kernel.org/r/20230127203729.10205-4-hdegoede%40redhat.com > patch subject: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED > config: i386-randconfig-r004-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281537.fKVHsgf4-lkp@intel.com/config) > compiler: gcc-11 (Debian 11.3.0-8) 11.3.0 > reproduce (this is a W=1 build): > # https://github.com/intel-lab-lkp/linux/commit/d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233 > git checkout d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d > # save the config file > mkdir build_dir && cp config build_dir/.config > make W=1 O=build_dir ARCH=i386 olddefconfig > make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/platform/ > > If you fix the issue, kindly add following tag where applicable > | Reported-by: kernel test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > In file included from drivers/platform/x86/intel/int3472/discrete.c:17: >>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type > 107 | struct led_lookup_data lookup; > | ^~~~~~ > -- > In file included from drivers/platform/x86/intel/int3472/led.c:7: >>> drivers/platform/x86/intel/int3472/common.h:107:40: error: field 'lookup' has incomplete type > 107 | struct led_lookup_data lookup; > | ^~~~~~ > drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_register_pled': >>> drivers/platform/x86/intel/int3472/led.c:57:9: error: implicit declaration of function 'led_add_lookup'; did you mean 'd_can_lookup'? [-Werror=implicit-function-declaration] > 57 | led_add_lookup(&int3472->pled.lookup); > | ^~~~~~~~~~~~~~ > | d_can_lookup > drivers/platform/x86/intel/int3472/led.c: In function 'skl_int3472_unregister_pled': >>> drivers/platform/x86/intel/int3472/led.c:71:9: error: implicit declaration of function 'led_remove_lookup' [-Werror=implicit-function-declaration] > 71 | led_remove_lookup(&int3472->pled.lookup); > | ^~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors As mentioned in the cover-letter this series depends on this immutable-branch: https://lore.kernel.org/platform-driver-x86/Y9QGcA+9nlmOOy2d@google.com/ That branch not being present in the base used by LKP is what is causing this error. Regards, Hans
Hi Hans,
I love your patch! Yet something to improve:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.2-rc5 next-20230127]
[cannot apply to media-tree/master]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
patch link: https://lore.kernel.org/r/20230127203729.10205-4-hdegoede%40redhat.com
patch subject: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
config: x86_64-randconfig-a012-20230123 (https://download.01.org/0day-ci/archive/20230128/202301281800.sm8woBKh-lkp@intel.com/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Hans-de-Goede/media-v4l2-core-Make-the-v4l2-core-code-enable-disable-the-privacy-LED-if-present/20230128-131233
git checkout d71a1bce9c9ea0bd5b98920b2d72a5b0a36ca19d
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/platform/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from drivers/platform/x86/intel/int3472/discrete.c:17:
>> drivers/platform/x86/intel/int3472/common.h:107:26: error: field has incomplete type 'struct led_lookup_data'
struct led_lookup_data lookup;
^
drivers/platform/x86/intel/int3472/common.h:107:10: note: forward declaration of 'struct led_lookup_data'
struct led_lookup_data lookup;
^
1 error generated.
--
In file included from drivers/platform/x86/intel/int3472/led.c:7:
>> drivers/platform/x86/intel/int3472/common.h:107:26: error: field has incomplete type 'struct led_lookup_data'
struct led_lookup_data lookup;
^
drivers/platform/x86/intel/int3472/common.h:107:10: note: forward declaration of 'struct led_lookup_data'
struct led_lookup_data lookup;
^
>> drivers/platform/x86/intel/int3472/led.c:57:2: error: implicit declaration of function 'led_add_lookup' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
led_add_lookup(&int3472->pled.lookup);
^
>> drivers/platform/x86/intel/int3472/led.c:71:2: error: implicit declaration of function 'led_remove_lookup' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
led_remove_lookup(&int3472->pled.lookup);
^
3 errors generated.
vim +107 drivers/platform/x86/intel/int3472/common.h
80
81 struct int3472_discrete_device {
82 struct acpi_device *adev;
83 struct device *dev;
84 struct acpi_device *sensor;
85 const char *sensor_name;
86
87 const struct int3472_sensor_config *sensor_config;
88
89 struct int3472_gpio_regulator {
90 char regulator_name[GPIO_REGULATOR_NAME_LENGTH];
91 char supply_name[GPIO_REGULATOR_SUPPLY_NAME_LENGTH];
92 struct gpio_desc *gpio;
93 struct regulator_dev *rdev;
94 struct regulator_desc rdesc;
95 } regulator;
96
97 struct int3472_gpio_clock {
98 struct clk *clk;
99 struct clk_hw clk_hw;
100 struct clk_lookup *cl;
101 struct gpio_desc *ena_gpio;
102 u32 frequency;
103 } clock;
104
105 struct int3472_pled {
106 struct led_classdev classdev;
> 107 struct led_lookup_data lookup;
108 char name[INT3472_LED_MAX_NAME_LEN];
109 struct gpio_desc *gpio;
110 } pled;
111
112 unsigned int ngpios; /* how many GPIOs have we seen */
113 unsigned int n_sensor_gpios; /* how many have we mapped to sensor */
114 struct gpiod_lookup_table gpios;
115 };
116
Hi Hans, On Fri, Jan 27, 2023 at 09:37:27PM +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. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > Changes in v4: > - Make struct led_classdev the first member of the pled struct > - Use strchr to replace the : with _ in the acpi_dev_name() > --- > 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 | 58 ++++----------- > drivers/platform/x86/intel/int3472/led.c | 74 +++++++++++++++++++ > 5 files changed, 105 insertions(+), 47 deletions(-) > create mode 100644 drivers/platform/x86/intel/int3472/led.c > > 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 74dc2cff799e..e3b597d93388 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..82dc37e08882 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 { > + struct led_classdev classdev; > + struct led_lookup_data lookup; > + char name[INT3472_LED_MAX_NAME_LEN]; > + 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 708d51f9b41d..38b1372e0745 100644 > --- a/drivers/platform/x86/intel/int3472/discrete.c > +++ b/drivers/platform/x86/intel/int3472/discrete.c > @@ -155,37 +155,21 @@ 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; > - /* Ensure the pin is in output mode and non-active state */ > - gpiod_direction_output(int3472->clock.ena_gpio, 0); > - 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; > - /* Ensure the pin is in output mode and non-active state */ > - gpiod_direction_output(int3472->clock.led_gpio, 0); > - break; > - default: > - dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type); > - break; > - } > + int3472->clock.ena_gpio = gpio; > + /* Ensure the pin is in output mode and non-active state */ > + gpiod_direction_output(int3472->clock.ena_gpio, 0); > > - return 0; > + return skl_int3472_register_clock(int3472); > } > > static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) > @@ -293,11 +277,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); > @@ -341,21 +330,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); > > @@ -372,8 +346,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..251c6524458e > --- /dev/null > +++ b/drivers/platform/x86/intel/int3472/led.c > @@ -0,0 +1,74 @@ > +// 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_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 *p, *path = agpio->resource_source.string_ptr; > + int 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)) > + return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), > + "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)); > + p = strchr(int3472->pled.name, ':'); > + *p = '_'; While I suppose ACPI device names generally are shorter than sizeof(int3472->pled.name), it'd be nice to still check p is non-NULL here, just to be sure. > + > + int3472->pled.classdev.name = int3472->pled.name; > + int3472->pled.classdev.max_brightness = 1; > + int3472->pled.classdev.brightness_set_blocking = int3472_pled_set; > + > + ret = led_classdev_register(int3472->dev, &int3472->pled.classdev); > + if (ret) > + goto err_free_gpio; > + > + int3472->pled.lookup.provider = int3472->pled.name; > + int3472->pled.lookup.dev_id = int3472->sensor_name; > + int3472->pled.lookup.con_id = "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); > +}
Hi, On 1/30/23 11:12, Sakari Ailus wrote: > Hi Hans, > > On Fri, Jan 27, 2023 at 09:37:27PM +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. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> Changes in v4: >> - Make struct led_classdev the first member of the pled struct >> - Use strchr to replace the : with _ in the acpi_dev_name() >> --- >> 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 | 58 ++++----------- >> drivers/platform/x86/intel/int3472/led.c | 74 +++++++++++++++++++ >> 5 files changed, 105 insertions(+), 47 deletions(-) >> create mode 100644 drivers/platform/x86/intel/int3472/led.c >> >> 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 74dc2cff799e..e3b597d93388 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..82dc37e08882 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 { >> + struct led_classdev classdev; >> + struct led_lookup_data lookup; >> + char name[INT3472_LED_MAX_NAME_LEN]; >> + 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 708d51f9b41d..38b1372e0745 100644 >> --- a/drivers/platform/x86/intel/int3472/discrete.c >> +++ b/drivers/platform/x86/intel/int3472/discrete.c >> @@ -155,37 +155,21 @@ 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; >> - /* Ensure the pin is in output mode and non-active state */ >> - gpiod_direction_output(int3472->clock.ena_gpio, 0); >> - 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; >> - /* Ensure the pin is in output mode and non-active state */ >> - gpiod_direction_output(int3472->clock.led_gpio, 0); >> - break; >> - default: >> - dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type); >> - break; >> - } >> + int3472->clock.ena_gpio = gpio; >> + /* Ensure the pin is in output mode and non-active state */ >> + gpiod_direction_output(int3472->clock.ena_gpio, 0); >> >> - return 0; >> + return skl_int3472_register_clock(int3472); >> } >> >> static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) >> @@ -293,11 +277,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); >> @@ -341,21 +330,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); >> >> @@ -372,8 +346,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..251c6524458e >> --- /dev/null >> +++ b/drivers/platform/x86/intel/int3472/led.c >> @@ -0,0 +1,74 @@ >> +// 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_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 *p, *path = agpio->resource_source.string_ptr; >> + int 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)) >> + return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), >> + "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)); >> + p = strchr(int3472->pled.name, ':'); >> + *p = '_'; > > While I suppose ACPI device names generally are shorter than > sizeof(int3472->pled.name), it'd be nice to still check p is non-NULL here, > just to be sure. Sure, I've added a check for this while merging this. Regards, Hans > >> + >> + int3472->pled.classdev.name = int3472->pled.name; >> + int3472->pled.classdev.max_brightness = 1; >> + int3472->pled.classdev.brightness_set_blocking = int3472_pled_set; >> + >> + ret = led_classdev_register(int3472->dev, &int3472->pled.classdev); >> + if (ret) >> + goto err_free_gpio; >> + >> + int3472->pled.lookup.provider = int3472->pled.name; >> + int3472->pled.lookup.dev_id = int3472->sensor_name; >> + int3472->pled.lookup.con_id = "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); >> +} >
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 74dc2cff799e..e3b597d93388 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..82dc37e08882 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 { + struct led_classdev classdev; + struct led_lookup_data lookup; + char name[INT3472_LED_MAX_NAME_LEN]; + 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 708d51f9b41d..38b1372e0745 100644 --- a/drivers/platform/x86/intel/int3472/discrete.c +++ b/drivers/platform/x86/intel/int3472/discrete.c @@ -155,37 +155,21 @@ 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; - /* Ensure the pin is in output mode and non-active state */ - gpiod_direction_output(int3472->clock.ena_gpio, 0); - 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; - /* Ensure the pin is in output mode and non-active state */ - gpiod_direction_output(int3472->clock.led_gpio, 0); - break; - default: - dev_err(int3472->dev, "Invalid GPIO type 0x%02x for clock\n", type); - break; - } + int3472->clock.ena_gpio = gpio; + /* Ensure the pin is in output mode and non-active state */ + gpiod_direction_output(int3472->clock.ena_gpio, 0); - return 0; + return skl_int3472_register_clock(int3472); } static void int3472_get_func_and_polarity(u8 type, const char **func, u32 *polarity) @@ -293,11 +277,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); @@ -341,21 +330,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); @@ -372,8 +346,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..251c6524458e --- /dev/null +++ b/drivers/platform/x86/intel/int3472/led.c @@ -0,0 +1,74 @@ +// 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_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 *p, *path = agpio->resource_source.string_ptr; + int 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)) + return dev_err_probe(int3472->dev, PTR_ERR(int3472->pled.gpio), + "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)); + p = strchr(int3472->pled.name, ':'); + *p = '_'; + + int3472->pled.classdev.name = int3472->pled.name; + int3472->pled.classdev.max_brightness = 1; + int3472->pled.classdev.brightness_set_blocking = int3472_pled_set; + + ret = led_classdev_register(int3472->dev, &int3472->pled.classdev); + if (ret) + goto err_free_gpio; + + int3472->pled.lookup.provider = int3472->pled.name; + int3472->pled.lookup.dev_id = int3472->sensor_name; + int3472->pled.lookup.con_id = "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> --- Changes in v4: - Make struct led_classdev the first member of the pled struct - Use strchr to replace the : with _ in the acpi_dev_name() --- 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 | 58 ++++----------- drivers/platform/x86/intel/int3472/led.c | 74 +++++++++++++++++++ 5 files changed, 105 insertions(+), 47 deletions(-) create mode 100644 drivers/platform/x86/intel/int3472/led.c