Re: [PATCH V3] gpio: New driver for LSI ZEVIO SoCs

Re: [PATCH V3] gpio: New driver for LSI ZEVIO SoCs

[Stephen Warren]

On 08/07/2013 06:53 AM, Fabian Vogt wrote:
> This driver supports the GPIO controller found in LSI ZEVIO SoCs.
> It has been successfully tested on a TI nspire CX calculator.

> diff --git a/Documentation/devicetree/bindings/gpio/gpio-zevio.txt b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt

> +Zevio GPIO controller
> +
> +Required properties:
> +- compatible = "lsi,zevio-gpio"

Is there only one zevio chip, or a series? Is "zevio" the full name of
the chip, including any version number?

> +- reg = <BASEADDR SIZE>
> +- #gpio-cells = <2>
> +- gpio-controller;
> +
> +Optional:
> +- #ngpios = <32>: Number of GPIOs. Defaults to 32 if absent

Perhaps one can derive that from the compatible value? The fact this
property exists implies there's more than one zevio chip, so perhaps
each should have an explicit compatible value described above?

Is the GPIO block not also an interrupt source/controller? I see the
following in the patch, and references to some IRQ registers...

> +	select GENERIC_IRQ_CHIP

Re: [PATCH V3] gpio: New driver for LSI ZEVIO SoCs

[Fabian Vogt]

Hi,

> On 08/07/2013 06:53 AM, Fabian Vogt wrote:
>> This driver supports the GPIO controller found in LSI ZEVIO SoCs.
>> It has been successfully tested on a TI nspire CX calculator.
>
>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-zevio.txt  
>> b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
>
>> +Zevio GPIO controller
>> +
>> +Required properties:
>> +- compatible = "lsi,zevio-gpio"
>
> Is there only one zevio chip, or a series? Is "zevio" the full name of
> the chip, including any version number?
We don't know, it's a relableled chip with
TI-NSPIRE / L9A0702 / TI-NS2006A-0 / LSI LOGIC / ZEVIO / U 0714 /  
WYJ14052-1
on it. But this driver should match the other drivers (lsi,zevio-intc,  
lsi,zevio-timer).

>> +- reg = <BASEADDR SIZE>
>> +- #gpio-cells = <2>
>> +- gpio-controller;
>> +
>> +Optional:
>> +- #ngpios = <32>: Number of GPIOs. Defaults to 32 if absent
>
> Perhaps one can derive that from the compatible value? The fact this
> property exists implies there's more than one zevio chip, so perhaps
> each should have an explicit compatible value described above?
I added it just for someone who maybe needs it. It's only two lines and  
maybe
it'll be helpful for someone. We don't know whether some similiar or this  
controller
exist in different configurations (pin count, section sice, register  
layout).
Also I hate hardcoded values which require a recompile to change..

> Is the GPIO block not also an interrupt source/controller? I see the
> following in the patch, and references to some IRQ registers...
>
>> +	select GENERIC_IRQ_CHIP
I forgot to remove this line after testing the interrupts, the tests went  
horribly (hard lockups)...

V4 should be underway soon.

Bye,
Fabian

Re: [PATCH V3] gpio: New driver for LSI ZEVIO SoCs

[Stephen Warren]

On 08/25/2013 01:44 PM, Fabian Vogt wrote:
> Hi,
> 
>> On 08/07/2013 06:53 AM, Fabian Vogt wrote:
>>> This driver supports the GPIO controller found in LSI ZEVIO SoCs.
>>> It has been successfully tested on a TI nspire CX calculator.
>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
>>> b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
>>
>>> +Zevio GPIO controller
>>> +
>>> +Required properties:
>>> +- compatible = "lsi,zevio-gpio"
>>
>> Is there only one zevio chip, or a series? Is "zevio" the full name of
>> the chip, including any version number?
>
> We don't know, it's a relableled chip with
> TI-NSPIRE / L9A0702 / TI-NS2006A-0 / LSI LOGIC / ZEVIO / U 0714 /
> WYJ14052-1
> on it. But this driver should match the other drivers (lsi,zevio-intc,
> lsi,zevio-timer).

OK, I guess that's fine then. I suppose if we find "zevio2" chips, we
can always use that for the compatible value later.

>>> +- reg = <BASEADDR SIZE>
>>> +- #gpio-cells = <2>
>>> +- gpio-controller;
>>> +
>>> +Optional:
>>> +- #ngpios = <32>: Number of GPIOs. Defaults to 32 if absent
>>
>> Perhaps one can derive that from the compatible value? The fact this
>> property exists implies there's more than one zevio chip, so perhaps
>> each should have an explicit compatible value described above?
>
> I added it just for someone who maybe needs it. It's only two lines and
> maybe
> it'll be helpful for someone. We don't know whether some similiar or
> this controller
> exist in different configurations (pin count, section sice, register
> layout).
> Also I hate hardcoded values which require a recompile to change..

If there's no reason for this property, I think we should remove it.