gpio: remove gpiobus_attach_bus

[vexeduxr]

Summary

Since gpiobus_attach_bus can attach the gpiobus child along with its
children in the same bus pass, the parent controller's reference to
gpiobus might not be set by the time the children need it. Instead,
drivers should use gpiobus_add_bus and explicitly call
bus_attach_children.

[jhb]

To be clear, is the issue that bus drivers need to access this member in their softc in various bus methods called by child drivers?

I actually think it would be cleaner to push bus_attach_children out to the callers FWIW even though that means a bit more code in each driver. In particular, what if a parent device has multiple children (both a gpio bus and something else, similar to how some PCI-ISA bridges on x86 also have iicbus or smbus children)? In that case, this function can end up attaching other non-gpio devices. I think it is better to let the parent driver figure out the timing of when it attaches child devices.

sys/dev/amdgpio/amdgpio.c

412

sys/dev/bhnd/cores/chipc/chipc_gpio.c

176

sys/dev/gpio/chvgpio.c

446

[vexeduxr]

In D51578#1182442, @jhb wrote:

To be clear, is the issue that bus drivers need to access this member in their softc in various bus methods called by child drivers?

Yes. Specifically, children may call GPIO_GET_BUS, which expects sc->sc_busdev to be initialized.

I actually think it would be cleaner to push bus_attach_children out to the callers FWIW even though that means a bit more code in each driver. In particular, what if a parent device has multiple children (both a gpio bus and something else, similar to how some PCI-ISA bridges on x86 also have iicbus or smbus children)? In that case, this function can end up attaching other non-gpio devices. I think it is better to let the parent driver figure out the timing of when it attaches child devices.

We recently added gpiobus_add_bus for this use case. AFAICT only the as3722 and max77620 drivers need it at this point. I did think about removing gpiobus_attach_bus and using gpiobus_add_bus instead, but I thought this way would be better since it retains the expected behavior when adding a gpiobus child, and drivers that need more control can use gpiobus_add_bus anyways.

sys/dev/amdgpio/amdgpio.c

412

I'm just following the style of the file here. Ditto for the other two.

[vexeduxr]

Rebase after D43034

[mmel]

Sorry for the delayed response, I've very busy in real life again.

IMHO, This looks like a hack, papering over real issue.

The comment above gpiobus_attach_bus() says that this function can only be used after the controller has been fully initialized, including the initialization of all sc fields. In all other cases, we should use gpiobus_add_bus() and a separate bus_attach_children() function in the right place. This, of course, greatly reduces the usefulness of gpiobus_attach_bus(). I didn't realize this when reviewing the previous patch, my apologies.

[vexeduxr]

In D51578#1183603, @mmel wrote:

Sorry for the delayed response, I've very busy in real life again.

No worries.

IMHO, This looks like a hack, papering over real issue.

The comment above gpiobus_attach_bus() says that this function can only be used after the controller has been fully initialized, including the initialization of all sc fields. In all other cases, we should use gpiobus_add_bus() and a separate bus_attach_children() function in the right place. This, of course, greatly reduces the usefulness of gpiobus_attach_bus(). I didn't realize this when reviewing the previous patch, my apologies.

Sorry, not sure I follow. All of these drivers should be fully initialized apart from the busdev field (which isn't really the GPIO driver's fault). We can update comment above gpiobus_attach_bus to clarify that the function itself initializes busdev though.
The only reason I'd like to keep gpiobus_attach_bus around is to keep the expected/known behavior unchanged.

[vexeduxr]

If there aren't any objections, I'd like to land this on Monday.

[imp]

Absent objections, this is approved for monday

[jrtc27]

I read mmel and jhb as objecting?

[vexeduxr]

I believe I addressed jhb's concerns. I tried to address mmel's concerns, but I'm not sure if I did. He'd need to elaborate.

[mmel]

I apologize. I should not write reviews when I am short on time.
I wanted to say that I fully agree with jhb. The gpiobus_attach_bus() function is too limited to be generally useful.
That is why I vote to replace it with the gpiobus_add_bus() function and explicit bus_attach_children().

In an ideal world, we should request call to bus_attach_children() function based on the selected return code from the driver's attach() function and let newbus to attach the children at the optimal place...

[vexeduxr]

Alright, maybe I was too focused on keeping the original behavior. I'll remove gpiobus_attach_bus and replace it with gpiobus_add_bus. This would also make gpiobus more inline with other busses, which is good.

[vexeduxr]

Remove gpiobus_attach_bus() and use gpiobus_add_bus() in it's place.

[mmel]

Thanks for patience, I know how much frustrating can be these change requests based on "subjective" nuances .

[vexeduxr]

In D51578#1184984, @mmel wrote:

Thanks for patience, I know how much frustrating can be these change requests based on "subjective" nuances .

It's no problem :)
Thanks for taking the time to review.

[jhb]

Thanks for taking the time to rework this. I was AFK most of this week, but this looks good to me post-commit.