[PATCH v1 0/9] pinctrl: apple: misc fixes from post-commit review

[PATCH v1 0/9] pinctrl: apple: misc fixes from post-commit review

[Joey Gouly]

Hi all,

Here's a series of small fixes that Andy made comments about, after the driver
was already in the pinctrl tree.

Most of the patches are only a few lines, apart from the first which is a
whitespace only change.

A branch can be found here:
    https://gitlab.arm.com/linux-arm/jg-open/-/tree/pinctrl_apple_fixes_v1

The branch is based on torvald's tree, with Sven's recent fix [1] cherry-picked.
(pinctrl fixes branch didn't have Sven's commit yet)

Thanks,
Joey

[1]
https://lore.kernel.org/linux-gpio/CACRpkdZJVRn9zAC8t=_X=29+kWw2QPKBHd9wiX9C3ms+gte40w@mail.gmail.com/T/

Joey Gouly (9):
  pinctrl: apple: fix some formatting issues
  pinctrl: apple: add missing bits.h header
  pinctrl: apple: handle regmap_read errors
  pinctrl: apple: make apple_gpio_get_direction more readable
  pinctrl: apple: give error label a specific name
  pinctrl: apple: remove gpio-controller check
  pinctrl: apple: don't set gpio_chip.of_node
  pinctrl: apple: use modulo rather than bitwise and
  pinctrl: apple: return an error if pinmux is missing in the DT

 drivers/pinctrl/pinctrl-apple-gpio.c | 103 ++++++++++++---------------
 1 file changed, 46 insertions(+), 57 deletions(-)

-- 
2.17.1

[PATCH v1 1/9] pinctrl: apple: fix some formatting issues

[Joey Gouly]

Reflow some of the code now that the extra '_gpio' was removed.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 75 ++++++++++++----------------
 1 file changed, 31 insertions(+), 44 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index a7861079a650..23466ee59d79 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -70,18 +70,18 @@ struct regmap_config regmap_config = {
 	.cache_type = REGCACHE_FLAT,
 	.max_register = 512 * sizeof(u32),
 	.num_reg_defaults_raw = 512,
-	.use_relaxed_mmio = true
+	.use_relaxed_mmio = true,
 };
 
-// No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register.
+/* No locking needed to mask/unmask IRQs as the interrupt mode is per pin-register. */
 static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl,
-			       unsigned int pin, u32 mask, u32 value)
+                               unsigned int pin, u32 mask, u32 value)
 {
 	regmap_update_bits(pctl->map, REG_GPIO(pin), mask, value);
 }
 
-static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
-				   unsigned int pin)
+static u32 apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
+                              unsigned int pin)
 {
 	unsigned int val = 0;
 
@@ -92,9 +92,9 @@ static uint32_t apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
 /* Pin controller functions */
 
 static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
-				     struct device_node *node,
-				     struct pinctrl_map **map,
-				     unsigned *num_maps)
+                                     struct device_node *node,
+                                     struct pinctrl_map **map,
+                                     unsigned *num_maps)
 {
 	unsigned reserved_maps;
 	struct apple_gpio_pinctrl *pctl;
@@ -119,8 +119,7 @@ static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
 
 	num_pins = ret;
 
-	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps,
-					num_pins);
+	ret = pinctrl_utils_reserve_map(pctldev, map, &reserved_maps, num_maps, num_pins);
 	if (ret)
 		return ret;
 
@@ -138,11 +137,10 @@ static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
 		}
 
 		group_name = pinctrl_generic_get_group_name(pctldev, pin);
-		function_name =
-			pinmux_generic_get_function_name(pctl->pctldev, func);
+		function_name = pinmux_generic_get_function_name(pctl->pctldev, func);
 		ret = pinctrl_utils_add_map_mux(pctl->pctldev, map,
-						&reserved_maps, num_maps,
-						group_name, function_name);
+		                                &reserved_maps, num_maps,
+		                                group_name, function_name);
 		if (ret)
 			goto free_map;
 	}
