| Message ID | 20240704-dev-spi-xcomm-gpiochip-v1-0-653db6fbef36@analog.com |
|---|---|
| Headers | show |
| Series | spi: xcomm: support GPO pin | expand |
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.
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().
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á
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á