Re: patch for sysfs in the cyclades driver

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

RE: patch for sysfs in the cyclades driver

[Kilau, Scott]

> > 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

RE: patch for sysfs in the cyclades driver

[Germano]

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
> 

RE: patch for sysfs in the cyclades driver

[Kilau, Scott]

> 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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

RE: patch for sysfs in the cyclades driver

[Kilau, Scott]

> 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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

RE: patch for sysfs in the cyclades driver

[Kilau, Scott]

> > 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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

RE: patch for sysfs in the cyclades driver

[Kilau, Scott]

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:greg@kroah.com] 
Sent: Tuesday, November 02, 2004 8:28 PM
To: Kilau, Scott
Cc: germano.barreiro@cyclades.com; linux-kernel@vger.kernel.org
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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

Re: patch for sysfs in the cyclades driver

[Germano]

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

Re: patch for sysfs in the cyclades driver

[Marcelo Tosatti]

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.

Re: patch for sysfs in the cyclades driver

[Roland Dreier]

    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

Re: patch for sysfs in the cyclades driver

[Marcelo Tosatti]

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. 

Re: patch for sysfs in the cyclades driver

[Roland Dreier]

    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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

Re: patch for sysfs in the cyclades driver

[Roland Dreier]

    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.

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

patch for sysfs in the cyclades driver

[Germano Barreiro]

[-- Attachment #1: Type: text/plain, Size: 1865 bytes --]

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 <greg@kroah.com>
Date: Tue, 28 Sep 2004 07:12:43 -0700
To: Marcelo Tosatti <marcelo.tosatti@cyclades.com>
Cc: linux-kernel@vger.kernel.org, torvalds@osdl.org,
        germano.barreiro@cyclades.com
In-Reply-To: <20040928120421.GB11779@logos.cnet>
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 --
=================================================================================================








[-- Attachment #2: sysfs-2.6.10rc1.patch --]
[-- Type: text/x-patch, Size: 21361 bytes --]

--- 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"};
+
+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
+};
+
+
+
 #ifdef CONFIG_ISA
 /* This is the address lookup table. The driver will probe for
    Cyclom-Y/ISA boards at all addresses in here. If you want the
@@ -2621,9 +2650,12 @@ cy_open(struct tty_struct *tty, struct f
     info->throttle = 0;
 
 #ifdef CY_DEBUG_OPEN
-    printk(" cyc:cy_open done\n");/**/
+    printk(" cyc:cy_open done\n");
 #endif
+    /*records line struct pointer for recovering when reading sysfs*/
+    cy_card[info->card].pdev->dev.driver_data=info; 
 
+    createsysfiles(info); //create the sys files associated with this channel
     return 0;
 } /* cy_open */
 
@@ -2837,6 +2869,7 @@ cy_close(struct tty_struct *tty, struct 
 #endif
 
     CY_UNLOCK(info, flags);
+    removesysfiles(info);
     return;
 } /* cy_close */
 
@@ -5154,18 +5187,18 @@ cy_detect_pci(void)
  * This routine prints out the appropriate serial driver version number
  * and identifies which options were configured into this driver.
  */
-static inline void
-show_version(void)
+static inline void show_version(void)
 {
-  char *rcsvers, *rcsdate, *tmp;
-    rcsvers = strchr(rcsid, ' '); rcsvers++;
-    tmp = strchr(rcsvers, ' '); *tmp++ = '\0';
-    rcsdate = strchr(tmp, ' '); rcsdate++;
-    tmp = strrchr(rcsdate, ' '); *tmp = '\0';
-    printk("Cyclades driver %s %s\n",
-        rcsvers, rcsdate);
-    printk("        built %s %s\n",
-	__DATE__, __TIME__);
+	char *rcsvers, *rcsdate, *tmp;
+	rcsvers = strchr(rcsid, ' '); rcsvers++;
+	tmp = strchr(rcsvers, ' '); *tmp++ = '\0';
+	rcsdate = strchr(tmp, ' '); rcsdate++;
+	tmp = strrchr(rcsdate, ' '); *tmp = '\0';
+	printk("Cyclades driver %s %s\n", rcsvers, rcsdate);
+	printk("        built %s %s\n", __DATE__, __TIME__);
+	/* _version is later used by show_sys_data */
+	snprintf(_version, sizeof(_version), "Cyclades driver %s %s\n         built %s %s\n", 
+			rcsvers, rcsdate, __DATE__, __TIME__);
 } /* show_version */
 
 static int 
