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

["Ye, Xiang" <xiang.ye@xxxxxxxxx>]

Hi Krzysztof,

Thanks for your review.
On Sat, Mar 11, 2023 at 01:13:30PM +0100, Krzysztof Kozlowski wrote:
> 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.
Got it. Will add a blank line before return.
> 
> > +	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?
driver.name = LJCA_GPIO_DRV_NAME, this driver name is used to bind
with devices. 
> > +
> > +static struct platform_driver ljca_gpio_driver = {
> > +	.driver.name = LJCA_GPIO_DRV_NAME,
> > +	.probe = ljca_gpio_probe,
> > +	.remove = ljca_gpio_remove,
> 
> 
--
Thanks
Ye Xiang