Re: [PATCH 5/8] pinctrl: add driver for MB86S7x

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux