lkml.org 
[lkml]   [2013]   [Jan]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 3/4] pinctrl: add abx500 pinctrl driver core
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.


\
 
 \ /
  Last update: 2013-01-15 20:22    [W:0.593 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site