[PATCH] staging: comedi: make attach handler optional

H Hartley Sweeten hartleys at visionengravers.com
Wed Aug 15 18:00:15 UTC 2012


On Wednesday, August 15, 2012 7:03 AM, Ian Abbott wrote:
> Some low-level Comedi drivers no longer support manual configuration of
> devices with the COMEDI_DEVCONFIG ioctl (used by the comedi_config
> program).  For those drivers, the 'attach_pci' or 'attach_usb' handler
> will be set in the struct comedi_driver to configure devices
> automatically (via comedi_pci_auto_config() or
> comedi_usb_auto_config()).  Their 'attach' handlers are redundant but
> the the comedi core module currently requires it to be set.
>
> Make the 'attach' handler optional and issue a warning if something
> wants to call it.
>
> Signed-off-by: Ian Abbott <abbotti at mev.co.uk>
> ---
>  drivers/staging/comedi/drivers.c |   19 ++++++++++++++++---
>  1 files changed, 16 insertions(+), 3 deletions(-)

Ian,

Assuming this gets merged, I will remove the 'attach' callback from
the drivers that no longer need it.

> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> index c0fdb00..c8adc5e 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -186,6 +186,14 @@ int comedi_device_attach(struct comedi_device *dev, struct comedi_devconfig *it)
>  		}
>  		return -EIO;
>  	}
> +	if (driv->attach == NULL) {
> +		/* driver does not support manual configuration */
> +		dev_warn(dev->class_dev,
> +			 "driver '%s' does not support attach using comedi_config\n",
> +			 driv->driver_name);
> +		module_put(driv->module);
> +		return -ENOSYS;
> +	}
>  	/* initialize dev->driver here so
>  	 * comedi_error() can be called from attach */
>  	dev->driver = driv;
> @@ -885,13 +893,18 @@ static int comedi_auto_config_wrapper(struct comedi_device *dev, void *context)
>  		dev->board_ptr = comedi_recognize(driv, it->board_name);
>  		if (dev->board_ptr == NULL) {
>  			printk(KERN_WARNING
> -			       "comedi: auto config failed to find board entry"
> -			       " '%s' for driver '%s'\n", it->board_name,
> -			       driv->driver_name);
> +			       "comedi: auto config failed to find board entry '%s' for driver '%s'\n",
> +			       it->board_name, driv->driver_name);

Since your modifying this printk, it could (should) also be changed to a dev_warn().

>  			comedi_report_boards(driv);
>  			return -EINVAL;
>  		}
>  	}
> +	if (!driv->attach) {
> +		printk(KERN_WARNING
> +		       "comedi: BUG! driver '%s' using old-style auto config but has no attach handler\n",
> +		       driv->driver_name);

This one should also be a dev_warn().

> +		return -EINVAL;
> +	}
>  	return driv->attach(dev, it);
>  }

Side note about this file.

I would like to break the pci and usb specific functions out into
separate source files (comedi_pci.c and comedi_usb.c) and
conditionally compile them in based on the Kconfig
CONFIG_COMEDI_PCI_DRIVERS and CONFIG_COMEDI_USB_DRIVERS
options. This lets us get rid of the '#if IS_ENABLED(CONFIG_USB)' .

I also would like to break the comedi_buf_* functions out to
a separate source file (comedi_buf.c). It doesn't seem appropriate
for them to be in the "drivers.c" source.

Do you have any objections to this?

Regards,
Hartley




More information about the devel mailing list