linux-mips
[Top] [All Lists]

Re: [PATCH 04/14] OF: pinctrl: MIPS: lantiq: implement lantiq/xway pinct

To: John Crispin <blogic@openwrt.org>
Subject: Re: [PATCH 04/14] OF: pinctrl: MIPS: lantiq: implement lantiq/xway pinctrl support
From: Linus Walleij <linus.walleij@linaro.org>
Date: Tue, 8 May 2012 15:21:33 +0200
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org, devicetree-discuss@lists.ozlabs.org, Stephen Warren <swarren@wwwdotorg.org>
In-reply-to: <1336133919-26525-4-git-send-email-blogic@openwrt.org>
References: <1336133919-26525-1-git-send-email-blogic@openwrt.org> <1336133919-26525-4-git-send-email-blogic@openwrt.org>
Sender: linux-mips-bounce@linux-mips.org
On Fri, May 4, 2012 at 2:18 PM, John Crispin <blogic@openwrt.org> wrote:

> Implement support for pinctrl on lantiq/xway socs. The IO core found on these
> socs has the registers for pinctrl, pinconf and gpio mixed up in the same
> register range. As the gpio_chip handling is only a few lines, the driver also
> implements the gpio functionality. This obseletes the old gpio driver that was
> located in the arch/ folder.
>
> Signed-off-by: John Crispin <blogic@openwrt.org>
> Cc: devicetree-discuss@lists.ozlabs.org
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>

Overall this is looking very good and a positive development for these SoCs.

Nitpicking below.

> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -216,6 +216,7 @@ config MACH_JZ4740
>  config LANTIQ
>        bool "Lantiq based platforms"
>        select DMA_NONCOHERENT
> +       select PINCTRL

Shouldn't this be:
select PINCTRL
select PINCTRL_LANTIQ

?

> diff --git a/arch/mips/lantiq/Kconfig b/arch/mips/lantiq/Kconfig
> index 9485fe5..b86d942 100644
> --- a/arch/mips/lantiq/Kconfig
> +++ b/arch/mips/lantiq/Kconfig
> @@ -2,6 +2,7 @@ if LANTIQ
>
>  config SOC_TYPE_XWAY
>        bool
> +       select PINCTRL_XWAY
>        default n

OK...

> -obj-y := prom.o pmu.o ebu.o reset.o gpio.o gpio_stp.o gpio_ebu.o dma.o
> +obj-y := prom.o pmu.o ebu.o reset.o gpio_stp.o gpio_ebu.o dma.o

Yeah good riddance :-)

> diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
> index f73a5ea..a19bac96 100644
> --- a/drivers/pinctrl/Kconfig
> +++ b/drivers/pinctrl/Kconfig
> @@ -30,6 +30,11 @@ config PINCTRL_PXA3xx
>        bool
>        select PINMUX
>
> +config PINCTRL_LANTIQ
> +       bool
> +       select PINMUX
> +       select PINCONF

depends on LANTIQ

?

I don't think anyone else is going to want to compile
this.

> +
>  config PINCTRL_MMP2
>        bool "MMP2 pin controller driver"
>        depends on ARCH_MMP
> @@ -83,6 +88,10 @@ config PINCTRL_COH901
>          COH 901 335 and COH 901 571/3. They contain 3, 5 or 7
>          ports of 8 GPIO pins each.
>
> +config PINCTRL_XWAY
> +       bool
> +       select PINCTRL_LANTIQ

Shouldn't this be:

depends on SOC_TYPE_XWAY
depends on PINCTRL_LANTIQ

?

So LANTIQ selects it's pinctrl driver, the the xway SoC
selects its driver and they both are dependent on their
respective system.

> diff --git a/drivers/pinctrl/pinctrl-lantiq.h 
> b/drivers/pinctrl/pinctrl-lantiq.h

> +#define ARRAY_AND_SIZE(x)      (x), ARRAY_SIZE(x)

Hm if we redefine this I start to wonder if this
should go into <linux/kernel.h>

No big deal, we can alsway refactor later.

> +#define LTQ_PINCONF_PACK(_param_, _arg_)       ((_param_) << 16 | (_arg_))
> +#define LTQ_PINCONF_UNPACK_PARAM(_conf_)       ((_conf_) >> 16)
> +#define LTQ_PINCONF_UNPACK_ARG(_conf_)         ((_conf_) & 0xffff)

No need to add _underscores_ around the macro parameters?
I've not seen any coding convention requiring this.

