Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Wed, Mar 09, 2005 at 12:18:21PM -0500, Wen Xiong wrote:
> Greg KH wrote:
>
> >On Wed, Mar 09, 2005 at 10:47:22AM -0500, Wen Xiong wrote:
> >
> >
> >>+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char
> >>*buf)
> >>+{
> >>+ return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_debug);
> >>+}
> >>+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
> >>
> >>
> >
> >Should just be a module paramater, right? So you can drop this too...
> >
> >This file is getting quite small now :)
> >
> If I removed two module paramaters, only two files left: version and state.
> Removed all of them?
Move them to a different file?
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Wed, Mar 09, 2005 at 10:47:22AM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
Should just be a module paramater, right? So you can drop this too...
This file is getting quite small now :)
thanks,
greg k-h
If I removed two module paramaters, only two files left: version and state.
Removed all of them?
Thanks,
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-09
11:17:37.055947624 -0600
@@ -0,0 +1,53 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau <[EMAIL PROTECTED]>
+ * Wendy Xiong <[EMAIL PROTECTED]>
+ *
+ ***/
+#include
+#include
+
+#include "jsm_driver.h"
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%s\n", JSM_VERSION);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%s\n",
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, _attr_version);
+ driver_create_file(driverfs, _attr_state);
+}
+
+void jsm_remove_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_remove_file(driverfs, _attr_version);
+ driver_remove_file(driverfs, _attr_state);
+}
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Wed, Mar 09, 2005 at 10:47:22AM -0500, Wen Xiong wrote:
> +static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_debug);
> +}
> +static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
Should just be a module paramater, right? So you can drop this too...
This file is getting quite small now :)
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Mon, Mar 07, 2005 at 10:44:25PM -0800, Greg KH wrote:
> On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
> > +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
> > +{
> > + struct jsm_channel *ch;
> > +
> > + if (class_dev) {
> > + ch = class_get_devdata(class_dev);
> > + if ( ch && (ch->ch_bd->state == BOARD_READY))
> > + return snprintf(buf, PAGE_SIZE, "%d\n", ch->ch_old_baud);
> > + }
> > +
> > + return -EINVAL;
> > +}
> > +static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
>
> No, please delete these, and the other sysfs files that duplicate the
> same info that you can get by using the standard Linux termios calls.
> There is no need for them here.
Greg, there's several other points about why the above is Bad(tm).
"class_dev" will always be non-null.
Note that this (and similar code) is racy. Consider what happens when
the class device is removed (and the class devdata is NULL'd or kfree'd)
while another process on another CPU reads from one of these sysfs files.
*BANG*.
Also, if a class device attribute method is going to access data outside
the same allocation which the class device is a part of, you *absolutely*
*must* have some form of locking.
Also note that the formatting of these snippets of code is abismal.
There is a missing tab which makes it very non-readable.
With all of the above comments, it should be something like:
+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+ int ret = -EINVAL;
+
+ down(_lock);
+ ch = class_get_devdata(class_dev);
+ if (ch && (ch->ch_bd->state == BOARD_READY))
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", ch->ch_old_baud);
+ up(_lock);
+
+ return ret;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
where "some_lock" is also taken in the device unregistration path, at
the point where the class devdata is NULL'd out. (which the driver is
also missing.)
--
Russell King
Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 03:47:45PM -0600, Kilau, Scott wrote: > For example, lets say a customer has a modem connected to a serial port. > > If you were to open up the port with an "stty -a" to get the current > settings and signals, you would unintentionally raise RTS and DTR. That isn't special to this driver though. Maybe it should be fixed for all serial drivers, since the situation you mention above is not limited to just this driver. As you say, you may have a modem connected, which may have been configured to automatically dial a predetermined number when DTR is raised. Maybe we need a solution which applies to all drivers? > This is why we export the various signals/stats/signals to sysfs (used > to be proc), so our management tools can get the information about the > serial port without being intrusive by opening up the port. Note that exporting statistics can be a security bug, especially if that includes the number of bytes sent/received. For an explaination of this, please lookup the reason why the /proc/tty/driver directory was made unreadable to userspace. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: 2.6 Serial core - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Tue, Mar 08, 2005 at 01:55:33PM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = jsm_total_boardnum();
+ return snprintf(buf, PAGE_SIZE, "%d\n", adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
Why is this file even needed? You can just look at the number of sysfs
directories attached to this device, right?
thanks,
greg k-h
We can find out the number of adapters in several ways in linux. But we
are not sure the end customer really know how to find out.
Anyway, attatched the new patch4.jasmine for you.
Thanks,
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-09
09:37:00.520967448 -0600
@@ -0,0 +1,69 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau <[EMAIL PROTECTED]>
+ * Wendy Xiong <[EMAIL PROTECTED]>
+ *
+ ***/
+#include
+#include
+
+#include "jsm_driver.h"
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%s\n", JSM_VERSION);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%s\n",
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
+
+static ssize_t jsm_driver_rawreadok_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_rawreadok);
+}
+static DRIVER_ATTR(rawreadok, S_IRUSR, jsm_driver_rawreadok_show, NULL);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, _attr_version);
+ driver_create_file(driverfs, _attr_debug);
+ driver_create_file(driverfs, _attr_rawreadok);
+ driver_create_file(driverfs, _attr_state);
+}
+
+void jsm_remove_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_remove_file(driverfs, _attr_version);
+ driver_remove_file(driverfs, _attr_debug);
+ driver_remove_file(driverfs, _attr_rawreadok);
+ driver_remove_file(driverfs, _attr_state);
+}
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Tue, Mar 08, 2005 at 01:55:33PM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = jsm_total_boardnum();
+ return snprintf(buf, PAGE_SIZE, %d\n, adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
Why is this file even needed? You can just look at the number of sysfs
directories attached to this device, right?
thanks,
greg k-h
We can find out the number of adapters in several ways in linux. But we
are not sure the end customer really know how to find out.
Anyway, attatched the new patch4.jasmine for you.
Thanks,
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-09
09:37:00.520967448 -0600
@@ -0,0 +1,69 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau [EMAIL PROTECTED]
+ * Wendy Xiong [EMAIL PROTECTED]
+ *
+ ***/
+#include linux/device.h
+#include linux/serial_reg.h
+
+#include jsm_driver.h
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, %s\n, JSM_VERSION);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, %s\n,
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
+
+static ssize_t jsm_driver_rawreadok_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_rawreadok);
+}
+static DRIVER_ATTR(rawreadok, S_IRUSR, jsm_driver_rawreadok_show, NULL);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, driver_attr_version);
+ driver_create_file(driverfs, driver_attr_debug);
+ driver_create_file(driverfs, driver_attr_rawreadok);
+ driver_create_file(driverfs, driver_attr_state);
+}
+
+void jsm_remove_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_remove_file(driverfs, driver_attr_version);
+ driver_remove_file(driverfs, driver_attr_debug);
+ driver_remove_file(driverfs, driver_attr_rawreadok);
+ driver_remove_file(driverfs, driver_attr_state);
+}
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Mon, Mar 07, 2005 at 10:44:25PM -0800, Greg KH wrote:
On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch (ch-ch_bd-state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, %d\n, ch-ch_old_baud);
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
No, please delete these, and the other sysfs files that duplicate the
same info that you can get by using the standard Linux termios calls.
There is no need for them here.
Greg, there's several other points about why the above is Bad(tm).
class_dev will always be non-null.
Note that this (and similar code) is racy. Consider what happens when
the class device is removed (and the class devdata is NULL'd or kfree'd)
while another process on another CPU reads from one of these sysfs files.
*BANG*.
Also, if a class device attribute method is going to access data outside
the same allocation which the class device is a part of, you *absolutely*
*must* have some form of locking.
Also note that the formatting of these snippets of code is abismal.
There is a missing tab which makes it very non-readable.
With all of the above comments, it should be something like:
+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+ int ret = -EINVAL;
+
+ down(some_lock);
+ ch = class_get_devdata(class_dev);
+ if (ch (ch-ch_bd-state == BOARD_READY))
+ ret = snprintf(buf, PAGE_SIZE, %d\n, ch-ch_old_baud);
+ up(some_lock);
+
+ return ret;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
where some_lock is also taken in the device unregistration path, at
the point where the class devdata is NULL'd out. (which the driver is
also missing.)
--
Russell King
Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 03:47:45PM -0600, Kilau, Scott wrote: For example, lets say a customer has a modem connected to a serial port. If you were to open up the port with an stty -a to get the current settings and signals, you would unintentionally raise RTS and DTR. That isn't special to this driver though. Maybe it should be fixed for all serial drivers, since the situation you mention above is not limited to just this driver. As you say, you may have a modem connected, which may have been configured to automatically dial a predetermined number when DTR is raised. Maybe we need a solution which applies to all drivers? This is why we export the various signals/stats/signals to sysfs (used to be proc), so our management tools can get the information about the serial port without being intrusive by opening up the port. Note that exporting statistics can be a security bug, especially if that includes the number of bytes sent/received. For an explaination of this, please lookup the reason why the /proc/tty/driver directory was made unreadable to userspace. -- Russell King Linux kernel2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of: 2.6 Serial core - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Wed, Mar 09, 2005 at 10:47:22AM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
Should just be a module paramater, right? So you can drop this too...
This file is getting quite small now :)
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Wed, Mar 09, 2005 at 10:47:22AM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
Should just be a module paramater, right? So you can drop this too...
This file is getting quite small now :)
thanks,
greg k-h
If I removed two module paramaters, only two files left: version and state.
Removed all of them?
Thanks,
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-09
11:17:37.055947624 -0600
@@ -0,0 +1,53 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau [EMAIL PROTECTED]
+ * Wendy Xiong [EMAIL PROTECTED]
+ *
+ ***/
+#include linux/device.h
+#include linux/serial_reg.h
+
+#include jsm_driver.h
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, %s\n, JSM_VERSION);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, %s\n,
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, driver_attr_version);
+ driver_create_file(driverfs, driver_attr_state);
+}
+
+void jsm_remove_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_remove_file(driverfs, driver_attr_version);
+ driver_remove_file(driverfs, driver_attr_state);
+}
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Wed, Mar 09, 2005 at 12:18:21PM -0500, Wen Xiong wrote:
Greg KH wrote:
On Wed, Mar 09, 2005 at 10:47:22AM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char
*buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
Should just be a module paramater, right? So you can drop this too...
This file is getting quite small now :)
If I removed two module paramaters, only two files left: version and state.
Removed all of them?
Move them to a different file?
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 08:36:57PM -0600, Kilau, Scott wrote: > > > > > > If you were to open up the port with an "stty -a" to get the current > > > > settings and signals, you would unintentionally raise RTS and DTR. > > > > Why not fix the driver to not change the current line settings if it > is > > not being opened for the first time? That seems like a much simpler > way > > to solve this, and probably the saner way, as you don't want any user > to > > be able to mess up your modem... > > > > Oh, when the port is already open, the driver correctly would not muck > with DTR/RTS. > > I am talking about when the port is currently not open. > > On first port open, DTR (and usually RTS) will always be raised. > The serial device would see this DTR raise, and under some > circumstances, react to it... Ok, well, that sounds like something that all serial devices should support, right? And if so, why not add this to the serial core for everyone to benifit? thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [ patch 4/7] drivers/serial/jsm: new serial device driver
> > > > If you were to open up the port with an "stty -a" to get the current > > settings and signals, you would unintentionally raise RTS and DTR. > > Why not fix the driver to not change the current line settings if it is > not being opened for the first time? That seems like a much simpler way > to solve this, and probably the saner way, as you don't want any user to > be able to mess up your modem... > > thanks, > > greg k-h Oh, when the port is already open, the driver correctly would not muck with DTR/RTS. I am talking about when the port is currently not open. On first port open, DTR (and usually RTS) will always be raised. The serial device would see this DTR raise, and under some circumstances, react to it... We have customers that use *really* *really* old serial devices on our products, (we are talking 110 baud and even 50 baud (!!!)), where an unintentional raise of DTR/RTS will freak the device out. At any rate, that's the reason I exported the values to sysfs in the original "dgnc" (outside-the-kernel-sources) driver. Thanks, Scott - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 03:47:45PM -0600, Kilau, Scott wrote: > > Who needs to know if a port is open or not? > > > > > > > +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf) > > > No, please delete these, and the other sysfs files that duplicate the > > same info that you can get by using the standard Linux termios calls. > > There is no need for them here. > > Our serial port monitoring tools need to know these things, and to > find them out *without* opening up the serial port to do an ioctl(). > > For example, lets say a customer has a modem connected to a serial port. > > If you were to open up the port with an "stty -a" to get the current > settings and signals, you would unintentionally raise RTS and DTR. Why not fix the driver to not change the current line settings if it is not being opened for the first time? That seems like a much simpler way to solve this, and probably the saner way, as you don't want any user to be able to mess up your modem... thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 01:55:33PM -0500, Wen Xiong wrote:
> +static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
> +{
> + int adapter_count = 0;
> + adapter_count = jsm_total_boardnum();
> + return snprintf(buf, PAGE_SIZE, "%d\n", adapter_count);
> +}
> +static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
Why is this file even needed? You can just look at the number of sysfs
directories attached to this device, right?
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
> Who needs to know if a port is open or not? > > > +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf) > No, please delete these, and the other sysfs files that duplicate the > same info that you can get by using the standard Linux termios calls. > There is no need for them here. > > thanks, > greg k-h Hi Greg, Wendy, all, Our serial port monitoring tools need to know these things, and to find them out *without* opening up the serial port to do an ioctl(). For example, lets say a customer has a modem connected to a serial port. If you were to open up the port with an "stty -a" to get the current settings and signals, you would unintentionally raise RTS and DTR. Now the modem sees DTR raised, and might react to it by mistake. Usually raising and dropping RTS/DTR quickly on a modem won't hurt anything, but its not particularly a "good" when the modem is not expecting it. This is why we export the various signals/stats/signals to sysfs (used to be proc), so our management tools can get the information about the serial port without being intrusive by opening up the port. Scott - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "jsm_version: %s\n", JSM_VERSION);
Again, drop the "prefix:" from every sysfs file, it should not be there
(the data type is inferred by the name of the file, you do not have to
repeat it again.)
+static ssize_t jsm_tty_state_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch && (ch->ch_bd->state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, "%s\n", ch->ch_open_count ? "Open" : "Closed");
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(state, S_IRUGO, jsm_tty_state_show, NULL);
Who needs to know if a port is open or not?
+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch && (ch->ch_bd->state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, "%d\n", ch->ch_old_baud);
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
No, please delete these, and the other sysfs files that duplicate the
same info that you can get by using the standard Linux termios calls.
There is no need for them here.
thanks,
greg k-h
Hi Greg,
Removed some codes in jsm_sysfs.c.
Thanks for your reviewing the code.
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-08
12:19:57.498967368 -0600
@@ -0,0 +1,98 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau <[EMAIL PROTECTED]>
+ * Wendy Xiong <[EMAIL PROTECTED]>
+ *
+ ***/
+#include
+#include
+
+#include "jsm_driver.h"
+
+int jsm_total_boardnum(void)
+{
+ struct list_head *tmp;
+ struct jsm_board *cur_board_entry;
+ int adapter_count = 0;
+ u64 lock_flags;
+
+ spin_lock_irqsave(_board_head_lock, lock_flags);
+ list_for_each(tmp, _board_head) {
+ cur_board_entry =
+ list_entry(tmp, struct jsm_board,
+ jsm_board_entry);
+ adapter_count++;
+ }
+ spin_unlock_irqrestore(_board_head_lock, lock_flags);
+
+ return adapter_count;
+}
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%s\n", JSM_VERSION);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = jsm_total_boardnum();
+ return snprintf(buf, PAGE_SIZE, "%d\n", adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%s\n",
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
+
+static ssize_t jsm_driver_rawreadok_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", jsm_rawreadok);
+}
+static DRIVER_ATTR(rawreadok, S_IRUSR, jsm_driver_rawreadok_show, NULL);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, _attr_version);
+ driver_create_file(driverfs, _attr_boards);
+ driver_create_file(driverfs, _attr_debug);
+ driver_create_file(driverfs,
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, jsm_version: %s\n, JSM_VERSION);
Again, drop the prefix: from every sysfs file, it should not be there
(the data type is inferred by the name of the file, you do not have to
repeat it again.)
+static ssize_t jsm_tty_state_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch (ch-ch_bd-state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, %s\n, ch-ch_open_count ? Open : Closed);
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(state, S_IRUGO, jsm_tty_state_show, NULL);
Who needs to know if a port is open or not?
+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch (ch-ch_bd-state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, %d\n, ch-ch_old_baud);
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
No, please delete these, and the other sysfs files that duplicate the
same info that you can get by using the standard Linux termios calls.
There is no need for them here.
thanks,
greg k-h
Hi Greg,
Removed some codes in jsm_sysfs.c.
Thanks for your reviewing the code.
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-08
12:19:57.498967368 -0600
@@ -0,0 +1,98 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau [EMAIL PROTECTED]
+ * Wendy Xiong [EMAIL PROTECTED]
+ *
+ ***/
+#include linux/device.h
+#include linux/serial_reg.h
+
+#include jsm_driver.h
+
+int jsm_total_boardnum(void)
+{
+ struct list_head *tmp;
+ struct jsm_board *cur_board_entry;
+ int adapter_count = 0;
+ u64 lock_flags;
+
+ spin_lock_irqsave(jsm_board_head_lock, lock_flags);
+ list_for_each(tmp, jsm_board_head) {
+ cur_board_entry =
+ list_entry(tmp, struct jsm_board,
+ jsm_board_entry);
+ adapter_count++;
+ }
+ spin_unlock_irqrestore(jsm_board_head_lock, lock_flags);
+
+ return adapter_count;
+}
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, %s\n, JSM_VERSION);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = jsm_total_boardnum();
+ return snprintf(buf, PAGE_SIZE, %d\n, adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, %s\n,
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_debug);
+}
+static DRIVER_ATTR(debug, S_IRUSR, jsm_driver_debug_show, NULL);
+
+static ssize_t jsm_driver_rawreadok_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, jsm_rawreadok);
+}
+static DRIVER_ATTR(rawreadok, S_IRUSR, jsm_driver_rawreadok_show, NULL);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, driver_attr_version);
+ driver_create_file(driverfs, driver_attr_boards);
+ driver_create_file(driverfs, driver_attr_debug);
+
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Who needs to know if a port is open or not? snipped some code +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf) No, please delete these, and the other sysfs files that duplicate the same info that you can get by using the standard Linux termios calls. There is no need for them here. thanks, greg k-h Hi Greg, Wendy, all, Our serial port monitoring tools need to know these things, and to find them out *without* opening up the serial port to do an ioctl(). For example, lets say a customer has a modem connected to a serial port. If you were to open up the port with an stty -a to get the current settings and signals, you would unintentionally raise RTS and DTR. Now the modem sees DTR raised, and might react to it by mistake. Usually raising and dropping RTS/DTR quickly on a modem won't hurt anything, but its not particularly a good when the modem is not expecting it. This is why we export the various signals/stats/signals to sysfs (used to be proc), so our management tools can get the information about the serial port without being intrusive by opening up the port. Scott - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 01:55:33PM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = jsm_total_boardnum();
+ return snprintf(buf, PAGE_SIZE, %d\n, adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
Why is this file even needed? You can just look at the number of sysfs
directories attached to this device, right?
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 03:47:45PM -0600, Kilau, Scott wrote: Who needs to know if a port is open or not? snipped some code +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf) No, please delete these, and the other sysfs files that duplicate the same info that you can get by using the standard Linux termios calls. There is no need for them here. Our serial port monitoring tools need to know these things, and to find them out *without* opening up the serial port to do an ioctl(). For example, lets say a customer has a modem connected to a serial port. If you were to open up the port with an stty -a to get the current settings and signals, you would unintentionally raise RTS and DTR. Why not fix the driver to not change the current line settings if it is not being opened for the first time? That seems like a much simpler way to solve this, and probably the saner way, as you don't want any user to be able to mess up your modem... thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [ patch 4/7] drivers/serial/jsm: new serial device driver
If you were to open up the port with an stty -a to get the current settings and signals, you would unintentionally raise RTS and DTR. Why not fix the driver to not change the current line settings if it is not being opened for the first time? That seems like a much simpler way to solve this, and probably the saner way, as you don't want any user to be able to mess up your modem... thanks, greg k-h Oh, when the port is already open, the driver correctly would not muck with DTR/RTS. I am talking about when the port is currently not open. On first port open, DTR (and usually RTS) will always be raised. The serial device would see this DTR raise, and under some circumstances, react to it... We have customers that use *really* *really* old serial devices on our products, (we are talking 110 baud and even 50 baud (!!!)), where an unintentional raise of DTR/RTS will freak the device out. At any rate, that's the reason I exported the values to sysfs in the original dgnc (outside-the-kernel-sources) driver. Thanks, Scott - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Tue, Mar 08, 2005 at 08:36:57PM -0600, Kilau, Scott wrote: If you were to open up the port with an stty -a to get the current settings and signals, you would unintentionally raise RTS and DTR. Why not fix the driver to not change the current line settings if it is not being opened for the first time? That seems like a much simpler way to solve this, and probably the saner way, as you don't want any user to be able to mess up your modem... Oh, when the port is already open, the driver correctly would not muck with DTR/RTS. I am talking about when the port is currently not open. On first port open, DTR (and usually RTS) will always be raised. The serial device would see this DTR raise, and under some circumstances, react to it... Ok, well, that sounds like something that all serial devices should support, right? And if so, why not add this to the serial core for everyone to benifit? thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
> +static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "jsm_version: %s\n", JSM_VERSION);
Again, drop the "prefix:" from every sysfs file, it should not be there
(the data type is inferred by the name of the file, you do not have to
repeat it again.)
> +static ssize_t jsm_tty_state_show(struct class_device *class_dev, char *buf)
> +{
> + struct jsm_channel *ch;
> +
> + if (class_dev) {
> + ch = class_get_devdata(class_dev);
> + if ( ch && (ch->ch_bd->state == BOARD_READY))
> + return snprintf(buf, PAGE_SIZE, "%s\n",
> ch->ch_open_count ? "Open" : "Closed");
> + }
> +
> + return -EINVAL;
> +}
> +static CLASS_DEVICE_ATTR(state, S_IRUGO, jsm_tty_state_show, NULL);
Who needs to know if a port is open or not?
> +static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
> +{
> + struct jsm_channel *ch;
> +
> + if (class_dev) {
> + ch = class_get_devdata(class_dev);
> + if ( ch && (ch->ch_bd->state == BOARD_READY))
> + return snprintf(buf, PAGE_SIZE, "%d\n", ch->ch_old_baud);
> + }
> +
> + return -EINVAL;
> +}
> +static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
No, please delete these, and the other sysfs files that duplicate the
same info that you can get by using the standard Linux termios calls.
There is no need for them here.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Fri, Mar 04, 2005 at 04:08:17PM -0500, Wen Xiong wrote:
+int get_jsm_board_number(void)
+{
+struct list_head *tmp;
+struct jsm_board *cur_board_entry;
+int adapter_count = 0;
+u64 lock_flags;
+
+spin_lock_irqsave(_board_head_lock, lock_flags);
+list_for_each(tmp, _board_head) {
+cur_board_entry =
+list_entry(tmp, struct jsm_board,
+jsm_board_entry);
+adapter_count++;
+}
+spin_unlock_irqrestore(_board_head_lock, lock_flags);
+
+return adapter_count;
+}
Should this be static?
And it's returning the number of boards, not the current board number,
right?
And you have a indenting error in the list_for_each() section...
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "jsm_version: %s\n", "jsm: 1.1-1-INKERNEL");
Shouldn't that value also be in MODULE_VERSION()? And if so, it should
be a #define somewhere.
Also, don't put "jsm:" in your sysfs files, the file name describes what
the value should be. That goes for a lot of your sysfs files.
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", debug);
+}
"debug" is not a nice variable to have in the global namespace :(
Also, why not just make this a module paramater, that way it can be
modified through that interface, and you don't have to create your own?
+#define JSM_VERIFY_BOARD(p, bd)\
+ if (!p) \
+ return 0;\
+ bd = (struct jsm_board *)dev_get_drvdata(p); \
Cast is not needed.
+ if (!bd) \
+ return 0;\
+ if (bd->state != BOARD_READY) \
+ return 0;\
Don't break out of functions from within a macro. Will cause headaches
for people reviewing your code in the future.
And shouldn't you be returning an error if one of these checks fail?
+static ssize_t jsm_ports_state_show(struct device *p, char *buf)
+{
+ struct jsm_board *bd;
+ int count = 0;
+ int i = 0;
+
+ JSM_VERIFY_BOARD(p, bd);
+
+ for (i = 0; i < bd->nasync; i++) {
+ count += snprintf(buf + count, PAGE_SIZE - count,
+ "%d %s\n", bd->channels[i]->ch_portnum,
+ bd->channels[i]->ch_open_count ? "Open" : "Closed");
+ }
+ return count;
No, make this a per-port value. You are showing more than one value in
a single file. You do this for a few other sysfs files :(
And who cares about this value?
+static ssize_t jsm_ports_baud_show(struct device *p, char *buf)
+{
+ struct jsm_board *bd;
+ int count = 0;
+ int i = 0;
+
+ JSM_VERIFY_BOARD(p, bd);
+
+ for (i = 0; i < bd->nasync; i++) {
+ count += snprintf(buf + count, PAGE_SIZE - count,
+ "%d %d\n", bd->channels[i]->ch_portnum, bd->channels[i]->ch_old_baud);
+ }
+ return count;
+}
+static DEVICE_ATTR(ports_baud, S_IRUSR, jsm_ports_baud_show, NULL);
What's wrong with the standard tty ioctls that return this value, and
the other values you are exporting through sysfs? What is all of this
data needed for?
+#define JSM_VERIFY_CHANNEL(p, ch) \
+ if (!p) \
+ return 0;\
+ ch = (struct jsm_channel *)class_get_devdata(p);\
+ if (!ch) \
+ return 0;\
+ if (ch->ch_bd->state != BOARD_READY) \
+ return 0;\
Again, don't put return in a macro, and return an error if there really
is one.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Hi Greg,
Since some tools in Digi need some new ioctls, so I still keep some new
ioctls.
Thanks for your reviewing!
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-07
16:27:47.293998096 -0600
@@ -0,0 +1,289 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Fri, Mar 04, 2005 at 04:08:17PM -0500, Wen Xiong wrote:
+int get_jsm_board_number(void)
+{
+struct list_head *tmp;
+struct jsm_board *cur_board_entry;
+int adapter_count = 0;
+u64 lock_flags;
+
+spin_lock_irqsave(jsm_board_head_lock, lock_flags);
+list_for_each(tmp, jsm_board_head) {
+cur_board_entry =
+list_entry(tmp, struct jsm_board,
+jsm_board_entry);
+adapter_count++;
+}
+spin_unlock_irqrestore(jsm_board_head_lock, lock_flags);
+
+return adapter_count;
+}
Should this be static?
And it's returning the number of boards, not the current board number,
right?
And you have a indenting error in the list_for_each() section...
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, jsm_version: %s\n, jsm: 1.1-1-INKERNEL);
Shouldn't that value also be in MODULE_VERSION()? And if so, it should
be a #define somewhere.
Also, don't put jsm: in your sysfs files, the file name describes what
the value should be. That goes for a lot of your sysfs files.
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, debug);
+}
debug is not a nice variable to have in the global namespace :(
Also, why not just make this a module paramater, that way it can be
modified through that interface, and you don't have to create your own?
+#define JSM_VERIFY_BOARD(p, bd)\
+ if (!p) \
+ return 0;\
+ bd = (struct jsm_board *)dev_get_drvdata(p); \
Cast is not needed.
+ if (!bd) \
+ return 0;\
+ if (bd-state != BOARD_READY) \
+ return 0;\
Don't break out of functions from within a macro. Will cause headaches
for people reviewing your code in the future.
And shouldn't you be returning an error if one of these checks fail?
+static ssize_t jsm_ports_state_show(struct device *p, char *buf)
+{
+ struct jsm_board *bd;
+ int count = 0;
+ int i = 0;
+
+ JSM_VERIFY_BOARD(p, bd);
+
+ for (i = 0; i bd-nasync; i++) {
+ count += snprintf(buf + count, PAGE_SIZE - count,
+ %d %s\n, bd-channels[i]-ch_portnum,
+ bd-channels[i]-ch_open_count ? Open : Closed);
+ }
+ return count;
No, make this a per-port value. You are showing more than one value in
a single file. You do this for a few other sysfs files :(
And who cares about this value?
+static ssize_t jsm_ports_baud_show(struct device *p, char *buf)
+{
+ struct jsm_board *bd;
+ int count = 0;
+ int i = 0;
+
+ JSM_VERIFY_BOARD(p, bd);
+
+ for (i = 0; i bd-nasync; i++) {
+ count += snprintf(buf + count, PAGE_SIZE - count,
+ %d %d\n, bd-channels[i]-ch_portnum, bd-channels[i]-ch_old_baud);
+ }
+ return count;
+}
+static DEVICE_ATTR(ports_baud, S_IRUSR, jsm_ports_baud_show, NULL);
What's wrong with the standard tty ioctls that return this value, and
the other values you are exporting through sysfs? What is all of this
data needed for?
+#define JSM_VERIFY_CHANNEL(p, ch) \
+ if (!p) \
+ return 0;\
+ ch = (struct jsm_channel *)class_get_devdata(p);\
+ if (!ch) \
+ return 0;\
+ if (ch-ch_bd-state != BOARD_READY) \
+ return 0;\
Again, don't put return in a macro, and return an error if there really
is one.
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Hi Greg,
Since some tools in Digi need some new ioctls, so I still keep some new
ioctls.
Thanks for your reviewing!
wendy
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-07
16:27:47.293998096 -0600
@@ -0,0 +1,289 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Mon, Mar 07, 2005 at 05:46:51PM -0500, Wen Xiong wrote:
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, jsm_version: %s\n, JSM_VERSION);
Again, drop the prefix: from every sysfs file, it should not be there
(the data type is inferred by the name of the file, you do not have to
repeat it again.)
+static ssize_t jsm_tty_state_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch (ch-ch_bd-state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, %s\n,
ch-ch_open_count ? Open : Closed);
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(state, S_IRUGO, jsm_tty_state_show, NULL);
Who needs to know if a port is open or not?
+static ssize_t jsm_tty_baud_show(struct class_device *class_dev, char *buf)
+{
+ struct jsm_channel *ch;
+
+ if (class_dev) {
+ ch = class_get_devdata(class_dev);
+ if ( ch (ch-ch_bd-state == BOARD_READY))
+ return snprintf(buf, PAGE_SIZE, %d\n, ch-ch_old_baud);
+ }
+
+ return -EINVAL;
+}
+static CLASS_DEVICE_ATTR(baud, S_IRUGO, jsm_tty_baud_show, NULL);
No, please delete these, and the other sysfs files that duplicate the
same info that you can get by using the standard Linux termios calls.
There is no need for them here.
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Fri, Mar 04, 2005 at 04:08:17PM -0500, Wen Xiong wrote:
> +int get_jsm_board_number(void)
> +{
> +struct list_head *tmp;
> +struct jsm_board *cur_board_entry;
> +int adapter_count = 0;
> +u64 lock_flags;
> +
> +spin_lock_irqsave(_board_head_lock, lock_flags);
> +list_for_each(tmp, _board_head) {
> +cur_board_entry =
> +list_entry(tmp, struct jsm_board,
> +jsm_board_entry);
> +adapter_count++;
> +}
> +spin_unlock_irqrestore(_board_head_lock, lock_flags);
> +
> +return adapter_count;
> +}
Should this be static?
And it's returning the number of boards, not the current board number,
right?
And you have a indenting error in the list_for_each() section...
> +static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "jsm_version: %s\n", "jsm:
> 1.1-1-INKERNEL");
Shouldn't that value also be in MODULE_VERSION()? And if so, it should
be a #define somewhere.
Also, don't put "jsm:" in your sysfs files, the file name describes what
the value should be. That goes for a lot of your sysfs files.
> +static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
> +{
> + return snprintf(buf, PAGE_SIZE, "0x%x\n", debug);
> +}
"debug" is not a nice variable to have in the global namespace :(
Also, why not just make this a module paramater, that way it can be
modified through that interface, and you don't have to create your own?
> +#define JSM_VERIFY_BOARD(p, bd) \
> + if (!p) \
> + return 0; \
> + bd = (struct jsm_board *)dev_get_drvdata(p);\
Cast is not needed.
> + if (!bd)\
> + return 0; \
> + if (bd->state != BOARD_READY) \
> + return 0; \
Don't break out of functions from within a macro. Will cause headaches
for people reviewing your code in the future.
And shouldn't you be returning an error if one of these checks fail?
> +static ssize_t jsm_ports_state_show(struct device *p, char *buf)
> +{
> + struct jsm_board *bd;
> + int count = 0;
> + int i = 0;
> +
> + JSM_VERIFY_BOARD(p, bd);
> +
> + for (i = 0; i < bd->nasync; i++) {
> + count += snprintf(buf + count, PAGE_SIZE - count,
> + "%d %s\n", bd->channels[i]->ch_portnum,
> + bd->channels[i]->ch_open_count ? "Open" : "Closed");
> + }
> + return count;
No, make this a per-port value. You are showing more than one value in
a single file. You do this for a few other sysfs files :(
And who cares about this value?
> +static ssize_t jsm_ports_baud_show(struct device *p, char *buf)
> +{
> + struct jsm_board *bd;
> + int count = 0;
> + int i = 0;
> +
> + JSM_VERIFY_BOARD(p, bd);
> +
> + for (i = 0; i < bd->nasync; i++) {
> + count += snprintf(buf + count, PAGE_SIZE - count,
> + "%d %d\n", bd->channels[i]->ch_portnum,
> bd->channels[i]->ch_old_baud);
> + }
> + return count;
> +}
> +static DEVICE_ATTR(ports_baud, S_IRUSR, jsm_ports_baud_show, NULL);
What's wrong with the standard tty ioctls that return this value, and
the other values you are exporting through sysfs? What is all of this
data needed for?
> +#define JSM_VERIFY_CHANNEL(p, ch)\
> + if (!p) \
> + return 0; \
> + ch = (struct jsm_channel *)class_get_devdata(p);\
> + if (!ch)\
> + return 0; \
> + if (ch->ch_bd->state != BOARD_READY)\
> + return 0; \
Again, don't put return in a macro, and return an error if there really
is one.
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Sun, Feb 27, 2005 at 06:39:51PM -0500, Wen Xiong wrote:
This patch is for jsm_proc.c and includes the functions relating to
/proc/jsm entry.
No, don't add new /proc stuff. Use sysfs, and if you want to spit out
more data, use debugfs.
What is the need for these files?
thanks,
greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Changed to use sysfs.
Signed-off-by: Wen Xiong <[EMAIL PROTECTED]>
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-04
11:24:37.962944848 -0600
@@ -0,0 +1,463 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau <[EMAIL PROTECTED]>
+ * Wendy Xiong <[EMAIL PROTECTED]>
+ *
+ ***/
+
+#include "jsm_driver.h"
+
+static struct class_simple *jsm_tty_class;
+
+int get_jsm_board_number(void)
+{
+struct list_head *tmp;
+struct jsm_board *cur_board_entry;
+int adapter_count = 0;
+u64 lock_flags;
+
+spin_lock_irqsave(_board_head_lock, lock_flags);
+list_for_each(tmp, _board_head) {
+cur_board_entry =
+list_entry(tmp, struct jsm_board,
+jsm_board_entry);
+adapter_count++;
+}
+spin_unlock_irqrestore(_board_head_lock, lock_flags);
+
+return adapter_count;
+}
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "jsm_version: %s\n", "jsm:
1.1-1-INKERNEL");
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = get_jsm_board_number();
+ return snprintf(buf, PAGE_SIZE, "jsm_board_number: %d\n",
adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "jsm_state: %s\n",
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", debug);
+}
+
+static ssize_t jsm_driver_debug_store(struct device_driver *ddp, const char
*buf, size_t count)
+{
+ sscanf(buf, "0x%x\n", );
+ return count;
+}
+static DRIVER_ATTR(debug, (S_IRUSR | S_IWUSR), jsm_driver_debug_show,
jsm_driver_debug_store);
+
+
+static ssize_t jsm_driver_rawreadok_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "0x%x\n", rawreadok);
+}
+
+static ssize_t jsm_driver_rawreadok_store(struct device_driver *ddp, const
char *buf, size_t count)
+{
+ sscanf(buf, "0x%x\n", );
+ return count;
+}
+static DRIVER_ATTR(rawreadok, (S_IRUSR | S_IWUSR), jsm_driver_rawreadok_show,
jsm_driver_rawreadok_store);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, _attr_version);
+ driver_create_file(driverfs, _attr_boards);
+ driver_create_file(driverfs, _attr_debug);
+ driver_create_file(driverfs, _attr_rawreadok);
+ driver_create_file(driverfs, _attr_state);
+}
+
+void jsm_remove_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_remove_file(driverfs, _attr_version);
+ driver_remove_file(driverfs, _attr_boards);
+ driver_remove_file(driverfs, _attr_debug);
+ driver_remove_file(driverfs, _attr_rawreadok);
+ driver_remove_file(driverfs, _attr_state);
+}
+
+#define
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
Greg KH wrote:
On Sun, Feb 27, 2005 at 06:39:51PM -0500, Wen Xiong wrote:
This patch is for jsm_proc.c and includes the functions relating to
/proc/jsm entry.
No, don't add new /proc stuff. Use sysfs, and if you want to spit out
more data, use debugfs.
What is the need for these files?
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Changed to use sysfs.
Signed-off-by: Wen Xiong [EMAIL PROTECTED]
diff -Nuar linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c
linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c
--- linux-2.6.11.org/drivers/serial/jsm/jsm_sysfs.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.11.new/drivers/serial/jsm/jsm_sysfs.c 2005-03-04
11:24:37.962944848 -0600
@@ -0,0 +1,463 @@
+/
+ * Copyright 2003 Digi International (www.digi.com)
+ *
+ * Copyright (C) 2004 IBM Corporation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 * Temple Place - Suite 330, Boston,
+ * MA 02111-1307, USA.
+ *
+ * Contact Information:
+ * Scott H Kilau [EMAIL PROTECTED]
+ * Wendy Xiong [EMAIL PROTECTED]
+ *
+ ***/
+
+#include jsm_driver.h
+
+static struct class_simple *jsm_tty_class;
+
+int get_jsm_board_number(void)
+{
+struct list_head *tmp;
+struct jsm_board *cur_board_entry;
+int adapter_count = 0;
+u64 lock_flags;
+
+spin_lock_irqsave(jsm_board_head_lock, lock_flags);
+list_for_each(tmp, jsm_board_head) {
+cur_board_entry =
+list_entry(tmp, struct jsm_board,
+jsm_board_entry);
+adapter_count++;
+}
+spin_unlock_irqrestore(jsm_board_head_lock, lock_flags);
+
+return adapter_count;
+}
+
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, jsm_version: %s\n, jsm:
1.1-1-INKERNEL);
+}
+static DRIVER_ATTR(version, S_IRUSR, jsm_driver_version_show, NULL);
+
+static ssize_t jsm_driver_boards_show(struct device_driver *ddp, char *buf)
+{
+ int adapter_count = 0;
+ adapter_count = get_jsm_board_number();
+ return snprintf(buf, PAGE_SIZE, jsm_board_number: %d\n,
adapter_count);
+}
+static DRIVER_ATTR(boards, S_IRUSR, jsm_driver_boards_show, NULL);
+
+static ssize_t jsm_driver_state_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, jsm_state: %s\n,
jsm_driver_state_text[jsm_driver_state]);
+}
+static DRIVER_ATTR(state, S_IRUSR, jsm_driver_state_show, NULL);
+
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, debug);
+}
+
+static ssize_t jsm_driver_debug_store(struct device_driver *ddp, const char
*buf, size_t count)
+{
+ sscanf(buf, 0x%x\n, debug);
+ return count;
+}
+static DRIVER_ATTR(debug, (S_IRUSR | S_IWUSR), jsm_driver_debug_show,
jsm_driver_debug_store);
+
+
+static ssize_t jsm_driver_rawreadok_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, rawreadok);
+}
+
+static ssize_t jsm_driver_rawreadok_store(struct device_driver *ddp, const
char *buf, size_t count)
+{
+ sscanf(buf, 0x%x\n, rawreadok);
+ return count;
+}
+static DRIVER_ATTR(rawreadok, (S_IRUSR | S_IWUSR), jsm_driver_rawreadok_show,
jsm_driver_rawreadok_store);
+
+void jsm_create_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_create_file(driverfs, driver_attr_version);
+ driver_create_file(driverfs, driver_attr_boards);
+ driver_create_file(driverfs, driver_attr_debug);
+ driver_create_file(driverfs, driver_attr_rawreadok);
+ driver_create_file(driverfs, driver_attr_state);
+}
+
+void jsm_remove_driver_sysfiles(struct device_driver *driverfs)
+{
+ driver_remove_file(driverfs, driver_attr_version);
+ driver_remove_file(driverfs, driver_attr_boards);
+ driver_remove_file(driverfs, driver_attr_debug);
+ driver_remove_file(driverfs, driver_attr_rawreadok);
+
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Fri, Mar 04, 2005 at 04:08:17PM -0500, Wen Xiong wrote:
+int get_jsm_board_number(void)
+{
+struct list_head *tmp;
+struct jsm_board *cur_board_entry;
+int adapter_count = 0;
+u64 lock_flags;
+
+spin_lock_irqsave(jsm_board_head_lock, lock_flags);
+list_for_each(tmp, jsm_board_head) {
+cur_board_entry =
+list_entry(tmp, struct jsm_board,
+jsm_board_entry);
+adapter_count++;
+}
+spin_unlock_irqrestore(jsm_board_head_lock, lock_flags);
+
+return adapter_count;
+}
Should this be static?
And it's returning the number of boards, not the current board number,
right?
And you have a indenting error in the list_for_each() section...
+static ssize_t jsm_driver_version_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, jsm_version: %s\n, jsm:
1.1-1-INKERNEL);
Shouldn't that value also be in MODULE_VERSION()? And if so, it should
be a #define somewhere.
Also, don't put jsm: in your sysfs files, the file name describes what
the value should be. That goes for a lot of your sysfs files.
+static ssize_t jsm_driver_debug_show(struct device_driver *ddp, char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, 0x%x\n, debug);
+}
debug is not a nice variable to have in the global namespace :(
Also, why not just make this a module paramater, that way it can be
modified through that interface, and you don't have to create your own?
+#define JSM_VERIFY_BOARD(p, bd) \
+ if (!p) \
+ return 0; \
+ bd = (struct jsm_board *)dev_get_drvdata(p);\
Cast is not needed.
+ if (!bd)\
+ return 0; \
+ if (bd-state != BOARD_READY) \
+ return 0; \
Don't break out of functions from within a macro. Will cause headaches
for people reviewing your code in the future.
And shouldn't you be returning an error if one of these checks fail?
+static ssize_t jsm_ports_state_show(struct device *p, char *buf)
+{
+ struct jsm_board *bd;
+ int count = 0;
+ int i = 0;
+
+ JSM_VERIFY_BOARD(p, bd);
+
+ for (i = 0; i bd-nasync; i++) {
+ count += snprintf(buf + count, PAGE_SIZE - count,
+ %d %s\n, bd-channels[i]-ch_portnum,
+ bd-channels[i]-ch_open_count ? Open : Closed);
+ }
+ return count;
No, make this a per-port value. You are showing more than one value in
a single file. You do this for a few other sysfs files :(
And who cares about this value?
+static ssize_t jsm_ports_baud_show(struct device *p, char *buf)
+{
+ struct jsm_board *bd;
+ int count = 0;
+ int i = 0;
+
+ JSM_VERIFY_BOARD(p, bd);
+
+ for (i = 0; i bd-nasync; i++) {
+ count += snprintf(buf + count, PAGE_SIZE - count,
+ %d %d\n, bd-channels[i]-ch_portnum,
bd-channels[i]-ch_old_baud);
+ }
+ return count;
+}
+static DEVICE_ATTR(ports_baud, S_IRUSR, jsm_ports_baud_show, NULL);
What's wrong with the standard tty ioctls that return this value, and
the other values you are exporting through sysfs? What is all of this
data needed for?
+#define JSM_VERIFY_CHANNEL(p, ch)\
+ if (!p) \
+ return 0; \
+ ch = (struct jsm_channel *)class_get_devdata(p);\
+ if (!ch)\
+ return 0; \
+ if (ch-ch_bd-state != BOARD_READY)\
+ return 0; \
Again, don't put return in a macro, and return an error if there really
is one.
thanks,
greg k-h
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Sun, Feb 27, 2005 at 06:39:51PM -0500, Wen Xiong wrote: > This patch is for jsm_proc.c and includes the functions relating to > /proc/jsm entry. No, don't add new /proc stuff. Use sysfs, and if you want to spit out more data, use debugfs. What is the need for these files? thanks, greg k-h - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Sun, Feb 27, 2005 at 06:39:51PM -0500, Wen Xiong wrote: > This patch is for jsm_proc.c and includes the functions relating to > /proc/jsm entry. please don't put in more procfs driver interfaces. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[ patch 4/7] drivers/serial/jsm: new serial device driver
This patch is for jsm_proc.c and includes the functions relating to
/proc/jsm entry.
Signed-off-by: Wen Xiong <[EMAIL PROTECTED]>
diff -Nuar linux-2.6.9.orig/drivers/serial/jsm/jsm_proc.c
linux-2.6.9.new/drivers/serial/jsm/jsm_proc.c
--- linux-2.6.9.orig/drivers/serial/jsm/jsm_proc.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.9.new/drivers/serial/jsm/jsm_proc.c 2005-02-27
17:13:14.339983544 -0600
@@ -0,0 +1,951 @@
+/*
+ * Copyright 2003 Digi International (www.digi.com)
+ * Scott H Kilau
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ *
+ * NOTE TO LINUX KERNEL HACKERS: DO NOT REFORMAT THIS CODE!
+ *
+ * This is shared code between Digi's CVS archive and the
+ * Linux Kernel sources.
+ * Changing the source just for reformatting needlessly breaks
+ * our CVS diff history.
+ *
+ * Send any bug fixes/changes to: Eng.Linux at digi dot com.
+ * Thank you.
+ *
+ *
+ * $Id: jsm_proc.c,v 1.38 2004/08/30 21:39:40 scottk Exp $
+ */
+#include
+#include
+#include/* For jiffies, task states */
+#include/* For tasklet and interrupt structs/defines */
+#include
+#include
+#include
+#include
+#include
+#include/* For in_egroup_p() */
+#include
+#include/* For copy_from_user/copy_to_user */
+
+#include "jsm_driver.h"
+#include "jsm_mgmt.h"
+
+/* The /proc/jsm directory */
+static struct proc_dir_entry *ProcJSM;
+
+static void *jsm_info_seq_start(struct seq_file *seq, loff_t *pos)
+{
+ if (*pos > 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void *jsm_info_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ ++*pos;
+ if (*pos > 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void jsm_info_seq_stop(struct seq_file *seq, void *v)
+{
+
+}
+
+static int jsm_info_seq_show(struct seq_file *seq, void *v)
+{
+
+ seq_printf(seq, "Driver:\t\t%s\n", "jsm - 1.1-1-INKERNEL");
+ seq_printf(seq, "\n");
+ seq_printf(seq, "Debug:\t\t0x%x\n", jsm_debug);
+ seq_printf(seq, "Rawreadok:\t0x%x\n", jsm_rawreadok);
+ seq_printf(seq, "Max Boards:\t%d\n", MAXBOARDS);
+ seq_printf(seq, "Total Boards:\t%d\n", jsm_NumBoards);
+ seq_printf(seq, "Poll counter:\t%ld\n", jsm_poll_counter);
+ seq_printf(seq, "State:\t\t%s\n",
jsm_driver_state_text[jsm_driver_state]);
+ return 0;
+
+}
+
+static struct seq_operations jsm_info_seq_ops = {
+ .start = jsm_info_seq_start,
+ .next = jsm_info_seq_next,
+ .stop = jsm_info_seq_stop,
+ .show = jsm_info_seq_show,
+};
+
+static int jsm_info_open(struct inode *inode, struct file *file)
+{
+ struct seq_file *seq;
+ struct proc_dir_entry *proc;
+ int res;
+
+ res = seq_open(file, _info_seq_ops);
+ if (!res) {
+ /* recover the pointer buried in proc_dir_entry data */
+ seq = file->private_data;
+ proc = PDE(inode);
+ seq->private = proc->data;
+ }
+ return res;
+}
+
+
+static void *jsm_mknod_seq_start(struct seq_file *seq, loff_t *pos)
+{
+ if (*pos > 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void *jsm_mknod_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ ++*pos;
+ if (*pos > 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void jsm_mknod_seq_stop(struct seq_file *seq, void *v)
+{
+
+}
+
+static int jsm_mknod_seq_show(struct seq_file *seq, void *v)
+{
+ int i;
+ seq_printf(seq, "#\tCreate the management devices.\n");
+
+ for (i = 0; i < MAXMGMTDEVICES; i++) {
+ char tmp[100];
+ sprintf(tmp, "/dev/jsm/mgmt%d", i);
+ seq_printf(seq, "%s\t%d\t%d\t%d\n",
+ tmp, jsm_Major, i, 1);
+ }
+ return 0;
+}
+
+static struct seq_operations jsm_mknod_seq_ops = {
+ .start = jsm_mknod_seq_start,
+ .next = jsm_mknod_seq_next,
+ .stop = jsm_mknod_seq_stop,
+ .show = jsm_mknod_seq_show,
+};
+
+static int jsm_mknod_open(struct inode *inode, struct file *file)
+{
+ struct seq_file *seq;
+ struct proc_dir_entry *proc;
+ int
[ patch 4/7] drivers/serial/jsm: new serial device driver
This patch is for jsm_proc.c and includes the functions relating to
/proc/jsm entry.
Signed-off-by: Wen Xiong [EMAIL PROTECTED]
diff -Nuar linux-2.6.9.orig/drivers/serial/jsm/jsm_proc.c
linux-2.6.9.new/drivers/serial/jsm/jsm_proc.c
--- linux-2.6.9.orig/drivers/serial/jsm/jsm_proc.c 1969-12-31
18:00:00.0 -0600
+++ linux-2.6.9.new/drivers/serial/jsm/jsm_proc.c 2005-02-27
17:13:14.339983544 -0600
@@ -0,0 +1,951 @@
+/*
+ * Copyright 2003 Digi International (www.digi.com)
+ * Scott H Kilau Scott_Kilau at digi dot com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2, or (at your option)
+ * any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY, EXPRESS OR IMPLIED; without even the
+ * implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR
+ * PURPOSE. See the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ *
+ *
+ * NOTE TO LINUX KERNEL HACKERS: DO NOT REFORMAT THIS CODE!
+ *
+ * This is shared code between Digi's CVS archive and the
+ * Linux Kernel sources.
+ * Changing the source just for reformatting needlessly breaks
+ * our CVS diff history.
+ *
+ * Send any bug fixes/changes to: Eng.Linux at digi dot com.
+ * Thank you.
+ *
+ *
+ * $Id: jsm_proc.c,v 1.38 2004/08/30 21:39:40 scottk Exp $
+ */
+#include linux/kernel.h
+#include linux/version.h
+#include linux/sched.h /* For jiffies, task states */
+#include linux/interrupt.h /* For tasklet and interrupt structs/defines */
+#include linux/module.h
+#include linux/ctype.h
+#include linux/proc_fs.h
+#include linux/seq_file.h
+#include linux/serial_reg.h
+#include linux/sched.h /* For in_egroup_p() */
+#include linux/string.h
+#include asm/uaccess.h /* For copy_from_user/copy_to_user */
+
+#include jsm_driver.h
+#include jsm_mgmt.h
+
+/* The /proc/jsm directory */
+static struct proc_dir_entry *ProcJSM;
+
+static void *jsm_info_seq_start(struct seq_file *seq, loff_t *pos)
+{
+ if (*pos 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void *jsm_info_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ ++*pos;
+ if (*pos 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void jsm_info_seq_stop(struct seq_file *seq, void *v)
+{
+
+}
+
+static int jsm_info_seq_show(struct seq_file *seq, void *v)
+{
+
+ seq_printf(seq, Driver:\t\t%s\n, jsm - 1.1-1-INKERNEL);
+ seq_printf(seq, \n);
+ seq_printf(seq, Debug:\t\t0x%x\n, jsm_debug);
+ seq_printf(seq, Rawreadok:\t0x%x\n, jsm_rawreadok);
+ seq_printf(seq, Max Boards:\t%d\n, MAXBOARDS);
+ seq_printf(seq, Total Boards:\t%d\n, jsm_NumBoards);
+ seq_printf(seq, Poll counter:\t%ld\n, jsm_poll_counter);
+ seq_printf(seq, State:\t\t%s\n,
jsm_driver_state_text[jsm_driver_state]);
+ return 0;
+
+}
+
+static struct seq_operations jsm_info_seq_ops = {
+ .start = jsm_info_seq_start,
+ .next = jsm_info_seq_next,
+ .stop = jsm_info_seq_stop,
+ .show = jsm_info_seq_show,
+};
+
+static int jsm_info_open(struct inode *inode, struct file *file)
+{
+ struct seq_file *seq;
+ struct proc_dir_entry *proc;
+ int res;
+
+ res = seq_open(file, jsm_info_seq_ops);
+ if (!res) {
+ /* recover the pointer buried in proc_dir_entry data */
+ seq = file-private_data;
+ proc = PDE(inode);
+ seq-private = proc-data;
+ }
+ return res;
+}
+
+
+static void *jsm_mknod_seq_start(struct seq_file *seq, loff_t *pos)
+{
+ if (*pos 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void *jsm_mknod_seq_next(struct seq_file *seq, void *v, loff_t *pos)
+{
+ ++*pos;
+ if (*pos 0)
+ return NULL;
+ else
+ return (void *)1;
+}
+
+static void jsm_mknod_seq_stop(struct seq_file *seq, void *v)
+{
+
+}
+
+static int jsm_mknod_seq_show(struct seq_file *seq, void *v)
+{
+ int i;
+ seq_printf(seq, #\tCreate the management devices.\n);
+
+ for (i = 0; i MAXMGMTDEVICES; i++) {
+ char tmp[100];
+ sprintf(tmp, /dev/jsm/mgmt%d, i);
+ seq_printf(seq, %s\t%d\t%d\t%d\n,
+ tmp, jsm_Major, i, 1);
+ }
+ return 0;
+}
+
+static struct seq_operations jsm_mknod_seq_ops = {
+ .start = jsm_mknod_seq_start,
+ .next = jsm_mknod_seq_next,
+ .stop = jsm_mknod_seq_stop,
+ .show =
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Sun, Feb 27, 2005 at 06:39:51PM -0500, Wen Xiong wrote: This patch is for jsm_proc.c and includes the functions relating to /proc/jsm entry. please don't put in more procfs driver interfaces. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [ patch 4/7] drivers/serial/jsm: new serial device driver
On Sun, Feb 27, 2005 at 06:39:51PM -0500, Wen Xiong wrote: This patch is for jsm_proc.c and includes the functions relating to /proc/jsm entry. No, don't add new /proc stuff. Use sysfs, and if you want to spit out more data, use debugfs. What is the need for these files? thanks, greg k-h - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
