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-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
