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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdYNFpj0VR-N9=9haCT2bAvHoQs-qAhSWOdqhxizsFUbQA@mail.gmail.com>
Date:	Tue, 27 May 2014 11:11:34 +0200
From:	Linus Walleij <linus.walleij@...aro.org>
To:	"Zhu, Lejun" <lejun.zhu@...ux.intel.com>
Cc:	Alexandre Courbot <gnurou@...il.com>,
	Mika Westerberg <mika.westerberg@...ux.intel.com>,
	Mathias Nyman <mathias.nyman@...ux.intel.com>,
	"linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	bin.yang@...el.com
Subject: Re: [PATCH v2] gpio: Add support for Intel SoC PMIC (Crystal Cove)

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