@@ -165,7 +163,7 @@ static const struct pinctrl_ops apple_gpio_pinctrl_ops = {
 /* Pin multiplexer functions */
 
 static int apple_gpio_pinmux_set(struct pinctrl_dev *pctldev, unsigned func,
-				    unsigned group)
+                                 unsigned group)
 {
 	struct apple_gpio_pinctrl *pctl = pinctrl_dev_get_drvdata(pctldev);
 
@@ -186,8 +184,7 @@ static const struct pinmux_ops apple_gpio_pinmux_ops = {
 
 /* GPIO chip functions */
 
-static int apple_gpio_get_direction(struct gpio_chip *chip,
-					 unsigned int offset)
+static int apple_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
 	unsigned int reg = apple_gpio_get_reg(pctl, offset);
@@ -211,17 +208,14 @@ static int apple_gpio_get(struct gpio_chip *chip, unsigned offset)
 	return !!(reg & REG_GPIOx_DATA);
 }
 
-static void apple_gpio_set(struct gpio_chip *chip, unsigned int offset,
-				int value)
+static void apple_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
 
-	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA,
-			   value ? REG_GPIOx_DATA : 0);
+	apple_gpio_set_reg(pctl, offset, REG_GPIOx_DATA, value ? REG_GPIOx_DATA : 0);
 }
 
-static int apple_gpio_direction_input(struct gpio_chip *chip,
-					   unsigned int offset)
+static int apple_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
 
@@ -234,7 +228,7 @@ static int apple_gpio_direction_input(struct gpio_chip *chip,
 }
 
 static int apple_gpio_direction_output(struct gpio_chip *chip,
-					    unsigned int offset, int value)
+                                       unsigned int offset, int value)
 {
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
 
@@ -249,13 +243,10 @@ static int apple_gpio_direction_output(struct gpio_chip *chip,
 
 static void apple_gpio_irq_ack(struct irq_data *data)
 {
-	struct apple_gpio_pinctrl *pctl =
-		gpiochip_get_data(irq_data_get_irq_chip_data(data));
-	unsigned int irqgrp =
-		FIELD_GET(REG_GPIOx_GRP, apple_gpio_get_reg(pctl, data->hwirq));
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	unsigned int irqgrp = FIELD_GET(REG_GPIOx_GRP, apple_gpio_get_reg(pctl, data->hwirq));
 
-	writel(BIT(data->hwirq & 31),
-	       pctl->base + REG_IRQ(irqgrp, data->hwirq));
+	writel(BIT(data->hwirq & 31), pctl->base + REG_IRQ(irqgrp, data->hwirq));
 }
 
 static unsigned int apple_gpio_irq_type(unsigned int type)
@@ -278,20 +269,19 @@ static unsigned int apple_gpio_irq_type(unsigned int type)
 
 static void apple_gpio_irq_mask(struct irq_data *data)
 {
-	struct apple_gpio_pinctrl *pctl =
-		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(irq_data_get_irq_chip_data(data));
+
 	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
-			   FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF));
+	                   FIELD_PREP(REG_GPIOx_MODE, REG_GPIOx_IN_IRQ_OFF));
 }
 
 static void apple_gpio_irq_unmask(struct irq_data *data)
 {
-	struct apple_gpio_pinctrl *pctl =
-		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(irq_data_get_irq_chip_data(data));
 	unsigned int irqtype = apple_gpio_irq_type(irqd_get_trigger_type(data));
 
 	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
-			   FIELD_PREP(REG_GPIOx_MODE, irqtype));
+	                   FIELD_PREP(REG_GPIOx_MODE, irqtype));
 }
 
 static unsigned int apple_gpio_irq_startup(struct irq_data *data)
@@ -300,7 +290,7 @@ static unsigned int apple_gpio_irq_startup(struct irq_data *data)
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
 
 	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_GRP,
-			   FIELD_PREP(REG_GPIOx_GRP, 0));
+	                   FIELD_PREP(REG_GPIOx_GRP, 0));
 
 	apple_gpio_direction_input(chip, data->hwirq);
 	apple_gpio_irq_unmask(data);
@@ -308,18 +298,16 @@ static unsigned int apple_gpio_irq_startup(struct irq_data *data)
 	return 0;
 }
 
-static int apple_gpio_irq_set_type(struct irq_data *data,
-					unsigned int type)
+static int apple_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 {
-	struct apple_gpio_pinctrl *pctl =
-		gpiochip_get_data(irq_data_get_irq_chip_data(data));
+	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(irq_data_get_irq_chip_data(data));
 	unsigned int irqtype = apple_gpio_irq_type(type);
 
 	if (irqtype == REG_GPIOx_IN_IRQ_OFF)
 		return -EINVAL;
 
 	apple_gpio_set_reg(pctl, data->hwirq, REG_GPIOx_MODE,
-			   FIELD_PREP(REG_GPIOx_MODE, irqtype));
+	                   FIELD_PREP(REG_GPIOx_MODE, irqtype));
 
 	if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(data, handle_level_irq);
