[PATCH 0/4] CAN TRANSCEIVER: Add support for CAN transceivers

[PATCH 0/4] CAN TRANSCEIVER: Add support for CAN transceivers

[Aswath Govindraju]

The following series of patches add support for CAN transceivers.

TCAN1042 has a standby signal that needs to be pulled high for
sending/receiving messages[1]. TCAN1043 has a enable signal along with
standby signal that needs to be pulled up for sending/receiving
messages[2], and other combinations of the two lines can be used to put the
transceiver in different states to reduce power consumption. On boards
like the AM654-idk and J721e-evm these signals are controlled using gpios.

Patch 1 models the transceiver as a phy device tree node with properties
for max bit rate supported, gpio properties for indicating gpio pin numbers
to which standby and enable signals are connected.

Patch 2 adds a generic driver to support CAN transceivers.

Patches 3 & 4 add support for implementing the transceiver as a phy of
m_can_platform driver.

Aswath Govindraju (2):
  dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers
  phy: phy-can-transceiver: Add support for generic CAN transceiver
    driver

Faiz Abbas (2):
  dt-bindings: net: can: Document transceiver implementation as phy
  can: m_can_platform: Add support for transceiver as phy

 .../bindings/net/can/bosch,m_can.yaml         |   6 +
 .../bindings/phy/ti,tcan104x-can.yaml         |  56 +++++++
 drivers/net/can/m_can/m_can_platform.c        |  25 ++++
 drivers/phy/Kconfig                           |   9 ++
 drivers/phy/Makefile                          |   1 +
 drivers/phy/phy-can-transceiver.c             | 140 ++++++++++++++++++
 6 files changed, 237 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
 create mode 100644 drivers/phy/phy-can-transceiver.c

-- 
2.17.1

[PATCH 1/4] dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers

[Aswath Govindraju]

Add binding documentation for TI TCAN104x CAN transceivers.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 .../bindings/phy/ti,tcan104x-can.yaml         | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml

diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
new file mode 100644
index 000000000000..4abfc30a97d0
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
@@ -0,0 +1,56 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/phy/ti,tcan104x-can.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: TCAN104x CAN TRANSCEIVER PHY
+
+maintainers:
+  - Aswath Govindraju <a-govindraju@ti.com>
+
+properties:
+  $nodename:
+    pattern: "^tcan104x-phy"
+
+  compatible:
+    enum:
+      - ti,tcan1042
+      - ti,tcan1043
+
+  '#phy-cells':
+    const: 0
+
+  standby-gpios:
+    description:
+      gpio node to toggle standby signal on transceiver
+    maxItems: 1
+
+  enable-gpios:
+    description:
+      gpio node to toggle enable signal on transceiver
+    maxItems: 1
+
+  max-bitrate:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      max bit rate supported in bps
+    minimum: 1
+
+required:
+  - compatible
+  - '#phy-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    transceiver1: tcan104x-phy {
+      compatible = "ti,tcan1043";
+      #phy-cells = <0>;
+      max-bitrate = <5000000>;
+      standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
+      enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_LOW>;
+    };
-- 
2.17.1

[PATCH 2/4] phy: phy-can-transceiver: Add support for generic CAN transceiver driver

[Aswath Govindraju]

The driver adds support for generic CAN transceivers. Currently
the modes supported by this driver are standby and normal modes for TI
TCAN1042 and TCAN1043 CAN transceivers.

The transceiver is modelled as a phy with pins controlled by gpios, to put
the transceiver in various device functional modes. It also gets the phy
attribute max_link_rate for the usage of m_can drivers.

Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 drivers/phy/Kconfig               |   9 ++
 drivers/phy/Makefile              |   1 +
 drivers/phy/phy-can-transceiver.c | 140 ++++++++++++++++++++++++++++++
 3 files changed, 150 insertions(+)
 create mode 100644 drivers/phy/phy-can-transceiver.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 54c1f2f0985f..51902b629fc6 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -61,6 +61,15 @@ config USB_LGM_PHY
 	  interface to interact with USB GEN-II and USB 3.x PHY that is part
 	  of the Intel network SOC.
 
+config PHY_CAN_TRANSCEIVER
+	tristate "CAN transceiver PHY"
+	select GENERIC_PHY
+	help
+	  This option enables support for CAN transceivers as a PHY. This
+	  driver provides function for putting the transceivers in various
+	  functional modes using gpios and sets the attribute max link
+	  rate, for mcan drivers.
+
 source "drivers/phy/allwinner/Kconfig"
 source "drivers/phy/amlogic/Kconfig"
 source "drivers/phy/broadcom/Kconfig"
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index adac1b1a39d1..9c66101c9605 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_PHY_LPC18XX_USB_OTG)	+= phy-lpc18xx-usb-otg.o
 obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
 obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
 obj-$(CONFIG_USB_LGM_PHY)		+= phy-lgm-usb.o
+obj-$(CONFIG_PHY_CAN_TRANSCEIVER)	+= phy-can-transceiver.o
 obj-y					+= allwinner/	\
 					   amlogic/	\
 					   broadcom/	\
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
new file mode 100644
index 000000000000..14496f6e1666
--- /dev/null
+++ b/drivers/phy/phy-can-transceiver.c
@@ -0,0 +1,140 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * phy-can-transceiver.c - phy driver for CAN transceivers
+ *
+ * Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com
+ *
+ */
+#include<linux/phy/phy.h>
+#include<linux/platform_device.h>
+#include<linux/module.h>
+#include<linux/gpio.h>
+#include<linux/gpio/consumer.h>
+
+struct can_transceiver_data {
+	u32 flags;
+#define STB_PRESENT	BIT(0)
+#define EN_PRESENT	BIT(1)
+};
+
+struct can_transceiver_phy {
+	struct phy *generic_phy;
+	struct gpio_desc *standby_gpio;
+	struct gpio_desc *enable_gpio;
+};
+
+/* Power on function */
+static int can_transceiver_phy_power_on(struct phy *phy)
+{
+	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
+
+	if (can_transceiver_phy->standby_gpio)
+		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
+	if (can_transceiver_phy->enable_gpio)
+		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 1);
+	return 0;
+}
+
+/* Power off function */
+static int can_transceiver_phy_power_off(struct phy *phy)
+{
+	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
+
+	if (can_transceiver_phy->standby_gpio)
+		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
+	if (can_transceiver_phy->enable_gpio)
+		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
+	return 0;
+}
+
+static const struct phy_ops can_transceiver_phy_ops = {
+	.power_on	= can_transceiver_phy_power_on,
+	.power_off	= can_transceiver_phy_power_off,
+	.owner		= THIS_MODULE,
+};
+
+static const struct can_transceiver_data tcan1042_drvdata = {
+	.flags = STB_PRESENT,
+};
+
+static const struct can_transceiver_data tcan1043_drvdata = {
+	.flags = STB_PRESENT | EN_PRESENT,
+};
+
+static const struct of_device_id can_transceiver_phy_ids[] = {
+	{
+		.compatible = "ti,tcan1042",
+		.data = &tcan1042_drvdata
+	},
+	{
+		.compatible = "ti,tcan1043",
+		.data = &tcan1043_drvdata
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
+
+int can_transceiver_phy_probe(struct platform_device *pdev)
+{
+	struct phy_provider *phy_provider;
+	struct device *dev = &pdev->dev;
+	struct can_transceiver_phy *can_transceiver_phy;
+	const struct can_transceiver_data *drvdata;
+	const struct of_device_id *match;
+	struct phy *phy;
+	struct gpio_desc *standby_gpio;
+	struct gpio_desc *enable_gpio;
+	u32 max_bitrate = 0;
+
+	can_transceiver_phy = devm_kzalloc(dev, sizeof(struct can_transceiver_phy), GFP_KERNEL);
+
+	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
+	drvdata = match->data;
+
+	phy = devm_phy_create(dev, dev->of_node,
+			      &can_transceiver_phy_ops);
+	if (IS_ERR(phy)) {
+		dev_err(dev, "failed to create can transceiver phy\n");
+		return PTR_ERR(phy);
+	}
+
+	device_property_read_u32(dev, "max-bitrate", &max_bitrate);
+	phy->attrs.max_link_rate = max_bitrate / 1000000;
+
+	can_transceiver_phy->generic_phy = phy;
+
+	if (drvdata->flags & STB_PRESENT) {
+		standby_gpio = devm_gpiod_get(dev, "standby",   GPIOD_OUT_LOW);
+		if (IS_ERR(standby_gpio))
+			return PTR_ERR(standby_gpio);
+		can_transceiver_phy->standby_gpio = standby_gpio;
+	}
+
+	if (drvdata->flags & EN_PRESENT) {
+		enable_gpio = devm_gpiod_get(dev, "enable",   GPIOD_OUT_LOW);
+		if (IS_ERR(enable_gpio))
+			return PTR_ERR(enable_gpio);
+		can_transceiver_phy->enable_gpio = enable_gpio;
+	}
+
+	phy_set_drvdata(can_transceiver_phy->generic_phy, can_transceiver_phy);
+
+	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+
+	return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static struct platform_driver can_transceiver_phy_driver = {
+	.probe = can_transceiver_phy_probe,
+	.driver = {
+		.name = "can-transceiver-phy",
+		.of_match_table = can_transceiver_phy_ids,
+	},
+};
+
+module_platform_driver(can_transceiver_phy_driver);
+
+MODULE_AUTHOR("Faiz Abbas <faiz_abbas@ti.com>");
+MODULE_AUTHOR("Aswath Govindraju <a-govindraju@ti.com>");
+MODULE_DESCRIPTION("CAN TRANSCEIVER PHY driver");
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

[PATCH 3/4] dt-bindings: net: can: Document transceiver implementation as phy

[Aswath Govindraju]

From: Faiz Abbas <faiz_abbas@ti.com>

Some transceivers need a configuration step (for example, pulling the
standby or enable lines) for them to start sending messages. The
transceiver can be implemented as a phy with the configuration done in the
phy driver. The bit rate limitation can the be obtained by the driver using
the phy node.

Document the above implementation in the bosch mcan bindings

Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
---
 Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
index 798fa5fb7bb2..2c01899b1a3e 100644
--- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
+++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
@@ -109,6 +109,12 @@ properties:
   can-transceiver:
     $ref: can-transceiver.yaml#
 
+  phys:
+    minItems: 1
+
+  phy-names:
+    const: can_transceiver
+
 required:
   - compatible
   - reg
-- 
2.17.1

[PATCH 4/4] can: m_can_platform: Add support for transceiver as phy

[Aswath Govindraju]

From: Faiz Abbas <faiz_abbas@ti.com>

Add support for implementing transceiver node as phy. The max_bitrate is
obtained by getting a phy attribute.

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

diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
index 599de0e08cd7..4a762b5a21d8 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/phy/phy.h>
 
 #include "m_can.h"
 
@@ -67,7 +68,9 @@ static int m_can_plat_probe(struct platform_device *pdev)
 	struct resource *res;
 	void __iomem *addr;
 	void __iomem *mram_addr;
+	struct phy *transceiver;
 	int irq, ret = 0;
+	u32 bitrate_max;
 
 	mcan_class = m_can_class_allocate_dev(&pdev->dev,
 					      sizeof(struct m_can_plat_priv));
@@ -101,6 +104,28 @@ static int m_can_plat_probe(struct platform_device *pdev)
 		goto probe_fail;
 	}
 
+	transceiver = devm_phy_optional_get(&pdev->dev, "can_transceiver");
+	if (IS_ERR(transceiver)) {
+		ret = PTR_ERR(transceiver);
+		dev_err(&pdev->dev, "error while getting phy, err=%d\n", ret);
+		return ret;
+	}
+
+	if (!transceiver) {
+		dev_warn(&pdev->dev, "No transceiver phy found\n");
+	} else {
+		ret = phy_power_on(transceiver);
+		if (ret) {
+			dev_err(&pdev->dev, "error powering on phy, err=%d\n", ret);
+			return ret;
+		}
+		/* converting from Mbps to bps */
+		bitrate_max = (transceiver->attrs.max_link_rate) * 1000000;
+		if (!bitrate_max)
+			dev_warn(&pdev->dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit\n");
+		priv->cdev.can.bitrate_max = bitrate_max;
+	}
+
 	priv->base = addr;
 	priv->mram_base = mram_addr;
 
-- 
2.17.1

Re: [PATCH 2/4] phy: phy-can-transceiver: Add support for generic CAN transceiver driver

[Marc Kleine-Budde]

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

On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> The driver adds support for generic CAN transceivers. Currently
> the modes supported by this driver are standby and normal modes for TI
> TCAN1042 and TCAN1043 CAN transceivers.
> 
> The transceiver is modelled as a phy with pins controlled by gpios, to put
> the transceiver in various device functional modes. It also gets the phy
> attribute max_link_rate for the usage of m_can drivers.

This driver should be independent of CAN driver, so you should not mention a
specific driver here.

> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/phy/Kconfig               |   9 ++
>  drivers/phy/Makefile              |   1 +
>  drivers/phy/phy-can-transceiver.c | 140 ++++++++++++++++++++++++++++++
>  3 files changed, 150 insertions(+)
>  create mode 100644 drivers/phy/phy-can-transceiver.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 54c1f2f0985f..51902b629fc6 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -61,6 +61,15 @@ config USB_LGM_PHY
>  	  interface to interact with USB GEN-II and USB 3.x PHY that is part
>  	  of the Intel network SOC.
>  
> +config PHY_CAN_TRANSCEIVER
> +	tristate "CAN transceiver PHY"
> +	select GENERIC_PHY
> +	help
> +	  This option enables support for CAN transceivers as a PHY. This
> +	  driver provides function for putting the transceivers in various
> +	  functional modes using gpios and sets the attribute max link
> +	  rate, for mcan drivers.
> +
>  source "drivers/phy/allwinner/Kconfig"
>  source "drivers/phy/amlogic/Kconfig"
>  source "drivers/phy/broadcom/Kconfig"
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index adac1b1a39d1..9c66101c9605 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -9,6 +9,7 @@ obj-$(CONFIG_PHY_LPC18XX_USB_OTG)	+= phy-lpc18xx-usb-otg.o
>  obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
>  obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
>  obj-$(CONFIG_USB_LGM_PHY)		+= phy-lgm-usb.o
> +obj-$(CONFIG_PHY_CAN_TRANSCEIVER)	+= phy-can-transceiver.o
>  obj-y					+= allwinner/	\
>  					   amlogic/	\
>  					   broadcom/	\
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> new file mode 100644
> index 000000000000..14496f6e1666
> --- /dev/null
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -0,0 +1,140 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * phy-can-transceiver.c - phy driver for CAN transceivers
> + *
> + * Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com
> + *
> + */
> +#include<linux/phy/phy.h>
> +#include<linux/platform_device.h>
> +#include<linux/module.h>
> +#include<linux/gpio.h>
> +#include<linux/gpio/consumer.h>
> +
> +struct can_transceiver_data {
> +	u32 flags;
> +#define STB_PRESENT	BIT(0)
> +#define EN_PRESENT	BIT(1)

please add a common prefix to the defines

> +};
> +
> +struct can_transceiver_phy {
> +	struct phy *generic_phy;
> +	struct gpio_desc *standby_gpio;
> +	struct gpio_desc *enable_gpio;
> +};
> +
> +/* Power on function */
> +static int can_transceiver_phy_power_on(struct phy *phy)
> +{
> +	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
> +
> +	if (can_transceiver_phy->standby_gpio)
> +		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
> +	if (can_transceiver_phy->enable_gpio)
> +		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 1);

Please add a newline before the return.

> +	return 0;
> +}
> +
> +/* Power off function */
> +static int can_transceiver_phy_power_off(struct phy *phy)
> +{
> +	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
> +
> +	if (can_transceiver_phy->standby_gpio)
> +		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
> +	if (can_transceiver_phy->enable_gpio)
> +		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);

same here

> +	return 0;
> +}
> +
> +static const struct phy_ops can_transceiver_phy_ops = {
> +	.power_on	= can_transceiver_phy_power_on,
> +	.power_off	= can_transceiver_phy_power_off,
> +	.owner		= THIS_MODULE,
> +};
> +
> +static const struct can_transceiver_data tcan1042_drvdata = {
> +	.flags = STB_PRESENT,
> +};
> +
> +static const struct can_transceiver_data tcan1043_drvdata = {
> +	.flags = STB_PRESENT | EN_PRESENT,
> +};
> +
> +static const struct of_device_id can_transceiver_phy_ids[] = {
> +	{
> +		.compatible = "ti,tcan1042",
> +		.data = &tcan1042_drvdata
> +	},
> +	{
> +		.compatible = "ti,tcan1043",
> +		.data = &tcan1043_drvdata
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
> +
> +int can_transceiver_phy_probe(struct platform_device *pdev)
> +{
> +	struct phy_provider *phy_provider;
> +	struct device *dev = &pdev->dev;
> +	struct can_transceiver_phy *can_transceiver_phy;
> +	const struct can_transceiver_data *drvdata;
> +	const struct of_device_id *match;
> +	struct phy *phy;
> +	struct gpio_desc *standby_gpio;
> +	struct gpio_desc *enable_gpio;
> +	u32 max_bitrate = 0;
> +
> +	can_transceiver_phy = devm_kzalloc(dev, sizeof(struct can_transceiver_phy), GFP_KERNEL);

error handling?

> +
> +	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
> +	drvdata = match->data;
> +
> +	phy = devm_phy_create(dev, dev->of_node,
> +			      &can_transceiver_phy_ops);
> +	if (IS_ERR(phy)) {
> +		dev_err(dev, "failed to create can transceiver phy\n");
> +		return PTR_ERR(phy);
> +	}
> +
> +	device_property_read_u32(dev, "max-bitrate", &max_bitrate);
> +	phy->attrs.max_link_rate = max_bitrate / 1000000;

The problem is, there are CAN transceivers with a max of 83.3 kbit/s or 125 kbit/s.

> +	can_transceiver_phy->generic_phy = phy;
> +
> +	if (drvdata->flags & STB_PRESENT) {
> +		standby_gpio = devm_gpiod_get(dev, "standby",   GPIOD_OUT_LOW);

please use only one space after the ",".
Why do you request the gpio standby low?

> +		if (IS_ERR(standby_gpio))
> +			return PTR_ERR(standby_gpio);
> +		can_transceiver_phy->standby_gpio = standby_gpio;
> +	}
> +
> +	if (drvdata->flags & EN_PRESENT) {
> +		enable_gpio = devm_gpiod_get(dev, "enable",   GPIOD_OUT_LOW);
> +		if (IS_ERR(enable_gpio))
> +			return PTR_ERR(enable_gpio);
> +		can_transceiver_phy->enable_gpio = enable_gpio;
> +	}
> +
> +	phy_set_drvdata(can_transceiver_phy->generic_phy, can_transceiver_phy);
> +
> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
> +
> +	return PTR_ERR_OR_ZERO(phy_provider);
> +}
> +
> +static struct platform_driver can_transceiver_phy_driver = {
> +	.probe = can_transceiver_phy_probe,
> +	.driver = {
> +		.name = "can-transceiver-phy",
> +		.of_match_table = can_transceiver_phy_ids,
> +	},
> +};
> +
> +module_platform_driver(can_transceiver_phy_driver);
> +
> +MODULE_AUTHOR("Faiz Abbas <faiz_abbas@ti.com>");
> +MODULE_AUTHOR("Aswath Govindraju <a-govindraju@ti.com>");
> +MODULE_DESCRIPTION("CAN TRANSCEIVER PHY driver");
> +MODULE_LICENSE("GPL v2");
> 

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 1/4] dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers

[Marc Kleine-Budde]

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

On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> Add binding documentation for TI TCAN104x CAN transceivers.
> 
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  .../bindings/phy/ti,tcan104x-can.yaml         | 56 +++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> new file mode 100644
> index 000000000000..4abfc30a97d0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> @@ -0,0 +1,56 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/phy/ti,tcan104x-can.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: TCAN104x CAN TRANSCEIVER PHY
> +
> +maintainers:
> +  - Aswath Govindraju <a-govindraju@ti.com>
> +
> +properties:
> +  $nodename:
> +    pattern: "^tcan104x-phy"
> +
> +  compatible:
> +    enum:
> +      - ti,tcan1042
> +      - ti,tcan1043

Can you create a generic standby only and a generic standby and enable transceiver?

> +
> +  '#phy-cells':
> +    const: 0
> +
> +  standby-gpios:
> +    description:
> +      gpio node to toggle standby signal on transceiver
> +    maxItems: 1
> +
> +  enable-gpios:
> +    description:
> +      gpio node to toggle enable signal on transceiver
> +    maxItems: 1
> +
> +  max-bitrate:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description:
> +      max bit rate supported in bps
> +    minimum: 1
> +
> +required:
> +  - compatible
> +  - '#phy-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +
> +    transceiver1: tcan104x-phy {
> +      compatible = "ti,tcan1043";
> +      #phy-cells = <0>;
> +      max-bitrate = <5000000>;
> +      standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
> +      enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_LOW>;
> +    };
> 


-- 
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 4/4] can: m_can_platform: Add support for transceiver as phy

[Marc Kleine-Budde]

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

On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> From: Faiz Abbas <faiz_abbas@ti.com>
> 
> Add support for implementing transceiver node as phy. The max_bitrate is
> obtained by getting a phy attribute.
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  drivers/net/can/m_can/m_can_platform.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/net/can/m_can/m_can_platform.c b/drivers/net/can/m_can/m_can_platform.c
> index 599de0e08cd7..4a762b5a21d8 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/phy/phy.h>
>  
>  #include "m_can.h"
>  
> @@ -67,7 +68,9 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  	struct resource *res;
>  	void __iomem *addr;
>  	void __iomem *mram_addr;
> +	struct phy *transceiver;
>  	int irq, ret = 0;
> +	u32 bitrate_max;
>  
>  	mcan_class = m_can_class_allocate_dev(&pdev->dev,
>  					      sizeof(struct m_can_plat_priv));
> @@ -101,6 +104,28 @@ static int m_can_plat_probe(struct platform_device *pdev)
>  		goto probe_fail;
>  	}
>  
> +	transceiver = devm_phy_optional_get(&pdev->dev, "can_transceiver");
> +	if (IS_ERR(transceiver)) {
> +		ret = PTR_ERR(transceiver);
> +		dev_err(&pdev->dev, "error while getting phy, err=%d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!transceiver) {
> +		dev_warn(&pdev->dev, "No transceiver phy found\n");

I think that's a bit to loud.

> +	} else {
> +		ret = phy_power_on(transceiver);

Please move the phy power on/off to the ndo_open and ndo_stop callbacks. There's
no need to power the transceivers if the CAN interface is down.

> +		if (ret) {
> +			dev_err(&pdev->dev, "error powering on phy, err=%d\n", ret);
> +			return ret;
> +		}
> +		/* converting from Mbps to bps */
> +		bitrate_max = (transceiver->attrs.max_link_rate) * 1000000;
> +		if (!bitrate_max)
> +			dev_warn(&pdev->dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit\n");

Please move this check to the generic transcevier code.

> +		priv->cdev.can.bitrate_max = bitrate_max;
> +	}
> +
>  	priv->base = addr;
>  	priv->mram_base = mram_addr;
>  
> 

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 1/4] dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers

[Rob Herring]

On Mon, Apr 12, 2021 at 12:19:30PM +0200, Marc Kleine-Budde wrote:
> On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> > Add binding documentation for TI TCAN104x CAN transceivers.
> > 
> > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> > ---
> >  .../bindings/phy/ti,tcan104x-can.yaml         | 56 +++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > new file mode 100644
> > index 000000000000..4abfc30a97d0
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > @@ -0,0 +1,56 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/phy/ti,tcan104x-can.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: TCAN104x CAN TRANSCEIVER PHY
> > +
> > +maintainers:
> > +  - Aswath Govindraju <a-govindraju@ti.com>
> > +
> > +properties:
> > +  $nodename:
> > +    pattern: "^tcan104x-phy"
> > +
> > +  compatible:
> > +    enum:
> > +      - ti,tcan1042
> > +      - ti,tcan1043
> 
> Can you create a generic standby only and a generic standby and enable transceiver?

As a fallback compatible fine, but no generic binding please. A generic 
binding can't describe any timing requirements between the 2 GPIO as 
well as supplies when someone wants to add those (and they will).

> 
> > +
> > +  '#phy-cells':
> > +    const: 0
> > +
> > +  standby-gpios:
> > +    description:
> > +      gpio node to toggle standby signal on transceiver
> > +    maxItems: 1
> > +
> > +  enable-gpios:
> > +    description:
> > +      gpio node to toggle enable signal on transceiver
> > +    maxItems: 1
> > +
> > +  max-bitrate:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      max bit rate supported in bps

We already have 'max-speed' for serial devices, use that.

> > +    minimum: 1
> > +
> > +required:
> > +  - compatible
> > +  - '#phy-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/gpio/gpio.h>
> > +
> > +    transceiver1: tcan104x-phy {
> > +      compatible = "ti,tcan1043";
> > +      #phy-cells = <0>;
> > +      max-bitrate = <5000000>;
> > +      standby-gpios = <&wakeup_gpio1 16 GPIO_ACTIVE_LOW>;
> > +      enable-gpios = <&main_gpio1 67 GPIO_ACTIVE_LOW>;
> > +    };
> > 
> 
> 
> -- 
> 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 |
> 

Re: [PATCH 3/4] dt-bindings: net: can: Document transceiver implementation as phy

[Rob Herring]

On Fri, Apr 09, 2021 at 07:10:53PM +0530, Aswath Govindraju wrote:
> From: Faiz Abbas <faiz_abbas@ti.com>
> 
> Some transceivers need a configuration step (for example, pulling the
> standby or enable lines) for them to start sending messages. The
> transceiver can be implemented as a phy with the configuration done in the
> phy driver. The bit rate limitation can the be obtained by the driver using
> the phy node.
> 
> Document the above implementation in the bosch mcan bindings
> 
> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> ---
>  Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> index 798fa5fb7bb2..2c01899b1a3e 100644
> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> @@ -109,6 +109,12 @@ properties:
>    can-transceiver:
>      $ref: can-transceiver.yaml#
>  
> +  phys:
> +    minItems: 1

maxItems: 1

> +
> +  phy-names:
> +    const: can_transceiver

Kind of a pointless name. You don't really need a name if there's a 
single entry.

> +
>  required:
>    - compatible
>    - reg
> -- 
> 2.17.1
> 

Re: [PATCH 1/4] dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3026 bytes --]

