Re: [PATCH 3/4] pinctrl: add abx500 pinctrl driver core

[Stephen Warren <swarren@wwwdotorg.org>]

On 01/15/2013 02:43 AM, Linus Walleij wrote:
> From: Patrice Chotard <patrice.chotard@st.com>
> 
> This adds the AB8500 core driver, which will be utilized by
> the follow-on drivers for different ABx500 variants.
> Sselect the driver from the DBX500_SOC, as this chip is
> powering and clocking that SoC.

> diff --git a/drivers/pinctrl/pinctrl-abx500.c b/drivers/pinctrl/pinctrl-abx500.c

> +static int abx500_gpio_get(struct gpio_chip *chip, unsigned offset)

Shouldn't this call abx500_gpio_get_bit(), just like abx500_gpio_set()
calls abx500_gpio_set_bit()?

> +static int abx500_gpio_direction_output(struct gpio_chip *chip,
> +					unsigned offset,
> +					int val)
...
> +	/* disable pull down */
...
> +	/* if supported, disable both pull down and pull up */

Why the need to override those options?

> +static u8 abx500_get_mode(struct pinctrl_dev *pctldev, struct gpio_chip *chip,
> +		unsigned gpio)

> +	if (af.gpiosel_bit == UNUSED)
> +		return ABX500_DEFAULT;

That's odd; abx500_set_mode() seems to allow setting the mode to
something other than default even if (af.gpiosel_bit == UNUSED). Are
set_mode/get_mode actually correct inverses of each-other?

> +static int abx500_gpio_irq_init(struct abx500_pinctrl *pct)
...
> +#ifdef CONFIG_ARM
> +		set_irq_flags(irq, IRQF_VALID);
> +#else
> +		irq_set_noprobe(irq);
> +#endif

I assume that ifdef is always set one particular way?

> +static void abx500_gpio_irq_remove(struct abx500_pinctrl *pct)
...
> +#ifdef CONFIG_ARM
> +		set_irq_flags(irq, 0);
> +#endif

Same there.

> +static void abx500_pmx_disable(struct pinctrl_dev *pctldev,
> +			    unsigned function, unsigned group)
> +{
> +	struct abx500_pinctrl *pct = pinctrl_dev_get_drvdata(pctldev);
> +	const struct abx500_pingroup *g;
> +
> +	g = &pct->soc->groups[group];
> +	if (g->altsetting < 0)
> +		return;
> +
> +	/* Poke out the mux, set the pin to some default state? */
> +	dev_dbg(pct->dev, "disable group %s, %u pins\n", g->name, g->npins);
> +}

That looks basically unimplemented, and the comment seems like a FIXME?

> +int abx500_gpio_request_enable(struct pinctrl_dev *pctldev,
> +			    struct pinctrl_gpio_range *range,
> +			    unsigned offset)
...
> +	/*
> +	 * by default, for ABx5xx family, GPIO mode is selected by
> +	 * writing 1 in GPIOSELx registers
> +	 */
> +	ret = abx500_mask_and_set_register_interruptible(pct->dev,
> +		AB8500_MISC, reg, 1 << pos, 1 << pos);

It sounds like this should be implemented using abx500_set_mode()?

> +int abx500_pin_config_set(struct pinctrl_dev *pctldev,
> +		       unsigned pin,
> +		       unsigned long config)

> +	switch (param) {
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		/*
> +		 * if argument = 1 set the pull down
> +		 * else clear the pull down
> +		 */
> +		ret = abx500_gpio_direction_input(chip, offset);

That looks odd; why force the pin to be a GPIO just to enable a pull down?

> +		/* check if pin supports pull updown feature */
> +		if (pullud && pin >= pullud->first_pin	&& pin <= pullud->last_pin)
> +			ret = abx500_config_pull_updown(pct,
> +				offset,
> +				argument ? ABX500_GPIO_PULL_DOWN : ABX500_GPIO_PULL_NONE);
> +		else
> +			ret = abx500_gpio_set_bits(chip, AB8500_GPIO_PUD1_REG,
> +				offset, argument ? 0 : 1);

Hmm. Wouldn't it be better to remove the if statement, and just store
ABX500_GPIO_PULL_DOWN or 0, and ABX500_GPIO_PULL_NONE or 1, in the soc_data?

> +static int __devinit abx500_gpio_probe(struct platform_device *pdev)

> +	/* Poke in other ASIC variants here */
> +	switch (platid->driver_data) {
> +	case PINCTRL_AB8500:
> +		abx500_pinctrl_ab8500_init(&pct->soc);
> +		break;
> +	case PINCTRL_AB8540:
> +		abx500_pinctrl_ab8540_init(&pct->soc);
> +		break;
> +	case PINCTRL_AB9540:
> +		abx500_pinctrl_ab9540_init(&pct->soc);
> +		break;
> +	case PINCTRL_AB8505:
> +		abx500_pinctrl_ab8505_init(&pct->soc);
> +		break;
> +	default:
> +		dev_err(&pdev->dev, "Unsupported pinctrl sub driver (%d)\n",
> +				(int) platid->driver_data);
> +		return -EINVAL;
> +	}

Most of those functions don't exist yet. I see there are dummy inlines
below for them if the /config/ options aren't turned on, but what's to
stop somebody turning on the config option before the real
implementation exists?

In the past, Arnd requested that each variant had a separate top-level
driver object that called into a utility probe() function, rather than
having a probe() function that knew about all the SoC variants, and
dispatched out to a variant-specific function.

> +static int __devexit abx500_gpio_remove(struct platform_device *pdev)

> +	platform_set_drvdata(pdev, NULL);

There's no point doing that; nothing should touch the drvdata while the
device doesn't exist (or isn't probed rather).

> +	mutex_destroy(&pct->lock);
> +	kfree(pct);

That was allocated using devm_kzalloc(). There's no point freeing it
here, and if there were, devm_kfree() should be used, or a double-free
will occur.