[PATCH v3 0/3] greybus: gpio: Switch to the gpio descriptor interface

[PATCH v3 0/3] greybus: gpio: Switch to the gpio descriptor interface

[Nishad Kamdar]

This patch series converts uses of the old GPIO API to the GPIO
descriptor API. It also converts the GPIO driver to use the
GPIO irqchip library GPIOLIB_IRQCHIP instead of repimplementing
the same.

Changes in v3:
 - Combines the latest versions of the three greybus patches together
   in a patch series.

Nishad Kamdar (3):
  staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP
  staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor
    interface
  staging: greybus: arche-platform: Switch to the gpio descriptor
    interface

 drivers/staging/greybus/Kconfig          |   1 +
 drivers/staging/greybus/arche-apb-ctrl.c | 159 ++++++++------------
 drivers/staging/greybus/arche-platform.c | 119 +++++----------
 drivers/staging/greybus/gpio.c           | 184 +++--------------------
 4 files changed, 130 insertions(+), 333 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Nishad Kamdar]

Convert the GPIO driver to use the GPIO irqchip library
GPIOLIB_IRQCHIP instead of reimplementing the same.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
Changes in v2:
 - Retained irq.h and irqdomain.h headers.
 - Dropped function gb_gpio_irqchip_add() and
   called gpiochip_irqchip_add() from probe().
 - Referred https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.walleij@linaro.org.
---
 drivers/staging/greybus/Kconfig |   1 +
 drivers/staging/greybus/gpio.c  | 184 ++++----------------------------
 2 files changed, 24 insertions(+), 161 deletions(-)

diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
index ab096bcef98c..b571e4e8060b 100644
--- a/drivers/staging/greybus/Kconfig
+++ b/drivers/staging/greybus/Kconfig
@@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
 config GREYBUS_GPIO
 	tristate "Greybus GPIO Bridged PHY driver"
 	depends on GPIOLIB
+	select GPIOLIB_IRQCHIP
 	---help---
 	  Select this option if you have a device that follows the
 	  Greybus GPIO Bridged PHY Class specification.
diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
index b1d4698019a1..2ec54744171d 100644
--- a/drivers/staging/greybus/gpio.c
+++ b/drivers/staging/greybus/gpio.c
@@ -9,9 +9,9 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/gpio.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
+#include <linux/gpio/driver.h>
 #include <linux/mutex.h>
 
 #include "greybus.h"
@@ -39,15 +39,8 @@ struct gb_gpio_controller {
 
 	struct gpio_chip	chip;
 	struct irq_chip		irqc;
-	struct irq_chip		*irqchip;
-	struct irq_domain	*irqdomain;
-	unsigned int		irq_base;
-	irq_flow_handler_t	irq_handler;
-	unsigned int		irq_default_type;
 	struct mutex		irq_lock;
 };
-#define gpio_chip_to_gb_gpio_controller(chip) \
-	container_of(chip, struct gb_gpio_controller, chip)
 #define irq_data_to_gpio_chip(d) (d->domain->host_data)
 
 static int gb_gpio_line_count_operation(struct gb_gpio_controller *ggc)
@@ -276,7 +269,7 @@ static void _gb_gpio_irq_set_type(struct gb_gpio_controller *ggc,
 static void gb_gpio_irq_mask(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 
 	line->masked = true;
@@ -286,7 +279,7 @@ static void gb_gpio_irq_mask(struct irq_data *d)
 static void gb_gpio_irq_unmask(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 
 	line->masked = false;
@@ -296,7 +289,7 @@ static void gb_gpio_irq_unmask(struct irq_data *d)
 static int gb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 	struct device *dev = &ggc->gbphy_dev->dev;
 	u8 irq_type;
@@ -334,7 +327,7 @@ static int gb_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 static void gb_gpio_irq_bus_lock(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	mutex_lock(&ggc->irq_lock);
 }
@@ -342,7 +335,7 @@ static void gb_gpio_irq_bus_lock(struct irq_data *d)
 static void gb_gpio_irq_bus_sync_unlock(struct irq_data *d)
 {
 	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	struct gb_gpio_line *line = &ggc->lines[d->hwirq];
 
 	if (line->irq_type_pending) {
@@ -391,7 +384,7 @@ static int gb_gpio_request_handler(struct gb_operation *op)
 		return -EINVAL;
 	}
 
-	irq = irq_find_mapping(ggc->irqdomain, event->which);
+	irq = irq_find_mapping(ggc->chip.irq.domain, event->which);
 	if (!irq) {
 		dev_err(dev, "failed to find IRQ\n");
 		return -EINVAL;
@@ -411,21 +404,21 @@ static int gb_gpio_request_handler(struct gb_operation *op)
 
 static int gb_gpio_request(struct gpio_chip *chip, unsigned int offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	return gb_gpio_activate_operation(ggc, (u8)offset);
 }
 
 static void gb_gpio_free(struct gpio_chip *chip, unsigned int offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	gb_gpio_deactivate_operation(ggc, (u8)offset);
 }
 
 static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	u8 which;
 	int ret;
 
@@ -439,7 +432,7 @@ static int gb_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 
 static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	return gb_gpio_direction_in_operation(ggc, (u8)offset);
 }
@@ -447,14 +440,14 @@ static int gb_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
 static int gb_gpio_direction_output(struct gpio_chip *chip, unsigned int offset,
 				    int value)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	return gb_gpio_direction_out_operation(ggc, (u8)offset, !!value);
 }
 
 static int gb_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	u8 which;
 	int ret;
 
@@ -468,7 +461,7 @@ static int gb_gpio_get(struct gpio_chip *chip, unsigned int offset)
 
 static void gb_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 
 	gb_gpio_set_value_operation(ggc, (u8)offset, !!value);
 }
@@ -476,7 +469,7 @@ static void gb_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 static int gb_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 			      unsigned long config)
 {
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
+	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
 	u32 debounce;
 
 	if (pinconf_to_config_param(config) != PIN_CONFIG_INPUT_DEBOUNCE)
@@ -506,135 +499,6 @@ static int gb_gpio_controller_setup(struct gb_gpio_controller *ggc)
 	return ret;
 }
 
-/**
- * gb_gpio_irq_map() - maps an IRQ into a GB gpio irqchip
- * @d: the irqdomain used by this irqchip
- * @irq: the global irq number used by this GB gpio irqchip irq
- * @hwirq: the local IRQ/GPIO line offset on this GB gpio
- *
- * This function will set up the mapping for a certain IRQ line on a
- * GB gpio by assigning the GB gpio as chip data, and using the irqchip
- * stored inside the GB gpio.
- */
-static int gb_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
-			   irq_hw_number_t hwirq)
-{
-	struct gpio_chip *chip = domain->host_data;
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
-
-	irq_set_chip_data(irq, ggc);
-	irq_set_chip_and_handler(irq, ggc->irqchip, ggc->irq_handler);
-	irq_set_noprobe(irq);
-	/*
-	 * No set-up of the hardware will happen if IRQ_TYPE_NONE
-	 * is passed as default type.
-	 */
-	if (ggc->irq_default_type != IRQ_TYPE_NONE)
-		irq_set_irq_type(irq, ggc->irq_default_type);
-
-	return 0;
-}
-
-static void gb_gpio_irq_unmap(struct irq_domain *d, unsigned int irq)
-{
-	irq_set_chip_and_handler(irq, NULL, NULL);
-	irq_set_chip_data(irq, NULL);
-}
-
-static const struct irq_domain_ops gb_gpio_domain_ops = {
-	.map	= gb_gpio_irq_map,
-	.unmap	= gb_gpio_irq_unmap,
-};
-
-/**
- * gb_gpio_irqchip_remove() - removes an irqchip added to a gb_gpio_controller
- * @ggc: the gb_gpio_controller to remove the irqchip from
- *
- * This is called only from gb_gpio_remove()
- */
-static void gb_gpio_irqchip_remove(struct gb_gpio_controller *ggc)
-{
-	unsigned int offset;
-
-	/* Remove all IRQ mappings and delete the domain */
-	if (ggc->irqdomain) {
-		for (offset = 0; offset < (ggc->line_max + 1); offset++)
-			irq_dispose_mapping(irq_find_mapping(ggc->irqdomain,
-							     offset));
-		irq_domain_remove(ggc->irqdomain);
-	}
-
-	if (ggc->irqchip)
-		ggc->irqchip = NULL;
-}
-
-/**
- * gb_gpio_irqchip_add() - adds an irqchip to a gpio chip
- * @chip: the gpio chip to add the irqchip to
- * @irqchip: the irqchip to add to the adapter
- * @first_irq: if not dynamically assigned, the base (first) IRQ to
- * allocate gpio irqs from
- * @handler: the irq handler to use (often a predefined irq core function)
- * @type: the default type for IRQs on this irqchip, pass IRQ_TYPE_NONE
- * to have the core avoid setting up any default type in the hardware.
- *
- * This function closely associates a certain irqchip with a certain
- * gpio chip, providing an irq domain to translate the local IRQs to
- * global irqs, and making sure that the gpio chip
- * is passed as chip data to all related functions. Driver callbacks
- * need to use container_of() to get their local state containers back
- * from the gpio chip passed as chip data. An irqdomain will be stored
- * in the gpio chip that shall be used by the driver to handle IRQ number
- * translation. The gpio chip will need to be initialized and registered
- * before calling this function.
- */
-static int gb_gpio_irqchip_add(struct gpio_chip *chip,
-			 struct irq_chip *irqchip,
-			 unsigned int first_irq,
-			 irq_flow_handler_t handler,
-			 unsigned int type)
-{
-	struct gb_gpio_controller *ggc;
-	unsigned int offset;
-	unsigned int irq_base;
-
-	if (!chip || !irqchip)
-		return -EINVAL;
-
-	ggc = gpio_chip_to_gb_gpio_controller(chip);
-
-	ggc->irqchip = irqchip;
-	ggc->irq_handler = handler;
-	ggc->irq_default_type = type;
-	ggc->irqdomain = irq_domain_add_simple(NULL,
-					ggc->line_max + 1, first_irq,
-					&gb_gpio_domain_ops, chip);
-	if (!ggc->irqdomain) {
-		ggc->irqchip = NULL;
-		return -EINVAL;
-	}
-
-	/*
-	 * Prepare the mapping since the irqchip shall be orthogonal to
-	 * any gpio calls. If the first_irq was zero, this is
-	 * necessary to allocate descriptors for all IRQs.
-	 */
-	for (offset = 0; offset < (ggc->line_max + 1); offset++) {
-		irq_base = irq_create_mapping(ggc->irqdomain, offset);
-		if (offset == 0)
-			ggc->irq_base = irq_base;
-	}
-
-	return 0;
-}
-
-static int gb_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
-{
-	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
-
-	return irq_find_mapping(ggc->irqdomain, offset);
-}
-
 static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
 			 const struct gbphy_device_id *id)
 {
@@ -693,7 +557,6 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
 	gpio->get = gb_gpio_get;
 	gpio->set = gb_gpio_set;
 	gpio->set_config = gb_gpio_set_config;
-	gpio->to_irq = gb_gpio_to_irq;
 	gpio->base = -1;		/* Allocate base dynamically */
 	gpio->ngpio = ggc->line_max + 1;
 	gpio->can_sleep = true;
@@ -702,24 +565,24 @@ static int gb_gpio_probe(struct gbphy_device *gbphy_dev,
 	if (ret)
 		goto exit_line_free;
 
-	ret = gb_gpio_irqchip_add(gpio, irqc, 0,
-				   handle_level_irq, IRQ_TYPE_NONE);
+	ret = gpiochip_add(gpio);
 	if (ret) {
-		dev_err(&gbphy_dev->dev, "failed to add irq chip: %d\n", ret);
+		dev_err(&gbphy_dev->dev, "failed to add gpio chip: %d\n", ret);
 		goto exit_line_free;
 	}
 
-	ret = gpiochip_add(gpio);
+	ret = gpiochip_irqchip_add(gpio, irqc, 0, handle_level_irq,
+				   IRQ_TYPE_NONE);
 	if (ret) {
-		dev_err(&gbphy_dev->dev, "failed to add gpio chip: %d\n", ret);
-		goto exit_gpio_irqchip_remove;
+		dev_err(&gbphy_dev->dev, "failed to add irq chip: %d\n", ret);
+		goto exit_gpiochip_remove;
 	}
 
 	gbphy_runtime_put_autosuspend(gbphy_dev);
 	return 0;
 
-exit_gpio_irqchip_remove:
-	gb_gpio_irqchip_remove(ggc);
+exit_gpiochip_remove:
+	gpiochip_remove(gpio);
 exit_line_free:
 	kfree(ggc->lines);
 exit_connection_disable:
@@ -743,7 +606,6 @@ static void gb_gpio_remove(struct gbphy_device *gbphy_dev)
 
 	gb_connection_disable_rx(connection);
 	gpiochip_remove(&ggc->chip);
-	gb_gpio_irqchip_remove(ggc);
 	gb_connection_disable(connection);
 	gb_connection_destroy(connection);
 	kfree(ggc->lines);
-- 
2.17.1

Re: [PATCH v3 1/3] staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Johan Hovold]

On Thu, Nov 22, 2018 at 10:37:16PM +0530, Nishad Kamdar wrote:
> Convert the GPIO driver to use the GPIO irqchip library
> GPIOLIB_IRQCHIP instead of reimplementing the same.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
> Changes in v2:
>  - Retained irq.h and irqdomain.h headers.
>  - Dropped function gb_gpio_irqchip_add() and
>    called gpiochip_irqchip_add() from probe().
>  - Referred https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.walleij@linaro.org.

Thanks for the update, and sorry about the late review. This looks
mostly good now, except for a couple minor things pointed out below.

You also included the conversion to gpiochip_get_data() (as Linus also
did in his patch) although that's really a separate change and should go
in its own patch. Please break that bit out in a follow-up patch.

Also note that someone did a bunch random white space changes to this
file in the staging tree, so it will not apply cleanly any more.

> ---
>  drivers/staging/greybus/Kconfig |   1 +
>  drivers/staging/greybus/gpio.c  | 184 ++++----------------------------
>  2 files changed, 24 insertions(+), 161 deletions(-)
> 
> diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
> index ab096bcef98c..b571e4e8060b 100644
> --- a/drivers/staging/greybus/Kconfig
> +++ b/drivers/staging/greybus/Kconfig
> @@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
>  config GREYBUS_GPIO
>  	tristate "Greybus GPIO Bridged PHY driver"
>  	depends on GPIOLIB
> +	select GPIOLIB_IRQCHIP
>  	---help---
>  	  Select this option if you have a device that follows the
>  	  Greybus GPIO Bridged PHY Class specification.
> diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> index b1d4698019a1..2ec54744171d 100644
> --- a/drivers/staging/greybus/gpio.c
> +++ b/drivers/staging/greybus/gpio.c
> @@ -9,9 +9,9 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> -#include <linux/gpio.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> +#include <linux/gpio/driver.h>
>  #include <linux/mutex.h>
>  
>  #include "greybus.h"
> @@ -39,15 +39,8 @@ struct gb_gpio_controller {
>  
>  	struct gpio_chip	chip;
>  	struct irq_chip		irqc;

Turns out struct gpio_chip will have an irqchip whenever
CONFIG_GPIOLIB_IRQCHIP is selected so you can drop this one too.

> -	struct irq_chip		*irqchip;
> -	struct irq_domain	*irqdomain;
> -	unsigned int		irq_base;
> -	irq_flow_handler_t	irq_handler;
> -	unsigned int		irq_default_type;
>  	struct mutex		irq_lock;
>  };
> -#define gpio_chip_to_gb_gpio_controller(chip) \
> -	container_of(chip, struct gb_gpio_controller, chip)
>  #define irq_data_to_gpio_chip(d) (d->domain->host_data)
>  
>  static int gb_gpio_line_count_operation(struct gb_gpio_controller *ggc)
> @@ -276,7 +269,7 @@ static void _gb_gpio_irq_set_type(struct gb_gpio_controller *ggc,
>  static void gb_gpio_irq_mask(struct irq_data *d)
>  {
>  	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
> -	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
> +	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);

So please split these changes into a separate patch as they are not
related to the irqchip changes.

Oh, and don't forget to update the TODO file now that the conversion is
done. :)

Thanks,
Johan

Re: [PATCH v3 1/3] staging: greybus: gpio: switch GPIO portions to use GPIOLIB_IRQCHIP

[Nishad Kamdar]

On Tue, Dec 18, 2018 at 12:10:34PM +0100, Johan Hovold wrote:
> On Thu, Nov 22, 2018 at 10:37:16PM +0530, Nishad Kamdar wrote:
> > Convert the GPIO driver to use the GPIO irqchip library
> > GPIOLIB_IRQCHIP instead of reimplementing the same.
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > ---
> > Changes in v2:
> >  - Retained irq.h and irqdomain.h headers.
> >  - Dropped function gb_gpio_irqchip_add() and
> >    called gpiochip_irqchip_add() from probe().
> >  - Referred https://lkml.kernel.org/r/1476054589-28422-1-git-send-email-linus.walleij@linaro.org.
> 
> Thanks for the update, and sorry about the late review. This looks
> mostly good now, except for a couple minor things pointed out below.
> 
> You also included the conversion to gpiochip_get_data() (as Linus also
> did in his patch) although that's really a separate change and should go
> in its own patch. Please break that bit out in a follow-up patch.
> 
> Also note that someone did a bunch random white space changes to this
> file in the staging tree, so it will not apply cleanly any more.
> 
> > ---
> >  drivers/staging/greybus/Kconfig |   1 +
> >  drivers/staging/greybus/gpio.c  | 184 ++++----------------------------
> >  2 files changed, 24 insertions(+), 161 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/Kconfig b/drivers/staging/greybus/Kconfig
> > index ab096bcef98c..b571e4e8060b 100644
> > --- a/drivers/staging/greybus/Kconfig
> > +++ b/drivers/staging/greybus/Kconfig
> > @@ -148,6 +148,7 @@ if GREYBUS_BRIDGED_PHY
> >  config GREYBUS_GPIO
> >  	tristate "Greybus GPIO Bridged PHY driver"
> >  	depends on GPIOLIB
> > +	select GPIOLIB_IRQCHIP
> >  	---help---
> >  	  Select this option if you have a device that follows the
> >  	  Greybus GPIO Bridged PHY Class specification.
> > diff --git a/drivers/staging/greybus/gpio.c b/drivers/staging/greybus/gpio.c
> > index b1d4698019a1..2ec54744171d 100644
> > --- a/drivers/staging/greybus/gpio.c
> > +++ b/drivers/staging/greybus/gpio.c
> > @@ -9,9 +9,9 @@
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> > -#include <linux/gpio.h>
> >  #include <linux/irq.h>
> >  #include <linux/irqdomain.h>
> > +#include <linux/gpio/driver.h>
> >  #include <linux/mutex.h>
> >  
> >  #include "greybus.h"
> > @@ -39,15 +39,8 @@ struct gb_gpio_controller {
> >  
> >  	struct gpio_chip	chip;
> >  	struct irq_chip		irqc;
> 
> Turns out struct gpio_chip will have an irqchip whenever
> CONFIG_GPIOLIB_IRQCHIP is selected so you can drop this one too.
> 
Ok, I'll do that.
> > -	struct irq_chip		*irqchip;
> > -	struct irq_domain	*irqdomain;
> > -	unsigned int		irq_base;
> > -	irq_flow_handler_t	irq_handler;
> > -	unsigned int		irq_default_type;
> >  	struct mutex		irq_lock;
> >  };
> > -#define gpio_chip_to_gb_gpio_controller(chip) \
> > -	container_of(chip, struct gb_gpio_controller, chip)
> >  #define irq_data_to_gpio_chip(d) (d->domain->host_data)
> >  
> >  static int gb_gpio_line_count_operation(struct gb_gpio_controller *ggc)
> > @@ -276,7 +269,7 @@ static void _gb_gpio_irq_set_type(struct gb_gpio_controller *ggc,
> >  static void gb_gpio_irq_mask(struct irq_data *d)
> >  {
> >  	struct gpio_chip *chip = irq_data_to_gpio_chip(d);
> > -	struct gb_gpio_controller *ggc = gpio_chip_to_gb_gpio_controller(chip);
> > +	struct gb_gpio_controller *ggc = gpiochip_get_data(chip);
> 
> So please split these changes into a separate patch as they are not
> related to the irqchip changes.
> 
> Oh, and don't forget to update the TODO file now that the conversion is
> done. :)
> 
Sure, I'll do that.

Thanks for the review.
regards,
Nishad

> Thanks,
> Johan

[PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Nishad Kamdar]

Use the gpiod interface instead of the deprecated old non-descriptor
interface.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
Changes in v2:
 - Resolved compilation errors.
---
 drivers/staging/greybus/arche-apb-ctrl.c | 159 +++++++++--------------
 1 file changed, 65 insertions(+), 94 deletions(-)

diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
index be5ffed90bcf..e887f6aee20b 100644
--- a/drivers/staging/greybus/arche-apb-ctrl.c
+++ b/drivers/staging/greybus/arche-apb-ctrl.c
@@ -8,9 +8,8 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/interrupt.h>
-#include <linux/of_gpio.h>
 #include <linux/of_irq.h>
 #include <linux/module.h>
 #include <linux/pinctrl/consumer.h>
@@ -24,12 +23,12 @@ static void apb_bootret_deassert(struct device *dev);
 
 struct arche_apb_ctrl_drvdata {
 	/* Control GPIO signals to and from AP <=> AP Bridges */
-	int resetn_gpio;
-	int boot_ret_gpio;
-	int pwroff_gpio;
-	int wake_in_gpio;
-	int wake_out_gpio;
-	int pwrdn_gpio;
+	struct gpio_desc *resetn;
+	struct gpio_desc *boot_ret;
+	struct gpio_desc *pwroff;
+	struct gpio_desc *wake_in;
+	struct gpio_desc *wake_out;
+	struct gpio_desc *pwrdn;
 
 	enum arche_platform_state state;
 	bool init_disabled;
@@ -37,28 +36,28 @@ struct arche_apb_ctrl_drvdata {
 	struct regulator *vcore;
 	struct regulator *vio;
 
-	int clk_en_gpio;
+	struct gpio_desc *clk_en;
 	struct clk *clk;
 
 	struct pinctrl *pinctrl;
 	struct pinctrl_state *pin_default;
 
 	/* V2: SPI Bus control  */
-	int spi_en_gpio;
+	struct gpio_desc *spi_en;
 	bool spi_en_polarity_high;
 };
 
 /*
  * Note that these low level api's are active high
  */
-static inline void deassert_reset(unsigned int gpio)
+static inline void deassert_reset(struct gpio_desc *gpio)
 {
-	gpio_set_value(gpio, 1);
+	gpiod_set_value(gpio, 1);
 }
 
-static inline void assert_reset(unsigned int gpio)
+static inline void assert_reset(struct gpio_desc *gpio)
 {
-	gpio_set_value(gpio, 0);
+	gpiod_set_value(gpio, 0);
 }
 
 /*
@@ -75,11 +74,11 @@ static int coldboot_seq(struct platform_device *pdev)
 		return 0;
 
 	/* Hold APB in reset state */
-	assert_reset(apb->resetn_gpio);
+	assert_reset(apb->resetn);
 
 	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
-	    gpio_is_valid(apb->spi_en_gpio))
-		devm_gpio_free(dev, apb->spi_en_gpio);
+	    apb->spi_en)
+		devm_gpiod_put(dev, apb->spi_en);
 
 	/* Enable power to APB */
 	if (!IS_ERR(apb->vcore)) {
@@ -101,13 +100,13 @@ static int coldboot_seq(struct platform_device *pdev)
 	apb_bootret_deassert(dev);
 
 	/* On DB3 clock was not mandatory */
-	if (gpio_is_valid(apb->clk_en_gpio))
-		gpio_set_value(apb->clk_en_gpio, 1);
+	if (apb->clk_en)
+		gpiod_set_value(apb->clk_en, 1);
 
 	usleep_range(100, 200);
 
 	/* deassert reset to APB : Active-low signal */
-	deassert_reset(apb->resetn_gpio);
+	deassert_reset(apb->resetn);
 
 	apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
 
@@ -119,6 +118,7 @@ static int fw_flashing_seq(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct arche_apb_ctrl_drvdata *apb = platform_get_drvdata(pdev);
 	int ret;
+	unsigned long flags;
 
 	if (apb->init_disabled ||
 	    apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING)
@@ -136,25 +136,20 @@ static int fw_flashing_seq(struct platform_device *pdev)
 		return ret;
 	}
 
-	if (gpio_is_valid(apb->spi_en_gpio)) {
-		unsigned long flags;
+	if (apb->spi_en_polarity_high)
+		flags = GPIOD_OUT_HIGH;
+	else
+		flags = GPIOD_OUT_LOW;
 
-		if (apb->spi_en_polarity_high)
-			flags = GPIOF_OUT_INIT_HIGH;
-		else
-			flags = GPIOF_OUT_INIT_LOW;
-
-		ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
-					    flags, "apb_spi_en");
-		if (ret) {
-			dev_err(dev, "Failed requesting SPI bus en gpio %d\n",
-				apb->spi_en_gpio);
-			return ret;
-		}
+	apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);
+	if (IS_ERR(apb->spi_en)) {
+		ret = PTR_ERR(apb->spi_en);
+		dev_err(dev, "Failed requesting SPI bus en GPIO: %d\n", ret);
+		return ret;
 	}
 
 	/* for flashing device should be in reset state */
-	assert_reset(apb->resetn_gpio);
+	assert_reset(apb->resetn);
 	apb->state = ARCHE_PLATFORM_STATE_FW_FLASHING;
 
 	return 0;
@@ -177,8 +172,8 @@ static int standby_boot_seq(struct platform_device *pdev)
 		return 0;
 
 	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
-	    gpio_is_valid(apb->spi_en_gpio))
-		devm_gpio_free(dev, apb->spi_en_gpio);
+	    apb->spi_en)
+		devm_gpiod_put(dev, apb->spi_en);
 
 	/*
 	 * As per WDM spec, do nothing
@@ -202,12 +197,12 @@ static void poweroff_seq(struct platform_device *pdev)
 		return;
 
 	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
-	    gpio_is_valid(apb->spi_en_gpio))
-		devm_gpio_free(dev, apb->spi_en_gpio);
+	    apb->spi_en)
+		devm_gpiod_put(dev, apb->spi_en);
 
 	/* disable the clock */
