lkml.org 
[lkml]   [2015]   [Dec]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/2] gpio: Add driver for SPI serializers
From
Date
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
>


\
 
 \ /
  Last update: 2015-12-14 18:01    [W:1.178 / U:0.464 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site