[1/2] net: can: c_can: Fix default pinmux glitch at init
diff mbox

Message ID 1436279277-3386-2-git-send-email-rogerq@ti.com
State New
Headers show

Commit Message

Roger Quadros July 7, 2015, 2:27 p.m. UTC
From: "J.D. Schroeder" <jay.schroeder@garmin.com>

The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
is down) causes a slight glitch on the pinctrl settings when used. Since
commit ab78029 (drivers/pinctrl: grab default handles from device core),
the device core will automatically set the default pins. This causes
the pins to be momentarily set to the default and then to the sleep
state in register_c_can_dev(). By adding an optional "enable" state,
boards can set the default pin state to be disabled and avoid the
glitch when the switch from default to sleep first occurs. If the
"enable" state is not available c_can_pinctrl_select_state() falls
back to using the "default" pinctrl state.

[Roger Q] - Forward port to v4.2

Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

Comments

Marc Kleine-Budde July 7, 2015, 2:33 p.m. UTC | #1
On 07/07/2015 04:27 PM, Roger Quadros wrote:
> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
> 
> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
> is down) causes a slight glitch on the pinctrl settings when used. Since
> commit ab78029 (drivers/pinctrl: grab default handles from device core),
> the device core will automatically set the default pins. This causes
> the pins to be momentarily set to the default and then to the sleep
> state in register_c_can_dev(). By adding an optional "enable" state,
> boards can set the default pin state to be disabled and avoid the
> glitch when the switch from default to sleep first occurs. If the
> "enable" state is not available c_can_pinctrl_select_state() falls
> back to using the "default" pinctrl state.
> 
> [Roger Q] - Forward port to v4.2
> 
> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>  drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 041525d..66e98e7 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>  	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>  
>  	/* activate pins */
> -	pinctrl_pm_select_default_state(dev->dev.parent);
> +#ifdef CONFIG_PINCTRL

Please remove the ifdef, AFAICS there are static inline noop functions
if CONFIG_PINCTRL switched off.

> +	if (priv->device->pins) {
> +		struct pinctrl_state *s;
> +
> +		/* Attempt to use "active" if available else use "default" */
> +		s = pinctrl_lookup_state(priv->device->pins->p, "active");
> +		if (!IS_ERR(s))
> +			pinctrl_select_state(priv->device->pins->p, s);
> +		else
> +			pinctrl_pm_select_default_state(dev->dev.parent);
> +	}
> +#endif
>  	return 0;
>  }
>  
> 

Marc
Roger Quadros July 7, 2015, 2:35 p.m. UTC | #2
On 07/07/15 17:33, Marc Kleine-Budde wrote:
> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>
>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when CAN interface
>> is down) causes a slight glitch on the pinctrl settings when used. Since
>> commit ab78029 (drivers/pinctrl: grab default handles from device core),
>> the device core will automatically set the default pins. This causes
>> the pins to be momentarily set to the default and then to the sleep
>> state in register_c_can_dev(). By adding an optional "enable" state,
>> boards can set the default pin state to be disabled and avoid the
>> glitch when the switch from default to sleep first occurs. If the
>> "enable" state is not available c_can_pinctrl_select_state() falls
>> back to using the "default" pinctrl state.
>>
>> [Roger Q] - Forward port to v4.2
>>
>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> index 041525d..66e98e7 100644
>> --- a/drivers/net/can/c_can/c_can.c
>> +++ b/drivers/net/can/c_can/c_can.c
>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>   	priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>
>>   	/* activate pins */
>> -	pinctrl_pm_select_default_state(dev->dev.parent);
>> +#ifdef CONFIG_PINCTRL
>
> Please remove the ifdef, AFAICS there are static inline noop functions
> if CONFIG_PINCTRL switched off.

yes, you are right.

cheers,
-roger

