| 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
| ||
|
Message-ID: <CAN1soZxKMcFniW7wbOA6Bb2ZwNsAmypZmwmo1KdEhJBbx21wdA@mail.gmail.com>
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