MIME-Version: 1.0
References: <20230219183059.1029525-1-xiang.ye@intel.com> <20230219183059.1029525-3-xiang.ye@intel.com>
In-Reply-To: <20230219183059.1029525-3-xiang.ye@intel.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Fri, 24 Feb 2023 11:48:08 +0100
Message-ID: <CACRpkdYdDE+B=SF+kAuQBiwUi31y_xELLXuDK+9G+rqD9MQBGg@mail.gmail.com>
Subject: Re: [PATCH 2/5] gpio: Add support for Intel LJCA USB GPIO driver
To:     Ye Xiang <xiang.ye@intel.com>
Cc:     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-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"
Precedence: bulk

Hi Ye,

thanks for your patch!

I think your colleague Andy Shevchenko will provide the most detailed
and deep feedback, but here are some things I spotted immediately:

On Sun, Feb 19, 2023 at 7:31 PM 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 USB
> driver.
>
> Signed-off-by: Ye Xiang <xiang.ye@intel.com>
(...)

> +config GPIO_LJCA
> +       tristate "INTEL La Jolla Cove Adapter GPIO support"
> +       depends on MFD_LJCA

I would add

   default MFD_LJCA

so if you activate the MFD you get this subdriver by default
as module or built-in depending on what the MFD is built
as.

(Same goes for the other subdrivers I guess)

In addition you need:

select GPIOLIB_IRQCHIP

since you use this facility.

> +static struct irq_chip ljca_gpio_irqchip = {

static const ...

> +       .name = "ljca-irq",
> +       .irq_mask = ljca_irq_mask,
> +       .irq_unmask = ljca_irq_unmask,
> +       .irq_set_type = ljca_irq_set_type,
> +       .irq_bus_lock = ljca_irq_bus_lock,
> +       .irq_bus_sync_unlock = ljca_irq_bus_unlock,
> +       .flags = IRQCHIP_IMMUTABLE,
> +       GPIOCHIP_IRQ_RESOURCE_HELPERS,
> +};

Yours,
Linus Walleij