Messages in this thread | | | Subject | Re: [PATCH 2/2] gpio: Add driver for SPI serializers | From | "Andrew F. Davis" <> | Date | Mon, 14 Dec 2015 10:47:22 -0600 |
| |
On 12/11/2015 04:09 PM, Linus Walleij wrote: > On Fri, Dec 11, 2015 at 8:46 PM, Andrew F. Davis <afd@ti.com> wrote: > >> Add generic parallel-in/serial-out shift register GPIO driver. >> >> This includes SPI compatible devices like SN74165 serial-out shift >> registers and the SN65HVS88x series of industrial serializers that can >> be read over the SPI bus and used for GPI (General Purpose Input). >> >> Signed-off-by: Andrew F. Davis <afd@ti.com> > (...) >> +#include <linux/delay.h> >> +#include <linux/gpio.h> > > Use #include <linux/gpio/driver.h> instead. >
ACK
>> +#include <linux/module.h> >> +#include <linux/mutex.h> >> +#include <linux/spi/spi.h> >> + >> +#define DEFAULT_NGPIO 8 >> + >> +struct pisosr_gpio { >> + struct gpio_chip chip; >> + struct spi_device *spi; >> + u8 *buffer; >> + size_t buffer_size; >> + struct gpio_desc *load_gpio; >> + struct mutex lock; >> +}; > > Add kerneldoc to this struct. >
Will do.
>> +static inline struct pisosr_gpio *to_pisosr_gpio(struct gpio_chip *chip) >> +{ >> + return container_of(chip, struct pisosr_gpio, chip); >> +} > > We are doing away with this, but I can fix up the driver by a separate > patch later of we merge it. >
That will work, thanks.
>> +static int pisosr_gpio_refresh(struct pisosr_gpio *gpio) >> +{ >> + int ret; >> + >> + mutex_lock(&gpio->lock); >> + >> + if (gpio->load_gpio) { >> + gpiod_set_value(gpio->load_gpio, 1); >> + udelay(1); /* registers load time (~10ns) */ >> + gpiod_set_value(gpio->load_gpio, 0); >> + udelay(1); /* registers recovery time (~5ns) */ > > > So aren't these load/recovery times dependent on the device? > I think these should come from the compatible string. >
Yes, but they are all under 20ns or so, I just put the smallest reasonable delay to keep a fast host from going under this limit. (although I doubt any could)
>> +static int pisosr_gpio_get_direction(struct gpio_chip *chip, >> + unsigned offset) >> +{ >> + return GPIOF_DIR_IN; >> +} > > Just return 1, GPIOF_DIR_IN is for the external API. >
ACK
>> +static int pisosr_gpio_get(struct gpio_chip *chip, unsigned offset) >> +{ >> + struct pisosr_gpio *gpio = to_pisosr_gpio(chip); >> + >> + /* Refresh may not always be needed */ >> + pisosr_gpio_refresh(gpio); >> + >> + return (gpio->buffer[offset / 8] >> (offset % 8)) & 0x1; >> +} > > This looks like a good reason to implement .get_multiple() in the > same way that we have .set_multiple(), so you agree? > > But it's not like I'm requiring you to engineer all that. Just an > academic reflection of the fact. >
I was disappointed when I saw only set_multiple, so this would be something nice to have, I'll look into it.
>> +static int pisosr_gpio_probe(struct spi_device *spi) >> +{ >> + struct pisosr_gpio *gpio; >> + struct device_node *np = spi->dev.of_node; >> + int ret; > > > To match and get a pointer to a compatible-string-specific data variant, > look at the example in drivers/mfd/tc3589x.c >
ACK
>> + gpio = devm_kzalloc(&spi->dev, sizeof(*gpio), GFP_KERNEL); >> + if (!gpio) >> + return -ENOMEM; >> + >> + spi_set_drvdata(spi, gpio); >> + >> + gpio->chip = template_chip; >> + gpio->chip.parent = &spi->dev; >> + of_property_read_u16(np, "ngpios", &gpio->chip.ngpio); > > As I wrote elsewhere, should come from the variant data, based on the > compatible string. ngpios is in case you're not using all of them and > need to restrict the number of used GPIOs. Usually this only applies to > on-SoC GPIOs that are unrouted. >
(See my reply to your previous comment on this)
>> + gpio->spi = spi; >> + >> + gpio->buffer_size = DIV_ROUND_UP(gpio->chip.ngpio, 8); >> + gpio->buffer = devm_kzalloc(&spi->dev, gpio->buffer_size, GFP_KERNEL); >> + if (!gpio->buffer) >> + return -ENOMEM; >> + >> + gpio->load_gpio = devm_gpiod_get(&spi->dev, "load", GPIOD_OUT_LOW); >> + if (IS_ERR(gpio->load_gpio)) { >> + ret = PTR_ERR(gpio->load_gpio); >> + if (ret != -ENOENT && ret != -ENOSYS) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(&spi->dev, "Unable to allocate reset gpio\n"); > > Reset gpio? Really? Load GPIO? >
Ops, copy/paste error I think.
Thanks, Andrew
> Apart from that it looks good. > > Yours, > Linus Walleij >
| |