Messages in this thread | | | Date | Tue, 15 Jan 2013 11:36:39 -0700 | From | Stephen Warren <> | Subject | Re: [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.
| |