linux-kernel - Re: [PATCH RFC 2/2] phy: phy-can-transceiver: Add support for setting mux

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]

Date:   Fri, 12 Nov 2021 19:18:54 +0530
From:   Aswath Govindraju <a-govindraju@...com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>
CC:     Vignesh Raghavendra <vigneshr@...com>,
        Kishon Vijay Abraham I <kishon@...com>,
        Nishanth Menon <nm@...com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        Vinod Koul <vkoul@...nel.org>,
        Rob Herring <robh+dt@...nel.org>, <linux-can@...r.kernel.org>,
        <linux-phy@...ts.infradead.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Peter Rosin <peda@...ntia.se>
Subject: Re: [PATCH RFC 2/2] phy: phy-can-transceiver: Add support for setting
 mux

Hi Marc,

On 12/11/21 2:10 pm, Marc Kleine-Budde wrote:
> On 11.11.2021 22:13:12, Aswath Govindraju wrote:
>> On some boards, for routing CAN signals from controller to transceiver,
>> muxes might need to be set. Therefore, add support for setting the mux by
>> reading the mux-controls property from the device tree node.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
>> ---
>>  drivers/phy/phy-can-transceiver.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
>> index 6f3fe37dee0e..3d8da5226e27 100644
>> --- a/drivers/phy/phy-can-transceiver.c
>> +++ b/drivers/phy/phy-can-transceiver.c
>> @@ -10,6 +10,7 @@
>>  #include<linux/module.h>
>>  #include<linux/gpio.h>
>>  #include<linux/gpio/consumer.h>
>> +#include <linux/mux/consumer.h>
>>  
>>  struct can_transceiver_data {
>>  	u32 flags;
>> @@ -21,13 +22,22 @@ struct can_transceiver_phy {
>>  	struct phy *generic_phy;
>>  	struct gpio_desc *standby_gpio;
>>  	struct gpio_desc *enable_gpio;
>> +	struct mux_control *mux_ctrl;
>>  };
>>  
>>  /* Power on function */
>>  static int can_transceiver_phy_power_on(struct phy *phy)
>>  {
>> +	int ret;
>>  	struct can_transceiver_phy *can_transceiver_phy = phy_get_drvdata(phy);
>>  
>> +	if (can_transceiver_phy->mux_ctrl) {
>> +		ret = mux_control_select(can_transceiver_phy->mux_ctrl, 1);
> 
> Hard coding the "1" looks wrong here. I have seen some boards where you
> can select between a CAN-2.0 and a single wire CAN transceiver with a
> mux. So I think we cannot hard code the "1" here.
> 

Yes, as you mentioned it is not ideal to hard code "1". I feel that, it
would be much better to read the state of the mux to be set from the
mux-controls property. The issue that I see with this approach is that
the current implementation in the mux framework only allows for one
argument, which is for indicating the line to be toggled in the mux. If
more arguments are added then an error is returned from the
"mux_control_get". I am not sure why this limitation was added.


>> +		if (ret) {
>> +			dev_err(&phy->dev, "Failed to select CAN mux: %d\n", ret);
>> +			return ret;
>> +		}
>> +	}
>>  	if (can_transceiver_phy->standby_gpio)
>>  		gpiod_set_value_cansleep(can_transceiver_phy->standby_gpio, 0);
>>  	if (can_transceiver_phy->enable_gpio)
>> @@ -45,6 +55,8 @@ static int can_transceiver_phy_power_off(struct phy *phy)
>>  		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);
>> +	if (can_transceiver_phy->mux_ctrl)
>> +		mux_control_deselect(can_transceiver_phy->mux_ctrl);
>>  
>>  	return 0;
>>  }
>> @@ -95,6 +107,15 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
>>  	match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
>>  	drvdata = match->data;
>>  
>> +	if (of_property_read_bool(dev->of_node, "mux-controls")) {
> 
> Is this the proper way of doing this? Looks like we need a
> devm_mux_control_get_optional(), which doesn't return a -ENODEV if the
> device doesn't exist.
> 
> Cc'ed Peter Rosin.
> 
>> +		struct mux_control *control;
>> +
>> +		control = devm_mux_control_get(dev, NULL);
>> +		if (IS_ERR(control))
>> +			return PTR_ERR(control);
> 
> What about making use of dev_err_probe()?
> 

Sure, I will make this change.

Thank you for the comments.

Regards,
Aswath

>> +		can_transceiver_phy->mux_ctrl = control;
>> +	}
>> +
>>  	phy = devm_phy_create(dev, dev->of_node,
>>  			      &can_transceiver_phy_ops);
>>  	if (IS_ERR(phy)) {
>> -- 
>> 2.17.1
>>
>>
> 
> Regards,
> Marc
>