Messages in this thread | | | Date | Wed, 14 Dec 2011 19:00:35 +0100 | Subject | Re: [RFC PATCH v2 2/4] pinctrl: imx: add pinmux imx driver | From | Linus Walleij <> |
| |
On Wed, Dec 14, 2011 at 5:03 PM, Dong Aisheng <b29396@freescale.com> wrote:
> The driver contains the initial support for imx53 and > imx6q.
Cool! Comments below...
> --- /dev/null > +++ b/drivers/pinctrl/pinmux-imx-core.c
We should start naming these pinctrl-* rather than pinmux-* so just rename it.
Until here it looks like business as usual..
> +#ifdef CONFIG_OF > +static int __devinit imx_pmx_parse_functions(struct device_node *np, > + struct imx_pinctrl_info *info, u32 num) > +{ > + struct imx_pmx_func *function; > + struct imx_pin_group *group; > + int ret, len; > + > + dev_dbg(info->dev, "parse function %d\n", num); > + > + group = &info->groups[num]; > + function = &info->functions[num]; > + > + /* Initialise group */ > + ret = of_property_read_string(np, "grp_name", &group->name); > + if (ret) { > + dev_err(info->dev, "failed to get grp_name\n"); > + return ret; > + } > + > + ret = of_property_read_u32(np, "num_pins", &group->num_pins); > + if (ret) { > + dev_err(info->dev, "failed to get num_pins\n"); > + return ret; > + } > + > + ret = of_property_read_u32(np, "num_mux", &group->num_mux); > + if (ret) { > + dev_err(info->dev, "failed to get num_mux\n"); > + return ret; > + } > + > + if (group->num_pins != group->num_mux) > + return -EINVAL; > + > + group->pins = devm_kzalloc(info->dev, group->num_pins * sizeof(unsigned int), > + GFP_KERNEL); > + group->mux_mode = devm_kzalloc(info->dev, group->num_mux * sizeof(unsigned int), > + GFP_KERNEL); > + if (!group->pins || !group->mux_mode) > + return -ENOMEM; > + > + /* sanity check */ > + if (of_get_property(np, "grp_pins", &len) && > + len != group->num_pins * sizeof(unsigned int)) { > + dev_err(info->dev, "wrong pins number?\n"); > + return -EINVAL; > + } > + > + if (of_get_property(np, "grp_mux", &len) && > + len != group->num_mux * sizeof(unsigned int)) { > + dev_err(info->dev, "wrong pin mux number?\n"); > + return -EINVAL; > + } > + > + ret = of_property_read_u32_array(np, "grp_pins", > + group->pins, group->num_pins); > + if (ret) { > + dev_err(info->dev, "failed to get grp_pins\n"); > + return ret; > + } > + > + ret = of_property_read_u32_array(np, "grp_mux", > + group->mux_mode, group->num_mux); > + if (ret) { > + dev_err(info->dev, "failed to get grp_mux\n"); > + return ret; > + } > + > + /* Initialise function */ > + ret = of_property_read_string(np, "func_name", &function->name); > + if (ret) { > + dev_err(info->dev, "failed to get func_name\n"); > + return ret; > + } > + > + function->groups = devm_kzalloc(info->dev, sizeof(char **), GFP_KERNEL); > + function->num_groups = 1; > + function->groups[0] = group->name; > + > + dev_dbg(info->dev, "func_name %s grp_name %s num_groups %d\n", > + function->name, function->groups[0], > + function->num_groups); > + > + return 0; > +} > + > +static int __devinit imx_pmx_probe_dt(struct platform_device *pdev, > + struct imx_pinctrl_info *info) > +{ > + struct device_node *np = pdev->dev.of_node; > + struct device_node *child = NULL; > + int ret, i; > + u32 nfuncs = 0; > + > + if (!np) > + return -ENODEV; > + > + nfuncs = of_get_child_number(np); > + if (nfuncs <= 0) { > + dev_err(&pdev->dev, "no functions defined\n"); > + return -EINVAL; > + } > + > + info->nfunctions = nfuncs; > + info->functions = devm_kzalloc(&pdev->dev, nfuncs * sizeof(struct imx_pmx_func), > + GFP_KERNEL); > + if (!info->functions) > + return -ENOMEM; > + > + /* DT file only passes one group per one function */ > + info->ngroups = nfuncs; > + info->groups = devm_kzalloc(&pdev->dev, nfuncs * sizeof(struct imx_pin_group), > + GFP_KERNEL); > + if (!info->groups) > + return -ENOMEM; > + > + child = NULL; > + i = 0; > + for_each_child_of_node(np, child) { > + ret = imx_pmx_parse_functions(child, info, i++); > + if (ret) { > + dev_err(&pdev->dev, "failed to parse function\n"); > + return ret; > + } > + } > + > + return 0; > +} > +#else > +static int __devinit imx_pmx_probe_dt(struct platform_device *pdev, > + struct imx_pinctrl_info *info) > +{ > + return -ENODEV; > +} > +#endif
Looks clever to me! And I think we have agreed that this probing must be inside each driver.
Can we have a look at the corresponding device tree? (...)
> +++ b/drivers/pinctrl/pinmux-imx-core.h
Name this pinctrl-* too for consistency.
(...) > +#define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)
Now I start to think about the range generators I discussed with Haojian... but there doesn't seem to be very many ranges in this driver so I guess it would only confuse things.
> +#define IMX_PIN_GROUP(n, p, m) \ > + { \ > + .name = n, \ > + .pins = p, \ > + .num_pins = ARRAY_SIZE(p), \ > + .mux_mode = m, \ > + .num_mux = ARRAY_SIZE(m), \ > + } > + > +#define IMX_PMX_FUNC(n, g) \ > + { \ > + .name = n, \ > + .groups = g, \ > + .num_groups = ARRAY_SIZE(g), \ > + }
This looks pretty smart!
> +++ b/drivers/pinctrl/pinmux-imx53.c
Name this pinctrl-*
> diff --git a/drivers/pinctrl/pinmux-imx6q.c b/drivers/pinctrl/pinmux-imx6q.c
This too.
This patch is overall very good, we're quickly approaching merge quality.
Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |