Re: [PATCH 4/4] pinctrl: Add Intel Cherryview/Braswell pin controller support

[Linus Walleij]

On Fri, Oct 24, 2014 at 2:16 PM, Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:

> This driver supports the pin/GPIO controllers found in newer Intel SoCs
> like Cherryview and Braswell. The driver provides full GPIO support and
> minimal set of pin controlling funtionality.
>
> The driver is based on the original Cherryview GPIO driver authored by Ning
> Li and Alan Cox.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>

*VERY* nice work Mika! Just minor nitpicks...

(...)
> +static int chv_config_get(struct pinctrl_dev *pctldev, unsigned pin,
> +                         unsigned long *config)
> +{
> +       struct chv_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
> +       enum pin_config_param param = pinconf_to_config_param(*config);
> +       unsigned long flags;
> +       u32 ctrl0, ctrl1;
> +       u16 arg = 0;
> +       u32 term;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> +       ctrl1 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL1));
> +       spin_unlock_irqrestore(&pctrl->lock, flags);
> +
> +       term = (ctrl0 & CHV_PADCTRL0_TERM_MASK) >> CHV_PADCTRL0_TERM_SHIFT;
> +
> +       switch (param) {
> +       case PIN_CONFIG_BIAS_DISABLE:
> +               if (term)
> +                       return -EINVAL;
> +               break;
> +
> +       case PIN_CONFIG_BIAS_PULL_UP:
> +               if (!(ctrl0 & CHV_PADCTRL0_TERM_UP))
> +                       return -EINVAL;
> +
> +               switch (term) {
> +               case CHV_PADCTRL0_TERM_20K:
> +                       arg = 20;

These are in Ohms IIRC so should be 20000

> +                       break;
> +               case CHV_PADCTRL0_TERM_5K:
> +                       arg = 5;

5000

> +                       break;
> +               case CHV_PADCTRL0_TERM_1K:
> +                       arg = 1;

1000

> +                       break;
> +               }
> +
> +               break;
> +
> +       case PIN_CONFIG_BIAS_PULL_DOWN:
> +               if (!term || (ctrl0 & CHV_PADCTRL0_TERM_UP))
> +                       return -EINVAL;
> +
> +               switch (term) {
> +               case CHV_PADCTRL0_TERM_20K:
> +                       arg = 20;

20000

> +                       break;
> +               case CHV_PADCTRL0_TERM_5K:
> +                       arg = 5;

5000

(...)
> +static int chv_config_set_pull(struct chv_pinctrl *pctrl, unsigned pin,
> +                              enum pin_config_param param, u16 arg)
> +{
> +       void __iomem *reg = chv_padreg(pctrl, pin, CHV_PADCTRL0);
> +       unsigned long flags;
> +       u32 ctrl0, pull;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       ctrl0 = readl(reg);
> +
> +       pull = CHV_PADCTRL0_TERM_20K << CHV_PADCTRL0_TERM_SHIFT;
> +       switch (arg) {

This looks seriously convoluted: you can't inspect an argument before
checking what parameter you're dealing with. This should be
under a case PIN_CONFIG_BIAS_PULL_UP in the switch (param)
below I think?

> +       case 1:

case 1000

> +               /* For 1k there is only pull up */
> +               if (param == PIN_CONFIG_BIAS_PULL_UP)
> +                       pull = CHV_PADCTRL0_TERM_1K << CHV_PADCTRL0_TERM_SHIFT;

Well you do check it here but...0

> +               break;
> +       case 5:

case 5000

> +               pull = CHV_PADCTRL0_TERM_5K << CHV_PADCTRL0_TERM_SHIFT;

This will be applied to whatever config with arg == 5000 comes here!

(...)
> +static unsigned chv_gpio_offset_to_pin(struct chv_pinctrl *pctrl,
> +                                      unsigned offset)
> +{
> +       return pctrl->community->pins[offset].number;
> +}

I'm a bit worried about the massive pin<->offsets<->gpio# translations
happening in this and patch 1/4 etc. It's a bit unsettling. Are you
sure we are translating in the simplest way?

> +static int chv_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> +       struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(chip);
> +       int pin = chv_gpio_offset_to_pin(pctrl, offset);
> +       unsigned long flags;
> +       u32 ctrl0, cfg;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +       ctrl0 = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> +       spin_unlock_irqrestore(&pctrl->lock, flags);

If you need a lock before and after reading every register in this
range, consider regmap-mmio, because that is part of what
it does. Just a hint...

(...)
> +static void chv_gpio_irq_mask_unmask(struct irq_data *d, bool mask)
> +{
> +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +       struct chv_pinctrl *pctrl = gpiochip_to_pinctrl(gc);
> +       int pin = chv_gpio_offset_to_pin(pctrl, irqd_to_hwirq(d));
> +       u32 value, intr_line;
> +       unsigned long flags;
> +
> +       spin_lock_irqsave(&pctrl->lock, flags);
> +
> +       intr_line = readl(chv_padreg(pctrl, pin, CHV_PADCTRL0));
> +       intr_line &= CHV_PADCTRL0_INTSEL_MASK;
> +       intr_line >>= CHV_PADCTRL0_INTSEL_SHIFT;
> +
> +       value = readl(pctrl->regs + CHV_INTMASK);
> +       if (mask)
> +               value &= ~(1 << intr_line);

I usually do this kind of stuff with

#include <linux/bitops.h>

value &= ~BIT(intr_line);

> +       else
> +               value |= (1 << intr_line);

value |= BIT(intr_line);

(probably a few more occasions in the driver)

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/