Re: Re: Re: [PATCH V6] serial/uart/8250: Add tunable RX interrupt trigger I/F of FIFO buffers
From: Yoshihiro YUNOMAE
Date: Wed May 07 2014 - 22:16:13 EST
Hi Greg,
Sorry for the late reply.
(2014/04/26 1:01), Greg Kroah-Hartman wrote:
On Fri, Apr 25, 2014 at 05:53:02PM +0900, Yoshihiro YUNOMAE wrote:
Hi Greg,
Thank you for your review.
(2014/04/25 8:11), Greg Kroah-Hartman wrote:
On Thu, Apr 17, 2014 at 03:06:44PM +0900, Yoshihiro YUNOMAE wrote:
[snip]
+static DEVICE_ATTR(rx_int_trig, S_IRUSR | S_IWUSR | S_IRGRP,
+ serial8250_get_attr_rx_int_trig,
+ serial8250_set_attr_rx_int_trig);
+
As you are adding a new sysfs attribute, you have to add a
Documentation/ABI/ entry as well.
I added this attribute to /sys/dev/char/*
What? No. That's not ok, why would it be?
See, Documentation would have pointed that problem out very obviously :)
, so the documentation may be sysfs-dev. However, any other attributes
are not written at all. Should I add this description to it or is
there another file?
It shouldn't be on a char device, that's not acceptable.
My reply that I added this attribute to /sys/dev/char/* was
inappropriate. Actually, I added it to a serial device(ttyS0) in
/sys/devices/ tree. /sys/dev/char/* stores symlinks of devices
in /sys/devices/ tree.
I think the documentation where I should add the description is
Documentation/ABI/testing/sysfs-tty. (/sys/class/* also stores
symlinks of devices in /sys/devices/ tree.)
+static struct attribute *serial8250_dev_attrs[] = {
+ &dev_attr_rx_int_trig.attr,
+ NULL,
+ };
+
+static struct attribute_group serial8250_dev_attr_group = {
+ .attrs = serial8250_dev_attrs,
+ };
What's wrong with the macro to create a group?
I'll explain about this below.
+
+static void register_dev_spec_attr_grp(struct uart_8250_port *up)
+{
+ const struct serial8250_config *conf_type = &uart_config[up->port.type];
+
+ if (conf_type->rxtrig_bytes[0])
+ up->port.dev_spec_attr_group = &serial8250_dev_attr_group;
+}
+
static void serial8250_config_port(struct uart_port *port, int flags)
{
struct uart_8250_port *up =
@@ -2708,6 +2848,9 @@ static void serial8250_config_port(struct uart_port *port, int flags)
if ((port->type == PORT_XR17V35X) ||
(port->type == PORT_XR17D15X))
port->handle_irq = exar_handle_irq;
+
+ register_dev_spec_attr_grp(up);
+ up->fcr = uart_config[up->port.type].fcr;
}
static int
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 2cf5649..41ac44b 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2548,15 +2548,16 @@ static struct attribute *tty_dev_attrs[] = {
NULL,
};
-static const struct attribute_group tty_dev_attr_group = {
+static struct attribute_group tty_dev_attr_group = {
.attrs = tty_dev_attrs,
};
-static const struct attribute_group *tty_dev_attr_groups[] = {
- &tty_dev_attr_group,
- NULL
- };
-
+static void make_uport_attr_grps(struct uart_port *uport)
+{
+ uport->attr_grps[0] = &tty_dev_attr_group;
+ if (uport->dev_spec_attr_group)
+ uport->attr_grps[1] = uport->dev_spec_attr_group;
+}
/**
* uart_add_one_port - attach a driver-defined port structure
@@ -2607,12 +2608,15 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
uart_configure_port(drv, state, uport);
+ make_uport_attr_grps(uport);
+
/*
* Register the port whether it's detected or not. This allows
* setserial to be used to alter this port's parameters.
*/
tty_dev = tty_port_register_device_attr(port, drv->tty_driver,
- uport->line, uport->dev, port, tty_dev_attr_groups);
+ uport->line, uport->dev, port,
+ (const struct attribute_group **)uport->attr_grps);
If you have to cast that hard, something is wrong here, why are you
doing that?
The attribute group in serial layer was defined as constant
because serial layer has only common sysfs I/F. However, I want to
change sysfs I/F for specific devices. So, I deleted 'const' from the
definition of the attribute group in serial layer in order to make the
attribute group be changeable. On the other hand, to pass the attribute
group to tty layer, the group must be const because the 5th variable of
tty_port_register_device_attr() is an attribute group with 'const', so
I implemented like this. Although I investigated again,
tty_port_register_device_attr() is used only here, and
tty_register_device_attr() called by the function is called from 2
locations (the one of them passes NULL in the 5th variable).
Therefore, we can delete 'const' for those functions, I think.
How do you think about this?
I think you need to not be messing with the devices in /sys/dev/char/ at
all...
And why do you feel you need a sysfs attribute at all? What is it going
to be used for? Who needs it? Without knowing that, I can't really
answer your questions...
In the 1st and 2nd patches, I realized this feature by using ioctl(2),
but that was not easy to use because the RX trigger was only changeable
after opening serial fdã(i.e. setting termios). Alan suggested using
sysfs I/F. I think this is better approach because
we can solve the problem easily by using existing platform of serial
devices and users can easily change the RX trigger. So, I added the
device specific sysfs attribute.
Thanks,
Yoshihiro YUNOMAE
--
Yoshihiro YUNOMAE
Software Platform Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: yoshihiro.yunomae.ez@xxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/