Re: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control

From: Linus Walleij <linus.walleij@linaro.org>
To: Brad Larson <brad@pensando.io>
Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>,
	Mark Brown <broonie@kernel.org>,
	Serge Semin <fancer.lancer@gmail.com>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Ulf Hansson <ulf.hansson@linaro.org>,
	Olof Johansson <olof@lixom.net>,
	"open list:GPIO SUBSYSTEM" <linux-gpio@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	linux-mmc <linux-mmc@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/8] gpio: Add Elba SoC gpio driver for spi cs control
Date: Thu, 4 Mar 2021 09:29:33 +0100	[thread overview]
Message-ID: <CACRpkdbQD6p7fbGtuu1c92uXfSFDCTwqjqsXHpgnD5Lg4v0Okw@mail.gmail.com> (raw)
In-Reply-To: <20210304034141.7062-2-brad@pensando.io>

Hi Brad,

thanks for your patch!

On Thu, Mar 4, 2021 at 4:42 AM Brad Larson <brad@pensando.io> wrote:

> This GPIO driver is for the Pensando Elba SoC which
> provides control of four chip selects on two SPI busses.
>
> Signed-off-by: Brad Larson <brad@pensando.io>
(...)

> +#include <linux/gpio.h>

Use this in new drivers:
#include <linux/gpio/driver.h>

> + * pin:             3            2        |       1            0
> + * bit:         7------6------5------4----|---3------2------1------0
> + *     cs1  cs1_ovr  cs0  cs0_ovr |  cs1  cs1_ovr  cs0  cs0_ovr
> + *                ssi1            |             ssi0
> + */
> +#define SPICS_PIN_SHIFT(pin)   (2 * (pin))
> +#define SPICS_MASK(pin)                (0x3 << SPICS_PIN_SHIFT(pin))
> +#define SPICS_SET(pin, val)    ((((val) << 1) | 0x1) << SPICS_PIN_SHIFT(pin))

So 2 bits per GPIO line in one register? (Nice doc!)

> +struct elba_spics_priv {
> +       void __iomem *base;
> +       spinlock_t lock;
> +       struct gpio_chip chip;
> +};
> +
> +static int elba_spics_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       return -ENXIO;
> +}

Write a comment that the chip only supports output mode,
because it repurposes SPI CS pins as generic GPIO out,
maybe at the top of the file?

I suppose these systems also actually (ab)use the SPI cs
for things that are not really SPI CS? Because otherwise
this could just be part of the SPI driver (native chip select).

> +static const struct of_device_id ebla_spics_of_match[] = {
> +       { .compatible = "pensando,elba-spics" },

Have you documented this?

Other than that this is a nice and complete driver.

Yours,
Linus Walleij