On 12.04.2021 12:49:56, Rob Herring wrote:
> On Mon, Apr 12, 2021 at 12:19:30PM +0200, Marc Kleine-Budde wrote:
> > On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> > > Add binding documentation for TI TCAN104x CAN transceivers.
> > > 
> > > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> > > ---
> > >  .../bindings/phy/ti,tcan104x-can.yaml         | 56 +++++++++++++++++++
> > >  1 file changed, 56 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > new file mode 100644
> > > index 000000000000..4abfc30a97d0
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > @@ -0,0 +1,56 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: "http://devicetree.org/schemas/phy/ti,tcan104x-can.yaml#"
> > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > +
> > > +title: TCAN104x CAN TRANSCEIVER PHY
> > > +
> > > +maintainers:
> > > +  - Aswath Govindraju <a-govindraju@ti.com>
> > > +
> > > +properties:
> > > +  $nodename:
> > > +    pattern: "^tcan104x-phy"
> > > +
> > > +  compatible:
> > > +    enum:
> > > +      - ti,tcan1042
> > > +      - ti,tcan1043
> > 
> > Can you create a generic standby only and a generic standby and enable transceiver?
> 
> As a fallback compatible fine, but no generic binding please. A generic 
> binding can't describe any timing requirements between the 2 GPIO as 
> well as supplies when someone wants to add those (and they will).

Right - that makes sense.

> > > +
> > > +  '#phy-cells':
> > > +    const: 0
> > > +
> > > +  standby-gpios:
> > > +    description:
> > > +      gpio node to toggle standby signal on transceiver
> > > +    maxItems: 1
> > > +
> > > +  enable-gpios:
> > > +    description:
> > > +      gpio node to toggle enable signal on transceiver
> > > +    maxItems: 1
> > > +
> > > +  max-bitrate:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    description:
> > > +      max bit rate supported in bps
> 
> We already have 'max-speed' for serial devices, use that.

There is already the neither Ethernet PHY (PHYLINK/PHYLIB) nor generic
PHY (GENERIC_PHY) can-transceiver binding
Documentation/devicetree/bindings/net/can/can-transceiver.yaml which
specifies max-bitrate. I don't have strong feelings whether to use
max-bitrate or max-speed.

Speaking about Ethernet PHYs, what are to pros and cons to use the
generic PHY compared to the Ethernet PHY infrastructure?

regards,
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers

[Rob Herring]

On Tue, Apr 13, 2021 at 2:41 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>
> On 12.04.2021 12:49:56, Rob Herring wrote:
> > On Mon, Apr 12, 2021 at 12:19:30PM +0200, Marc Kleine-Budde wrote:
> > > On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> > > > Add binding documentation for TI TCAN104x CAN transceivers.
> > > >
> > > > Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> > > > ---
> > > >  .../bindings/phy/ti,tcan104x-can.yaml         | 56 +++++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > > new file mode 100644
> > > > index 000000000000..4abfc30a97d0
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
> > > > @@ -0,0 +1,56 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: "http://devicetree.org/schemas/phy/ti,tcan104x-can.yaml#"
> > > > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > > > +
> > > > +title: TCAN104x CAN TRANSCEIVER PHY
> > > > +
> > > > +maintainers:
> > > > +  - Aswath Govindraju <a-govindraju@ti.com>
> > > > +
> > > > +properties:
> > > > +  $nodename:
> > > > +    pattern: "^tcan104x-phy"
> > > > +
> > > > +  compatible:
> > > > +    enum:
> > > > +      - ti,tcan1042
> > > > +      - ti,tcan1043
> > >
> > > Can you create a generic standby only and a generic standby and enable transceiver?
> >
> > As a fallback compatible fine, but no generic binding please. A generic
> > binding can't describe any timing requirements between the 2 GPIO as
> > well as supplies when someone wants to add those (and they will).
>
> Right - that makes sense.
>
> > > > +
> > > > +  '#phy-cells':
> > > > +    const: 0
> > > > +
> > > > +  standby-gpios:
> > > > +    description:
> > > > +      gpio node to toggle standby signal on transceiver
> > > > +    maxItems: 1
> > > > +
> > > > +  enable-gpios:
> > > > +    description:
> > > > +      gpio node to toggle enable signal on transceiver
> > > > +    maxItems: 1
> > > > +
> > > > +  max-bitrate:
> > > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > > +    description:
> > > > +      max bit rate supported in bps
> >
> > We already have 'max-speed' for serial devices, use that.
>
> There is already the neither Ethernet PHY (PHYLINK/PHYLIB) nor generic
> PHY (GENERIC_PHY) can-transceiver binding
> Documentation/devicetree/bindings/net/can/can-transceiver.yaml which
> specifies max-bitrate. I don't have strong feelings whether to use
> max-bitrate or max-speed.

