mfd: arizona: Update reset pin to use GPIOD

Signed-off-by Charles Keepax <[email protected]>

Reviewed-by Rob Herring <[email protected]>

---

No changes since v2.

Thanks,
Charles

 Documentation/devicetree/bindings/mfd/arizona.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Now GPIOD has support for both pdata systems and for non-standard DT bindings the Arizona reset GPIO can be converted to use it.

Signed-off-by Charles Keepax <[email protected]>

---

Changes since v2:
- Kept null check in arizona_enable_reset, although gpiod_set_value_cansleep does its own null check it will also issue a fat WARN we don't want if gpiolib is not built in.
- Added a check for ENOSYS in arizona_of_get_core_pdata, just to silence the extra error that would be printed here if gpiolib is not built in.

Thanks,
Charles

 drivers/mfd/arizona-core.c        | 50 ++++++++++++++++++++++++++-------------
 include/linux/mfd/arizona/pdata.h |  3 ++-
 2 files changed, 36 insertions(+), 17 deletions(-)

drivers/mfd/arizona-core.c +10 -8

index 77875250abe5..9558c4d9c8ca 100644
@@ -279,7 +279,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 static inline void arizona_enable_reset(struct arizona *arizona)
 {
 	if (arizona->pdata.reset)
-		gpio_set_value_cansleep(arizona->pdata.reset, 0);
+		gpiod_set_value_cansleep(arizona->pdata.reset, 0);
 }
 
 static void arizona_disable_reset(struct arizona *arizona)

@@ -295,7 +295,7 @@ static void arizona_disable_reset(struct arizona *arizona)
 			break;
 		}
 
-		gpio_set_value_cansleep(arizona->pdata.reset, 1);
+		gpiod_set_value_cansleep(arizona->pdata.reset, 1);
 		usleep_range(1000, 5000);
 	}
 }

@@ -799,14 +799,26 @@ static int arizona_of_get_core_pdata(struct arizona *arizona)
 	struct arizona_pdata *pdata = &arizona->pdata;
 	int ret, i;
 
