Re: [PATCH v2] v4l2_device/v4l2_subdev: final (?) version

[David Brownell <david-b@pacbell.net>]

On Saturday 29 November 2008, Hans Verkuil wrote:
> +void v4l2_device_register(struct device *dev, struct v4l2_device *v4l2_dev)
> +{
> +       BUG_ON(!dev || !v4l2_dev || dev_get_drvdata(dev));
> 

Ouch.  Better to return -EINVAL, like most register() calls,
than *ever* use a BUG_ON() for bad parameters.  Same applies
every other place you use BUG_ON, from a quick scan ...

For the unregister() paths a WARN() would be fair.  Again,
any time you're tempted to use BUG() or BUG_ON(), you need
to re-think.  It's hardly ever the right thing to do.  Just
report the error and continue; callers should check, and
clean up if something went wrong.


> +EXPORT_SYMBOL(v4l2_device_register);

This may be a nit, but I wonder why not EXPORT_SYMBOL_GPL,
which seems to be more correct in this case.


Another quasi-style point:  v4l2-device.s and v4l-subdev.c
are so small, and conceptually related, that I'd be tempted
to have one file not two.  Ditto their headers.

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html