Re: two patches - request for comments

[Andrew Zabolotny]

On Fri, 28 May 2004 15:10:06 -0700
Greg KH <greg@xxxxxxxxx> wrote:

> 	- you create the DEVICE_ATTR macro, why not use the one already
> 	  created for you (CLASS_DEVICE_ATTR will work I think.)
Because it would involve unneeded extra garbage into data segment. See:

CLASS_DEVICE_ATTR(_name,_mode,_show,_store) is:
struct class_device_attribute class_device_attr_##_name = {
  ...
}

DECLARE_ATTR is just:
{ ... }

This way, I cannot use CLASS_DEVICE_ATTR for things like:

static struct class_device_attribute bl_class_device_attributes[] = {
	DECLARE_ATTR(...),
	DECLARE_ATTR(...),
	...
};

Instead, I must declare it this way:

CLASS_DEVICE_ATTR(blah1)
CLASS_DEVICE_ATTR(blah2)
CLASS_DEVICE_ATTR(blah3)

and after that:

struct class_device_attribut *blah_ptr [] = {
	blah1, blah2, blah3
};

The second array is absolutely unneeded here (one garbage pointer for every
device attribute plus alignment), since the array is absolutely static. And
even worse, the array cannot be declared __initdata since devices can
register and unregister at any time.

> 	- Don't do a unregister function by passing a string to it.
> 	  Explicitly pass the pointer of the object that you want to
> 	  unregister, like all other kernel interfaces do.  With that
> 	  change you no longer need the class_find_device() patch,
> 	  right?
No. class_find_device was written for lcd_find_device() and
backlight_find_device() (framebuffer devices use them). On the other hand,
the driver that registers the backlight device doesn't have a pointer to the
class device (well, the lcd_register_device could return it). Since the
lcd/backlight names are unique anyway, I don't see any problems with that, and
moreover, it is *registered* by giving it a name, why it should be
unregistered in a different way?

In any case, if you have strong objections against that, this could be changed
of course. But again, it will result in more useless code/data (the driver
will have to store somewhere the device it has registered, or alternatively,
use lcd/backlight_device_find()).

> 	- How about some drivers that actually use this interface?
> 	  Again, you are creating interfaces with no examples of users
> 	  of the interface, which isn't acceptable.
There are already four drivers that implement the lcd/backlight devices (for
Dell Axim X5, iPAQ 2210, Jornada 560, Rover P5), and three framebuffer devices
that were modified to use it (sa1100fb, pxafb and mq1100fb). I would paste
them here but they are quite large, and I wouldn't like to pollute this list,
it's so high-traffic already.

Instead, you can download them from
http://zap.eltrast.ru/data/backlight-lcd-class-devices.tar.bz2 (~33k).

The full sources are in handhelds.org' public CVS
(:pserver:anoncvs@xxxxxxxxxxxxxxxxx:/cvs, repository linux/kernel26).

--
Greetings,
   Andrew
-
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/