diff mbox series

[1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios

Message ID 20200122080310.24653-2-faiz_abbas@ti.com
State Awaiting Upstream
Delegated to: David Miller
Headers show
Series Add Support for MCAN in AM654x-idk | expand

Commit Message

Faiz Abbas Jan. 22, 2020, 8:03 a.m. UTC 
The CAN transceiver on some boards has an STB pin which is
used to control its standby mode. Add an optional property
stb-gpios to toggle the same.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---
 Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
 1 file changed, 2 insertions(+)

Comments

Dan Murphy Jan. 22, 2020, 1:35 p.m. UTC | #1 
Faiz

On 1/22/20 2:03 AM, Faiz Abbas wrote:
> The CAN transceiver on some boards has an STB pin which is
> used to control its standby mode. Add an optional property
> stb-gpios to toggle the same.
>
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> ---
>   Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
> index ed614383af9c..cc8ba3f7a2aa 100644
> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> @@ -48,6 +48,8 @@ Optional Subnode:
>   			  that can be used for CAN/CAN-FD modes. See
>   			  Documentation/devicetree/bindings/net/can/can-transceiver.txt
>   			  for details.
> +stb-gpios		: gpio node to toggle the STB (standby) signal on the transceiver
> +

The m_can.txt is for the m_can framework.  If this is specific to the 
platform then it really does not belong here.

If the platform has specific nodes then maybe we need a 
m_can_platform.txt binding for specific platform nodes.  But I leave 
that decision to Rob.

Also I prefer you spell out standby like the gpios are spelled out in 
the tcan binding.

Dan


>   Example:
>   SoC dtsi:
>   m_can1: can@20e8000 {
Sekhar Nori Jan. 22, 2020, 2:24 p.m. UTC | #2 
On 22/01/20 7:05 PM, Dan Murphy wrote:
> Faiz
> 
> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>> The CAN transceiver on some boards has an STB pin which is
>> used to control its standby mode. Add an optional property
>> stb-gpios to toggle the same.
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>> ---
>>   Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>> index ed614383af9c..cc8ba3f7a2aa 100644
>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>> @@ -48,6 +48,8 @@ Optional Subnode:
>>                 that can be used for CAN/CAN-FD modes. See
>>                
>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>                 for details.
>> +stb-gpios        : gpio node to toggle the STB (standby) signal on
>> the transceiver
>> +
> 
> The m_can.txt is for the m_can framework.  If this is specific to the
> platform then it really does not belong here.
> 
> If the platform has specific nodes then maybe we need a
> m_can_platform.txt binding for specific platform nodes.  But I leave
> that decision to Rob.

Since this is transceiver enable, should this not be in
Documentation/devicetree/bindings/net/can/can-transceiver.txt?

Thanks,
Sekhar
Dan Murphy Jan. 22, 2020, 2:34 p.m. UTC | #3 
Sekhar

On 1/22/20 8:24 AM, Sekhar Nori wrote:
> On 22/01/20 7:05 PM, Dan Murphy wrote:
>> Faiz
>>
>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>>> The CAN transceiver on some boards has an STB pin which is
>>> used to control its standby mode. Add an optional property
>>> stb-gpios to toggle the same.
>>>
>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>> ---
>>>    Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>>> index ed614383af9c..cc8ba3f7a2aa 100644
>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>> @@ -48,6 +48,8 @@ Optional Subnode:
>>>                  that can be used for CAN/CAN-FD modes. See
>>>                 
>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>>                  for details.
>>> +stb-gpios        : gpio node to toggle the STB (standby) signal on
>>> the transceiver
>>> +
>> The m_can.txt is for the m_can framework.  If this is specific to the
>> platform then it really does not belong here.
>>
>> If the platform has specific nodes then maybe we need a
>> m_can_platform.txt binding for specific platform nodes.  But I leave
>> that decision to Rob.
> Since this is transceiver enable, should this not be in
> Documentation/devicetree/bindings/net/can/can-transceiver.txt?

+1

Dan
Faiz Abbas Jan. 23, 2020, 7:39 a.m. UTC | #4 
Hi,

On 22/01/20 8:04 pm, Dan Murphy wrote:
> Sekhar
> 
> On 1/22/20 8:24 AM, Sekhar Nori wrote:
>> On 22/01/20 7:05 PM, Dan Murphy wrote:
>>> Faiz
>>>
>>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>>>> The CAN transceiver on some boards has an STB pin which is
>>>> used to control its standby mode. Add an optional property
>>>> stb-gpios to toggle the same.
>>>>
>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> index ed614383af9c..cc8ba3f7a2aa 100644
>>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>> @@ -48,6 +48,8 @@ Optional Subnode:
>>>>                  that can be used for CAN/CAN-FD modes. See
>>>>                
>>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>>>                  for details.
>>>> +stb-gpios        : gpio node to toggle the STB (standby) signal on
>>>> the transceiver
>>>> +
>>> The m_can.txt is for the m_can framework.  If this is specific to the
>>> platform then it really does not belong here.
>>>
>>> If the platform has specific nodes then maybe we need a
>>> m_can_platform.txt binding for specific platform nodes.  But I leave
>>> that decision to Rob.
>> Since this is transceiver enable, should this not be in
>> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
> 

The transceiver node is just a node without an associated device. I had
tried to convert it to a phy implementation but that idea got shot down
here:

https://lore.kernel.org/patchwork/patch/1006238/

Thanks,
Faiz
Rob Herring Feb. 3, 2020, 12:06 p.m. UTC | #5 
On Thu, Jan 23, 2020 at 01:09:41PM +0530, Faiz Abbas wrote:
> Hi,
> 
> On 22/01/20 8:04 pm, Dan Murphy wrote:
> > Sekhar
> > 
> > On 1/22/20 8:24 AM, Sekhar Nori wrote:
> >> On 22/01/20 7:05 PM, Dan Murphy wrote:
> >>> Faiz
> >>>
> >>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
> >>>> The CAN transceiver on some boards has an STB pin which is
> >>>> used to control its standby mode. Add an optional property
> >>>> stb-gpios to toggle the same.
> >>>>
> >>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
> >>>> ---
> >>>>    Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
> >>>>    1 file changed, 2 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> index ed614383af9c..cc8ba3f7a2aa 100644
> >>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
> >>>> @@ -48,6 +48,8 @@ Optional Subnode:
> >>>>                  that can be used for CAN/CAN-FD modes. See
> >>>>                
> >>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
> >>>>                  for details.
> >>>> +stb-gpios        : gpio node to toggle the STB (standby) signal on
> >>>> the transceiver
> >>>> +
> >>> The m_can.txt is for the m_can framework.  If this is specific to the
> >>> platform then it really does not belong here.
> >>>
> >>> If the platform has specific nodes then maybe we need a
> >>> m_can_platform.txt binding for specific platform nodes.  But I leave
> >>> that decision to Rob.
> >> Since this is transceiver enable, should this not be in
> >> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
> > 
> 
> The transceiver node is just a node without an associated device. I had
> tried to convert it to a phy implementation but that idea got shot down
> here:
> 
> https://lore.kernel.org/patchwork/patch/1006238/

Nodes and drivers are not a 1-1 thing. Is the transceiver a separate h/w 
device? If so, then it should be a separate node and properties of that 
device go in its node. Also, nothing is stopping you from using the PHY 
binding without using the kernel's PHY framework.

As to whether it should be a separate phy driver, I think probably the 
wrong decision was made. We always seem to start out with no PHY on 
these things and the complexity just grows until we need one. 

Rob
Faiz Abbas Feb. 17, 2020, 1:53 p.m. UTC | #6 
Rob,

On 03/02/20 5:36 pm, Rob Herring wrote:
> On Thu, Jan 23, 2020 at 01:09:41PM +0530, Faiz Abbas wrote:
>> Hi,
>>
>> On 22/01/20 8:04 pm, Dan Murphy wrote:
>>> Sekhar
>>>
>>> On 1/22/20 8:24 AM, Sekhar Nori wrote:
>>>> On 22/01/20 7:05 PM, Dan Murphy wrote:
>>>>> Faiz
>>>>>
>>>>> On 1/22/20 2:03 AM, Faiz Abbas wrote:
>>>>>> The CAN transceiver on some boards has an STB pin which is
>>>>>> used to control its standby mode. Add an optional property
>>>>>> stb-gpios to toggle the same.
>>>>>>
>>>>>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>>>>>> Signed-off-by: Sekhar Nori <nsekhar@ti.com>
>>>>>> ---
>>>>>>    Documentation/devicetree/bindings/net/can/m_can.txt | 2 ++
>>>>>>    1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> index ed614383af9c..cc8ba3f7a2aa 100644
>>>>>> --- a/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> +++ b/Documentation/devicetree/bindings/net/can/m_can.txt
>>>>>> @@ -48,6 +48,8 @@ Optional Subnode:
>>>>>>                  that can be used for CAN/CAN-FD modes. See
>>>>>>                
>>>>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt
>>>>>>                  for details.
>>>>>> +stb-gpios        : gpio node to toggle the STB (standby) signal on
>>>>>> the transceiver
>>>>>> +
>>>>> The m_can.txt is for the m_can framework.  If this is specific to the
>>>>> platform then it really does not belong here.
>>>>>
>>>>> If the platform has specific nodes then maybe we need a
>>>>> m_can_platform.txt binding for specific platform nodes.  But I leave
>>>>> that decision to Rob.
>>>> Since this is transceiver enable, should this not be in
>>>> Documentation/devicetree/bindings/net/can/can-transceiver.txt?
>>>
>>
>> The transceiver node is just a node without an associated device. I had
>> tried to convert it to a phy implementation but that idea got shot down
>> here:
>>
>> https://lore.kernel.org/patchwork/patch/1006238/
> 
> Nodes and drivers are not a 1-1 thing. Is the transceiver a separate h/w 
> device? If so, then it should be a separate node and properties of that 
> device go in its node. 

The transceiver is indeed a separate device.

Also, nothing is stopping you from using the PHY
> binding without using the kernel's PHY framework.

The phy framework seemed like the best code reuse to implement it.

> 
> As to whether it should be a separate phy driver, I think probably the 
> wrong decision was made. We always seem to start out with no PHY on 
> these things and the complexity just grows until we need one. 
> 

We should be able to handle two properties (one max-datarate and the
other regulator node) for now. If we have to add more complex parts then
maybe we can think about the driver. I am just adding a xceiver
regulator for now.

Thanks,
Faiz
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/net/can/m_can.txt b/Documentation/devicetree/bindings/net/can/m_can.txt
index ed614383af9c..cc8ba3f7a2aa 100644
--- a/Documentation/devicetree/bindings/net/can/m_can.txt
+++ b/Documentation/devicetree/bindings/net/can/m_can.txt
@@ -48,6 +48,8 @@  Optional Subnode:
 			  that can be used for CAN/CAN-FD modes. See
 			  Documentation/devicetree/bindings/net/can/can-transceiver.txt
 			  for details.
+stb-gpios		: gpio node to toggle the STB (standby) signal on the transceiver
+
 Example:
 SoC dtsi:
 m_can1: can@20e8000 {