Okay, max-bitrate is fine.

>
> Speaking about Ethernet PHYs, what are to pros and cons to use the
> generic PHY compared to the Ethernet PHY infrastructure?

For higher speed ethernet, both are used. There's the serdes phy and
the ethernet phy with serdes phy using the generic phy binding. For
CAN, it probably comes down to what's a better fit.

Rob

Re: [PATCH 2/4] phy: phy-can-transceiver: Add support for generic CAN transceiver driver

[Aswath Govindraju]

Hi Marc,

On 12/04/21 3:48 pm, Marc Kleine-Budde wrote:
> On 4/9/21 3:40 PM, Aswath Govindraju wrote:
>> The driver adds support for generic CAN transceivers. Currently
>> the modes supported by this driver are standby and normal modes for TI
>> TCAN1042 and TCAN1043 CAN transceivers.
>>
>> The transceiver is modelled as a phy with pins controlled by gpios, to put
>> the transceiver in various device functional modes. It also gets the phy
>> attribute max_link_rate for the usage of m_can drivers.
> 
> This driver should be independent of CAN driver, so you should not mention a
> specific driver here.
> 

I will substitute m_can with can in the respin.

>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  drivers/phy/Kconfig               |   9 ++
>>  drivers/phy/Makefile              |   1 +
>>  drivers/phy/phy-can-transceiver.c | 140 ++++++++++++++++++++++++++++++
>>  3 files changed, 150 insertions(+)
>>  create mode 100644 drivers/phy/phy-can-transceiver.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 54c1f2f0985f..51902b629fc6 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -61,6 +61,15 @@ config USB_LGM_PHY
>>  	  interface to interact with USB GEN-II and USB 3.x PHY that is part
>>  	  of the Intel network SOC.
>>  
>> +config PHY_CAN_TRANSCEIVER
>> +	tristate "CAN transceiver PHY"
>> +	select GENERIC_PHY
>> +	help
>> +	  This option enables support for CAN transceivers as a PHY. This
>> +	  driver provides function for putting the transceivers in various
>> +	  functional modes using gpios and sets the attribute max link
>> +	  rate, for mcan drivers.
>> +
>>  source "drivers/phy/allwinner/Kconfig"
>>  source "drivers/phy/amlogic/Kconfig"
>>  source "drivers/phy/broadcom/Kconfig"
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index adac1b1a39d1..9c66101c9605 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -9,6 +9,7 @@ obj-$(CONFIG_PHY_LPC18XX_USB_OTG)	+= phy-lpc18xx-usb-otg.o
>>  obj-$(CONFIG_PHY_XGENE)			+= phy-xgene.o
>>  obj-$(CONFIG_PHY_PISTACHIO_USB)		+= phy-pistachio-usb.o
>>  obj-$(CONFIG_USB_LGM_PHY)		+= phy-lgm-usb.o
>> +obj-$(CONFIG_PHY_CAN_TRANSCEIVER)	+= phy-can-transceiver.o
>>  obj-y					+= allwinner/	\
>>  					   amlogic/	\
>>  					   broadcom/	\
>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>> new file mode 100644
>> index 000000000000..14496f6e1666
>> --- /dev/null
>> +++ b/drivers/phy/phy-can-transceiver.c
>> @@ -0,0 +1,140 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * phy-can-transceiver.c - phy driver for CAN transceivers
>> + *
>> + * Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com
>> + *
>> + */
>> +#include<linux/phy/phy.h>
>> +#include<linux/platform_device.h>
>> +#include<linux/module.h>
>> +#include<linux/gpio.h>
>> +#include<linux/gpio/consumer.h>
>> +
>> +struct can_transceiver_data {
>> +	u32 flags;
>> +#define STB_PRESENT	BIT(0)
>> +#define EN_PRESENT	BIT(1)
> 
> please add a common prefix to the defines

I will add a common prefix(GPIO) in the respin.

> 
>> +};
>> +
>> +struct can_transceiver_phy {
>> +	struct phy *generic_phy;
>> +	struct gpio_desc *standby_gpio;
>> +	struct gpio_desc *enable_gpio;
>> +};
>> +
>> +/* Power on function */
>> +static int can_transceiver_phy_power_on(struct phy *phy)
>> +{
>> +	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>> +
>> +	if (can_transceiver_phy->standby_gpio)
>> +		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>> +	if (can_transceiver_phy->enable_gpio)
>> +		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 1);
> 
> Please add a newline before the return.
> 

Will make this change in the respin.

>> +	return 0;
>> +}
>> +
>> +/* Power off function */
>> +static int can_transceiver_phy_power_off(struct phy *phy)
>> +{
>> +	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>> +
>> +	if (can_transceiver_phy->standby_gpio)
>> +		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 1);
>> +	if (can_transceiver_phy->enable_gpio)
>> +		gpiod_set_value_cansleep(can_transceiver_phy->enable_gpio, 0);
> 
> same here
> 

Will make this change in the respin

>> +	return 0;
>> +}
>> +
>> +static const struct phy_ops can_transceiver_phy_ops = {
>> +	.power_on	= can_transceiver_phy_power_on,
>> +	.power_off	= can_transceiver_phy_power_off,
>> +	.owner		= THIS_MODULE,
>> +};
>> +
>> +static const struct can_transceiver_data tcan1042_drvdata = {
>> +	.flags = STB_PRESENT,
>> +};
>> +
>> +static const struct can_transceiver_data tcan1043_drvdata = {
>> +	.flags = STB_PRESENT | EN_PRESENT,
>> +};
>> +
>> +static const struct of_device_id can_transceiver_phy_ids[] = {
>> +	{
>> +		.compatible = "ti,tcan1042",
>> +		.data = &tcan1042_drvdata
>> +	},
>> +	{
>> +		.compatible = "ti,tcan1043",
>> +		.data = &tcan1043_drvdata
>> +	},
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
>> +
>> +int can_transceiver_phy_probe(struct platform_device *pdev)
>> +{
>> +	struct phy_provider *phy_provider;
>> +	struct device *dev = &pdev->dev;
>> +	struct can_transceiver_phy *can_transceiver_phy;
>> +	const struct can_transceiver_data *drvdata;
>> +	const struct of_device_id *match;
>> +	struct phy *phy;
>> +	struct gpio_desc *standby_gpio;
>> +	struct gpio_desc *enable_gpio;
>> +	u32 max_bitrate = 0;
>> +
>> +	can_transceiver_phy = devm_kzalloc(dev, sizeof(struct can_transceiver_phy), GFP_KERNEL);
> 
> error handling?
> 

Will add this in the respin.

>> +
>> +	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>> +	drvdata = match->data;
>> +
>> +	phy = devm_phy_create(dev, dev->of_node,
>> +			      &can_transceiver_phy_ops);
>> +	if (IS_ERR(phy)) {
>> +		dev_err(dev, "failed to create can transceiver phy\n");
>> +		return PTR_ERR(phy);
>> +	}
>> +
>> +	device_property_read_u32(dev, "max-bitrate", &max_bitrate);
>> +	phy->attrs.max_link_rate = max_bitrate / 1000000;
> 
> The problem is, there are CAN transceivers with a max of 83.3 kbit/s or 125 kbit/s.
> 

The only way that I was able to find for this is to add a phy attribute
"max_bit_rate" in include/linux/phy/phy.h. Would this be an acceptable
solution ?

>> +	can_transceiver_phy->generic_phy = phy;
>> +
>> +	if (drvdata->flags & STB_PRESENT) {
>> +		standby_gpio = devm_gpiod_get(dev, "standby",   GPIOD_OUT_LOW);
> 
> please use only one space after the ",".

Will correct this in respin.

> Why do you request the gpio standby low?

While probing the transceiver has to be in standby state and only after
calling the power on does the transceiver go to enable state. This was
the reason behind requesting gpio standby low.

Thank you for for review.

Regards,
Aswath

> 
>> +		if (IS_ERR(standby_gpio))
>> +			return PTR_ERR(standby_gpio);
>> +		can_transceiver_phy->standby_gpio = standby_gpio;
>> +	}
>> +
>> +	if (drvdata->flags & EN_PRESENT) {
>> +		enable_gpio = devm_gpiod_get(dev, "enable",   GPIOD_OUT_LOW);
>> +		if (IS_ERR(enable_gpio))
>> +			return PTR_ERR(enable_gpio);
>> +		can_transceiver_phy->enable_gpio = enable_gpio;
>> +	}
>> +
>> +	phy_set_drvdata(can_transceiver_phy->generic_phy, can_transceiver_phy);
>> +
>> +	phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>> +
>> +	return PTR_ERR_OR_ZERO(phy_provider);
>> +}
>> +
>> +static struct platform_driver can_transceiver_phy_driver = {
>> +	.probe = can_transceiver_phy_probe,
>> +	.driver = {
>> +		.name = "can-transceiver-phy",
>> +		.of_match_table = can_transceiver_phy_ids,
>> +	},
>> +};
>> +
>> +module_platform_driver(can_transceiver_phy_driver);
>> +
>> +MODULE_AUTHOR("Faiz Abbas <faiz_abbas@ti.com>");
>> +MODULE_AUTHOR("Aswath Govindraju <a-govindraju@ti.com>");
>> +MODULE_DESCRIPTION("CAN TRANSCEIVER PHY driver");
>> +MODULE_LICENSE("GPL v2");
>>
> 
> marc
> 

Re: [PATCH 3/4] dt-bindings: net: can: Document transceiver implementation as phy

[Aswath Govindraju]

Hi Rob,

On 12/04/21 11:21 pm, Rob Herring wrote:
> On Fri, Apr 09, 2021 at 07:10:53PM +0530, Aswath Govindraju wrote:
>> From: Faiz Abbas <faiz_abbas@ti.com>
>>
>> Some transceivers need a configuration step (for example, pulling the
>> standby or enable lines) for them to start sending messages. The
>> transceiver can be implemented as a phy with the configuration done in the
>> phy driver. The bit rate limitation can the be obtained by the driver using
>> the phy node.
>>
>> Document the above implementation in the bosch mcan bindings
>>
>> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>  Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
>> index 798fa5fb7bb2..2c01899b1a3e 100644
>> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
>> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
>> @@ -109,6 +109,12 @@ properties:
>>    can-transceiver:
>>      $ref: can-transceiver.yaml#
>>  
>> +  phys:
>> +    minItems: 1
> 
> maxItems: 1

Will add this in the respin.

> 
>> +
>> +  phy-names:
>> +    const: can_transceiver
> 
> Kind of a pointless name. You don't really need a name if there's a 
> single entry.
> 

This name used by devm_phy_optional_get() in m_can driver to get the phy
data structure.

Thank you for the review.

Regards,
Aswath

>> +
>>  required:
>>    - compatible
>>    - reg
>> -- 
>> 2.17.1
>>

Re: [PATCH 2/4] phy: phy-can-transceiver: Add support for generic CAN transceiver driver

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4351 bytes --]