@@ -5558,4 +5591,456 @@ cy_setup(char *str, int *ints)
 } /* cy_setup */
 #endif /* MODULE */
 
+/*****************************************************************************************************
+ SYSFS SUPPORT FUNCTIONS
+ Germano Barreiro - 2/09/2004
+ ****************************************************************************************************/
+
+static ssize_t show_sys_stat(struct device *p, char *buf) //exports status of the port
+{ 
+ 	return get_sys_data(p, buf, STAT);
+}
+
+static ssize_t show_sys_card(struct device *p, char *buf) //exports card type information
+{
+ 	return get_sys_data(p, buf, CARD);
+}
+
+static ssize_t show_sys_baud(struct device *p, char *buf) //exports baud rate configuration
+{
+ 	return get_sys_data(p, buf, BAUD);
+}
+
+static ssize_t show_sys_flow(struct device *p, char *buf) //exports flow control type
+{
+ 	return get_sys_data(p, buf, FLOW);
+}
+
+static ssize_t show_sys_rxfl(struct device *p, char *buf) //exports rx flow control status
+{
+ 	return get_sys_data(p, buf, RXFL);
+}
+
+static ssize_t show_sys_txfl(struct device *p, char *buf) //exports tx flow control status
+{
+ 	return get_sys_data(p, buf, TXFL);
+}
+
+static ssize_t show_sys_dcd(struct device *p, char *buf) //exports dcd signal
+{
+ 	return get_sys_data(p, buf, DCD);
+}
+
+static ssize_t show_sys_dsr(struct device *p, char *buf) //exports dsr signal
+{
+ 	return get_sys_data(p, buf, DSR);
+}
+
+static ssize_t show_sys_cts(struct device *p, char *buf) //exports cts signal
+{
+ 	return get_sys_data(p, buf, CTS);
+}
+
+static ssize_t show_sys_rts(struct device *p, char *buf) //exports rts signal
+{
+ 	return get_sys_data(p, buf, RTS);
+}
+
+static ssize_t show_sys_dtr(struct device *p, char *buf) //exports dtr signal
+{
+ 	return get_sys_data(p, buf, DTR);
+}
+
+static ssize_t show_sys_rx(struct device *p, char *buf) //exports rx buffer size
+{
+ 	return get_sys_data(p, buf, RX);
+}
+
+static ssize_t show_sys_tx(struct device *p, char *buf) //exports tx buffer size
+{
+ 	return get_sys_data(p, buf, TX);
+}
+
+static ssize_t show_sys_bdrx(struct device *p, char *buf) //exports board rx buffer size (z board only)
+{
+ 	return get_sys_data(p, buf, BDRX);
+}
+
+static ssize_t show_sys_bdtx(struct device *p, char *buf) //exports board tx buffer size (z board only)
+{
+ 	return get_sys_data(p, buf, BDTX);
+}
+
+static ssize_t show_sys_par(struct device *p, char *buf) //exports parity configuration
+{
+ 	return get_sys_data(p, buf, PAR);
+}
+
+static ssize_t show_sys_stop(struct device *p, char *buf) //exports stop bits configuration
+{
+ 	return get_sys_data(p, buf, STOP);
+}
+
+static ssize_t show_sys_chlen(struct device *p, char *buf) //exports word size configuration
+{
+ 	return get_sys_data(p, buf, CHLEN);
+}
+
+
+/* this function creates the sys files that will export each signal status to sysfs 
+   each value will be put in a separate filename */
+static void createsysfiles(struct cyclades_port *port)
+{
+	enum tserinfo index;
+	for(index=STAT; index<=CHLEN; index++){
+		sprintf(port->sysdata.sysnames[index],"cycser%d_%s",port->line,sysufixes[index]); 
+		port->sysdata.cydas[index].attr.name = port->sysdata.sysnames[index];
+		port->sysdata.cydas[index].attr.mode = S_IRUGO;
+		port->sysdata.cydas[index].attr.owner = THIS_MODULE;
+		port->sysdata.cydas[index].show  =  showfunctions[index];
+		port->sysdata.cydas[index].store = NULL;
+		device_create_file(&(cy_card[port->card].pdev->dev), &port->sysdata.cydas[index]);            
+	}
+}
+ 
+/* removes all the sys files created for that port */
+static void removesysfiles(struct cyclades_port *port){
+	enum tserinfo index;
+	for(index=STAT; index <= CHLEN; index++)
+		device_remove_file(&cy_card[port->card].pdev->dev, &port->sysdata.cydas[index]);
+}
+
+/*******************************************************************************************************
+  get_sys_data - get the data to be exported to sysfs/device, mostly the signals status involved in the
+  serial communication such as CTS,RTS,DTS,etc as well as some config info.
+  This funcion is called by each specific function that exports to the sys virtual filesystem
+  values about signals and states related to each serial port of the card.
+  The code internal to this function, with few changes, was extracted from the original procfs 
+  implementation in the cyclades async serial driver, made by Ivan. 
+
+  p - points to the struct device associated with this channel. Besides some other potentially useful
+      information for future debugging tasks, cy_open() saves there a pointer to the cyclades_port
+      associated with this channel, which in recovered in this function.
+  buf - points to the buffer where the data to be available in the file mentioned above must be written
+  whatinfo - the information asked. To see all possible values, look at cyclades.h where the definition
+             of this enum is.
+
+  Returns the number of bytes written to buf 
+********************************************************************************************************/
+static ssize_t get_sys_data(struct device *p, char *buf, enum tserinfo whatinfo)
+{  
+	int len=0, z=0;
+	int	flow=0; // 0:none, 1:soft ctr, 2: hard ctr
+	int chip;
+	//cy_open recorded the cyclades_port pointer here
+	struct cyclades_port *info = p->driver_data;
+	//ind will be an index to the cyclades_port struct associated to this device in the array cy_port[]
+	int ind = info - cy_port; 
+
+	struct tty_struct * tty = info->tty;
+	unsigned char *base_addr = (unsigned char *) cy_card[info->card].base_addr;
+	struct FIRM_ID *firm_id = (struct FIRM_ID *) (base_addr + ID_ADDRESS);
+	struct ZFW_CTRL *zfw_ctrl = (struct ZFW_CTRL *) \
+					(base_addr + (cy_readl(&firm_id->zfwctrl_addr) & 0xfffff));
+
+	struct CH_CTRL *ch_ctrl = zfw_ctrl->ch_ctrl;
+	struct BUF_CTRL *buf_ctrl = &(zfw_ctrl->buf_ctrl[ind - cy_card[info->card].first_line]);
+	int card = info->card;
+	int channel = (info->line) - (cy_card[card].first_line);
+	int index = cy_card[card].bus_index;
+	uclong control;
+	char par;
+	int nstop; //number of stop bits
+	int char_len; //size of the word
+
+	//verifies if it is a z board
+	if (IS_CYC_Z(cy_card[card])) z = 1;
+
+	//checks if it is being used soft or hardware flow control
+	if (I_IXOFF(tty))  flow = 1;	
+	else if ( C_CRTSCTS(tty) ) flow = 2;	
+	else flow = 0;	
+   
+	//necessary for printing status of the signals: DCD, DSR, CTS, RTS, DTR
+	if (z == 1) {// Cyclades-Z board
+		control = cy_readl(&ch_ctrl[channel].rs_status);
+	} else {
+		control = cy_readb((u_long) base_addr + (CyMSVR1 << index));
+	}
+	
+        switch(whatinfo){
+
+		case STAT: //open/closed
+			if ( tty == 0 ) 
+				len = sprintf(buf, "%s:%s\n", cysys_state, cysys_close);
+			else
+				len = sprintf(buf, "%s:%s\n", cysys_state, cysys_open);
+		break;
+
+		case CARD: //card type
+			if (z) //it's a z-card 
+				len += sprintf(buf, "%s:%s\n", cysys_ct, cysys_z);
+			else 
+				len += sprintf(buf, "%s:%s\n", cysys_ct, cysys_y);
+		break;
+
+		case BAUD: //baud rate
+			len += sprintf(buf, "%s:%d\n", cysys_baud, tty_get_baud_rate(tty) );
+		break;
+
+		case FLOW: //flow control type
+			if (flow==1)  
+				len += sprintf(buf, "%s:%s\n", cysys_fc, cysys_fc_soft);
+			else if (flow==2)
+				len += sprintf(buf, "%s:%s\n", cysys_fc, cysys_fc_hard);
+			else 
+				len += sprintf(buf, "%s:%s\n", cysys_fc, cysys_fc_none);
+		break;
+
+		case RXFL: //rx hardware flow control status
+
+			if (z == 1) {// Cyclades-Z board
+				if (control & C_RS_RTS) {
+					len += sprintf(buf, "%s:%s\n", cysys_rxfcs, cysys_on);
+				} else {
+					len += sprintf(buf, "%s:%s\n", cysys_rxfcs, cysys_off);
+				}	
+			
+			} else {// Cyclades-Y board
+				len += get_rxfl_y(buf,info);
+			}
+		break;
+
+ 		case TXFL: //hardware tx flow control
+
+			if (z == 1){// Cyclades-Z board
+				uclong control = cy_readl(&ch_ctrl[channel].rs_status);
+			
+				if (control & C_RS_CTS) {
+					len += sprintf(buf, "%s:%s\n", cysys_txfcs, cysys_on);
+				} else {
+					len += sprintf(buf, "%s:%s\n", cysys_txfcs, cysys_off);
+				}
+			} else {// Cyclades-Y board
+				uclong aux;
+				chip = channel >> 2;
+				channel &= 0x03;
+				base_addr = (unsigned char *)(cy_card[card].base_addr
+							 + (cy_chip_offset[chip] << index));
+				uclong control = cy_readb((u_long) base_addr + (CyMSVR1 << index));
+				if (info->rtsdtr_inv) {
+					aux = CyDTR;
+				} else {
+					aux = CyRTS;
+				}
+				if (control & aux) {
+					len += sprintf(buf, "%s:%s\n", cysys_rxfcs, cysys_on);
+				} else {
+					len += sprintf(buf, "%s:%s\n", cysys_rxfcs, cysys_off);
+				}	
+			}
+		break;
+
+
+        	case DCD: //DCD status
+			len += sprintf(buf, "%s:%s\n", cysys_dcd,
+		                 (control & (z ? C_RS_DCD:CyDCD) ) ? cysys_on : cysys_off);
+		break;				   
+		
+		case DSR: //DSR status
+	        	len += sprintf(buf, "%s:%s\n", cysys_dsr,
+		                 (control & (z ? C_RS_DSR:CyDSR) ) ? cysys_on : cysys_off);
+	        break;
+
+		case CTS: //CTS status
+	        	len += sprintf(buf, "%s:%s\n", cysys_cts,
+		                 (control & (z ? C_RS_CTS:CyCTS) ) ? cysys_on : cysys_off);
+		break;
+	
+		case RTS: //RTS status
+	        	len += sprintf(buf, "%s:%s\n", cysys_rts,
+		                 (control & (z ? C_RS_RTS:CyRTS) ) ? cysys_on : cysys_off);
+		break;
+	
+		case DTR: //DTR status
+			len += sprintf(buf, "%s:%s\n", cysys_dtr,
+		      		(control & (z ? C_RS_DTR:(info->rtsdtr_inv ? CyRTS : CyDTR)) ) \
+				? cysys_on : cysys_off); 
+		break;
+
+		case RX: //rx buffer
+	        	len += sprintf(buf,"%s:%d/%d\n", cysys_ttyrxbuf,tty->read_cnt,N_TTY_BUF_SIZE);
+		break;
+	
+		case TX: //tx buffer
+	        	len += sprintf(buf, "%s:%d/%ld\n", cysys_ttytxbuf,info->xmit_cnt, SERIAL_XMIT_SIZE);
+		break;
+
+		case BDRX: //board buffer, apply only to z-boards
+	
+			if ( z == 1) {
+				volatile uclong put, get, bufsize;
+				volatile int char_count;
+				//rx
+				get = cy_readl(&buf_ctrl->rx_get);
+				put = cy_readl(&buf_ctrl->rx_put);
+				bufsize = cy_readl(&buf_ctrl->rx_bufsize);
+				if (put >= get) {
+					char_count = put - get;
+				} else {
+					char_count = put - get + bufsize;
+				}
+				len += sprintf(buf, "%s:%d/%ld\n", cysys_boardrxbuf,
+							char_count, bufsize);
+			}
+			else len += sprintf(buf, "board rx buffer is specific for Z-boards\n");
+        	break;	
+	
+		case BDTX: //board buffer, apply only to z-boards
+	
+			if ( z == 1) {
+				volatile uclong put, get, bufsize;
+				volatile int char_count;
+
+				//tx
+				get = cy_readl(&buf_ctrl->tx_get);
+				put = cy_readl(&buf_ctrl->tx_put);
+				bufsize = cy_readl(&buf_ctrl->tx_bufsize);
+				if (put >= get) {
+					char_count = put - get;
+				} else {
+					char_count = put - get + bufsize;
+				}
+				len += sprintf(buf, "%s:%d/%ld\n", cysys_boardtxbuf,
+								char_count, bufsize);
+			}
+			else len += sprintf(buf, "board tx buffer is specific for Z-boards\n");
+        	break;	
+	
+		case PAR://parity configuration info
+			if(z == 1){
+				uclong pari;
+				pari = cy_readl(&ch_ctrl[channel].comm_parity);
+				switch (pari) {
+					case C_PR_NONE:
+						par = 'n';
+					break;
+					case C_PR_ODD:
+						par = 'o';
+					break;
+					case C_PR_EVEN:
+						par = 'e';
+					break;
+					default:
+						par = 'n';
+					break;
+				}
+		       	len += sprintf(buf, "%s:%c\n", cysys_parity, par); 
+			}
+			else{
+				switch ( info->cor1 & 0xc0) {
+					case CyPARITY_NONE:
+						par = 'n';
+					break;
+					case CyPARITY_O:
+						par = 'o';
+					break;
+					case CyPARITY_E:
+						par = 'e';
+					break;
+					default:
+						par = 'n';
+					break;
+				}
+				len += sprintf(buf, "%s:%c\n", cysys_parity, par);
+			}
+
+		break;
+
+		case STOP: //number of stop bits info
+			if(z == 1){
+				uclong data_len;
+				data_len = cy_readl(&ch_ctrl[channel].comm_data_l);
+				if ( data_len & C_DL_2STOP) {
+					nstop = 2;
+		   		} else {
+					nstop = 1;
+		   		}
+			}
+			else{
+				if ( info->cor1 & Cy_2_STOP) {
+					nstop = 2;
+				} else {
+					nstop = 1;
+				}
+
+			}
+			len += sprintf(buf, "%s:%d\n",cysys_stopbits, nstop);
+		break;
+	
+		case CHLEN:
+
+		//Printing communication data len
+		if (z == 1){
+			uclong data_len;
+			data_len = cy_readl(&ch_ctrl[channel].comm_data_l);
+
+			switch (data_len & 0x0f) {
+				case 1:
+					char_len = 5;
+				break;
+				case 2:
+					char_len = 6;
+				break;
+				case 4:
+					char_len = 7;
+				break;
+				case 8:
+					char_len = 8;
+				break;
+			default:
+				char_len = 8;
+				break;
+		}
+				
+		} else {
+			char_len = (info->cor1 & 0x03) + 5;
+		}	
+		len += sprintf(buf, "%s:%d\n", cysys_charlen, char_len); 
+        	break;
+	
+	}/*switch(whatinfo)*/	
+
+	return len;
+}
+
+/* get the hardware rx flow control status for y board*/
+inline ssize_t get_rxfl_y(char *buf, struct cyclades_port *info)
+{
+	ssize_t len=0;
+	int card = info->card;
+	int index = cy_card[card].bus_index;
+	int channel = (info->line) - (cy_card[card].first_line);
+	int chip = channel >> 2;
+	unsigned char *base_addr = (unsigned char *)(cy_card[card].base_addr
+					+ (cy_chip_offset[chip] << index));
+
+	uclong control = cy_readb((u_long) base_addr + (CyMSVR1 << index));
+	uclong aux;
+
+	if (info->rtsdtr_inv) {
+		aux = CyDTR;
+	} else {
+		aux = CyRTS;
+	}
+
+	if (control & aux) {
+		len += sprintf(buf, "%s:%s\n", cysys_rxfcs, cysys_on);
+	} else {
+		len += sprintf(buf, "%s:%s\n", cysys_rxfcs, cysys_off);
+	}
+	return len;
+}
+
+
 MODULE_LICENSE("GPL");
--- linux-2.6.10rc1.orig/include/linux/cyclades.h	2004-10-18 19:55:35.000000000 -0200
+++ linux-2.6.10rc1/include/linux/cyclades.h	2004-10-26 13:04:04.000000000 -0200
@@ -401,6 +401,45 @@ struct	FIRM_ID {
 #define	C_CM_FATAL	0x91		/* fatal error */
 #define	C_CM_HW_RESET	0x92		/* reset board */
 
+
+//strings
+char cysys_state[] = 		"State                 ";
+char cysys_open[] = "Open";
+char cysys_close[] = "Close";
+
+char cysys_ct[] = 			"Card Type             ";
+char cysys_z[] = "Cyclades Z";
+char cysys_y[] = "Cyclades Y";
+
+char cysys_baud[] = 		"Baud Rate             ";
+
+char cysys_fc[] = 			"Flow Control          ";
+char cysys_fc_soft[] = "software";
+char cysys_fc_hard[] = "hardware";
+char cysys_fc_none[] = "none";
+
+char cysys_rxfcs[] = 		"RX Flow Control Status";
+char cysys_txfcs[] = 		"TX Flow Control Status";
+
+char cysys_on[] = "on";
+char cysys_off[] = "off";
+
+char cysys_dcd[] = 		"DCD                   ";
+char cysys_dsr[] = 		"DSR                   ";
+char cysys_cts[] = 		"CTS                   ";
+char cysys_rts[] = 		"RTS                   ";
+char cysys_dtr[] = 		"DTR                   ";
+
+char cysys_ttyrxbuf[] = 	"TTY Rx Buffer         ";
+char cysys_ttytxbuf[] = 	"TTY Tx Buffer         ";
+
+char cysys_boardrxbuf[] = 	"On Board Rx Buffer    ";
+char cysys_boardtxbuf[] = 	"On Board Tx Buffer    ";
+
+char cysys_charlen[] = 	"Character Len         ";
+char cysys_parity[]  =  "Parity:               ";
+char cysys_stopbits[] = "Stop bits:            ";
+
 /*
  *	CH_CTRL - This per port structure contains all parameters
  *	that control an specific port. It can be seen as the
@@ -556,6 +595,11 @@ struct cyclades_icount {
 	__u32	buf_overrun;
 };
 
+struct cycsysinfo{
+  char sysnames[18][20]; /* this buffer will keep the sys filenames for each channel*/
+  struct device_attribute cydas[18];   
+};
+
 /*
  * This is our internal structure for each serial port's state.
  * 
@@ -609,6 +653,8 @@ struct cyclades_port {
 	wait_queue_head_t       shutdown_wait;
 	wait_queue_head_t       delta_msr_wait;
 	int throttle;
+	struct cycsysinfo sysdata;
+        /*struct device_attribute cydas[18];*/
 };
 
 /*
@@ -823,5 +869,39 @@ struct cyclades_port {
 
 /***************************************************************************/
 
