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 <[email protected]>
---
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);
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)) {
--
2.17.1
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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)) {
>>