Re: [PATCH RFC v3 4/4] phy: phy-can-transceiver: Add support for setting mux

[Peter Rosin <peda@axentia.se>]

Hi!

On 2021-11-23 09: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@ti.com>
> ---
>  drivers/phy/Kconfig               |  1 +
>  drivers/phy/phy-can-transceiver.c | 22 ++++++++++++++++++++++
>  2 files changed, 23 insertions(+)
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 82b63e60c5a2..300b0f2b5f84 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -64,6 +64,7 @@ config USB_LGM_PHY
>  config PHY_CAN_TRANSCEIVER
>  	tristate "CAN transceiver PHY"
>  	select GENERIC_PHY
> +	select MULTIPLEXER
>  	help
>  	  This option enables support for CAN transceivers as a PHY. This
>  	  driver provides function for putting the transceivers in various
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 6f3fe37dee0e..6c94e3846410 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_state *mux_state;
>  };
>  
>  /* 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_state) {
> +		ret = mux_state_select(can_transceiver_phy->mux_state);
> +		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_state)
> +		mux_state_deselect(can_transceiver_phy->mux_state);

Careful, it is not obvious that you are following the documentation you
added in patch 3/4

+ * Therefore, make sure to call mux_state_deselect() when the operation is
+ * complete and the mux-control is free for others to use, but do not call
+ * mux_state_deselect() if mux_state_select() fails.

Or, are you absolutely certain that can_transceiver_phy_power_off cannot,
in any curcumstance, be called without a successful previous call to can_transceiver_phy_power_on? Or that can_transceiver_phy_power_on will
never ever be called again without a can_transceiver_phy_power_off in
between the two on calls?

If there is any doubt, you need to remember if you have selected/deselected
the mux. Maybe this should be remebered inside struct mux_state so that it
is always safe to call mux_state_select/mux_state_deselect? That's one way
to solve this difficulty.

But then again, the phy layer might ensure that extra precaution is not
needed. But it might also be that it for sure is intended that this is solved
in the phy layer, but that callbacks (or whatever) has been added "after the
fact" and that an extra "on" or "off" has just been just shrugged at.

Cheers,
Peter

>  
>  	return 0;
>  }
> @@ -95,6 +107,16 @@ 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")) {
> +		struct mux_state *mux_state;
> +
> +		mux_state = devm_mux_state_get(dev, NULL);
> +		if (IS_ERR(mux_state))
> +			return dev_err_probe(&pdev->dev, PTR_ERR(mux_state),
> +					     "failed to get mux\n");
> +		can_transceiver_phy->mux_state = mux_state;
> +	}
> +
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);
>  	if (IS_ERR(phy)) {
>