On 14.04.2021 11:54:36, Aswath Govindraju wrote:
> Hi Marc,
> 
> On 12/04/21 3:48 pm, Marc Kleine-Budde wrote:
> > On 4/9/21 3:40 PM, Aswath Govindraju wrote:
> >> The driver adds support for generic CAN transceivers. Currently
> >> the modes supported by this driver are standby and normal modes for TI
> >> TCAN1042 and TCAN1043 CAN transceivers.
> >>
> >> The transceiver is modelled as a phy with pins controlled by gpios, to put
> >> the transceiver in various device functional modes. It also gets the phy
> >> attribute max_link_rate for the usage of m_can drivers.
> > 
> > This driver should be independent of CAN driver, so you should not mention a
> > specific driver here.
> > 
> 
> I will substitute m_can with can in the respin.

Better use uppercase CAN instead of can.

[...]

> >> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> >> new file mode 100644
> >> index 000000000000..14496f6e1666
> >> --- /dev/null
> >> +++ b/drivers/phy/phy-can-transceiver.c
> >> @@ -0,0 +1,140 @@
> >> +// SPDX-License-Identifier: GPL-2.0
> >> +/*
> >> + * phy-can-transceiver.c - phy driver for CAN transceivers
> >> + *
> >> + * Copyright (C) 2021 Texas Instruments Incorporated - http://www.ti.com
> >> + *
> >> + */
> >> +#include<linux/phy/phy.h>
> >> +#include<linux/platform_device.h>
> >> +#include<linux/module.h>
> >> +#include<linux/gpio.h>
> >> +#include<linux/gpio/consumer.h>
> >> +
> >> +struct can_transceiver_data {
> >> +	u32 flags;
> >> +#define STB_PRESENT	BIT(0)
> >> +#define EN_PRESENT	BIT(1)
> > 
> > please add a common prefix to the defines
> 
> I will add a common prefix(GPIO) in the respin.

I was thinking about something like CAN_TRANSCEIVER_

[...]

> >> +int can_transceiver_phy_probe(struct platform_device *pdev)
> >> +{
> >> +	struct phy_provider *phy_provider;
> >> +	struct device *dev = &pdev->dev;
> >> +	struct can_transceiver_phy *can_transceiver_phy;
> >> +	const struct can_transceiver_data *drvdata;
> >> +	const struct of_device_id *match;
> >> +	struct phy *phy;
> >> +	struct gpio_desc *standby_gpio;
> >> +	struct gpio_desc *enable_gpio;
> >> +	u32 max_bitrate = 0;
> >> +
> >> +	can_transceiver_phy = devm_kzalloc(dev, sizeof(struct can_transceiver_phy), GFP_KERNEL);
> > 
> > error handling?
> > 
> 
> Will add this in the respin.
> 
> >> +
> >> +	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
> >> +	drvdata = match->data;
> >> +
> >> +	phy = devm_phy_create(dev, dev->of_node,
> >> +			      &can_transceiver_phy_ops);
> >> +	if (IS_ERR(phy)) {
> >> +		dev_err(dev, "failed to create can transceiver phy\n");
> >> +		return PTR_ERR(phy);
> >> +	}
> >> +
> >> +	device_property_read_u32(dev, "max-bitrate", &max_bitrate);
> >> +	phy->attrs.max_link_rate = max_bitrate / 1000000;
> > 
> > The problem is, there are CAN transceivers with a max of 83.3 kbit/s or 125 kbit/s.
> > 
> 
> The only way that I was able to find for this is to add a phy attribute
> "max_bit_rate" in include/linux/phy/phy.h. Would this be an acceptable
> solution ?

I think that's up to the phy people.

Another solution would be to have a public struct can_transceiver:

| struct can_transceiver { 
| 	struct phy *generic_phy;
|       u32 max_bitrate;
| };

which holds the max_bitrate. In the CAN controller driver you can use
container_of to get that struct and access the max_bitrate.

> >> +	can_transceiver_phy->generic_phy = phy;
> >> +
> >> +	if (drvdata->flags & STB_PRESENT) {
> >> +		standby_gpio = devm_gpiod_get(dev, "standby",   GPIOD_OUT_LOW);
> > 
> > please use only one space after the ",".
> 
> Will correct this in respin.
> 
> > Why do you request the gpio standby low?
> 
> While probing the transceiver has to be in standby state and only after
> calling the power on does the transceiver go to enable state. This was
> the reason behind requesting gpio standby low.

This isn't consistent with the power_on and power_off functions.

regards,
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 0/4] CAN TRANSCEIVER: Add support for CAN transceivers

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1190 bytes --]

On 09.04.2021 19:10:50, Aswath Govindraju wrote:
> The following series of patches add support for CAN transceivers.
> 
> TCAN1042 has a standby signal that needs to be pulled high for
> sending/receiving messages[1]. TCAN1043 has a enable signal along with
> standby signal that needs to be pulled up for sending/receiving
> messages[2], and other combinations of the two lines can be used to put the
> transceiver in different states to reduce power consumption. On boards
> like the AM654-idk and J721e-evm these signals are controlled using gpios.
> 
> Patch 1 models the transceiver as a phy device tree node with properties
> for max bit rate supported, gpio properties for indicating gpio pin numbers
> to which standby and enable signals are connected.

Please add a patch that adds the driver and the bindings to the
MAINTAINERS file. Feel free to add it CAN NETWORK DRIVERS.

regards,
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] dt-bindings: phy: Add binding for TI TCAN104x CAN transceivers

[Aswath Govindraju]

Hi Marc,

On 13/04/21 6:45 pm, Rob Herring wrote:
> On Tue, Apr 13, 2021 at 2:41 AM Marc Kleine-Budde <mkl@pengutronix.de> wrote:
>>
>> On 12.04.2021 12:49:56, Rob Herring wrote:
>>> On Mon, Apr 12, 2021 at 12:19:30PM +0200, Marc Kleine-Budde wrote:
>>>> On 4/9/21 3:40 PM, Aswath Govindraju wrote:
>>>>> Add binding documentation for TI TCAN104x CAN transceivers.
>>>>>
>>>>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>>>>> ---
>>>>>  .../bindings/phy/ti,tcan104x-can.yaml         | 56 +++++++++++++++++++
>>>>>  1 file changed, 56 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..4abfc30a97d0
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/phy/ti,tcan104x-can.yaml
>>>>> @@ -0,0 +1,56 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: "http://devicetree.org/schemas/phy/ti,tcan104x-can.yaml#"
>>>>> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
>>>>> +
>>>>> +title: TCAN104x CAN TRANSCEIVER PHY
>>>>> +
>>>>> +maintainers:
>>>>> +  - Aswath Govindraju <a-govindraju@ti.com>
>>>>> +
>>>>> +properties:
>>>>> +  $nodename:
>>>>> +    pattern: "^tcan104x-phy"
>>>>> +
>>>>> +  compatible:
>>>>> +    enum:
>>>>> +      - ti,tcan1042
>>>>> +      - ti,tcan1043
>>>>
>>>> Can you create a generic standby only and a generic standby and enable transceiver?
>>>
>>> As a fallback compatible fine, but no generic binding please. A generic
>>> binding can't describe any timing requirements between the 2 GPIO as
>>> well as supplies when someone wants to add those (and they will).
>>
>> Right - that makes sense.
>>

So, I need not add any new compatible strings right because as a
fallback the existing compatible strings can be used, as there are no
timing constraints on them.

Thanks,
Aswath

>>>>> +
>>>>> +  '#phy-cells':
>>>>> +    const: 0
>>>>> +
>>>>> +  standby-gpios:
>>>>> +    description:
>>>>> +      gpio node to toggle standby signal on transceiver
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  enable-gpios:
>>>>> +    description:
>>>>> +      gpio node to toggle enable signal on transceiver
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  max-bitrate:
>>>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>>>> +    description:
>>>>> +      max bit rate supported in bps
>>>
>>> We already have 'max-speed' for serial devices, use that.
>>
>> There is already the neither Ethernet PHY (PHYLINK/PHYLIB) nor generic
>> PHY (GENERIC_PHY) can-transceiver binding
>> Documentation/devicetree/bindings/net/can/can-transceiver.yaml which
>> specifies max-bitrate. I don't have strong feelings whether to use
>> max-bitrate or max-speed.
> 
> Okay, max-bitrate is fine.
> 
>>
>> Speaking about Ethernet PHYs, what are to pros and cons to use the
>> generic PHY compared to the Ethernet PHY infrastructure?
> 
> For higher speed ethernet, both are used. There's the serdes phy and
> the ethernet phy with serdes phy using the generic phy binding. For
> CAN, it probably comes down to what's a better fit.
> 
> Rob
> 

Re: [PATCH 3/4] dt-bindings: net: can: Document transceiver implementation as phy

[Rob Herring]

On Wed, Apr 14, 2021 at 1:49 AM Aswath Govindraju <a-govindraju@ti.com> wrote:
>
> Hi Rob,
>
> On 12/04/21 11:21 pm, Rob Herring wrote:
> > On Fri, Apr 09, 2021 at 07:10:53PM +0530, Aswath Govindraju wrote:
> >> From: Faiz Abbas <faiz_abbas@ti.com>
> >>
> >> Some transceivers need a configuration step (for example, pulling the
> >> standby or enable lines) for them to start sending messages. The
> >> transceiver can be implemented as a phy with the configuration done in the
> >> phy driver. The bit rate limitation can the be obtained by the driver using
> >> the phy node.
> >>
> >> Document the above implementation in the bosch mcan bindings
> >>
> >> Signed-off-by: Faiz Abbas <faiz_abbas@ti.com>
> >> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
> >> ---
> >>  Documentation/devicetree/bindings/net/can/bosch,m_can.yaml | 6 ++++++
> >>  1 file changed, 6 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> >> index 798fa5fb7bb2..2c01899b1a3e 100644
> >> --- a/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> >> +++ b/Documentation/devicetree/bindings/net/can/bosch,m_can.yaml
> >> @@ -109,6 +109,12 @@ properties:
> >>    can-transceiver:
> >>      $ref: can-transceiver.yaml#
> >>
> >> +  phys:
> >> +    minItems: 1
> >
> > maxItems: 1
>
> Will add this in the respin.
>
> >
> >> +
> >> +  phy-names:
> >> +    const: can_transceiver
> >
> > Kind of a pointless name. You don't really need a name if there's a
> > single entry.
> >
>
> This name used by devm_phy_optional_get() in m_can driver to get the phy
> data structure.

With no name, you'll get the 1st one. Looks like the phy subsystem
warns on this. That's wrong, so please fix that.

Rob