Re: [PATCH] Samsung backlight driver

[Dmitry Torokhov]

Hi Greg,

On Fri, Aug 14, 2009 at 12:48:15PM -0700, Greg KH wrote:
> +
> +static int update_status(struct backlight_device *bd)
> +{
> +	if (!pci_device)
> +		return -ENODEV;
> +	if (!backlight_device)

How is this possible?

> +		return -ENODEV;
> +
> +	current_brightness = bd->props.brightness;
> +	set_brightness();
> +	return 0;
> +}
> +
> +static struct backlight_ops backlight_ops = {
> +	.get_brightness	= get_brightness,
> +	.update_status	= update_status,
> +};
> +
> +static int find_video_card(void)
> +{
> +	struct pci_dev *dev = NULL;
> +
> +	while ((dev = pci_get_device(0x8086, 0x27ae, dev)) != NULL) {
> +		/* Found one, so let's save it off */
> +		if (!pci_device)
> +			pci_device = pci_dev_get(dev);

If you need to iterate all matching devices you need to save them all or
release references to "extras" right here... Or, better yet, just break
after you find the first one.

> +	}
> +
> +	if (!pci_device)
> +		return 0;

Why not -ENODEV? What is the point of having this driver loaded if the
device is not available? Then you could drop all silly "if (!pci_device)"
checks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/