@@ -402,8 +390,7 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)
 		}
 
 		for (i = 0; i < girq->num_parents; i++) {
-			ret = platform_get_irq(to_platform_device(pctl->dev),
-					       i);
+			ret = platform_get_irq(to_platform_device(pctl->dev), i);
 			if (ret < 0)
 				goto out;
 
-- 
2.17.1

[PATCH v1 2/9] pinctrl: apple: add missing bits.h header

[Joey Gouly]

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index 23466ee59d79..4f7fb79d8e11 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -11,6 +11,7 @@
  */
 
 #include <dt-bindings/pinctrl/apple.h>
+#include <linux/bits.h>
 #include <linux/gpio/driver.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
-- 
2.17.1

[PATCH v1 3/9] pinctrl: apple: handle regmap_read errors

[Joey Gouly]

Explicitly return 0 if the regmap_read fails.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index 4f7fb79d8e11..bd7f28150ac0 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -84,9 +84,13 @@ static void apple_gpio_set_reg(struct apple_gpio_pinctrl *pctl,
 static u32 apple_gpio_get_reg(struct apple_gpio_pinctrl *pctl,
                               unsigned int pin)
 {
-	unsigned int val = 0;
+	int ret;
+	u32 val;
+
+	ret = regmap_read(pctl->map, REG_GPIO(pin), &val);
+	if (ret)
+		return 0;
 
-	regmap_read(pctl->map, REG_GPIO(pin), &val);
 	return val;
 }
 
-- 
2.17.1

[PATCH v1 4/9] pinctrl: apple: make apple_gpio_get_direction more readable

[Joey Gouly]

Try to make this more readable by not using a long line with
a ternary operator.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index bd7f28150ac0..fba2d4ec383c 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -194,8 +194,9 @@ static int apple_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(chip);
 	unsigned int reg = apple_gpio_get_reg(pctl, offset);
 
-	return (FIELD_GET(REG_GPIOx_MODE, reg) == REG_GPIOx_OUT) ?
-		       GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+	if (FIELD_GET(REG_GPIOx_MODE, reg) == REG_GPIOx_OUT)
+		return GPIO_LINE_DIRECTION_OUT;
+	return GPIO_LINE_DIRECTION_IN;
 }
 
 static int apple_gpio_get(struct gpio_chip *chip, unsigned offset)
-- 
2.17.1

[PATCH v1 5/9] pinctrl: apple: give error label a specific name

[Joey Gouly]

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index fba2d4ec383c..fb34cbe189ce 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -392,13 +392,13 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)
 					 GFP_KERNEL);
 		if (!girq->parents || !irq_data) {
 			ret = -ENOMEM;
-			goto out;
+			goto out_free_irq_data;
 		}
 
 		for (i = 0; i < girq->num_parents; i++) {
 			ret = platform_get_irq(to_platform_device(pctl->dev), i);
 			if (ret < 0)
-				goto out;
+				goto out_free_irq_data;
 
 			girq->parents[i] = ret;
 			pctl->irqgrps[i] = i;
@@ -412,7 +412,8 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)
 	}
 
 	ret = devm_gpiochip_add_data(pctl->dev, &pctl->gpio_chip, pctl);
-out:
+
+out_free_irq_data:
 	kfree(girq->parents);
 	kfree(irq_data);
 
-- 
2.17.1

[PATCH v1 6/9] pinctrl: apple: remove gpio-controller check

[Joey Gouly]

Having this missing, but everything else valid shouldn't result
in a failure.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index fb34cbe189ce..471d3abf9d99 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -360,10 +360,6 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)
 	void **irq_data = NULL;
 	int ret;
 
-	if (!of_property_read_bool(pctl->dev->of_node, "gpio-controller"))
-		return dev_err_probe(pctl->dev,	-ENODEV,
-				     "No gpio-controller property\n");
-
 	pctl->irq_chip = apple_gpio_irqchip;
 
 	pctl->gpio_chip.label = dev_name(pctl->dev);
-- 
2.17.1

[PATCH v1 7/9] pinctrl: apple: don't set gpio_chip.of_node

[Joey Gouly]

The gpio core code sets of_node, so no need to do it here.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index 471d3abf9d99..8e794ebb47da 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -373,7 +373,6 @@ static int apple_gpio_register(struct apple_gpio_pinctrl *pctl)
 	pctl->gpio_chip.base = -1;
 	pctl->gpio_chip.ngpio = pctl->pinctrl_desc.npins;
 	pctl->gpio_chip.parent = pctl->dev;
-	pctl->gpio_chip.of_node = pctl->dev->of_node;
 
 	if (girq->num_parents) {
 		int i;
-- 
2.17.1

[PATCH v1 8/9] pinctrl: apple: use modulo rather than bitwise and

[Joey Gouly]

This expresses the intention clearer.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index 8e794ebb47da..8a58c862c33a 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -252,7 +252,7 @@ static void apple_gpio_irq_ack(struct irq_data *data)
 	struct apple_gpio_pinctrl *pctl = gpiochip_get_data(irq_data_get_irq_chip_data(data));
 	unsigned int irqgrp = FIELD_GET(REG_GPIOx_GRP, apple_gpio_get_reg(pctl, data->hwirq));
 
-	writel(BIT(data->hwirq & 31), pctl->base + REG_IRQ(irqgrp, data->hwirq));
+	writel(BIT(data->hwirq % 32), pctl->base + REG_IRQ(irqgrp, data->hwirq));
 }
 
 static unsigned int apple_gpio_irq_type(unsigned int type)
-- 
2.17.1

[PATCH v1 9/9] pinctrl: apple: return an error if pinmux is missing in the DT

[Joey Gouly]

If of_property_count_u32_elems returned 0, return -EINVAL to indicate
a failure. Previously this would return 0.

Signed-off-by: Joey Gouly <joey.gouly@arm.com>
---
 drivers/pinctrl/pinctrl-apple-gpio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-apple-gpio.c b/drivers/pinctrl/pinctrl-apple-gpio.c
index 8a58c862c33a..89602aa14c6a 100644
--- a/drivers/pinctrl/pinctrl-apple-gpio.c
+++ b/drivers/pinctrl/pinctrl-apple-gpio.c
@@ -119,7 +119,7 @@ static int apple_gpio_dt_node_to_map(struct pinctrl_dev *pctldev,
 		dev_err(pctl->dev,
 			"missing or empty pinmux property in node %pOFn.\n",
 			node);
-		return ret;
+		return ret ? ret : -EINVAL;
 	}
 
 	num_pins = ret;
-- 
2.17.1

Re: [PATCH v1 0/9] pinctrl: apple: misc fixes from post-commit review

[Andy Shevchenko]

On Sat, Nov 13, 2021 at 6:30 PM Joey Gouly <joey.gouly@arm.com> wrote:
>
> Hi all,
>
> Here's a series of small fixes that Andy made comments about, after the driver
> was already in the pinctrl tree.
>
> Most of the patches are only a few lines, apart from the first which is a
> whitespace only change.
>
> A branch can be found here:
>     https://gitlab.arm.com/linux-arm/jg-open/-/tree/pinctrl_apple_fixes_v1
>
> The branch is based on torvald's tree, with Sven's recent fix [1] cherry-picked.
> (pinctrl fixes branch didn't have Sven's commit yet)

Thank you for doing this!

One suggestion here is to put the Suggested-by tag where it's applicable.
Side note: usually we still file a commit message body even if it
almost duplicates the subject line.

WRT patch 1, it seems to me it has a  lot of different style changes
at once. Perhaps split it more?

> Thanks,
> Joey
>
> [1]
> https://lore.kernel.org/linux-gpio/CACRpkdZJVRn9zAC8t=_X=29+kWw2QPKBHd9wiX9C3ms+gte40w@mail.gmail.com/T/
>
> Joey Gouly (9):
>   pinctrl: apple: fix some formatting issues
>   pinctrl: apple: add missing bits.h header
>   pinctrl: apple: handle regmap_read errors
>   pinctrl: apple: make apple_gpio_get_direction more readable
>   pinctrl: apple: give error label a specific name
>   pinctrl: apple: remove gpio-controller check
>   pinctrl: apple: don't set gpio_chip.of_node
>   pinctrl: apple: use modulo rather than bitwise and
>   pinctrl: apple: return an error if pinmux is missing in the DT
>
>  drivers/pinctrl/pinctrl-apple-gpio.c | 103 ++++++++++++---------------
>  1 file changed, 46 insertions(+), 57 deletions(-)
>
> --
> 2.17.1
>


-- 
With Best Regards,
Andy Shevchenko