Re: [PATCH v2] gpio: Add support for Intel SoC PMIC (Crystal Cove)

["Zhu, Lejun" <lejun.zhu@linux.intel.com>]

On 5/27/2014 5:11 PM, Linus Walleij wrote:
> On Wed, May 21, 2014 at 7:22 AM, Zhu, Lejun <lejun.zhu@linux.intel.com> wrote:
> 
>> Devices based on Intel SoC products such as Baytrail have a Power
>> Management IC. In the PMIC there are subsystems for voltage regulation,
>> A/D conversion, GPIO and PWMs. The PMIC in Baytrail-T platform is called
>> Crystal Cove.
>>
>> This patch adds support for the GPIO function in Crystal Cove.
>>
>> v2:
>> - Use IRQ chip helper to provide irqdomain.
>> - Implement .remove and can now build as a module.
>> - Various fix for unreadable or ugly code pieces.
> 
> This is much improved! I still have comments though.
> 
>> +#define GPIO_TO_CTL(gpio, dir)         \
>> +       ((gpio < 8 ? GPIO0P0CTL ## dir : GPIO1P0CTL ## dir) + (gpio % 8))
> 
> This is unreadble. Use an explicit static inline function instead.
> 
>> +static void crystalcove_update_irq_type(int gpio, int type)
>> +{
>> +       u8 ctli = GPIO_TO_CTL(gpio, I);
>> +
>> +       type &= IRQ_TYPE_EDGE_BOTH;
> 
> You silently ignore all other type configurations?
> 
>> +       intel_soc_pmic_clearb(ctli, CTLI_INTCNT_BE);
>> +
>> +       if (type == IRQ_TYPE_EDGE_BOTH)
>> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_BE);
>> +       else if (type == IRQ_TYPE_EDGE_RISING)
>> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_PE);
>> +       else if (type & IRQ_TYPE_EDGE_FALLING)
>> +               intel_soc_pmic_setb(ctli, CTLI_INTCNT_NE);
>> +}
> 
> I would prefer a switch(type) {} construction with a default:
> that warns.
> 
> (...)
>> +static int crystalcove_irq_type(struct irq_data *data, unsigned type)
>> +{
>> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
>> +       struct crystalcove_gpio *cg =
>> +               container_of(gc, struct crystalcove_gpio, chip);
> 
> I would create a static inline at the top of the file instead of
> using container_of() explicitly everywhere:
> 
> static inline struct crystalcove_gpio *to_cg(struct gpio_chip *gc)
> {
>     return container_of(gc, struct crystalcove_gpio, chip);
> }
> 
> Then just use:
> 
> struct crystalcove_gpio *cg = to_cg(gc);
> 
> Everywhere. Or if you only want the cg in some case (like this?)
> 
> struct crystalcove_gpio *cg = to_cg(irq_data_get_irq_chip_data(data));
> 
>> +       cg->trigger_type = type;
>> +       cg->update |= UPDATE_TYPE;
>> +
>> +       return 0;
>> +}
> (...)
> 
>> +       gpiochip_irqchip_add(&cg->chip, &crystalcove_irqchip, 0,
>> +                            handle_simple_irq, IRQ_TYPE_NONE);
> 
> Really nice. Thanks for doing this!
> 
> Yours,
> Linus Walleij
> 

Thank you. I will return -EINVAL for unsupported IRQ types and fix all
the coding style problems you listed here in the next version.

Best Regards
Lejun