Re: [PATCH] gpio: Renesas RZ GPIO driver

[Magnus Damm]

Hi Linus,

On Wed, Nov 13, 2013 at 4:59 AM, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Thu, Nov 7, 2013 at 12:47 AM, Magnus Damm <magnus.damm@xxxxxxxxx> wrote:
>
>> From: Magnus Damm <damm@xxxxxxxxxxxxx>
>>
>> This patch adds a GPIO driver for the RZ series of SoCs from
>> Renesas. The driver can be used as platform device with dynamic
>> or static GPIO assignment or via DT using dynamic GPIOs.
>
> So given that this is for a new system which should only ever
> be booted using device tree, why are we bothering with supporting
> platform data passing at all?

Mainly to support the same interfaces as our other GPIO drivers. But I
can easily remove the platform data init method if that is the
preferred way, no problem.

> Is it so that arch/sh is more soft on this for example...?
> Can some arch maintainer like SH/Paul ACK this approach?
>
> Read: SH is not moving to device tree...?

>From what I can tell this GPIO block is not used with SH, so I don't
think SH is related, but regarding DT on SH, do you know when it was
decided that other architectures also were supposed to move DT?

> (...)
>> Tested with yet-to-be-posted platform device and DT devices on
>> r7s72100 and Genmai using LEDs, DIP switches and I2C bitbang.
>
> Do you think the maintainers will merge the platform
> device approach?

I would not assume so. But the goal with these patches is  not
upstream, instead they basically serve as a stop-gap solution between
now and when I get OK that the DT bits in this GPIO driver looks fine.
If they are going to be merged or not is a different question IMO.

>> --- /dev/null
>> +++ work/include/linux/platform_data/gpio-rz.h  2013-11-06 14:18:46.000000000 +0900
>> @@ -0,0 +1,13 @@
>> +#ifndef __GPIO_RZ_H__
>> +#define __GPIO_RZ_H__
>> +
>> +struct gpio_rz_config {
>> +       int gpio_base;
>
> Passing these static base offsets around is not good for the
> kernel and we're trying to get rid of it :-(

Sure.

>> +       const char *pctl_name;
>
> Ho hum... This needs some kerneldoc describing that this is
> used to map the GPIO range to the right pin controller.

Ok, but it will just go away when the platform data init method is removed

>> +#define RZ_GPIOS_PER_PORT 16
>
> This is only used in the driver so move it into the driver.

It is also used by the macro below. =)

>> +#define RZ_PORT_PIN(bank, pin) (((bank) * RZ_GPIOS_PER_PORT) + (pin))
>
> This is not used anywhere so delete it.
>
> If it is to be kept I'd like "pin" replaced with "line" to avoid
> confusion with the pin control business.

The idea with that macro is to allow board code to select which pin
from which bank, but I realize it may clash with pinctrl terminology.
Also, since the only reason for this header file is to provide a
platform data init interface for the driver all these things will go
away if we go DT-only.

I'll ditch the platform data interface and post a V2.

Thanks for your feedback!

Cheers,

/ magnus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/