Re: [RFC 4/8] gpio: altera-a10sr: Add A10 System Resource Chip GPIO support.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Linus,

On 04/01/2016 07:17 AM, Linus Walleij wrote:
On Tue, Mar 29, 2016 at 9:13 PM,  <tthayer@xxxxxxxxxxxxxxxxxxxxx> wrote:

From: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>

Add the GPIO functionality for the Altera Arria10 MAX5 System Resource
Chip. The A10 MAX5 has 12 bits of GPIO assigned to switches, buttons,
and LEDs as a GPIO extender on the SPI bus.

Signed-off-by: Thor Thayer <tthayer@xxxxxxxxxxxxxxxxxxxxx>

OK...

As Lee says: split off the MFD patch so it is a pure GPIO driver
patch.


ACK

+#include <linux/gpio.h>

You should instead #include <linux/gpio/driver.h>

+#include <linux/mfd/altera-a10sr.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/seq_file.h>
+#include <linux/syscalls.h>
+#include <linux/uaccess.h>

Syscalls and uaccess??? I don't think so.

OK.

+struct altr_a10sr_gpio {
+       struct altr_a10sr *a10sc;
+       struct gpio_chip gp;
+};

Add some kerneldoc.

OK. To clarify, is this comment referring to the bindings document or something different?


+static inline struct altr_a10sr_gpio *to_altr_a10sr_gpio(struct gpio_chip *chip)
+{
+       return container_of(chip, struct altr_a10sr_gpio, gp);
+}

Don't use this old design pattern.

Use [devm_]gpiochip_add_data() and use gpiochip_get_data(gc) to get
a data pointer from the gpiochip.

+static int altr_a10sr_gpio_get(struct gpio_chip *gc, unsigned int nr)
+{
+       struct altr_a10sr_gpio *gpio = to_altr_a10sr_gpio(gc);

So this becomes
struct altr_a10sr_gpio *gpio = gpiochip_get_data(gc);


Got it. Thanks!

+       int ret;
+       unsigned char reg = ALTR_A10SR_LED_RD_REG + ALTR_A10SR_REG_OFFSET(nr);
+
+       ret = altr_a10sr_reg_read(gpio->a10sc, reg);
+
+       if (ret < 0)
+               return ret;
+
+       if (ret & (1 << ALTR_A10SR_REG_BIT(nr)))
+               return 1;

Do this instead:

return !!(ret & (1 << ALTR_A10SR_REG_BIT(nr)))

It raises the question whether ALTR_A10SR_REG_BIT
is just a reimplementation of the BIT() macro from
<linux/bitops.h>, please check this.


Got it. Yes, I will check that. Thanks.

+static int altr_a10sr_gpio_direction_input(struct gpio_chip *gc,
+                                          unsigned int nr)
+{
+       if ((nr >= ALTR_A10SR_IN_VALID_RANGE_LO) &&
+           (nr <= ALTR_A10SR_IN_VALID_RANGE_HI))
+               return 0;
+       return -EINVAL;
+}
+
+static int altr_a10sr_gpio_direction_output(struct gpio_chip *gc,
+                                           unsigned int nr, int value)
+{
+       if ((nr >= ALTR_A10SR_OUT_VALID_RANGE_LO) &&
+           (nr <= ALTR_A10SR_OUT_VALID_RANGE_HI))
+               return 0;
+       return -EINVAL;
+}

Does this mean that all lines are *always* input and output
at the same time?

If there is no .set_direction() callback and all lines are both
input and output it kind of implies that all lines are also
implicitly open drain do you agree?

Please check:
- If there is really no direction setting anywhere
- For example if some lines are hardwired as input and
   some lines are hardwired as output
- If that is not the case, verify that all lines are really
   open drain, they should be if all are both input and
   output at the same time.


I see your point. I'll investigate how to do this properly for your 2nd check above. Registers are hard-wired as input or output so I'll need to handle this properly and is why I didn't implement the .set_direction callback. Thanks for the explanation.

In my case, there are 12 valid GPIOs out of the 16 bits (the first 4 bits are unused). Bits 4-7 are output and bits 8-15 are inputs. I was using the IN_VALID range for the inputs and the OUT_VALID range for the outputs.

+       ret = gpiochip_add(&gpio->gp);
+       if (ret < 0) {
+               dev_err(&pdev->dev, "Could not register gpiochip, %d\n", ret);
+               return ret;
+       }

Use devm_gpiochip_add_data() instead.


OK. Thanks for reviewing!

Yours,
Linus Walleij

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux