[U-Boot] [PATCH 1/4] gpio: UniPhier: add driver for UniPhier GPIO controller

[Simon Glass]

Hi Masahiro,

On 21 July 2015 at 21:46, Simon Glass <sjg at chromium.org> wrote:
> Hi Masahiro,
>
> On 21 July 2015 at 12:19, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>> Hi Simon,
>>
>>
>> 2015-07-18 23:36 GMT+09:00 Simon Glass <sjg at chromium.org>:
>>> Hi Masahiro,
>>>
>>> On 13 July 2015 at 02:29, Masahiro Yamada <yamada.masahiro at socionext.com> wrote:
>>>> This GPIO controller device is used on UniPhier SoCs.
>>>>
>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro at socionext.com>
>>>> ---
>>>>
>>>>  drivers/gpio/Kconfig         |   6 ++
>>>>  drivers/gpio/Makefile        |   1 +
>>>>  drivers/gpio/gpio-uniphier.c | 186 +++++++++++++++++++++++++++++++++++++++++++
>>>>  3 files changed, 193 insertions(+)
>>>>  create mode 100644 drivers/gpio/gpio-uniphier.c
>>>>

[snip]

>>>> +static void uniphier_gpio_offset_write(struct udevice *dev, unsigned offset,
>>>> +                                      unsigned reg_type, int value)
>>>> +{
>>>> +       struct uniphier_gpio_priv *priv = dev_get_priv(dev);
>>>> +       unsigned bank = offset / UNIPHIER_GPIO_PORTS_PER_BANK;
>>>> +       unsigned bit = offset % UNIPHIER_GPIO_PORTS_PER_BANK;
>>>> +       unsigned reg;
>>>> +       u32 tmp;
>>>> +
>>>> +       reg = uniphier_gpio_bank_to_reg(bank, reg_type);
>>>
>>> I think what you want is a struct gpio_bank or similar, and then store
>>> the bank pointer in the device. We should use C struct access for I/O.
>>
>> Why?  Rationale please?
>>
>> When I see drivers/gpio/gpio-tegra.c in Linux,
>> I only see macros
>>
>> #define GPIO_CNF(x) (GPIO_REG(x) + 0x00)
>> #define GPIO_OE(x) (GPIO_REG(x) + 0x10)
>> #define GPIO_OUT(x) (GPIO_REG(x) + 0X20)
>> #define GPIO_IN(x) (GPIO_REG(x) + 0x30)
>> #define GPIO_INT_STA(x) (GPIO_REG(x) + 0x40)
>> #define GPIO_INT_ENB(x) (GPIO_REG(x) + 0x50)
>> #define GPIO_INT_LVL(x) (GPIO_REG(x) + 0x60)
>> #define GPIO_INT_CLR(x) (GPIO_REG(x) + 0x70)
>>
>>
>> Actually, looks like macros are more often used in Linux for accessing I/O.
>
> Right, but U-Boot uses structures normally.
>
>>
>> I am negative about using C struct for this purpose because:
>>
>>  - C struct is not flexible because we can not change the register stride
>>    with the "reg-shift" property.
>
> Do you need to? To me that would be unusual hardware.

Or are you saying that on Uniphier each SoC uses a different register
size and so the driver has to handle this? If so then I agree what you
have is best. I noticed something similar in your pinctrl series.

[snip]

Regards,
Simon