Re: [PATCH 10/16] gpio: add driver for Atheros AR2315 SoC GPIO controller

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, Oct 15, 2014 at 1:12 PM, Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx> wrote:
> 2014-10-15 12:58 GMT+04:00, Linus Walleij <linus.walleij@xxxxxxxxxx>:
>> On Sun, Sep 28, 2014 at 8:33 PM, Sergey Ryazanov <ryazanov.s.a@xxxxxxxxx>

> I have one more generic question: could you merge driver without
> GPIOLIB_IRQCHIP usage?

No.

> Currently no one driver for the AR231x SoCs
> uses irq_domain and I do not like to enable IRQ_DOMAIN just for one
> driver. I plan to convert drivers to make them irq_domain aware a bit
> later.

I don't believe any such promises. It's nothing personal, just I've
been burned too many times by people promising to "fix later".

>>> +static u32 ar2315_gpio_intmask;
>>> +static u32 ar2315_gpio_intval;
>>> +static unsigned ar2315_gpio_irq_base;
>>> +static void __iomem *ar2315_mem;
>>
>> No static locals. Allocate and use a state container, see
>> Documentation/driver-model/design-patterns.txt
>>
> Is that rule mandatory for drivers, which serve only one device?

There is no central authority which decides what is mandatory
or not. It is mandatory to get a driver past the GPIO maintainer.

>>> +static inline u32 ar2315_gpio_reg_read(unsigned reg)
>>> +{
>>> +       return __raw_readl(ar2315_mem + reg);
>>> +}
>>
>> Use readl_relaxed() instead.
>>
> readl_relaxed() converts the bit ordering and seems inapplicable in this case.

It assumes the peripherals IO memory is little endian.

If the IO memory for this device is little endian, please stay with
[readl|writel]_relaxed so it looks familiar.

Or is this machine really using big endian hardware registers?
In that case I understand your comment...

>> When you use the state container, you need to do a
>> state dereference like that:
>>
>> mystate->base + reg
>>
>> So I don't think these inlines buy you anything. Just use
>> readl/writel_relaxed directly in the code.
>>
> These helpers make code shorter and clearer. I can use macros if you
> do preferred.

No big deal. Keep it if you like it this way.

>> So why is .set_type() not implemented and instead hard-coded into
>> the unmask function? Please fix this. It will be called by the
>> core eventually no matter what.
>>
> The interrupt configuration is a bit complex. This controller could be
> configured to generate interrupts only for two lines at once. Or in
> other words: user could select any two lines to generate interrupt.

Oh well, better just handle it I guess...

>>> +static int __init ar2315_gpio_init(void)
>>> +{
>>> +       return platform_driver_register(&ar2315_gpio_driver);
>>> +}
>>> +subsys_initcall(ar2315_gpio_init);
>>
>> Why are you using subsys_initcall()?
>>
>> This should not be necessary.
>>
> I have users of GPIO in arch code, what called earlier than the
> devices initcall.

OK? Why are there such users that early and what do they
use the GPIOs for? Any reason they cannot be device_initcall()s?

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