-	pdata->reset = of_get_named_gpio(arizona->dev->of_node, "wlf,reset", 0);
-	if (pdata->reset == -EPROBE_DEFER) {
-		return pdata->reset;
-	} else if (pdata->reset < 0) {
-		dev_err(arizona->dev, "Reset GPIO missing/malformed: %d\n",
-			pdata->reset);
+	pdata->reset = devm_gpiod_get_from_of_node(arizona->dev,
+						   arizona->dev->of_node,
+						   "wlf,reset", 0,
+						   GPIOD_OUT_LOW,
+						   "arizona /RESET");
+	if (IS_ERR(pdata->reset)) {
+		ret = PTR_ERR(pdata->reset);
+		switch (ret) {

drivers/mfd/arizona-core.c +2 -8

-		pdata->reset = 0;
+		pdata->reset = NULL;
 	}
 
 	ret = of_property_read_u32_array(arizona->dev->of_node,

@@ -1050,14 +1062,20 @@ int arizona_dev_init(struct arizona *arizona)
 		goto err_early;
 	}
 
-	if (arizona->pdata.reset) {
+	if (!arizona->pdata.reset) {
 		/* Start out with /RESET low to put the chip into reset */
-		ret = devm_gpio_request_one(arizona->dev, arizona->pdata.reset,
-					    GPIOF_DIR_OUT | GPIOF_INIT_LOW,
-					    "arizona /RESET");
-		if (ret != 0) {
-			dev_err(dev, "Failed to request /RESET: %d\n", ret);
-			goto err_dcvdd;

drivers/mfd/arizona-core.c +1 -0

+			arizona->pdata.reset = NULL;
 		}
 	}
 

include/linux/mfd/arizona/pdata.h +1 -0

index f72dc53848d7..0013075d4cda 100644
@@ -56,6 +56,7 @@
 #define ARIZONA_MAX_PDM_SPK 2
 
 struct regulator_init_data;
+struct gpio_desc;
 
 struct arizona_micbias {
 	int mV;                    /** Regulated voltage */

drivers/mfd/arizona-core.c static int arizona_of_get_core_pdata(struct arizona *arizona) · line 819

Quoted 13 lines · Charles Keepax

drivers/mfd/arizona-core.c +13 -0

+	if (IS_ERR(pdata->reset)) {
+		ret = PTR_ERR(pdata->reset);
+		switch (ret) {
+		case -ENOENT:
+		case -ENOSYS:
+			break;
+		case -EPROBE_DEFER:
+			return ret;
+		default:
+			dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
+				ret);
+			break;
+		}

I haven't seen a switch statement used in the error handling path before. There is probably good reason for that.

What is the 'default' case? -EINVAL?

Quoted 13 lines

drivers/mfd/arizona-core.c +5 -7

-	if (arizona->pdata.reset) {
+	if (!arizona->pdata.reset) {
 		/* Start out with /RESET low to put the chip into reset */
-		ret = devm_gpio_request_one(arizona->dev, arizona->pdata.reset,
-					    GPIOF_DIR_OUT | GPIOF_INIT_LOW,
-					    "arizona /RESET");
-		if (ret != 0) {
-			dev_err(dev, "Failed to request /RESET: %d\n", ret);
-			goto err_dcvdd;
+		arizona->pdata.reset = devm_gpiod_get(arizona->dev, "reset",
+						      GPIOD_OUT_LOW);
+		if (IS_ERR(arizona->pdata.reset)) {
+			ret = PTR_ERR(arizona->pdata.reset);

drivers/mfd/arizona-core.c int arizona_dev_init(struct arizona *arizona) · line 1076

Quoted 6 lines

drivers/mfd/arizona-core.c +6 -0

+			if (ret == -EPROBE_DEFER)
+				goto err_dcvdd;
+			else
+				dev_err(arizona->dev,
+					"Reset GPIO missing/malformed: %d\n",
+					ret);

You don't need the else.

Quoted 17 lines

include/linux/mfd/arizona/pdata.h +2 -1

@@ -56,6 +56,7 @@
 #define ARIZONA_MAX_PDM_SPK 2
 
 struct regulator_init_data;
+struct gpio_desc;
 
 struct arizona_micbias {
 	int mV;                    /** Regulated voltage */

@@ -77,7 +78,7 @@ struct arizona_micd_range {
 };
 
 struct arizona_pdata {
-	int reset;      /** GPIO controlling /RESET, if any */
+	struct gpio_desc *reset;      /** GPIO controlling /RESET, if any */
 

include/linux/mfd/arizona/pdata.h struct arizona_micd_range { · line 84

Quoted 2 lines

include/linux/mfd/arizona/pdata.h +0 -0

 	/** Regulator configuration for MICVDD */
 	struct arizona_micsupp_pdata micvdd;

I know it doesn't have much to do with this patch, but it's probably better to use a Kerneldoc header for this struct.

Quoted 18 lines · Lee Jones

drivers/mfd/arizona-core.c static int arizona_of_get_core_pdata(struct arizona *arizona) · line 819

Quoted 13 lines · Charles Keepax

drivers/mfd/arizona-core.c +13 -0

+	if (IS_ERR(pdata->reset)) {
+		ret = PTR_ERR(pdata->reset);
+		switch (ret) {
+		case -ENOENT:
+		case -ENOSYS:
+			break;
+		case -EPROBE_DEFER:
+			return ret;
+		default:
+			dev_err(arizona->dev, "Reset GPIO malformed: %d\n",
+				ret);
+			break;
+		}

I haven't seen a switch statement used in the error handling path before. There is probably good reason for that.

There are a fair few of them "grep -rI "switch (ret)" | wc -l" ==
205. Although to be fair that has rarely been an argument in somethings favour.

Quoted 2 lines

What is the 'default' case? -EINVAL?

I would guess most of the time yes, but I would rather not assume that. I can redo this as if's if you prefer it that way? The if is slightly less lines although I do think the switch is much clearer as to intent.

if (ret == -EPROBE_DEFER) {
	return ret;
} else if (ret != -ENOENT && ret != -ENOSYS {
	dev_err(arizona->dev, ....);
}

Quoted 7 lines · Charles Keepax

Quoted 1 line · Lee Jones

On Tue, 20 Feb 2018, Charles Keepax wrote:

I would guess most of the time yes, but I would rather not assume that. I can redo this as if's if you prefer it that way? The if is slightly less lines although I do think the switch is much clearer as to intent.

Quoted 5 lines

if (ret == -EPROBE_DEFER) {
	return ret;
} else if (ret != -ENOENT && ret != -ENOSYS {
	dev_err(arizona->dev, ....);
}

I don't know enough about the API to see why -ENOENT and -ENOSYS do not deserve error messages.

What do the other users of the API do?

drivers/mfd/arizona-core.c static int arizona_wait_for_boot(struct arizona *arizona) · line 282

Quoted 5 lines

static inline void arizona_enable_reset(struct arizona *arizona)

 {
        if (arizona->pdata.reset)

drivers/mfd/arizona-core.c +1 -1

-               gpio_set_value_cansleep(arizona->pdata.reset, 0);
+               gpiod_set_value_cansleep(arizona->pdata.reset, 0);

This should be:

gpiod_set_value_cansleep(arizona->pdata.reset, 1);

as here you want to put the reset GPIO into its active state.

Assuming that in the dts this GPIO is defined as GPIO_ACTIVE_LOW, then the command should put it to logic level 0 as done originally.

Quoted 5 lines

static void arizona_disable_reset(struct arizona *arizona)

(quoted diff) +0 -0

@@ -295,7 +295,7 @@ static void arizona_disable_reset(struct arizona *arizona)
                        break;
                }
 

drivers/mfd/arizona-core.c static void arizona_disable_reset(struct arizona *arizona) · line 298

Quoted 2 lines

drivers/mfd/arizona-core.c +1 -1

-               gpio_set_value_cansleep(arizona->pdata.reset, 1);
+               gpiod_set_value_cansleep(arizona->pdata.reset, 1);

This should be:

gpiod_set_value_cansleep(arizona->pdata.reset, 0);

as here you want to put the reset GPIO into its inactive state.

Hmm... this raises a rather good point that I hadn't considered. This driver is used in a lot of shipping devices and the old style GPIO calls didn't take the activeness of the GPIO into account just blindly setting the value you asked for. However the new style calls do take this into account.

The trouble is I guess we don't know whether most users bothered to set GPIO_ACTIVE_LOW or not. So it is very hard to say here if we are about to breaking a lot of existing device trees here.

I guess probably the safest approach is to use
gpiod_set_raw_value_cansleep here?