Hi Peter,

On 25/11/21 7:37 pm, Peter Rosin wrote:

Hi!

On 2021-11-23 09:12, Aswath Govindraju wrote:

quoted

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 <redacted>
---
 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.

Thank you for pointing this out. I did forget to think about this case.
However, as you mentioned the phy layer does indeed only call the
can_transceiver_phy_power_off only if can_transceiver_phy_power_on was
called earlier and this is being done using power_count,

int phy_power_off(struct phy *phy)
{
        int ret;

        if (!phy)
                return 0;

        mutex_lock(&phy->mutex);
        if (phy->power_count == 1 && phy->ops->power_off) {
                ret =  phy->ops->power_off(phy);
                if (ret < 0) {
                        dev_err(&phy->dev, "phy poweroff failed -->
%d\n", ret);
                        mutex_unlock(&phy->mutex);
                        return ret;
                }
        }
        --phy->power_count;
        mutex_unlock(&phy->mutex);
        phy_pm_runtime_put(phy);

        if (phy->pwr)
                regulator_disable(phy->pwr);

        return 0;
}

Thanks,
Aswath

Cheers,
Peter

quoted

 
 	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)) {

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy