[PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Hans de Goede]

Hi All,

While working on the earlier discussed changes for dealing with
the privacy LED for "discrete" INT3472 ACPI devices I was wondering
"how are we dealing with the privacy LED when the INT3472 ACPI device
is a tps68470?".

Well it turns out we were not dealing with this at all, leading to
the privacy LED on the back of the Surface Go series not lighting up
when the back camera is on.

This series fixes this, it consists of:

Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
Patch4:      Add support for a privacy LED to the ov8865 sensor driver
Patch5:      Add gpio-lookup table entry for the privacy LED.

There is one small issue here, I believe that patches 1-3 need to land before
4 + 5 do. Once 4 + 5 have landed the ov8865 driver will try to get a
GPIO with pin number 10 from the gpio-tps68470 provider and without patch 3,
that will fail because only pins 0-9 exist until patch 3 lands.

The easiest way to avoid this issue would be for me to merge patches 1-3 +
5 through the pdx86 tree. GPIO subsystem maintainers, may I have your ack
for this ?

Note patch 4 is not a problem without patch 5, it uses gpiod_get_optional,
so as long as there is no lookup entry for a "privacy-led" GPIO it is
a no-op.

Regards,

Hans



Hans de Goede (5):
  gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong
    register
  gpio: tps68470: Make tps68470_gpio_output() always set the initial
    value
  gpio: tps68470: Add support for the indicator LED outputs
  media: ov8865: Add support for a privacy-led GPIO
  platform/x86: int3472: Add support for the back privacy LED on Surface
    Go models

 drivers/gpio/gpio-tps68470.c                  | 52 +++++++++++++------
 drivers/media/i2c/ov8865.c                    |  9 ++++
 .../x86/intel/int3472/tps68470_board_data.c   |  1 +
 include/linux/mfd/tps68470.h                  |  4 ++
 4 files changed, 49 insertions(+), 17 deletions(-)

-- 
2.38.1

[PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Hans de Goede]

For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
so that the actual value of the pin is read, rather then the value the pin
would output when put in output mode.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpio-tps68470.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
index aaddcabe9b35..778a72cf800c 100644
--- a/drivers/gpio/gpio-tps68470.c
+++ b/drivers/gpio/gpio-tps68470.c
@@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
 	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
-	unsigned int reg = TPS68470_REG_GPDO;
+	unsigned int reg = TPS68470_REG_GPDI;
 	int val, ret;
 
 	if (offset >= TPS68470_N_REGULAR_GPIO) {
-- 
2.38.1

[PATCH 2/5] gpio: tps68470: Make tps68470_gpio_output() always set the initial value

[Hans de Goede]

Make tps68470_gpio_output() call tps68470_gpio_set() for output-only pins
too, so that the initial value passed to gpiod_direction_output() is
honored for these pins too.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpio-tps68470.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
index 778a72cf800c..2ca86fbe1d84 100644
--- a/drivers/gpio/gpio-tps68470.c
+++ b/drivers/gpio/gpio-tps68470.c
@@ -91,13 +91,13 @@ static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
 	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
 	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
 
+	/* Set the initial value */
+	tps68470_gpio_set(gc, offset, value);
+
 	/* rest are always outputs */
 	if (offset >= TPS68470_N_REGULAR_GPIO)
 		return 0;
 
-	/* Set the initial value */
-	tps68470_gpio_set(gc, offset, value);
-
 	return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
 				 TPS68470_GPIO_MODE_MASK,
 				 TPS68470_GPIO_MODE_OUT_CMOS);
-- 
2.38.1

[PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Hans de Goede]

The tps68470 has support for 2 indicator LED outputs called
ileda and iledb, at support for these as GPIO pins 10 + 11.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/gpio-tps68470.c | 46 +++++++++++++++++++++++++-----------
 include/linux/mfd/tps68470.h |  4 ++++
 2 files changed, 36 insertions(+), 14 deletions(-)

diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
index 2ca86fbe1d84..33febd96c001 100644
--- a/drivers/gpio/gpio-tps68470.c
+++ b/drivers/gpio/gpio-tps68470.c
@@ -19,24 +19,43 @@
 
 #define TPS68470_N_LOGIC_OUTPUT	3
 #define TPS68470_N_REGULAR_GPIO	7
-#define TPS68470_N_GPIO	(TPS68470_N_LOGIC_OUTPUT + TPS68470_N_REGULAR_GPIO)
+#define TPS68470_N_ILEDS 2
+#define TPS68470_N_GPIO	(TPS68470_N_LOGIC_OUTPUT + TPS68470_N_REGULAR_GPIO + TPS68470_N_ILEDS)
 
 struct tps68470_gpio_data {
 	struct regmap *tps68470_regmap;
 	struct gpio_chip gc;
 };
 
+static void tps68470_gpio_get_reg_and_mask(bool get, unsigned int offset,
+					   unsigned int *reg, int *mask)
+{
+	if (offset < TPS68470_N_REGULAR_GPIO) {
+		if (get)
+			*reg = TPS68470_REG_GPDI;
+		else
+			*reg = TPS68470_REG_GPDO;
+		*mask = BIT(offset);
+	} else if (offset < (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT)) {
+		*reg = TPS68470_REG_SGPO;
+		*mask = BIT(offset - TPS68470_N_REGULAR_GPIO);
+	} else {
+		*reg = TPS68470_REG_ILEDCTL;
+		if (offset == (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT))
+			*mask = TPS68470_ILEDCTL_ENA;
+		else
+			*mask = TPS68470_ILEDCTL_ENB;
+	}
+}
+
 static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
 {
 	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
 	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
-	unsigned int reg = TPS68470_REG_GPDI;
-	int val, ret;
+	int val, mask, ret;
+	unsigned int reg;
 
-	if (offset >= TPS68470_N_REGULAR_GPIO) {
-		offset -= TPS68470_N_REGULAR_GPIO;
-		reg = TPS68470_REG_SGPO;
-	}
+	tps68470_gpio_get_reg_and_mask(true, offset, &reg, &mask);
 
 	ret = regmap_read(regmap, reg, &val);
 	if (ret) {
@@ -44,7 +63,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
 			TPS68470_REG_SGPO);
 		return ret;
 	}
-	return !!(val & BIT(offset));
+	return !!(val & mask);
 }
 
 static int tps68470_gpio_get_direction(struct gpio_chip *gc,
@@ -75,14 +94,12 @@ static void tps68470_gpio_set(struct gpio_chip *gc, unsigned int offset,
 {
 	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
 	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
-	unsigned int reg = TPS68470_REG_GPDO;
+	unsigned int reg;
+	int mask;
 
-	if (offset >= TPS68470_N_REGULAR_GPIO) {
-		reg = TPS68470_REG_SGPO;
-		offset -= TPS68470_N_REGULAR_GPIO;
-	}
+	tps68470_gpio_get_reg_and_mask(false, offset, &reg, &mask);
 
-	regmap_update_bits(regmap, reg, BIT(offset), value ? BIT(offset) : 0);
+	regmap_update_bits(regmap, reg, mask, value ? mask : 0);
 }
 
 static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
@@ -120,6 +137,7 @@ static const char *tps68470_names[TPS68470_N_GPIO] = {
 	"gpio.0", "gpio.1", "gpio.2", "gpio.3",
 	"gpio.4", "gpio.5", "gpio.6",
 	"s_enable", "s_idle", "s_resetn",
+	"ileda", "iledb",
 };
 
 static int tps68470_gpio_probe(struct platform_device *pdev)
diff --git a/include/linux/mfd/tps68470.h b/include/linux/mfd/tps68470.h
index 7807fa329db0..2a1b6de65540 100644
--- a/include/linux/mfd/tps68470.h
+++ b/include/linux/mfd/tps68470.h
@@ -34,6 +34,7 @@
 #define TPS68470_REG_SGPO		0x22
 #define TPS68470_REG_GPDI		0x26
 #define TPS68470_REG_GPDO		0x27
+#define TPS68470_REG_ILEDCTL		0x28
 #define TPS68470_REG_VCMVAL		0x3C
 #define TPS68470_REG_VAUX1VAL		0x3D
 #define TPS68470_REG_VAUX2VAL		0x3E
@@ -94,4 +95,7 @@
 #define TPS68470_GPIO_MODE_OUT_CMOS	2
 #define TPS68470_GPIO_MODE_OUT_ODRAIN	3
 
+#define TPS68470_ILEDCTL_ENA		BIT(2)
+#define TPS68470_ILEDCTL_ENB		BIT(6)
+
 #endif /* __LINUX_MFD_TPS68470_H */
-- 
2.38.1

[PATCH 4/5] media: ov8865: Add support for a privacy-led GPIO

[Hans de Goede]

Add support for a privacy-led GPIO.

Making the privacy LED to controlable from userspace, as using the LED
class subsystem would do, would make it too easy for spy-ware to disable
the LED.

To avoid this have the sensor driver directly control the LED.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Note an additional advantage of directly controlling the GPIO is that
GPIOs are tied directly to consumer devices. Where as with a LED class
device, there would need to be some mechanism to tie the right LED
(e.g front or back) to the right sensor.
---
 drivers/media/i2c/ov8865.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ov8865.c b/drivers/media/i2c/ov8865.c
index cae1866134a0..fe86b166b28b 100644
--- a/drivers/media/i2c/ov8865.c
+++ b/drivers/media/i2c/ov8865.c
@@ -688,6 +688,7 @@ struct ov8865_sensor {
 	struct i2c_client *i2c_client;
 	struct gpio_desc *reset;
 	struct gpio_desc *powerdown;
+	struct gpio_desc *privacy_led;
 	struct regulator *avdd;
 	struct regulator *dvdd;
 	struct regulator *dovdd;
@@ -2432,10 +2433,12 @@ static int ov8865_sensor_power(struct ov8865_sensor *sensor, bool on)
 
 		gpiod_set_value_cansleep(sensor->reset, 0);
 		gpiod_set_value_cansleep(sensor->powerdown, 0);
+		gpiod_set_value_cansleep(sensor->privacy_led, 1);
 
 		/* Time to enter streaming mode according to power timings. */
 		usleep_range(10000, 12000);
 	} else {
+		gpiod_set_value_cansleep(sensor->privacy_led, 0);
 		gpiod_set_value_cansleep(sensor->powerdown, 1);
 		gpiod_set_value_cansleep(sensor->reset, 1);
 
@@ -3009,6 +3012,12 @@ static int ov8865_probe(struct i2c_client *client)
 		goto error_endpoint;
 	}
 
+	sensor->privacy_led = devm_gpiod_get_optional(dev, "privacy-led", GPIOD_OUT_LOW);
+	if (IS_ERR(sensor->privacy_led)) {
+		ret = PTR_ERR(sensor->privacy_led);
+		goto error_endpoint;
+	}
+
 	/* External Clock */
 
 	sensor->extclk = devm_clk_get(dev, NULL);
-- 
2.38.1

[PATCH 5/5] platform/x86: int3472: Add support for the back privacy LED on Surface Go models

[Hans de Goede]

The back camera on the Surface Go series has a privacy LED too,
this is connected to the indicator-LED-A output of the tps68470 PMIC.

Add a GPIO mapping for this. Together with the patches to add support
for the indicator-LED outputs to the gpio-tps68470 driver + the patch to
add privacy LED support to the ov8865 drivers this fixes the back privacy
LED on the Surface Go models not turning on when the back camera is
active.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/platform/x86/intel/int3472/tps68470_board_data.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/platform/x86/intel/int3472/tps68470_board_data.c b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
index 309eab9c0558..ac40fdc13876 100644
--- a/drivers/platform/x86/intel/int3472/tps68470_board_data.c
+++ b/drivers/platform/x86/intel/int3472/tps68470_board_data.c
@@ -134,6 +134,7 @@ static struct gpiod_lookup_table surface_go_int347a_gpios = {
 	.table = {
 		GPIO_LOOKUP("tps68470-gpio", 9, "reset", GPIO_ACTIVE_LOW),
 		GPIO_LOOKUP("tps68470-gpio", 7, "powerdown", GPIO_ACTIVE_LOW),
+		GPIO_LOOKUP("tps68470-gpio", 10, "privacy-led", GPIO_ACTIVE_HIGH),
 		{ }
 	}
 };
-- 
2.38.1

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
> so that the actual value of the pin is read, rather then the value the pin

than

> would output when put in output mode.

I don't see it here and haven't checked the context, but the idea is
to check the direction and return either input (if pin is in input
mode) or [cached] output.If it's the case, the patch looks good to me.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpio/gpio-tps68470.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index aaddcabe9b35..778a72cf800c 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
>  {
>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
> -       unsigned int reg = TPS68470_REG_GPDO;
> +       unsigned int reg = TPS68470_REG_GPDI;
>         int val, ret;
>
>         if (offset >= TPS68470_N_REGULAR_GPIO) {
> --
> 2.38.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] gpio: tps68470: Make tps68470_gpio_output() always set the initial value

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Make tps68470_gpio_output() call tps68470_gpio_set() for output-only pins
> too, so that the initial value passed to gpiod_direction_output() is
> honored for these pins too.

This smells like a fix. Shouldn't be the Fixes tag?

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpio/gpio-tps68470.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index 778a72cf800c..2ca86fbe1d84 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -91,13 +91,13 @@ static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
>
> +       /* Set the initial value */
> +       tps68470_gpio_set(gc, offset, value);
> +
>         /* rest are always outputs */
>         if (offset >= TPS68470_N_REGULAR_GPIO)
>                 return 0;
>
> -       /* Set the initial value */
> -       tps68470_gpio_set(gc, offset, value);
> -
>         return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
>                                  TPS68470_GPIO_MODE_MASK,
>                                  TPS68470_GPIO_MODE_OUT_CMOS);
> --
> 2.38.1
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Andy Shevchenko]

On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> The tps68470 has support for 2 indicator LED outputs called
> ileda and iledb, at support for these as GPIO pins 10 + 11.

add ?

...

> +static void tps68470_gpio_get_reg_and_mask(bool get, unsigned int offset,
> +                                          unsigned int *reg, int *mask)

Hmm... Usual way is to put the get/set flag at the end of the list of
parameters.
Also why not naming it as 'dir' to avoid confusion with the _get in
the function name?

> +{
> +       if (offset < TPS68470_N_REGULAR_GPIO) {
> +               if (get)
> +                       *reg = TPS68470_REG_GPDI;
> +               else
> +                       *reg = TPS68470_REG_GPDO;
> +               *mask = BIT(offset);
> +       } else if (offset < (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT)) {
> +               *reg = TPS68470_REG_SGPO;
> +               *mask = BIT(offset - TPS68470_N_REGULAR_GPIO);
> +       } else {
> +               *reg = TPS68470_REG_ILEDCTL;
> +               if (offset == (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT))
> +                       *mask = TPS68470_ILEDCTL_ENA;
> +               else
> +                       *mask = TPS68470_ILEDCTL_ENB;
> +       }
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Hans de Goede]

Hi,

On 11/29/22 11:22, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
>> so that the actual value of the pin is read, rather then the value the pin
> 
> than

Ack.

>> would output when put in output mode.
> 
> I don't see it here and haven't checked the context, but the idea is
> to check the direction and return either input (if pin is in input
> mode) or [cached] output.If it's the case, the patch looks good to me.

No the idea is to always actually use the input register when reading
the pins, independent of the input/output mode. Instead of always
reading the [cached] output register value.

The input buffer will still work when the device is in output mode
and if somehow an external force is pushing the pin low/high against
the output value then the input buffer will correctly reflect this
(assuming the output is current limited and thus the magic smoke
stays inside the chip).

Regards,

Hans



> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpio/gpio-tps68470.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
>> index aaddcabe9b35..778a72cf800c 100644
>> --- a/drivers/gpio/gpio-tps68470.c
>> +++ b/drivers/gpio/gpio-tps68470.c
>> @@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
>>  {
>>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
>> -       unsigned int reg = TPS68470_REG_GPDO;
>> +       unsigned int reg = TPS68470_REG_GPDI;
>>         int val, ret;
>>
>>         if (offset >= TPS68470_N_REGULAR_GPIO) {
>> --
>> 2.38.1
>>
> 
> 

Re: [PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Hans de Goede]

Hi Andy,

Thank you for the reviews.

On 11/29/22 11:28, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> The tps68470 has support for 2 indicator LED outputs called
>> ileda and iledb, at support for these as GPIO pins 10 + 11.
> 
> add ?

This models ileda and iledb outputs *as* GPIO pins 10 + 11
on the linux gpiochip.

But yes it also adds gpio pins 10 + 11 to the gpiochip, so
either one works I guess :)

> 
> ...
> 
>> +static void tps68470_gpio_get_reg_and_mask(bool get, unsigned int offset,
>> +                                          unsigned int *reg, int *mask)
> 
> Hmm... Usual way is to put the get/set flag at the end of the list of
> parameters.

For functions returning values by reference I always follow the
pattern of input parameters first, then output parameters.

> Also why not naming it as 'dir' to avoid confusion with the _get in
> the function name?

Because dir is meaningless without an enum to to define what a dir
of 0/false means. Where as get is clear without such an enum.
get is set to true when this function is called from
tps68470_gpio_get() and false when it is called from
tps68470_gpio_set(). It does not get more straight forward then that.

Regards,

Hans




> 
>> +{
>> +       if (offset < TPS68470_N_REGULAR_GPIO) {
>> +               if (get)
>> +                       *reg = TPS68470_REG_GPDI;
>> +               else
>> +                       *reg = TPS68470_REG_GPDO;
>> +               *mask = BIT(offset);
>> +       } else if (offset < (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT)) {
>> +               *reg = TPS68470_REG_SGPO;
>> +               *mask = BIT(offset - TPS68470_N_REGULAR_GPIO);
>> +       } else {
>> +               *reg = TPS68470_REG_ILEDCTL;
>> +               if (offset == (TPS68470_N_REGULAR_GPIO + TPS68470_N_LOGIC_OUTPUT))
>> +                       *mask = TPS68470_ILEDCTL_ENA;
>> +               else
>> +                       *mask = TPS68470_ILEDCTL_ENB;
>> +       }
>> +}
> 

Re: [PATCH 2/5] gpio: tps68470: Make tps68470_gpio_output() always set the initial value

[Hans de Goede]

Hi,

On 11/29/22 11:24, Andy Shevchenko wrote:
> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Make tps68470_gpio_output() call tps68470_gpio_set() for output-only pins
>> too, so that the initial value passed to gpiod_direction_output() is
>> honored for these pins too.
> 
> This smells like a fix. Shouldn't be the Fixes tag?

Ack, and the same really goes for 1/3 too. I'll add a fixed tag
there for v2 too.

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks.

Regards,

Hans


> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/gpio/gpio-tps68470.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
>> index 778a72cf800c..2ca86fbe1d84 100644
>> --- a/drivers/gpio/gpio-tps68470.c
>> +++ b/drivers/gpio/gpio-tps68470.c
>> @@ -91,13 +91,13 @@ static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
>>         struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>>         struct regmap *regmap = tps68470_gpio->tps68470_regmap;
>>
>> +       /* Set the initial value */
>> +       tps68470_gpio_set(gc, offset, value);
>> +
>>         /* rest are always outputs */
>>         if (offset >= TPS68470_N_REGULAR_GPIO)
>>                 return 0;
>>
>> -       /* Set the initial value */
>> -       tps68470_gpio_set(gc, offset, value);
>> -
>>         return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
>>                                  TPS68470_GPIO_MODE_MASK,
>>                                  TPS68470_GPIO_MODE_OUT_CMOS);
>> --
>> 2.38.1
>>
> 
> 

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/29/22 11:22, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
> >> so that the actual value of the pin is read, rather then the value the pin
> >
> > than
>
> Ack.
>
> >> would output when put in output mode.
> >
> > I don't see it here and haven't checked the context, but the idea is
> > to check the direction and return either input (if pin is in input
> > mode) or [cached] output.If it's the case, the patch looks good to me.
>
> No the idea is to always actually use the input register when reading
> the pins, independent of the input/output mode. Instead of always
> reading the [cached] output register value.

But why? This makes a little sense to me.

> The input buffer will still work when the device is in output mode

Does this hardware support HiZ?

> and if somehow an external force is pushing the pin low/high against
> the output value then the input buffer will correctly reflect this
> (assuming the output is current limited and thus the magic smoke
> stays inside the chip).

Exactly, when smoke comes out, the hardware is broken and code,
whatever is it, makes no difference at all.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 1:32 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 11/29/22 11:28, Andy Shevchenko wrote:
> > On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>
> >> The tps68470 has support for 2 indicator LED outputs called
> >> ileda and iledb, at support for these as GPIO pins 10 + 11.
> >
> > add ?
>
> This models ileda and iledb outputs *as* GPIO pins 10 + 11
> on the linux gpiochip.
>
> But yes it also adds gpio pins 10 + 11 to the gpiochip, so
> either one works I guess :)

