Hi,
This patch presents as an example one possible way to use the dynamic
callbacks to clean up one of the i2c chip drivers, adm1026 (for more
information please see
http://archives.andrew.net.au/lm-sensors/msg31310.html).
The first patch defines a new macros like DEVICE_ATTR that also sets
the attribute private data (Greg whats your opinion on defining a
separate set of macros for this v.s. rolling it into one macro?).
The second patch changes adm1026 to pass the sensor index/number via
the private data pointer. I can't test this patch (so you won't want
to apply this) but I'm CCing it to the adm1026 maintainer.
The size difference:
-----------------2.6.11.7--------------------
Module Size Used by
adm1026 44692 0
------2.6.12-rc4-sysdyncallback-----
Module Size Used by
adm1026 32656 0
Signed-off-by: Yani Ioannou <[email protected]>
Thanks,
Yani
---
On Wed, May 11, 2005 at 03:58:35AM -0400, Yani Ioannou wrote:
> -static ssize_t show_in(struct device *dev, char *buf, int nr)
> +static ssize_t show_in(struct device *dev, char *buf, void *private)
> {
> + int nr = *((int*)private);
What's wrong with a simple:
int nr = (int)private;
thanks,
greg k-h
On Wed, May 11, 2005 at 03:58:35AM -0400, Yani Ioannou wrote:
> Hi,
>
> This patch presents as an example one possible way to use the dynamic
> callbacks to clean up one of the i2c chip drivers, adm1026 (for more
> information please see
> http://archives.andrew.net.au/lm-sensors/msg31310.html).
>
> The first patch defines a new macros like DEVICE_ATTR that also sets
> the attribute private data (Greg whats your opinion on defining a
> separate set of macros for this v.s. rolling it into one macro?).
We should make a __ATTR macro instead, right?
> The second patch changes adm1026 to pass the sensor index/number via
> the private data pointer. I can't test this patch (so you won't want
> to apply this) but I'm CCing it to the adm1026 maintainer.
One patch per email please...
thanks,
greg k-h
On 5/11/05, Greg KH <[email protected]> wrote:
> On Wed, May 11, 2005 at 03:58:35AM -0400, Yani Ioannou wrote:
> > -static ssize_t show_in(struct device *dev, char *buf, int nr)
> > +static ssize_t show_in(struct device *dev, char *buf, void *private)
> > {
> > + int nr = *((int*)private);
>
> What's wrong with a simple:
> int nr = (int)private;
Ouch, yes thanks for catching that, that's horribly wrong. Its a
leftover from a previous example where I was actually was passing int*
not int. I'll fix up the example and resend it. That is what comes
from not being able to test it I guess.
> Sorry, but I need a real patch in email form so I can apply it. I can
> handle a 300K+ email :)
>
> Or you can break it up into smaller pieces, like one per major part of
> the kernel. That is the preferred way.
I'd like to break it up, but I think even broken up by major part of
the kernel it one piece will still be too large since the majority of
the changes take place in drivers & drivers/i2c and are very
asymmetric :-(. I'll send you the patch inline privately for now.
> We should make a __ATTR macro instead, right?
Well another __ATTR macro (e.g. ATTR_PRIVATE) would make declaring the
new DEVICE_ATTR_PRIVATE macro, etc, easier. We can't really use __ATTR
nicely though when declaring static attributes and wanting to set the
private data, hence why I think there is the need for a macro (see
http://archives.andrew.net.au/lm-sensors/msg31399.html).
The question really is, is it better to just add that new parameter to
the DEVICE_ATTR macro, or to declare a new DEVICE_ATTR_PRIVATE macro
instead. The former obviously breaks a lot of code although my scripts
can generate another large patch for that too...
Thanks,
Yani
Attached is the corrected adm1026 patch (incorrect casting of void *
argument in callbacks). Justin if you have the ability to test this
patch that would be much appreciated.
Signed-off-by: Yani Ioannou <[email protected]>
Thanks,
Yani
---
adm1026.c | 235
++++++++++++++++++++------------------------------------------ 1 files
changed, 78 insertions(+), 157 deletions(-)
---
On Wed, May 11, 2005 at 03:57:37PM -0400, Yani Ioannou wrote:
> On 5/11/05, Greg KH <[email protected]> wrote:
> > On Wed, May 11, 2005 at 03:58:35AM -0400, Yani Ioannou wrote:
> > Sorry, but I need a real patch in email form so I can apply it. I can
> > handle a 300K+ email :)
> >
> > Or you can break it up into smaller pieces, like one per major part of
> > the kernel. That is the preferred way.
>
> I'd like to break it up, but I think even broken up by major part of
> the kernel it one piece will still be too large since the majority of
> the changes take place in drivers & drivers/i2c and are very
> asymmetric :-(. I'll send you the patch inline privately for now.
No, please break it up. "too large" is a problem for someone trying to
review it too. If the i2c parts are too big, then split them up into
multiple patches too.
> > We should make a __ATTR macro instead, right?
>
> Well another __ATTR macro (e.g. ATTR_PRIVATE) would make declaring the
> new DEVICE_ATTR_PRIVATE macro, etc, easier.
Sorry, yes, that's what I ment.
> The question really is, is it better to just add that new parameter to
> the DEVICE_ATTR macro, or to declare a new DEVICE_ATTR_PRIVATE macro
> instead. The former obviously breaks a lot of code although my scripts
> can generate another large patch for that too...
No, use a new macro.
thanks,
greg k-h