On 14.04.2021 11:54:36, Aswath Govindraju wrote:

Hi Marc,

On 12/04/21 3:48 pm, Marc Kleine-Budde wrote:

quoted

On 4/9/21 3:40 PM, Aswath Govindraju wrote:

quoted

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.

[...]

quoted

quoted

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_

[...]

quoted

quoted

+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.

quoted

quoted

+
+	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.

quoted

quoted

+	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.

quoted

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 |