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:
signature.asc
Description: PGP signature