I had to be a bit more precise. 'at support'?! Perhaps it should be
'add support'?

...

> >> +static void tps68470_gpio_get_reg_and_mask(bool get, unsigned int offset,
> >> +                                          unsigned int *reg, int *mask)
> >
> > Hmm... Usual way is to put the get/set flag at the end of the list of
> > parameters.
>
> For functions returning values by reference I always follow the
> pattern of input parameters first, then output parameters.
>
> > Also why not naming it as 'dir' to avoid confusion with the _get in
> > the function name?
>
> Because dir is meaningless without an enum to to define what a dir
> of 0/false means. Where as get is clear without such an enum.
> get is set to true when this function is called from
> tps68470_gpio_get() and false when it is called from
> tps68470_gpio_set(). It does not get more straight forward then that.

But it's about the buffer (in hw sense) to read value from. What about
naming it out_or_in?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Hans de Goede]

Hi,

On 11/29/22 12:56, Andy Shevchenko wrote:
> On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/29/22 11:22, Andy Shevchenko wrote:
>>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
>>>> so that the actual value of the pin is read, rather then the value the pin
>>>
>>> than
>>
>> Ack.
>>
>>>> would output when put in output mode.
>>>
>>> I don't see it here and haven't checked the context, but the idea is
>>> to check the direction and return either input (if pin is in input
>>> mode) or [cached] output.If it's the case, the patch looks good to me.
>>
>> No the idea is to always actually use the input register when reading
>> the pins, independent of the input/output mode. Instead of always
>> reading the [cached] output register value.
> 
> But why? This makes a little sense to me.

I don't understand what your problem is with this patch ?

This is standard behavior for GPIO drivers, the get() callback
always reads the actual pin values when there is a registers
to get the actual pin-values. AFAIK this is no different from what
countless other GPIO drivers do ?

>> The input buffer will still work when the device is in output mode
> 
> Does this hardware support HiZ?

Yes the 7 standard GPIO pins can be put in input mode, aka HiZ mode.

>> and if somehow an external force is pushing the pin low/high against
>> the output value then the input buffer will correctly reflect this
>> (assuming the output is current limited and thus the magic smoke
>> stays inside the chip).
> 
> Exactly, when smoke comes out, the hardware is broken and code,
> whatever is it, makes no difference at all.

The GPIO part of the TPS68470 supports open-drain outputs, to correctly
get the actual value on the pin from the get() callback, the GPDI
register must be used. And being able to detect some outside chip
forcing the line low in open-drain mode is important to be able to
e.g. implement a software I2C master.

As I mentioned above actually using the input buffer value in
the get() method is standard behavior for GPIO drivers, exactly
for reasons like allowing a sw I2C master implementation to
detect clock stretching or conflicts (in the multi master case).

I really don't understand what is so controversial about this
patch?

Note the datasheet describes the GPDO / GPDI bit 0  values as:

GPDO bit 0: "Control of the GPIO0 output"
GPDI bit 0: "State of the GPIO0 input"

So clearly GPDI is the correct register to use for the get()
callback, which is what this patch is doing.

Regards,

Hans

Re: [PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Hans de Goede]

Hi,

On 11/29/22 12:59, Andy Shevchenko wrote:
> On Tue, Nov 29, 2022 at 1:32 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 11/29/22 11:28, Andy Shevchenko wrote:
>>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>
>>>> The tps68470 has support for 2 indicator LED outputs called
>>>> ileda and iledb, at support for these as GPIO pins 10 + 11.
>>>
>>> add ?
>>
>> This models ileda and iledb outputs *as* GPIO pins 10 + 11
>> on the linux gpiochip.
>>
>> But yes it also adds gpio pins 10 + 11 to the gpiochip, so
>> either one works I guess :)
> 
> I had to be a bit more precise. 'at support'?! Perhaps it should be
> 'add support'?
> 
> ...
> 
>>>> +static void tps68470_gpio_get_reg_and_mask(bool get, unsigned int offset,
>>>> +                                          unsigned int *reg, int *mask)
>>>
>>> Hmm... Usual way is to put the get/set flag at the end of the list of
>>> parameters.
>>
>> For functions returning values by reference I always follow the
>> pattern of input parameters first, then output parameters.
>>
>>> Also why not naming it as 'dir' to avoid confusion with the _get in
>>> the function name?
>>
>> Because dir is meaningless without an enum to to define what a dir
>> of 0/false means. Where as get is clear without such an enum.
>> get is set to true when this function is called from
>> tps68470_gpio_get() and false when it is called from
>> tps68470_gpio_set(). It does not get more straight forward then that.
> 
> But it's about the buffer (in hw sense) to read value from. What about
> naming it out_or_in?

"out or in" still does not make clear what a value of true means
does true mean out or does it mean in ? 

I'm sorry, but just like with your comments on patch 1/3
I really don't see the problem here. Unlike "dir" or "out_or_in"
get is a perfectly fine parameter name which is actually clearly
defined by the parameter-name itself.

And the function + the callers are all in the same file, so
anyone reading the code can easily deduce the meaning from
the code.

Regards,

Hans

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 01:19:57PM +0100, Hans de Goede wrote:
> On 11/29/22 12:56, Andy Shevchenko wrote:
> > On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 11/29/22 11:22, Andy Shevchenko wrote:
> >>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>>
> >>>> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
> >>>> so that the actual value of the pin is read, rather then the value the pin
> >>>
> >>> than
> >>
> >> Ack.
> >>
> >>>> would output when put in output mode.
> >>>
> >>> I don't see it here and haven't checked the context, but the idea is
> >>> to check the direction and return either input (if pin is in input
> >>> mode) or [cached] output.If it's the case, the patch looks good to me.
> >>
> >> No the idea is to always actually use the input register when reading
> >> the pins, independent of the input/output mode. Instead of always
> >> reading the [cached] output register value.
> > 
> > But why? This makes a little sense to me.
> 
> I don't understand what your problem is with this patch ?
> 
> This is standard behavior for GPIO drivers, the get() callback
> always reads the actual pin values when there is a registers
> to get the actual pin-values. AFAIK this is no different from what
> countless other GPIO drivers do ?

