Re: [PATCH v9 1/1] gpio: mpfs: add polarfire soc gpio support

[Bartosz Golaszewski <brgl@xxxxxxxx>]

On Fri, Nov 1, 2024 at 4:16 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
>
> On Fri, Nov 01, 2024 at 03:32:16PM +0100, Bartosz Golaszewski wrote:
> > On Fri, Nov 1, 2024 at 3:28 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > >
> > > On Fri, Nov 01, 2024 at 02:47:51PM +0100, Bartosz Golaszewski wrote:
> > > > On Fri, Nov 1, 2024 at 2:37 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > > >
> > > > > On Fri, Nov 01, 2024 at 02:09:06PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Fri, Nov 1, 2024 at 1:17 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > On Thu, Oct 31, 2024 at 02:00:22PM +0100, Bartosz Golaszewski wrote:
> > > > > > > > On Thu, Oct 31, 2024 at 1:04 PM Conor Dooley <conor@xxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > > > +       mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
> > > > > > > > > +       mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
> > > > > > > > > +       mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
> > > > > > > > > +       mpfs_gpio->gc.get = mpfs_gpio_get;
> > > > > > > > > +       mpfs_gpio->gc.set = mpfs_gpio_set;
> > > > > > > > > +       mpfs_gpio->gc.base = -1;
> > > > > > > > > +       mpfs_gpio->gc.ngpio = ngpios;
> > > > > > > >
> > > > > > > > The "ngpios" property will be parsed by GPIO core so no need to set it.
> > > > > > >
> > > > > > > Are you sure it'll work here? I tried dropping the ngpios parsing, but I
> > > > > > > get:
> > > > > > > gpiochip_add_data_with_key: GPIOs 0..-1 (20122000.gpio) failed to register, -22
> > > > > > >
> > > > > > > That's coming from the device_property_read_u32(dev, "ngpios", &ngpios);
> > > > > > > in gpiochip_get_ngpios(). Checking against the bluefield driver and the
> > > > > > > code in gpiochip_add_data_with_key(), it's not immediately obvious what
> > > > > > > I am missing.
> > > > > >
> > > > > > Does dev have an fwnode correctly assigned? What does dev_fwnode(dev) return?
> > > > >
> > > > > It's not a null pointer or something obviously wrong by virtue of being
> > > > > null-adjacent, it is a virtual address but not one that %ps can identify.
> > > >
> > > > This is an OF system right? If you do dev_of_node(dev), does the
> > > > returned node->name show the OF node you expect?
> > >
> > > Yes.
> >
> > I mean inside gpiochip_get_ngpios(), sorry for confusion.
>
> That is what I checked actually, didn't think you'd ask me to check the
> one in probe that works.
>
> > > > Does it have the
> > > > "ngpios" property? Can you read it with of_property_read_u32()?
> > >
> > > No, EINVAL there too.
> >
> > I assume the node is not assigned correctly. What if in your probe you
> > do: gc->fwnode = dev_fwnode(dev)?
>
> Makes no difference, same probe failure as before...
>
>
> ...but I just realised something: ngpios isn't a required property for
> this device as it has a default of 32 and 32 is how many gpios this
> controller has. Simple oversight, hours wasted. That's always the way of
> it I suppose. The core code can't be used here I suppose, since ngpios
> is optional.

Ah right. If we get to gpiochip_get_ngpios() without a GPIO number
set, the property becomes mandatory.

Nevermind my comment from the review then.

Bart