Re: [PATCH] pinctrl: pinmux: Don't free pins requested by other devices

[Stephen Warren <swarren@wwwdotorg.org>]

On 08/07/2013 10:23 AM, Linus Walleij wrote:
> On Wed, Jul 17, 2013 at 7:31 AM, Sonic Zhang <sonic.adi@gmail.com> wrote:
> 
> I'd like Stephen and Axel to have a look at this as well...
> 
>> From: Sonic Zhang <sonic.zhang@analog.com>
>>
>> in pinmux_disable_setting after current device fails to request
>> the same pins.
>>
>> Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
> 
> I don't quite understand the patch. Can you provide more context?

Yes, the commit description needs to describe the problem this solves.

I'm *guessing* the issue is:

Something tries to enable a new mux setting on some pins. One of those
pins is already owned by something else. So, applying the current
setting fails. So, pinctrl core attempts to unapply the partially
applied setting. This ends up incorrectly over-writing the conflicting
ownership of the pins with NULL, and hence forgetting about it.

I think a better change would be something more along the lines of:

  for (i = 0; i < num_pins; i++)
+ 	if (this_device_owns_pin(pins[i])
  		pin_free(pctldev, pins[i], NULL);

?

Where this_device_owns_pin() might be someting like:

	desc->owning_setting == setting

(which would be a new field that needed to be assigned during
pinmux_enable_setting).

Or perhaps the strcmp() is fine.

>> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
>> index 88cc509..9ebcf3b 100644
>> --- a/drivers/pinctrl/pinmux.c
>> +++ b/drivers/pinctrl/pinmux.c
>> @@ -482,13 +482,14 @@ void pinmux_disable_setting(struct pinctrl_setting const *setting)
>>                                  pins[i]);
>>                         continue;
>>                 }
>> +               /* And release the pins */
>> +               if (desc->mux_usecount &&
>> +                       !strcmp(desc->mux_owner, setting->dev_name))
>> +                       pin_free(pctldev, pins[i], NULL);
>> +
>>                 desc->mux_setting = NULL;
>>         }
>>
>> -       /* And release the pins */
>> -       for (i = 0; i < num_pins; i++)
>> -               pin_free(pctldev, pins[i], NULL);
>> -
> 
> For pinmux_disable_setting() to inspect desc->mux_usecount seems
> assymetric. This is something pin_free() should do, shouldn't it?
> 
> Should not this codepath be kept and a change made inside pin_free()
> for the check above instead?