Hi
This patch implements the sysfs support in the cyclades async driver,
which is needed by the Cyclades' CyMonitor application. It is based in
the last one sent (see copied message below), but implements the changes
asked for that patch by Greg (the array of attributes was eliminated and
now there is only one file for each value to be exported).
I hope it is ok to be applied now, but if more changes are needed I
would be pleased to listen about them. By the way, I'm grateful to
Marcelo for taking time to examining many "middle" versions of this
patch, and also to Greg for his comments to the last patch.
Kind regards,
Germano
========================================================================
From: Greg KH <[email protected]>
Date: Tue, 28 Sep 2004 07:12:43 -0700
To: Marcelo Tosatti <[email protected]>
Cc: [email protected], [email protected],
[email protected]
In-Reply-To: <[email protected]>
Subject: Re: [PATCH] cyclades.c sysfs statistics support
X-MIMETrack: Itemize by SMTP Server on USMail/Cyclades(Release
6.5.1|January 21, 2004) at
09/28/2004 07:12:51
On Tue, Sep 28, 2004 at 09:04:21AM -0300, Marcelo Tosatti wrote:
> + device_create_file(&(cy_card[info->card].pdev->dev),
&_cydas[line]);
Why the array of attributes? As you only have one (which is wrong...)
you only need one attribute structure.
> + show_sys_data - shows the data exported to sysfs/device, mostly the
signals status involved in the
> + serial communication such as CTS,RTS,DTS,etc
NO! sysfs is 1 value per file. Not a whole bunch of values per one
file. Please change this to create a whole bunch of little files, not
one big one.
thanks,
greg k-h
-- End forwarded message --
=================================================================================================
On Thu, Oct 28, 2004 at 04:56:30PM -0200, Germano Barreiro wrote:
> Hi
>
> This patch implements the sysfs support in the cyclades async driver,
> which is needed by the Cyclades' CyMonitor application. It is based in
> the last one sent (see copied message below), but implements the changes
> asked for that patch by Greg (the array of attributes was eliminated and
> now there is only one file for each value to be exported).
> I hope it is ok to be applied now, but if more changes are needed I
> would be pleased to listen about them. By the way, I'm grateful to
> Marcelo for taking time to examining many "middle" versions of this
> patch, and also to Greg for his comments to the last patch.
Close, it's getting better, but you still have a ways to go...
> --- linux-2.6.10rc1.orig/drivers/char/cyclades.c 2004-10-25 16:40:00.000000000 -0200
> +++ linux-2.6.10rc1/drivers/char/cyclades.c 2004-10-26 17:13:20.000000000 -0200
> @@ -646,6 +646,7 @@ static char rcsid[] =
> #include <linux/string.h>
> #include <linux/fcntl.h>
> #include <linux/ptrace.h>
> +#include <linux/device.h>
> #include <linux/cyclades.h>
> #include <linux/mm.h>
> #include <linux/ioport.h>
> @@ -700,8 +701,36 @@ static void cy_send_xchar (struct tty_st
>
> #define JIFFIES_DIFF(n, j) ((j) - (n))
>
> +static char _version[150];
> static struct tty_driver *cy_serial_driver;
>
> +
> +const static char sysufixes[18][10]={"stat","card","baud","flow","rxfl","txfl","dcd","dsr","cts",
> + "rts","dtr","rx","tx","bdrx","bdtx","par","stop","chlen"};
Ick, this isn't needed.
> +ssize_t (*showfunctions[])(struct device *, char *)={
> + show_sys_stat,
> + show_sys_card,
> + show_sys_baud,
> + show_sys_flow,
> + show_sys_rxfl,
> + show_sys_txfl,
> + show_sys_dcd,
> + show_sys_dsr,
> + show_sys_cts,
> + show_sys_rts,
> + show_sys_dtr,
> + show_sys_rx,
> + show_sys_tx,
> + show_sys_bdrx,
> + show_sys_bdtx,
> + show_sys_par,
> + show_sys_stop,
> + show_sys_chlen
> +};
Why not just do as the i2c chip drivers do? Use a macro for your
show functions, it's much simpler.
> + switch(whatinfo){
> +
> + case STAT: //open/closed
Break this big switch statement up into the individual show functions.
Hm, ok, you can't use a macro for them then, sorry. But it should be
simpler.
> + if ( tty == 0 )
> + len = sprintf(buf, "%s:%s\n", cysys_state, cysys_close);
> + else
> + len = sprintf(buf, "%s:%s\n", cysys_state, cysys_open);
No, don't put the %s: stuff in there. The user read from the "stat"
file. They know what the value should be, you don't have to remind them
again :)
thanks,
greg k-h
Hi Greg
Firstly, very thanks again you for your comments. I think I understood them and
I will check the driver you pointed as an example.
Kind regards,
Germano
C?pia Greg KH <[email protected]>:
> On Thu, Oct 28, 2004 at 04:56:30PM -0200, Germano Barreiro wrote:
> > Hi
> >
> > This patch implements the sysfs support in the cyclades async driver,
> > which is needed by the Cyclades' CyMonitor application. It is based
> in
> > the last one sent (see copied message below), but implements the
> changes
> > asked for that patch by Greg (the array of attributes was eliminated
> and
> > now there is only one file for each value to be exported).
> > I hope it is ok to be applied now, but if more changes are needed I
> > would be pleased to listen about them. By the way, I'm grateful to
> > Marcelo for taking time to examining many "middle" versions of this
> > patch, and also to Greg for his comments to the last patch.
>
> Close, it's getting better, but you still have a ways to go...
>
> > --- linux-2.6.10rc1.orig/drivers/char/cyclades.c 2004-10-25
> 16:40:00.000000000 -0200
> > +++ linux-2.6.10rc1/drivers/char/cyclades.c 2004-10-26
> 17:13:20.000000000 -0200
> > @@ -646,6 +646,7 @@ static char rcsid[] =
> > #include <linux/string.h>
> > #include <linux/fcntl.h>
> > #include <linux/ptrace.h>
> > +#include <linux/device.h>
> > #include <linux/cyclades.h>
> > #include <linux/mm.h>
> > #include <linux/ioport.h>
> > @@ -700,8 +701,36 @@ static void cy_send_xchar (struct tty_st
> >
> > #define JIFFIES_DIFF(n, j) ((j) - (n))
> >
> > +static char _version[150];
> > static struct tty_driver *cy_serial_driver;
> >
> > +
> > +const static char
> sysufixes[18][10]={"stat","card","baud","flow","rxfl","txfl","dcd","dsr","cts",
> > +
> "rts","dtr","rx","tx","bdrx","bdtx","par","stop","chlen"};
>
> Ick, this isn't needed.
>
> > +ssize_t (*showfunctions[])(struct device *, char *)={
> > + show_sys_stat,
> > + show_sys_card,
> > + show_sys_baud,
> > + show_sys_flow,
> > + show_sys_rxfl,
> > + show_sys_txfl,
> > + show_sys_dcd,
> > + show_sys_dsr,
> > + show_sys_cts,
> > + show_sys_rts,
> > + show_sys_dtr,
> > + show_sys_rx,
> > + show_sys_tx,
> > + show_sys_bdrx,
> > + show_sys_bdtx,
> > + show_sys_par,
> > + show_sys_stop,
> > + show_sys_chlen
> > +};
>
> Why not just do as the i2c chip drivers do? Use a macro for your
> show functions, it's much simpler.
>
> > + switch(whatinfo){
> > +
> > + case STAT: //open/closed
>
> Break this big switch statement up into the individual show functions.
> Hm, ok, you can't use a macro for them then, sorry. But it should be
> simpler.
>
> > + if ( tty == 0 )
> > + len = sprintf(buf, "%s:%s\n", cysys_state, cysys_close);
> > + else
> > + len = sprintf(buf, "%s:%s\n", cysys_state, cysys_open);
>
> No, don't put the %s: stuff in there. The user read from the "stat"
> file. They know what the value should be, you don't have to remind
> them
> again :)
>
> thanks,
>
> greg k-h
>
On Tue, Nov 02, 2004 at 02:51:33PM -0600, Kilau, Scott wrote:
> I know you have done work on USB serial drivers with devices with
> multiple ports...
> Is there any way to create a file in sys that can point back to a port,
> and NOT the port's
> parent (ie, the board) WITHOUT having to create a new kobject per port?
What's wrong with the kobject in /sys/class/tty/ which has one object
per port? I think we might not be exporting that class_device
structure, but I would not have a problem with doing that.
thanks,
greg k-h
Hi
I will have to study again the code I tried first (it was long ago), but
the main problem was that due to that class be (somehow) derived from
class_simple, I can one export using it the major and minor numbers for
the device. Tosatti, maybe you can complete my answer with details,
since it was you that advised me about this limitation.
However, this was some time ago (kernel 2.6.7 was going to be released),
and I didn't check how much sysfs for the tty drivers has changed since
them. If I can attach this data (signalling states) to the port, it
would be very preferable than attaching to the board as me and Scott are
trying. Even because his advise about the possibility of my patch be
overwritting one channel data with other's make a lot of sense and I
will have to test it (I'm grateful for you, Scott).
Cheers :)
Germano
On Tue, Nov 02, 2004 at 02:51:33PM -0600, Kilau, Scott wrote:
> > I know you have done work on USB serial drivers with devices with
> > multiple ports...
> > Is there any way to create a file in sys that can point back to a port,
> > and NOT the port's
> > parent (ie, the board) WITHOUT having to create a new kobject per port?
What's wrong with the kobject in /sys/class/tty/ which has one object
per port? I think we might not be exporting that class_device
structure, but I would not have a problem with doing that.
thanks,
greg k-h
Hi Greg, all,
> What's wrong with the kobject in /sys/class/tty/ which has one object
> per port? I think we might not be exporting that class_device
> structure, but I would not have a problem with doing that.
> greg k-h
Using the simple class tty kobject that tty_io.c keeps might work for my
needs.
However, there is one thing that stopped me from using it earlier...
The naming of the directory (tty name) in /sys/class/tty is forced to
be:
"sprintf(p, "%s%d", driver->name, index + driver->name_base);"
Is it possible we could change this to be more relaxed about the naming
scheme?
Maybe we can allow a "custom" name to be sent into the
tty_register_device() call?
Like add another option parameter called "custom_name" that if non-NULL,
is used instead of the derived name?
Scott Kilau
Digi International
-----Original Message-----
From: Greg KH [mailto:[email protected]]
Sent: Tuesday, November 02, 2004 8:28 PM
To: Kilau, Scott
Cc: [email protected]; [email protected]
Subject: Re: patch for sysfs in the cyclades driver
On Tue, Nov 02, 2004 at 02:51:33PM -0600, Kilau, Scott wrote:
> I know you have done work on USB serial drivers with devices with
> multiple ports...
> Is there any way to create a file in sys that can point back to a
port,
> and NOT the port's
> parent (ie, the board) WITHOUT having to create a new kobject per
port?
> What's wrong with the kobject in /sys/class/tty/ which has one object
> per port? I think we might not be exporting that class_device
> structure, but I would not have a problem with doing that.
> greg k-h
On Wed, Nov 03, 2004 at 01:55:51PM -0600, Kilau, Scott wrote:
> Hi Greg, all,
>
> > What's wrong with the kobject in /sys/class/tty/ which has one object
> > per port? I think we might not be exporting that class_device
> > structure, but I would not have a problem with doing that.
> > greg k-h
>
> Using the simple class tty kobject that tty_io.c keeps might work for my
> needs.
>
> However, there is one thing that stopped me from using it earlier...
>
> The naming of the directory (tty name) in /sys/class/tty is forced to
> be:
> "sprintf(p, "%s%d", driver->name, index + driver->name_base);"
>
> Is it possible we could change this to be more relaxed about the naming
> scheme?
Why? That's the kernel name for this tty device, right? Why would you
want to change this?
> Maybe we can allow a "custom" name to be sent into the
> tty_register_device() call? Like add another option parameter called
> "custom_name" that if non-NULL, is used instead of the derived name?
Why? What would you call it that would be any different from what we
use today? I guess I don't understand why you don't like the kernel
names.
thanks,
greg k-h
> > Maybe we can allow a "custom" name to be sent into the
> > tty_register_device() call? Like add another option parameter
called
> > "custom_name" that if non-NULL, is used instead of the derived name?
> Why? What would you call it that would be any different from what we
> use today? I guess I don't understand why you don't like the kernel
> names.
> greg k-h
Well, tty name compatibly reasons with a couple of our drivers.
Most of our new Linux users for a couple of our older products are
coming
from a specific different OS who are adamant that we keep the tty names
the way they were used to under that OS.
Also, I can see some oddball products out there that might need
only 1 tty out there.
Instead of forcing "ttyoddball0" for the name, it would
be nice to let the driver use "ttyoddball", or whatever it wanted.
In fact, it would be similar to the existing entries for "console" and
"ptmx"...
Scott Kilau
Digi International
On Wed, Nov 03, 2004 at 02:20:39PM -0600, Kilau, Scott wrote:
> > > Maybe we can allow a "custom" name to be sent into the
> > > tty_register_device() call? Like add another option parameter
> called
> > > "custom_name" that if non-NULL, is used instead of the derived name?
>
> > Why? What would you call it that would be any different from what we
> > use today? I guess I don't understand why you don't like the kernel
> > names.
>
> > greg k-h
>
> Well, tty name compatibly reasons with a couple of our drivers.
>
> Most of our new Linux users for a couple of our older products are
> coming
> from a specific different OS who are adamant that we keep the tty names
> the way they were used to under that OS.
>
> Also, I can see some oddball products out there that might need
> only 1 tty out there.
>
> Instead of forcing "ttyoddball0" for the name, it would
> be nice to let the driver use "ttyoddball", or whatever it wanted.
Your driver can use whatever name you wanted, as long as it's the
LANNANA name that you asked for and were assigned. We do have standards
for a good reason, and the kernel will follow them.
That being said, have your customers use a tool like udev. Then they
can name their tty devices whatever they want. No limitations there at
all.
Hope this helps,
greg k-h
> Your driver can use whatever name you wanted, as long as it's the
> LANNANA name that you asked for and were assigned. We do have
standards
> for a good reason, and the kernel will follow them.
>That being said, have your customers use a tool like udev. Then they
> can name their tty devices whatever they want. No limitations there
at
> all.
The problem is that LANANA assumes a product/driver
may only have a few tty's, say up to maybe 256 or so.
For example, we have a driver that can support 1000s or 10000s of tty's.
When dealing with that large amount of ttys, telling a customer
that they should remember that a tty down in Austin TX is ttyD19234,
and that the tty over in England is ttyD57267 is pretty ridiculous.
Our customers want to select a custom tty name base,
as well as a custom tty port number...
This way they can use logical names in an "area" specific range.
For example, maybe they have 10 16 port units in down in Texas,
they may want to group them as /dev/ttytx000 to /dev/ttytx159,
and the guy in england might want to name theirs
/dev/ttyengland_a to /dev/ttyengland_h
I agree using udev is a solution to the final name problem in /dev,
as long as they are using 2.6, (altough I have to support 2.4 as well).
But using udev still doesn't allow me to create that custom name in
/sys/class/tty.
I understand this is where you would say we should use the ttyD12143
value,
but I feel that it simply doesn't show the "linkage" between that value
and the "custom" name in /dev as easily as it would if I could create a
custom name for the tty in /sys/class/tty.
Scott
On Wed, Nov 03, 2004 at 04:35:44PM -0600, Kilau, Scott wrote:
>
> > Your driver can use whatever name you wanted, as long as it's the
> > LANNANA name that you asked for and were assigned. We do have
> standards
> > for a good reason, and the kernel will follow them.
>
> >That being said, have your customers use a tool like udev. Then they
> > can name their tty devices whatever they want. No limitations there
> at
> > all.
>
> The problem is that LANANA assumes a product/driver
> may only have a few tty's, say up to maybe 256 or so.
>
> For example, we have a driver that can support 1000s or 10000s of tty's.
Great, use udev for that.
> When dealing with that large amount of ttys, telling a customer
> that they should remember that a tty down in Austin TX is ttyD19234,
> and that the tty over in England is ttyD57267 is pretty ridiculous.
I agree, use udev.
> Our customers want to select a custom tty name base,
> as well as a custom tty port number...
> This way they can use logical names in an "area" specific range.
I agree, use udev.
> For example, maybe they have 10 16 port units in down in Texas,
> they may want to group them as /dev/ttytx000 to /dev/ttytx159,
> and the guy in england might want to name theirs
> /dev/ttyengland_a to /dev/ttyengland_h
Fine, have them use udev.
> I agree using udev is a solution to the final name problem in /dev,
> as long as they are using 2.6, (altough I have to support 2.4 as well).
We aren't talking about 2.4 here though. 2.4 is a totally different
subject.
> But using udev still doesn't allow me to create that custom name in
> /sys/class/tty.
You don't want to do that. The kernel doesn't want you to do that.
Userspace doesn't want you to do that. No one wants that.
> I understand this is where you would say we should use the ttyD12143
> value,
> but I feel that it simply doesn't show the "linkage" between that value
> and the "custom" name in /dev as easily as it would if I could create a
> custom name for the tty in /sys/class/tty.
udev will show you that /dev/ttyfoo really is /sys/class/tty/ttyD12143:
$ udevinfo -n /dev/ttyfoo -q path
/class/tty/ttyD12143
So you do have that "linkage". Don't mess around with kernel names,
it's not allowed. Mess around with userspace names, that's allowed.
thanks,
greg k-h
On Wed, Nov 03, 2004 at 11:09:08AM -0200, Germano wrote:
> Hi
>
> I will have to study again the code I tried first (it was long ago), but
> the main problem was that due to that class be (somehow) derived from
> class_simple, I can one export using it the major and minor numbers for
> the device. Tosatti, maybe you can complete my answer with details,
> since it was you that advised me about this limitation.
> However, this was some time ago (kernel 2.6.7 was going to be released),
> and I didn't check how much sysfs for the tty drivers has changed since
> them. If I can attach this data (signalling states) to the port, it
> would be very preferable than attaching to the board as me and Scott are
> trying. Even because his advise about the possibility of my patch be
> overwritting one channel data with other's make a lot of sense and I
> will have to test it (I'm grateful for you, Scott).
The problem was class_simple only contains the "dev" attribute. You can't
add other attributes to it.
The correct thing should be to create "class_tty" with all necessary attributes
(speed, data transferred, etc).
But thats not a v2.6 thing I believe.
I talked to Greg about this at the time and he agreed we need a "class_tty"
type.
> Cheers :)
> Germano
>
> On Tue, Nov 02, 2004 at 02:51:33PM -0600, Kilau, Scott wrote:
> > > I know you have done work on USB serial drivers with devices with
> > > multiple ports...
> > > Is there any way to create a file in sys that can point back to a port,
> > > and NOT the port's
> > > parent (ie, the board) WITHOUT having to create a new kobject per port?
> What's wrong with the kobject in /sys/class/tty/ which has one object
> per port? I think we might not be exporting that class_device
> structure, but I would not have a problem with doing that.
> From: Marcelo Tosatti
> The problem was class_simple only contains the "dev" attribute. You
can't
> add other attributes to it.
Ah, that changes everything.
The entire reason Germano and I were chasing down this option,
was so we could export various "tty" statistic files out to below
each respective tty name in /sys/class/tty
If its currently not possible to add more attributes to the simple
class,
then we are probably going down the wrong avenue here, at least for now.
Greg,
I know you are a very busy person...
Is making a "tty class" even in the cards for 2.6, or is it scheduled
for 2.7+ ?
Germano,
I still hate doing it, and I know it is against the "1 value per file in
sys" rule,
but for now I think exporting the values to the "card" directory with
each file
containing the value as a list of ports, 1 per line, might be the best
option
to work with here, at least until the "tty class" gets developed.
Scott Kilau
Digi International
Marcelo> The problem was class_simple only contains the "dev"
Marcelo> attribute. You can't add other attributes to it.
I believe, based on the comment in class_simple.c:
Any further sysfs files that might be required can be created using this pointer.
and the implementation in in drivers/scsi/st.c, that there's no
problem adding attributes to a device in a simple class. You can just
use class_set_devdata() on your class_device to set whatever context
you need to get back to your internal structures, and then use
class_device_create_file() to add the attributes.
I assume this is OK (since there is already one in-kernel driver doing
it), but Greg, can you confirm that it's definitely OK for a driver to
use class_set_devdata() on a class_device from class_simple_device_add()?
Thanks,
Roland
On Thu, Nov 04, 2004 at 08:58:21AM -0800, Roland Dreier wrote:
> Marcelo> The problem was class_simple only contains the "dev"
> Marcelo> attribute. You can't add other attributes to it.
>
> I believe, based on the comment in class_simple.c:
>
> Any further sysfs files that might be required can be created using this pointer.
>
> and the implementation in in drivers/scsi/st.c, that there's no
> problem adding attributes to a device in a simple class. You can just
> use class_set_devdata() on your class_device to set whatever context
> you need to get back to your internal structures, and then use
> class_device_create_file() to add the attributes.
>
> I assume this is OK (since there is already one in-kernel driver doing
> it), but Greg, can you confirm that it's definitely OK for a driver to
> use class_set_devdata() on a class_device from class_simple_device_add()?
Hi Roland,
Oh thanks, I didnt knew the existance of such possibily.
I once asked here on the list:
---------
Hope this is not a FAQ.
I want to export some read-only attributes (statistics) from cyclades.c char
driver to userspace via sysfs.
I can't figure out the right place to do it - I could create a class under
/sys/class/cyclades for example, but that doesnt sound right since this
is not a "class" of device, but a device itself.
Hooking the statistics into /sys/class/tty/ttyC$/ sounds reasonable, but
its not possible it seems because "tty" is a class_simple class, which only implements
the "dev" attribute.
------ Greg answer was:
For a driver only attribute, you want them to show up in the place for
the driver (like under /sys/bus/pci/driver/MY_FOO_DRIVER/). To do that
use the DRIVER_ATTR() and the driver_add_file() functions. For
examples, see the other drivers that use these functions.
But I hope you are right - /sys/class/tty/tty$/
sounds the correct place for those files - I thought a "class_tty"
class was required for new attributes.
Marcelo> ------ Greg answer was:
Greg> For a driver only attribute, you want them to show up in the
Greg> place for the driver (like under
Greg> /sys/bus/pci/driver/MY_FOO_DRIVER/). To do that use the
Greg> DRIVER_ATTR() and the driver_add_file() functions. For
Greg> examples, see the other drivers that use these functions.
I think Greg may have misunderstood the question and told you how to
expose per-driver attributes. But I think the attributes you want
really are per-device and should be attached to the class_device, not
the driver.
- Roland
On Thu, Nov 04, 2004 at 08:58:21AM -0800, Roland Dreier wrote:
> Marcelo> The problem was class_simple only contains the "dev"
> Marcelo> attribute. You can't add other attributes to it.
>
> I believe, based on the comment in class_simple.c:
>
> Any further sysfs files that might be required can be created using this pointer.
>
> and the implementation in in drivers/scsi/st.c, that there's no
> problem adding attributes to a device in a simple class. You can just
> use class_set_devdata() on your class_device to set whatever context
> you need to get back to your internal structures, and then use
> class_device_create_file() to add the attributes.
>
> I assume this is OK (since there is already one in-kernel driver doing
> it), but Greg, can you confirm that it's definitely OK for a driver to
> use class_set_devdata() on a class_device from class_simple_device_add()?
Hm, I think that should be ok, but I'd make sure to test it before
verifying that it really is :)
thanks,
greg k-h
On Thu, Nov 04, 2004 at 10:40:26AM -0600, Kilau, Scott wrote:
> > From: Marcelo Tosatti
> > The problem was class_simple only contains the "dev" attribute. You
> can't
> > add other attributes to it.
>
> Ah, that changes everything.
No, you can add attributes to it, I just don't think the pointer is
accessable for a tty driver to be able to find the thing. Need to go
look at the code again to verify this or not.
> The entire reason Germano and I were chasing down this option,
> was so we could export various "tty" statistic files out to below
> each respective tty name in /sys/class/tty
>
> If its currently not possible to add more attributes to the simple
> class,
> then we are probably going down the wrong avenue here, at least for now.
No, that's the proper way to go.
> Greg,
> I know you are a very busy person...
> Is making a "tty class" even in the cards for 2.6, or is it scheduled
> for 2.7+ ?
It's scheduled for whenever someone gets around to doing it :)
As there is no 2.7 tree, nor is there going to be, why not do it right
now? I'd gladly take patches that do this, and I don't think they would
be all that big at all.
> Germano,
> I still hate doing it, and I know it is against the "1 value per file in
> sys" rule,
> but for now I think exporting the values to the "card" directory with
> each file
> containing the value as a list of ports, 1 per line, might be the best
> option
> to work with here, at least until the "tty class" gets developed.
No, I will not allow that to go into the kernel tree, sorry. Just
export the tty class pointer in the proper structure, and you should be
fine.
thanks,
greg k-h
On Thu, Nov 04, 2004 at 12:29:25PM -0200, Marcelo Tosatti wrote:
> On Thu, Nov 04, 2004 at 08:58:21AM -0800, Roland Dreier wrote:
> > Marcelo> The problem was class_simple only contains the "dev"
> > Marcelo> attribute. You can't add other attributes to it.
> >
> > I believe, based on the comment in class_simple.c:
> >
> > Any further sysfs files that might be required can be created using this pointer.
> >
> > and the implementation in in drivers/scsi/st.c, that there's no
> > problem adding attributes to a device in a simple class. You can just
> > use class_set_devdata() on your class_device to set whatever context
> > you need to get back to your internal structures, and then use
> > class_device_create_file() to add the attributes.
> >
> > I assume this is OK (since there is already one in-kernel driver doing
> > it), but Greg, can you confirm that it's definitely OK for a driver to
> > use class_set_devdata() on a class_device from class_simple_device_add()?
>
> Hi Roland,
>
> Oh thanks, I didnt knew the existance of such possibily.
>
> I once asked here on the list:
>
> ---------
>
> Hope this is not a FAQ.
>
> I want to export some read-only attributes (statistics) from cyclades.c char
> driver to userspace via sysfs.
>
> I can't figure out the right place to do it - I could create a class under
> /sys/class/cyclades for example, but that doesnt sound right since this
> is not a "class" of device, but a device itself.
>
> Hooking the statistics into /sys/class/tty/ttyC$/ sounds reasonable, but
> its not possible it seems because "tty" is a class_simple class, which only implements
> the "dev" attribute.
>
> ------ Greg answer was:
>
> For a driver only attribute, you want them to show up in the place for
> the driver (like under /sys/bus/pci/driver/MY_FOO_DRIVER/). To do that
> use the DRIVER_ATTR() and the driver_add_file() functions. For
> examples, see the other drivers that use these functions.
>
>
> But I hope you are right - /sys/class/tty/tty$/
> sounds the correct place for those files - I thought a "class_tty"
> class was required for new attributes.
No, Roland is right, just use the class_device pointer you get back from
the core when a tty device is created.
Right now we aren't saving the pointer anywhere (see
tty_register_device() for where it is created.) Just add that to the
proper tty_device structure, and you should be fine (yeah, it's going to
be a pain, and you will quickly see why I didn't do that in the first
place, but it is going to need to be changed eventually...)
Good luck,
greg k-h
> > and the implementation in in drivers/scsi/st.c, that there's no
> > problem adding attributes to a device in a simple class. You can
just
> > use class_set_devdata() on your class_device to set whatever context
> > you need to get back to your internal structures, and then use
> > class_device_create_file() to add the attributes.
> >
> > I assume this is OK (since there is already one in-kernel driver
doing
> > it), but Greg, can you confirm that it's definitely OK for a driver
to
> > use class_set_devdata() on a class_device from
class_simple_device_add()?
> Hm, I think that should be ok, but I'd make sure to test it before
> verifying that it really is :)
I have just added this code and tested it, and indeed it *does* work!
So I will graciously redraw my comments from my previous email.
It works, and this is definitely the way Germano and I should go in
each of our respective drivers.
Thanks again for everyones comments/help!
Scott Kilau
Digi International
I send my thanks to all too. Finally (I hope no new problems appear) the
exit of the maze :)
Em Qui, 2004-11-04 ?s 15:50, Kilau, Scott escreveu:
> > > and the implementation in in drivers/scsi/st.c, that there's no
> > > problem adding attributes to a device in a simple class. You can
> just
> > > use class_set_devdata() on your class_device to set whatever context
> > > you need to get back to your internal structures, and then use
> > > class_device_create_file() to add the attributes.
> > >
> > > I assume this is OK (since there is already one in-kernel driver
> doing
> > > it), but Greg, can you confirm that it's definitely OK for a driver
> to
> > > use class_set_devdata() on a class_device from
> class_simple_device_add()?
>
> > Hm, I think that should be ok, but I'd make sure to test it before
> > verifying that it really is :)
>
> I have just added this code and tested it, and indeed it *does* work!
>
> So I will graciously redraw my comments from my previous email.
> It works, and this is definitely the way Germano and I should go in
> each of our respective drivers.
>
> Thanks again for everyones comments/help!
>
> Scott Kilau
> Digi International
>
On Thu, Nov 04, 2004 at 09:40:21AM -0800, Roland Dreier wrote:
> Marcelo> ------ Greg answer was:
>
> Greg> For a driver only attribute, you want them to show up in the
> Greg> place for the driver (like under
> Greg> /sys/bus/pci/driver/MY_FOO_DRIVER/). To do that use the
> Greg> DRIVER_ATTR() and the driver_add_file() functions. For
> Greg> examples, see the other drivers that use these functions.
>
> I think Greg may have misunderstood the question and told you how to
> expose per-driver attributes. But I think the attributes you want
> really are per-device and should be attached to the class_device, not
> the driver.
Heh, yes. Per-driver attributes go in the driver's directory. Per port
attributes go in the port's directory. Simple as that :)
thanks,
greg k-h
Roland> I assume this is OK (since there is already one in-kernel
Roland> driver doing it), but Greg, can you confirm that it's
Roland> definitely OK for a driver to use class_set_devdata() on a
Roland> class_device from class_simple_device_add()?
Greg> Hm, I think that should be ok, but I'd make sure to test it
Greg> before verifying that it really is :)
Well class_simple.c definitely doesn't use class_data/class_set_devdata()
now (and as I said drivers/scsi/st.c is using this on a class_simple
device). The question is whether you can bless this situation as part
of the API, or whether some time in the future class_simple might
start using class_data.
- R.
On Thu, Nov 04, 2004 at 11:05:40AM -0800, Roland Dreier wrote:
> Roland> I assume this is OK (since there is already one in-kernel
> Roland> driver doing it), but Greg, can you confirm that it's
> Roland> definitely OK for a driver to use class_set_devdata() on a
> Roland> class_device from class_simple_device_add()?
>
> Greg> Hm, I think that should be ok, but I'd make sure to test it
> Greg> before verifying that it really is :)
>
> Well class_simple.c definitely doesn't use class_data/class_set_devdata()
> now (and as I said drivers/scsi/st.c is using this on a class_simple
> device). The question is whether you can bless this situation as part
> of the API, or whether some time in the future class_simple might
> start using class_data.
Hey, if it works today, great! If I, or someone else breaks this in the
future, they they will have to deal with the issue then :)
In short, use it.
thanks,
greg k-h