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