-	if (gpio_is_valid(apb->clk_en_gpio))
-		gpio_set_value(apb->clk_en_gpio, 0);
+	if (apb->clk_en)
+		gpiod_set_value(apb->clk_en, 0);
 
 	if (!IS_ERR(apb->vcore) && regulator_is_enabled(apb->vcore) > 0)
 		regulator_disable(apb->vcore);
@@ -216,7 +211,7 @@ static void poweroff_seq(struct platform_device *pdev)
 		regulator_disable(apb->vio);
 
 	/* As part of exit, put APB back in reset state */
-	assert_reset(apb->resetn_gpio);
+	assert_reset(apb->resetn);
 	apb->state = ARCHE_PLATFORM_STATE_OFF;
 
 	/* TODO: May have to send an event to SVC about this exit */
@@ -226,7 +221,7 @@ static void apb_bootret_deassert(struct device *dev)
 {
 	struct arche_apb_ctrl_drvdata *apb = dev_get_drvdata(dev);
 
-	gpio_set_value(apb->boot_ret_gpio, 0);
+	gpiod_set_value(apb->boot_ret, 0);
 }
 
 int apb_ctrl_coldboot(struct device *dev)
@@ -322,66 +317,46 @@ static int apb_ctrl_get_devtree_data(struct platform_device *pdev,
 				     struct arche_apb_ctrl_drvdata *apb)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *np = dev->of_node;
 	int ret;
 
-	apb->resetn_gpio = of_get_named_gpio(np, "reset-gpios", 0);
-	if (apb->resetn_gpio < 0) {
-		dev_err(dev, "failed to get reset gpio\n");
-		return apb->resetn_gpio;
-	}
-	ret = devm_gpio_request_one(dev, apb->resetn_gpio,
-				    GPIOF_OUT_INIT_LOW, "apb-reset");
-	if (ret) {
-		dev_err(dev, "Failed requesting reset gpio %d\n",
-			apb->resetn_gpio);
+	apb->resetn = devm_gpiod_get(dev, "gb,reset-gpios", GPIOD_OUT_LOW);
+	if (IS_ERR(apb->resetn)) {
+		ret = PTR_ERR(apb->resetn);
+		dev_err(dev, "Failed requesting reset GPIO: %d\n", ret);
 		return ret;
 	}
 
-	apb->boot_ret_gpio = of_get_named_gpio(np, "boot-ret-gpios", 0);
-	if (apb->boot_ret_gpio < 0) {
-		dev_err(dev, "failed to get boot retention gpio\n");
-		return apb->boot_ret_gpio;
-	}
-	ret = devm_gpio_request_one(dev, apb->boot_ret_gpio,
-				    GPIOF_OUT_INIT_LOW, "boot retention");
-	if (ret) {
-		dev_err(dev, "Failed requesting bootret gpio %d\n",
-			apb->boot_ret_gpio);
+	apb->boot_ret = devm_gpiod_get(dev, "gb,boot-ret-gpios", GPIOD_OUT_LOW);
+	if (IS_ERR(apb->boot_ret)) {
+		ret = PTR_ERR(apb->boot_ret);
+		dev_err(dev, "Failed requesting bootret GPIO: %d\n", ret);
 		return ret;
 	}
 
 	/* It's not mandatory to support power management interface */
-	apb->pwroff_gpio = of_get_named_gpio(np, "pwr-off-gpios", 0);
-	if (apb->pwroff_gpio < 0) {
-		dev_err(dev, "failed to get power off gpio\n");
-		return apb->pwroff_gpio;
-	}
-	ret = devm_gpio_request_one(dev, apb->pwroff_gpio,
-				    GPIOF_IN, "pwroff_n");
-	if (ret) {
-		dev_err(dev, "Failed requesting pwroff_n gpio %d\n",
-			apb->pwroff_gpio);
+	apb->pwroff = devm_gpiod_get_optional(dev, "gb,pwr-off-gpios",
+					      GPIOD_IN);
+	if (IS_ERR(apb->pwroff)) {
+		ret = PTR_ERR(apb->pwroff);
+		dev_err(dev, "Failed requesting pwroff_n GPIO: %d\n", ret);
 		return ret;
 	}
 
 	/* Do not make clock mandatory as of now (for DB3) */
-	apb->clk_en_gpio = of_get_named_gpio(np, "clock-en-gpio", 0);
-	if (apb->clk_en_gpio < 0) {
-		dev_warn(dev, "failed to get clock en gpio\n");
-	} else if (gpio_is_valid(apb->clk_en_gpio)) {
-		ret = devm_gpio_request_one(dev, apb->clk_en_gpio,
-					    GPIOF_OUT_INIT_LOW, "apb_clk_en");
-		if (ret) {
-			dev_warn(dev, "Failed requesting APB clock en gpio %d\n",
-				 apb->clk_en_gpio);
-			return ret;
-		}
+	apb->clk_en = devm_gpiod_get_optional(dev, "gb,clock-en-gpio",
+					      GPIOD_OUT_LOW);
+	if (IS_ERR(apb->clk_en)) {
+		ret = PTR_ERR(apb->clk_en);
+		dev_err(dev, "Failed requesting APB clock en GPIO: %d\n", ret);
+		return ret;
 	}
 
-	apb->pwrdn_gpio = of_get_named_gpio(np, "pwr-down-gpios", 0);
-	if (apb->pwrdn_gpio < 0)
-		dev_warn(dev, "failed to get power down gpio\n");
+	apb->pwrdn = devm_gpiod_get(dev, "gb,pwr-down-gpios", GPIOD_OUT_LOW);
+	if (IS_ERR(apb->pwrdn)) {
+		ret = PTR_ERR(apb->pwrdn);
+		dev_warn(dev, "Failed requesting power down GPIO: %d\n", ret);
+		return ret;
+	}
 
 	/* Regulators are optional, as we may have fixed supply coming in */
 	apb->vcore = devm_regulator_get(dev, "vcore");
@@ -404,12 +379,8 @@ static int apb_ctrl_get_devtree_data(struct platform_device *pdev,
 	}
 
 	/* Only applicable for platform >= V2 */
-	apb->spi_en_gpio = of_get_named_gpio(np, "spi-en-gpio", 0);
-	if (apb->spi_en_gpio >= 0) {
-		if (of_property_read_bool(pdev->dev.of_node,
-					  "spi-en-active-high"))
-			apb->spi_en_polarity_high = true;
-	}
+	if (of_property_read_bool(pdev->dev.of_node, "gb,spi-en-active-high"))
+		apb->spi_en_polarity_high = true;
 
 	return 0;
 }
-- 
2.17.1

Re: [PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Johan Hovold]

On Thu, Nov 22, 2018 at 10:38:18PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated old non-descriptor
> interface.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
> Changes in v2:
>  - Resolved compilation errors.
> ---
>  drivers/staging/greybus/arche-apb-ctrl.c | 159 +++++++++--------------
>  1 file changed, 65 insertions(+), 94 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> index be5ffed90bcf..e887f6aee20b 100644
> --- a/drivers/staging/greybus/arche-apb-ctrl.c
> +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> @@ -8,9 +8,8 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/interrupt.h>
> -#include <linux/of_gpio.h>
>  #include <linux/of_irq.h>
>  #include <linux/module.h>
>  #include <linux/pinctrl/consumer.h>
> @@ -24,12 +23,12 @@ static void apb_bootret_deassert(struct device *dev);
>  
>  struct arche_apb_ctrl_drvdata {
>  	/* Control GPIO signals to and from AP <=> AP Bridges */
> -	int resetn_gpio;
> -	int boot_ret_gpio;
> -	int pwroff_gpio;
> -	int wake_in_gpio;
> -	int wake_out_gpio;
> -	int pwrdn_gpio;
> +	struct gpio_desc *resetn;
> +	struct gpio_desc *boot_ret;
> +	struct gpio_desc *pwroff;
> +	struct gpio_desc *wake_in;
> +	struct gpio_desc *wake_out;
> +	struct gpio_desc *pwrdn;
>  
>  	enum arche_platform_state state;
>  	bool init_disabled;
> @@ -37,28 +36,28 @@ struct arche_apb_ctrl_drvdata {
>  	struct regulator *vcore;
>  	struct regulator *vio;
>  
> -	int clk_en_gpio;
> +	struct gpio_desc *clk_en;
>  	struct clk *clk;
>  
>  	struct pinctrl *pinctrl;
>  	struct pinctrl_state *pin_default;
>  
>  	/* V2: SPI Bus control  */
> -	int spi_en_gpio;
> +	struct gpio_desc *spi_en;
>  	bool spi_en_polarity_high;
>  };
>  
>  /*
>   * Note that these low level api's are active high
>   */
> -static inline void deassert_reset(unsigned int gpio)
> +static inline void deassert_reset(struct gpio_desc *gpio)
>  {
> -	gpio_set_value(gpio, 1);
> +	gpiod_set_value(gpio, 1);
>  }
>  
> -static inline void assert_reset(unsigned int gpio)
> +static inline void assert_reset(struct gpio_desc *gpio)
>  {
> -	gpio_set_value(gpio, 0);
> +	gpiod_set_value(gpio, 0);
>  }

As the comment above deassert_reset() suggests, this change will
actually change the semantics of these calls by taking any gpio flags
into account (e.g. ACTIVE_LOW which will invert the signals).

Perhaps you should just use gpiod_set_raw_value() for now, and this can
be addressed later. Alternatively, drop the comment and invert the
polarity here and any users will need to update their device trees.

Either way, mention this in the commit message.

>  /*
> @@ -75,11 +74,11 @@ static int coldboot_seq(struct platform_device *pdev)
>  		return 0;
>  
>  	/* Hold APB in reset state */
> -	assert_reset(apb->resetn_gpio);
> +	assert_reset(apb->resetn);
>  
>  	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> -	    gpio_is_valid(apb->spi_en_gpio))
> -		devm_gpio_free(dev, apb->spi_en_gpio);
> +	    apb->spi_en)

No need to break the line any more.

> +		devm_gpiod_put(dev, apb->spi_en);
>  
>  	/* Enable power to APB */
>  	if (!IS_ERR(apb->vcore)) {
> @@ -101,13 +100,13 @@ static int coldboot_seq(struct platform_device *pdev)
>  	apb_bootret_deassert(dev);
>  
>  	/* On DB3 clock was not mandatory */
> -	if (gpio_is_valid(apb->clk_en_gpio))
> -		gpio_set_value(apb->clk_en_gpio, 1);
> +	if (apb->clk_en)
> +		gpiod_set_value(apb->clk_en, 1);
>  
>  	usleep_range(100, 200);
>  
>  	/* deassert reset to APB : Active-low signal */
> -	deassert_reset(apb->resetn_gpio);
> +	deassert_reset(apb->resetn);
>  
>  	apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
>  
> @@ -119,6 +118,7 @@ static int fw_flashing_seq(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct arche_apb_ctrl_drvdata *apb = platform_get_drvdata(pdev);
>  	int ret;
> +	unsigned long flags;
>  
>  	if (apb->init_disabled ||
>  	    apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING)
> @@ -136,25 +136,20 @@ static int fw_flashing_seq(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> -	if (gpio_is_valid(apb->spi_en_gpio)) {

spi_en_gpio is currently optional, so cannot just drop the check.

> -		unsigned long flags;
> +	if (apb->spi_en_polarity_high)
> +		flags = GPIOD_OUT_HIGH;
> +	else
> +		flags = GPIOD_OUT_LOW;

This should probably also be converted to honouring the gpio flags, but
perhaps better to do in a later patch.

>  
> -		if (apb->spi_en_polarity_high)
> -			flags = GPIOF_OUT_INIT_HIGH;
> -		else
> -			flags = GPIOF_OUT_INIT_LOW;
> -
> -		ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
> -					    flags, "apb_spi_en");
> -		if (ret) {
> -			dev_err(dev, "Failed requesting SPI bus en gpio %d\n",
> -				apb->spi_en_gpio);
> -			return ret;
> -		}
> +	apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);
> +	if (IS_ERR(apb->spi_en)) {
> +		ret = PTR_ERR(apb->spi_en);
> +		dev_err(dev, "Failed requesting SPI bus en GPIO: %d\n", ret);
> +		return ret;
>  	}
>  
>  	/* for flashing device should be in reset state */
> -	assert_reset(apb->resetn_gpio);
> +	assert_reset(apb->resetn);
>  	apb->state = ARCHE_PLATFORM_STATE_FW_FLASHING;
>  
>  	return 0;
> @@ -177,8 +172,8 @@ static int standby_boot_seq(struct platform_device *pdev)
>  		return 0;
>  
>  	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> -	    gpio_is_valid(apb->spi_en_gpio))
> -		devm_gpio_free(dev, apb->spi_en_gpio);
> +	    apb->spi_en)

No line break. Check this throughout.

> +		devm_gpiod_put(dev, apb->spi_en);
>  
>  	/*
>  	 * As per WDM spec, do nothing

> @@ -404,12 +379,8 @@ static int apb_ctrl_get_devtree_data(struct platform_device *pdev,
>  	}
>  
>  	/* Only applicable for platform >= V2 */
> -	apb->spi_en_gpio = of_get_named_gpio(np, "spi-en-gpio", 0);
> -	if (apb->spi_en_gpio >= 0) {
> -		if (of_property_read_bool(pdev->dev.of_node,
> -					  "spi-en-active-high"))
> -			apb->spi_en_polarity_high = true;
> -	}
> +	if (of_property_read_bool(pdev->dev.of_node, "gb,spi-en-active-high"))
> +		apb->spi_en_polarity_high = true;

Guess it's fine to drop the spi_en check here though.

Johan

Re: [PATCH v3 2/3] staging: greybus: arche-apb-ctrl.c: Switch to the gpio descriptor interface

[Nishad Kamdar]

On Tue, Dec 18, 2018 at 12:35:40PM +0100, Johan Hovold wrote:
> On Thu, Nov 22, 2018 at 10:38:18PM +0530, Nishad Kamdar wrote:
> > Use the gpiod interface instead of the deprecated old non-descriptor
> > interface.
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > ---
> > Changes in v2:
> >  - Resolved compilation errors.
> > ---
> >  drivers/staging/greybus/arche-apb-ctrl.c | 159 +++++++++--------------
> >  1 file changed, 65 insertions(+), 94 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/arche-apb-ctrl.c b/drivers/staging/greybus/arche-apb-ctrl.c
> > index be5ffed90bcf..e887f6aee20b 100644
> > --- a/drivers/staging/greybus/arche-apb-ctrl.c
> > +++ b/drivers/staging/greybus/arche-apb-ctrl.c
> > @@ -8,9 +8,8 @@
> >  
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/of_gpio.h>
> >  #include <linux/of_irq.h>
> >  #include <linux/module.h>
> >  #include <linux/pinctrl/consumer.h>
> > @@ -24,12 +23,12 @@ static void apb_bootret_deassert(struct device *dev);
> >  
> >  struct arche_apb_ctrl_drvdata {
> >  	/* Control GPIO signals to and from AP <=> AP Bridges */
> > -	int resetn_gpio;
> > -	int boot_ret_gpio;
> > -	int pwroff_gpio;
> > -	int wake_in_gpio;
> > -	int wake_out_gpio;
> > -	int pwrdn_gpio;
> > +	struct gpio_desc *resetn;
> > +	struct gpio_desc *boot_ret;
> > +	struct gpio_desc *pwroff;
> > +	struct gpio_desc *wake_in;
> > +	struct gpio_desc *wake_out;
> > +	struct gpio_desc *pwrdn;
> >  
> >  	enum arche_platform_state state;
> >  	bool init_disabled;
> > @@ -37,28 +36,28 @@ struct arche_apb_ctrl_drvdata {
> >  	struct regulator *vcore;
> >  	struct regulator *vio;
> >  
> > -	int clk_en_gpio;
> > +	struct gpio_desc *clk_en;
> >  	struct clk *clk;
> >  
> >  	struct pinctrl *pinctrl;
> >  	struct pinctrl_state *pin_default;
> >  
> >  	/* V2: SPI Bus control  */
> > -	int spi_en_gpio;
> > +	struct gpio_desc *spi_en;
> >  	bool spi_en_polarity_high;
> >  };
> >  
> >  /*
> >   * Note that these low level api's are active high
> >   */
> > -static inline void deassert_reset(unsigned int gpio)
> > +static inline void deassert_reset(struct gpio_desc *gpio)
> >  {
> > -	gpio_set_value(gpio, 1);
> > +	gpiod_set_value(gpio, 1);
> >  }
> >  
> > -static inline void assert_reset(unsigned int gpio)
> > +static inline void assert_reset(struct gpio_desc *gpio)
> >  {
> > -	gpio_set_value(gpio, 0);
> > +	gpiod_set_value(gpio, 0);
> >  }
> 
> As the comment above deassert_reset() suggests, this change will
> actually change the semantics of these calls by taking any gpio flags
> into account (e.g. ACTIVE_LOW which will invert the signals).
> 
> Perhaps you should just use gpiod_set_raw_value() for now, and this can
> be addressed later. Alternatively, drop the comment and invert the
> polarity here and any users will need to update their device trees.
> 
> Either way, mention this in the commit message.
> 
Ok, I'll use the gpiod_set_raw_value().

> >  /*
> > @@ -75,11 +74,11 @@ static int coldboot_seq(struct platform_device *pdev)
> >  		return 0;
> >  
> >  	/* Hold APB in reset state */
> > -	assert_reset(apb->resetn_gpio);
> > +	assert_reset(apb->resetn);
> >  
> >  	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> > -	    gpio_is_valid(apb->spi_en_gpio))
> > -		devm_gpio_free(dev, apb->spi_en_gpio);
> > +	    apb->spi_en)
> 
> No need to break the line any more.
> 
Ok.

> > +		devm_gpiod_put(dev, apb->spi_en);
> >  
> >  	/* Enable power to APB */
> >  	if (!IS_ERR(apb->vcore)) {
> > @@ -101,13 +100,13 @@ static int coldboot_seq(struct platform_device *pdev)
> >  	apb_bootret_deassert(dev);
> >  
> >  	/* On DB3 clock was not mandatory */
> > -	if (gpio_is_valid(apb->clk_en_gpio))
> > -		gpio_set_value(apb->clk_en_gpio, 1);
> > +	if (apb->clk_en)
> > +		gpiod_set_value(apb->clk_en, 1);
> >  
> >  	usleep_range(100, 200);
> >  
> >  	/* deassert reset to APB : Active-low signal */
> > -	deassert_reset(apb->resetn_gpio);
> > +	deassert_reset(apb->resetn);
> >  
> >  	apb->state = ARCHE_PLATFORM_STATE_ACTIVE;
> >  
> > @@ -119,6 +118,7 @@ static int fw_flashing_seq(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct arche_apb_ctrl_drvdata *apb = platform_get_drvdata(pdev);
> >  	int ret;
> > +	unsigned long flags;
> >  
> >  	if (apb->init_disabled ||
> >  	    apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING)
> > @@ -136,25 +136,20 @@ static int fw_flashing_seq(struct platform_device *pdev)
> >  		return ret;
> >  	}
> >  
> > -	if (gpio_is_valid(apb->spi_en_gpio)) {
> 
> spi_en_gpio is currently optional, so cannot just drop the check.
>
Ok, I'll restore it.

> > -		unsigned long flags;
> > +	if (apb->spi_en_polarity_high)
> > +		flags = GPIOD_OUT_HIGH;
> > +	else
> > +		flags = GPIOD_OUT_LOW;
> 
> This should probably also be converted to honouring the gpio flags, but
> perhaps better to do in a later patch.
> 
Ok.

> >  
> > -		if (apb->spi_en_polarity_high)
> > -			flags = GPIOF_OUT_INIT_HIGH;
> > -		else
> > -			flags = GPIOF_OUT_INIT_LOW;
> > -
> > -		ret = devm_gpio_request_one(dev, apb->spi_en_gpio,
> > -					    flags, "apb_spi_en");
> > -		if (ret) {
> > -			dev_err(dev, "Failed requesting SPI bus en gpio %d\n",
> > -				apb->spi_en_gpio);
> > -			return ret;
> > -		}
> > +	apb->spi_en = devm_gpiod_get(dev, "gb,spi-en-gpio", flags);
> > +	if (IS_ERR(apb->spi_en)) {
> > +		ret = PTR_ERR(apb->spi_en);
> > +		dev_err(dev, "Failed requesting SPI bus en GPIO: %d\n", ret);
> > +		return ret;
> >  	}
> >  
> >  	/* for flashing device should be in reset state */
> > -	assert_reset(apb->resetn_gpio);
> > +	assert_reset(apb->resetn);
> >  	apb->state = ARCHE_PLATFORM_STATE_FW_FLASHING;
> >  
> >  	return 0;
> > @@ -177,8 +172,8 @@ static int standby_boot_seq(struct platform_device *pdev)
> >  		return 0;
> >  
> >  	if (apb->state == ARCHE_PLATFORM_STATE_FW_FLASHING &&
> > -	    gpio_is_valid(apb->spi_en_gpio))
> > -		devm_gpio_free(dev, apb->spi_en_gpio);
> > +	    apb->spi_en)
> 
> No line break. Check this throughout.
>
Yes.

> > +		devm_gpiod_put(dev, apb->spi_en);
> >  
> >  	/*
> >  	 * As per WDM spec, do nothing
> 
> > @@ -404,12 +379,8 @@ static int apb_ctrl_get_devtree_data(struct platform_device *pdev,
> >  	}
> >  
> >  	/* Only applicable for platform >= V2 */
> > -	apb->spi_en_gpio = of_get_named_gpio(np, "spi-en-gpio", 0);
> > -	if (apb->spi_en_gpio >= 0) {
> > -		if (of_property_read_bool(pdev->dev.of_node,
> > -					  "spi-en-active-high"))
> > -			apb->spi_en_polarity_high = true;
> > -	}
> > +	if (of_property_read_bool(pdev->dev.of_node, "gb,spi-en-active-high"))
> > +		apb->spi_en_polarity_high = true;
> 
> Guess it's fine to drop the spi_en check here though.
>
Ok.
Thanks for the review.
regards,
Nishad

> Johan

[PATCH v3 3/3] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Nishad Kamdar]

Use the gpiod interface instead of the deprecated
old non-descriptor interface.

Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
---
Changes in v2:
 - Move comment to the same line as to what it applies to.
---
 drivers/staging/greybus/arche-platform.c | 119 ++++++++---------------
 1 file changed, 41 insertions(+), 78 deletions(-)

diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
index 4c36e88766e7..a5cea79d8e32 100644
--- a/drivers/staging/greybus/arche-platform.c
+++ b/drivers/staging/greybus/arche-platform.c
@@ -8,10 +8,9 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/init.h>
 #include <linux/module.h>
-#include <linux/of_gpio.h>
 #include <linux/of_platform.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
@@ -45,14 +44,14 @@ enum svc_wakedetect_state {
 
 struct arche_platform_drvdata {
 	/* Control GPIO signals to and from AP <=> SVC */
-	int svc_reset_gpio;
+	struct gpio_desc *svc_reset;
+	struct gpio_desc *svc_sysboot;
 	bool is_reset_act_hi;
-	int svc_sysboot_gpio;
-	int wake_detect_gpio; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
+	struct gpio_desc *wake_detect; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
 
 	enum arche_platform_state state;
 
-	int svc_refclk_req;
+	struct gpio_desc *svc_refclk_req;
 	struct clk *svc_ref_clk;
 
 	struct pinctrl *pinctrl;
@@ -85,9 +84,9 @@ static void arche_platform_set_wake_detect_state(
 	arche_pdata->wake_detect_state = state;
 }
 
-static inline void svc_reset_onoff(unsigned int gpio, bool onoff)
+static inline void svc_reset_onoff(struct gpio_desc *gpio, bool onoff)
 {
-	gpio_set_value(gpio, onoff);
+	gpiod_set_value(gpio, onoff);
 }
 
 static int apb_cold_boot(struct device *dev, void *data)
@@ -116,7 +115,6 @@ static int apb_poweroff(struct device *dev, void *data)
 static void arche_platform_wd_irq_en(struct arche_platform_drvdata *arche_pdata)
 {
 	/* Enable interrupt here, to read event back from SVC */
-	gpio_direction_input(arche_pdata->wake_detect_gpio);
 	enable_irq(arche_pdata->wake_detect_irq);
 }
 
@@ -160,7 +158,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
 
 	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
 
-	if (gpio_get_value(arche_pdata->wake_detect_gpio)) {
+	if (gpiod_get_value(arche_pdata->wake_detect)) {
 		/* wake/detect rising */
 
 		/*
@@ -224,10 +222,10 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
 
 	dev_info(arche_pdata->dev, "Booting from cold boot state\n");
 
-	svc_reset_onoff(arche_pdata->svc_reset_gpio,
+	svc_reset_onoff(arche_pdata->svc_reset,
 			arche_pdata->is_reset_act_hi);
 
-	gpio_set_value(arche_pdata->svc_sysboot_gpio, 0);
+	gpiod_set_value(arche_pdata->svc_sysboot, 0);
 	usleep_range(100, 200);
 
 	ret = clk_prepare_enable(arche_pdata->svc_ref_clk);
@@ -238,7 +236,7 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
 	}
 
 	/* bring SVC out of reset */
-	svc_reset_onoff(arche_pdata->svc_reset_gpio,
+	svc_reset_onoff(arche_pdata->svc_reset,
 			!arche_pdata->is_reset_act_hi);
 
 	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_ACTIVE);
@@ -259,10 +257,10 @@ arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
 
 	dev_info(arche_pdata->dev, "Switching to FW flashing state\n");
 
-	svc_reset_onoff(arche_pdata->svc_reset_gpio,
+	svc_reset_onoff(arche_pdata->svc_reset,
 			arche_pdata->is_reset_act_hi);
 
-	gpio_set_value(arche_pdata->svc_sysboot_gpio, 1);
+	gpiod_set_value(arche_pdata->svc_sysboot, 1);
 
 	usleep_range(100, 200);
 
@@ -273,7 +271,7 @@ arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
 		return ret;
 	}
 
-	svc_reset_onoff(arche_pdata->svc_reset_gpio,
+	svc_reset_onoff(arche_pdata->svc_reset,
 			!arche_pdata->is_reset_act_hi);
 
 	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_FW_FLASHING);
@@ -305,7 +303,7 @@ arche_platform_poweroff_seq(struct arche_platform_drvdata *arche_pdata)
 	clk_disable_unprepare(arche_pdata->svc_ref_clk);
 
 	/* As part of exit, put APB back in reset state */
-	svc_reset_onoff(arche_pdata->svc_reset_gpio,
+	svc_reset_onoff(arche_pdata->svc_reset,
 			arche_pdata->is_reset_act_hi);
 
 	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_OFF);
@@ -435,6 +433,7 @@ static int arche_platform_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
 	int ret;
+	unsigned int flags;
 
 	arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata),
 				   GFP_KERNEL);
@@ -444,61 +443,33 @@ static int arche_platform_probe(struct platform_device *pdev)
 	/* setup svc reset gpio */
 	arche_pdata->is_reset_act_hi = of_property_read_bool(np,
 							     "svc,reset-active-high");
-	arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
-							"svc,reset-gpio",
-							0);
-	if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
-		dev_err(dev, "failed to get reset-gpio\n");
-		return arche_pdata->svc_reset_gpio;
-	}
-	ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
-	if (ret) {
-		dev_err(dev, "failed to request svc-reset gpio:%d\n", ret);
-		return ret;
-	}
-	ret = gpio_direction_output(arche_pdata->svc_reset_gpio,
-				    arche_pdata->is_reset_act_hi);
-	if (ret) {
-		dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
+	if (arche_pdata->is_reset_act_hi)
+		flags = GPIOD_OUT_HIGH;
+	else
+		flags = GPIOD_OUT_LOW;
+
+	arche_pdata->svc_reset = devm_gpiod_get(dev, "svc,reset-gpio", flags);
+	if (IS_ERR(arche_pdata->svc_reset)) {
+		ret = PTR_ERR(arche_pdata->svc_reset);
+		dev_err(dev, "failed to request svc-reset GPIO:%d\n", ret);
 		return ret;
 	}
 	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_OFF);
 
-	arche_pdata->svc_sysboot_gpio = of_get_named_gpio(np,
-							  "svc,sysboot-gpio",
-							  0);
-	if (!gpio_is_valid(arche_pdata->svc_sysboot_gpio)) {
-		dev_err(dev, "failed to get sysboot gpio\n");
-		return arche_pdata->svc_sysboot_gpio;
-	}
-	ret = devm_gpio_request(dev, arche_pdata->svc_sysboot_gpio, "sysboot0");
-	if (ret) {
-		dev_err(dev, "failed to request sysboot0 gpio:%d\n", ret);
-		return ret;
-	}
-	ret = gpio_direction_output(arche_pdata->svc_sysboot_gpio, 0);
-	if (ret) {
-		dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
+	arche_pdata->svc_sysboot = devm_gpiod_get(dev, "svc,sysboot-gpio",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(arche_pdata->svc_sysboot)) {
+		ret = PTR_ERR(arche_pdata->svc_sysboot);
+		dev_err(dev, "failed to request sysboot0 GPIO: %d\n", ret);
 		return ret;
 	}
 
 	/* setup the clock request gpio first */
-	arche_pdata->svc_refclk_req = of_get_named_gpio(np,
-							"svc,refclk-req-gpio",
-							0);
-	if (!gpio_is_valid(arche_pdata->svc_refclk_req)) {
-		dev_err(dev, "failed to get svc clock-req gpio\n");
-		return arche_pdata->svc_refclk_req;
-	}
-	ret = devm_gpio_request(dev, arche_pdata->svc_refclk_req,
-				"svc-clk-req");
-	if (ret) {
-		dev_err(dev, "failed to request svc-clk-req gpio: %d\n", ret);
-		return ret;
-	}
-	ret = gpio_direction_input(arche_pdata->svc_refclk_req);
-	if (ret) {
-		dev_err(dev, "failed to set svc-clk-req gpio dir :%d\n", ret);
+	arche_pdata->svc_refclk_req = devm_gpiod_get(dev, "svc,refclk-req-gpio",
+						     GPIOD_IN);
+	if (IS_ERR(arche_pdata->svc_refclk_req)) {
+		ret = PTR_ERR(arche_pdata->svc_refclk_req);
+		dev_err(dev, "failed to request svc-clk-req GPIO: %d\n", ret);
 		return ret;
 	}
 
@@ -515,19 +486,11 @@ static int arche_platform_probe(struct platform_device *pdev)
 	arche_pdata->num_apbs = of_get_child_count(np);
 	dev_dbg(dev, "Number of APB's available - %d\n", arche_pdata->num_apbs);
 
-	arche_pdata->wake_detect_gpio = of_get_named_gpio(np,
-							  "svc,wake-detect-gpio",
-							  0);
-	if (arche_pdata->wake_detect_gpio < 0) {
-		dev_err(dev, "failed to get wake detect gpio\n");
-		return arche_pdata->wake_detect_gpio;
-	}
-
-	ret = devm_gpio_request(dev, arche_pdata->wake_detect_gpio,
-				"wake detect");
-	if (ret) {
-		dev_err(dev, "Failed requesting wake_detect gpio %d\n",
-			arche_pdata->wake_detect_gpio);
+	arche_pdata->wake_detect = devm_gpiod_get(dev, "svc,wake-detect-gpio",
+						  GPIOD_IN);
+	if (IS_ERR(arche_pdata->wake_detect)) {
+		ret = PTR_ERR(arche_pdata->wake_detect);
+		dev_err(dev, "Failed requesting wake_detect GPIO %d\n", ret);
 		return ret;
 	}
 
@@ -538,7 +501,7 @@ static int arche_platform_probe(struct platform_device *pdev)
 	spin_lock_init(&arche_pdata->wake_lock);
 	mutex_init(&arche_pdata->platform_state_mutex);
 	arche_pdata->wake_detect_irq =
-		gpio_to_irq(arche_pdata->wake_detect_gpio);
+		gpiod_to_irq(arche_pdata->wake_detect);
 
 	ret = devm_request_threaded_irq(dev, arche_pdata->wake_detect_irq,
 					arche_platform_wd_irq,
-- 
2.17.1

Re: [PATCH v3 3/3] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Johan Hovold]

On Thu, Nov 22, 2018 at 10:39:24PM +0530, Nishad Kamdar wrote:
> Use the gpiod interface instead of the deprecated
> old non-descriptor interface.
> 
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> ---
> Changes in v2:
>  - Move comment to the same line as to what it applies to.
> ---
>  drivers/staging/greybus/arche-platform.c | 119 ++++++++---------------
>  1 file changed, 41 insertions(+), 78 deletions(-)
> 
> diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> index 4c36e88766e7..a5cea79d8e32 100644
> --- a/drivers/staging/greybus/arche-platform.c
> +++ b/drivers/staging/greybus/arche-platform.c
> @@ -8,10 +8,9 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/init.h>
>  #include <linux/module.h>
> -#include <linux/of_gpio.h>
>  #include <linux/of_platform.h>
>  #include <linux/pinctrl/consumer.h>
>  #include <linux/platform_device.h>
> @@ -45,14 +44,14 @@ enum svc_wakedetect_state {
>  
>  struct arche_platform_drvdata {
>  	/* Control GPIO signals to and from AP <=> SVC */
> -	int svc_reset_gpio;
> +	struct gpio_desc *svc_reset;
> +	struct gpio_desc *svc_sysboot;

Why move sysboot? The flag below is about reset (but should eventually
go away).

>  	bool is_reset_act_hi;
> -	int svc_sysboot_gpio;
> -	int wake_detect_gpio; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
> +	struct gpio_desc *wake_detect; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
>  
>  	enum arche_platform_state state;
>  
> -	int svc_refclk_req;
> +	struct gpio_desc *svc_refclk_req;
>  	struct clk *svc_ref_clk;
>  
>  	struct pinctrl *pinctrl;
> @@ -85,9 +84,9 @@ static void arche_platform_set_wake_detect_state(
>  	arche_pdata->wake_detect_state = state;
>  }
>  
> -static inline void svc_reset_onoff(unsigned int gpio, bool onoff)
> +static inline void svc_reset_onoff(struct gpio_desc *gpio, bool onoff)
>  {
> -	gpio_set_value(gpio, onoff);
> +	gpiod_set_value(gpio, onoff);
>  }
>  
>  static int apb_cold_boot(struct device *dev, void *data)
> @@ -116,7 +115,6 @@ static int apb_poweroff(struct device *dev, void *data)
>  static void arche_platform_wd_irq_en(struct arche_platform_drvdata *arche_pdata)
>  {
>  	/* Enable interrupt here, to read event back from SVC */
> -	gpio_direction_input(arche_pdata->wake_detect_gpio);
>  	enable_irq(arche_pdata->wake_detect_irq);
>  }
>  
> @@ -160,7 +158,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
>  
>  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
>  
> -	if (gpio_get_value(arche_pdata->wake_detect_gpio)) {
> +	if (gpiod_get_value(arche_pdata->wake_detect)) {
>  		/* wake/detect rising */
>  
>  		/*
> @@ -224,10 +222,10 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
>  
>  	dev_info(arche_pdata->dev, "Booting from cold boot state\n");
>  
> -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> +	svc_reset_onoff(arche_pdata->svc_reset,

No need to break line here.

>  			arche_pdata->is_reset_act_hi);
>  
> -	gpio_set_value(arche_pdata->svc_sysboot_gpio, 0);
> +	gpiod_set_value(arche_pdata->svc_sysboot, 0);
>  	usleep_range(100, 200);
>  
>  	ret = clk_prepare_enable(arche_pdata->svc_ref_clk);
> @@ -238,7 +236,7 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
>  	}
>  
>  	/* bring SVC out of reset */
> -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> +	svc_reset_onoff(arche_pdata->svc_reset,

Same here. Please check throughout.

>  			!arche_pdata->is_reset_act_hi);
>  
>  	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_ACTIVE);
> @@ -259,10 +257,10 @@ arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
>  
>  	dev_info(arche_pdata->dev, "Switching to FW flashing state\n");
>  
> -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> +	svc_reset_onoff(arche_pdata->svc_reset,
>  			arche_pdata->is_reset_act_hi);
>  
> -	gpio_set_value(arche_pdata->svc_sysboot_gpio, 1);
> +	gpiod_set_value(arche_pdata->svc_sysboot, 1);
>  
>  	usleep_range(100, 200);
>  
> @@ -273,7 +271,7 @@ arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
>  		return ret;
>  	}
>  
> -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> +	svc_reset_onoff(arche_pdata->svc_reset,
>  			!arche_pdata->is_reset_act_hi);
>  
>  	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_FW_FLASHING);
> @@ -305,7 +303,7 @@ arche_platform_poweroff_seq(struct arche_platform_drvdata *arche_pdata)
>  	clk_disable_unprepare(arche_pdata->svc_ref_clk);
>  
>  	/* As part of exit, put APB back in reset state */
> -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> +	svc_reset_onoff(arche_pdata->svc_reset,
>  			arche_pdata->is_reset_act_hi);
>  
>  	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_OFF);
> @@ -435,6 +433,7 @@ static int arche_platform_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	struct device_node *np = dev->of_node;
>  	int ret;
> +	unsigned int flags;
>  
>  	arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata),
>  				   GFP_KERNEL);
> @@ -444,61 +443,33 @@ static int arche_platform_probe(struct platform_device *pdev)
>  	/* setup svc reset gpio */
>  	arche_pdata->is_reset_act_hi = of_property_read_bool(np,
>  							     "svc,reset-active-high");

This should also go away eventually in favour of gpio flags in
devicetree. Fine to keep for now, though.

> -	arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> -							"svc,reset-gpio",
> -							0);
> -	if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> -		dev_err(dev, "failed to get reset-gpio\n");
> -		return arche_pdata->svc_reset_gpio;
> -	}
> -	ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> -	if (ret) {
> -		dev_err(dev, "failed to request svc-reset gpio:%d\n", ret);
> -		return ret;
> -	}
> -	ret = gpio_direction_output(arche_pdata->svc_reset_gpio,
> -				    arche_pdata->is_reset_act_hi);
> -	if (ret) {
> -		dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
> +	if (arche_pdata->is_reset_act_hi)
> +		flags = GPIOD_OUT_HIGH;
> +	else
> +		flags = GPIOD_OUT_LOW;
> +
> +	arche_pdata->svc_reset = devm_gpiod_get(dev, "svc,reset-gpio", flags);
> +	if (IS_ERR(arche_pdata->svc_reset)) {
> +		ret = PTR_ERR(arche_pdata->svc_reset);
> +		dev_err(dev, "failed to request svc-reset GPIO:%d\n", ret);
>  		return ret;
>  	}

Thanks,
Johan

Re: [PATCH v3 3/3] staging: greybus: arche-platform: Switch to the gpio descriptor interface

[Nishad Kamdar]

On Tue, Dec 18, 2018 at 12:50:56PM +0100, Johan Hovold wrote:
> On Thu, Nov 22, 2018 at 10:39:24PM +0530, Nishad Kamdar wrote:
> > Use the gpiod interface instead of the deprecated
> > old non-descriptor interface.
> > 
> > Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
> > ---
> > Changes in v2:
> >  - Move comment to the same line as to what it applies to.
> > ---
> >  drivers/staging/greybus/arche-platform.c | 119 ++++++++---------------
> >  1 file changed, 41 insertions(+), 78 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/arche-platform.c b/drivers/staging/greybus/arche-platform.c
> > index 4c36e88766e7..a5cea79d8e32 100644
> > --- a/drivers/staging/greybus/arche-platform.c
> > +++ b/drivers/staging/greybus/arche-platform.c
> > @@ -8,10 +8,9 @@
> >  
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > -#include <linux/gpio.h>
> > +#include <linux/gpio/consumer.h>
> >  #include <linux/init.h>
> >  #include <linux/module.h>
> > -#include <linux/of_gpio.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/pinctrl/consumer.h>
> >  #include <linux/platform_device.h>
> > @@ -45,14 +44,14 @@ enum svc_wakedetect_state {
> >  
> >  struct arche_platform_drvdata {
> >  	/* Control GPIO signals to and from AP <=> SVC */
> > -	int svc_reset_gpio;
> > +	struct gpio_desc *svc_reset;
> > +	struct gpio_desc *svc_sysboot;
> 
> Why move sysboot? The flag below is about reset (but should eventually
> go away).
> 
Ok, I'll restore its position.

> >  	bool is_reset_act_hi;
> > -	int svc_sysboot_gpio;
> > -	int wake_detect_gpio; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
> > +	struct gpio_desc *wake_detect; /* bi-dir,maps to WAKE_MOD & WAKE_FRAME signals */
> >  
> >  	enum arche_platform_state state;
> >  
> > -	int svc_refclk_req;
> > +	struct gpio_desc *svc_refclk_req;
> >  	struct clk *svc_ref_clk;
> >  
> >  	struct pinctrl *pinctrl;
> > @@ -85,9 +84,9 @@ static void arche_platform_set_wake_detect_state(
> >  	arche_pdata->wake_detect_state = state;
> >  }
> >  
> > -static inline void svc_reset_onoff(unsigned int gpio, bool onoff)
> > +static inline void svc_reset_onoff(struct gpio_desc *gpio, bool onoff)
> >  {
> > -	gpio_set_value(gpio, onoff);
> > +	gpiod_set_value(gpio, onoff);
> >  }
> >  
> >  static int apb_cold_boot(struct device *dev, void *data)
> > @@ -116,7 +115,6 @@ static int apb_poweroff(struct device *dev, void *data)
> >  static void arche_platform_wd_irq_en(struct arche_platform_drvdata *arche_pdata)
> >  {
> >  	/* Enable interrupt here, to read event back from SVC */
> > -	gpio_direction_input(arche_pdata->wake_detect_gpio);
> >  	enable_irq(arche_pdata->wake_detect_irq);
> >  }
> >  
> > @@ -160,7 +158,7 @@ static irqreturn_t arche_platform_wd_irq(int irq, void *devid)
> >  
> >  	spin_lock_irqsave(&arche_pdata->wake_lock, flags);
> >  
> > -	if (gpio_get_value(arche_pdata->wake_detect_gpio)) {
> > +	if (gpiod_get_value(arche_pdata->wake_detect)) {
> >  		/* wake/detect rising */
> >  
> >  		/*
> > @@ -224,10 +222,10 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
> >  
> >  	dev_info(arche_pdata->dev, "Booting from cold boot state\n");
> >  
> > -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> > +	svc_reset_onoff(arche_pdata->svc_reset,
> 
> No need to break line here.
> 
Ok.

> >  			arche_pdata->is_reset_act_hi);
> >  
> > -	gpio_set_value(arche_pdata->svc_sysboot_gpio, 0);
> > +	gpiod_set_value(arche_pdata->svc_sysboot, 0);
> >  	usleep_range(100, 200);
> >  
> >  	ret = clk_prepare_enable(arche_pdata->svc_ref_clk);
> > @@ -238,7 +236,7 @@ arche_platform_coldboot_seq(struct arche_platform_drvdata *arche_pdata)
> >  	}
> >  
> >  	/* bring SVC out of reset */
> > -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> > +	svc_reset_onoff(arche_pdata->svc_reset,
> 
> Same here. Please check throughout.
> 
Yes.

> >  			!arche_pdata->is_reset_act_hi);
> >  
> >  	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_ACTIVE);
> > @@ -259,10 +257,10 @@ arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
> >  
> >  	dev_info(arche_pdata->dev, "Switching to FW flashing state\n");
> >  
> > -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> > +	svc_reset_onoff(arche_pdata->svc_reset,
> >  			arche_pdata->is_reset_act_hi);
> >  
> > -	gpio_set_value(arche_pdata->svc_sysboot_gpio, 1);
> > +	gpiod_set_value(arche_pdata->svc_sysboot, 1);
> >  
> >  	usleep_range(100, 200);
> >  
> > @@ -273,7 +271,7 @@ arche_platform_fw_flashing_seq(struct arche_platform_drvdata *arche_pdata)
> >  		return ret;
> >  	}
> >  
> > -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> > +	svc_reset_onoff(arche_pdata->svc_reset,
> >  			!arche_pdata->is_reset_act_hi);
> >  
> >  	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_FW_FLASHING);
> > @@ -305,7 +303,7 @@ arche_platform_poweroff_seq(struct arche_platform_drvdata *arche_pdata)
> >  	clk_disable_unprepare(arche_pdata->svc_ref_clk);
> >  
> >  	/* As part of exit, put APB back in reset state */
> > -	svc_reset_onoff(arche_pdata->svc_reset_gpio,
> > +	svc_reset_onoff(arche_pdata->svc_reset,
> >  			arche_pdata->is_reset_act_hi);
> >  
> >  	arche_platform_set_state(arche_pdata, ARCHE_PLATFORM_STATE_OFF);
> > @@ -435,6 +433,7 @@ static int arche_platform_probe(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	struct device_node *np = dev->of_node;
> >  	int ret;
> > +	unsigned int flags;
> >  
> >  	arche_pdata = devm_kzalloc(&pdev->dev, sizeof(*arche_pdata),
> >  				   GFP_KERNEL);
> > @@ -444,61 +443,33 @@ static int arche_platform_probe(struct platform_device *pdev)
> >  	/* setup svc reset gpio */
> >  	arche_pdata->is_reset_act_hi = of_property_read_bool(np,
> >  							     "svc,reset-active-high");
> 
> This should also go away eventually in favour of gpio flags in
> devicetree. Fine to keep for now, though.
>
Ok.

> > -	arche_pdata->svc_reset_gpio = of_get_named_gpio(np,
> > -							"svc,reset-gpio",
> > -							0);
> > -	if (!gpio_is_valid(arche_pdata->svc_reset_gpio)) {
> > -		dev_err(dev, "failed to get reset-gpio\n");
> > -		return arche_pdata->svc_reset_gpio;
> > -	}
> > -	ret = devm_gpio_request(dev, arche_pdata->svc_reset_gpio, "svc-reset");
> > -	if (ret) {
> > -		dev_err(dev, "failed to request svc-reset gpio:%d\n", ret);
> > -		return ret;
> > -	}
> > -	ret = gpio_direction_output(arche_pdata->svc_reset_gpio,
> > -				    arche_pdata->is_reset_act_hi);
> > -	if (ret) {
> > -		dev_err(dev, "failed to set svc-reset gpio dir:%d\n", ret);
> > +	if (arche_pdata->is_reset_act_hi)
> > +		flags = GPIOD_OUT_HIGH;
> > +	else
> > +		flags = GPIOD_OUT_LOW;
> > +
> > +	arche_pdata->svc_reset = devm_gpiod_get(dev, "svc,reset-gpio", flags);
> > +	if (IS_ERR(arche_pdata->svc_reset)) {
> > +		ret = PTR_ERR(arche_pdata->svc_reset);
> > +		dev_err(dev, "failed to request svc-reset GPIO:%d\n", ret);
> >  		return ret;
> >  	}
> 
> Thanks,
> Johan

Thanks for the review.

Regards,
Nishad