Re: [PATCH 6/8] gpio-tz1090: add TZ1090 gpio driver

[Linus Walleij]

On Fri, Apr 26, 2013 at 11:22 AM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
>> On Tue, Apr 23, 2013 at 4:33 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:

>> So when someone using this device tree for Symbian or Windows
>> Mobile start to work, what does "standard Linux gpio flags" tell them?
>
> well this is what of_gpio.h says:
>> /*
>>  * This is Linux-specific flags. By default controllers' and Linux' mapping
>>  * match, but GPIO controllers are free to translate their own flags to
>>  * Linux-specific in their .xlate callback. Though, 1:1 mapping is recommended.
>>  */
>> enum of_gpio_flags {
>>       OF_GPIO_ACTIVE_LOW = 0x1,
>> };
>
> So do you mean that the bindings docs should just explicitly state those
> flag values rather than referring to Linux?

Yes, without mentioning Linux.

"setting bit 0 means active low".

> So basically a bunch of global registers (e.g. pinctrl and gpio) are
> shared between all 3 cores (up to 4 OSes). The __global_lock2 should do
> all that is required to ensure exclusive access to the register (as long
> as the other OSes do something similar when they access the same
> registers). This is one of the reasons why there are 3 gpio banks with
> separate interrupts, and each bank's interrupt is optional in this driver.

OK I get it ...

I think this platform will never ever work with single zImage
though, that seems very unlikely given these constraints.

Well you will have to fight this out with the ARM SoC maintainers
anyway. If they are OK with it I will live with it.

(CC ARM SoC for this.)

>>> +/* caller must hold LOCK2 */
>>> +static inline void _tz1090_gpio_mod_bit(struct tz1090_gpio_bank *bank,
>>> +                                       unsigned int reg_offs,
>>> +                                       unsigned int offset,
>>> +                                       int val)
>>
>> If val is used as it is, make it a bool.
>>
>>> +{
>>> +       u32 value;
>>> +
>>> +       value = tz1090_gpio_read(bank, reg_offs);
>>> +       value &= ~(0x1 << offset);
>>> +       value |= !!val << offset;
>>
>> You're claming val to [0,1] obviously it's a bool.
>
> It's often simply passing on things like the int output_value from the
> direction_output and set callbacks. Is it necessary to confuse things by
> making it a bool and then turning it back into an int to write into the
> register?

Nah no big deal. Keep it like this if it's convenient.
Mainly pointing it out...

Yours,
Linus Walleij
--
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/