>
>> +	if (priv->device->pins) {
>> +		struct pinctrl_state *s;
>> +
>> +		/* Attempt to use "active" if available else use "default" */
>> +		s = pinctrl_lookup_state(priv->device->pins->p, "active");
>> +		if (!IS_ERR(s))
>> +			pinctrl_select_state(priv->device->pins->p, s);
>> +		else
>> +			pinctrl_pm_select_default_state(dev->dev.parent);
>> +	}
>> +#endif
>>   	return 0;
>>   }
>>
>>
>
> Marc
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros July 7, 2015, 2:37 p.m. UTC | #3
On 07/07/15 17:35, Roger Quadros wrote:
> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>
>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>> CAN interface
>>> is down) causes a slight glitch on the pinctrl settings when used. Since
>>> commit ab78029 (drivers/pinctrl: grab default handles from device core),
>>> the device core will automatically set the default pins. This causes
>>> the pins to be momentarily set to the default and then to the sleep
>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>> boards can set the default pin state to be disabled and avoid the
>>> glitch when the switch from default to sleep first occurs. If the
>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>> back to using the "default" pinctrl state.
>>>
>>> [Roger Q] - Forward port to v4.2
>>>
>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/can/c_can/c_can.c
>>> b/drivers/net/can/c_can/c_can.c
>>> index 041525d..66e98e7 100644
>>> --- a/drivers/net/can/c_can/c_can.c
>>> +++ b/drivers/net/can/c_can/c_can.c
>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>
>>>       /* activate pins */
>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>> +#ifdef CONFIG_PINCTRL
>>
>> Please remove the ifdef, AFAICS there are static inline noop functions
>> if CONFIG_PINCTRL switched off.
>
> yes, you are right.

On second thoughts

device->pins are not defined if CONFIG_PINCTRL is not set.
so we can't remove the #ifdef.

cheers,
-roger

>
>>
>>> +    if (priv->device->pins) {
>>> +        struct pinctrl_state *s;
>>> +
>>> +        /* Attempt to use "active" if available else use "default" */
>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>> +        if (!IS_ERR(s))
>>> +            pinctrl_select_state(priv->device->pins->p, s);
>>> +        else
>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
>>> +    }
>>> +#endif
>>>       return 0;
>>>   }
>>>
>>>
>>
>> Marc
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde July 7, 2015, 2:43 p.m. UTC | #4
On 07/07/2015 04:37 PM, Roger Quadros wrote:
>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>> b/drivers/net/can/c_can/c_can.c
>>>> index 041525d..66e98e7 100644
>>>> --- a/drivers/net/can/c_can/c_can.c
>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>
>>>>       /* activate pins */
>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>> +#ifdef CONFIG_PINCTRL
>>>
>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>> if CONFIG_PINCTRL switched off.
>>
>> yes, you are right.
> 
> On second thoughts
> 
> device->pins are not defined if CONFIG_PINCTRL is not set.
> so we can't remove the #ifdef.

Too bad :(

okay - should I add stable@v.k.o on Cc?

Marc
Grygorii Strashko July 7, 2015, 3:49 p.m. UTC | #5
On 07/07/2015 05:37 PM, Roger Quadros wrote:
> On 07/07/15 17:35, Roger Quadros wrote:
>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>
>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>> CAN interface
>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>> Since
>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>> core),
>>>> the device core will automatically set the default pins. This causes
>>>> the pins to be momentarily set to the default and then to the sleep
>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>> boards can set the default pin state to be disabled and avoid the
>>>> glitch when the switch from default to sleep first occurs. If the
>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>> back to using the "default" pinctrl state.
>>>>
>>>> [Roger Q] - Forward port to v4.2
>>>>
>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>> b/drivers/net/can/c_can/c_can.c
>>>> index 041525d..66e98e7 100644
>>>> --- a/drivers/net/can/c_can/c_can.c
>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>
>>>>       /* activate pins */
>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>> +#ifdef CONFIG_PINCTRL
>>>
>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>> if CONFIG_PINCTRL switched off.
>>
>> yes, you are right.
>
> On second thoughts
>
> device->pins are not defined if CONFIG_PINCTRL is not set.
> so we can't remove the #ifdef.

May be you can use [devm_]pinctrl_get?

