Re: [PATCH v4 2/5] gpio: Add support for Intel LJCA USB GPIO driver

[Krzysztof Kozlowski]

On 09/03/2023 08:10, Ye Xiang wrote:
> This patch implements the GPIO function of Intel USB-I2C/GPIO/SPI adapter
> device named "La Jolla Cove Adapter" (LJCA). It communicate with LJCA
> GPIO module with specific protocol through interfaces exported by LJCA USB
> driver.
> 
> Signed-off-by: Ye Xiang <xiang.ye@xxxxxxxxx>
> Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> ---
>  drivers/gpio/Kconfig     |  12 ++
>  drivers/gpio/Makefile    |   1 +
>  drivers/gpio/gpio-ljca.c | 454 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 467 insertions(+)
>  create mode 100644 drivers/gpio/gpio-ljca.c
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 13be729710f2..8be697f9f621 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1253,6 +1253,18 @@ config GPIO_KEMPLD
>  	  This driver can also be built as a module. If so, the module will be
>  	  called gpio-kempld.
>  
> +config GPIO_LJCA
> +	tristate "INTEL La Jolla Cove Adapter GPIO support"
> +	depends on MFD_LJCA
> +	select GPIOLIB_IRQCHIP
> +	default MFD_LJCA
> +	help
> +	  Select this option to enable GPIO driver for the INTEL
> +	  La Jolla Cove Adapter (LJCA) board.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called gpio-ljca.
> +
>  config GPIO_LP3943
>  	tristate "TI/National Semiconductor LP3943 GPIO expander"
>  	depends on MFD_LP3943
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index c048ba003367..eb59524d18c0 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_GPIO_IXP4XX)		+= gpio-ixp4xx.o
>  obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
>  obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
>  obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
> +obj-$(CONFIG_GPIO_LJCA) 		+= gpio-ljca.o
>  obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
>  obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
>  obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
> diff --git a/drivers/gpio/gpio-ljca.c b/drivers/gpio/gpio-ljca.c
> new file mode 100644
> index 000000000000..87863f0230f5
> --- /dev/null
> +++ b/drivers/gpio/gpio-ljca.c
> @@ -0,0 +1,454 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Intel La Jolla Cove Adapter USB-GPIO driver
> + *
> + * Copyright (c) 2023, Intel Corporation.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/dev_printk.h>
> +#include <linux/gpio/driver.h>
> +#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>
> +
> +/* 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_CONF_DISABLE		BIT(0)
> +#define LJCA_GPIO_CONF_INPUT		BIT(1)
> +#define LJCA_GPIO_CONF_OUTPUT		BIT(2)
> +#define LJCA_GPIO_CONF_PULLUP		BIT(3)
> +#define LJCA_GPIO_CONF_PULLDOWN		BIT(4)
> +#define LJCA_GPIO_CONF_DEFAULT		BIT(5)
> +#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)
> +
> +/* Intentional overlap with PULLUP / PULLDOWN */
> +#define LJCA_GPIO_CONF_SET	BIT(3)
> +#define LJCA_GPIO_CONF_CLR	BIT(4)
> +
> +struct gpio_op {
> +	u8 index;
> +	u8 value;
> +} __packed;
> +
> +struct gpio_packet {
> +	u8 num;
> +	struct gpio_op item[];
> +} __packed;
> +
> +#define LJCA_GPIO_BUF_SIZE 60
> +struct ljca_gpio_dev {
> +	struct platform_device *pdev;
> +	struct gpio_chip gc;
> +	struct ljca_gpio_info *gpio_info;
> +	DECLARE_BITMAP(unmasked_irqs, LJCA_MAX_GPIO_NUM);
> +	DECLARE_BITMAP(enabled_irqs, LJCA_MAX_GPIO_NUM);
> +	DECLARE_BITMAP(reenable_irqs, LJCA_MAX_GPIO_NUM);
> +	u8 *connect_mode;
> +	/* mutex to protect irq bus */
> +	struct mutex irq_lock;
> +	struct work_struct work;
> +	/* lock to protect package transfer to Hardware */
> +	struct mutex trans_lock;
> +
> +	u8 obuf[LJCA_GPIO_BUF_SIZE];
> +	u8 ibuf[LJCA_GPIO_BUF_SIZE];
> +};
> +
> +static int gpio_config(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id, u8 config)
> +{
> +	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
> +	int ret;
> +
> +	mutex_lock(&ljca_gpio->trans_lock);
> +	packet->item[0].index = gpio_id;
> +	packet->item[0].value = config | ljca_gpio->connect_mode[gpio_id];
> +	packet->num = 1;
> +
> +	ret = ljca_transfer(ljca_gpio->gpio_info->ljca, LJCA_GPIO_CONFIG, packet,
> +			    struct_size(packet, item, packet->num), NULL, NULL);
> +	mutex_unlock(&ljca_gpio->trans_lock);
> +	return ret;
> +}
> +
> +static int ljca_gpio_read(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id)
> +{
> +	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
> +	struct gpio_packet *ack_packet = (struct gpio_packet *)ljca_gpio->ibuf;
> +	unsigned int ibuf_len = LJCA_GPIO_BUF_SIZE;
> +	int ret;
> +
> +	mutex_lock(&ljca_gpio->trans_lock);
> +	packet->num = 1;
> +	packet->item[0].index = gpio_id;
> +	ret = ljca_transfer(ljca_gpio->gpio_info->ljca, LJCA_GPIO_READ, packet,
> +			    struct_size(packet, item, packet->num), ljca_gpio->ibuf, &ibuf_len);
> +	if (ret)
> +		goto out_unlock;
> +
> +	if (!ibuf_len || ack_packet->num != packet->num) {
> +		dev_err(&ljca_gpio->pdev->dev, "failed gpio_id:%u %u", gpio_id, ack_packet->num);
> +		ret = -EIO;
> +	}
> +
> +out_unlock:
> +	mutex_unlock(&ljca_gpio->trans_lock);
> +	if (ret)
> +		return ret;
> +	return ack_packet->item[0].value > 0;
> +}
> +
> +static int ljca_gpio_write(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id,
> +			   int value)
> +{
> +	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
> +	int ret;
> +
> +	mutex_lock(&ljca_gpio->trans_lock);
> +	packet->num = 1;
> +	packet->item[0].index = gpio_id;
> +	packet->item[0].value = value & 1;
> +
> +	ret = ljca_transfer(ljca_gpio->gpio_info->ljca, LJCA_GPIO_WRITE, packet,
> +			    struct_size(packet, item, packet->num), NULL, NULL);
> +	mutex_unlock(&ljca_gpio->trans_lock);

Everywhere you have unusual coding pattern around return. There is
almost always line break before return. Adjust to the kernel style.

> +	return ret;
> +}

(...)

> +
> +#define LJCA_GPIO_DRV_NAME "ljca-gpio"
> +static const struct platform_device_id ljca_gpio_id[] = {
> +	{ LJCA_GPIO_DRV_NAME, 0 },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(platform, ljca_gpio_id);

ljca_gpio_id is unused (except module autoloading). How do you bind your
devices?

> +
> +static struct platform_driver ljca_gpio_driver = {
> +	.driver.name = LJCA_GPIO_DRV_NAME,
> +	.probe = ljca_gpio_probe,
> +	.remove = ljca_gpio_remove,



Best regards,
Krzysztof