On 5/14/05, Greg KH <greg at kroah.com> wrote: > On Sat, May 14, 2005 at 10:18:38PM +0100, Russell King wrote: > > There are two advantages to this way: > > > > 1. you're not having to impose the extra void * pointer in the > > attribute on everyone. Well this is less of an advantage when you consider that most sysfs attributes in the kernel should be taking advantage of it. A quick look through the various sysfs attributes (e.g. class_attribute, device_attribute, etc) in the kernel and how they are used quickly convinces you that the idea doesn't just address a problem in one or two drivers, or even one particular type of attribute, but most of the kernel sysfs attributes. Of course that problem is more obvious in some sections of the kernel than others. > > 2. you allow people to add whatever data they please to the attribute > > in whatever format they wish - whether it be a void pointer, integer, > > or whatever. You can do just as much with a void * , e.g. define a struct with whatever in it and point to it is functionally the same thing, and my net-sysfs.c patch did this (see lm_sensors archive again), but this is much more obvious, transparent and less error prone (in my opinion). However I never liked passing an int via a pointer for adm1026, despite how 'natural' Greg claims it is ;-). > Ah, nice, I hadn't thought about that. But yes, it would be much > smaller and simpler to do this, very good idea. > > Yani, what do you think? I like it and think its a good idea, just not for the advantages Russell listed :-). Functionally it does the same thing, but in a much more semantically appropriate way: - It is much more obvious by passing the attribute pointer to the callbacks what that extra parameter is used for (identifying which attribute the callback is for). - No awkward and error-prone casting when all you want to do is pass an int. - It will be much more transparent to an outsider reading through the sysfs attribute code that adm1026_attribute represents an adm1026 sensors attribute, and how, etc. > And if enough i2c drivers want to do this, just make a i2c driver > attribute that they all use to achieve this. This sounds like a good idea for the majority of i2c chip drivers, however in special cases like bmcsensors it might have to use it's own attribute type (which in bmcsensor's case would contain the sdr_data struct). My only present worry is that this makes the creation of a standard method to dynamically allocate and create derived sysfs attributes a lot harder (impossible?)... Thanks, Yani
