lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 15 Dec 2011 10:10:08 +0800 From: Haojian Zhuang <haojian.zhuang@...il.com> To: Stephen Warren <swarren@...dia.com> Cc: Haojian Zhuang <haojian.zhuang@...vell.com>, "linus.walleij@...aro.org" <linus.walleij@...aro.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, "eric.y.miao@...il.com" <eric.y.miao@...il.com>, "linux@....linux.org.uk" <linux@....linux.org.uk>, "arnd@...db.de" <arnd@...db.de> Subject: Re: [PATCH V2 1/2] pinctrl: enable pinmux for pxa series On Wed, Dec 14, 2011 at 2:28 AM, Stephen Warren <swarren@...dia.com> wrote: > Haojian Zhuang wrote at Tuesday, December 13, 2011 2:41 AM: >> Support pxa3xx/pxa168/pxa910/mmp2. Support to switch pin configuration. > >> drivers/pinctrl/Kconfig | 15 + >> drivers/pinctrl/Makefile | 3 + >> drivers/pinctrl/pinctrl-pxa3xx.c | 193 +++++++++++ >> drivers/pinctrl/pinmux-pxa168.c | 170 ++++++++++ >> drivers/pinctrl/pinmux-pxa300.c | 647 ++++++++++++++++++++++++++++++++++++++ >> drivers/pinctrl/pinmux-pxa910.c | 373 ++++++++++++++++++++++ > > > >> +static int pxa3xx_pmx_request_gpio(struct pinctrl_dev *pctrldev, >> + struct pinctrl_gpio_range *range, >> + unsigned pin) >> +{ > ... >> + /* write gpio function into mfpr register */ >> + data = readl_relaxed(info->virt_base + mfpr) & MFPR_FUNC_MASK; > > FOO_MASK is usually the mask for the field, so you'd want to and with > ~MFPR_FUNC_MASK here. Still, I see that MFPR_FUNC_MASK==~MFPR_FUNC, so > the code is correct as written, albeit confusing. > > I'd suggest: > * Delete MFPR_FUNC_MASK define. > * Rename MFPR_FUNC to MFPR_FUNC_MASK. > * and with ~MFPR_FUNC_MASK instead of MFPR_FUNC_MASK here. > OK. >> +static int pxa3xx_pmx_enable(struct pinctrl_dev *pctrldev, unsigned func, >> + unsigned group) >> +{ >> + struct pxa3xx_pinmux_info *info = pinctrl_dev_get_drvdata(pctrldev); >> + struct pxa3xx_pin_group *pin_grp = &info->grp[group]; >> + unsigned int data, pin_func; >> + int i, mfpr; >> + >> + for (i = 0; i < pin_grp->num_pins; i++) { >> + mfpr = pxa3xx_get_mfpr(info, pin_grp->pins[i]); >> + if (mfpr < 0) { >> + dev_err(info->dev, "error pin:%d mfpr offset:%x\n", >> + pin_grp->pins[i], mfpr); >> + goto out; >> + } >> + pin_func = pin_grp->func[i]; >> + data = readl_relaxed(info->virt_base + mfpr); >> + data &= MFPR_FUNC_MASK; >> + data |= pin_func; > > There's an indentation problem here. > >> + writel_relaxed(data, info->virt_base + mfpr); >> + } >> + return 0; >> +out: >> + return -EINVAL; >> +} > > I don't see "func" used anywhere in this function. How does this code > know /which/ function to activate on the pins, or is there only a single > supported function for each pin? If so, I think you should at least > validate the "func" is the expected function for the pin. > > Related, if the HW supports configuring the mux function at a per-pin > Level (as appears to be the case, since you're looping over pins above), > I'd expect that each group definition in your driver to contain a single > pin, rather than an array of pins. The set of pins/groups/functions > exposed by your driver should represent the raw HW capabilities, and not > any logical grouping of pins into e.g. a whole UART or SD port. > There's multiple function on one pin. In my patch, I force to define multiple pins into one group with same function. It seems not good. I'll a groupe only on single pin. > ----------==========----------==========----------==========----------========== > Yes, looking at e.g. pinmux-pxa168.c, I believe you are creating > artificial groups, when you should be creating a group for each pin. > I wonder if the pinctrl core should grow the ability to synthesize a > group for each pin, in the case where the HW doesn't use groups. That > would remove the need for you to type out all those group definitions. > Alternatively, the mapping table lookups could first search for a group, > and if one isn't found, search for a pin of that name. Then, groups > would not be needed at all if the HW didn't use them. > >> diff --git a/drivers/pinctrl/pinmux-pxa168.c b/drivers/pinctrl/pinmux-pxa168.c > >> +static struct pinctrl_desc pxa168_pctrl_desc = { >> + .name = "pxa168-pinctrl", > ... >> +static struct platform_driver pxa168_pinmux_driver = { >> + .driver = { >> + .name = "pxa168-pinmux", > > Shouldn't those two names match? > >> diff --git a/drivers/pinctrl/pinmux-pxa300.c b/drivers/pinctrl/pinmux-pxa300.c > > This file appears to contain 4 sets of data without any commonality. > Shouldn't this be 4 separate files to keep the different CPUs separate? > you could still share probe() etc. by putting that in a common file, and > having that file call out to the per-CPU file to get a structure containing > the CPU-specific data. That way, I think you can probably share probe() > etc. across all 3 of the files you have now too, thus reducing the total > amount of code even if increasing the file count. > > See the Tegra patches I posted for example. > >> +++ b/drivers/pinctrl/pinmux-pxa300.c > ... >> + * linux/drivers/pinctrl/pinmux-pxa3xx.c > > The comment doesn't match the filename. > >> diff --git a/drivers/pinctrl/pinmux-pxa910.c b/drivers/pinctrl/pinmux-pxa910.c > > Same comment for this file; it appears to contain the data for two > different CPUs, and there's no commonality. > > -- > nvpublic > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@...r.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists