Re: [PATCH 1/2] dt-bindings: usb: renesas,usbhs: Deprecate renesas,enable-gpio

Re: [PATCH 1/2] dt-bindings: usb: renesas,usbhs: Deprecate renesas,enable-gpio

[Geert Uytterhoeven]

Hi Wolfram,

CC gpio

On Wed, Oct 2, 2024 at 9:56 AM Wolfram Sang
<wsa+renesas@sang-engineering.com> wrote:
> > +  renesas,enable-gpios:
>
> Isn't this a good occasion to drop the "renesas"-prefix? Binding docs
> are full of plain "enable-gpios".

Well, that's of course another option (actually 3 ;-)
Compared to simply switching from "renesas,enable-gpio" to
"renesas,enable-gpios", dropping the vendor prefix requires changes to
the HS-USB driver and/or to gpiolib.  Worse, this would also become
a hard dependency for updating the DTS files.

Option A: Add a call to devm_gpiod_get_optional(dev, "enable", GPIOD_IN)
as a fallback to usbhs_probe().

Option B: Switch usbhs_probe() from "renesas,enable" to "enable"
and add quirks to of_find_gpio_rename():

    #if IS_ENABLED(CONFIG_USB_RENESAS_USBHS)
                   /*
                    * The Renesas HS-USB DT bindings happened before
enable-gpios
                    * was established as a generic property
                    */
                   { "enable",     "renesas,enable-gpio",
"renesas,rza1-usbhs" },
                   { "enable",     "renesas,enable-gpio",
"renesas,rza2-usbhs" },
                   { "enable",     "renesas,enable-gpio",
"renesas,rzg2l-usbhs" },
                   { "enable",     "renesas,enable-gpio",
"renesas,rcar-gen2-usbhs" },
                   { "enable",     "renesas,enable-gpio",
"renesas,rcar-gen3-usbhs" },
     #endif

Option C: Add a generic "strip vendor prefix" fallback to
of_find_gpio():

    const char *stripped;

    if (gpiod_not_found(desc) && con_id &&
        (stripped = strchr(con_id, ',')) && *(++stripped)) {
            for_each_gpio_property_name(propname, stripped) {
                    desc = of_get_named_gpiod_flags(np, propname, idx,
&of_flags);
                    if (!gpiod_not_found(desc))
                            break;
            }
    }

Option B adds a bit too much to my liking.
Option C may be useful for others (e.g. {ti,nxp,maxim},enable-gpio(s)),
but might be considered too dangerous as a general fallback?

Thoughts?
Thanks!

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 1/2] dt-bindings: usb: renesas,usbhs: Deprecate renesas,enable-gpio

[Linus Walleij]

On Wed, Oct 2, 2024 at 2:10 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:

> Option B: Switch usbhs_probe() from "renesas,enable" to "enable"
> and add quirks to of_find_gpio_rename():
>
>     #if IS_ENABLED(CONFIG_USB_RENESAS_USBHS)
>                    /*
>                     * The Renesas HS-USB DT bindings happened before
> enable-gpios
>                     * was established as a generic property
>                     */
>                    { "enable",     "renesas,enable-gpio",
> "renesas,rza1-usbhs" },
(...)

I would actually prefer this.

> Option C: Add a generic "strip vendor prefix" fallback to
> of_find_gpio():

I understand the appeal, but the idea is for the quirks to be
very specific (hence they are enabled only if specific
drivers are compiled in) and not start to be helpful. Doing
this would make any vendor,foo start to work and I don't like that
at all: any such mechanism will invariably be abused.

Yours,
Linus Walleij

Re: [PATCH 1/2] dt-bindings: usb: renesas,usbhs: Deprecate renesas,enable-gpio

[Rob Herring]

On Wed, Oct 02, 2024 at 04:09:28PM +0200, Linus Walleij wrote:
> On Wed, Oct 2, 2024 at 2:10 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> 
> > Option B: Switch usbhs_probe() from "renesas,enable" to "enable"
> > and add quirks to of_find_gpio_rename():
> >
> >     #if IS_ENABLED(CONFIG_USB_RENESAS_USBHS)
> >                    /*
> >                     * The Renesas HS-USB DT bindings happened before
> > enable-gpios
> >                     * was established as a generic property
> >                     */
> >                    { "enable",     "renesas,enable-gpio",
> > "renesas,rza1-usbhs" },
> (...)
> 
> I would actually prefer this.
> 
> > Option C: Add a generic "strip vendor prefix" fallback to
> > of_find_gpio():
> 
> I understand the appeal, but the idea is for the quirks to be
> very specific (hence they are enabled only if specific
> drivers are compiled in) and not start to be helpful. Doing
> this would make any vendor,foo start to work and I don't like that
> at all: any such mechanism will invariably be abused.

+1

Unless there's a bunch more platforms coming, I'd just stick with this 
patch (or even do nothing).

Rob