[PATCH] gpio: uclass: Introduce gpio-hog-optional property

[Simon Glass]

Hi,

On Wed, 14 Sept 2022 at 08:31, Nate Drude <nate.d at variscite.com> wrote:
>
> Hi All,
>
> On 9/14/22 9:16 AM, Tom Rini wrote:
> > On Wed, Sep 14, 2022 at 08:59:52AM -0500, Nate Drude wrote:
> >> Hi Simon,
> >>
> >> On 9/14/22 7:49 AM, Simon Glass wrote:
> >>> Hi Nate,
> >>>
> >>> On Mon, 12 Sept 2022 at 14:57, Nate Drude <nate.d at variscite.com> wrote:
> >>>>
> >>>> Hi Simon and Fabio,
> >>>>
> >>>> On 9/12/22 3:16 PM, Simon Glass wrote:
> >>>>> Hi,
> >>>>>
> >>>>> On Mon, 12 Sept 2022 at 12:48, Fabio Estevam <festevam at gmail.com> wrote:
> >>>>>>
> >>>>>> Hi Nate and Simon,
> >>>>>>
> >>>>>> On Mon, Sep 12, 2022 at 2:55 PM Nate Drude <nate.d at variscite.com> wrote:
> >>>>>>>
> >>>>>>> gpio_hog_probe_all is invoked by init_sequence_r in board_r.c.
> >>>>>>> If device_probe fails for any gpio-hog, boot hangs with the following error:
> >>>>>>>
> >>>>>>>> initcall sequence 00000000fffc8e18 failed at call 000000004023b320 (err=-121)
> >>>>>>>> ### ERROR ### Please RESET the board ###
> >>>>>>>
> >>>>>>> gpio-hog-optional allows the boot sequence to continue if device_probe
> >>>>>>> fails for optional gpio-hog(s).
> >>>>>>>
> >>>>>>> Signed-off-by: Nate Drude <nate.d at variscite.com>
> >>>>>>> ---
> >>>>>>>     doc/device-tree-bindings/gpio/gpio.txt | 1 +
> >>>>>>>     drivers/gpio/gpio-uclass.c             | 4 +++-
> >>>>>>>     2 files changed, 4 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/doc/device-tree-bindings/gpio/gpio.txt b/doc/device-tree-bindings/gpio/gpio.txt
> >>>>>>> index 1481ed607d..02d296316b 100644
> >>>>>>> --- a/doc/device-tree-bindings/gpio/gpio.txt
> >>>>>>> +++ b/doc/device-tree-bindings/gpio/gpio.txt
> >>>>>>> @@ -189,6 +189,7 @@ gpio-controller's driver probe function.
> >>>>>>>     Each GPIO hog definition is represented as a child node of the GPIO controller.
> >>>>>>>     Required properties:
> >>>>>>>     - gpio-hog:   A property specifying that this child node represents a GPIO hog.
> >>>>>>> +- gpio-hog-optional: A property specifying to continue boot when device_probe fails in gpio_hog_probe_all
> >>>>>>
> >>>>>> gpio-hog-optional property does not exist in Linux.
> >>>>>>
> >>>>>> If this property is introduced then U-Boot and Linux devicetrees will
> >>>>>> not be in sync.
> >>>>>>
> >>>>>> Can this be fixed differently?
> >>>>>
> >>>>> Nate, can you send a patch to Linux with the binding update?
> >>>>>
> >>>>> Regards,
> >>>>> Simon
> >>>>
> >>>> Thanks for your responses and feedback.
> >>>>
> >>>> I don't think gpio-hog-optional is relevant to Linux.
> >>>
> >>> Sure, but Linux is (for better or worse) the main repo for the device
> >>> tree bindings.
> >>
> >> I am not understanding the action. I think you're suggesting I update the
> >> Linux device tree bindings so they stay aligned with U-Boot, adding a
> >> property gpio-hog-optional after this line: https://github.com/torvalds/linux/blob/v6.0-rc5/Documentation/devicetree/bindings/gpio/gpio.txt#L191
> >>
> >> However, since it's not relevant to Linux, I think it will be confusing
> >> since it will have no effect and won't be be used in any Linux code.
> >>
> >> Can you please advise what description I should use for the
> >> gpio-hog-optional property so that the Linux maintainers would accept such a
> >> patch?

You can mention why it is needed and use a u-boot, property-name prefix perhaps.

> >
> > Yes, that would be one way to go about this, and there are other
> > non-Linux bindings in the Linux kernel tree, but this might be the first
> > property of an existing binding, so that might also be a bit challenging
> > to get accepted, or find out what the preferred solution is.
> >
> > [snip]
> >>>> Do you have any suggestions for a better approach? Does it make sense
> >>>> for gpio_hog_probe_all to cause a fatal error when the gpio hog probe
> >>>> fails (most devices, including the gpio-expander, will not cause a hang
> >>>> if they fail to probe)?
> >>>
> >>> I think your approach is fine as is.
> >>
> >> An alternate approach is to modify the default behavior so that
> >> gpio_hog_probe_all will not trigger a fatal error. Do you think this is
> >> better?
> >
> > This too would be acceptable.

I don't like this as it introduces undefined behaviour and may cause
the boot to fail with no indication of what has happened, nor any way
for higher-level code to resolve the issue.

Anything we do here should be deterministic. Failures should be
ignored only when the board vendor wants that.

> >
>
> Thanks for the discussion and feedback. I prefer to avoid changing the
> bindings in Linux if possible.
>
> Would it be acceptable if I rework gpio_hog_probe_all so that it prints
> an error "Failed to probe device..." if any device_probe fails, but
> always returns 0 to avoid a fatal error in board_init_r? I can submit a
> v2 patch, but I want to screen any problems with the approach first.

How would this be controlled? One option might be to add a binding to
the /options node. I don't like it, but it could be a fallback if you
get push-back on this binding.

Regards,
Simon