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

Greg KH
2023-09-28T14:17:19Z

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?

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

>
> #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