>>>
>>>> +    if (priv->device->pins) {
>>>> +        struct pinctrl_state *s;
>>>> +
>>>> +        /* Attempt to use "active" if available else use "default" */
>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>> +        if (!IS_ERR(s))
>>>> +            pinctrl_select_state(priv->device->pins->p, s);
>>>> +        else
>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
>>>> +    }
>>>> +#endif
>>>>       return 0;
>>>>   }
>>>>
Roger Quadros July 8, 2015, 8:09 a.m. UTC | #6
Marc,

On 07/07/15 17:43, Marc Kleine-Budde wrote:
> On 07/07/2015 04:37 PM, Roger Quadros wrote:
>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>> b/drivers/net/can/c_can/c_can.c
>>>>> index 041525d..66e98e7 100644
>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>        priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>
>>>>>        /* activate pins */
>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>> +#ifdef CONFIG_PINCTRL
>>>>
>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>> if CONFIG_PINCTRL switched off.
>>>
>>> yes, you are right.
>>
>> On second thoughts
>>
>> device->pins are not defined if CONFIG_PINCTRL is not set.
>> so we can't remove the #ifdef.
>
> Too bad :(
>
> okay - should I add stable@v.k.o on Cc?

I'm not sure if it would help. This patch by itself won't fix anything. 
It needs to go in together with the DTS change in patch 2.

Maybe if Tony can Ack the second patch then both should be applicable
on v4.0+ for stable.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Roger Quadros July 8, 2015, 8:13 a.m. UTC | #7
On 07/07/15 18:49, Grygorii Strashko wrote:
> On 07/07/2015 05:37 PM, Roger Quadros wrote:
>> On 07/07/15 17:35, Roger Quadros wrote:
>>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>>
>>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>>> CAN interface
>>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>>> Since
>>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>>> core),
>>>>> the device core will automatically set the default pins. This causes
>>>>> the pins to be momentarily set to the default and then to the sleep
>>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>>> boards can set the default pin state to be disabled and avoid the
>>>>> glitch when the switch from default to sleep first occurs. If the
>>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>>> back to using the "default" pinctrl state.
>>>>>
>>>>> [Roger Q] - Forward port to v4.2
>>>>>
>>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> ---
>>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>> b/drivers/net/can/c_can/c_can.c
>>>>> index 041525d..66e98e7 100644
>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>
>>>>>       /* activate pins */
>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>> +#ifdef CONFIG_PINCTRL
>>>>
>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>> if CONFIG_PINCTRL switched off.
>>>
>>> yes, you are right.
>>
>> On second thoughts
>>
>> device->pins are not defined if CONFIG_PINCTRL is not set.
>> so we can't remove the #ifdef.
>
> May be you can use [devm_]pinctrl_get?

Why should we do that? The device core has already done the 
pinctrl_get() and it is available in device->pins->p.

cheers,
-roger

>
>>>>
>>>>> +    if (priv->device->pins) {
>>>>> +        struct pinctrl_state *s;
>>>>> +
>>>>> +        /* Attempt to use "active" if available else use "default" */
>>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>>> +        if (!IS_ERR(s))
>>>>> +            pinctrl_select_state(priv->device->pins->p, s);
>>>>> +        else
>>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
>>>>> +    }
>>>>> +#endif
>>>>>       return 0;
>>>>>   }
>>>>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Kleine-Budde July 8, 2015, 8:17 a.m. UTC | #8
On 07/08/2015 10:09 AM, Roger Quadros wrote:

