linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] Add Support for MCAN in AM654x-idk
@ 2020-01-22  8:03 Faiz Abbas
  2020-01-22  8:03 ` [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios Faiz Abbas
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Faiz Abbas @ 2020-01-22  8:03 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, dmurphy, faiz_abbas, nm, t-kristo

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-01-22  8:03 [PATCH 0/3] Add Support for MCAN in AM654x-idk Faiz Abbas
@ 2020-01-22  8:03 ` Faiz Abbas
  2020-01-22 13:35   ` Dan Murphy
  2020-01-22  8:03 ` [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line Faiz Abbas
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 14+ messages in thread
From: Faiz Abbas @ 2020-01-22  8:03 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, dmurphy, faiz_abbas, nm, t-kristo

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line
  2020-01-22  8:03 [PATCH 0/3] Add Support for MCAN in AM654x-idk Faiz Abbas
  2020-01-22  8:03 ` [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios Faiz Abbas
@ 2020-01-22  8:03 ` Faiz Abbas
  2020-01-22 14:53   ` Sriram Dash
  2020-01-22  8:03 ` [PATCH 3/3] arm64: defconfig: Add Support for Bosch M_CAN controllers Faiz Abbas
  2020-01-23 11:17 ` [PATCH 0/3] Add Support for MCAN in AM654x-idk Marc Kleine-Budde
  3 siblings, 1 reply; 14+ messages in thread
From: Faiz Abbas @ 2020-01-22  8:03 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, dmurphy, faiz_abbas, nm, t-kristo

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH 3/3] arm64: defconfig: Add Support for Bosch M_CAN controllers
  2020-01-22  8:03 [PATCH 0/3] Add Support for MCAN in AM654x-idk Faiz Abbas
  2020-01-22  8:03 ` [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios Faiz Abbas
  2020-01-22  8:03 ` [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line Faiz Abbas
@ 2020-01-22  8:03 ` Faiz Abbas
  2020-01-23 11:17 ` [PATCH 0/3] Add Support for MCAN in AM654x-idk Marc Kleine-Budde
  3 siblings, 0 replies; 14+ messages in thread
From: Faiz Abbas @ 2020-01-22  8:03 UTC (permalink / raw)
  To: linux-arm-kernel, linux-kernel, devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, dmurphy, faiz_abbas, nm, t-kristo

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


^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-01-22  8:03 ` [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios Faiz Abbas
@ 2020-01-22 13:35   ` Dan Murphy
  2020-01-22 14:24     ` Sekhar Nori
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Murphy @ 2020-01-22 13:35 UTC (permalink / raw)
  To: Faiz Abbas, linux-arm-kernel, linux-kernel, devicetree, netdev,
	linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, nm, t-kristo

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 {

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-01-22 13:35   ` Dan Murphy
@ 2020-01-22 14:24     ` Sekhar Nori
  2020-01-22 14:34       ` Dan Murphy
  0 siblings, 1 reply; 14+ messages in thread
From: Sekhar Nori @ 2020-01-22 14:24 UTC (permalink / raw)
  To: Dan Murphy, Faiz Abbas, linux-arm-kernel, linux-kernel,
	devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, nm, t-kristo

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-01-22 14:24     ` Sekhar Nori
@ 2020-01-22 14:34       ` Dan Murphy
  2020-01-23  7:39         ` Faiz Abbas
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Murphy @ 2020-01-22 14:34 UTC (permalink / raw)
  To: Sekhar Nori, Faiz Abbas, linux-arm-kernel, linux-kernel,
	devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, nm, t-kristo

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* RE: [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line
  2020-01-22  8:03 ` [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line Faiz Abbas
@ 2020-01-22 14:53   ` Sriram Dash
  0 siblings, 0 replies; 14+ messages in thread
From: Sriram Dash @ 2020-01-22 14:53 UTC (permalink / raw)
  To: 'Faiz Abbas',
	linux-arm-kernel, linux-kernel, devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg, dmurphy,
	nm, t-kristo

> 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



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-01-22 14:34       ` Dan Murphy
@ 2020-01-23  7:39         ` Faiz Abbas
  2020-02-03 12:06           ` Rob Herring
  0 siblings, 1 reply; 14+ messages in thread
From: Faiz Abbas @ 2020-01-23  7:39 UTC (permalink / raw)
  To: Dan Murphy, Sekhar Nori, linux-arm-kernel, linux-kernel,
	devicetree, netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, mkl, wg,
	sriram.dash, nm, t-kristo

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] Add Support for MCAN in AM654x-idk
  2020-01-22  8:03 [PATCH 0/3] Add Support for MCAN in AM654x-idk Faiz Abbas
                   ` (2 preceding siblings ...)
  2020-01-22  8:03 ` [PATCH 3/3] arm64: defconfig: Add Support for Bosch M_CAN controllers Faiz Abbas
@ 2020-01-23 11:17 ` Marc Kleine-Budde
  2020-01-23 11:46   ` Faiz Abbas
  3 siblings, 1 reply; 14+ messages in thread
From: Marc Kleine-Budde @ 2020-01-23 11:17 UTC (permalink / raw)
  To: Faiz Abbas, linux-arm-kernel, linux-kernel, devicetree, netdev,
	linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, wg, sriram.dash,
	dmurphy, nm, t-kristo


[-- 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] Add Support for MCAN in AM654x-idk
  2020-01-23 11:17 ` [PATCH 0/3] Add Support for MCAN in AM654x-idk Marc Kleine-Budde
@ 2020-01-23 11:46   ` Faiz Abbas
  2020-01-23 11:54     ` Marc Kleine-Budde
  0 siblings, 1 reply; 14+ messages in thread
From: Faiz Abbas @ 2020-01-23 11:46 UTC (permalink / raw)
  To: Marc Kleine-Budde, linux-arm-kernel, linux-kernel, devicetree,
	netdev, linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, wg, sriram.dash,
	dmurphy, nm, t-kristo

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/3] Add Support for MCAN in AM654x-idk
  2020-01-23 11:46   ` Faiz Abbas
@ 2020-01-23 11:54     ` Marc Kleine-Budde
  0 siblings, 0 replies; 14+ messages in thread
From: Marc Kleine-Budde @ 2020-01-23 11:54 UTC (permalink / raw)
  To: Faiz Abbas, linux-arm-kernel, linux-kernel, devicetree, netdev,
	linux-can
  Cc: catalin.marinas, mark.rutland, robh+dt, davem, wg, sriram.dash,
	dmurphy, nm, t-kristo


[-- 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 --]

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-01-23  7:39         ` Faiz Abbas
@ 2020-02-03 12:06           ` Rob Herring
  2020-02-17 13:53             ` Faiz Abbas
  0 siblings, 1 reply; 14+ messages in thread
From: Rob Herring @ 2020-02-03 12:06 UTC (permalink / raw)
  To: Faiz Abbas
  Cc: Dan Murphy, Sekhar Nori, linux-arm-kernel, linux-kernel,
	devicetree, netdev, linux-can, catalin.marinas, mark.rutland,
	davem, mkl, wg, sriram.dash, nm, t-kristo

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios
  2020-02-03 12:06           ` Rob Herring
@ 2020-02-17 13:53             ` Faiz Abbas
  0 siblings, 0 replies; 14+ messages in thread
From: Faiz Abbas @ 2020-02-17 13:53 UTC (permalink / raw)
  To: Rob Herring
  Cc: Dan Murphy, Sekhar Nori, linux-arm-kernel, linux-kernel,
	devicetree, netdev, linux-can, catalin.marinas, mark.rutland,
	davem, mkl, wg, sriram.dash, nm, t-kristo

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2020-02-17 13:52 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-22  8:03 [PATCH 0/3] Add Support for MCAN in AM654x-idk Faiz Abbas
2020-01-22  8:03 ` [PATCH 1/3] dt-bindings: net: can: m_can: Add Documentation for stb-gpios Faiz Abbas
2020-01-22 13:35   ` Dan Murphy
2020-01-22 14:24     ` Sekhar Nori
2020-01-22 14:34       ` Dan Murphy
2020-01-23  7:39         ` Faiz Abbas
2020-02-03 12:06           ` Rob Herring
2020-02-17 13:53             ` Faiz Abbas
2020-01-22  8:03 ` [PATCH 2/3] can: m_can: m_can_platform: Add support for enabling transceiver through the STB line Faiz Abbas
2020-01-22 14:53   ` Sriram Dash
2020-01-22  8:03 ` [PATCH 3/3] arm64: defconfig: Add Support for Bosch M_CAN controllers Faiz Abbas
2020-01-23 11:17 ` [PATCH 0/3] Add Support for MCAN in AM654x-idk Marc Kleine-Budde
2020-01-23 11:46   ` Faiz Abbas
2020-01-23 11:54     ` Marc Kleine-Budde

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).