> +struct ltq_pinmux_info {
> +       struct device *dev;
> +       struct pinctrl_dev *pctrl;
> +
> +       /* we need to manage up to 5 padcontrolers */

controllers

> +       void __iomem *membase[5];
> +
> +       /* the handler for the subsystem */
> +       struct pinctrl_desc *desc;

It's a descriptor not a handler.

> +       /* we expose our pads to the subsystem */
> +       struct pinctrl_pin_desc *pads;
> +
> +       /* the number of pads. this varies between socs */
> +       unsigned int num_gpio;

Why not call it num_pads, atleast use the same name for
the array and the count.

> +       /* these are our multifunction pins */
> +       const struct ltq_mfp_pin *mfp;
> +       unsigned int num_mfp;
> +
> +       /* a number of multifunction pins can be grouped together */
> +       const struct ltq_pin_group *grps;
> +       unsigned int num_grps;
> +
> +       /* a mapping between function string and id */
> +       const struct ltq_pmx_func *funcs;
> +       unsigned int num_funcs;
> +
> +       /* the pinconf options that we are able to read from the DT */
> +       const struct ltq_cfg_param *params;
> +       unsigned int num_params;
> +
> +       /* soc specific callback used to apply muxing */
> +       int (*apply_mux)(struct pinctrl_dev *pctrldev, int pin, int mux);
> +};
>
> +enum ltq_pin_list {
> +       GPIO0 = 0,
> +       GPIO1,

Wait, this enum is called something ending with _list but it's not
a list, it's a pin. The enum is like a type, so should define the basic
unit we're dealing with, not a collection of such units, so rename it
"ltq_pin" simply.

You could also consider using:
#define GPIO0 0
#define GPIO1 1

etc like some other drivers do. (Your pick.)

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

> +/* macros to help us access the registers */
> +#define gpio_getbit(m, r, p)   (!!(ltq_r32(m + r) & (1 << p)))
> +#define gpio_setbit(m, r, p)   ltq_w32_mask(0, (1 << p), m + r)
> +#define gpio_clearbit(m, r, p) ltq_w32_mask((1 << p), 0, m + r)

So what makes this arch so fantastic that it needs its own read/write functions?
(Just curious...)

You could replace (1 << p) with BIT(p) by using <linux/bitops.h>

> +       {"pci",         ARRAY_AND_SIZE(xrx_pci_grps)},
> +       {"ebu",         ARRAY_AND_SIZE(xrx_ebu_grps)},
> +};
> +
> +
> +
> +
> +
> +

You can never get enough whitespace :-)

Please trim it down...

> +/* ---------  pinconf related code --------- */
> +static int xway_pinconf_group_get(struct pinctrl_dev *pctldev,
> +                               unsigned group,
> +                               unsigned long *config)
> +{
> +       return -ENOTSUPP;
> +}
> +
> +static int xway_pinconf_group_set(struct pinctrl_dev *pctldev,
> +                               unsigned group,
> +                               unsigned long config)
> +{
> +       return -ENOTSUPP;
> +}

Just don't define these function pointers and leave callbacks as NULL.
The pinctrl core should handle this and if it doesn't, patch the core
in pinconf.c.

> +static void xway_pinconf_dbg_show(struct pinctrl_dev *pctldev,
> +                                       struct seq_file *s, unsigned offset)
> +{
> +}
> +
> +static void xway_pinconf_group_dbg_show(struct pinctrl_dev *pctldev,
> +                                       struct seq_file *s, unsigned selector)
> +{
> +}

Just don't define them if you don't use them, leave pointers as NULL.

These are however good for verbose pretty-printing the pins/groups
hardware state.

> +static inline int xway_mux_apply(struct pinctrl_dev *pctrldev,
> +                               int pin, int mux)
> +{
> +       struct ltq_pinmux_info *info = pinctrl_dev_get_drvdata(pctrldev);
> +       int port = PORT(pin);
> +
> +       if (mux & 0x1)
> +               gpio_setbit(info->membase[0], GPIO_ALT0(pin), PORT_PIN(pin));
> +       else
> +               gpio_clearbit(info->membase[0], GPIO_ALT0(pin), 
> PORT_PIN(pin));
> +
> +       if ((port == 3) && (mux & 0x2))
> +               gpio_setbit(info->membase[0], GPIO3_ALT1, PORT_PIN(pin));
> +       else if (mux & 0x2)
> +               gpio_setbit(info->membase[0], GPIO_ALT1(pin), PORT_PIN(pin));
> +       else if (port == 3)
> +               gpio_clearbit(info->membase[0], GPIO3_ALT1, PORT_PIN(pin));
> +       else
> +               gpio_clearbit(info->membase[0], GPIO_ALT1(pin), 
> PORT_PIN(pin));
> +
> +       return 0;
> +}

Please introduce some #defines for the magic numbers used above:

0x1, 3, 0x2, 3, etc so we can easily figure out what's going on.

> +static struct ltq_pinmux_info xway_info = {
> +       .mfp            = xway_mfp,
> +       .desc           = &xway_pctrl_desc,
> +       .apply_mux      = xway_mux_apply,
> +       .params         = xway_cfg_params,
> +       .num_params     = ARRAY_SIZE(xway_cfg_params),
> +};
> +
> +
> +
> +

Whitespacey...

> +/* ---------  gpio_chip related code --------- */
> +
> +int gpio_to_irq(unsigned int gpio)
> +{
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(gpio_to_irq);
> +
> +int irq_to_gpio(unsigned int gpio)
> +{
> +       return -EINVAL;
> +}
> +EXPORT_SYMBOL(irq_to_gpio);

Can't you just leave them undefined?

> +static struct gpio_chip xway_chip = {
> +       .label = "gpio-xway",
> +       .direction_input = xway_gpio_dir_in,
> +       .direction_output = xway_gpio_dir_out,
> +       .get = xway_gpio_get,
> +       .set = xway_gpio_set,
> +       .request = xway_gpio_req,
> +       .free = xway_gpio_free,
> +       .base = 0,
> +};
> +
> +
> +
> +

Whitespace.

Yours,
Linus Walleij

<Prev in Thread] Current Thread [Next in Thread>