If the GPIO is requested is output and the pin in the HiZ mode, what value
should you return?

> >> The input buffer will still work when the device is in output mode
> > 
> > Does this hardware support HiZ?
> 
> Yes the 7 standard GPIO pins can be put in input mode, aka HiZ mode.

No, input and HiZ are two different states. If the hardware doesn't
support the latter, then your patch doesn't make any difference indeed, except
involving "in" buffer for "out" value. Which I consider an unneeded
step instead of reading [cached] "out" value.

[cached] here is used in a broader sense: either SW cache, or read back value
from the GPIO out register (not all hardware support that).

> >> and if somehow an external force is pushing the pin low/high against
> >> the output value then the input buffer will correctly reflect this
> >> (assuming the output is current limited and thus the magic smoke
> >> stays inside the chip).
> > 
> > Exactly, when smoke comes out, the hardware is broken and code,
> > whatever is it, makes no difference at all.
> 
> The GPIO part of the TPS68470 supports open-drain outputs, to correctly
> get the actual value on the pin from the get() callback, the GPDI
> register must be used. And being able to detect some outside chip
> forcing the line low in open-drain mode is important to be able to
> e.g. implement a software I2C master.

Right, but for push-pull mode this is not needed, correct?

> As I mentioned above actually using the input buffer value in
> the get() method is standard behavior for GPIO drivers, exactly
> for reasons like allowing a sw I2C master implementation to
> detect clock stretching or conflicts (in the multi master case).

> I really don't understand what is so controversial about this
> patch?

There are couple of remarks above:
1) what to read at the real HiZ mode ("in" and "out" buffers are disconnected
   from the pin) in case hardware supports it;
2) why to read "out" value via "in" buffer in case of push-pull mode.

> Note the datasheet describes the GPDO / GPDI bit 0  values as:
> 
> GPDO bit 0: "Control of the GPIO0 output"
> GPDI bit 0: "State of the GPIO0 input"
> 
> So clearly GPDI is the correct register to use for the get()
> callback, which is what this patch is doing.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/5] gpio: tps68470: Add support for the indicator LED outputs

[Andy Shevchenko]

On Tue, Nov 29, 2022 at 01:20:03PM +0100, Hans de Goede wrote:
> On 11/29/22 12:59, Andy Shevchenko wrote:

...

> I'm sorry, but just like with your comments on patch 1/3
> I really don't see the problem here. Unlike "dir" or "out_or_in"
> get is a perfectly fine parameter name which is actually clearly
> defined by the parameter-name itself.
> 
> And the function + the callers are all in the same file, so
> anyone reading the code can easily deduce the meaning from
> the code.

OK. I'm not insisting, it's a bikeshedding anyway.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Hans de Goede]

Hi,

On 11/29/22 13:34, Andy Shevchenko wrote:
> On Tue, Nov 29, 2022 at 01:19:57PM +0100, Hans de Goede wrote:
>> On 11/29/22 12:56, Andy Shevchenko wrote:
>>> On Tue, Nov 29, 2022 at 1:27 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 11/29/22 11:22, Andy Shevchenko wrote:
>>>>> On Mon, Nov 28, 2022 at 11:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>>
>>>>>> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
>>>>>> so that the actual value of the pin is read, rather then the value the pin
>>>>>
>>>>> than
>>>>
>>>> Ack.
>>>>
>>>>>> would output when put in output mode.
>>>>>
>>>>> I don't see it here and haven't checked the context, but the idea is
>>>>> to check the direction and return either input (if pin is in input
>>>>> mode) or [cached] output.If it's the case, the patch looks good to me.
>>>>
>>>> No the idea is to always actually use the input register when reading
>>>> the pins, independent of the input/output mode. Instead of always
>>>> reading the [cached] output register value.
>>>
>>> But why? This makes a little sense to me.
>>
>> I don't understand what your problem is with this patch ?
>>
>> This is standard behavior for GPIO drivers, the get() callback
>> always reads the actual pin values when there is a registers
>> to get the actual pin-values. AFAIK this is no different from what
>> countless other GPIO drivers do ?
> 
> If the GPIO is requested is output and the pin in the HiZ mode, what value
> should you return?

The actual value measured on the pin. The only output + HiZ mode is
the GPDO register saying the pin should be high while it is configured
as an open-drain pin and then get() should return the actual value
on the pin which might be high (pulled-up) or low, because of
being pulled down by another open-drain output on the same bus
(e.g. I2C) and presumably the caller wants to know the actual
pin value to detect e.g. clock stretching with I2C.

> 
>>>> The input buffer will still work when the device is in output mode
>>>
>>> Does this hardware support HiZ?
>>
>> Yes the 7 standard GPIO pins can be put in input mode, aka HiZ mode.
> 
> No, input and HiZ are two different states. If the hardware doesn't
> support the latter, then your patch doesn't make any difference indeed, except
> involving "in" buffer for "out" value. Which I consider an unneeded
> step instead of reading [cached] "out" value.

See above.

> [cached] here is used in a broader sense: either SW cache, or read back value
> from the GPIO out register (not all hardware support that).
> 
>>>> and if somehow an external force is pushing the pin low/high against
>>>> the output value then the input buffer will correctly reflect this
>>>> (assuming the output is current limited and thus the magic smoke
>>>> stays inside the chip).
>>>
>>> Exactly, when smoke comes out, the hardware is broken and code,
>>> whatever is it, makes no difference at all.
>>
>> The GPIO part of the TPS68470 supports open-drain outputs, to correctly
>> get the actual value on the pin from the get() callback, the GPDI
>> register must be used. And being able to detect some outside chip
>> forcing the line low in open-drain mode is important to be able to
>> e.g. implement a software I2C master.
> 
> Right, but for push-pull mode this is not needed, correct?
> 
>> As I mentioned above actually using the input buffer value in
>> the get() method is standard behavior for GPIO drivers, exactly
>> for reasons like allowing a sw I2C master implementation to
>> detect clock stretching or conflicts (in the multi master case).
> 
>> I really don't understand what is so controversial about this
>> patch?
> 
> There are couple of remarks above:
> 1) what to read at the real HiZ mode ("in" and "out" buffers are disconnected
>    from the pin) in case hardware supports it;

If your definition of real HiZ mode is the input buffer also
being disconnected from the pin, then AFAICT from the datasheet
this hw does not support real (see above for what I consider
HiZ mode).

> 2) why to read "out" value via "in" buffer in case of push-pull mode.

Because it keeps the code KISS, because even then the bus
might be short-circuited to ground?

A better question is why would a GPIO API user ever call get()
on a pin in push-pull mode. The only answer I can come up with
is to get *the actual pin value* to e.g. detect a short-circuit
on the bus to gnd / Vdd.

>> Note the datasheet describes the GPDO / GPDI bit 0  values as:
>>
>> GPDO bit 0: "Control of the GPIO0 output"
>> GPDI bit 0: "State of the GPIO0 input"
>>
>> So clearly GPDI is the correct register to use for the get()
>> callback, which is what this patch is doing.
> 

Regards,

Hans

