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@d8110000 {
> compatible = "wm,wm8650-gpio";
> gpio-controller;
> reg = <0xd8110000 0x10000>;
> #gpio-cells = <3>;
> };
> +*/
> + pinmux: pinmux@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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
