Messages in this thread | | | Date | Sat, 29 May 2004 12:44:21 +0400 | From | Andrew Zabolotny <> | Subject | Re: two patches - request for comments |
| |
On Fri, 28 May 2004 15:10:06 -0700 Greg KH <greg@kroah.com> 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@cvs.handhelds.org:/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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |