| Message ID | 20211123081222.27979-5-a-govindraju@ti.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | MUX: Add support for reading enable state from DT | expand |
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)) { >
Hi Peter, On 25/11/21 7:37 pm, Peter Rosin wrote: > 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. > 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 > >> >> 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)) { >>
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); 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)) {
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(+)