>>> device->pins are not defined if CONFIG_PINCTRL is not set.
>>> so we can't remove the #ifdef.
>>
>> Too bad :(
>>
>> okay - should I add stable@v.k.o on Cc?
> 
> I'm not sure if it would help. This patch by itself won't fix anything. 
> It needs to go in together with the DTS change in patch 2.

Of course.

> Maybe if Tony can Ack the second patch then both should be applicable
> on v4.0+ for stable.

Marc
Grygorii Strashko July 8, 2015, 10:31 a.m. UTC | #9
On 07/08/2015 11:13 AM, Roger Quadros wrote:
> 
> On 07/07/15 18:49, Grygorii Strashko wrote:
>> On 07/07/2015 05:37 PM, Roger Quadros wrote:
>>> On 07/07/15 17:35, Roger Quadros wrote:
>>>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>>>
>>>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>>>> CAN interface
>>>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>>>> Since
>>>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>>>> core),
>>>>>> the device core will automatically set the default pins. This causes
>>>>>> the pins to be momentarily set to the default and then to the sleep
>>>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>>>> boards can set the default pin state to be disabled and avoid the
>>>>>> glitch when the switch from default to sleep first occurs. If the
>>>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>>>> back to using the "default" pinctrl state.
>>>>>>
>>>>>> [Roger Q] - Forward port to v4.2
>>>>>>
>>>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>> ---
>>>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>>> b/drivers/net/can/c_can/c_can.c
>>>>>> index 041525d..66e98e7 100644
>>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>
>>>>>>       /* activate pins */
>>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>>> +#ifdef CONFIG_PINCTRL
>>>>>
>>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>>> if CONFIG_PINCTRL switched off.
>>>>
>>>> yes, you are right.
>>>
>>> On second thoughts
>>>
>>> device->pins are not defined if CONFIG_PINCTRL is not set.
>>> so we can't remove the #ifdef.
>>
>> May be you can use [devm_]pinctrl_get?
> 
> Why should we do that? The device core has already done the 
> pinctrl_get() and it is available in device->pins->p.

True. But It seems, you are going to be only one direct user of pin->p in whole
kernel (outside of pictrl core) !? ;)

pinctrl_get() will just return a pointer on pinctrl associated with dev
if called not the first time and increment kbobj use-counter.

Also, there is nice API pinctrl_get_select() and example in kernel 2c7c2c1d.

So, according to the documentation and pinctrl code, below sequence should work:

struct pinctrl *p;
p = pinctrl_get_select(priv->device, "active");

if (!IS_ERR(p))
   pinctrl_put(p);
else
   pinctrl_pm_select_default_state(priv->device);




>>>>>
>>>>>> +    if (priv->device->pins) {
>>>>>> +        struct pinctrl_state *s;
>>>>>> +
>>>>>> +        /* Attempt to use "active" if available else use 
>>>>>> "default" */
>>>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>>>> +        if (!IS_ERR(s))
>>>>>> +            pinctrl_select_state(priv->device->pins->p, s);

 ^^ different devices passed here

>>>>>> +        else
>>>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);

^^ and here ? Is it ok?

