RFC: exposing uartclk value to sysfs

RFC: exposing uartclk value to sysfs

[Tomas Hlavacek]

Hello!

I have encountered quite a lot PCI cards with many different
oscillator speeds while having no differences in PCI IDs (i.e. vendor
and subsystem IDs were chip-dafault). Namely I own several no-name
cards with OX16PCI954 and various speeds. These cards need different
baud_base (= uartclk*16) settings but it can not be
autodetected/pre-set because there is no way to distinguish them.
Although there is a way to set baud_base via setserial(8) I was
wandering whether it would be possible to expose uartclk in sysfs for
read/write. I have created a following proof-of-concept experimental
patch:

From 12c7e940c978da6cca9a6d51f499e7c5b53ec4b1 Mon Sep 17 00:00:00 2001
From: Tomas Hlavacek <tmshlvck@gmail.com>
Date: Fri, 10 Aug 2012 23:43:17 +0200
Subject: [PATCH 1/1] [RFC] [experimental]: uartclk exposed to sysfs.

TODO:

* Get opinions on the whole idea of exposing uartclk to sysfs.

* Get opinions on getting struct uart_port pointer in
the get_attr_uart routine by changing driver data of the tty device.

* Correct removal of the file (problem: How to get the device
handle?)

* Synchronization.

* Add write routine.

Signed-off-by: Tomas Hlavacek <tmshlvck@gmail.com>
---
 drivers/tty/serial/serial_core.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a21dc8e..2cb6ed5 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,17 @@ 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)
+{
+	struct uart_port *port = (struct uart_port *)(dev_get_drvdata(dev));
+	return snprintf(buf, PAGE_SIZE, "%d\n", port->uartclk);
+}
+
+static DEVICE_ATTR(uartclk, S_IRUGO, 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
@@ -2354,6 +2365,12 @@ int uart_add_one_port(struct uart_driver *drv,
struct uart_port *uport)
 		       uport->line);
 	}

+	dev_set_drvdata(tty_dev, uport);
+	int sret = device_create_file(tty_dev, &dev_attr_uartclk);
+	if (sret < 0)
+		dev_err(tty_dev, "failed to add uartclk attr.\n");
+
 	/*
 	 * Ensure UPF_DEAD is not set.
 	 */
-- 
1.7.10.4

Tomas

-- 
Tomáš Hlaváček <tmshlvck@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: exposing uartclk value to sysfs

[Alan Cox]

> Although there is a way to set baud_base via setserial(8) I was
> wandering whether it would be possible to expose uartclk in sysfs for
> read/write. I have created a following proof-of-concept experimental
> patch:

That would probably be a *lot* more civilised. As a conceptual idea I
think its a good one.

Alan

Re: RFC: exposing uartclk value to sysfs

[Tomas Hlavacek]

On Sat, Aug 11, 2012 at 12:48 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
>> Although there is a way to set baud_base via setserial(8) I was
>> wandering whether it would be possible to expose uartclk in sysfs for
>> read/write. I have created a following proof-of-concept experimental
>> patch:
>
> That would probably be a *lot* more civilised. As a conceptual idea I
> think its a good one.

OK, I will dig into it and I will come up with more elaborate patch next time.

Tomas

[PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs

[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 <tmshlvck@gmail.com>
---
 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

Re: [PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Marek Vasut]

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 <tmshlvck@gmail.com>
> ---
>  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);

Re: [PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

Hello Marek,

On Tue, Aug 14, 2012 at 2:50 PM, Marek Vasut <marek.vasut@gmail.com> 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 <tmshlvck@gmail.com>

[PATCHv2 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

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 <tmshlvck@gmail.com>
---
 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

Re: [PATCH 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Alan Cox]

> +	/*
> +	 * 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

[PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

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 <tmshlvck@gmail.com>
---
 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

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Greg KH]

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 <tmshlvck@gmail.com>
> ---
>  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

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

Hello Greg!

On Fri, Aug 17, 2012 at 5:06 PM, Greg KH <gregkh@linuxfoundation.org> 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

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[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 <gregkh@linuxfoundation.org> 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

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Marek Vasut]

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 <gregkh@linuxfoundation.org> 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

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Greg KH]

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 <gregkh@linuxfoundation.org> 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

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

Hello!

On Fri, Aug 17, 2012 at 9:01 PM, Greg KH <gregkh@linuxfoundation.org> 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 <gregkh@linuxfoundation.org> 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 <tmshlvck@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Alan Cox]

On Fri, 17 Aug 2012 16:43:57 +0200
Tomas Hlavacek <tmshlvck@gmail.com> 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.
> 
> Signed-off-by: Tomas Hlavacek <tmshlvck@gmail.com>

Acked-by: Alan Cox <alan@linux.intel.com>

[PATCHv4 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

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 <tmshlvck@gmail.com>
---
 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 <tmshlvck@gmail.com>
+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

Re: [PATCHv4 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Tomas Hlavacek]

Hello!

On Sun, Aug 19, 2012 at 8:34 PM, Tomas Hlavacek <tmshlvck@gmail.com> 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

Re: [PATCHv4 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Alan Cox]

> 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

Re: [PATCHv4 1/1] [RFC] uartclk from serial_core exposed to sysfs

[Greg KH]

On Sun, Aug 19, 2012 at 08:34:45PM +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.
> 
> 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 <tmshlvck@gmail.com>

This looks good, but it doesn't apply to my tty-next branch in
linux-next, so I can't apply it.

But, when you redo it, here's one tiny thing to change:

> +/**
> + *	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).

No need to mention the NULL thing here, "do not touch" doesn't mean
much to me.

> +	if (attr_grp)
> +		dev->groups = attr_grp;
> +	if (drvdata)
> +		dev_set_drvdata(dev, drvdata);

No need to test for NULL, just set them, it can't really hurt, right?

thanks,

greg k-h

[PATCHv5 1/1] uartclk value from serial_core exposed to sysfs

[Tomas Hlavacek]

Added file /sys/devices/.../tty/ttySX/uartclk to allow reading
uartclk value in struct uart_port in serial_core via sysfs.

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 <tmshlvck@gmail.com>
---
 Documentation/ABI/testing/sysfs-tty |    9 ++++++
 drivers/tty/serial/serial_core.c    |   36 ++++++++++++++++++++-
 drivers/tty/tty_io.c                |   59 +++++++++++++++++++++++++++++++++--
 include/linux/tty.h                 |    4 +++
 4 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
index b138b66..2f10855 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:		Sep 2012
+Contact:	Tomas Hlavacek <tmshlvck@gmail.com>
+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..ca64a93 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2293,6 +2293,37 @@ 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 const 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 +2376,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..befa99c 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3013,8 +3013,39 @@ struct class *tty_class;
 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.
+ *	@attr_grp: Attribute group to be set on device.
+ *
+ *	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,
+				   const 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 +3058,31 @@ 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);
+	dev->groups = attr_grp;
+	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..664252b 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);
+extern struct device *tty_register_device_attr(struct tty_driver *driver,
+				unsigned index, struct device *device,
+				void *drvdata,
+				const 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

Re: [PATCHv5 1/1] uartclk value from serial_core exposed to sysfs

[Greg KH]

On Thu, Sep 06, 2012 at 01:16:56AM +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.
> 
> 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 <tmshlvck@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-tty |    9 ++++++
>  drivers/tty/serial/serial_core.c    |   36 ++++++++++++++++++++-
>  drivers/tty/tty_io.c                |   59 +++++++++++++++++++++++++++++++++--
>  include/linux/tty.h                 |    4 +++
>  4 files changed, 104 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
> index b138b66..2f10855 100644

Odd, what tree did you make this against?  It doesn't apply at all, even
if I mess with it to try a --3way merge, it still throws up a ton of
conflicts.

Care to do it against my tty-next branch in git?

thanks,

greg k-h

Re: [PATCHv5 1/1] uartclk value from serial_core exposed to sysfs

[Tomas Hlavacek]

Hello Greg,

On Thu, Sep 6, 2012 at 1:42 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Thu, Sep 06, 2012 at 01:16:56AM +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.
>>
>> 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 <tmshlvck@gmail.com>
>> ---
>>  Documentation/ABI/testing/sysfs-tty |    9 ++++++
>>  drivers/tty/serial/serial_core.c    |   36 ++++++++++++++++++++-
>>  drivers/tty/tty_io.c                |   59 +++++++++++++++++++++++++++++++++--
>>  include/linux/tty.h                 |    4 +++
>>  4 files changed, 104 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
>> index b138b66..2f10855 100644
>
> Odd, what tree did you make this against?  It doesn't apply at all, even
> if I mess with it to try a --3way merge, it still throws up a ton of
> conflicts.
>
> Care to do it against my tty-next branch in git?

I did it wrong obviously. Sorry. I am resending it (it is going to be
against tty-next branch in
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git, I hope
it is right this time).

Tomas

[PATCH v6] uartclk value from serial_core exposed to sysfs

[Tomas Hlavacek]

Added file /sys/devices/.../tty/ttySX/uartclk to allow reading
uartclk value in struct uart_port in serial_core via sysfs.

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 <tmshlvck@gmail.com>
---
 Documentation/ABI/testing/sysfs-tty |    9 +++++
 drivers/tty/serial/serial_core.c    |   34 ++++++++++++++++-
 drivers/tty/tty_io.c                |   69 ++++++++++++++++++++++++++++++-----
 include/linux/tty.h                 |    4 ++
 4 files changed, 104 insertions(+), 12 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-tty b/Documentation/ABI/testing/sysfs-tty
index b138b66..0c43015 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:		Sep 2012
+Contact:	Tomas Hlavacek <tmshlvck@gmail.com>
+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 137b25c..5d212f8 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -2309,6 +2309,36 @@ 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 const 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
@@ -2362,8 +2392,8 @@ 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.
 	 */
-	tty_dev = tty_port_register_device(port, 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 d3bf91a..dcb30d5 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -3041,9 +3041,39 @@ static int tty_cdev_add(struct tty_driver *driver, dev_t dev,
 struct device *tty_register_device(struct tty_driver *driver, unsigned index,
 				   struct device *device)
 {
-	struct device *ret;
+	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.
+ *	@attr_grp: Attribute group to be set on device.
+ *
+ *	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,
+				   const struct attribute_group **attr_grp)
+{
 	char name[64];
-	dev_t dev = MKDEV(driver->major, driver->minor_start) + index;
+	dev_t devt = MKDEV(driver->major, driver->minor_start) + index;
+	struct device *dev = NULL;
+	int retval = -ENODEV;
 	bool cdev = false;
 
 	if (index >= driver->num) {
@@ -3058,19 +3088,38 @@ struct device *tty_register_device(struct tty_driver *driver, unsigned index,
 		tty_line_name(driver, index, name);
 
 	if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
-		int error = tty_cdev_add(driver, dev, index, 1);
-		if (error)
-			return ERR_PTR(error);
+		retval = tty_cdev_add(driver, devt, index, 1);
+		if (retval)
+			goto error;
 		cdev = true;
 	}
 
-	ret = device_create(tty_class, device, dev, NULL, name);
-	if (IS_ERR(ret) && cdev)
-		cdev_del(&driver->cdevs[index]);
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev) {
+		retval = -ENOMEM;
+		goto error;
+	}
 
-	return ret;
+	dev->devt = devt;
+	dev->class = tty_class;
+	dev->parent = device;
+	dev_set_name(dev, "%s", name);
+	dev->groups = attr_grp;
+	dev_set_drvdata(dev, drvdata);
+
+	retval = device_register(dev);
+	if (retval)
+		goto error;
+
+	return dev;
+
+error:
+	put_device(dev);
+	if (cdev)
+		cdev_del(&driver->cdevs[index]);
+	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 9892121..599d603 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -412,6 +412,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);
+extern struct device *tty_register_device_attr(struct tty_driver *driver,
+				unsigned index, struct device *device,
+				void *drvdata,
+				const 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

Re: [PATCH v6] tty: uartclk value from serial_core exposed to sysfs

[Greg KH]

On Thu, Sep 06, 2012 at 03:17:18AM +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.
> 
> 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 <tmshlvck@gmail.com>

Thanks, that worked, I've now applied this.

greg k-h

Re: [PATCH v6] uartclk value from serial_core exposed to sysfs

[Jiri Slaby]

On 09/06/2012 03:17 AM, Tomas Hlavacek wrote:
> @@ -2362,8 +2392,8 @@ 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.
>  	 */
> -	tty_dev = tty_port_register_device(port, 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);

This makes me believe you have not tested the change at all?

-- 
js
suse labs

Re: [PATCH v6] uartclk value from serial_core exposed to sysfs

[Tomas Hlavacek]

Hi Jiri,

On Thu, Sep 6, 2012 at 7:54 PM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 09/06/2012 03:17 AM, Tomas Hlavacek wrote:
>> @@ -2362,8 +2392,8 @@ 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.
>>        */
>> -     tty_dev = tty_port_register_device(port, 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);
>
> This makes me believe you have not tested the change at all?

Thanks! I can't believe I missed that. (And I actually tested that,
but I have to admit that it was not enough apparently.)

I will re-send the patch (after some additional testing and double-checking).

Tomas

Re: [PATCH v6] uartclk value from serial_core exposed to sysfs

[Jiri Slaby]

On 09/06/2012 08:39 PM, Tomas Hlavacek wrote:
> On Thu, Sep 6, 2012 at 7:54 PM, Jiri Slaby <jslaby@suse.cz> wrote:
>> On 09/06/2012 03:17 AM, Tomas Hlavacek wrote:
>>> @@ -2362,8 +2392,8 @@ 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.
>>>        */
>>> -     tty_dev = tty_port_register_device(port, 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);
>>
>> This makes me believe you have not tested the change at all?
> 
> Thanks! I can't believe I missed that. (And I actually tested that,
> but I have to admit that it was not enough apparently.)
> 
> I will re-send the patch (after some additional testing and double-checking).

Ok. A couple more questions...

* why are you passing tty_port to the struct device's private data and
not uart_port proper? Is this for some future use?
* cannot be all those attribute structs const?
* kdoc for tty_register_device_attr says that when
TTY_DRIVER_DYNAMIC_DEV is not set, tty_register_device_attr *should* not
be called. But it must not be called, otherwise it will fail and emit a
warning as a bonus, right?
* final remark. I would prefer declaration and code be delimited by a
new line in uart_get_attr_uartclk:
<===>
+       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);
<===>

Like:
<===>
  struct tty_port *port = ...;
  struct uart_state *state = ...;
  int ret;

  mutex_lock(&state->port.mutex);
<===>

regards,
-- 
js
suse labs

Re: [PATCH v6] uartclk value from serial_core exposed to sysfs

[Tomas Hlavacek]

Hello!

On Thu, Sep 6, 2012 at 8:54 PM, Jiri Slaby <jslaby@suse.cz> wrote:
> On 09/06/2012 08:39 PM, Tomas Hlavacek wrote:
>> On Thu, Sep 6, 2012 at 7:54 PM, Jiri Slaby <jslaby@suse.cz> wrote:
>>> On 09/06/2012 03:17 AM, Tomas Hlavacek wrote:
>>>> @@ -2362,8 +2392,8 @@ 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.
>>>>        */
>>>> -     tty_dev = tty_port_register_device(port, 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);
>>>
>>> This makes me believe you have not tested the change at all?
>>
>> Thanks! I can't believe I missed that. (And I actually tested that,
>> but I have to admit that it was not enough apparently.)
>>
>> I will re-send the patch (after some additional testing and double-checking).
>
> Ok. A couple more questions...
>
> * why are you passing tty_port to the struct device's private data and
> not uart_port proper? Is this for some future use?

I actually used the uart_port structure in older RFC versions of the
patch. Alan Cox advised to use struct tty_port because of consistency.
More precisely he said (in an e-mail from Aug 12):

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.

> * cannot be all those attribute structs const?

It seems that making

static const struct attribute *tty_dev_attrs[] = ...

produces warning:

drivers/tty/serial/serial_core.c:2334:2: warning: initialization from
incompatible pointer type [enabled by default]
drivers/tty/serial/serial_core.c:2334:2: warning: (near initialization
for ‘tty_dev_attr_group.attrs’) [enabled by default]

But others can. I am going to make them const in following patch.

> * kdoc for tty_register_device_attr says that when
> TTY_DRIVER_DYNAMIC_DEV is not set, tty_register_device_attr *should* not
> be called. But it must not be called, otherwise it will fail and emit a
> warning as a bonus, right?

Yes. In fact it does the same thing as tty_register_device() did
before itself. The _attr version is only slightly refactored and the
doc regarding the TTY_DRIVER_DYNAMIC_DEV test is the same as in old
non-_attr version.
I am not sure that I am the right person to change the doc because I
am not an author of this part of doc nor of the test in the function.

> * final remark. I would prefer declaration and code be delimited by a
> new line in uart_get_attr_uartclk:
> <===>
> +       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);
> <===>
>
> Like:
> <===>
>   struct tty_port *port = ...;
>   struct uart_state *state = ...;
>   int ret;
>
>   mutex_lock(&state->port.mutex);
> <===>

Yes, it looks better. I am going to it accordingly.

Tomas
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] uartclk value from serial_core exposed to sysfs

[Jiri Slaby]

On 09/06/2012 09:41 PM, Tomas Hlavacek wrote:
>> * why are you passing tty_port to the struct device's private data and
>> not uart_port proper? Is this for some future use?
> 
> I actually used the uart_port structure in older RFC versions of the
> patch. Alan Cox advised to use struct tty_port because of consistency.
> More precisely he said (in an e-mail from Aug 12):
> 
> I'd rather however it pointed
> to the tty_port that each tty device has (or very soon will be required
> to have).

Yes, this is already a requirement in tty-next. And every driver has to
provide us with the tty_port as you might persuaded yourself of that
already ;).

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

Ah, OK, makes sense.

>> * cannot be all those attribute structs const?
> 
> It seems that making
> 
> static const struct attribute *tty_dev_attrs[] = ...
> 
> produces warning:
> 
> drivers/tty/serial/serial_core.c:2334:2: warning: initialization from
> incompatible pointer type [enabled by default]
> drivers/tty/serial/serial_core.c:2334:2: warning: (near initialization
> for ‘tty_dev_attr_group.attrs’) [enabled by default]
> 
> But others can. I am going to make them const in following patch.

Ok, it was only a suggestion. I did not know either.

regards,
-- 
js
suse labs
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html