Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230325154711.2419569-1-xiang.ye@intel.com> <20230325154711.2419569-5-xiang.ye@intel.com>
In-Reply-To: <20230325154711.2419569-5-xiang.ye@intel.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Wed, 29 Mar 2023 10:37:05 +0200
Message-ID: <CACRpkdbDmPpKFVxmn1ntKJWr5_mraKuafuTgJtgBU+39PxEYgg@mail.gmail.com>
Subject: Re: [PATCH v7 4/6] gpio: Add support for Intel LJCA USB GPIO driver
To:     Ye Xiang <xiang.ye@intel.com>, Marc Zyngier <maz@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Matthias Kaehlcke <mka@chromium.org>,
        Lee Jones <lee@kernel.org>, Wolfram Sang <wsa@kernel.org>,
        Tyrone Ting <kfting@nuvoton.com>,
        Mark Brown <broonie@kernel.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>, linux-usb@vger.kernel.org,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-spi@vger.kernel.org, linux-gpio@vger.kernel.org,
        srinivas.pandruvada@intel.com, heikki.krogerus@linux.intel.com,
        andriy.shevchenko@linux.intel.com, sakari.ailus@linux.intel.com,
        zhifeng.wang@intel.com, wentong.wu@intel.com, lixu.zhang@intel.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Ye,

thanks for your patch!

On Sat, Mar 25, 2023 at 4:48=E2=80=AFPM Ye Xiang <xiang.ye@intel.com> 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 US=
B
> driver.
>
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>

Lots of improvements! here are some comments:

> @@ -1253,6 +1253,18 @@ config GPIO_KEMPLD
>           This driver can also be built as a module. If so, the module wi=
ll be
>           called gpio-kempld.
>
> +config GPIO_LJCA
> +       tristate "INTEL La Jolla Cove Adapter GPIO support"
> +       depends on USB_LJCA
> +       select GPIOLIB_IRQCHIP
> +       default USB_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.

The GPIO Kconfig has a separate submenu for USB expanders, so
put this Kconfig in that submenu. This makes the choice come in
a more logical spot and not appear on configs that don't even
have USB.

(...)
> +       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;
(...)

With IRQ code like this from a USB callback:

> +static void ljca_gpio_event_cb(void *context, u8 cmd, const void *evt_da=
ta, int len)
> +{
> +       const struct gpio_packet *packet =3D evt_data;
> +       struct ljca_gpio_dev *ljca_gpio =3D context;
> +       int i;
> +       int irq;
> +
> +       if (cmd !=3D LJCA_GPIO_INT_EVENT)
> +               return;
> +
> +       for (i =3D 0; i < packet->num; i++) {
> +               irq =3D irq_find_mapping(ljca_gpio->gc.irq.domain, packet=
->item[i].index);
> +               if (!irq) {
> +                       dev_err(ljca_gpio->gc.parent, "gpio_id %u does no=
t mapped to IRQ yet\n",
> +                               packet->item[i].index);
> +                       return;
> +               }
> +
> +               generic_handle_domain_irq(ljca_gpio->gc.irq.domain, irq);
> +               set_bit(packet->item[i].index, ljca_gpio->reenable_irqs);
> +       }
> +
> +       schedule_work(&ljca_gpio->work);
> +}

I don't feel comfortable merging this unless Marc Zyngier has looked at the
code first, so please CC him on this patch next time.

> +static const struct irq_chip ljca_gpio_irqchip =3D {
> +       .name =3D "ljca-irq",
> +       .irq_mask =3D ljca_irq_mask,
> +       .irq_unmask =3D ljca_irq_unmask,
> +       .irq_set_type =3D ljca_irq_set_type,
> +       .irq_bus_lock =3D ljca_irq_bus_lock,
> +       .irq_bus_sync_unlock =3D ljca_irq_bus_unlock,
> +       .flags =3D IRQCHIP_IMMUTABLE,
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

Thanks for fixing the immutable irq chip!

> +       ljca_gpio->auxdev =3D auxdev;
> +       ljca_gpio->gc.direction_input =3D ljca_gpio_direction_input;
> +       ljca_gpio->gc.direction_output =3D ljca_gpio_direction_output;

Can you implement .get_direction()?

It's scanned on probe to determine the initial state of each
line so it is very nice to have.

Yours,
Linus Walleij