Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 18 Dec 2018 12:10:34 +0100
From:   Johan Hovold <johan@kernel.org>
To:     Nishad Kamdar <nishadkamdar@gmail.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Johan Hovold <johan@kernel.org>, Alex Elder <elder@kernel.org>,
        Rui Miguel Silva <rmfrfs@gmail.com>,
        greybus-dev@lists.linaro.org, devel@driverdev.osuosl.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 1/3] staging: greybus: gpio: switch GPIO portions to
 use GPIOLIB_IRQCHIP
Message-ID: <20181218111034.GM20658@localhost>
References: <cover.1542898267.git.nishadkamdar@gmail.com>
 <e12d2f21115c71820d4d68168181e5ebe4340f0f.1542898267.git.nishadkamdar@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e12d2f21115c71820d4d68168181e5ebe4340f0f.1542898267.git.nishadkamdar@gmail.com>
User-Agent: Mutt/1.11.1 (2018-12-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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