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

[Greg Kroah-Hartman]

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?

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

thanks,

greg k-h