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

Wu, Wentong
2023-09-29T11:33:57Z

> 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@intel.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> > 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