Support for read/modify of uartclk via sysfs added.
It may prove useful with some no-name cards that
has different oscillator speeds and no distinguishing
PCI IDs to allow autodetection. It allows better integration
with udev and/or init scripts.
Signed-off-by: Tomas Hlavacek <[email protected]>
---
drivers/tty/serial/serial_core.c | 54 ++++++++++++++++++++++++++++++++++++++
drivers/tty/tty_io.c | 17 ++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 73 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..059b438 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,46 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
return p->tty_driver;
}
+static ssize_t get_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+
+ struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
+ mutex_lock(&state->port.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
+ mutex_unlock(&state->port.mutex);
+ return ret;
+}
+
+static ssize_t set_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
+ unsigned int val;
+ int ret;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&state->port.mutex);
+
+ /*
+ * Check value: baud_base has to be more than 9600
+ * and uartclock = baud_base * 16 .
+ */
+ if (val >= 153600)
+ state->uart_port->uartclk = val;
+
+ mutex_unlock(&state->port.mutex);
+
+ return count;
+}
+
+static DEVICE_ATTR(uartclk, S_IRUGO | S_IWUSR, get_attr_uartclk,
+ set_attr_uartclk);
+
/**
* uart_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
@@ -2355,6 +2395,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
}
/*
+ * Expose uartclk in sysfs. Use driverdata of the tty device for
+ * referencing the UART port.
+ */
+ dev_set_drvdata(tty_dev, state);
+ if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
+ dev_err(tty_dev, "Failed to add uartclk attr\n");
+
+ /*
* Ensure UPF_DEAD is not set.
*/
uport->flags &= ~UPF_DEAD;
@@ -2397,6 +2445,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
/*
+ * Remove uartclk file from sysfs.
+ */
+ device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
+ &dev_attr_uartclk);
+
+ /*
* Remove the devices from the tty layer
*/
tty_unregister_device(drv->tty_driver, uport->line);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..8ea8622 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
}
EXPORT_SYMBOL(tty_unregister_device);
+/*
+ * tty_lookup_device - lookup a tty device
+ * @driver: the tty driver that describes the tty device
+ * @index: the index in the tty driver for this tty device
+ *
+ * This function returns a struct device pointer when device has
+ * been found and NULL otherwise.
+ *
+ * Locking: ??
+ */
+struct device *tty_lookup_device(struct tty_driver *driver, unsigned index)
+{
+ dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
+ return class_find_device(tty_class, NULL, &devt, dev_match_devt);
+}
+EXPORT_SYMBOL(tty_lookup_device);
+
struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
{
struct tty_driver *driver;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..5d408a1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver *driver);
extern int tty_unregister_driver(struct tty_driver *driver);
extern struct device *tty_register_device(struct tty_driver *driver,
unsigned index, struct device *dev);
+extern struct device *tty_lookup_device(struct tty_driver *driver,
+ unsigned index);
extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
int buflen);
--
1.7.10.4
Dear Tomas Hlavacek,
> Support for read/modify of uartclk via sysfs added.
> It may prove useful with some no-name cards that
> has different oscillator speeds and no distinguishing
> PCI IDs to allow autodetection. It allows better integration
> with udev and/or init scripts.
>
> Signed-off-by: Tomas Hlavacek <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 54
> ++++++++++++++++++++++++++++++++++++++ drivers/tty/tty_io.c |
> 17 ++++++++++++
> include/linux/tty.h | 2 ++
> 3 files changed, 73 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c
> b/drivers/tty/serial/serial_core.c index a21dc8e..059b438 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2293,6 +2293,46 @@ struct tty_driver *uart_console_device(struct
> console *co, int *index) return p->tty_driver;
> }
>
> +static ssize_t get_attr_uartclk(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int ret;
> +
> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
You don't need this cast here.
> + mutex_lock(&state->port.mutex);
> + ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
Do you really need such a large buffer (PAGE_SIZE) ?
> + mutex_unlock(&state->port.mutex);
> + return ret;
> +}
> +
> +static ssize_t set_attr_uartclk(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
> + unsigned int val;
> + int ret;
> +
> + ret = kstrtouint(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + mutex_lock(&state->port.mutex);
> +
> + /*
> + * Check value: baud_base has to be more than 9600
> + * and uartclock = baud_base * 16 .
> + */
> + if (val >= 153600)
> + state->uart_port->uartclk = val;
This magic value here would use some documentation.
> + mutex_unlock(&state->port.mutex);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(uartclk, S_IRUGO | S_IWUSR, get_attr_uartclk,
> + set_attr_uartclk);
> +
> /**
> * uart_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> @@ -2355,6 +2395,14 @@ int uart_add_one_port(struct uart_driver *drv,
> struct uart_port *uport) }
>
> /*
> + * Expose uartclk in sysfs. Use driverdata of the tty device for
> + * referencing the UART port.
> + */
> + dev_set_drvdata(tty_dev, state);
> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
> + dev_err(tty_dev, "Failed to add uartclk attr\n");
> +
> + /*
> * Ensure UPF_DEAD is not set.
> */
> uport->flags &= ~UPF_DEAD;
> @@ -2397,6 +2445,12 @@ int uart_remove_one_port(struct uart_driver *drv,
> struct uart_port *uport) mutex_unlock(&port->mutex);
>
> /*
> + * Remove uartclk file from sysfs.
> + */
> + device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
> + &dev_attr_uartclk);
> +
> + /*
> * Remove the devices from the tty layer
> */
> tty_unregister_device(drv->tty_driver, uport->line);
> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
> index b425c79..8ea8622 100644
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver
> *driver, unsigned index) }
> EXPORT_SYMBOL(tty_unregister_device);
>
> +/*
> + * tty_lookup_device - lookup a tty device
> + * @driver: the tty driver that describes the tty device
> + * @index: the index in the tty driver for this tty device
> + *
> + * This function returns a struct device pointer when device has
> + * been found and NULL otherwise.
> + *
> + * Locking: ??
> + */
> +struct device *tty_lookup_device(struct tty_driver *driver, unsigned
> index) +{
> + dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
> + return class_find_device(tty_class, NULL, &devt, dev_match_devt);
> +}
> +EXPORT_SYMBOL(tty_lookup_device);
> +
> struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
> {
> struct tty_driver *driver;
> diff --git a/include/linux/tty.h b/include/linux/tty.h
> index 9f47ab5..5d408a1 100644
> --- a/include/linux/tty.h
> +++ b/include/linux/tty.h
> @@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver
> *driver); extern int tty_unregister_driver(struct tty_driver *driver);
> extern struct device *tty_register_device(struct tty_driver *driver,
> unsigned index, struct device *dev);
> +extern struct device *tty_lookup_device(struct tty_driver *driver,
> + unsigned index);
> extern void tty_unregister_device(struct tty_driver *driver, unsigned
> index); extern int tty_read_raw_data(struct tty_struct *tty, unsigned char
> *bufp, int buflen);
Hello Marek,
On Tue, Aug 14, 2012 at 2:50 PM, Marek Vasut <[email protected]> wrote:
> Dear Tomas Hlavacek,
>
>> +static ssize_t get_attr_uartclk(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + int ret;
>> +
>> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
>
> You don't need this cast here.
Yes, you are right. Thanks.
>
>> + mutex_lock(&state->port.mutex);
>> + ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
>
> Do you really need such a large buffer (PAGE_SIZE) ?
Well no, but I believe that I get the buffer of length equal to
PAGE_SIZE anyway. Documentation/filesystems/sysfs.txt says so on line
179.
>
>> + mutex_unlock(&state->port.mutex);
>> + return ret;
>> +}
>> +
>> +static ssize_t set_attr_uartclk(struct device *dev,
>> + struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> + struct uart_state *state = (struct uart_state *)(dev_get_drvdata(dev));
>> + unsigned int val;
>> + int ret;
>> +
>> + ret = kstrtouint(buf, 10, &val);
>> + if (ret)
>> + return ret;
>> +
>> + mutex_lock(&state->port.mutex);
>> +
>> + /*
>> + * Check value: baud_base has to be more than 9600
>> + * and uartclock = baud_base * 16 .
>> + */
>> + if (val >= 153600)
>> + state->uart_port->uartclk = val;
>
> This magic value here would use some documentation.
OK. Do you think this would be sufficient comment?:
/*
* Check value: baud_base does not make sense to be set below 9600
* and since uartclock = (baud_base * 16) it has to be equal or greater than
* 9600 * 16 = 153600.
*/
PATCHv2 follows.
Tomas
--
Tomáš Hlaváček <[email protected]>
Added file /sys/devices/.../tty/ttySX/uartclk to allow read/modify
uartclk value in struct uart_port in serial_core via sysfs.
It simplifies initialization of no-name cards that have non-standard
oscillator speed while having no distinguishing PCI IDs to allow
autodetection.
Signed-off-by: Tomas Hlavacek <[email protected]>
---
drivers/tty/serial/serial_core.c | 55 ++++++++++++++++++++++++++++++++++++++
drivers/tty/tty_io.c | 17 ++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 74 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..0929fe3 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,47 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
return p->tty_driver;
}
+static ssize_t uart_get_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+
+ struct uart_state *state = dev_get_drvdata(dev);
+ mutex_lock(&state->port.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
+ mutex_unlock(&state->port.mutex);
+ return ret;
+}
+
+static ssize_t uart_set_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, const char *buf, size_t count)
+{
+ struct uart_state *state = dev_get_drvdata(dev);
+ unsigned int val;
+ int ret;
+
+ ret = kstrtouint(buf, 10, &val);
+ if (ret)
+ return ret;
+
+ mutex_lock(&state->port.mutex);
+
+ /*
+ * Check value: baud_base does not make sense to be set below 9600
+ * and since uartclock = (baud_base * 16) it has to be equal or greater
+ * than 9600 * 16 = 153600.
+ */
+ if (val >= 153600)
+ state->uart_port->uartclk = val;
+
+ mutex_unlock(&state->port.mutex);
+
+ return count;
+}
+
+static DEVICE_ATTR(uartclk, S_IRUGO | S_IWUSR, uart_get_attr_uartclk,
+ uart_set_attr_uartclk);
+
/**
* uart_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
@@ -2355,6 +2396,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
}
/*
+ * Expose uartclk in sysfs. Use driverdata of the tty device for
+ * referencing the UART port.
+ */
+ dev_set_drvdata(tty_dev, state);
+ if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
+ dev_err(tty_dev, "Failed to add uartclk attr\n");
+
+ /*
* Ensure UPF_DEAD is not set.
*/
uport->flags &= ~UPF_DEAD;
@@ -2397,6 +2446,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
/*
+ * Remove uartclk file from sysfs.
+ */
+ device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
+ &dev_attr_uartclk);
+
+ /*
* Remove the devices from the tty layer
*/
tty_unregister_device(drv->tty_driver, uport->line);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..8ea8622 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
}
EXPORT_SYMBOL(tty_unregister_device);
+/*
+ * tty_lookup_device - lookup a tty device
+ * @driver: the tty driver that describes the tty device
+ * @index: the index in the tty driver for this tty device
+ *
+ * This function returns a struct device pointer when device has
+ * been found and NULL otherwise.
+ *
+ * Locking: ??
+ */
+struct device *tty_lookup_device(struct tty_driver *driver, unsigned index)
+{
+ dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
+ return class_find_device(tty_class, NULL, &devt, dev_match_devt);
+}
+EXPORT_SYMBOL(tty_lookup_device);
+
struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
{
struct tty_driver *driver;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..5d408a1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver *driver);
extern int tty_unregister_driver(struct tty_driver *driver);
extern struct device *tty_register_device(struct tty_driver *driver,
unsigned index, struct device *dev);
+extern struct device *tty_lookup_device(struct tty_driver *driver,
+ unsigned index);
extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
int buflen);
--
1.7.10.4
> + /*
> + * Check value: baud_base has to be more than 9600
> + * and uartclock = baud_base * 16 .
> + */
> + if (val >= 153600)
> + state->uart_port->uartclk = val;
> +
> + mutex_unlock(&state->port.mutex);
> +
> + return count;
This breaks if for example the port is in use. Fixing that looks pretty
horrible as you need a valid tty pointer to stop and restart the pot.
It's also not calling the verfy method of the port as is expected.
At minimum I think you need to be able to do the same work
uart_get_info/uart_set_info perform and with the same checks on ->verify
etc.
I'm not 100% sure the drvdata on the tty_dev is clear to use. It does
seem to be in all the drivers I looked at. I'd rather however it pointed
to the tty_port that each tty device has (or very soon will be required
to have). You can still find the uart_foo structs from that but it means
we can do the dev_set_drvdata() in a consistent manner for all tty
devices in the kernel. That in turn means we can make some of the sysfs
valid the same way.
I want to have think about the setting side of it. Can you submit a
revised version that just allows the user to read the value this way but
does the drvdata setting etc and sysfs node create/delete.
I'll merge that and we can throw it over the parapet and see if anything
explodes.
To make the setting part work properly I think I need to sort out
uart_get_info/set_info so the core part of it can be called with kernel
space structures and the caller handling locks.
Alan
Added file /sys/devices/.../tty/ttySX/uartclk to allow reading
uartclk value in struct uart_port in serial_core via sysfs.
It simplifies initialization verification of no-name cards that
have non-standard oscillator speed while having no distinguishing
PCI IDs to allow autodetection.
Signed-off-by: Tomas Hlavacek <[email protected]>
---
drivers/tty/serial/serial_core.c | 32 ++++++++++++++++++++++++++++++++
drivers/tty/tty_io.c | 17 +++++++++++++++++
include/linux/tty.h | 2 ++
3 files changed, 51 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..454e9d3 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,24 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
return p->tty_driver;
}
+static ssize_t uart_get_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+
+ struct tty_port *port = dev_get_drvdata(dev);
+ struct uart_state *state = container_of(port, struct uart_state, port);
+
+ mutex_lock(&state->port.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
+ mutex_unlock(&state->port.mutex);
+
+ return ret;
+}
+
+static DEVICE_ATTR(uartclk, S_IRUSR | S_IRGRP, uart_get_attr_uartclk,
+ NULL);
+
/**
* uart_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
@@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
}
/*
+ * Expose uartclk in sysfs. Use driverdata of the tty device for
+ * referencing the UART port.
+ */
+ dev_set_drvdata(tty_dev, port);
+ if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
+ dev_err(tty_dev, "Failed to add uartclk attr\n");
+
+ /*
* Ensure UPF_DEAD is not set.
*/
uport->flags &= ~UPF_DEAD;
@@ -2397,6 +2423,12 @@ int uart_remove_one_port(struct uart_driver *drv, struct uart_port *uport)
mutex_unlock(&port->mutex);
/*
+ * Remove uartclk file from sysfs.
+ */
+ device_remove_file(tty_lookup_device(drv->tty_driver, uport->line),
+ &dev_attr_uartclk);
+
+ /*
* Remove the devices from the tty layer
*/
tty_unregister_device(drv->tty_driver, uport->line);
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..8ea8622 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
}
EXPORT_SYMBOL(tty_unregister_device);
+/*
+ * tty_lookup_device - lookup a tty device
+ * @driver: the tty driver that describes the tty device
+ * @index: the index in the tty driver for this tty device
+ *
+ * This function returns a struct device pointer when device has
+ * been found and NULL otherwise.
+ *
+ * Locking: ??
+ */
+struct device *tty_lookup_device(struct tty_driver *driver, unsigned index)
+{
+ dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
+ return class_find_device(tty_class, NULL, &devt, dev_match_devt);
+}
+EXPORT_SYMBOL(tty_lookup_device);
+
struct tty_driver *__alloc_tty_driver(int lines, struct module *owner)
{
struct tty_driver *driver;
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..5d408a1 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -410,6 +410,8 @@ extern int tty_register_driver(struct tty_driver *driver);
extern int tty_unregister_driver(struct tty_driver *driver);
extern struct device *tty_register_device(struct tty_driver *driver,
unsigned index, struct device *dev);
+extern struct device *tty_lookup_device(struct tty_driver *driver,
+ unsigned index);
extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
int buflen);
--
1.7.10.4
On Fri, Aug 17, 2012 at 04:43:57PM +0200, Tomas Hlavacek wrote:
> Added file /sys/devices/.../tty/ttySX/uartclk to allow reading
> uartclk value in struct uart_port in serial_core via sysfs.
Whenever you add/remove/modify a sysfs file, you also need to add an
update to Documentation/ABI/ as well.
Please add this to the patch and resend.
> It simplifies initialization verification of no-name cards that
> have non-standard oscillator speed while having no distinguishing
> PCI IDs to allow autodetection.
>
> Signed-off-by: Tomas Hlavacek <[email protected]>
> ---
> drivers/tty/serial/serial_core.c | 32 ++++++++++++++++++++++++++++++++
> drivers/tty/tty_io.c | 17 +++++++++++++++++
> include/linux/tty.h | 2 ++
> 3 files changed, 51 insertions(+)
>
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index a21dc8e..454e9d3 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -2293,6 +2293,24 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
> return p->tty_driver;
> }
>
> +static ssize_t uart_get_attr_uartclk(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + int ret;
> +
> + struct tty_port *port = dev_get_drvdata(dev);
> + struct uart_state *state = container_of(port, struct uart_state, port);
> +
> + mutex_lock(&state->port.mutex);
> + ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
> + mutex_unlock(&state->port.mutex);
> +
> + return ret;
> +}
> +
> +static DEVICE_ATTR(uartclk, S_IRUSR | S_IRGRP, uart_get_attr_uartclk,
> + NULL);
> +
> /**
> * uart_add_one_port - attach a driver-defined port structure
> * @drv: pointer to the uart low level driver structure for this port
> @@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> }
>
> /*
> + * Expose uartclk in sysfs. Use driverdata of the tty device for
> + * referencing the UART port.
> + */
> + dev_set_drvdata(tty_dev, port);
> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
> + dev_err(tty_dev, "Failed to add uartclk attr\n");
I think you just raced with userspace in creating the file after the
device was announced to userspace. Are you sure it's ok?
If not (hint, I don't think so), please make it a default attribute of
the device, which will then cause the file to be created before it is
announced to userspace. It will also be less code as you don't have to
clean it up by hand :)
> --- a/drivers/tty/tty_io.c
> +++ b/drivers/tty/tty_io.c
> @@ -3049,6 +3049,23 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
> }
> EXPORT_SYMBOL(tty_unregister_device);
>
> +/*
> + * tty_lookup_device - lookup a tty device
> + * @driver: the tty driver that describes the tty device
> + * @index: the index in the tty driver for this tty device
> + *
> + * This function returns a struct device pointer when device has
> + * been found and NULL otherwise.
> + *
> + * Locking: ??
> + */
> +struct device *tty_lookup_device(struct tty_driver *driver, unsigned index)
> +{
> + dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
> + return class_find_device(tty_class, NULL, &devt, dev_match_devt);
> +}
> +EXPORT_SYMBOL(tty_lookup_device);
EXPORT_SYMBOL_GPL as you are just wrapping a class_find_device() call?
thanks,
greg k-h
Hello Greg!
On Fri, Aug 17, 2012 at 5:06 PM, Greg KH <[email protected]> wrote:
>> @@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
>> }
>>
>> /*
>> + * Expose uartclk in sysfs. Use driverdata of the tty device for
>> + * referencing the UART port.
>> + */
>> + dev_set_drvdata(tty_dev, port);
>> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
>> + dev_err(tty_dev, "Failed to add uartclk attr\n");
>
> I think you just raced with userspace in creating the file after the
> device was announced to userspace. Are you sure it's ok?
>
> If not (hint, I don't think so), please make it a default attribute of
> the device, which will then cause the file to be created before it is
> announced to userspace. It will also be less code as you don't have to
> clean it up by hand :)
Do you mean I should modify the tty_register_device() function not to
use device_create() but it should rather do the device initialization
on it's own. And I should add add the attribute (via struct
attribute_group) to struct device in between device_initialize() and
device_add() calls. Did I get it right?
Thanks,
Tomas
On Fri, Aug 17, 2012 at 06:30:36PM +0200, Tomas Hlavacek wrote:
> Hello Greg!
>
> On Fri, Aug 17, 2012 at 5:06 PM, Greg KH <[email protected]> wrote:
> >> @@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
> >> }
> >>
> >> /*
> >> + * Expose uartclk in sysfs. Use driverdata of the tty device for
> >> + * referencing the UART port.
> >> + */
> >> + dev_set_drvdata(tty_dev, port);
> >> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
> >> + dev_err(tty_dev, "Failed to add uartclk attr\n");
> >
> > I think you just raced with userspace in creating the file after the
> > device was announced to userspace. Are you sure it's ok?
> >
> > If not (hint, I don't think so), please make it a default attribute of
> > the device, which will then cause the file to be created before it is
> > announced to userspace. It will also be less code as you don't have to
> > clean it up by hand :)
>
> Do you mean I should modify the tty_register_device() function not to
> use device_create() but it should rather do the device initialization
> on it's own.
No, not at all.
> And I should add add the attribute (via struct attribute_group) to
> struct device in between device_initialize() and device_add() calls.
> Did I get it right?
No, make this a driver attribute, that way when the device is
registered, it adds the attribute automagically to the device that is
bound to it.
Does that make sense?
greg k-h
Dear Greg KH,
> On Fri, Aug 17, 2012 at 06:30:36PM +0200, Tomas Hlavacek wrote:
> > Hello Greg!
> >
> > On Fri, Aug 17, 2012 at 5:06 PM, Greg KH <[email protected]> wrote:
> > >> @@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv,
> > >> struct uart_port *uport)
> > >>
> > >> }
> > >>
> > >> /*
> > >>
> > >> + * Expose uartclk in sysfs. Use driverdata of the tty device for
> > >> + * referencing the UART port.
> > >> + */
> > >> + dev_set_drvdata(tty_dev, port);
> > >> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
> > >> + dev_err(tty_dev, "Failed to add uartclk attr\n");
> > >
> > > I think you just raced with userspace in creating the file after the
> > > device was announced to userspace. Are you sure it's ok?
> > >
> > > If not (hint, I don't think so), please make it a default attribute of
> > > the device, which will then cause the file to be created before it is
> > > announced to userspace. It will also be less code as you don't have to
> > > clean it up by hand :)
> >
> > Do you mean I should modify the tty_register_device() function not to
> > use device_create() but it should rather do the device initialization
> > on it's own.
>
> No, not at all.
>
> > And I should add add the attribute (via struct attribute_group) to
> > struct device in between device_initialize() and device_add() calls.
> > Did I get it right?
>
> No, make this a driver attribute, that way when the device is
> registered, it adds the attribute automagically to the device that is
> bound to it.
(hint, DEVICE_ATTR), right ?
> Does that make sense?
>
> greg k-h
Best regards,
Marek Vasut
On Fri, Aug 17, 2012 at 08:44:14PM +0200, Marek Vasut wrote:
> Dear Greg KH,
>
> > On Fri, Aug 17, 2012 at 06:30:36PM +0200, Tomas Hlavacek wrote:
> > > Hello Greg!
> > >
> > > On Fri, Aug 17, 2012 at 5:06 PM, Greg KH <[email protected]> wrote:
> > > >> @@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv,
> > > >> struct uart_port *uport)
> > > >>
> > > >> }
> > > >>
> > > >> /*
> > > >>
> > > >> + * Expose uartclk in sysfs. Use driverdata of the tty device for
> > > >> + * referencing the UART port.
> > > >> + */
> > > >> + dev_set_drvdata(tty_dev, port);
> > > >> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
> > > >> + dev_err(tty_dev, "Failed to add uartclk attr\n");
> > > >
> > > > I think you just raced with userspace in creating the file after the
> > > > device was announced to userspace. Are you sure it's ok?
> > > >
> > > > If not (hint, I don't think so), please make it a default attribute of
> > > > the device, which will then cause the file to be created before it is
> > > > announced to userspace. It will also be less code as you don't have to
> > > > clean it up by hand :)
> > >
> > > Do you mean I should modify the tty_register_device() function not to
> > > use device_create() but it should rather do the device initialization
> > > on it's own.
> >
> > No, not at all.
> >
> > > And I should add add the attribute (via struct attribute_group) to
> > > struct device in between device_initialize() and device_add() calls.
> > > Did I get it right?
> >
> > No, make this a driver attribute, that way when the device is
> > registered, it adds the attribute automagically to the device that is
> > bound to it.
>
> (hint, DEVICE_ATTR), right ?
No, that's just a macro that creates the structure for the attribute.
You need to take that structure and tie it to the driver itself, using
the struct device_driver->groups; field.
greg k-h
Hello!
On Fri, Aug 17, 2012 at 9:01 PM, Greg KH <[email protected]> wrote:
> On Fri, Aug 17, 2012 at 08:44:14PM +0200, Marek Vasut wrote:
>> Dear Greg KH,
>>
>> > On Fri, Aug 17, 2012 at 06:30:36PM +0200, Tomas Hlavacek wrote:
>> > > Hello Greg!
>> > >
>> > > On Fri, Aug 17, 2012 at 5:06 PM, Greg KH <[email protected]> wrote:
>> > > >> @@ -2355,6 +2373,14 @@ int uart_add_one_port(struct uart_driver *drv,
>> > > >> struct uart_port *uport)
>> > > >>
>> > > >> }
>> > > >>
>> > > >> /*
>> > > >>
>> > > >> + * Expose uartclk in sysfs. Use driverdata of the tty device for
>> > > >> + * referencing the UART port.
>> > > >> + */
>> > > >> + dev_set_drvdata(tty_dev, port);
>> > > >> + if (device_create_file(tty_dev, &dev_attr_uartclk) < 0)
>> > > >> + dev_err(tty_dev, "Failed to add uartclk attr\n");
>> > > >
>> > > > I think you just raced with userspace in creating the file after the
>> > > > device was announced to userspace. Are you sure it's ok?
>> > > >
>> > > > If not (hint, I don't think so), please make it a default attribute of
>> > > > the device, which will then cause the file to be created before it is
>> > > > announced to userspace. It will also be less code as you don't have to
>> > > > clean it up by hand :)
>> > >
>> > > Do you mean I should modify the tty_register_device() function not to
>> > > use device_create() but it should rather do the device initialization
>> > > on it's own.
>> >
>> > No, not at all.
>> >
>> > > And I should add add the attribute (via struct attribute_group) to
>> > > struct device in between device_initialize() and device_add() calls.
>> > > Did I get it right?
>> >
>> > No, make this a driver attribute, that way when the device is
>> > registered, it adds the attribute automagically to the device that is
>> > bound to it.
>>
>> (hint, DEVICE_ATTR), right ?
>
> No, that's just a macro that creates the structure for the attribute.
> You need to take that structure and tie it to the driver itself, using
> the struct device_driver->groups; field.
Please forgive me my ignorance, but I am lost in this. I tried to read
through the serial_core and tty_io.c over and over to figure out how
the drivers are registered and where could I hook the driver
initialization for all UART ports, but I do not get it.
And there is another thing I am confused of: Should I use macro
DEVICE_ATTR or DRIVER_ATTR? There is a different signature of
callbacks in case of DRIVER_ATTR and I do not know how to find out
from struct device_driver which particular UART port it is associated
to? (Is it indeed associated with one particular port?) Value uartclk
could be, AFAIK, different on each port of the same chip and therefore
I need to know which particular TTY device I want to operate on. More
precisely, I have to get to proper struct uart_state associated with
the particular port.
Thanks,
Tomas
--
Tomáš Hlaváček <[email protected]>
Added file /sys/devices/.../tty/ttySX/uartclk to allow reading
uartclk value in struct uart_port in serial_core via sysfs.
It simplifies initialization verification of no-name cards that
have non-standard oscillator speed while having no distinguishing
PCI IDs to allow autodetection.
tty_register_device() has been generalized and refactored in order
to add support for setting drvdata and attribute_group to the device.
Signed-off-by: Tomas Hlavacek <[email protected]>
---
Documentation/ABI/testing/sysfs-tty | 9 +++++
drivers/tty/serial/serial_core.c | 37 +++++++++++++++++++-
drivers/tty/tty_io.c | 63 ++++++++++++++++++++++++++++++++---
include/linux/tty.h | 4 +++
4 files changed, 108 insertions(+), 5 deletions(-)
diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
index b138b66..b93a174 100644
--- a/Documentation/ABI/testing/sysfs-tty
+++ b/Documentation/ABI/testing/sysfs-tty
@@ -17,3 +17,12 @@ Description:
device, like 'tty1'.
The file supports poll() to detect virtual
console switches.
+
+What: /sys/class/tty/ttyS0/uartclk
+Date: Aug 2012
+Contact: Tomas Hlavacek <[email protected]>
+Description:
+ Shows the current uartclk value associated with the
+ UART port in serial_core, that is bound to TTY like ttyS0.
+ uartclk = 16 * baud_base
+
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..e0c11da 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,38 @@ struct tty_driver *uart_console_device(struct console *co, int *index)
return p->tty_driver;
}
+static ssize_t uart_get_attr_uartclk(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+
+ struct tty_port *port = dev_get_drvdata(dev);
+ struct uart_state *state = container_of(port, struct uart_state, port);
+
+ mutex_lock(&state->port.mutex);
+ ret = snprintf(buf, PAGE_SIZE, "%d\n", state->uart_port->uartclk);
+ mutex_unlock(&state->port.mutex);
+
+ return ret;
+}
+
+static DEVICE_ATTR(uartclk, S_IRUSR | S_IRGRP, uart_get_attr_uartclk,
+ NULL);
+
+static struct attribute *tty_dev_attrs[] = {
+ &dev_attr_uartclk.attr,
+ NULL,
+};
+
+static struct attribute_group tty_dev_attr_group = {
+ .attrs = tty_dev_attrs,
+};
+
+static struct attribute_group *tty_dev_attr_groups[] = {
+ &tty_dev_attr_group,
+ NULL
+};
+
/**
* uart_add_one_port - attach a driver-defined port structure
* @drv: pointer to the uart low level driver structure for this port
@@ -2345,8 +2377,11 @@ int uart_add_one_port(struct uart_driver *drv, struct uart_port *uport)
/*
* Register the port whether it's detected or not. This allows
* setserial to be used to alter this ports parameters.
+ * Use driverdata of the tty device for referencing the UART port.
+ * Set default device attributes to the new device.
*/
- tty_dev = tty_register_device(drv->tty_driver, uport->line, uport->dev);
+ tty_dev = tty_register_device_attr(drv->tty_driver, uport->line,
+ uport->dev, port, tty_dev_attr_groups);
if (likely(!IS_ERR(tty_dev))) {
device_set_wakeup_capable(tty_dev, 1);
} else {
diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index b425c79..1c9d5b5 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3009,12 +3009,42 @@ struct class *tty_class;
*
* Locking: ??
*/
-
struct device *tty_register_device(struct tty_driver *driver, unsigned index,
struct device *device)
{
+ return tty_register_device_attr(driver, index, device, NULL, NULL);
+}
+EXPORT_SYMBOL(tty_register_device);
+
+/**
+ * tty_register_device_attr - register a tty device
+ * @driver: the tty driver that describes the tty device
+ * @index: the index in the tty driver for this tty device
+ * @device: a struct device that is associated with this tty device.
+ * This field is optional, if there is no known struct device
+ * for this tty device it can be set to NULL safely.
+ * @drvdata: Driver data to be set to device (NULL = do not touch).
+ * @attr_grp: Attribute group to be set on device (NULL = do not touch).
+ *
+ * Returns a pointer to the struct device for this tty device
+ * (or ERR_PTR(-EFOO) on error).
+ *
+ * This call is required to be made to register an individual tty device
+ * if the tty driver's flags have the TTY_DRIVER_DYNAMIC_DEV bit set. If
+ * that bit is not set, this function should not be called by a tty
+ * driver.
+ *
+ * Locking: ??
+ */
+struct device *tty_register_device_attr(struct tty_driver *driver,
+ unsigned index, struct device *device,
+ void *drvdata,
+ struct attribute_group **attr_grp)
+{
char name[64];
- dev_t dev = MKDEV(driver->major, driver->minor_start) + index;
+ struct device *dev = NULL;
+ int retval = -ENODEV;
+ dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
if (index >= driver->num) {
printk(KERN_ERR "Attempt to register invalid tty line number "
@@ -3027,9 +3057,34 @@ struct device *tty_register_device(struct tty_driver *driver, unsigned index,
else
tty_line_name(driver, index, name);
- return device_create(tty_class, device, dev, NULL, name);
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ retval = -ENOMEM;
+ goto error;
+ }
+
+ dev->devt = devt;
+ dev->class = tty_class;
+ dev->parent = device;
+ dev_set_name(dev, "%s", name);
+ if (attr_grp)
+ dev->groups = attr_grp;
+ if (drvdata)
+ dev_set_drvdata(dev, drvdata);
+
+ retval = device_register(dev);
+ if (retval)
+ goto error;
+
+ return dev;
+
+error:
+ put_device(dev);
+ return ERR_PTR(retval);
+
}
-EXPORT_SYMBOL(tty_register_device);
+EXPORT_SYMBOL_GPL(tty_register_device_attr);
/**
* tty_unregister_device - unregister a tty device
diff --git a/include/linux/tty.h b/include/linux/tty.h
index 9f47ab5..424777a 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -410,6 +410,10 @@ extern int tty_register_driver(struct tty_driver *driver);
extern int tty_unregister_driver(struct tty_driver *driver);
extern struct device *tty_register_device(struct tty_driver *driver,
unsigned index, struct device *dev);
+struct device *tty_register_device_attr(struct tty_driver *driver,
+ unsigned index, struct device *device,
+ void *drvdata,
+ struct attribute_group **attr_grp);
extern void tty_unregister_device(struct tty_driver *driver, unsigned index);
extern int tty_read_raw_data(struct tty_struct *tty, unsigned char *bufp,
int buflen);
--
1.7.10.4
Hello!
On Sun, Aug 19, 2012 at 8:34 PM, Tomas Hlavacek <[email protected]> wrote:
> Added file /sys/devices/.../tty/ttySX/uartclk to allow reading
> uartclk value in struct uart_port in serial_core via sysfs.
>
> It simplifies initialization verification of no-name cards that
> have non-standard oscillator speed while having no distinguishing
> PCI IDs to allow autodetection.
>
> tty_register_device() has been generalized and refactored in order
> to add support for setting drvdata and attribute_group to the device.
>
I have updated the patch to a new v4 in order to remove the race in
sysfs file creation and add sysfs file description to a Documentation
directory. But still the patch creates the sysfs file separately for
each serial TTY device by assigning attribute_groups to the struct
device and not for the whole driver at once as Greg advised because I
was unable to figure out how to do that (even though I tried pretty
hard). Does it make sense like this? Or do you have any hint for a
better way to do it, please?
Thanks,
Tomas
> I have updated the patch to a new v4 in order to remove the race in
> sysfs file creation and add sysfs file description to a Documentation
> directory. But still the patch creates the sysfs file separately for
> each serial TTY device by assigning attribute_groups to the struct
> device and not for the whole driver at once as Greg advised because I
> was unable to figure out how to do that (even though I tried pretty
> hard). Does it make sense like this? Or do you have any hint for a
> better way to do it, please?
I'm fine with it - dunno about GregKH. Given the way tty 'drivers' and
tty devices fit together I'm not 100% sure it makes sense to do it per
driver. Plus once we have the basis we can fix a lot of the detail
afterwards.
Alan