On Fri, May 4, 2012 at 2:18 PM, John Crispin [off-list ref] 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 <redacted>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: Linus Walleij <redacted>
Cc: Stephen Warren <redacted>

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

Nitpicking below.

Shouldn't this be:
select PINCTRL
select PINCTRL_LANTIQ

?

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 :-)

depends on LANTIQ

?

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

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.

+#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.)

+/* 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