[PATCH 0/4] spi: xcomm: support GPO pin

[PATCH 0/4] spi: xcomm: support GPO pin

[Nuno Sa]

Main purpose of the series is to expose one supported pin as GPO through
a simple gpiochip.

While in here and as the driver is fairly simple add some more straight
forward cleanups. 

---
Michael Hennerich (1):
      spi: xcomm: add gpiochip support

Nuno Sa (3):
      spi: xcomm: make use of devm_spi_alloc_host()
      spi: xcomm: remove i2c_set_clientdata()
      spi: xcomm: fix coding style

 drivers/spi/spi-xcomm.c | 88 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 70 insertions(+), 18 deletions(-)
---
base-commit: 5b6cad81d0c1b1c71533f2898a47f3d2fcfc4595
change-id: 20240704-dev-spi-xcomm-gpiochip-8114c9894f07
--

Thanks!
- Nuno Sá

[PATCH 1/4] spi: xcomm: add gpiochip support

[Nuno Sa]

From: Michael Hennerich <michael.hennerich@analog.com>

The hardware can expose one pin as a GPO. Hence, register a simple
gpiochip to support it.

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Co-developed-by: Nuno Sa <nuno.sa@analog.com>
Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/spi/spi-xcomm.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-xcomm.c b/drivers/spi/spi-xcomm.c
index 63354dd3110f..358e0b84ee60 100644
--- a/drivers/spi/spi-xcomm.c
+++ b/drivers/spi/spi-xcomm.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/delay.h>
 #include <linux/i2c.h>
+#include <linux/gpio/driver.h>
 #include <linux/spi/spi.h>
 #include <asm/unaligned.h>
 
@@ -26,12 +27,16 @@
 
 #define SPI_XCOMM_CMD_UPDATE_CONFIG	0x03
 #define SPI_XCOMM_CMD_WRITE		0x04
