On Wed, Aug 07, 2013 at 09:25:35PM +0200, Linus Walleij wrote:

On Fri, Aug 2, 2013 at 12:38 PM, Markus Pargmann [off-list ref] wrote:

quoted

Signed-off-by: Markus Pargmann <redacted>

I think you need a bit of patch description here. Zero lines of
commit message is not acceptable for an entire new driver.
Elaborate a bit on the imx27 hardware and so on.

quoted

+/*
+ * Calculates the register offset from a pin_id
+ */
+static void __iomem *imx1_mem(struct imx1_pinctrl *ipctl, unsigned int pin_id)
+{
+       unsigned int port = pin_id / 32;
+       return ipctl->base + port * 0x100;

Use this:
#define IMX1_PORT_STRIDE 0x100

quoted

+/*
+ * Write to a register with 2 bits per pin. The function will automatically
+ * use the next register if the pin is managed in the second register.
+ */
+static void imx1_write_2bit(struct imx1_pinctrl *ipctl, unsigned int pin_id,
+               u32 value, u32 reg_offset)
+{
+       void __iomem *reg = imx1_mem(ipctl, pin_id) + reg_offset;
+       int shift = (pin_id % 16) * 2;
+       int mask = ~(0x3 << shift);

I think I understand this, but insert a comment on what this is
anyway.

quoted

+       u32 old_value;
+
+       dev_dbg(ipctl->dev, "write: register 0x%p shift %d value 0x%x\n",
+                       reg, shift, value);
+
+       if (pin_id % 32 >= 16)
+               reg += 0x04;

Comment on what is happening here.

quoted

+
+       value = (value & 0x11) << shift;

What is this? 0x11? Shall this be #defined or
what does it mean? The "value" argument really needs
some documentation I think.

You're modifying the argument "value" which is confusing.
Try to avoid this.

quoted

+       old_value = readl(reg) & mask;
+       writel(value | old_value, reg);

This is a bit over-complicating things. Write out
the sequence and let the compiler do the optimization.

val = readl(reg);
val &= mask;
val |= value;
writel(val, reg);

quoted

+static void imx1_write_bit(struct imx1_pinctrl *ipctl, unsigned int pin_id,
+               u32 value, u32 reg_offset)
+{
+       void __iomem *reg = imx1_mem(ipctl, pin_id) + reg_offset;
+       int shift = pin_id % 32;
+       int mask = ~(0x1 << shift);
+       u32 old_value;
+
+       dev_dbg(ipctl->dev, "write: register 0x%p shift %d value 0x%x\n",
+                       reg, shift, value);
+
+       value = (value & 0x1) << shift;
+       old_value = readl(reg) & mask;
+       writel(value | old_value, reg);

Same comments here.

quoted

+static int imx1_read_bit(struct imx1_pinctrl *ipctl, unsigned int pin_id,
+               u32 reg_offset)
+{
+       void __iomem *reg = imx1_mem(ipctl, pin_id) + reg_offset;
+       int shift = pin_id % 32;
+
+       return (readl(reg) >> shift) & 0x1;

Hard to read. Can't you just do this?

#include <linux/bitops.h>

u32 offset = pin_id % 32;

return !!(readl(reg) & BIT(offset));

quoted

+static void imx1_pin_dbg_show(struct pinctrl_dev *pctldev, struct seq_file *s,
+                  unsigned offset)
+{
+       seq_printf(s, "%s", dev_name(pctldev->dev));
+}

Don't you want to print some other more interesting things about
each pin?

This template is really just an example. The debugfs file is
intended to be helpful.

quoted

+static int imx1_pmx_enable(struct pinctrl_dev *pctldev, unsigned selector,
+                          unsigned group)
+{
+       struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+       const struct imx_pinctrl_soc_info *info = ipctl->info;
+       const unsigned *pins, *mux;
+       unsigned int npins;
+       int i;
+
+       /*
+        * Configure the mux mode for each pin in the group for a specific
+        * function.
+        */
+       pins = info->groups[group].pins;
+       npins = info->groups[group].npins;
+       mux = info->groups[group].mux_mode;
+
+       WARN_ON(!pins || !npins || !mux);
+
+       dev_dbg(ipctl->dev, "enable function %s group %s\n",
+               info->functions[selector].name, info->groups[group].name);
+
+       for (i = 0; i < npins; i++) {
+               unsigned int pin_id = pins[i];
+               unsigned int afunction = 0x001 & mux[i];
+               unsigned int gpio_in_use = (0x002 & mux[i]) >> 1;
+               unsigned int direction = (0x004 & mux[i]) >> 2;
+               unsigned int gpio_oconf = (0x030 & mux[i]) >> 4;
+               unsigned int gpio_iconfa = (0x300 & mux[i]) >> 8;
+               unsigned int gpio_iconfb = (0xc00 & mux[i]) >> 10;

If you use <linux/bitops.h> this can be made more understandable,
BIT(0), BIT(1), BIT(2) etc.

The shift offsets should be #defined.

quoted

+static void imx1_pinconf_dbg_show(struct pinctrl_dev *pctldev,
+                                  struct seq_file *s, unsigned pin_id)
+{
+       struct imx1_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
+       const struct imx_pinctrl_soc_info *info = ipctl->info;
+       const struct imx_pin_reg *pin_reg = &info->pin_regs[pin_id];
+       unsigned long config;
+
+       if (!pin_reg || !pin_reg->conf_reg) {
+               seq_puts(s, "N/A");
+               return;
+       }
+
+       config = readl(ipctl->base + pin_reg->conf_reg);
+       seq_printf(s, "0x%lx", config);
+}

That's pretty helpful, nice!

quoted

+static int imx1_pinctrl_parse_gpio(struct platform_device *pdev,
+               struct imx1_pinctrl *pctl, struct device_node *np, int i,
+               u32 base)
+{
+       int ret;
+       u32 memoffset;
+
+       ret = of_property_read_u32(np, "reg", &memoffset);
+       if (ret)
+               return ret;
+
+       memoffset -= base;
+       pctl->gpio_pdata[i].base = pctl->base + memoffset;
+
+       pctl->gpio_dev[i] = of_device_alloc(np, NULL, &pdev->dev);
+       pctl->gpio_dev[i]->dev.platform_data = &pctl->gpio_pdata[i];
+       pctl->gpio_dev[i]->dev.bus = &platform_bus_type;
+
+       ret = of_device_add(pctl->gpio_dev[i]);
+       if (ret) {
+               dev_err(&pdev->dev,
+                               "Failed to add child gpio device\n");
+               return ret;
+       }
+       return 0;
+}

As mentioned for the other patch I think this is the wrong approach.
Try to make the GPIO driver wholly independent.

Thanks for all your comments, I tried to fix everything you mentioned.

Regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |