On Fri, Aug 2, 2013 at 12:38 PM, Markus Pargmann <mpa@xxxxxxxxxxxxxx> wrote:
> Signed-off-by: Markus Pargmann <mpa@xxxxxxxxxxxxxx>
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.
> +/*
> + * 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
> +/*
> + * 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.
> + 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.
> +
> + 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.
> + 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);
> +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.
> +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));
> +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.
> +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.
> +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!
> +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.
Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