+#define SPI_XCOMM_CMD_GPIO_SET		0x05
 
 #define SPI_XCOMM_CLOCK 48000000
 
 struct spi_xcomm {
 	struct i2c_client *i2c;
 
+	struct gpio_chip gc;
+	int gpio_val;
+
 	uint16_t settings;
 	uint16_t chipselect;
 
@@ -40,6 +45,54 @@ struct spi_xcomm {
 	uint8_t buf[63];
 };
 
+static void spi_xcomm_gpio_set_value(struct gpio_chip *chip,
+				     unsigned int offset, int val)
+{
+	struct spi_xcomm *spi_xcomm = gpiochip_get_data(chip);
+	unsigned char buf[2];
+	int ret;
+
+	buf[0] = SPI_XCOMM_CMD_GPIO_SET;
+	buf[1] = !!val;
+	ret = i2c_master_send(spi_xcomm->i2c, buf, 2);
+	if (ret < 0)
+		return;
+
+	spi_xcomm->gpio_val = !!val;
+}
+
+static int spi_xcomm_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
+{
+	struct spi_xcomm *spi_xcomm = gpiochip_get_data(chip);
+
+	return spi_xcomm->gpio_val;
+}
+
+static int spi_xcomm_gpio_get_direction(struct gpio_chip *chip,
+					unsigned int offset)
+{
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int spi_xcomm_gpio_add(struct spi_xcomm *spi_xcomm)
+{
+	struct device *dev = &spi_xcomm->i2c->dev;
+
+	if (!IS_ENABLED(CONFIG_GPIOLIB))
+		return 0;
+
+	spi_xcomm->gc.get_direction = spi_xcomm_gpio_get_direction;
+	spi_xcomm->gc.get = spi_xcomm_gpio_get_value;
+	spi_xcomm->gc.set = spi_xcomm_gpio_set_value;
+	spi_xcomm->gc.can_sleep = 1;
+	spi_xcomm->gc.base = -1;
+	spi_xcomm->gc.ngpio = 1;
+	spi_xcomm->gc.label = spi_xcomm->i2c->name;
+	spi_xcomm->gc.owner = THIS_MODULE;
+
+	return devm_gpiochip_add_data(dev, &spi_xcomm->gc, spi_xcomm);
+}
+
 static int spi_xcomm_sync_config(struct spi_xcomm *spi_xcomm, unsigned int len)
 {
 	uint16_t settings;
@@ -227,7 +280,7 @@ static int spi_xcomm_probe(struct i2c_client *i2c)
 	if (ret < 0)
 		spi_controller_put(host);
 
-	return ret;
+	return spi_xcomm_gpio_add(spi_xcomm);
 }
 
 static const struct i2c_device_id spi_xcomm_ids[] = {

-- 
2.45.2

[PATCH 2/4] spi: xcomm: make use of devm_spi_alloc_host()

[Nuno Sa]

Use devm_spi_alloc_host() so that there's no need to call
spi_controller_put() in the error path.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/spi/spi-xcomm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-xcomm.c b/drivers/spi/spi-xcomm.c
index 358e0b84ee60..5a27eaed4183 100644
--- a/drivers/spi/spi-xcomm.c
+++ b/drivers/spi/spi-xcomm.c
@@ -261,7 +261,7 @@ static int spi_xcomm_probe(struct i2c_client *i2c)
 	struct spi_controller *host;
 	int ret;
 
-	host = spi_alloc_host(&i2c->dev, sizeof(*spi_xcomm));
+	host = devm_spi_alloc_host(&i2c->dev, sizeof(*spi_xcomm));
 	if (!host)
 		return -ENOMEM;
 
@@ -278,7 +278,7 @@ static int spi_xcomm_probe(struct i2c_client *i2c)
 
 	ret = devm_spi_register_controller(&i2c->dev, host);
 	if (ret < 0)
-		spi_controller_put(host);
+		return ret;
 
 	return spi_xcomm_gpio_add(spi_xcomm);
 }

-- 
2.45.2

[PATCH 3/4] spi: xcomm: remove i2c_set_clientdata()

[Nuno Sa]

i2c_get_clientdata() is not being called anywhere so that we do not need
to set clientdata.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/spi/spi-xcomm.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/spi/spi-xcomm.c b/drivers/spi/spi-xcomm.c
index 5a27eaed4183..136d81fb5f56 100644
--- a/drivers/spi/spi-xcomm.c
+++ b/drivers/spi/spi-xcomm.c
@@ -274,7 +274,6 @@ static int spi_xcomm_probe(struct i2c_client *i2c)
 	host->flags = SPI_CONTROLLER_HALF_DUPLEX;
 	host->transfer_one_message = spi_xcomm_transfer_one;
 	host->dev.of_node = i2c->dev.of_node;
-	i2c_set_clientdata(i2c, host);
 
 	ret = devm_spi_register_controller(&i2c->dev, host);
 	if (ret < 0)

-- 
2.45.2

[PATCH 4/4] spi: xcomm: fix coding style

[Nuno Sa]

Just cosmetics. No functional change intended.

While at it, removed a couple of redundant else if() statements.

Signed-off-by: Nuno Sa <nuno.sa@analog.com>
---
 drivers/spi/spi-xcomm.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-xcomm.c b/drivers/spi/spi-xcomm.c
index 136d81fb5f56..9052bddedd10 100644
--- a/drivers/spi/spi-xcomm.c
+++ b/drivers/spi/spi-xcomm.c
@@ -37,12 +37,12 @@ struct spi_xcomm {
 	struct gpio_chip gc;
 	int gpio_val;
 
-	uint16_t settings;
-	uint16_t chipselect;
+	u16 settings;
+	u16 chipselect;
 
 	unsigned int current_speed;
 
-	uint8_t buf[63];
+	u8 buf[63];
 };
 
 static void spi_xcomm_gpio_set_value(struct gpio_chip *chip,
@@ -95,8 +95,8 @@ static int spi_xcomm_gpio_add(struct spi_xcomm *spi_xcomm)
 
 static int spi_xcomm_sync_config(struct spi_xcomm *spi_xcomm, unsigned int len)
 {
-	uint16_t settings;
-	uint8_t *buf = spi_xcomm->buf;
+	u16 settings;
+	u8 *buf = spi_xcomm->buf;
 
 	settings = spi_xcomm->settings;
 	settings |= len << SPI_XCOMM_SETTINGS_LEN_OFFSET;
@@ -109,10 +109,10 @@ static int spi_xcomm_sync_config(struct spi_xcomm *spi_xcomm, unsigned int len)
 }
 
 static void spi_xcomm_chipselect(struct spi_xcomm *spi_xcomm,
-	struct spi_device *spi, int is_active)
+				 struct spi_device *spi, int is_active)
 {
 	unsigned long cs = spi_get_chipselect(spi, 0);
-	uint16_t chipselect = spi_xcomm->chipselect;
+	u16 chipselect = spi_xcomm->chipselect;
 
 	if (is_active)
 		chipselect |= BIT(cs);
@@ -123,7 +123,8 @@ static void spi_xcomm_chipselect(struct spi_xcomm *spi_xcomm,
 }
 
 static int spi_xcomm_setup_transfer(struct spi_xcomm *spi_xcomm,
-	struct spi_device *spi, struct spi_transfer *t, unsigned int *settings)
+				    struct spi_device *spi, struct spi_transfer *t,
+				    unsigned int *settings)
 {
 	if (t->len > 62)
 		return -EINVAL;
@@ -161,7 +162,7 @@ static int spi_xcomm_setup_transfer(struct spi_xcomm *spi_xcomm,
 }
 
 static int spi_xcomm_txrx_bufs(struct spi_xcomm *spi_xcomm,
-	struct spi_device *spi, struct spi_transfer *t)
+			       struct spi_device *spi, struct spi_transfer *t)
 {
 	int ret;
 
@@ -172,13 +173,13 @@ static int spi_xcomm_txrx_bufs(struct spi_xcomm *spi_xcomm,
 		ret = i2c_master_send(spi_xcomm->i2c, spi_xcomm->buf, t->len + 1);
 		if (ret < 0)
 			return ret;
-		else if (ret != t->len + 1)
+		if (ret != t->len + 1)
 			return -EIO;
 	} else if (t->rx_buf) {
 		ret = i2c_master_recv(spi_xcomm->i2c, t->rx_buf, t->len);
 		if (ret < 0)
 			return ret;
-		else if (ret != t->len)
+		if (ret != t->len)
 			return -EIO;
 	}
 
@@ -186,12 +187,12 @@ static int spi_xcomm_txrx_bufs(struct spi_xcomm *spi_xcomm,
 }
 
 static int spi_xcomm_transfer_one(struct spi_controller *host,
-	struct spi_message *msg)
+				  struct spi_message *msg)
 {
 	struct spi_xcomm *spi_xcomm = spi_controller_get_devdata(host);
 	unsigned int settings = spi_xcomm->settings;
 	struct spi_device *spi = msg->spi;
-	unsigned cs_change = 0;
+	unsigned int cs_change = 0;
 	struct spi_transfer *t;
 	bool is_first = true;
 	int status = 0;
@@ -200,7 +201,6 @@ static int spi_xcomm_transfer_one(struct spi_controller *host,
 	spi_xcomm_chipselect(spi_xcomm, spi, true);
 
 	list_for_each_entry(t, &msg->transfers, transfer_list) {
-
 		if (!t->tx_buf && !t->rx_buf && t->len) {
 			status = -EINVAL;
 			break;

-- 
2.45.2

Re: [PATCH 1/4] spi: xcomm: add gpiochip support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 393 bytes --]

On Thu, Jul 04, 2024 at 03:49:12PM +0200, Nuno Sa wrote:

> +static int spi_xcomm_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
> +{
> +	struct spi_xcomm *spi_xcomm = gpiochip_get_data(chip);
> +
> +	return spi_xcomm->gpio_val;
> +}

It seems like the hardware doesn't support input at all so should there
even be a get operation?  gpiolib appears to cope fine with omitting it.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: xcomm: add gpiochip support

[Nuno Sá]

On Thu, 2024-07-04 at 14:53 +0100, Mark Brown wrote:
> On Thu, Jul 04, 2024 at 03:49:12PM +0200, Nuno Sa wrote:
> 
> > +static int spi_xcomm_gpio_get_value(struct gpio_chip *chip, unsigned int
> > offset)
> > +{
> > +	struct spi_xcomm *spi_xcomm = gpiochip_get_data(chip);
> > +
> > +	return spi_xcomm->gpio_val;
> > +}
> 
> It seems like the hardware doesn't support input at all so should there
> even be a get operation?  gpiolib appears to cope fine with omitting it.

Just following recommendations :)

https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/gpio/driver.h#L336

- Nuno Sá

Re: [PATCH 1/4] spi: xcomm: add gpiochip support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Thu, Jul 04, 2024 at 04:00:15PM +0200, Nuno Sá wrote:
> On Thu, 2024-07-04 at 14:53 +0100, Mark Brown wrote:
> > On Thu, Jul 04, 2024 at 03:49:12PM +0200, Nuno Sa wrote:
> > 
> > > +static int spi_xcomm_gpio_get_value(struct gpio_chip *chip, unsigned int
> > > offset)
> > > +{
> > > +	struct spi_xcomm *spi_xcomm = gpiochip_get_data(chip);
> > > +
> > > +	return spi_xcomm->gpio_val;
> > > +}

> > It seems like the hardware doesn't support input at all so should there
> > even be a get operation?  gpiolib appears to cope fine with omitting it.

> Just following recommendations :)

> https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/gpio/driver.h#L336

That comment is for get_direction(), not get().

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: xcomm: add gpiochip support

[Nuno Sá]

On Thu, 2024-07-04 at 16:45 +0100, Mark Brown wrote:
> On Thu, Jul 04, 2024 at 04:00:15PM +0200, Nuno Sá wrote:
> > On Thu, 2024-07-04 at 14:53 +0100, Mark Brown wrote:
> > > On Thu, Jul 04, 2024 at 03:49:12PM +0200, Nuno Sa wrote:
> > > 
> > > > +static int spi_xcomm_gpio_get_value(struct gpio_chip *chip, unsigned int
> > > > offset)
> > > > +{
> > > > +	struct spi_xcomm *spi_xcomm = gpiochip_get_data(chip);
> > > > +
> > > > +	return spi_xcomm->gpio_val;
> > > > +}
> 
> > > It seems like the hardware doesn't support input at all so should there
> > > even be a get operation?  gpiolib appears to cope fine with omitting it.
> 
> > Just following recommendations :)
> 
> > https://elixir.bootlin.com/linux/v6.10-rc6/source/include/linux/gpio/driver.h#L336
> 
> That comment is for get_direction(), not get().

Oh right, sorry. For some reason I assumed get_direction()... Well, get() was mainly
to not get an error when reading the GPIO value from userspace (eg:
/sys/clagg/gpio). 

That said, we're just caching the value in the driver in case the i2c transfer does
not fail so I guess yes, we can make this even simpler and remove get() and
'gpio_val'. Userspace apps can very well cache the value themselves.

- Nuno Sá