On the Renesas Gray Hawk Single development board:

    can-transceiver-phy can-phy0: /can-phy0: failed to get mux-state (0)

"mux-states" is an optional property for CAN transceivers.  However,
mux_get() always prints an error message in case of an error, including
when the property is not present, confusing the user.

Fix this by re-instating the property presence check (this time using
the proper API) in a wrapper around devm_mux_state_get().  When the
multiplexer subsystem gains support for optional muxes, the wrapper can
just be removed.

In addition, propagate all real errors upstream, instead of ignoring
them.

Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Alternatively, the multiplexer subsystem needs to gain support for
getting an optional mux...

v2:
  - Add and use devm_mux_state_get_optional(),
  - No given tags added, as the new solution is different.
---
 drivers/phy/phy-can-transceiver.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 2bec70615449f94d..f59caff4b3d4c267 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -93,6 +93,16 @@ static const struct of_device_id can_transceiver_phy_ids[] = {
 };
 MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
 
+/* Temporary wrapper until the multiplexer subsystem supports optional muxes */
+static inline struct mux_state *
+devm_mux_state_get_optional(struct device *dev, const char *mux_name)
+{
+	if (!of_property_present(dev->of_node, "mux-states"))
+		return NULL;
+
+	return devm_mux_state_get(dev, mux_name);
+}
+
 static int can_transceiver_phy_probe(struct platform_device *pdev)
 {
 	struct phy_provider *phy_provider;
@@ -114,13 +124,11 @@ 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;
 
-	mux_state = devm_mux_state_get(dev, NULL);
-	if (IS_ERR(mux_state)) {
-		if (PTR_ERR(mux_state) == -EPROBE_DEFER)
-			return PTR_ERR(mux_state);
-	} else {
-		can_transceiver_phy->mux_state = mux_state;
-	}
+	mux_state = devm_mux_state_get_optional(dev, NULL);
+	if (IS_ERR(mux_state))
+		return PTR_ERR(mux_state);
+
+	can_transceiver_phy->mux_state = mux_state;
 
 	phy = devm_phy_create(dev, dev->of_node,
 			      &can_transceiver_phy_ops);
-- 
2.43.0

On Thu, 20 Mar 2025 16:15:42 +0100, Geert Uytterhoeven wrote:
> On the Renesas Gray Hawk Single development board:
> 
>     can-transceiver-phy can-phy0: /can-phy0: failed to get mux-state (0)
> 
> "mux-states" is an optional property for CAN transceivers.  However,
> mux_get() always prints an error message in case of an error, including
> when the property is not present, confusing the user.
> 
> [...]

Applied, thanks!

[1/1] phy: can-transceiver: Re-instate "mux-states" property presence check
      commit: e153fdea9db04dd0e2e536e2eb125b16bbbc2af7

Best regards,
-- 
~Vinod

On 21/03/2025 at 00:15, Geert Uytterhoeven wrote:
> On the Renesas Gray Hawk Single development board:
> 
>     can-transceiver-phy can-phy0: /can-phy0: failed to get mux-state (0)
> 
> "mux-states" is an optional property for CAN transceivers.  However,
> mux_get() always prints an error message in case of an error, including
> when the property is not present, confusing the user.
> 
> Fix this by re-instating the property presence check (this time using
> the proper API) in a wrapper around devm_mux_state_get().  When the
> multiplexer subsystem gains support for optional muxes, the wrapper can
> just be removed.
> 
> In addition, propagate all real errors upstream, instead of ignoring
> them.

I would like to hear Rob's opinion on this. In d02dfd4ceb2e9f34 Rob
purposely changed the function to ignore errors other than EPROBE_DEFER.

Right now, I do not see a reason not to propagate the other errors, so
I am in favor of this roll back. But maybe we are missing something?

Otherwise, the patch looks good to me. Nice improvement over the v1. I
will wait for Rob's comment to give my review tag.

> Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Alternatively, the multiplexer subsystem needs to gain support for
> getting an optional mux...
> 
> v2:
>   - Add and use devm_mux_state_get_optional(),
>   - No given tags added, as the new solution is different.
> ---
>  drivers/phy/phy-can-transceiver.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 2bec70615449f94d..f59caff4b3d4c267 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -93,6 +93,16 @@ static const struct of_device_id can_transceiver_phy_ids[] = {
>  };
>  MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
>  
> +/* Temporary wrapper until the multiplexer subsystem supports optional muxes */
> +static inline struct mux_state *
> +devm_mux_state_get_optional(struct device *dev, const char *mux_name)
> +{
> +	if (!of_property_present(dev->of_node, "mux-states"))
> +		return NULL;
> +
> +	return devm_mux_state_get(dev, mux_name);
> +}
> +
>  static int can_transceiver_phy_probe(struct platform_device *pdev)
>  {
>  	struct phy_provider *phy_provider;
> @@ -114,13 +124,11 @@ 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;
>  
> -	mux_state = devm_mux_state_get(dev, NULL);
> -	if (IS_ERR(mux_state)) {
> -		if (PTR_ERR(mux_state) == -EPROBE_DEFER)> -			return PTR_ERR(mux_state);
> -	} else {
> -		can_transceiver_phy->mux_state = mux_state;
> -	}
> +	mux_state = devm_mux_state_get_optional(dev, NULL);
> +	if (IS_ERR(mux_state))
> +		return PTR_ERR(mux_state);
> +
> +	can_transceiver_phy->mux_state = mux_state;
>  
>  	phy = devm_phy_create(dev, dev->of_node,
>  			      &can_transceiver_phy_ops);

Yours sincerely,
Vincent Mailhol

On 21/03/2025 at 13:18, Vincent Mailhol wrote:
> On 21/03/2025 at 00:15, Geert Uytterhoeven wrote:
>> On the Renesas Gray Hawk Single development board:
>>
>>     can-transceiver-phy can-phy0: /can-phy0: failed to get mux-state (0)
>>
>> "mux-states" is an optional property for CAN transceivers.  However,
>> mux_get() always prints an error message in case of an error, including
>> when the property is not present, confusing the user.
>>
>> Fix this by re-instating the property presence check (this time using
>> the proper API) in a wrapper around devm_mux_state_get().  When the
>> multiplexer subsystem gains support for optional muxes, the wrapper can
>> just be removed.
>>
>> In addition, propagate all real errors upstream, instead of ignoring
>> them.
> 
> I would like to hear Rob's opinion on this. In d02dfd4ceb2e9f34 Rob
> purposely changed the function to ignore errors other than EPROBE_DEFER.
> 
> Right now, I do not see a reason not to propagate the other errors, so
> I am in favor of this roll back. But maybe we are missing something?
> 
> Otherwise, the patch looks good to me. Nice improvement over the v1. I
> will wait for Rob's comment to give my review tag.

One week has gone, and we did not hear back. So let's assume it is OK.

>> Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check")
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

(...)

Yours sincerely,
Vincent Mailhol