Re: [PATCH 2/2] gpio: Add driver for SPI serializers

[Andrew F. Davis]

On 12/11/2015 04:09 PM, Linus Walleij wrote:
> On Fri, Dec 11, 2015 at 8:46 PM, Andrew F. Davis <afd@xxxxxx> 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@xxxxxx>
> (...)
> > +#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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/