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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210414065743.36cemub3ag7rzrvk@pengutronix.de>
Date:   Wed, 14 Apr 2021 08:57:43 +0200
From:   Marc Kleine-Budde <mkl@...gutronix.de>
To:     Aswath Govindraju <a-govindraju@...com>
Cc:     Vignesh Raghavendra <vigneshr@...com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Lokesh Vutla <lokeshvutla@...com>,
        Grygorii Strashko <grygorii.strashko@...com>,
        Chandrasekar Ramakrishnan <rcsekar@...sung.com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Vinod Koul <vkoul@...nel.org>,
        Sriram Dash <sriram.dash@...sung.com>,
        linux-can@...r.kernel.org, netdev@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-phy@...ts.infradead.org
Subject: Re: [PATCH 2/4] phy: phy-can-transceiver: Add support for generic
 CAN transceiver driver

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 |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)