| Message ID | 6bcfde63b3a6b25640a56be2e24a357e41f8400f.1742390569.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | phy: can-transceiver: Re-instate "mux-states" property presence check | expand |
For some reasons, I received your message twice (with a two minutes interval between both messages). These look identical. I am answering the most recent. :) On 19/03/2025 at 22:27, 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. Hmmm, I understand why you are doing this patch. But on the long term, wouldn't it make more sense to have a devm_mux_state_get_optional()? Or maybe add a property somewhere to inform devm_mux_state_get() that this is optional? Regardless, just see this as an open question. I am OK with the approach of your patch. > Fix this by re-instating the property presence check. > > This is bascially a revert of commit d02dfd4ceb2e9f34 ("phy: > can-transceiver: Drop unnecessary "mux-states" property presence > check"), with two changes: > 1. Use the proper API for checking whether a property is present, > 2. Do not print an error message, as the mux core already takes care > of that. > > Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check")> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Notwithstanding of above comment: Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Yours sincerely, Vincent Mailhol
On 3/19/25 4:27 PM, 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 is bascially a revert of commit d02dfd4ceb2e9f34 ("phy: Basically. :-) > can-transceiver: Drop unnecessary "mux-states" property presence > check"), with two changes: > 1. Use the proper API for checking whether a property is present, > 2. Do not print an error message, as the mux core already takes care > of that. > > 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... [...] MBR, Sergey
On 19/03/2025 at 23:06, Vincent Mailhol wrote: > For some reasons, I received your message twice (with a two minutes > interval between both messages). These look identical. I am answering > the most recent. :) > > On 19/03/2025 at 22:27, 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. > > Hmmm, I understand why you are doing this patch. But on the long term, > wouldn't it make more sense to have a devm_mux_state_get_optional()? Or > maybe add a property somewhere to inform devm_mux_state_get() that this > is optional? > > Regardless, just see this as an open question. I am OK with the approach > of your patch. Ah, and I just realized that you mentioned the exact same thing under the --- cutter, which for some reasons my eyes refused to see. Sorry for the noise. >> Fix this by re-instating the property presence check. >> >> This is bascially a revert of commit d02dfd4ceb2e9f34 ("phy: >> can-transceiver: Drop unnecessary "mux-states" property presence >> check"), with two changes: >> 1. Use the proper API for checking whether a property is present, >> 2. Do not print an error message, as the mux core already takes care >> of that. >> >> Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check")> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Notwithstanding of above comment: > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Yours sincerely, Vincent Mailhol
Hi Vincent, On Wed, 19 Mar 2025 at 15:07, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote: > For some reasons, I received your message twice (with a two minutes > interval between both messages). These look identical. I am answering My scripting didn't handle the comment in Rob's address correctly, so I resent the patch with the fixed address. > the most recent. :) Good ;-) > On 19/03/2025 at 22:27, 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. > > Hmmm, I understand why you are doing this patch. But on the long term, > wouldn't it make more sense to have a devm_mux_state_get_optional()? Or > maybe add a property somewhere to inform devm_mux_state_get() that this > is optional? > > Regardless, just see this as an open question. I am OK with the approach > of your patch. Alternatively, we can be proactive and add a temporary local wrapper: /* Dummy wrapper until optional muxes are supported */ 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); } and call that instead? Then the probe function needs no future changes when the real devm_mux_state_get_optional() arrives. > > Fix this by re-instating the property presence check. > > > > This is bascially a revert of commit d02dfd4ceb2e9f34 ("phy: > > can-transceiver: Drop unnecessary "mux-states" property presence > > check"), with two changes: > > 1. Use the proper API for checking whether a property is present, > > 2. Do not print an error message, as the mux core already takes care > > of that. > > > > Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check")> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Notwithstanding of above comment: > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Thanks! Gr{oetje,eeting}s, Geert
On 20/03/2025 at 19:25, Geert Uytterhoeven wrote: > Hi Vincent, (...) >> On 19/03/2025 at 22:27, 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. >> >> Hmmm, I understand why you are doing this patch. But on the long term, >> wouldn't it make more sense to have a devm_mux_state_get_optional()? Or >> maybe add a property somewhere to inform devm_mux_state_get() that this >> is optional? >> >> Regardless, just see this as an open question. I am OK with the approach >> of your patch. > > Alternatively, we can be proactive and add a temporary local wrapper: > > /* Dummy wrapper until optional muxes are supported */ > 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); > } > > and call that instead? Then the probe function needs no future changes > when the real devm_mux_state_get_optional() arrives. This looks like a more elegant and more long term solution! Yours sincerely, Vincent Mailhol
Hi Geert, > -----Original Message----- > From: Geert Uytterhoeven <geert+renesas@glider.be> > Sent: 19 March 2025 13:26 > Subject: [PATCH] phy: can-transceiver: Re-instate "mux-states" property presence check > > 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 is bascially a revert of commit d02dfd4ceb2e9f34 ("phy: > can-transceiver: Drop unnecessary "mux-states" property presence check"), with two changes: > 1. Use the proper API for checking whether a property is present, > 2. Do not print an error message, as the mux core already takes care > of that. > > Fixes: d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence > check") > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> Tested-by: Biju Das <biju.das.jz@bp.renesas.com> Tested on RZ/G3E SMARC EVK that has tcan1046v-q1 that is modelled as two instances of tcan1042. Cheers, Biju > --- > Alternatively, the multiplexer subsystem needs to gain support for getting an optional mux... > --- > drivers/phy/phy-can-transceiver.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c > index 2bec70615449f94d..539b3446b9c33eed 100644 > --- a/drivers/phy/phy-can-transceiver.c > +++ b/drivers/phy/phy-can-transceiver.c > @@ -103,7 +103,6 @@ static int can_transceiver_phy_probe(struct platform_device *pdev) > struct phy *phy; > struct gpio_desc *standby_gpio; > struct gpio_desc *enable_gpio; > - struct mux_state *mux_state; > u32 max_bitrate = 0; > int err; > > @@ -114,11 +113,13 @@ 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) > + if (of_property_present(dev->of_node, "mux-states")) { > + struct mux_state *mux_state; > + > + mux_state = devm_mux_state_get(dev, NULL); > + if (IS_ERR(mux_state)) > return PTR_ERR(mux_state); > - } else { > + > can_transceiver_phy->mux_state = mux_state; > } > > -- > 2.43.0 >
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c index 2bec70615449f94d..539b3446b9c33eed 100644 --- a/drivers/phy/phy-can-transceiver.c +++ b/drivers/phy/phy-can-transceiver.c @@ -103,7 +103,6 @@ static int can_transceiver_phy_probe(struct platform_device *pdev) struct phy *phy; struct gpio_desc *standby_gpio; struct gpio_desc *enable_gpio; - struct mux_state *mux_state; u32 max_bitrate = 0; int err; @@ -114,11 +113,13 @@ 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) + if (of_property_present(dev->of_node, "mux-states")) { + struct mux_state *mux_state; + + mux_state = devm_mux_state_get(dev, NULL); + if (IS_ERR(mux_state)) return PTR_ERR(mux_state); - } else { + can_transceiver_phy->mux_state = mux_state; }
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 is bascially a revert of commit d02dfd4ceb2e9f34 ("phy: can-transceiver: Drop unnecessary "mux-states" property presence check"), with two changes: 1. Use the proper API for checking whether a property is present, 2. Do not print an error message, as the mux core already takes care of that. 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... --- drivers/phy/phy-can-transceiver.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)