Re: [PATCH 1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register

[Dan Scally]

Hi Hans

On 28/11/2022 21:44, Hans de Goede wrote:
> For the regular GPIO pins the value should be read from TPS68470_REG_GPDI,
> so that the actual value of the pin is read, rather then the value the pin
> would output when put in output mode.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Good spot:


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>


Just building the series to test it too

> ---
>   drivers/gpio/gpio-tps68470.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index aaddcabe9b35..778a72cf800c 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -30,7 +30,7 @@ static int tps68470_gpio_get(struct gpio_chip *gc, unsigned int offset)
>   {
>   	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>   	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
> -	unsigned int reg = TPS68470_REG_GPDO;
> +	unsigned int reg = TPS68470_REG_GPDI;
>   	int val, ret;
>   
>   	if (offset >= TPS68470_N_REGULAR_GPIO) {

Re: [PATCH 2/5] gpio: tps68470: Make tps68470_gpio_output() always set the initial value

[Dan Scally]

Hi Hans

On 28/11/2022 21:44, Hans de Goede wrote:
> Make tps68470_gpio_output() call tps68470_gpio_set() for output-only pins
> too, so that the initial value passed to gpiod_direction_output() is
> honored for these pins too.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>


Reviewed-by: Daniel Scally <dan.scally@ideasonboard.com>

Tested-by: Daniel Scally <dan.scally@ideasonboard.com>

> ---
>   drivers/gpio/gpio-tps68470.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index 778a72cf800c..2ca86fbe1d84 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -91,13 +91,13 @@ static int tps68470_gpio_output(struct gpio_chip *gc, unsigned int offset,
>   	struct tps68470_gpio_data *tps68470_gpio = gpiochip_get_data(gc);
>   	struct regmap *regmap = tps68470_gpio->tps68470_regmap;
>   
> +	/* Set the initial value */
> +	tps68470_gpio_set(gc, offset, value);
> +
>   	/* rest are always outputs */
>   	if (offset >= TPS68470_N_REGULAR_GPIO)
>   		return 0;
>   
> -	/* Set the initial value */
> -	tps68470_gpio_set(gc, offset, value);
> -
>   	return regmap_update_bits(regmap, TPS68470_GPIO_CTL_REG_A(offset),
>   				 TPS68470_GPIO_MODE_MASK,
>   				 TPS68470_GPIO_MODE_OUT_CMOS);

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Linus Walleij]

On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede <hdegoede@redhat.com> wrote:

> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code

Good, please merge this.

> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
> Patch5:      Add gpio-lookup table entry for the privacy LED.

OK so I have to call out the hippo in the room:

these "gpios" are not called "gpios" anywhere else than in this
patch. General purpose input/output, remember. These are special
purpose LED control registers.

So can you provide a good explanation why these registers aren't
handled in the drivers/led subsystem instead?

IIUC the leds subsystem has gained support for leds as resources.

I don't mind a LED driver inside of the GPIO driver if that is what
it takes as a compromise, just that it should be handled by the right
subsystem.

Given that flash leds which are used by cameras are already in
drivers/leds/flash this should be no different and there will be more
cameras with these privacy leds.

Yours,
Linus Walleij

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Hans de Goede]

Hi,

On 12/3/22 10:32, Linus Walleij wrote:
> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
> 
> Good, please merge this.
> 
>> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
>> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
>> Patch5:      Add gpio-lookup table entry for the privacy LED.
> 
> OK so I have to call out the hippo in the room:
> 
> these "gpios" are not called "gpios" anywhere else than in this
> patch. General purpose input/output, remember. These are special
> purpose LED control registers.
> 
> So can you provide a good explanation why these registers aren't
> handled in the drivers/led subsystem instead?

This was discussed in another thread:

https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/

There were 2 problems identified which has lead to the current
direction of just modelling at as an (output only) GPIO:

1. The LED class allows userspace control of the LED which is
bad from a privacy pov. This will make it easy, too easy
(was the conclusion) for spy-ware to turn on the camera without
the LED turning on.

Later in the thread it was pointed out that there is a flag to
suspend userspace control, we could use this to permanently disable
userspace control which I guess would be some what of a solution,
although we would then also need to look into disallow changing
triggers, because I think those could still be used as a way around
this.

2. GPIO(s) can be tied directly to the device so that on a device
with both front and back privacy-LEDs (real world example) doing
 gpiod_get(dev, "privacy-led") gets us the right privacy-led,
where as with LED class devices tying the sensor and LED class
device is going to be tricky.

> IIUC the leds subsystem has gained support for leds as resources.

Interesting that would mitigate problem 2 from above and since
people keep circling back to "its a LED please use the LED class",
this is definitely worth looking into.

Do you have any pointers / examples about led class devices as
resources?

###

Note though that these indicator LED outputs, both functionality
wise as well as at the register level of this PMIC only support
turning them on/off. So this maps pretty well to the GPIO subsystem
and all the functionality of the LED class subsystem is mostly
functionality which we want to avoid since we don't want userspace
control, neither directly through sysfs or by attaching triggers.

So this does map pretty well to just modelling it as a GPIO,
if we model this as a LED then we end up having to workaround
a bunch of stuff the LED subsytem does which we do not want in
this case. And this may even require patches to the LED subsystem
to disallow userspace changing the trigger (I would need to check).

So from my pov modelling this as an output-only GPIO pin is
actually a more KISS solution then involving the LED subsystem...

> I don't mind a LED driver inside of the GPIO driver if that is what
> it takes as a compromise, just that it should be handled by the right
> subsystem.

The PMIC already is a MFD device, so if we go the LED class route
we can just add a separate MFD child device for the new LED driver
to bind to.

> Given that flash leds which are used by cameras are already in
> drivers/leds/flash this should be no different and there will be more
> cameras with these privacy leds.

Actually this patch is for the back camera privacy LED on a
Microsoft Surface Go tablet. The front camera privacy LED is
directly attached to a GPIO of the main SoC. So for that camera
just adding a GPIO lookup table entry to map the ACPI provided
GPIO info to a "privacy-led" GPIO on the sensor i2c_client device
(which we already do for the "reset" and "powerdown" gpios) also
by far is the most KISS approach.

Doing things this way in the code translating the ACPI "magic"
to standard Linux device-model stuff is literary a single line
of code (add an extra case: to an existing list of cases in a
switch-case). Where as instantiating a LED class device for this
and then somehow tying that to the i2c_client for the sensor will
be more code.

So again treating these on/off only LEDs, where we want to
*disallow* userspace control, as a GPIO is by far the most KISS
solution.

Regards,

Hans

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Sakari Ailus]

Hi Hans,

On Mon, Nov 28, 2022 at 10:44:03PM +0100, Hans de Goede wrote:
> Hi All,
> 
> While working on the earlier discussed changes for dealing with
> the privacy LED for "discrete" INT3472 ACPI devices I was wondering
> "how are we dealing with the privacy LED when the INT3472 ACPI device
> is a tps68470?".
> 
> Well it turns out we were not dealing with this at all, leading to
> the privacy LED on the back of the Surface Go series not lighting up
> when the back camera is on.

The tps68470 on ACPI was originally only supported via the OP region
interface and on all of those systems the privacy LED was hard wired. So
the LED outputs were not supported. Thanks for adding them.

> 
> This series fixes this, it consists of:
> 
> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
> Patch5:      Add gpio-lookup table entry for the privacy LED.
> 
> There is one small issue here, I believe that patches 1-3 need to land before
> 4 + 5 do. Once 4 + 5 have landed the ov8865 driver will try to get a
> GPIO with pin number 10 from the gpio-tps68470 provider and without patch 3,
> that will fail because only pins 0-9 exist until patch 3 lands.
> 
> The easiest way to avoid this issue would be for me to merge patches 1-3 +
> 5 through the pdx86 tree. GPIO subsystem maintainers, may I have your ack
> for this ?
> 
> Note patch 4 is not a problem without patch 5, it uses gpiod_get_optional,
> so as long as there is no lookup entry for a "privacy-led" GPIO it is
> a no-op.

For the patches:

Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Although I'd still like to see another, better scaling solution for
controlling privacy LEDs.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Hans de Goede]

Hi All,

On 12/3/22 13:28, Hans de Goede wrote:
> Hi,
> 
> On 12/3/22 10:32, Linus Walleij wrote:
>> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>>> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
>>
>> Good, please merge this.
>>
>>> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
>>> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
>>> Patch5:      Add gpio-lookup table entry for the privacy LED.
>>
>> OK so I have to call out the hippo in the room:
>>
>> these "gpios" are not called "gpios" anywhere else than in this
>> patch. General purpose input/output, remember. These are special
>> purpose LED control registers.
>>
>> So can you provide a good explanation why these registers aren't
>> handled in the drivers/led subsystem instead?
> 
> This was discussed in another thread:
> 
> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
> 
> There were 2 problems identified which has lead to the current
> direction of just modelling at as an (output only) GPIO:
> 
> 1. The LED class allows userspace control of the LED which is
> bad from a privacy pov. This will make it easy, too easy
> (was the conclusion) for spy-ware to turn on the camera without
> the LED turning on.
> 
> Later in the thread it was pointed out that there is a flag to
> suspend userspace control, we could use this to permanently disable
> userspace control which I guess would be some what of a solution,
> although we would then also need to look into disallow changing
> triggers, because I think those could still be used as a way around
> this.

I have spend today looking into the feasibility of using the LED
class subsystem instead of modelling these on/off only LEDs as a GPIO.

Good news, there is a LED_SYSFS_DISABLE flag which also stops
userspace from messing with the trigger of the LED, so this
first issue can easily be fixed.

> 2. GPIO(s) can be tied directly to the device so that on a device
> with both front and back privacy-LEDs (real world example) doing
>  gpiod_get(dev, "privacy-led") gets us the right privacy-led,
> where as with LED class devices tying the sensor and LED class
> device is going to be tricky.
> 
>> IIUC the leds subsystem has gained support for leds as resources.
> 
> Interesting that would mitigate problem 2 from above and since
> people keep circling back to "its a LED please use the LED class",
> this is definitely worth looking into.
> 
> Do you have any pointers / examples about led class devices as
> resources?

I have been looking into this, but atm the only way to tie a
led-classdev to a device is through a fwnode reference.

Since this is x86 where there is no DTS file where we can
easily add this, I have been looking into doing this with
swnode-s.

LED directly attached to main SoC GPIO
======================================

For the simple LED is attached to a GPIO on the main
SoC case, this requires 2 steps:

1. Have the INT3472 code register a LED classdev for the
privacy-led instead of a GPIO lookup table entry. This LED
classdev must have a swnode as fwnode, so that we can put
a reference to that swnode in a "leds" reference-array 
property on the sensor i2c_client. This is about a 100
lines of extra code and seems fine / doable.

2. Add a "leds" reference-array property on the
i2c_client device by adding a swnode with this property
to the i2c_client device.  This sounds straight forward
(once we have the swnode for the LED class-device to point to)
but this is actually not straight forward at all.

There is a whole bunch of properties which needs to be
added on the sensor to describe the media-graph between
the sensor and the IPU, as well as what VCM (if any) is paired
up with the sensor. These properties are all added through
adding a swnode from the CSI bridge driver:

drivers/media/pci/intel/ipu3/cio2-bridge.c

But a device can only have one swnode added. So we cannot
add a swnode to the sensor i2c_client in the INT3472 code.

Instead the only thing which we could do is give the swnode
for the privacy LED classdev a predictable name, derived
from the sensor's device name and then
have drivers/media/pci/intel/ipu3/cio2-bridge.c
call software_node_find_by_name() to get the swnode and
have it add the "leds" reference-array property on the
i2c_client device for the sensor.

However the INT3472 code is shared between multiple ISP/IPU
implementations, so then we would need to duplicate this code
for the other IPU versions (currently IPU6 which is out of tree),
further complicating things.

And this is for the direct usage of a SoC GPIO case.


LED attached to TPS68470 PMIC indicator LED pin
===============================================

In this case the LED-classdev should be instantiated
by a driver for a new TPS68470 MFD cell. But this
also introduces a bunch of probe ordering systems,
so modelling things as a LED classdev here would involve:

1. Making the IN3472 code create + register a swnode for
the LED classdev, this must be done here because of
probe ordering.

2. Make the IN3472 code create a new TPS68470 MFD cell
and pass the swnode as fwnode to this cell.

3. Write a new driver for the new TPS68470 MFD cell,
which registers a LED classdev using the fwnode from
the MFD cell as fwnode for the LED classdev.


And this still does not solve the issue of how to get
the privacy-LED as LED classdev model to work on the IPU6.

Alternative approach
====================

An alternative approach, would be to add support for LED
lookup tables to the LED class code (like we already have
for GPIOs) and use this to allow tying a LED classdev to
a struct device on non devicetree platforms.

Given the problems with the swnode approach from above
I believe that this would actually be better then
the swnode approach.

Lookup tables like this use device-names, so we don't need
to have swnode-s ready for both the provider and the consumer
at the time of adding the lookup table entry. Instead all
that is necessary is to know the device-names of both
the provider and the consumer which are both known in
advance.

Is this really worth all the trouble ?
======================================

So I really have to wonder what is using the LED 
classdev / framework actually buying us over using
modelling these on/off only LEDs as a GPIO ?

I know that some (x86) have a flash-LED for the back
camera and given the experience with trying to tie
a LED class dev to a specific struct device (to the
sensor's i2c_client) I guess we are eventually going to
need some sort of lookup tables for tying LED class
devices to a specific device anyways.

That and we want to avoid moving from the current
approach (for some INT3472 using devices) of tying
the privacy LED on/off to the INT3472 registered
clk being enabled/disabled to modelling this as
GPIOs, to then later modelling it as LED class
devices after all.

To avoid this double conversion issue I'm going to
give the LED class route a second go, replacing
the swnode approach which I tried today with
a lookup-table approach.

Regards,

Hans






> ###
> 
> Note though that these indicator LED outputs, both functionality
> wise as well as at the register level of this PMIC only support
> turning them on/off. So this maps pretty well to the GPIO subsystem
> and all the functionality of the LED class subsystem is mostly
> functionality which we want to avoid since we don't want userspace
> control, neither directly through sysfs or by attaching triggers.
> 
> So this does map pretty well to just modelling it as a GPIO,
> if we model this as a LED then we end up having to workaround
> a bunch of stuff the LED subsytem does which we do not want in
> this case. And this may even require patches to the LED subsystem
> to disallow userspace changing the trigger (I would need to check).
> 
> So from my pov modelling this as an output-only GPIO pin is
> actually a more KISS solution then involving the LED subsystem...
> 
>> I don't mind a LED driver inside of the GPIO driver if that is what
>> it takes as a compromise, just that it should be handled by the right
>> subsystem.
> 
> The PMIC already is a MFD device, so if we go the LED class route
> we can just add a separate MFD child device for the new LED driver
> to bind to.
> 
>> Given that flash leds which are used by cameras are already in
>> drivers/leds/flash this should be no different and there will be more
>> cameras with these privacy leds.
> 
> Actually this patch is for the back camera privacy LED on a
> Microsoft Surface Go tablet. The front camera privacy LED is
> directly attached to a GPIO of the main SoC. So for that camera
> just adding a GPIO lookup table entry to map the ACPI provided
> GPIO info to a "privacy-led" GPIO on the sensor i2c_client device
> (which we already do for the "reset" and "powerdown" gpios) also
> by far is the most KISS approach.
> 
> Doing things this way in the code translating the ACPI "magic"
> to standard Linux device-model stuff is literary a single line
> of code (add an extra case: to an existing list of cases in a
> switch-case). Where as instantiating a LED class device for this
> and then somehow tying that to the i2c_client for the sensor will
> be more code.
> 
> So again treating these on/off only LEDs, where we want to
> *disallow* userspace control, as a GPIO is by far the most KISS
> solution.
> 
> Regards,
> 
> Hans
> 

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Laurent Pinchart]

Hi Hans,

On Mon, Dec 05, 2022 at 04:01:20PM +0100, Hans de Goede wrote:
> On 12/3/22 13:28, Hans de Goede wrote:
> > On 12/3/22 10:32, Linus Walleij wrote:
> >> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede wrote:
> >>
> >>> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
> >>
> >> Good, please merge this.
> >>
> >>> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
> >>> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
> >>> Patch5:      Add gpio-lookup table entry for the privacy LED.
> >>
> >> OK so I have to call out the hippo in the room:
> >>
> >> these "gpios" are not called "gpios" anywhere else than in this
> >> patch. General purpose input/output, remember. These are special
> >> purpose LED control registers.
> >>
> >> So can you provide a good explanation why these registers aren't
> >> handled in the drivers/led subsystem instead?
> > 
> > This was discussed in another thread:
> > 
> > https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
> > 
> > There were 2 problems identified which has lead to the current
> > direction of just modelling at as an (output only) GPIO:
> > 
> > 1. The LED class allows userspace control of the LED which is
> > bad from a privacy pov. This will make it easy, too easy
> > (was the conclusion) for spy-ware to turn on the camera without
> > the LED turning on.
> > 
> > Later in the thread it was pointed out that there is a flag to
> > suspend userspace control, we could use this to permanently disable
> > userspace control which I guess would be some what of a solution,
> > although we would then also need to look into disallow changing
> > triggers, because I think those could still be used as a way around
> > this.
> 
> I have spend today looking into the feasibility of using the LED
> class subsystem instead of modelling these on/off only LEDs as a GPIO.
> 
> Good news, there is a LED_SYSFS_DISABLE flag which also stops
> userspace from messing with the trigger of the LED, so this
> first issue can easily be fixed.
> 
> > 2. GPIO(s) can be tied directly to the device so that on a device
> > with both front and back privacy-LEDs (real world example) doing
> >  gpiod_get(dev, "privacy-led") gets us the right privacy-led,
> > where as with LED class devices tying the sensor and LED class
> > device is going to be tricky.
> > 
> >> IIUC the leds subsystem has gained support for leds as resources.
> > 
> > Interesting that would mitigate problem 2 from above and since
> > people keep circling back to "its a LED please use the LED class",
> > this is definitely worth looking into.
> > 
> > Do you have any pointers / examples about led class devices as
> > resources?
> 
> I have been looking into this, but atm the only way to tie a
> led-classdev to a device is through a fwnode reference.
> 
> Since this is x86 where there is no DTS file where we can
> easily add this, I have been looking into doing this with
> swnode-s.
> 
> LED directly attached to main SoC GPIO
> ======================================
> 
> For the simple LED is attached to a GPIO on the main
> SoC case, this requires 2 steps:
> 
> 1. Have the INT3472 code register a LED classdev for the
> privacy-led instead of a GPIO lookup table entry. This LED
> classdev must have a swnode as fwnode, so that we can put
> a reference to that swnode in a "leds" reference-array 
> property on the sensor i2c_client. This is about a 100
> lines of extra code and seems fine / doable.
> 
> 2. Add a "leds" reference-array property on the
> i2c_client device by adding a swnode with this property
> to the i2c_client device.  This sounds straight forward
> (once we have the swnode for the LED class-device to point to)
> but this is actually not straight forward at all.
> 
> There is a whole bunch of properties which needs to be
> added on the sensor to describe the media-graph between
> the sensor and the IPU, as well as what VCM (if any) is paired
> up with the sensor. These properties are all added through
> adding a swnode from the CSI bridge driver:
> 
> drivers/media/pci/intel/ipu3/cio2-bridge.c
> 
> But a device can only have one swnode added. So we cannot
> add a swnode to the sensor i2c_client in the INT3472 code.
> 
> Instead the only thing which we could do is give the swnode
> for the privacy LED classdev a predictable name, derived
> from the sensor's device name and then
> have drivers/media/pci/intel/ipu3/cio2-bridge.c
> call software_node_find_by_name() to get the swnode and
> have it add the "leds" reference-array property on the
> i2c_client device for the sensor.
> 
> However the INT3472 code is shared between multiple ISP/IPU
> implementations, so then we would need to duplicate this code
> for the other IPU versions (currently IPU6 which is out of tree),
> further complicating things.
> 
> And this is for the direct usage of a SoC GPIO case.
> 
> 
> LED attached to TPS68470 PMIC indicator LED pin
> ===============================================
> 
> In this case the LED-classdev should be instantiated
> by a driver for a new TPS68470 MFD cell. But this
> also introduces a bunch of probe ordering systems,
> so modelling things as a LED classdev here would involve:
> 
> 1. Making the IN3472 code create + register a swnode for
> the LED classdev, this must be done here because of
> probe ordering.
> 
> 2. Make the IN3472 code create a new TPS68470 MFD cell
> and pass the swnode as fwnode to this cell.
> 
> 3. Write a new driver for the new TPS68470 MFD cell,
> which registers a LED classdev using the fwnode from
> the MFD cell as fwnode for the LED classdev.
> 
> 
> And this still does not solve the issue of how to get
> the privacy-LED as LED classdev model to work on the IPU6.
> 
> Alternative approach
> ====================
> 
> An alternative approach, would be to add support for LED
> lookup tables to the LED class code (like we already have
> for GPIOs) and use this to allow tying a LED classdev to
> a struct device on non devicetree platforms.
> 
> Given the problems with the swnode approach from above
> I believe that this would actually be better then
> the swnode approach.
> 
> Lookup tables like this use device-names, so we don't need
> to have swnode-s ready for both the provider and the consumer
> at the time of adding the lookup table entry. Instead all
> that is necessary is to know the device-names of both
> the provider and the consumer which are both known in
> advance.

Thank you for all this research.

> Is this really worth all the trouble ?
> ======================================
> 
> So I really have to wonder what is using the LED 
> classdev / framework actually buying us over using
> modelling these on/off only LEDs as a GPIO ?
> 
> I know that some (x86) have a flash-LED for the back
> camera and given the experience with trying to tie
> a LED class dev to a specific struct device (to the
> sensor's i2c_client) I guess we are eventually going to
> need some sort of lookup tables for tying LED class
> devices to a specific device anyways.

Probably, but for those, the effort will pay off better, as we need to
control the flash from userspace. For the privacy LED, I doubt we'll
ever seen a system where we'll have to control it through more than an
enable bit (for instance, an RGB or intensity-controlled privacy LED
sounds very unlikely), so a GPIO is fine with me.

> That and we want to avoid moving from the current
> approach (for some INT3472 using devices) of tying
> the privacy LED on/off to the INT3472 registered
> clk being enabled/disabled to modelling this as
> GPIOs, to then later modelling it as LED class
> devices after all.
> 
> To avoid this double conversion issue I'm going to
> give the LED class route a second go, replacing
> the swnode approach which I tried today with
> a lookup-table approach.
> 
> > ###
> > 
> > Note though that these indicator LED outputs, both functionality
> > wise as well as at the register level of this PMIC only support
> > turning them on/off. So this maps pretty well to the GPIO subsystem
> > and all the functionality of the LED class subsystem is mostly
> > functionality which we want to avoid since we don't want userspace
> > control, neither directly through sysfs or by attaching triggers.
> > 
> > So this does map pretty well to just modelling it as a GPIO,
> > if we model this as a LED then we end up having to workaround
> > a bunch of stuff the LED subsytem does which we do not want in
> > this case. And this may even require patches to the LED subsystem
> > to disallow userspace changing the trigger (I would need to check).
> > 
> > So from my pov modelling this as an output-only GPIO pin is
> > actually a more KISS solution then involving the LED subsystem...
> > 
> >> I don't mind a LED driver inside of the GPIO driver if that is what
> >> it takes as a compromise, just that it should be handled by the right
> >> subsystem.
> > 
> > The PMIC already is a MFD device, so if we go the LED class route
> > we can just add a separate MFD child device for the new LED driver
> > to bind to.
> > 
> >> Given that flash leds which are used by cameras are already in
> >> drivers/leds/flash this should be no different and there will be more
> >> cameras with these privacy leds.
> > 
> > Actually this patch is for the back camera privacy LED on a
> > Microsoft Surface Go tablet. The front camera privacy LED is
> > directly attached to a GPIO of the main SoC. So for that camera
> > just adding a GPIO lookup table entry to map the ACPI provided
> > GPIO info to a "privacy-led" GPIO on the sensor i2c_client device
> > (which we already do for the "reset" and "powerdown" gpios) also
> > by far is the most KISS approach.
> > 
> > Doing things this way in the code translating the ACPI "magic"
> > to standard Linux device-model stuff is literary a single line
> > of code (add an extra case: to an existing list of cases in a
> > switch-case). Where as instantiating a LED class device for this
> > and then somehow tying that to the i2c_client for the sensor will
> > be more code.
> > 
> > So again treating these on/off only LEDs, where we want to
> > *disallow* userspace control, as a GPIO is by far the most KISS
> > solution.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Hans de Goede]

Hi,

