| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZA3yUAHgZ2ATqaLd@ye-NUC7i7DNHE>
Date: Sun, 12 Mar 2023 23:40:00 +0800
From: "Ye, Xiang" <xiang.ye@...el.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
CC: Ye Xiang <xiang.ye@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Arnd Bergmann <arnd@...db.de>,
"Matthias Kaehlcke" <mka@...omium.org>, Lee Jones <lee@...nel.org>,
Wolfram Sang <wsa@...nel.org>,
Tyrone Ting <kfting@...oton.com>,
Mark Brown <broonie@...nel.org>,
Linus Walleij <linus.walleij@...aro.org>,
"Bartosz Golaszewski" <brgl@...ev.pl>, <linux-usb@...r.kernel.org>,
<linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <linux-gpio@...r.kernel.org>,
<srinivas.pandruvada@...el.com>, <heikki.krogerus@...ux.intel.com>,
<andriy.shevchenko@...ux.intel.com>,
<sakari.ailus@...ux.intel.com>, <zhifeng.wang@...el.com>,
<wentong.wu@...el.com>, <lixu.zhang@...el.com>
Subject: Re: [PATCH v4 2/5] gpio: Add support for Intel LJCA USB GPIO driver
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@...el.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
> > Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> > ---
> > 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
Powered by blists - more mailing lists