On 22 July 2014 21:41, Linus Walleij <linus.walleij@xxxxxxxxxx> wrote:
> On Sun, Jul 13, 2014 at 8:31 AM, Mollie Wu <mollie.wu@xxxxxxxxxx> wrote:
>
>> +static int
>> +mb86s70_pmx_enable(struct pinctrl_dev *pctl,
>> + unsigned func_selector, unsigned group_selector)
>> +{
>> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
>> + unsigned long flags;
>> + const unsigned *pins;
>> + int i, j;
>> +
>> + if (group_selector >= grp_count) {
>> + pr_err("%s:%d\n", __func__, __LINE__);
>> + return -EINVAL;
>> + }
>> +
>> + j = mb86s7x_pgrps[group_selector].num_pins;
>> + pins = mb86s7x_pgrps[group_selector].pins;
>> +
>> + spin_lock_irqsave(&pchip->lock, flags);
>> +
>> + /* Busy if any pin in the same 'bunch' is taken */
>> + for (i = 0; i < j; i++) {
>> + u32 val;
>> + int p = pins[i] / 4 * 4;
>> +
>> + if (pins[i] >= PINS_VIRT) /* Not for virtual pins */
>> + continue;
>> +
>> + val = readl(pchip->base + p);
>> + /* skip if no change needed */
>> + if (val == mb86s7x_pmx_funcs[func_selector].prog_val)
>> + continue;
>> +
>> + if (pchip->pin_busy[p]) {
>> + pr_err("%s:%d:%d %d busy\n",
>> + __func__, __LINE__, pins[i], p);
>> + goto busy_exit;
>> + }
>> +
>> + if (pchip->pin_busy[p + 1]) {
>> + pr_err("%s:%d:%d %d busy\n",
>> + __func__, __LINE__, pins[i], p+1);
>> + goto busy_exit;
>> + }
>
> I don't see why you are doing this and keeping track of
> pins as "busy" or not. One thing the pin control core does
> is to make sure pins do not collide in different use cases,
> it seems like you're re-implementing this check again.
>
>> + if (p == 64)
>> + continue;
>
> This if-clause seems dubious. At least add a comment as
> to what is happening here and why.
>
>> + if (pchip->pin_busy[p + 2]) {
>> + pr_err("%s:%d:%d %d busy\n",
>> + __func__, __LINE__, pins[i], p+2);
>> + goto busy_exit;
>> + }
>> +
>> + if (pchip->pin_busy[p + 3]) {
>> + pr_err("%s:%d:%d %d busy\n",
>> + __func__, __LINE__, pins[i], p+3);
>> + goto busy_exit;
>> + }
>
> I don't understand this either.
>
> Why all this fuzz around the four pins from p ... p+3?
>
>> + continue;
>> +busy_exit:
>> + spin_unlock_irqrestore(&pchip->lock, flags);
>> + pr_err("%s:%d Take a look!\n", __func__, __LINE__);
>> + return -EBUSY;
>> + }
>
> You don't have to have the busy_exit: inside the for-loop right?
>
> Just push it below the return 0; in the end of this function
> so you can get rid of the dangling continue;
>
>> + pr_debug("Going to enable %s on pins -",
>> + mb86s7x_pmx_funcs[func_selector].name);
>> + for (i = 0; i < j; i++) {
>> + int p = pins[i];
>> +
>> + pr_debug(" %d", p);
>> + pchip->pin_busy[p] = true;
>
> So I'm questioning this....
>
>> + if (p < PINS_VIRT) /* Not for virtual pins */
>
> We need an explanation somewhere about what "virtual pins"
> means in this driver, I have never seen that before.
>
>> + writel(mb86s7x_pmx_funcs[func_selector].prog_val,
>> + pchip->base + p / 4 * 4);
>
> This may need some static inline like described above to simplify
> the code and make it more readable, but I see what is going on.
>
>> + }
>> +
>> + spin_unlock_irqrestore(&pchip->lock, flags);
>> + pr_debug("\n");
>> +
>> + return 0;
>> +}
>> +
>> +static void
>> +mb86s70_pmx_disable(struct pinctrl_dev *pctl,
>> + unsigned func_selector, unsigned group_selector)
>> +{
>> + struct mb86s70_pmux_chip *pchip = pinctrl_dev_get_drvdata(pctl);
>> + unsigned long flags;
>> + const unsigned *pins;
>> + int i, j;
>> +
>> + if (group_selector >= grp_count) {
>> + pr_err("%s:%d\n", __func__, __LINE__);
>> + return;
>> + }
>> +
>> + j = mb86s7x_pgrps[group_selector].num_pins;
>> + pins = mb86s7x_pgrps[group_selector].pins;
>> +
>> + pr_debug("Going to disable %s on pins -",
>> + mb86s7x_pmx_funcs[func_selector].name);
>> + spin_lock_irqsave(&pchip->lock, flags);
>> + for (i = 0; i < j; i++) {
>> + pr_debug(" %d", pins[i]);
>> + pchip->pin_busy[pins[i]] = false;
>> + }
>> + spin_unlock_irqrestore(&pchip->lock, flags);
>> + pr_debug("\n");
>> +}
>
> Remove this function. The .disable member is gone from
> struct pinmux_ops, as it was ambiguous, see commit
> 2243a87d90b42eb38bc281957df3e57c712b5e56
> "pinctrl: avoid duplicated calling enable_pinmux_setting for a pin"
>
Hmm... I just got bitten by this while updating the patchset. Sorry I
missed the patch review.
The reasons given in changelog are
(1) 'Fix' desc->mux_usecount
(2) The .disable callback is not useful for any of the existing platforms.
Well, for (2) I think it is only reasonable for the provider to need
to know when some resource is released by a user just as when it was
requested.
Even if the core ensures only one user is provided access to a pin,
the controller driver may still need to know when a pin is no more in
use. For ex, within consecutive 4 pins my controller can not enable a
pin for some function if any in the bunch is already
enabled/configured. So I need to know if the pin has been
disabled/released, so I can go ahead enabling the 'neighbor' pin for
some other role.
For reason (1), there should be some way to fix within the core?
Regards,
Jassi
--
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
