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

[Thor Thayer <tthayer@opensource.altera.com>]

Hi Linus,

On 04/01/2016 07:17 AM, Linus Walleij wrote:
> On Tue, Mar 29, 2016 at 9:13 PM,  <tthayer@opensource.altera.com> wrote:
>
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> 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@opensource.altera.com>
>
> 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
>