From: swarren@wwwdotorg•org (Stephen Warren)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] pinctrl: gpio: vt8500: Add pin control driver for Wondermedia SoCs
Date: Wed, 27 Feb 2013 15:21:40 -0700 [thread overview]
Message-ID: <512E86F4.6000909@wwwdotorg.org> (raw)
In-Reply-To: <1360896534-20637-2-git-send-email-linux@prisktech.co.nz>
On 02/14/2013 07:48 PM, Tony Prisk wrote:
Sorry for the slow review.
No patch description?
> arch/arm/Kconfig | 4 +-
> arch/arm/boot/dts/wm8850-w70v2.dts | 15 +
> arch/arm/boot/dts/wm8850.dtsi | 7 +-
> arch/arm/mach-vt8500/Kconfig | 1 +
> drivers/pinctrl/Kconfig | 10 +
> drivers/pinctrl/Makefile | 2 +
> drivers/pinctrl/pinctrl-wm8850.c | 166 +++++++++++
> drivers/pinctrl/pinctrl-wmt.c | 565 ++++++++++++++++++++++++++++++++++++
> drivers/pinctrl/pinctrl-wmt.h | 73 +++++
No DT binding documentation?
It doesnt' seem a good idea to add the pinctrl driver and touch the
various DT files in a single patch.
> diff --git a/arch/arm/boot/dts/wm8850.dtsi b/arch/arm/boot/dts/wm8850.dtsi
> +/*
> gpio: gpio-controller at d8110000 {
> compatible = "wm,wm8650-gpio";
> gpio-controller;
> reg = <0xd8110000 0x10000>;
> #gpio-cells = <3>;
> };
> +*/
> + pinmux: pinmux at d8110000 {
> + compatible = "wm,wm8850-gpio";
> + reg = <0xd8110000 0x10000>;
> + };
If the old code is wrong, why not delete it? but the main change is just
removing the GPIO-related properties - is the new driver no longer a
GPIO provider, why?
> diff --git a/arch/arm/mach-vt8500/Kconfig b/arch/arm/mach-vt8500/Kconfig
> @@ -29,5 +29,6 @@ config ARCH_WM8850
> depends on ARCH_MULTI_V7
> select ARCH_VT8500
> select CPU_V7
> + select PINCTRL
Don't you want to select the specific pinctrl driver here too; is it
actually useful for it to be optional?
> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> +config PINCTRL_WM8850
> + bool "Wondermedia WM8850 pin controller driver"
> + depends on ARCH_VT8500
> + select PINCTRL_WMT
If this is user-visible, there should be a description.
> diff --git a/drivers/pinctrl/pinctrl-wm8850.c b/drivers/pinctrl/pinctrl-wm8850.c
> +/* Please keep sorted by bank/bit */
> +#define WMT_PIN_EXTGPIO0 WMT_PIN(0, 0)
> +#define WMT_PIN_EXTGPIO1 WMT_PIN(0, 1)
...
> +#define WMT_PIN_I2C2_SCL WMT_PIN(5, 12)
> +#define WMT_PIN_I2C2_SDA WMT_PIN(5, 13)
There are a lot of gaps in that list. Does the HW really not support pin
muxing on the rest of the bits in the registers?
> diff --git a/drivers/pinctrl/pinctrl-wmt.c b/drivers/pinctrl/pinctrl-wmt.c
> +static int wmt_pmx_gpio_set_direction(struct pinctrl_dev *pctldev,
> + struct pinctrl_gpio_range *range,
> + unsigned offset,
> + bool input)
> +{
> + struct wmt_pinctrl_data *data = pinctrl_dev_get_drvdata(pctldev);
> +#include <linux/pinctrl/consumer.h>
That should be at the top of the file.
> + wmt_set_pinmux(data, (input ? WMT_FSEL_GPIO_IN : WMT_FSEL_GPIO_OUT),
> + offset);
Nit: The inner brackets are redundant.
> +static int wmt_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> + struct wmt_pinctrl_data *data = pinctrl_dev_get_drvdata(pctldev);
> +
> + return data->ngroups;
> +}
> +
> +static const char *wmt_get_group_name(struct pinctrl_dev *pctldev,
> + unsigned selector)
> +{
> + struct wmt_pinctrl_data *data = pinctrl_dev_get_drvdata(pctldev);
> +
> + return data->groups[selector];
> +}
Hmmm. Given there's a 1:1 mapping between pin names and group names, I
wonder if this core driver shouldn't synthesize the array of group names
from the array of pins instead of requiring the per-SoC driver to
duplicate all the pin names in a group array.
Of course, we should probably fix the pinctrl core to allow pin muxing
on pins in addition to groups too. I keep feeling guilty about not
having done that before.
> +static int wmt_pctl_dt_node_to_map_pull(struct wmt_pinctrl_data *data,
...
> + struct pinctrl_map *map = *maps;
> +
> +
> + if (pull > 2) {
Nit: redundant blank line there.
> + configs[0] = 0;
= pull;?
> +static struct gpio_chip wmt_gpio_chip = {
...
> + .can_sleep = 0,
Nit: No need to set that; 0 is the default for any
not-explicitly-initialized fields.
> +int wmt_pinctrl_probe(struct platform_device *pdev,
...
> + dev_info(&pdev->dev, "Pin controller initialized\n");
Nit: I'm never really sure that's useful.
next prev parent reply other threads:[~2013-02-27 22:21 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-02-15 2:48 [RFC PATCH] Add pin control driver for Wondermedia SoCS Tony Prisk
2013-02-15 2:48 ` [PATCH] pinctrl: gpio: vt8500: Add pin control driver for Wondermedia SoCs Tony Prisk
2013-02-15 20:27 ` Linus Walleij
2013-02-15 22:26 ` Tony Prisk
2013-02-27 22:21 ` Stephen Warren [this message]
2013-02-28 6:25 ` Tony Prisk
2013-03-01 18:24 ` Stephen Warren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=512E86F4.6000909@wwwdotorg.org \
--to=swarren@wwwdotorg$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox