Re: [PATCH V2 1/2] pinctrl: enable pinmux for pxa series

[Haojian Zhuang]

On Wed, Dec 14, 2011 at 2:28 AM, Stephen Warren <swarren@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/