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

H Hartley Sweeten hartleys at visionengravers.com
Thu Apr 18 20:26:16 UTC 2013


On Thursday, April 18, 2013 10:26 AM, Ian Abbott wrote:
> 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.

Yah. But there are only 6 drivers that use it.

I did a quick look and 5 of them can be changed easily. They are the das16,
Das16m1, das1800, pcl816 and unioxx5 drivers. The 8255 driver is the only
other caller of __comedi_request_region(). That one is a bit ugly. I think
it's better leaving it the way it is now.

> 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.

Fair enough. 

I'll update the patch and hopefully get the series reposted today.

Regards,
Hartley



More information about the devel mailing list