On 22 August 2014 13:16, Jassi Brar <jaswinder.singh@xxxxxxxxxx> wrote:
> 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?
>
Polite ping...
I can't find a way around not knowing when a pin is let free. Do we
revert the commit and fix the situation for the author's platform
instead?
Thanks
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
