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

[Marc Kleine-Budde <mkl@pengutronix.de>]

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