>>>>>> +    }
>>>>>> +#endif
>>>>>>       return 0;
>>>>>>   }
>>>>>>
>>
Roger Quadros July 8, 2015, 10:44 a.m. UTC | #10
On 08/07/15 13:31, Grygorii Strashko wrote:
> On 07/08/2015 11:13 AM, Roger Quadros wrote:
>>
>> On 07/07/15 18:49, Grygorii Strashko wrote:
>>> On 07/07/2015 05:37 PM, Roger Quadros wrote:
>>>> On 07/07/15 17:35, Roger Quadros wrote:
>>>>> On 07/07/15 17:33, Marc Kleine-Budde wrote:
>>>>>> On 07/07/2015 04:27 PM, Roger Quadros wrote:
>>>>>>> From: "J.D. Schroeder" <jay.schroeder@garmin.com>
>>>>>>>
>>>>>>> The previous change 3973c526ae9c (net: can: c_can: Disable pins when
>>>>>>> CAN interface
>>>>>>> is down) causes a slight glitch on the pinctrl settings when used.
>>>>>>> Since
>>>>>>> commit ab78029 (drivers/pinctrl: grab default handles from device
>>>>>>> core),
>>>>>>> the device core will automatically set the default pins. This causes
>>>>>>> the pins to be momentarily set to the default and then to the sleep
>>>>>>> state in register_c_can_dev(). By adding an optional "enable" state,
>>>>>>> boards can set the default pin state to be disabled and avoid the
>>>>>>> glitch when the switch from default to sleep first occurs. If the
>>>>>>> "enable" state is not available c_can_pinctrl_select_state() falls
>>>>>>> back to using the "default" pinctrl state.
>>>>>>>
>>>>>>> [Roger Q] - Forward port to v4.2
>>>>>>>
>>>>>>> Signed-off-by: J.D. Schroeder <jay.schroeder@garmin.com>
>>>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>>>> ---
>>>>>>>   drivers/net/can/c_can/c_can.c | 13 ++++++++++++-
>>>>>>>   1 file changed, 12 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/can/c_can/c_can.c
>>>>>>> b/drivers/net/can/c_can/c_can.c
>>>>>>> index 041525d..66e98e7 100644
>>>>>>> --- a/drivers/net/can/c_can/c_can.c
>>>>>>> +++ b/drivers/net/can/c_can/c_can.c
>>>>>>> @@ -605,7 +605,18 @@ static int c_can_start(struct net_device *dev)
>>>>>>>       priv->can.state = CAN_STATE_ERROR_ACTIVE;
>>>>>>>
>>>>>>>       /* activate pins */
>>>>>>> -    pinctrl_pm_select_default_state(dev->dev.parent);
>>>>>>> +#ifdef CONFIG_PINCTRL
>>>>>>
>>>>>> Please remove the ifdef, AFAICS there are static inline noop functions
>>>>>> if CONFIG_PINCTRL switched off.
>>>>>
>>>>> yes, you are right.
>>>>
>>>> On second thoughts
>>>>
>>>> device->pins are not defined if CONFIG_PINCTRL is not set.
>>>> so we can't remove the #ifdef.
>>>
>>> May be you can use [devm_]pinctrl_get?
>>
>> Why should we do that? The device core has already done the 
>> pinctrl_get() and it is available in device->pins->p.
> 
> True. But It seems, you are going to be only one direct user of pin->p in whole
> kernel (outside of pictrl core) !? ;)

good point :).

> 
> pinctrl_get() will just return a pointer on pinctrl associated with dev
> if called not the first time and increment kbobj use-counter.
> 
> Also, there is nice API pinctrl_get_select() and example in kernel 2c7c2c1d.
> 
> So, according to the documentation and pinctrl code, below sequence should work:
> 
> struct pinctrl *p;
> p = pinctrl_get_select(priv->device, "active");
> 
> if (!IS_ERR(p))
>    pinctrl_put(p);
> else
>    pinctrl_pm_select_default_state(priv->device);

This is much neater. I will give that a try. Thanks.

cheers,
-roger

> 
> 
> 
> 
>>>>>>
>>>>>>> +    if (priv->device->pins) {
>>>>>>> +        struct pinctrl_state *s;
>>>>>>> +
>>>>>>> +        /* Attempt to use "active" if available else use 
>>>>>>> "default" */
>>>>>>> +        s = pinctrl_lookup_state(priv->device->pins->p, "active");
>>>>>>> +        if (!IS_ERR(s))
>>>>>>> +            pinctrl_select_state(priv->device->pins->p, s);
> 
>  ^^ different devices passed here
> 
>>>>>>> +        else
>>>>>>> +            pinctrl_pm_select_default_state(dev->dev.parent);
> 
> ^^ and here ? Is it ok?
> 
>>>>>>> +    }
>>>>>>> +#endif
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>>
>>>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 041525d..66e98e7 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -605,7 +605,18 @@  static int c_can_start(struct net_device *dev)
 	priv->can.state = CAN_STATE_ERROR_ACTIVE;
 
 	/* activate pins */
-	pinctrl_pm_select_default_state(dev->dev.parent);
+#ifdef CONFIG_PINCTRL
+	if (priv->device->pins) {
+		struct pinctrl_state *s;
+
+		/* Attempt to use "active" if available else use "default" */
+		s = pinctrl_lookup_state(priv->device->pins->p, "active");
+		if (!IS_ERR(s))
+			pinctrl_select_state(priv->device->pins->p, s);
+		else
+			pinctrl_pm_select_default_state(dev->dev.parent);
+	}
+#endif
 	return 0;
 }