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

[Linus Walleij <linus.walleij@xxxxxxxxxx>]

On Wed, May 21, 2014 at 7:22 AM, Zhu, Lejun <lejun.zhu@xxxxxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html