Re: [PATCH resend] serial: 8250: Add support for using platform_device resources

[Andy Shevchenko]

On Tue, May 21, 2019 at 04:43:18PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx> writes:
> 
> > On Tue, May 21, 2019 at 01:34:26PM +0200, Esben Haabendal wrote:
> >> Allow getting memory resource (mapbase or iobase) as well as irq from
> >> platform_device resources.
> >> 
> >> The UPF_DEV_RESOURCES flag must be set for devices where platform_device
> >> resources are to be used.  When not set, driver behaves as before.
> >> 
> >> This allows use of the serial8250 driver together with devices with
> >> resources added by platform_device_add_resources(), such as mfd child
> >> devices added with mfd_add_devices().
> >> 
> >> When UPF_DEV_RESOURCES flag is set, the following platform_data fields should
> >> not be used: mapbase, iobase, mapsize, and irq.  They are superseded by the
> >> resources attached to the device.
> >> 
> >
> > Same comment here: Requesting resource is orthogonal to the retrieving or
> > slicing them.
> 
> Yes.  But for MFD devices, I do think it makes sense for the MFD parent
> device to request the entire memory resource, and then split it.

Nope. This is layering violation here: The user of the resources is not
handling them in full.

> And for drivers that actually are aware of the struct resource given,
> both approaches work.  Throwing away the resource.parent information
> and calling out request_mem_region() manually breaks the idea of
> managing IORESOURCE_MEM as a tree structure.

How come? Can you show an example of output without and with your patches?

> Are we not supposed to be using the parent/child part of struct
> resource?

It's about slicing, no-one prevents you to do that. I don't see a problem.
Show the output!

> >> -		if (!request_mem_region(port->mapbase, size, "serial")) {
> >> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> >> +		    !request_mem_region(port->mapbase, size, "serial")) {
> >
> >> -				release_mem_region(port->mapbase, size);
> >> +				if (!(port->flags & UPF_DEV_RESOURCES))
> >> +					release_mem_region(port->mapbase, size);
> >
> >> -		if (!request_region(port->iobase, size, "serial"))
> >> +		if (!(port->flags & UPF_DEV_RESOURCES) &&
> >> +		    !request_region(port->iobase, size, "serial"))
> >
> >> -		release_mem_region(port->mapbase, size);
> >> +		if (!(port->flags & UPF_DEV_RESOURCES))
> >> +			release_mem_region(port->mapbase, size);
> >
> >> -		release_region(port->iobase, size);
> >> +		if (!(port->flags & UPF_DEV_RESOURCES))
> >> +			release_region(port->iobase, size);
> >
> > All these changes are not related to what you describe in the commit message.
> > is a workaround for the bug in the parent MFD driver of the 8250.
> 
> You are right, this is not adequately described in commit message.
> But unless we are not supposed to allow parent/child memory resource
> management, I don't think it is a workaround, but a fix.
> 
> But I can split it out in a separate patch.  Would be nice if I at least
> can get the other part of the change merged.

Like Lee said, and I agree, nothing prevents us to switch to
platform_get_resource().

The stumbling block here is the *requesting* in parent which I strongly
disagree with (at least in a form of this change, I already told you, that this
has to be "fixed" on generic level, not as a hack in one certain driver).

-- 
With Best Regards,
Andy Shevchenko