[PATCH 0/3] Add Support for MCAN in AM654x-idk

[PATCH 0/3] Add Support for MCAN in AM654x-idk

[Faiz Abbas]

This series adds driver patches to support MCAN in TI's AM654x-idk.

Faiz Abbas (3):
  dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  can: m_can: m_can_platform: Add support for enabling transceiver
    through the STB line
  arm64: defconfig: Add Support for Bosch M_CAN controllers

 Documentation/devicetree/bindings/net/can/m_can.txt |  2 ++
 arch/arm64/configs/defconfig                        |  3 +++
 drivers/net/can/m_can/m_can_platform.c              | 12 ++++++++++++
 3 files changed, 17 insertions(+)

-- 
2.19.2

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

[Faiz Abbas]

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
+
 Example:
 SoC dtsi:
 m_can1: can@20e8000 {
-- 
2.19.2

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

[Dan Murphy]

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 {

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

[Sekhar Nori]

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

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

[Dan Murphy]

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

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

[Faiz Abbas]

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

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

[Rob Herring]

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

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

[Faiz Abbas]

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

[PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line

[Faiz Abbas]

CAN transceivers on some boards have an STB (standby) line which can be
toggled to enable/disable the transceiver. Add support for enabling the
transceiver using a GPIO connected to the STB line.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index 38ea5e600fb8..b4e1423bd5d8 100644
--- a/drivers/net/can/m_can/m_can_platform.c
+++ b/drivers/net/can/m_can/m_can_platform.c
@@ -6,6 +6,7 @@
 // Copyright (C) 2018-19 Texas Instruments Incorporated - http://www.ti.com/
 
 #include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
 
 #include "m_can.h"
 
@@ -57,6 +58,7 @@ static int m_can_plat_probe(struct platform_device *pdev)
 {
 	struct m_can_classdev *mcan_class;
 	struct m_can_plat_priv *priv;
+	struct gpio_desc *stb;
 	struct resource *res;
 	void __iomem *addr;
 	void __iomem *mram_addr;
@@ -111,6 +113,16 @@ static int m_can_plat_probe(struct platform_device *pdev)
 
 	m_can_init_ram(mcan_class);
 
+	stb = devm_gpiod_get_optional(&pdev->dev, "stb", GPIOD_OUT_HIGH);
+	if (IS_ERR(stb)) {
+		ret = PTR_ERR(stb);
+		if (ret != -EPROBE_DEFER)
+			dev_err(&pdev->dev,
+				"gpio request failed, ret %d\n", ret);
+
+		goto failed_ret;
+	}
+
 	ret = m_can_class_register(mcan_class);
 
 failed_ret:
-- 
2.19.2

RE: [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line

[Sriram Dash]

> From: linux-can-owner@vger.kernel.org <linux-can-owner@vger.kernel.org> On
> Behalf Of Faiz Abbas
> Subject: [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling
> transceiver through the STB line
> 
> CAN transceivers on some boards have an STB (standby) line which can be
> toggled to enable/disable the transceiver. Add support for enabling the
> transceiver using a GPIO connected to the STB line.
> 

Looks good to me. 
Other than Dan's concern on stb  as standby,
Acked-by: Sriram Dash <sriram.dash@samsung.com>

> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> ---
>  drivers/net/can/m_can/m_can_platform.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can_platform.c
> b/drivers/net/can/m_can/m_can_platform.c
> index 38ea5e600fb8..b4e1423bd5d8 100644
> --- a/drivers/net/can/m_can/m_can_platform.c
> +++ b/drivers/net/can/m_can/m_can_platform.c
> @@ -6,6 +6,7 @@
>  // Copyright (C) 2018-19 Texas Instruments Incorporated -
http://www.ti.com/
> 
>  #include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> 
>  #include "m_can.h"
> 
> @@ -57,6 +58,7 @@ static int m_can_plat_probe(struct platform_device
*pdev)
> {
>  	struct m_can_classdev *mcan_class;
>  	struct m_can_plat_priv *priv;
> +	struct gpio_desc *stb;
>  	struct resource *res;
>  	void __iomem *addr;
>  	void __iomem *mram_addr;
> @@ -111,6 +113,16 @@ static int m_can_plat_probe(struct platform_device
> *pdev)
> 
>  	m_can_init_ram(mcan_class);
> 
> +	stb = devm_gpiod_get_optional(&pdev->dev, "stb", GPIOD_OUT_HIGH);
> +	if (IS_ERR(stb)) {
> +		ret = PTR_ERR(stb);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(&pdev->dev,
> +				"gpio request failed, ret %d\n", ret);
> +
> +		goto failed_ret;
> +	}
> +
>  	ret = m_can_class_register(mcan_class);
> 
>  failed_ret:
> --
> 2.19.2

[PATCH 3/3] arm64: defconfig: Add Support for Bosch M_CAN controllers

[Faiz Abbas]

Enable configs for supporting Bosch M_CAN controllers.

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
---
 arch/arm64/configs/defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 458bddeba89c..9d2ea46cc4ae 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -159,6 +159,9 @@ CONFIG_QRTR=m
 CONFIG_QRTR_SMD=m
 CONFIG_QRTR_TUN=m
 CONFIG_BPF_JIT=y
+CONFIG_CAN=m
+CONFIG_CAN_M_CAN=m
+CONFIG_CAN_M_CAN_PLATFORM=m
 CONFIG_BT=m
 CONFIG_BT_HIDP=m
 # CONFIG_BT_HS is not set
-- 
2.19.2

Re: [PATCH 0/3] Add Support for MCAN in AM654x-idk

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1037 bytes --]

On 1/22/20 9:03 AM, Faiz Abbas wrote:
> This series adds driver patches to support MCAN in TI's AM654x-idk.
> 
> Faiz Abbas (3):
>   dt-bindings: net: can: m_can: Add Documentation for stb-gpios
>   can: m_can: m_can_platform: Add support for enabling transceiver
>     through the STB line
>   arm64: defconfig: Add Support for Bosch M_CAN controllers
> 
>  Documentation/devicetree/bindings/net/can/m_can.txt |  2 ++
>  arch/arm64/configs/defconfig                        |  3 +++
>  drivers/net/can/m_can/m_can_platform.c              | 12 ++++++++++++
>  3 files changed, 17 insertions(+)

What about adding support for xceiver-supply as done in several other
drivers (ti_hecc.c, flexcan.c, mcp251x.c)? And using this for the stb line?

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/3] Add Support for MCAN in AM654x-idk

[Faiz Abbas]

Marc,

On 23/01/20 4:47 pm, Marc Kleine-Budde wrote:
> On 1/22/20 9:03 AM, Faiz Abbas wrote:
>> This series adds driver patches to support MCAN in TI's AM654x-idk.
>>
>> Faiz Abbas (3):
>>   dt-bindings: net: can: m_can: Add Documentation for stb-gpios
>>   can: m_can: m_can_platform: Add support for enabling transceiver
>>     through the STB line
>>   arm64: defconfig: Add Support for Bosch M_CAN controllers
>>
>>  Documentation/devicetree/bindings/net/can/m_can.txt |  2 ++
>>  arch/arm64/configs/defconfig                        |  3 +++
>>  drivers/net/can/m_can/m_can_platform.c              | 12 ++++++++++++
>>  3 files changed, 17 insertions(+)
> 
> What about adding support for xceiver-supply as done in several other
> drivers (ti_hecc.c, flexcan.c, mcp251x.c)? And using this for the stb line?

Looks like you had given this feedback a long time ago and I forgot
about it. Sorry about that :-)

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

But now that I think about it, its kinda weird that we are modelling
part of the transceiver as a separate child node
(Documentation/devicetree/bindings/net/can/can-transceiver.txt) and the
other parts as a regulator.

Anyone looking at the transceiver node would figure thats where the
enable gpio/regulator node needs to go instead of the parent node.
Shouldn't we have all transceiver properties under the same node?

Thanks,
Faiz

Re: [PATCH 0/3] Add Support for MCAN in AM654x-idk

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1991 bytes --]

On 1/23/20 12:46 PM, Faiz Abbas wrote:
> Marc,
> 
> On 23/01/20 4:47 pm, Marc Kleine-Budde wrote:
>> On 1/22/20 9:03 AM, Faiz Abbas wrote:
>>> This series adds driver patches to support MCAN in TI's AM654x-idk.
>>>
>>> Faiz Abbas (3):
>>>   dt-bindings: net: can: m_can: Add Documentation for stb-gpios
>>>   can: m_can: m_can_platform: Add support for enabling transceiver
>>>     through the STB line
>>>   arm64: defconfig: Add Support for Bosch M_CAN controllers
>>>
>>>  Documentation/devicetree/bindings/net/can/m_can.txt |  2 ++
>>>  arch/arm64/configs/defconfig                        |  3 +++
>>>  drivers/net/can/m_can/m_can_platform.c              | 12 ++++++++++++
>>>  3 files changed, 17 insertions(+)
>>
>> What about adding support for xceiver-supply as done in several other
>> drivers (ti_hecc.c, flexcan.c, mcp251x.c)? And using this for the stb line?
> 
> Looks like you had given this feedback a long time ago and I forgot
> about it. Sorry about that :-)
> 
> https://lore.kernel.org/patchwork/patch/1006238/
> 
> But now that I think about it, its kinda weird that we are modelling
> part of the transceiver as a separate child node
> (Documentation/devicetree/bindings/net/can/can-transceiver.txt) and the
> other parts as a regulator.

We need a regulator, as there are dual phy chips with a single enable line.

> Anyone looking at the transceiver node would figure thats where the
> enable gpio/regulator node needs to go instead of the parent node.
> Shouldn't we have all transceiver properties under the same node?

Feel free to add support for the regulator to the transceiver node and
convert the existing drivers to accept both bindings.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]