[PATCH 3/9] staging: comedi: change type of auto-config context

[Ian Abbott]

On 2012-10-22 20:43, Greg Kroah-Hartman wrote:
> On Mon, Oct 22, 2012 at 11:21:16AM -0500, H Hartley Sweeten wrote:
>> On Saturday, October 20, 2012 11:37 AM, Ian Abbott wrote:
>>> On 20/10/12 04:44, Dan Carpenter wrote:
>>>> On Mon, Oct 15, 2012 at 01:07:33PM +0100, Ian Abbott wrote:
>>>>> Change the type of the context parameter passed to
>>>>> `comedi_auto_config_helper()` from `void *` to `unsigned long`.  It's
>>>>> currently just an internal change and all current internal usages pass
>>>>> pointers in the context, but future uses of this function may pass
>>>>                                                               ^^^
>>>>> integer values
>>>>
>>>> "May?"  Don't make this kind of change until you have a user ready.
>>>> Using an unsigned long is uglier than using a void pointer so let's
>>>> not do it until it is needed.
>>>>
>>>> Obviously the same thing would apply to the other places.
>>>
>>> Patch 6 passes an unsigned integer value in the context.  In the end,
>>> the context probably won't be used much, but it's handy to have it in
>>> case it's needed.
>>
>> Patch 6 has a different issue..
>>
>> You want to change the context from a void * to an unsigned long.. OK..
>> In Patch 6 you are passing a context of COMEDI_AUTO_ATTACH_{PCI,USB}.
>> You define these as:
>>
>> +/* auto_attach() context values */
>> +#define COMEDI_AUTO_ATTACH_PCI	(-1UL)	/* from comedi_pci_auto_config() */
>> +#define COMEDI_AUTO_ATTACH_USB	(-2UL)	/* from comedi_usb_auto_config() */
>>
>> Seems goofy... -1 and -2 are not 'unsigned'...
>>
>> To me it just seems cleaner to leave the context as a void *. If you want
>> to pass a non-pointer context, cast that context to a void * and then back
>> to its real type when used. Casting a context that is a pointer to some type
>> of struct to an unsigned long and then back to the struct seems messy.
>>
>> Just my opinion...
>
> I agree with this, and I'll take it even farther, why do we need a
> context variable at all?  Shouldn't we be able to live with a properly
> defined type here?  We have control over the code, can't it be cleaned
> up to use an enum or something, that would define the above values
> properly?

Say if a single comedi driver handled both PCMCIA and PCI devices.  It's 
single auto_attach() routine would need some way to tell what sort of 
device it is dealing with (not that auto-attachment of PCMCIA devices is 
currently supported, but it should be easy enough to do).

If a driver needed to allocate some memory before calling 
comedi_auto_config() and pass the memory through to its auto_attach() 
routine, the context parameter could be used for this purpose.

> Relying on a context value to be a "magic" one is ripe for errors and
> confusion.

Most drivers would just ignore the context value anyway, but the "magic" 
values would only matter if they used a predefined wrapper around 
comedi_auto_config(), such as comedi_pci_auto_config().  The same driver 
shouldn't really mix direct calls of comedi_auto_config() with the calls 
to the predefined wrappers such as comedi_pci_auto_config() as that 
could lead to errors and confusion, although I tried to reduce this 
possibility by representing the magic values in a similar way to 
ERR_PTR() values.

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