* Re: [PATCH v2 2/2] phy: econet: Add PCIe PHY driver for EcoNet EN751221 and EN7528 SoCs.
From: Vinod Koul @ 2026-05-12 8:12 UTC (permalink / raw)
To: Caleb James DeLisle
Cc: linux-phy, naseefkm, neil.armstrong, robh, krzk+dt, conor+dt,
linux-mips, devicetree, linux-kernel
In-Reply-To: <23a04af8-beec-417d-8d3f-9d587bffe953@cjdns.fr>
On 10-05-26, 13:25, Caleb James DeLisle wrote:
>
> On 10/05/2026 13:16, Vinod Koul wrote:
> > On 04-04-26, 18:49, Caleb James DeLisle wrote:
> > > + val = field_prep(data[i].mask, data[i].val);
> > Please see:
> >
> > https://sashiko.dev/#/patchset/20260425173642.406089-1-cjd%40cjdns.fr
>
>
> I think this is an error in that the AI is not correctly differentiating
> between field_prep() which accepts a non-constant mask, and FIELD_PREP()
> which does not. In any case I can confirm that it does compile and work
> correctly on the device.
Right AI might be confusing FIELD_PREP with field_prep()! I will review
the series now
>
> On another note, I think you may be the creator of Sashiko, if so, thank you
> for your work - it helped me with another patch already.
Oh no, I cant claim credit. It is not me
--
~Vinod
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 2/6] usb: chipidea: tegra: Avoid controller/PHY init if bus is externally controlled
From: Svyatoslav Ryhel @ 2026-05-12 9:14 UTC (permalink / raw)
To: Peter Chen (CIX)
Cc: Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Vinod Koul, Neil Armstrong, Thierry Reding, Jonathan Hunter,
Greg Kroah-Hartman, netdev, devicetree, linux-kernel, linux-phy,
linux-tegra, linux-usb
In-Reply-To: <agLb6mgP45jHjvNt@nchen-desktop>
вт, 12 трав. 2026 р. о 10:51 Peter Chen (CIX) <peter.chen@kernel.org> пише:
>
> On 26-05-12 09:13:40, Svyatoslav Ryhel wrote:
> > вт, 12 трав. 2026 р. о 04:16 Peter Chen (CIX) <peter.chen@kernel.org> пише:
> > >
> > > On 26-05-11 16:56:57, Svyatoslav Ryhel wrote:
> > > > If the USB controller and PHY are externally controlled, then the
> > > > registration of the controller and the PHY initialization should be
> > > > skipped, since these configurations must be done by the device that
> > > > controls the bus to work correctly.
> > > >
> > >
> > > I find you only control USB controller device add at PHY driver, most of USB drivers
> > > has PHY control, for chipidea, it has PHY control at core.c, would please try to
> > > adapt for it?
> > >
> >
> > Usually yes, but this is not the case for Tegra unfortunately. As you
> > can see Tegra specific section of Chipidea driver specifically
> > describes why it has to set PHY manually.
> >
> > /*
> > * USB controller registers shouldn't be touched before PHY is
> > * initialized, otherwise CPU will hang because clocks are gated.
> > * PHY driver controls gating of internal USB clocks on Tegra.
> > */
> >
> > So in order to provide correct work of USB when set by an external
> > device, both PHY and controller init/add must be skipped.
>
> You could call generic PHY APIs at ci_hdrc_tegra.c, after PHY init or power on,
> call controller initialization.
>
And what it will give? Modem used in Tegra devices exposes its output
as USB device on an HSIC line. At the same time modem requires a
precise control when USB should be registered, to be able to register
and unregister it. This cannot be done by linking modem's phy to usb
controller, quite the opposite, controller must be linked to modem's
phy.
> >
> > > Peter
> > >
> > > > Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
> > > > ---
> > > > drivers/usb/chipidea/ci_hdrc_tegra.c | 36 +++++++++++++++++-----------
> > > > 1 file changed, 22 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> > > > index 372788f0f970..593390a818d1 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> > > > @@ -32,6 +32,7 @@ struct tegra_usb {
> > > > struct clk *clk;
> > > >
> > > > bool needs_double_reset;
> > > > + bool externally_controlled;
> > > > };
> > > >
> > > > struct tegra_usb_soc_info {
> > > > @@ -312,20 +313,25 @@ static int tegra_usb_probe(struct platform_device *pdev)
> > > > if (device_property_present(&pdev->dev, "nvidia,needs-double-reset"))
> > > > usb->needs_double_reset = true;
> > > >
> > > > + if (device_property_present(&pdev->dev, "nvidia,external-control"))
> > > > + usb->externally_controlled = true;
> > > > +
> > > > err = tegra_usb_reset_controller(&pdev->dev);
> > > > if (err) {
> > > > dev_err_probe(&pdev->dev, err, "failed to reset controller");
> > > > goto fail_power_off;
> > > > }
> > > >
> > > > - /*
> > > > - * USB controller registers shouldn't be touched before PHY is
> > > > - * initialized, otherwise CPU will hang because clocks are gated.
> > > > - * PHY driver controls gating of internal USB clocks on Tegra.
> > > > - */
> > > > - err = usb_phy_init(usb->phy);
> > > > - if (err)
> > > > - goto fail_power_off;
> > > > + if (!usb->externally_controlled) {
> > > > + /*
> > > > + * USB controller registers shouldn't be touched before PHY is
> > > > + * initialized, otherwise CPU will hang because clocks are gated.
> > > > + * PHY driver controls gating of internal USB clocks on Tegra.
> > > > + */
> > > > + err = usb_phy_init(usb->phy);
> > > > + if (err)
> > > > + goto fail_power_off;
> > > > + }
> > > >
> > > > /* setup and register ChipIdea HDRC device */
> > > > usb->soc = soc;
> > > > @@ -342,12 +348,14 @@ static int tegra_usb_probe(struct platform_device *pdev)
> > > > if (of_usb_get_phy_mode(pdev->dev.of_node) == USBPHY_INTERFACE_MODE_ULPI)
> > > > usb->data.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
> > > >
> > > > - usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> > > > - pdev->num_resources, &usb->data);
> > > > - if (IS_ERR(usb->dev)) {
> > > > - err = dev_err_probe(&pdev->dev, PTR_ERR(usb->dev),
> > > > - "failed to add HDRC device");
> > > > - goto phy_shutdown;
> > > > + if (!usb->externally_controlled) {
> > > > + usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> > > > + pdev->num_resources, &usb->data);
> > > > + if (IS_ERR(usb->dev)) {
> > > > + err = dev_err_probe(&pdev->dev, PTR_ERR(usb->dev),
> > > > + "failed to add HDRC device");
> > > > + goto phy_shutdown;
> > > > + }
> > > > }
> > > >
> > > > return 0;
> > > > --
> > > > 2.51.0
> > > >
> > >
> > > --
> > >
> > > Best regards,
> > > Peter
>
> --
>
> Best regards,
> Peter
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v2] phy: fsl-imx8mq-usb: set usb phy to be wakeup capable
From: Xu Yang @ 2026-05-12 10:10 UTC (permalink / raw)
To: vkoul, neil.armstrong, Frank.Li, s.hauer, kernel, festevam,
jun.li, a.fatoum, franz.schnyder, stefano.radaelli21, linux-phy,
imx, linux-arm-kernel, linux-kernel
USB remote wakeup need its PHY power domain to be active,
so set PHY to be wakeup capable.
Signed-off-by: Li Jun <jun.li@nxp.com>
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
Changes in v2:
- no changes
---
drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
index 958d114b0c83..50b67f1e6a90 100644
--- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
+++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
@@ -750,6 +750,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
"failed to get tca\n");
imx8m_get_phy_tuning_data(imx_phy);
+ device_set_wakeup_capable(dev, true);
phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
--
2.34.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v2] phy: fsl-imx8mq-usb: add control register regmap
From: Xu Yang @ 2026-05-12 10:12 UTC (permalink / raw)
To: vkoul, neil.armstrong, Frank.Li, s.hauer, kernel, festevam,
jun.li, a.fatoum, franz.schnyder, stefano.radaelli21, linux-phy,
imx, linux-arm-kernel, linux-kernel
The CR port is a simple 16-bit data/address parallel port that is
provided for on-chip access to the control registers inside the
USB 3.0 femtoPHY. Add control register regmap and export these
registers by debugfs to help PHY's diagnostic.
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
---
Changes in v2:
- no changes
---
drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 19 ++++++++++++++++++-
1 file changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
index b05d80e849a1..958d114b0c83 100644
--- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
+++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
@@ -1,5 +1,5 @@
// SPDX-License-Identifier: GPL-2.0+
-/* Copyright (c) 2017 NXP. */
+/* Copyright 2017-2026 NXP. */
#include <linux/bitfield.h>
#include <linux/clk.h>
@@ -9,6 +9,7 @@
#include <linux/of.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/regulator/consumer.h>
#include <linux/usb/typec_mux.h>
@@ -55,6 +56,8 @@
#define PHY_CTRL6_ALT_CLK_EN BIT(1)
#define PHY_CTRL6_ALT_CLK_SEL BIT(0)
+#define PHY_CRCTL 0x30
+
#define PHY_TUNE_DEFAULT 0xffffffff
#define TCA_CLK_RST 0x00
@@ -118,6 +121,7 @@ struct imx8mq_usb_phy {
void __iomem *base;
struct regulator *vbus;
struct tca_blk *tca;
+ struct regmap *cr_regmap;
u32 pcs_tx_swing_full;
u32 pcs_tx_deemph_3p5db;
u32 tx_vref_tune;
@@ -685,6 +689,14 @@ static const struct of_device_id imx8mq_usb_phy_of_match[] = {
};
MODULE_DEVICE_TABLE(of, imx8mq_usb_phy_of_match);
+static const struct regmap_config imx_cr_regmap_config = {
+ .name = "cr",
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+ .max_register = 0x7,
+};
+
static int imx8mq_usb_phy_probe(struct platform_device *pdev)
{
struct phy_provider *phy_provider;
@@ -713,6 +725,11 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
if (IS_ERR(imx_phy->base))
return PTR_ERR(imx_phy->base);
+ imx_phy->cr_regmap = devm_regmap_init_mmio(dev, imx_phy->base + PHY_CRCTL,
+ &imx_cr_regmap_config);
+ if (IS_ERR(imx_phy->cr_regmap))
+ return PTR_ERR(imx_phy->cr_regmap);
+
phy_ops = of_device_get_match_data(dev);
if (!phy_ops)
return -EINVAL;
--
2.34.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH v3 0/4] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
From: Andy Shevchenko @ 2026-05-12 13:06 UTC (permalink / raw)
To: Vinod Koul
Cc: Peng Fan, linux-can, linux-phy, linux-kernel, Marc Kleine-Budde,
Vincent Mailhol, Neil Armstrong, Josua Mayer, Ulf Hansson
In-Reply-To: <agIBJsOl4Trk4_KN@vaman>
On Mon, May 11, 2026 at 09:47:42PM +0530, Vinod Koul wrote:
> On 10-05-26, 15:51, Andy Shevchenko wrote:
> > On Sun, May 10, 2026 at 04:21:38PM +0530, Vinod Koul wrote:
> > > On 04-05-26, 08:58, Andy Shevchenko wrote:
> > > > The driver does two things that need to be addressed:
> > > > - includes subject to remove gpio.h
> > > > - checks for error code from device property APIs when it can be done in
> > > > a robust way
> > > >
> > > > This series addresses the above and adds a couple of additional refactoring.
> > >
> > > Sashiko flagged some issues, some of them not introduced by this, can
> > > you please check this:
> > >
> > > https://sashiko.dev/#/patchset/20260504070054.29508-1-andriy.shevchenko%40linux.intel.com
> >
> > "Could this result in a null pointer dereference if device_get_match_data()
> > returns null?"
> > Yes, it sounds legit but not introduced here.
> >
> > "In the original code, the warning was suppressed when the property was missing
> > because err evaluated to -EINVAL. Now, if the property is absent, max_bitrate
> > is explicitly set to 0 in the else block, which then unconditionally triggers
> > this warning."
> > True, but I don't know which is better here, I consider that it's
> > good to inform user about default being used as a fallback. I can change
> > this back to the original logic. What do you prefer?
> >
> > The third one is the repetition of the first one (see above).
> >
> > TL;DR: The only one legitimated question is about a (new old) warning.
>
> Yeah would be great if we could fix these as well please
I just sent a v4:
20260512130552.272476-1-andriy.shevchenko@linux.intel.com
--
With Best Regards,
Andy Shevchenko
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v4 1/5] phy: phy-can-transceiver: Check driver match and driver data against NULL
From: Andy Shevchenko @ 2026-05-12 13:01 UTC (permalink / raw)
To: Peng Fan, linux-can, linux-phy, linux-kernel
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Josua Mayer, Ulf Hansson, Andy Shevchenko
In-Reply-To: <20260512130552.272476-1-andriy.shevchenko@linux.intel.com>
Every platform driver can be forced to match a device that doesn't
match its list of device IDs because of device_match_driver_override()
so platform drivers that rely on the existence of a device's driver
data need to verify its presence.
Accordingly, add requisite match and driver data checks against NULL
to the driver where they are missing.
Fixes: a4a86d273ff1 ("phy: phy-can-transceiver: Add support for generic CAN transceiver driver")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/phy/phy-can-transceiver.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 2b52e47f247a..1808f903c057 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -162,6 +162,9 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
int err, i, num_ch = 1;
match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
+ if (!match || !match->data)
+ return -ENODEV;
+
drvdata = match->data;
if (drvdata->flags & CAN_TRANSCEIVER_DUAL_CH)
num_ch = 2;
--
2.50.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v4 5/5] phy: phy-can-transceiver: Drop unused include
From: Andy Shevchenko @ 2026-05-12 13:01 UTC (permalink / raw)
To: Peng Fan, linux-can, linux-phy, linux-kernel
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Josua Mayer, Ulf Hansson, Andy Shevchenko
In-Reply-To: <20260512130552.272476-1-andriy.shevchenko@linux.intel.com>
This file does not use the symbols from the legacy
<linux/gpio.h> header, so let's drop it.
Reviewed-by: Josua Mayer <josua@solid-run.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/phy/phy-can-transceiver.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 973cc7ce651f..534a4fc482aa 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -5,12 +5,11 @@
* Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com
*
*/
+#include <linux/gpio/consumer.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/property.h>
#include <linux/module.h>
-#include <linux/gpio.h>
-#include <linux/gpio/consumer.h>
#include <linux/mux/consumer.h>
struct can_transceiver_data {
--
2.50.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v4 3/5] phy: phy-can-transceiver: Move OF ID table closer to their user
From: Andy Shevchenko @ 2026-05-12 13:01 UTC (permalink / raw)
To: Peng Fan, linux-can, linux-phy, linux-kernel
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Josua Mayer, Ulf Hansson, Andy Shevchenko
In-Reply-To: <20260512130552.272476-1-andriy.shevchenko@linux.intel.com>
There is no code that uses ID table directly, except the
struct device_driver at the end of the file. Hence, move
table closer to its user. It's always possible to access
them via a pointer.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/phy/phy-can-transceiver.c | 58 +++++++++++++++----------------
1 file changed, 29 insertions(+), 29 deletions(-)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 5cfd09735204..23655956ace8 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -97,35 +97,6 @@ static const struct can_transceiver_data tja1057_drvdata = {
.flags = CAN_TRANSCEIVER_SILENT_PRESENT,
};
-static const struct of_device_id can_transceiver_phy_ids[] = {
- {
- .compatible = "ti,tcan1042",
- .data = &tcan1042_drvdata
- },
- {
- .compatible = "ti,tcan1043",
- .data = &tcan1043_drvdata
- },
- {
- .compatible = "nxp,tja1048",
- .data = &tja1048_drvdata
- },
- {
- .compatible = "nxp,tja1051",
- .data = &tja1051_drvdata
- },
- {
- .compatible = "nxp,tja1057",
- .data = &tja1057_drvdata
- },
- {
- .compatible = "nxp,tjr1443",
- .data = &tcan1043_drvdata
- },
- { }
-};
-MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
-
static struct phy *can_transceiver_phy_xlate(struct device *dev,
const struct of_phandle_args *args)
{
@@ -232,6 +203,35 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
return PTR_ERR_OR_ZERO(phy_provider);
}
+static const struct of_device_id can_transceiver_phy_ids[] = {
+ {
+ .compatible = "ti,tcan1042",
+ .data = &tcan1042_drvdata
+ },
+ {
+ .compatible = "ti,tcan1043",
+ .data = &tcan1043_drvdata
+ },
+ {
+ .compatible = "nxp,tja1048",
+ .data = &tja1048_drvdata
+ },
+ {
+ .compatible = "nxp,tja1051",
+ .data = &tja1051_drvdata
+ },
+ {
+ .compatible = "nxp,tja1057",
+ .data = &tja1057_drvdata
+ },
+ {
+ .compatible = "nxp,tjr1443",
+ .data = &tcan1043_drvdata
+ },
+ { }
+};
+MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
+
static struct platform_driver can_transceiver_phy_driver = {
.probe = can_transceiver_phy_probe,
.driver = {
--
2.50.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v4 0/5] phy: phy-can-transceiver: Ad-hoc cleanups and refactoring
From: Andy Shevchenko @ 2026-05-12 13:01 UTC (permalink / raw)
To: Peng Fan, linux-can, linux-phy, linux-kernel
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Josua Mayer, Ulf Hansson, Andy Shevchenko
The driver does two things that need to be addressed:
- includes subject to remove gpio.h
- checks for error code from device property APIs when it can be done in
a robust way
This series addresses the above and adds a couple of additional refactoring.
On top of that it fixes potential NULL pointer dereference when driver_override
is in use.
Changelog v4:
- prepended the series with match and driver data check (Sashiko, Vinod)
- made the max_bitrate==0 warning only when property is present (Sashiko, Vinod)
- rebased the rest accordingly
v3: 20260504070054.29508-1-andriy.shevchenko@linux.intel.com
Changelog v3:
- fixed commit message in patch 1 (Josua)
- dropped stray change (Vinod)
- collected tags (Josua)
v2: 20260317203001.2108568-1-andriy.shevchenko@linux.intel.com
Changelog v2:
- rebased on top of the latest changes in the driver
- Cc'ed to Ulf and Josua due to above
- elaborated dropping of_node parameter (Vladimir)
v1: 20260219202910.2304440-1-andriy.shevchenko@linux.intel.com
Andy Shevchenko (5):
phy: phy-can-transceiver: Check driver match and driver data against
NULL
phy: phy-can-transceiver: use device_get_match_data()
phy: phy-can-transceiver: Move OF ID table closer to their user
phy: phy-can-transceiver: Don't check for specific errors when parsing
properties
phy: phy-can-transceiver: Drop unused include
drivers/phy/phy-can-transceiver.c | 89 +++++++++++++++++--------------
1 file changed, 49 insertions(+), 40 deletions(-)
--
2.50.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* [PATCH v4 4/5] phy: phy-can-transceiver: Don't check for specific errors when parsing properties
From: Andy Shevchenko @ 2026-05-12 13:01 UTC (permalink / raw)
To: Peng Fan, linux-can, linux-phy, linux-kernel
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Josua Mayer, Ulf Hansson, Andy Shevchenko
In-Reply-To: <20260512130552.272476-1-andriy.shevchenko@linux.intel.com>
Instead of checking for the specific error codes (that can be considered
a layering violation to some extent) check for the property existence first
and then either parse it, or apply a default value.
With that, return an error when parsing of the existing property fails.
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/phy/phy-can-transceiver.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 23655956ace8..973cc7ce651f 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -128,8 +128,9 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
struct gpio_desc *standby_gpio;
struct gpio_desc *enable_gpio;
struct mux_state *mux_state;
- u32 max_bitrate = 0;
int err, i, num_ch = 1;
+ const char *propname;
+ u32 max_bitrate;
drvdata = device_get_match_data(dev);
if (drvdata)
@@ -151,9 +152,17 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
priv->mux_state = mux_state;
- err = device_property_read_u32(dev, "max-bitrate", &max_bitrate);
- if ((err != -EINVAL) && !max_bitrate)
- dev_warn(dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit\n");
+ propname = "max-bitrate";
+ if (device_property_present(dev, propname)) {
+ err = device_property_read_u32(dev, propname, &max_bitrate);
+ if (err)
+ return dev_err_probe(dev, err, "failed to parse %s\n", propname);
+
+ if (max_bitrate == 0)
+ dev_warn(dev, "Invalid value for transceiver max bitrate. Ignoring bitrate limit\n");
+ } else {
+ max_bitrate = 0;
+ }
for (i = 0; i < num_ch; i++) {
can_transceiver_phy = &priv->can_transceiver_phy[i];
--
2.50.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* [PATCH v4 2/5] phy: phy-can-transceiver: use device_get_match_data()
From: Andy Shevchenko @ 2026-05-12 13:01 UTC (permalink / raw)
To: Peng Fan, linux-can, linux-phy, linux-kernel
Cc: Marc Kleine-Budde, Vincent Mailhol, Vinod Koul, Neil Armstrong,
Josua Mayer, Ulf Hansson, Andy Shevchenko
In-Reply-To: <20260512130552.272476-1-andriy.shevchenko@linux.intel.com>
Use the generic firmware node interface for retrieving
device match data instead of the OF-specific one.
While at it, drop unneeded argument to devm_phy_create() which
extracts device node from the given device by default.
Reviewed-by: Josua Mayer <josua@solid-run.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/phy/phy-can-transceiver.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
index 1808f903c057..5cfd09735204 100644
--- a/drivers/phy/phy-can-transceiver.c
+++ b/drivers/phy/phy-can-transceiver.c
@@ -5,9 +5,9 @@
* Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com
*
*/
-#include <linux/of.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/property.h>
#include <linux/module.h>
#include <linux/gpio.h>
#include <linux/gpio/consumer.h>
@@ -152,7 +152,6 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
struct can_transceiver_phy *can_transceiver_phy;
struct can_transceiver_priv *priv;
const struct can_transceiver_data *drvdata;
- const struct of_device_id *match;
struct phy *phy;
struct gpio_desc *silent_gpio;
struct gpio_desc *standby_gpio;
@@ -161,11 +160,10 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
u32 max_bitrate = 0;
int err, i, num_ch = 1;
- match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
- if (!match || !match->data)
+ drvdata = device_get_match_data(dev);
+ if (drvdata)
return -ENODEV;
- drvdata = match->data;
if (drvdata->flags & CAN_TRANSCEIVER_DUAL_CH)
num_ch = 2;
@@ -190,7 +188,7 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
can_transceiver_phy = &priv->can_transceiver_phy[i];
can_transceiver_phy->priv = priv;
- phy = devm_phy_create(dev, dev->of_node, &can_transceiver_phy_ops);
+ phy = devm_phy_create(dev, NULL, &can_transceiver_phy_ops);
if (IS_ERR(phy)) {
dev_err(dev, "failed to create can transceiver phy\n");
return PTR_ERR(phy);
--
2.50.1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
* Re: [PATCH v2] phy: fsl-imx8mq-usb: add control register regmap
From: Frank Li @ 2026-05-12 14:59 UTC (permalink / raw)
To: Xu Yang
Cc: vkoul, neil.armstrong, s.hauer, kernel, festevam, jun.li,
a.fatoum, franz.schnyder, stefano.radaelli21, linux-phy, imx,
linux-arm-kernel, linux-kernel
In-Reply-To: <20260512101212.1498223-1-xu.yang_2@nxp.com>
On Tue, May 12, 2026 at 06:12:12PM +0800, Xu Yang wrote:
> The CR port is a simple 16-bit data/address parallel port that is
> provided for on-chip access to the control registers inside the
> USB 3.0 femtoPHY. Add control register regmap and export these
> registers by debugfs to help PHY's diagnostic.
>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>
> ---
> Changes in v2:
> - no changes
Do you means "resend" because not change in v2?
Reviewed-by: Frank Li <Frank.Li@nxp.com>
> ---
> drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 19 ++++++++++++++++++-
> 1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index b05d80e849a1..958d114b0c83 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -1,5 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0+
> -/* Copyright (c) 2017 NXP. */
> +/* Copyright 2017-2026 NXP. */
>
> #include <linux/bitfield.h>
> #include <linux/clk.h>
> @@ -9,6 +9,7 @@
> #include <linux/of.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/regmap.h>
> #include <linux/regulator/consumer.h>
> #include <linux/usb/typec_mux.h>
>
> @@ -55,6 +56,8 @@
> #define PHY_CTRL6_ALT_CLK_EN BIT(1)
> #define PHY_CTRL6_ALT_CLK_SEL BIT(0)
>
> +#define PHY_CRCTL 0x30
> +
> #define PHY_TUNE_DEFAULT 0xffffffff
>
> #define TCA_CLK_RST 0x00
> @@ -118,6 +121,7 @@ struct imx8mq_usb_phy {
> void __iomem *base;
> struct regulator *vbus;
> struct tca_blk *tca;
> + struct regmap *cr_regmap;
> u32 pcs_tx_swing_full;
> u32 pcs_tx_deemph_3p5db;
> u32 tx_vref_tune;
> @@ -685,6 +689,14 @@ static const struct of_device_id imx8mq_usb_phy_of_match[] = {
> };
> MODULE_DEVICE_TABLE(of, imx8mq_usb_phy_of_match);
>
> +static const struct regmap_config imx_cr_regmap_config = {
> + .name = "cr",
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .max_register = 0x7,
> +};
> +
> static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> {
> struct phy_provider *phy_provider;
> @@ -713,6 +725,11 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> if (IS_ERR(imx_phy->base))
> return PTR_ERR(imx_phy->base);
>
> + imx_phy->cr_regmap = devm_regmap_init_mmio(dev, imx_phy->base + PHY_CRCTL,
> + &imx_cr_regmap_config);
> + if (IS_ERR(imx_phy->cr_regmap))
> + return PTR_ERR(imx_phy->cr_regmap);
> +
> phy_ops = of_device_get_match_data(dev);
> if (!phy_ops)
> return -EINVAL;
> --
> 2.34.1
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH phy-next 3/5] phy: lynx-28g: require an OF node to probe
From: Ioana Ciornei @ 2026-05-12 15:01 UTC (permalink / raw)
To: Vladimir Oltean
Cc: linux-phy, netdev, Vinod Koul, Neil Armstrong, Josua Mayer,
linux-kernel
In-Reply-To: <20260511150023.1903577-4-vladimir.oltean@nxp.com>
On Mon, May 11, 2026 at 06:00:21PM +0300, Vladimir Oltean wrote:
> The driver will gain support for variants in an upcoming change, and
> will use of_device_get_match_data() to deduce the running variant from
> the compatible string.
>
> Currently, the driver expects the schema at phy/fsl,lynx-28g.yaml, and
> OF-based consumers, but doesn't enforce this. And it is possible for
> user space to force-bind the driver to a device without OF node using
> the driver_override sysfs.
>
> To avoid future surprise crashes for an unsupported configuration,
> explicitly test for the presence of an OF node and fail probing if
> found.
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH phy-next 4/5] phy: lynx-28g: probe on per-SoC and per-instance compatible strings
From: Ioana Ciornei @ 2026-05-12 15:02 UTC (permalink / raw)
To: Vladimir Oltean
Cc: linux-phy, netdev, Vinod Koul, Neil Armstrong, Josua Mayer,
linux-kernel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree
In-Reply-To: <20260511150023.1903577-5-vladimir.oltean@nxp.com>
On Mon, May 11, 2026 at 06:00:22PM +0300, Vladimir Oltean wrote:
> Add driver support for probing on the new, per-instance and per-SoC
> bindings, which provide the main benefit that they allow rejecting
> unsupported protocols per lane (10GbE on SerDes 2 lanes 0-5), but they
> also allow avoiding the creation of PHYs for lanes that don't exist
> (LX2162A lanes 0-3).
>
> For old device trees with just "fsl,lynx-28g", the only things that
> change are:
>
> - a probe time warning/encouragement to update the device tree. This is
> warranted by the fact that using "fsl,lynx-28g" may already provide
> incorrect behaviour (undetected absent 10GbE support on LX2160A
> SerDes 2 lanes 0-5). But we retain bug compatibility nonetheless.
>
> - the feature set is frozen in time (e.g. no 25GbE). Since we cannot
> guarantee that this protocol will work on a lane, just err on the safe
> side and don't offer it (and require a device tree update to get it).
>
> In terms of code, the lynx_28g_supports_lane_mode() function prototype
> changes. It was a SerDes-global function and now becomes per lane, to
> reflect the specific capabilities each instance may have. The
> implementation goes through priv->info->lane_supports_mode().
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2] phy: fsl-imx8mq-usb: set usb phy to be wakeup capable
From: Frank Li @ 2026-05-12 15:03 UTC (permalink / raw)
To: Xu Yang
Cc: vkoul, neil.armstrong, s.hauer, kernel, festevam, jun.li,
a.fatoum, franz.schnyder, stefano.radaelli21, linux-phy, imx,
linux-arm-kernel, linux-kernel
In-Reply-To: <20260512101046.1498096-1-xu.yang_2@nxp.com>
On Tue, May 12, 2026 at 06:10:46PM +0800, Xu Yang wrote:
> USB remote wakeup need its PHY power domain to be active,
> so set PHY to be wakeup capable.
The reason should usb phy support USB wakeup.
"Set PHY wakeup capable because this PHY supports remote wakeup function." ?
Frank
>
> Signed-off-by: Li Jun <jun.li@nxp.com>
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
>
> ---
> Changes in v2:
> - no changes
> ---
> drivers/phy/freescale/phy-fsl-imx8mq-usb.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 958d114b0c83..50b67f1e6a90 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> @@ -750,6 +750,7 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
> "failed to get tca\n");
>
> imx8m_get_phy_tuning_data(imx_phy);
> + device_set_wakeup_capable(dev, true);
>
> phy_provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
>
> --
> 2.34.1
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 1/3] phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask
From: sashiko-bot @ 2026-05-12 21:35 UTC (permalink / raw)
To: Radhey Shyam Pandey; +Cc: vkoul, olteanv, neil.armstrong, linux-phy
In-Reply-To: <20260511163135.2924642-2-radhey.shyam.pandey@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unhandled Error Path in `xpsgtr_phy_init()` masks clock initialization failures.
--
commit f38f7c8479703290a0f18153b9a9207f41500566
Author: Nava kishore Manne <nava.kishore.manne@amd.com>
phy: zynqmp: fix L0_TM_DISABLE_SCRAMBLE_ENCODER mask
The L0_TX_DIG_61 register bit 2 is a reserved read-only field.
The previous mask value 0x0f incorrectly included bit 2, causing
unintended writes to a reserved bit on every scrambler bypass
operation.
Correct the mask to (BIT(3) | GENMASK(1, 0)) to cover only the
valid scramble bypass control bits.
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index fe6b4925d1662..c037d7c13d485 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
> @@ -53,7 +53,7 @@
> #define L0_TM_DIG_6 0x106c
> #define L0_TM_DIS_DESCRAMBLE_DECODER 0x0f
> #define L0_TX_DIG_61 0x00f4
> -#define L0_TM_DISABLE_SCRAMBLE_ENCODER 0x0f
> +#define L0_TM_DISABLE_SCRAMBLE_ENCODER (BIT(3) | GENMASK(1, 0))
>
> /* PLL Test Mode register parameters */
> #define L0_TM_PLL_DIG_37 0x2094
This isn't a bug introduced by this patch, but there is a pre-existing
issue in xpsgtr_phy_init().
If clk_prepare_enable() fails, the error path appears to mask the failure:
drivers/phy/xilinx/phy-zynqmp.c:xpsgtr_phy_init() {
int ret = 0;
mutex_lock(>r_dev->gtr_mutex);
/* Configure and enable the clock when peripheral phy_init call */
if (clk_prepare_enable(gtr_dev->clk[gtr_phy->refclk]))
goto out;
...
out:
mutex_unlock(>r_dev->gtr_mutex);
return ret;
}
Since ret is initialized to 0, will this return 0 instead of the actual error
code from clk_prepare_enable()?
If the function falsely reports success, the PHY framework will consider the
device initialized.
During teardown, could xpsgtr_phy_exit() unconditionally call
clk_disable_unprepare() on a clock that was never enabled, causing a clock
reference count underflow and a kernel WARN_ON?
Furthermore, would allowing the upper layer to access PHY registers without a
running clock risk hanging the AXI bus on ZynqMP, potentially leading to an
asynchronous external abort (system panic)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511163135.2924642-1-radhey.shyam.pandey@amd.com?part=1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH RESEND v8 10/10] iio: adc: qcom-spmi-iadc: Remove regmap R/W wrapper functions
From: sashiko-bot @ 2026-05-12 21:55 UTC (permalink / raw)
To: AngeloGioacchino Del Regno; +Cc: neil.armstrong, linux-phy, vkoul, olteanv
In-Reply-To: <20260511100805.121432-11-angelogioacchino.delregno@collabora.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Format Specifier Mismatch in `iadc_version_check`
- [Low] Implicit Signed/Unsigned Arithmetic Conversion in `iadc_rsense_read`
--
commit e3f3f37b20b194577f3177eac52f98e565837579
Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
iio: adc: qcom-spmi-iadc: Remove regmap R/W wrapper functions
This commit migrates the driver to use the regmap api directly instead of
relying on custom read and write wrapper functions. It simplifies the driver
as the spmi subdevice natively handles the register base address internally.
> diff --git a/drivers/iio/adc/qcom-spmi-iadc.c b/drivers/iio/adc/qcom-spmi-iadc.c
> index 55a09c0e2d5cb..503f90b2a5bad 100644
> --- a/drivers/iio/adc/qcom-spmi-iadc.c
> +++ b/drivers/iio/adc/qcom-spmi-iadc.c
[ ... ]
> @@ -392,10 +369,10 @@ static int iadc_update_offset(struct iadc_chip *iadc)
>
> static int iadc_version_check(struct iadc_chip *iadc)
> {
> - u8 val;
> + u32 val;
This isn't a bug, but since val is now a u32, should the subsequent
dev_err() format specifiers in this function be updated from %d to %u to
avoid compiler warnings?
> int ret;
>
> - ret = iadc_read(iadc, IADC_PERPH_TYPE, &val);
> + ret = regmap_read(iadc->regmap, IADC_PERPH_TYPE, &val);
> if (ret < 0)
> return ret;
[ ... ]
> @@ -428,7 +405,7 @@ static int iadc_version_check(struct iadc_chip *iadc)
> static int iadc_rsense_read(struct iadc_chip *iadc, struct device_node *node)
> {
> int ret, sign, int_sense;
> - u8 deviation;
> + u32 deviation;
Does changing deviation to u32 cause an implicit unsigned arithmetic
conversion later in this function?
When calculating int_sense, sign can be -1:
int_sense += sign * deviation * IADC_INT_RSENSE_DEVIATION;
Since deviation is unsigned, -1 will be promoted to an unsigned value.
Could this trigger static analysis warnings, and would explicitly casting
deviation to int during the calculation be safer?
>
> ret = of_property_read_u32(node, "qcom,external-resistor-micro-ohms",
> &iadc->rsense[IADC_EXT_RSENSE]);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511100805.121432-1-angelogioacchino.delregno@collabora.com?part=10
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 3/3] phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB
From: sashiko-bot @ 2026-05-12 22:31 UTC (permalink / raw)
To: Radhey Shyam Pandey; +Cc: linux-phy, olteanv, vkoul, neil.armstrong
In-Reply-To: <20260511163135.2924642-4-radhey.shyam.pandey@amd.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] This problem wasn't introduced by this patch, but the scrambler and 8b/10b bypass state is not explicitly initialized for DisplayPort (DP) and PCIe, leaving them vulnerable to inheriting an incorrect hardware state.
--
commit bd13b5595b6bf913b77d4b61fb704c7e56fa4847
Author: Nava kishore Manne <nava.kishore.manne@amd.com>
phy: zynqmp: keep SERDES scrambler and 8b/10b enabled for USB
This commit ensures that physical layer scrambling and 8b/10b encoding are
not bypassed for USB operations. It explicitly disables the scrambler bypass
for USB3 while preserving it for SATA and SGMII modes where encoding is
handled by the controller.
> diff --git a/drivers/phy/xilinx/phy-zynqmp.c b/drivers/phy/xilinx/phy-zynqmp.c
> index 6c56c4df8523c..087fe402e4e20 100644
> --- a/drivers/phy/xilinx/phy-zynqmp.c
> +++ b/drivers/phy/xilinx/phy-zynqmp.c
[ ... ]
> @@ -707,6 +722,10 @@ static int xpsgtr_phy_init(struct phy *phy)
> case ICM_PROTOCOL_SGMII:
> xpsgtr_phy_init_sgmii(gtr_phy);
> break;
> +
> + case ICM_PROTOCOL_USB:
> + xpsgtr_bypass_scrambler_8b10b(gtr_phy, false);
> + break;
> }
Does this leave the scrambler and 8b/10b bypass state uninitialized for
DisplayPort (DP) and PCIe?
While this commit resolves the state leak for USB by explicitly clearing the
bypass bits, it appears DP and PCIe might still be vulnerable to similar
issues.
If a lane is repurposed from SATA or SGMII to DP or PCIe (for example, if a
bootloader probes an M.2 slot for SATA, and Linux later configures it for
PCIe NVMe), could the PHY inherit an erroneously bypassed state?
Should the bypass bits also be explicitly cleared for ICM_PROTOCOL_DP and
ICM_PROTOCOL_PCIE to ensure they do not rely on potentially corrupted hardware
defaults?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511163135.2924642-1-radhey.shyam.pandey@amd.com?part=3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 1/6] phy: rockchip: samsung-hdptx: Fix rate recalculation for high bpc
From: sashiko-bot @ 2026-05-12 22:51 UTC (permalink / raw)
To: Cristian Ciocaltea; +Cc: neil.armstrong, olteanv, vkoul, linux-phy
In-Reply-To: <20260511-hdptx-clk-fixes-v2-1-664e41379cab@collabora.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Clock rate calculation drops a rounding step, replacing DIV_ROUND_CLOSEST_ULL() with truncation via div_u64(), which can cause a 1 Hz clock rate mismatch.
--
commit f013b4b7bc1d4ae235d28b10cad3143d009d131c
Author: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
phy: rockchip: samsung-hdptx: Fix rate recalculation for high bpc
This commit fixes the clock rate recalculation in the HDMI PHY probe path
by directly reading the bpc value from hardware registers instead of
relying on uninitialized configuration struct members.
> diff --git a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> index 2d973bc37f076..7fb1c22318bbf 100644
> --- a/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> +++ b/drivers/phy/rockchip/phy-rockchip-samsung-hdptx.c
> @@ -2280,7 +2281,7 @@ static u64 rk_hdptx_phy_clk_calc_rate_from_pll_cfg(struct rk_hdptx_phy *hdptx)
> fout = fout + sdm;
> }
>
> - return div_u64(fout * 2, ropll_hw.pms_sdiv * 10);
> + return div_u64(fout * 2 * 8, ropll_hw.pms_sdiv * 10 * bpc);
> }
>
> static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
> @@ -2288,19 +2289,13 @@ static unsigned long rk_hdptx_phy_clk_recalc_rate(struct clk_hw *hw,
> {
> struct rk_hdptx_phy *hdptx = to_rk_hdptx_phy(hw);
> u32 status;
> - u64 rate;
> int ret;
>
> ret = regmap_read(hdptx->grf, GRF_HDPTX_CON0, &status);
> if (ret || !(status & HDPTX_I_PLL_EN))
> return 0;
>
> - rate = rk_hdptx_phy_clk_calc_rate_from_pll_cfg(hdptx);
> -
> - if (hdptx->hdmi_cfg.mode == PHY_HDMI_MODE_FRL)
> - return rate;
> -
> - return DIV_ROUND_CLOSEST_ULL(rate * 8, hdptx->hdmi_cfg.bpc);
> + return rk_hdptx_phy_clk_calc_rate_from_pll_cfg(hdptx);
Does this inadvertently change the rounding behavior?
By moving the * 8 and / bpc math into rk_hdptx_phy_clk_calc_rate_from_pll_cfg()
and using div_u64(), the result is now truncated instead of rounded.
For example, if the exact calculated value ends in .6 or .8, the truncation
will yield a value 1 Hz lower than the rounded value.
Since rk_hdptx_phy_clk_determine_rate() still calculates the expected rate
using DIV_ROUND_CLOSEST_ULL(), could this truncation lead to a 1 Hz mismatch
in the Common Clock Framework?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-hdptx-clk-fixes-v2-0-664e41379cab@collabora.com?part=1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 1/6] dt-bindings: usb: ci-hdrc-usb2: Document nvidia,external-control property
From: sashiko-bot @ 2026-05-12 23:29 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: conor+dt, robh, krzk+dt, neil.armstrong, olteanv, linux-phy,
devicetree, vkoul
In-Reply-To: <20260511135703.62470-2-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The vendor-specific property `nvidia,external-control` is not constrained to NVIDIA-compatible devices.
--
commit 2fc8a800ab9c1409217fd7f6b370e99b9cc6d3e7
Author: Svyatoslav Ryhel <clamor95@gmail.com>
dt-bindings: usb: ci-hdrc-usb2: Document nvidia,external-control property
This commit adds the nvidia,external-control property to the ci-hdrc-usb2
binding. It is used to indicate that the controller is configured externally,
such as by a modem requiring precise bus control.
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.yaml
[ ... ]
> @@ -75,6 +75,13 @@ properties:
> type: boolean
> deprecated: true
>
> + nvidia,external-control:
> + description:
> + Indicates that the controller is configured externally and that the host
> + should not attempt to touch it. Usually used by a modem which requires
> + precise bus configuration.
> + type: boolean
> +
Could this property be constrained to NVIDIA-compatible devices using an
allOf block with an if-then statement?
Because it is defined at the top level of the shared binding, unrelated
device nodes could incorrectly include nvidia,external-control and still
pass schema validation.
> ulpi:
> type: object
> additionalProperties: false
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=1
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 2/6] usb: chipidea: tegra: Avoid controller/PHY init if bus is externally controlled
From: sashiko-bot @ 2026-05-12 23:29 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: conor+dt, neil.armstrong, vkoul, krzk+dt, robh, devicetree,
olteanv, linux-phy
In-Reply-To: <20260511135703.62470-3-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unbinding the `tegra_usb` driver when `nvidia,external-control` is present causes a NULL pointer dereference.
- [Medium] Unconditional call to `usb_phy_shutdown()` in `tegra_usb_remove()` on an uninitialized PHY triggers a kernel warning.
--
commit 3826697c3ab50338add8e2c889c119f3dae6b570
Author: Svyatoslav Ryhel <clamor95@gmail.com>
usb: chipidea: tegra: Avoid controller/PHY init if bus is externally controlled
If the USB controller and PHY are externally controlled, the registration
of the controller and the PHY initialization are skipped. This delegates
configuration to the device that controls the bus.
> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 372788f0f9701..593390a818d12 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
[ ... ]
> @@ -342,12 +348,14 @@ static int tegra_usb_probe(struct platform_device *pdev)
> if (of_usb_get_phy_mode(pdev->dev.of_node) == USBPHY_INTERFACE_MODE_ULPI)
> usb->data.flags &= ~CI_HDRC_SUPPORTS_RUNTIME_PM;
>
> - usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> - pdev->num_resources, &usb->data);
> - if (IS_ERR(usb->dev)) {
> - err = dev_err_probe(&pdev->dev, PTR_ERR(usb->dev),
> - "failed to add HDRC device");
> - goto phy_shutdown;
> + if (!usb->externally_controlled) {
> + usb->dev = ci_hdrc_add_device(&pdev->dev, pdev->resource,
> + pdev->num_resources, &usb->data);
> + if (IS_ERR(usb->dev)) {
> + err = dev_err_probe(&pdev->dev, PTR_ERR(usb->dev),
> + "failed to add HDRC device");
> + goto phy_shutdown;
> + }
> }
>
> return 0;
When nvidia,external-control is present, usb->dev is left uninitialized
and remains NULL. Does the unbind path still try to use it?
Looking at tegra_usb_remove(), it unconditionally calls
ci_hdrc_remove_device() with usb->dev:
static void tegra_usb_remove(struct platform_device *pdev)
{
struct tegra_usb *usb = platform_get_drvdata(pdev);
ci_hdrc_remove_device(usb->dev);
...
This looks like it could lead to a NULL pointer dereference, as
ci_hdrc_remove_device() dereferences the pointer immediately without checking:
void ci_hdrc_remove_device(struct platform_device *pdev)
{
int id = pdev->id;
...
Could this regression cause a crash on device unbind or module removal
when the controller is externally controlled?
Additionally, when the PHY initialization is skipped during probe, does
tegra_usb_remove() still attempt to shut it down?
static void tegra_usb_remove(struct platform_device *pdev)
{
...
usb_phy_shutdown(usb->phy);
...
Since the PHY wasn't initialized, its frequency will be 0. Calling
tegra_usb_phy_shutdown() on this uninitialized PHY appears to trigger
a warning regression:
static void tegra_usb_phy_shutdown(struct usb_phy *u_phy)
{
struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
if (WARN_ON(!phy->freq))
return;
...
Should tegra_usb_remove() be updated to check if it is externally
controlled before calling the shutdown and remove functions?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=2
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 3/6] dt-bindings: net: Document Infineon/Intel XMM6260 modem
From: sashiko-bot @ 2026-05-12 23:29 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: neil.armstrong, vkoul, devicetree, conor+dt, olteanv, linux-phy,
robh, krzk+dt
In-Reply-To: <20260511135703.62470-4-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The description field in the YAML binding ends abruptly and appears truncated.
--
commit 8f6f0b35b760957b403f0e3021bc11d578bdef55
Author: Svyatoslav Ryhel <clamor95@gmail.com>
dt-bindings: net: Document Infineon/Intel XMM6260 modem
This commit adds the device tree binding documentation for the Infineon/Intel
XMM6260 3G embedded USB modem.
> diff --git a/Documentation/devicetree/bindings/net/infineon,xmm6260.yaml b/Documentation/devicetree/bindings/net/infineon,xmm6260.yaml
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/infineon,xmm6260.yaml
[ ... ]
> +title: Infineon/Intel XMM6260 embedded USB modem
> +
> +description:
> + The Infineon/Intel XMM6260 is a 3G-focused, slim modem platform designed
> + for smartphones, data cards, and Machine-to-Machine (M2M) applications.
> + The modem is usually connected via the application processor's USB line
> + in HSIC mode; however, to work properly, the modem must control this line
This isn't a bug, but the description text ends abruptly without punctuation.
Was some text accidentally truncated here regarding how the modem controls
this line?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=3
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 4/6] net: usb: Add Infineon XMM6260 Baseband modem support
From: sashiko-bot @ 2026-05-12 23:29 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: neil.armstrong, robh, linux-phy, olteanv, krzk+dt, devicetree,
vkoul, conor+dt
In-Reply-To: <20260511135703.62470-5-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Type confusion and out-of-bounds read in USB notifier callback.
- [High] Hardware teardown in `remove()` races with devres-managed IRQ and delayed work.
- [High] Missing synchronization for driver state variables across multiple contexts.
- [High] Delayed work bypasses intended timeout due to incorrect rescheduling logic.
- [High] Missing Kconfig dependency on RFKILL causes linker errors.
- [Medium] Incorrect error handling for `rfkill_alloc` causes `probe()` to succeed on memory allocation failure.
- [Medium] Unbalanced PHY and regulator calls in `remove()` and `set_block()`.
- [Medium] Missing USB disconnect handling stalls the modem's state machine.
--
commit 0ae4ba7bc0acd1df4aeeb4a594adcd61b86ea4a4
Author: Svyatoslav Ryhel <clamor95@gmail.com>
net: usb: Add Infineon XMM6260 Baseband modem support
The Infineon/Intel XMM6260 is a 3G-focused, slim modem platform designed
for smartphones, data cards, and Machine-to-Machine (M2M) applications.
> diff --git a/drivers/net/usb/Kconfig b/drivers/net/usb/Kconfig
> index 52a5c0922c79f..503f24a3cfa69 100644
> --- a/drivers/net/usb/Kconfig
> +++ b/drivers/net/usb/Kconfig
[ ... ]
> @@ -642,4 +642,19 @@ config USB_RTL8153_ECM
> CONFIG_USB_RTL8152 is not set, or the RTL8153 device is not
> supported by r8152 driver.
>
> +config USB_NET_XMM6260
> + tristate "Infineon XMM626X Baseband HSPA/HSUPA modem"
> + depends on GPIOLIB && OF && USB_CHIPIDEA
Should this configuration also depend on RFKILL || RFKILL=n?
Since the driver calls exported RFKILL functions like rfkill_alloc and
rfkill_register unconditionally, building this driver as built-in while
RFKILL is built as a module might cause linker errors.
> + help
> + Select this if you want to use an Infineon XMM626X modem, found in
[ ... ]
> diff --git a/drivers/net/usb/baseband-xmm6260.c b/drivers/net/usb/baseband-xmm6260.c
> new file mode 100644
> index 0000000000000..658f5351fab7e
> --- /dev/null
> +++ b/drivers/net/usb/baseband-xmm6260.c
[ ... ]
> +static int baseband_xmm_usb_notifier_call(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct baseband_xmm_data *priv =
> + container_of(nb, struct baseband_xmm_data, nb);
> + struct usb_device *udev = data;
> + u16 product = le16_to_cpu(udev->descriptor.idProduct);
> + u16 vendor = le16_to_cpu(udev->descriptor.idVendor);
Does this safely handle all USB notifier events? The global usb_notifier_list
broadcasts events for both devices and buses. For bus events (like
USB_BUS_ADD), the data parameter is a struct usb_bus pointer.
By unconditionally casting data to a struct usb_device pointer and
dereferencing udev->descriptor.idProduct before checking the action switch,
could this cause an out-of-bounds memory read when a bus event is received?
> +
> + switch (action) {
> + case USB_DEVICE_ADD:
> + /* Infineon XMM6260 ID 1519:0020 */
> + if (vendor == BASEBAND_VENDOR_ID_COMNEON &&
> + product == BASEBAND_PRODUCT_ID_XMM6260) {
> + cancel_delayed_work_sync(&priv->modem_work);
> + priv->inited = true;
> + }
> + break;
> + }
What happens if the USB device is removed?
There doesn't appear to be a handler for USB_DEVICE_REMOVE. If the modem
drops off the bus, priv->inited remains true and priv->ap_state stays stuck
in IPC_AP_WAKE_INIT2.
Will the interrupt handler ignore all subsequent wake attempts, leaving the
modem unresponsive until the module is reloaded?
> +
> + return NOTIFY_OK;
> +}
[ ... ]
> +static int baseband_xmm_set_block(void *data, bool blocked)
> +{
> + struct baseband_xmm_data *priv = data;
> +
> + if (blocked) {
> + if (priv->inited && priv->powered) {
[ ... ]
> + regulator_disable(priv->vbat_supply);
> +
> + priv->powered = false;
> + priv->inited = false;
> + }
> + } else {
> + if (priv->inited)
> + return 0;
> +
> + priv->ap_state = IPC_AP_WAKE_IRQ_READY;
> + baseband_xmm_reset(priv);
If the RFKill switch is repeatedly toggled before the USB device enumerates,
could this leak regulator enable counts indefinitely?
When blocking the device, regulator_disable is only called if priv->inited
is true. However, unblocking the device sets priv->inited to false and calls
baseband_xmm_reset which calls regulator_enable unconditionally.
> +
> + priv->powered = false;
> + priv->inited = false;
> + }
> +
> + return 0;
> +}
[ ... ]
> +static void baseband_xmm_work(struct work_struct *work)
> +{
> + struct baseband_xmm_data *priv =
> + container_of(work, struct baseband_xmm_data, modem_work.work);
> +
> + switch (priv->ap_state) {
> + case IPC_AP_WAKE_INIT1:
> + if (priv->powered)
> + return;
> +
> + phy_power_on(priv->mphy);
> + priv->powered = true;
Is there any synchronization protecting these state variables (priv->powered,
priv->inited, priv->ap_state)?
They appear to be accessed and modified concurrently by the delayed workqueue,
the threaded IRQ handler, the RFKill callback, and the USB notifier without
any locking.
For example, could the RFKill callback and this delayed workqueue both see
priv->powered as true and simultaneously execute phy_power_off, corrupting
the PHY framework's internal power count?
> + break;
[ ... ]
> +static irqreturn_t baseband_hostwake_interrupt(int irq, void *dev_id)
> +{
> + struct baseband_xmm_data *priv = dev_id;
> + int state = gpiod_get_value(priv->ipc_ap_gpio);
> +
> + switch (priv->ap_state) {
> + case IPC_AP_WAKE_IRQ_READY:
> + if (!state) {
> + priv->ap_state = IPC_AP_WAKE_INIT1;
> + schedule_delayed_work(&priv->modem_work, 0);
> + }
> +
> + break;
> +
> + case IPC_AP_WAKE_INIT1:
> + if (state) {
> + priv->ap_state = IPC_AP_WAKE_INIT2;
> + schedule_delayed_work(&priv->modem_work,
> + msecs_to_jiffies(BASEBAND_XMM_INIT_DELAY));
Will this properly delay the work if it is already pending?
If the work is already pending from the IPC_AP_WAKE_IRQ_READY state (which
queued it with a 0 delay), schedule_delayed_work will return false and won't
update the timer. The work would then execute immediately, observe
IPC_AP_WAKE_INIT2, and shut down the PHY, bypassing the 5-second delay.
Would mod_delayed_work be more appropriate here to update the timeout?
> + }
> +
> + break;
[ ... ]
> +static int baseband_xmm_probe(struct platform_device *pdev)
> +{
[ ... ]
> + priv->rfkill_dev = rfkill_alloc("xmm-modem", dev, RFKILL_TYPE_WWAN,
> + &baseband_xmm_rfkill_ops, priv);
> + if (priv->rfkill_dev) {
> + ret = rfkill_register(priv->rfkill_dev);
> + if (ret < 0) {
> + rfkill_destroy(priv->rfkill_dev);
> + return dev_err_probe(dev, ret,
> + "failed to register WWAN rfkill\n");
> + }
> + } else {
> + return dev_err_probe(dev, PTR_ERR(priv->rfkill_dev),
> + "failed to allocate WWAN rfkill\n");
Does this error path correctly return a negative error code?
Since rfkill_alloc returns NULL on allocation failure, PTR_ERR(NULL)
evaluates to 0. This causes probe to succeed and return 0 instead of -ENOMEM.
Could this leave the driver bound in a partially initialized state?
> + }
> +
> + priv->nb.notifier_call = baseband_xmm_usb_notifier_call;
> + usb_register_notify(&priv->nb);
> +
> + baseband_xmm_reset(priv);
> + priv->powered = false;
> +
> + return 0;
> +}
> +
> +static void baseband_xmm_remove(struct platform_device *pdev)
> +{
> + struct baseband_xmm_data *priv = platform_get_drvdata(pdev);
> +
> + rfkill_unregister(priv->rfkill_dev);
> + rfkill_destroy(priv->rfkill_dev);
> +
> + usb_unregister_notify(&priv->nb);
> + phy_power_off(priv->mphy);
> +
> + gpiod_set_value_cansleep(priv->reset_gpio, 1);
> + regulator_disable(priv->vbat_supply);
Does this removal sequence race with the devres-managed IRQ and workqueue?
The threaded IRQ and delayed workqueue were allocated with
devm_request_threaded_irq and devm_delayed_work_autocancel, so they are only
canceled after baseband_xmm_remove returns.
If the IRQ fires or the workqueue executes concurrently with or just after
this function, could they interact with the powered-off hardware or call
phy_power_on after the PHY is already disabled here?
Also, are these teardown calls always balanced?
If the device was already blocked via RFKill, or if the initialization
workqueue never ran to power on the PHY and regulator, unconditionally
calling phy_power_off and regulator_disable here might trigger negative
reference count warnings in their respective frameworks.
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=4
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 6/6] phy: tegra: Add support for Nvidia Tegra XMM6260 PHY
From: sashiko-bot @ 2026-05-12 23:29 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: conor+dt, krzk+dt, robh, devicetree, neil.armstrong, vkoul,
linux-phy, olteanv
In-Reply-To: <20260511135703.62470-7-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Incorrect error checking of `of_parse_phandle()` leads to arbitrary device matching and memory corruption.
- [Critical] Type confusion and NULL pointer dereference due to illegal access of an external device's driver data.
- [High] Permanent memory and reference count leak of the USB platform device.
- [Medium] Improper usage of `dev_err_probe()` inside runtime execution paths.
- [Medium] Missing Kconfig dependency on `CONFIG_USB_PHY`.
--
commit c599fd7b5a962e5696a6eb64dff41cd0427d8dde
Author: Svyatoslav Ryhel <clamor95@gmail.com>
phy: tegra: Add support for Nvidia Tegra XMM6260 PHY
Nvidia Tegra XMM6260 PHY is a hardware configuration used in Tegra SoCs
to provide proper interaction between the application processor and the
modem, as well as control over one of the SoC's USB lines for the modem.
> diff --git a/drivers/phy/tegra/Kconfig b/drivers/phy/tegra/Kconfig
> index 342fb736da4b1..41b5ce460f37c 100644
> --- a/drivers/phy/tegra/Kconfig
> +++ b/drivers/phy/tegra/Kconfig
> @@ -18,3 +18,15 @@ config PHY_TEGRA194_P2U
> help
> Enable this to support the P2U (PIPE to UPHY) that is part of Tegra 19x
> and 234 SOCs.
> +
> +config PHY_TEGRA_XMM6260
> + tristate "NVIDIA Tegra XMM6260 PHY driver"
> + depends on ARCH_TEGRA && USB_NET_XMM6260 && USB_SUPPORT
> + select GENERIC_PHY
This driver relies on USB PHY APIs like devm_usb_get_phy_by_phandle(), but
doesn't seem to depend on USB_PHY. Could this cause silent probe failures if
CONFIG_USB_PHY is disabled and the stub functions are used?
[ ... ]
> diff --git a/drivers/phy/tegra/phy-tegra-xmm6260.c b/drivers/phy/tegra/phy-tegra-xmm6260.c
> new file mode 100644
> index 0000000000000..7511de1333aaf
> --- /dev/null
> +++ b/drivers/phy/tegra/phy-tegra-xmm6260.c
[ ... ]
> +struct tegra_usb_device {
> + struct ci_hdrc_platform_data data;
> + struct platform_device *dev;
> +};
[ ... ]
> +static int tegra_xmm6260_phy_power_on(struct phy *phy)
> +{
> + struct tegra_xmm6260_phy *mphy = phy_get_drvdata(phy);
> + struct tegra_usb_device *usb = platform_get_drvdata(mphy->usb_dev);
Does this rely on the exact memory layout of an external driver's private
structure? The tegra_usb_device struct is defined locally here.
Also, if the external driver hasn't probed yet, platform_get_drvdata()
returns NULL, which could lead to a NULL pointer dereference later when
accessing usb->dev.
> + int ret;
> +
> + gpiod_set_value_cansleep(mphy->enable_gpio, 1);
> +
> + ret = usb_phy_init(mphy->usb_phy);
> + if (ret) {
> + gpiod_set_value_cansleep(mphy->enable_gpio, 0);
> + return dev_err_probe(mphy->dev, ret,
> + "failed to init USB PHY\n");
> + }
Is it safe to call dev_err_probe() outside of a probe function? This is a
runtime PHY operation callback, and using dev_err_probe() here might pollute
the deferred probe lists if the error code happens to match -EPROBE_DEFER.
[ ... ]
> +static int tegra_xmm6260_phy_probe(struct platform_device *pdev)
> +{
[ ... ]
> + usb_node = of_parse_phandle(dev->of_node, "nvidia,usb-bus", 0);
> + if (IS_ERR(usb_node))
> + return dev_err_probe(dev, PTR_ERR(usb_node),
> + "failed to parse modem USB bus\n");
Should this check be if (!usb_node) instead? of_parse_phandle() returns NULL
when a property is missing, not an ERR_PTR.
If usb_node is NULL, this check is bypassed and of_find_device_by_node(NULL)
will be called below, which matches the very first platform device in the
system lacking a device tree node.
> +
> + mphy->usb_dev = of_find_device_by_node(usb_node);
> + of_node_put(usb_node);
> + if (!mphy->usb_dev)
> + return dev_err_probe(dev, -ENODEV,
> + "failed to get modem USB bus\n");
Does this leak the reference to the platform_device returned by
of_find_device_by_node()? It increments the reference count via
get_device(), but there doesn't seem to be a corresponding put_device() in
the error paths below or in a remove callback.
> +
> + mphy->usb_phy = devm_usb_get_phy_by_phandle(dev, "nvidia,usb-bus", 1);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=6
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 5/6] dt-bindings: phy: tegra: Document Nvidia Tegra XMM6260 PHY
From: sashiko-bot @ 2026-05-12 23:29 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: krzk+dt, linux-phy, vkoul, robh, olteanv, devicetree,
neil.armstrong, conor+dt
In-Reply-To: <20260511135703.62470-6-clamor95@gmail.com>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The binding models an external USB/HSIC modem as a standalone, virtual 'PHY' node instead of representing it accurately in the hardware hierarchy.
- [Medium] The `nvidia,usb-bus` property groups two unrelated phandles (USB controller and USB PHY) and introduces a reverse-dependency.
- [Low] The compatible string `nvidia,tegra-xmm6260` uses the incorrect vendor prefix for the hardware it represents.
- [Low] The `nvidia,usb-bus` property is defined as a `phandle-array` but lacks the necessary `items` description to define the cell count, leading to a `dt_binding_check` validation failure.
--
commit 9a3c0eae039eb5a94c2d0ec7ecce7c586c326f99
Author: Svyatoslav Ryhel <clamor95@gmail.com>
dt-bindings: phy: tegra: Document Nvidia Tegra XMM6260 PHY
This commit documents the XMM6260 PHY binding, a hardware configuration
used in Tegra SoCs to control power sequencing and manage the USB lines
for the modem.
> diff --git a/Documentation/devicetree/bindings/phy/nvidia,tegra-xmm6260.yaml b/Documentation/devicetree/bindings/phy/nvidia,tegra-xmm6260.yaml
> new file mode 100644
> index 0000000000000..0346433c97725
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/nvidia,tegra-xmm6260.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/nvidia,tegra-xmm6260.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Nvidia Tegra PHY for XMM6260 modem
> +
> +description:
> + A hardware configuration used in Tegra SoCs to provide proper interaction
> + between the application processor and the modem, as well as control over
> + one of the SoC's USB lines for the modem.
Is modeling this external modem as a standalone virtual PHY node the best
approach for representing the hardware topology?
The device tree should describe physical hardware hierarchy. Since the
XMM6260 is an external modem connected to the host via a USB or HSIC bus,
it would traditionally be modeled as a child node of the USB controller.
Handling power sequencing by creating a virtual PHY node is a known
anti-pattern. Power requirements are typically managed by the USB subsystem's
device tree bindings or a generic power sequencing framework.
> +
> +maintainers:
> + - Svyatoslav Ryhel <clamor95@gmail.com>
> +
> +properties:
> + compatible:
> + const: nvidia,tegra-xmm6260
This isn't a bug, but the XMM6260 is a modem manufactured by Infineon/Intel.
Should the compatible string use a vendor prefix corresponding to the actual
manufacturer (e.g., "infineon" or "intel") instead of "nvidia", since this
represents an external component rather than a Tegra-specific SoC block?
> +
> + enable-gpios:
> + description: GPIO connected to the EINT1 pin
> + maxItems: 1
> +
> + nvidia,usb-bus:
> + description:
> + Contains two phandles; the first is pointing to the Host's USB controller
> + and the second linking to the controller's PHY.
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + minItems: 2
> + maxItems: 2
Does this custom property introduce an unnecessary reverse-dependency in the
device tree?
Standard bus hierarchy dictates that relationships are established naturally
via parent-child nodes. Pointing an external device back to its host
controller via a custom phandle is a non-standard way to represent a bus
connection. Additionally, grouping two distinct hardware components (a host
controller and its PHY) into a single phandle array mixes unrelated
references.
Can this property definition cause a validation failure during
dt_binding_check?
Because this is typed as a phandle-array, the dtschema validation framework
will attempt to parse the array elements by looking for a
"#nvidia,usb-bus-cells" property on the referenced target nodes (the USB
controller and PHY). Since those target nodes won't define that custom cells
property, the example schema validation is likely to fail.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511135703.62470-1-clamor95@gmail.com?part=5
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox