Hi Linus,
On Wed, Nov 13, 2013 at 4:59 AM, Linus Walleij <linus.wall...@linaro.org> wrote:
> On Thu, Nov 7, 2013 at 12:47 AM, Magnus Damm <magnus.d...@gmail.com> wrote:
>
>> From: Magnus Damm <d...@opensource.se>
>>
>> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
