RE: [PATCH v19 4/4] gpio: update Intel LJCA USB GPIO driver

["Wu, Wentong" <wentong.wu@xxxxxxxxx>]

> From: Greg KH
> On Sun, Sep 17, 2023 at 02:53:36AM +0800, Wentong Wu wrote:
> > This driver communicate with LJCA GPIO module with specific protocol
> > through interfaces exported by LJCA USB driver.
> > Update the driver according to LJCA USB driver's changes.
> >
> > Signed-off-by: Wentong Wu <wentong.wu@xxxxxxxxx>
> > Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > Acked-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > Acked-by: Bartosz Golaszewski <bartosz.golaszewski@xxxxxxxxxx>
> > Tested-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > ---
> >  drivers/gpio/Kconfig     |   4 +-
> >  drivers/gpio/gpio-ljca.c | 246
> > +++++++++++++++++++++++++++--------------------
> >  2 files changed, 145 insertions(+), 105 deletions(-)
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
> > 673bafb..8d5b6c3 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1312,9 +1312,9 @@ config GPIO_KEMPLD
> >
> >  config GPIO_LJCA
> >  	tristate "INTEL La Jolla Cove Adapter GPIO support"
> > -	depends on MFD_LJCA
> > +	depends on USB_LJCA
> >  	select GPIOLIB_IRQCHIP
> > -	default MFD_LJCA
> > +	default USB_LJCA
> >  	help
> >  	  Select this option to enable GPIO driver for the INTEL
> >  	  La Jolla Cove Adapter (LJCA) board.
> > diff --git a/drivers/gpio/gpio-ljca.c b/drivers/gpio/gpio-ljca.c index
> > 87863f0..7fae26d 100644
> > --- a/drivers/gpio/gpio-ljca.c
> > +++ b/drivers/gpio/gpio-ljca.c
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <linux/acpi.h>
> > +#include <linux/auxiliary_bus.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/bitops.h>
> >  #include <linux/dev_printk.h>
> > @@ -13,19 +14,18 @@
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> >  #include <linux/kref.h>
> > -#include <linux/mfd/ljca.h>
> >  #include <linux/module.h>
> > -#include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <linux/usb/ljca.h>
> >
> >  /* GPIO commands */
> > -#define LJCA_GPIO_CONFIG	1
> > -#define LJCA_GPIO_READ		2
> > -#define LJCA_GPIO_WRITE		3
> > -#define LJCA_GPIO_INT_EVENT	4
> > -#define LJCA_GPIO_INT_MASK	5
> > -#define LJCA_GPIO_INT_UNMASK	6
> > +#define LJCA_GPIO_CONFIG		1
> > +#define LJCA_GPIO_READ			2
> > +#define LJCA_GPIO_WRITE			3
> > +#define LJCA_GPIO_INT_EVENT		4
> > +#define LJCA_GPIO_INT_MASK		5
> > +#define LJCA_GPIO_INT_UNMASK		6
> 
> Why are you changing whitespace for no good reason?

This is to make all the macro value in the same column to
make them look better.

> 
> Please don't do that, it makes finding the actual changes in this driver impossible
> to notice and review.

Understand, I'll follow this going forward. Thanks

Thanks
Wentong
> 
> 
> 
> >
> >  #define LJCA_GPIO_CONF_DISABLE		BIT(0)
> >  #define LJCA_GPIO_CONF_INPUT		BIT(1)
> > @@ -36,45 +36,49 @@
> >  #define LJCA_GPIO_CONF_INTERRUPT	BIT(6)
> >  #define LJCA_GPIO_INT_TYPE		BIT(7)
> >
> > -#define LJCA_GPIO_CONF_EDGE	FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
> > -#define LJCA_GPIO_CONF_LEVEL	FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
> > +#define LJCA_GPIO_CONF_EDGE
> 	FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
> > +#define LJCA_GPIO_CONF_LEVEL
> 	FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
> >
> >  /* Intentional overlap with PULLUP / PULLDOWN */
> > -#define LJCA_GPIO_CONF_SET	BIT(3)
> > -#define LJCA_GPIO_CONF_CLR	BIT(4)
> > +#define LJCA_GPIO_CONF_SET		BIT(3)
> > +#define LJCA_GPIO_CONF_CLR		BIT(4)
> >
> > -struct gpio_op {
> > +#define LJCA_GPIO_BUF_SIZE		60u
> 
> Why "u"?  What requires that?
> 
> Odd, sorry, I know people are just getting tired of the constant churn here, but
> really, you know better than making changes that are not needed, or not
> documented.
> 
> greg k-h