From: Hans de Goede <hdegoede@redhat.com>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: Mark Gross <mgross@linux.intel.com>,
Andy Shevchenko <andy@kernel.org>,
Linus Walleij <linus.walleij@linaro.org>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
Daniel Scally <djrscally@gmail.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
platform-driver-x86@vger.kernel.org, linux-gpio@vger.kernel.org,
Kate Hsuan <hpa@redhat.com>,
Mark Pearson <markpearson@lenovo.com>,
Andy Yeh <andy.yeh@intel.com>, Hao Yao <hao.yao@intel.com>,
linux-media@vger.kernel.org
Subject: Re: [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED
Date: Mon, 30 Jan 2023 21:54:55 +0100 [thread overview]
Message-ID: <900ebfbc-5a8a-7aba-97b4-00dbc194beb5@redhat.com> (raw)
In-Reply-To: <Y9eYKxdo7BvqI9sR@kekkonen.localdomain>
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);
>> +}
>
next prev parent reply other threads:[~2023-01-30 20:55 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-27 20:37 [PATCH v6 0/5] int3472/media privacy LED support Hans de Goede
2023-01-27 20:37 ` [PATCH v6 1/5] media: v4l2-core: Make the v4l2-core code enable/disable the privacy LED if present Hans de Goede
2023-01-28 7:35 ` kernel test robot
2023-01-28 9:41 ` Hans de Goede
2023-01-28 13:42 ` Laurent Pinchart
2023-01-28 13:46 ` Hans de Goede
2023-01-28 8:47 ` kernel test robot
2023-01-30 10:17 ` Sakari Ailus
2023-01-30 21:00 ` Hans de Goede
2023-01-30 22:35 ` Linus Walleij
2023-01-27 20:37 ` [PATCH v6 2/5] platform/x86: int3472/discrete: Refactor GPIO to sensor mapping Hans de Goede
2023-01-27 20:37 ` [PATCH v6 3/5] platform/x86: int3472/discrete: Create a LED class device for the privacy LED Hans de Goede
2023-01-28 7:24 ` kernel test robot
2023-01-28 9:41 ` Hans de Goede
2023-01-28 10:10 ` kernel test robot
2023-01-30 10:12 ` Sakari Ailus
2023-01-30 20:54 ` Hans de Goede [this message]
2023-01-27 20:37 ` [PATCH v6 4/5] platform/x86: int3472/discrete: Move GPIO request to skl_int3472_register_clock() Hans de Goede
2023-01-27 20:37 ` [PATCH v6 5/5] platform/x86: int3472/discrete: Get the polarity from the _DSM entry Hans de Goede
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=900ebfbc-5a8a-7aba-97b4-00dbc194beb5@redhat.com \
--to=hdegoede@redhat.com \
--cc=andy.yeh@intel.com \
--cc=andy@kernel.org \
--cc=djrscally@gmail.com \
--cc=hao.yao@intel.com \
--cc=hpa@redhat.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linus.walleij@linaro.org \
--cc=linux-gpio@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=markpearson@lenovo.com \
--cc=mchehab@kernel.org \
--cc=mgross@linux.intel.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=sakari.ailus@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.