| Message ID | 20200122080310.24653-1-faiz_abbas@ti.com |
|---|---|
| Headers | show |
| Series | Add Support for MCAN in AM654x-idk | expand |
> 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
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
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
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
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(+)