[v1] phy: renesas: rcar-gen3-usb2: Simplify ID/VBUS detection logic

[PATCH] phy: renesas: rcar-gen3-usb2: Simplify ID/VBUS detection logic

Posted by Prabhakar 1 week, 2 days ago

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Read the USB2_ADPCTRL register once in rcar_gen3_check_id() and reuse
the value instead of performing multiple MMIO reads.

Simplify the return logic by comparing the IDDIG and VBUSVALID bits
directly. This preserves the existing behaviour while improving code
clarity and avoiding redundant register accesses.

Reported-by: Pavel Machek <pavel@nabladev.com>
Closes: https://lore.kernel.org/all/acJV-Xq-2uq_JFMn@duo.ucw.cz/
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Note, patch applies on top of next-20260323.
---
 drivers/phy/renesas/phy-rcar-gen3-usb2.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
index 79e820e2fe55..74d7561dbf79 100644
--- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
+++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
@@ -314,13 +314,14 @@ static void rcar_gen3_init_from_a_peri_to_a_host(struct rcar_gen3_chan *ch)
 static bool rcar_gen3_check_id(struct rcar_gen3_chan *ch)
 {
 	if (ch->phy_data->vblvl_ctrl) {
+		u32 val = readl(ch->base + USB2_ADPCTRL);
 		bool vbus_valid;
 		bool device;
 
-		device = !!(readl(ch->base + USB2_ADPCTRL) & USB2_ADPCTRL_IDDIG);
-		vbus_valid = !!(readl(ch->base + USB2_ADPCTRL) & USB2_ADPCTRL_VBUSVALID);
+		device = !!(val & USB2_ADPCTRL_IDDIG);
+		vbus_valid = !!(val & USB2_ADPCTRL_VBUSVALID);
 
-		return vbus_valid ? device : !device;
+		return device == vbus_valid;
 	}
 
 	if (!ch->uses_otg_pins)
-- 
2.53.0

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Simplify ID/VBUS detection logic

Posted by Geert Uytterhoeven 1 week, 2 days ago

Hi Prabhakar,

On Tue, 24 Mar 2026 at 13:16, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Read the USB2_ADPCTRL register once in rcar_gen3_check_id() and reuse
> the value instead of performing multiple MMIO reads.
>
> Simplify the return logic by comparing the IDDIG and VBUSVALID bits
> directly. This preserves the existing behaviour while improving code
> clarity and avoiding redundant register accesses.
>
> Reported-by: Pavel Machek <pavel@nabladev.com>
> Closes: https://lore.kernel.org/all/acJV-Xq-2uq_JFMn@duo.ucw.cz/
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -314,13 +314,14 @@ static void rcar_gen3_init_from_a_peri_to_a_host(struct rcar_gen3_chan *ch)
>  static bool rcar_gen3_check_id(struct rcar_gen3_chan *ch)
>  {
>         if (ch->phy_data->vblvl_ctrl) {
> +               u32 val = readl(ch->base + USB2_ADPCTRL);
>                 bool vbus_valid;
>                 bool device;
>
> -               device = !!(readl(ch->base + USB2_ADPCTRL) & USB2_ADPCTRL_IDDIG);
> -               vbus_valid = !!(readl(ch->base + USB2_ADPCTRL) & USB2_ADPCTRL_VBUSVALID);
> +               device = !!(val & USB2_ADPCTRL_IDDIG);
> +               vbus_valid = !!(val & USB2_ADPCTRL_VBUSVALID);

Perhaps combine variable declarations and assignments?
The "!!" is not needed when assigning to a bool.

>
> -               return vbus_valid ? device : !device;
> +               return device == vbus_valid;
>         }
>
>         if (!ch->uses_otg_pins)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Simplify ID/VBUS detection logic

Posted by Lad, Prabhakar 1 week, 2 days ago

Hi Geert,

Thank you for the review.

On Tue, Mar 24, 2026 at 1:05 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Prabhakar,
>
> On Tue, 24 Mar 2026 at 13:16, Prabhakar <prabhakar.csengg@gmail.com> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Read the USB2_ADPCTRL register once in rcar_gen3_check_id() and reuse
> > the value instead of performing multiple MMIO reads.
> >
> > Simplify the return logic by comparing the IDDIG and VBUSVALID bits
> > directly. This preserves the existing behaviour while improving code
> > clarity and avoiding redundant register accesses.
> >
> > Reported-by: Pavel Machek <pavel@nabladev.com>
> > Closes: https://lore.kernel.org/all/acJV-Xq-2uq_JFMn@duo.ucw.cz/
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Thanks for your patch!
>
> > --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> > @@ -314,13 +314,14 @@ static void rcar_gen3_init_from_a_peri_to_a_host(struct rcar_gen3_chan *ch)
> >  static bool rcar_gen3_check_id(struct rcar_gen3_chan *ch)
> >  {
> >         if (ch->phy_data->vblvl_ctrl) {
> > +               u32 val = readl(ch->base + USB2_ADPCTRL);
> >                 bool vbus_valid;
> >                 bool device;
> >
> > -               device = !!(readl(ch->base + USB2_ADPCTRL) & USB2_ADPCTRL_IDDIG);
> > -               vbus_valid = !!(readl(ch->base + USB2_ADPCTRL) & USB2_ADPCTRL_VBUSVALID);
> > +               device = !!(val & USB2_ADPCTRL_IDDIG);
> > +               vbus_valid = !!(val & USB2_ADPCTRL_VBUSVALID);
>
> Perhaps combine variable declarations and assignments?
> The "!!" is not needed when assigning to a bool.
>
Agreed, I will address it and send a v2.

Cheers,
Prabhakar

Re: [PATCH] phy: renesas: rcar-gen3-usb2: Simplify ID/VBUS detection logic

Posted by Lad, Prabhakar 1 week, 2 days ago

Hi All,

On Tue, Mar 24, 2026 at 12:16 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
>
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Read the USB2_ADPCTRL register once in rcar_gen3_check_id() and reuse
> the value instead of performing multiple MMIO reads.
>
> Simplify the return logic by comparing the IDDIG and VBUSVALID bits
> directly. This preserves the existing behaviour while improving code
> clarity and avoiding redundant register accesses.
>
> Reported-by: Pavel Machek <pavel@nabladev.com>
> Closes: https://lore.kernel.org/all/acJV-Xq-2uq_JFMn@duo.ucw.cz/
My bad, wrong link, https://lore.kernel.org/all/acJVCOdlchLiSe5n@duo.ucw.cz/


Cheers,
Prabhakar