[PATCH 01/12] staging: comedi: drivers: introduce comedi_legacy_detach()

Ian Abbott abbotti at mev.co.uk
Thu Apr 18 17:26:14 UTC 2013

On 2013/04/18 04:35 PM, H Hartley Sweeten wrote:
> On Thursday, April 18, 2013 3:37 AM, Ian Abbott wrote:
>> On 2013-04-17 19:16, H Hartley Sweeten wrote:
>>> This function is intended to be used by the comedi legacy (ISA) drivers
>>> either directly as the (*detach) function or as a helper in the drivers
>>> private (*detach) function.
>>> Modify the __comedi_request_region() helper so that it stores the first
>>> struct resource returned by request_region() in the comedi_device.
>>> The comedi_legacy_detach() function will then automatically release the
>>> region during the detach of the driver.
>>> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
>>> Cc: Ian Abbott <abbotti at mev.co.uk>
>>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> <snip>
>> An alternative implementation would be to leave 
>> __comedi_request_region() alone, but change comedi_request_region() to 
>> record the length in a new member dev->iolen at the same time that it 
>> records the start address in dev->iobase.  Then your 
>> comedi_legacy_detach() could do something like:
>> 	if (dev->iolen) {
>> 		release_region(dev->iobase, dev->iolen);
>> 		dev->iolen = 0;
>> 	}
>> I think it seems slightly simpler and shouldn't affect your other 
>> patches.  Any thoughts?
> I feel its cleaner to save the actual resource that was allocated by
> request_resource(), it's less error prone.

Apart from it describing the resource (whose details are already known),
the actual struct resource isn't all that useful.

> But, I could change __comedi_request_region() so it returns the
> allocated resource instead of an errno. Then comedi_request_resource()
> can save it in the comedi_device. Any drivers that request additional
> resources could so the same and use the saved resource when detaching.
> Also, comedi_request_resource() could (should?) sanity check the
> saved resource to make sure a driver does not call it multiple times.

That would mean changing all the callers of __comedi_request_region() again.

Your current method requires drivers requesting multiple regions to call
comedi_request_region() and __comedi_request_region() in a particular
order (if you don't want drivers to have to call release_region() for
dev->iobase by themselves, and remember which region was going to be
released automatically by comedi_legacy_detach()), whereas my suggestion
would allow drivers (regardless of the order they call
comedi_request_region() and __comedi_request_region()) to only have to
call release_region() for regions they requested with
__comedi_request_region(), and leave the releasing of the main
dev->iobase region up to comedi_legacy_detach().

In other words, with my suggestion, a driver using
comedi_legacy_detach() would always pair __comedi_request_region() with
release_region(), and always pair comedi_request_region() with
comedi_legacy_detach() for the dev->iobase region, regardless of the
order in which it requests the regions.

-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti at mev.co.uk>             )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587              )=-

More information about the devel mailing list