diff mbox series

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

Message ID 20221128214408.165726-4-hdegoede@redhat.com (mailing list archive)
State Superseded
Headers
Series gpio/media/int3472: Add support for tps68470 privacy-LED output |

Commit Message

Hans de Goede Nov. 28, 2022, 9:44 p.m. UTC 
  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(-)

Comments

Andy Shevchenko Nov. 29, 2022, 10:28 a.m. UTC | #1 
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;
> +       }
> +}
Hans de Goede Nov. 29, 2022, 11:32 a.m. UTC | #2 
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;
>> +       }
>> +}
>
Andy Shevchenko Nov. 29, 2022, 11:59 a.m. UTC | #3 
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?
Hans de Goede Nov. 29, 2022, 12:20 p.m. UTC | #4 
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
Andy Shevchenko Nov. 29, 2022, 12:35 p.m. UTC | #5 
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.
diff mbox series

Patch

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 */