On 12/5/22 22:26, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Mon, Dec 05, 2022 at 04:01:20PM +0100, Hans de Goede wrote:
>> On 12/3/22 13:28, Hans de Goede wrote:
>>> On 12/3/22 10:32, Linus Walleij wrote:
>>>> On Mon, Nov 28, 2022 at 10:44 PM Hans de Goede wrote:
>>>>
>>>>> Patches 1-2: 2 small bugfixes to the gpio-tps68470 code
>>>>
>>>> Good, please merge this.
>>>>
>>>>> Patch3:      Add support for the indicator LED outputs on the tps68470 as GPIOs
>>>>> Patch4:      Add support for a privacy LED to the ov8865 sensor driver
>>>>> Patch5:      Add gpio-lookup table entry for the privacy LED.
>>>>
>>>> OK so I have to call out the hippo in the room:
>>>>
>>>> these "gpios" are not called "gpios" anywhere else than in this
>>>> patch. General purpose input/output, remember. These are special
>>>> purpose LED control registers.
>>>>
>>>> So can you provide a good explanation why these registers aren't
>>>> handled in the drivers/led subsystem instead?
>>>
>>> This was discussed in another thread:
>>>
>>> https://lore.kernel.org/platform-driver-x86/20221124200007.390901-1-hdegoede@redhat.com/
>>>
>>> There were 2 problems identified which has lead to the current
>>> direction of just modelling at as an (output only) GPIO:
>>>
>>> 1. The LED class allows userspace control of the LED which is
>>> bad from a privacy pov. This will make it easy, too easy
>>> (was the conclusion) for spy-ware to turn on the camera without
>>> the LED turning on.
>>>
>>> Later in the thread it was pointed out that there is a flag to
>>> suspend userspace control, we could use this to permanently disable
>>> userspace control which I guess would be some what of a solution,
>>> although we would then also need to look into disallow changing
>>> triggers, because I think those could still be used as a way around
>>> this.
>>
>> I have spend today looking into the feasibility of using the LED
>> class subsystem instead of modelling these on/off only LEDs as a GPIO.
>>
>> Good news, there is a LED_SYSFS_DISABLE flag which also stops
>> userspace from messing with the trigger of the LED, so this
>> first issue can easily be fixed.
>>
>>> 2. GPIO(s) can be tied directly to the device so that on a device
>>> with both front and back privacy-LEDs (real world example) doing
>>>  gpiod_get(dev, "privacy-led") gets us the right privacy-led,
>>> where as with LED class devices tying the sensor and LED class
>>> device is going to be tricky.
>>>
>>>> IIUC the leds subsystem has gained support for leds as resources.
>>>
>>> Interesting that would mitigate problem 2 from above and since
>>> people keep circling back to "its a LED please use the LED class",
>>> this is definitely worth looking into.
>>>
>>> Do you have any pointers / examples about led class devices as
>>> resources?
>>
>> I have been looking into this, but atm the only way to tie a
>> led-classdev to a device is through a fwnode reference.
>>
>> Since this is x86 where there is no DTS file where we can
>> easily add this, I have been looking into doing this with
>> swnode-s.
>>
>> LED directly attached to main SoC GPIO
>> ======================================
>>
>> For the simple LED is attached to a GPIO on the main
>> SoC case, this requires 2 steps:
>>
>> 1. Have the INT3472 code register a LED classdev for the
>> privacy-led instead of a GPIO lookup table entry. This LED
>> classdev must have a swnode as fwnode, so that we can put
>> a reference to that swnode in a "leds" reference-array 
>> property on the sensor i2c_client. This is about a 100
>> lines of extra code and seems fine / doable.
>>
>> 2. Add a "leds" reference-array property on the
>> i2c_client device by adding a swnode with this property
>> to the i2c_client device.  This sounds straight forward
>> (once we have the swnode for the LED class-device to point to)
>> but this is actually not straight forward at all.
>>
>> There is a whole bunch of properties which needs to be
>> added on the sensor to describe the media-graph between
>> the sensor and the IPU, as well as what VCM (if any) is paired
>> up with the sensor. These properties are all added through
>> adding a swnode from the CSI bridge driver:
>>
>> drivers/media/pci/intel/ipu3/cio2-bridge.c
>>
>> But a device can only have one swnode added. So we cannot
>> add a swnode to the sensor i2c_client in the INT3472 code.
>>
>> Instead the only thing which we could do is give the swnode
>> for the privacy LED classdev a predictable name, derived
>> from the sensor's device name and then
>> have drivers/media/pci/intel/ipu3/cio2-bridge.c
>> call software_node_find_by_name() to get the swnode and
>> have it add the "leds" reference-array property on the
>> i2c_client device for the sensor.
>>
>> However the INT3472 code is shared between multiple ISP/IPU
>> implementations, so then we would need to duplicate this code
>> for the other IPU versions (currently IPU6 which is out of tree),
>> further complicating things.
>>
>> And this is for the direct usage of a SoC GPIO case.
>>
>>
>> LED attached to TPS68470 PMIC indicator LED pin
>> ===============================================
>>
>> In this case the LED-classdev should be instantiated
>> by a driver for a new TPS68470 MFD cell. But this
>> also introduces a bunch of probe ordering systems,
>> so modelling things as a LED classdev here would involve:
>>
>> 1. Making the IN3472 code create + register a swnode for
>> the LED classdev, this must be done here because of
>> probe ordering.
>>
>> 2. Make the IN3472 code create a new TPS68470 MFD cell
>> and pass the swnode as fwnode to this cell.
>>
>> 3. Write a new driver for the new TPS68470 MFD cell,
>> which registers a LED classdev using the fwnode from
>> the MFD cell as fwnode for the LED classdev.
>>
>>
>> And this still does not solve the issue of how to get
>> the privacy-LED as LED classdev model to work on the IPU6.
>>
>> Alternative approach
>> ====================
>>
>> An alternative approach, would be to add support for LED
>> lookup tables to the LED class code (like we already have
>> for GPIOs) and use this to allow tying a LED classdev to
>> a struct device on non devicetree platforms.
>>
>> Given the problems with the swnode approach from above
>> I believe that this would actually be better then
>> the swnode approach.
>>
>> Lookup tables like this use device-names, so we don't need
>> to have swnode-s ready for both the provider and the consumer
>> at the time of adding the lookup table entry. Instead all
>> that is necessary is to know the device-names of both
>> the provider and the consumer which are both known in
>> advance.
> 
> Thank you for all this research.
> 
>> Is this really worth all the trouble ?
>> ======================================
>>
>> So I really have to wonder what is using the LED 
>> classdev / framework actually buying us over using
>> modelling these on/off only LEDs as a GPIO ?
>>
>> I know that some (x86) have a flash-LED for the back
>> camera and given the experience with trying to tie
>> a LED class dev to a specific struct device (to the
>> sensor's i2c_client) I guess we are eventually going to
>> need some sort of lookup tables for tying LED class
>> devices to a specific device anyways.
> 
> Probably, but for those, the effort will pay off better, as we need to
> control the flash from userspace. For the privacy LED, I doubt we'll
> ever seen a system where we'll have to control it through more than an
> enable bit (for instance, an RGB or intensity-controlled privacy LED
> sounds very unlikely), so a GPIO is fine with me.

Ok, that is good to know. Going the GPIO route would be
a lot more KISS, so that has my preference too.

Linus would modelling these simple on/off LEDs as
GPIOs be acceptable to you too ?

Regards,

Hans





> 
>> That and we want to avoid moving from the current
>> approach (for some INT3472 using devices) of tying
>> the privacy LED on/off to the INT3472 registered
>> clk being enabled/disabled to modelling this as
>> GPIOs, to then later modelling it as LED class
>> devices after all.
>>
>> To avoid this double conversion issue I'm going to
>> give the LED class route a second go, replacing
>> the swnode approach which I tried today with
>> a lookup-table approach.
>>
>>> ###
>>>
>>> Note though that these indicator LED outputs, both functionality
>>> wise as well as at the register level of this PMIC only support
>>> turning them on/off. So this maps pretty well to the GPIO subsystem
>>> and all the functionality of the LED class subsystem is mostly
>>> functionality which we want to avoid since we don't want userspace
>>> control, neither directly through sysfs or by attaching triggers.
>>>
>>> So this does map pretty well to just modelling it as a GPIO,
>>> if we model this as a LED then we end up having to workaround
>>> a bunch of stuff the LED subsytem does which we do not want in
>>> this case. And this may even require patches to the LED subsystem
>>> to disallow userspace changing the trigger (I would need to check).
>>>
>>> So from my pov modelling this as an output-only GPIO pin is
>>> actually a more KISS solution then involving the LED subsystem...
>>>
>>>> I don't mind a LED driver inside of the GPIO driver if that is what
>>>> it takes as a compromise, just that it should be handled by the right
>>>> subsystem.
>>>
>>> The PMIC already is a MFD device, so if we go the LED class route
>>> we can just add a separate MFD child device for the new LED driver
>>> to bind to.
>>>
>>>> Given that flash leds which are used by cameras are already in
>>>> drivers/leds/flash this should be no different and there will be more
>>>> cameras with these privacy leds.
>>>
>>> Actually this patch is for the back camera privacy LED on a
>>> Microsoft Surface Go tablet. The front camera privacy LED is
>>> directly attached to a GPIO of the main SoC. So for that camera
>>> just adding a GPIO lookup table entry to map the ACPI provided
>>> GPIO info to a "privacy-led" GPIO on the sensor i2c_client device
>>> (which we already do for the "reset" and "powerdown" gpios) also
>>> by far is the most KISS approach.
>>>
>>> Doing things this way in the code translating the ACPI "magic"
>>> to standard Linux device-model stuff is literary a single line
>>> of code (add an extra case: to an existing list of cases in a
>>> switch-case). Where as instantiating a LED class device for this
>>> and then somehow tying that to the i2c_client for the sensor will
>>> be more code.
>>>
>>> So again treating these on/off only LEDs, where we want to
>>> *disallow* userspace control, as a GPIO is by far the most KISS
>>> solution.
> 

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Linus Walleij]

On Mon, Dec 5, 2022 at 4:01 PM Hans de Goede <hdegoede@redhat.com> wrote:

> An alternative approach, would be to add support for LED
> lookup tables to the LED class code (like we already have
> for GPIOs) and use this to allow tying a LED classdev to
> a struct device on non devicetree platforms.
>
> Given the problems with the swnode approach from above
> I believe that this would actually be better then
> the swnode approach.
>
> Lookup tables like this use device-names, so we don't need
> to have swnode-s ready for both the provider and the consumer
> at the time of adding the lookup table entry. Instead all
> that is necessary is to know the device-names of both
> the provider and the consumer which are both known in
> advance.

I think this looks like a good idea.

We attach other resources such as clocks, regulators,
DMA channels, GPIOs exactly this way. So why not LEDs?

As GPIO maintainer I every now and then get a suggestion
like this to "just let this pass as a GPIO because it makes
my life so much easier", but it is the same curse as the
input subsystem has: it is versatile and well engineered so
it starts to look like a golden hammer (everything start to
look like nails).

But we are two GPIO maintainers and right now Bartosz
does the majority of the work, and if he thinks it's the best
idea ever I will certainly reconsider.

Yours,
Linus Walleij

Re: [PATCH 0/5] gpio/media/int3472: Add support for tps68470 privacy-LED output

[Hans de Goede]

Hi,

On 12/7/22 01:25, Linus Walleij wrote:
> On Mon, Dec 5, 2022 at 4:01 PM Hans de Goede <hdegoede@redhat.com> wrote:
> 
>> An alternative approach, would be to add support for LED
>> lookup tables to the LED class code (like we already have
>> for GPIOs) and use this to allow tying a LED classdev to
>> a struct device on non devicetree platforms.
>>
>> Given the problems with the swnode approach from above
>> I believe that this would actually be better then
>> the swnode approach.
>>
>> Lookup tables like this use device-names, so we don't need
>> to have swnode-s ready for both the provider and the consumer
>> at the time of adding the lookup table entry. Instead all
>> that is necessary is to know the device-names of both
>> the provider and the consumer which are both known in
>> advance.
> 
> I think this looks like a good idea.
> 
> We attach other resources such as clocks, regulators,
> DMA channels, GPIOs exactly this way. So why not LEDs?
> 
> As GPIO maintainer I every now and then get a suggestion
> like this to "just let this pass as a GPIO because it makes
> my life so much easier", but it is the same curse as the
> input subsystem has: it is versatile and well engineered so
> it starts to look like a golden hammer (everything start to
> look like nails).
> 
> But we are two GPIO maintainers and right now Bartosz
> does the majority of the work, and if he thinks it's the best
> idea ever I will certainly reconsider.

Ok, I will give the LED lookup table approach a go and
post a new series replacing this one when its ready.

Feel free to drop this series from your queue.

(Note given that that is going to be a whole new approach
I plan to start the new series at v1 again).

Regards,

Hans