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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxx
> 
> 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