diff mbox series

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

Message ID 20221128214408.165726-4-hdegoede@redhat.com
State New
Headers show
Series [1/5] gpio: tps68470: Fix tps68470_gpio_get() reading from the wrong register | expand

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, 11:59 a.m. UTC | #1 
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?
Andy Shevchenko Nov. 29, 2022, 12:35 p.m. UTC | #2 
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 */