+/* one of the values of this enum is passed to get_sys_data in order to determine 
+   the information asked. DCD to DTR are the corresponding serial line signals.
+   RX and TX are the buffers
+   CHLEN is the size of the data
+   PAR is the parity configuration for the port
+   BBUF onboard buffer, apply only to z boards
+*/
+
+enum tserinfo {STAT,CARD,BAUD,FLOW,RXFL,TXFL,DCD,DSR,CTS,RTS,DTR,RX,TX,BDRX,BDTX,PAR,STOP,CHLEN};
+
+/*used for sysfs register purposes */
+static ssize_t show_sys_stat(struct device *p, char *buf); //exports status of the port
+static ssize_t show_sys_card(struct device *p, char *buf); //exports card type information
+static ssize_t show_sys_baud(struct device *p, char *buf); //exports baud rate configuration
+static ssize_t show_sys_flow(struct device *p, char *buf); //exports flow control type
+static ssize_t show_sys_rxfl(struct device *p, char *buf); //exports rx flow control status
+static ssize_t show_sys_txfl(struct device *p, char *buf); //exports tx flow control status
+static ssize_t show_sys_dcd(struct device *p, char *buf); //exports dcd signal
+static ssize_t show_sys_dsr(struct device *p, char *buf); //exports dsr signal
+static ssize_t show_sys_cts(struct device *p, char *buf); //exports cts signal
+static ssize_t show_sys_rts(struct device *p, char *buf); //exports rts signal
+static ssize_t show_sys_dtr(struct device *p, char *buf); //exports dtr signal
+static ssize_t show_sys_rx(struct device *p, char *buf); //exports rx buffer size
+static ssize_t show_sys_tx(struct device *p, char *buf); //exports tx buffer size
+static ssize_t show_sys_bdrx(struct device *p, char *buf); //exports board rx buffer size (z board only)
+static ssize_t show_sys_bdtx(struct device *p, char *buf); //exports board tx buffer size (z board only)
+static ssize_t show_sys_par(struct device *p, char *buf); //exports parity configuration
+static ssize_t show_sys_stop(struct device *p, char *buf); //exports stop bits configuration
+static ssize_t show_sys_chlen(struct device *p, char *buf); //exports word size configuration
+static ssize_t get_sys_data(struct device *p, char *buf, enum tserinfo whatinfo);
+inline ssize_t get_rxfl_y(char *buf, struct cyclades_port *info);//get rx hard flow for y board 
+static void createsysfiles(struct cyclades_port *port); //create sys files for a channel
+static void removesysfiles(struct cyclades_port *port); //remove sys files for a channel
+
 #endif /* __KERNEL__ */
 #endif /* _LINUX_CYCLADES_H */

Re: patch for sysfs in the cyclades driver

[Greg KH]

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

Re: patch for sysfs in the cyclades driver

[germano.barreiro]

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 <greg@kroah.com>:

> 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
>