On 29/05/13 16:32, Linus Walleij wrote:
> On Fri, May 24, 2013 at 6:21 PM, James Hogan <james.hogan@xxxxxxxxxx> wrote:
>
>> Add a GPIO driver for the main GPIOs found in the TZ1090 (Comet) SoC.
>> This doesn't include low-power GPIOs as they're controlled separately
>> via the Powerdown Controller (PDC) registers.
>>
>> The driver is instantiated by device tree and supports interrupts for
>> all GPIOs.
>
> (...)
>
> This is looking much better.
>
> However I have some more improvement comments, due to new
> knowledge. I am sorry about the moving target but it's not my fault...
No worries :-)
> And it will look like:
> interrupts = <13 IRQ_TYPE_LEVEL_HIGH>;
>
> Which is way easier to understand and you no longer
> need to insert comments to explain things.
Very nice. I like this. Thanks.
>> +/* REG_GPIO_IRQ_PLRT */
>> +#define GPIO_POLARITY_LOW 0
>> +#define GPIO_POLARITY_HIGH 1
>> +
>> +/* REG_GPIO_IRQ_TYPE */
>> +#define GPIO_LEVEL_TRIGGERED 0
>> +#define GPIO_EDGE_TRIGGERED 1
>
> Why does the comment start with REG_* but not the actual
> definition?
Legacy reasons (those constants were originally in that
<asm/soc-tz1090/gpio.h> header). I'll clean up the naming.
>
> (...)
>> +/* 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)
>> +{
>> + u32 value;
>> +
>> + value = tz1090_gpio_read(bank, reg_offs);
>> + value &= ~BIT(offset);
>> + value |= !!val << offset;
>
> I can't parse that last line, it is equivalent to writing:
>
> if (val)
> value |= BIT(offset);
>
> Which I think is easier to understand.
Apparently I was demonstrating how premature optimisation is the root of
all evil (as disassembling it testifies) :-). I'll stop doing this.
>
>
>> +/* set polarity to trigger on next edge, whether rising or falling */
>> +static void tz1090_gpio_irq_next_edge(struct tz1090_gpio_bank *bank,
>> + unsigned int offset)
>> +{
>> + unsigned int value_p, value_i;
>> + int lstat;
>> +
>> + __global_lock2(lstat);
>> + /* irq_polarity[offset] = !input[offset] */
>
> This comments probably need to be a bit more verbose, like explain
> to readers what is happening here.
Okay
>> +postcore_initcall(tz1090_gpio_init);
>
> Is it really necessary to have this so early?
It was necessary when I wanted GPIO setup to precede platform code that
hadn't been converted to DT yet but messed with GPIOs. For upstream I
think I can change it to subsys_initcall to match the majority of other
gpio drivers.
(for the record, in drivers/gpio/*.c:
1 core_initcall
1 device_initcall
1 late_initcall
2 pure_initcall
3 arch_initcall
14 postcore_initcall
29 subsys_initcall)
> Apart from these remarks it is looking very good.
Thanks for reviewing.
Cheers
James
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
