diff mbox series

[v6,3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED

Message ID 20230127203729.10205-4-hdegoede@redhat.com
State Accepted
Commit 5ae20a8050d08a51fef9769cd53237551f259dbe
Headers show
Series int3472/media privacy LED support | expand

Commit Message

Hans de Goede Jan. 27, 2023, 8:37 p.m. UTC
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

Comments

kernel test robot Jan. 28, 2023, 7:24 a.m. UTC | #1
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
Hans de Goede Jan. 28, 2023, 9:41 a.m. UTC | #2
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
kernel test robot Jan. 28, 2023, 10:10 a.m. UTC | #3
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
Sakari Ailus Jan. 30, 2023, 10:12 a.m. UTC | #4
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);
> +}
Hans de Goede Jan. 30, 2023, 8:54 p.m. UTC | #5
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 mbox series

Patch

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);
+}