Message-Id: <C73186C5-51E8-4EFB-B8FB-520C0A9B0D13@goldelico.com>
Date:	Wed, 1 Jul 2015 21:07:38 +0200
From:	"Dr. H. Nikolaus Schaller" <hns@...delico.com>
To:	Rob Herring <robherring2@...il.com>
Cc:	Marek Belisko <marek@...delico.com>, Jiri Slaby <jslaby@...e.cz>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Arnd Bergmann <arnd@...db.de>,
	Peter Hurley <peter@...leysoftware.com>,
	Mark Rutland <mark.rutland@....com>,
	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>,
	Sebastian Reichel <sre@...nel.org>,
	Neil Brown <neil@...wn.name>,
	Grant Likely <grant.likely@...aro.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>
Subject: Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle

Hi,

Am 01.07.2015 um 20:16 schrieb Rob Herring <robherring2@...il.com>:

> On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko <marek@...delico.com> wrote:
>> From: "H. Nikolaus Schaller" <hns@...delico.com>
>> 
>> 1. add registered uart_ports to a search list
>> 2. provide a function to search an uart_port by phandle. This copies the
>>   mechanism how devm_usb_get_phy_by_phandle() works
> 
> How does this relate to Neil's tty/uart slaves series?

we had questioned the approach of interpreting uarts as a degenerated single-client
bus and therefore handling uart slave devices by subnodes.

Rather than having a „UART n is connected to device“ subnode, we think that a
„this device is connected to UART n“ phandle is the most elegant and flexible
way to implement it. Like it is for PHY relations within the USB subsystem.

After a long theoretical discussion we were asked to submit code to better allow
to compare both approaches.

Here it is.

So common is the problem that we want to solve. Different is the solution.

Both implementations are tested to work on the GTA04 hardware.

> 
>> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
>> Signed-off-by: Marek Belisko <marek@...delico.com>
>> ---
>> Documentation/serial/slaves.txt  |  36 ++++++++++++++
> 
> You need to split this into DT binding and whatever kernel specific
> documentation you want. My comment below apply to what goes in the
> binding doc.

This driver does not define any specific bindings for the slave device, therefore
we have not provided a bindings document. It is up to the slave driver to provide
one.

This driver just provides means that a slave driver can interact with the tty/serial
driver.

Bindings for a specific slave device are defined in our
[PATCH RFC v2 3/3] misc: Add w2g0004 gps receiver driver
../devicetree/bindings/misc/wi2wi,w2sg0004.txt

> 
>> drivers/tty/serial/serial_core.c | 103 +++++++++++++++++++++++++++++++++++++++
>> include/linux/serial_core.h      |  10 ++++
>> 3 files changed, 149 insertions(+)
>> create mode 100644 Documentation/serial/slaves.txt
>> 
>> diff --git a/Documentation/serial/slaves.txt b/Documentation/serial/slaves.txt
>> new file mode 100644
>> index 0000000..6f8d44d
>> --- /dev/null
>> +++ b/Documentation/serial/slaves.txt
>> @@ -0,0 +1,36 @@
>> +UART slave device support
>> +
>> +A remote device connected to a RS232 interface is usually power controlled by the DTR line.
>> +The DTR line is managed automatically by the UART driver for open() and close() syscalls
>> +and on demand by tcsetattr().
>> +
>> +With embedded devices, the serial peripheral might be directly and always connected to the UART
>> +and there might be no physical DTR line involved. Power control (on/off) has to be done by some
>> +chip specific device driver (which we call "UART slave") through some mechanisms (I2C, GPIOs etc.)
>> +not related to the serial interface. Some devices do not explicitly tell their power state except
>> +by sending or not sending data to the UART. In such a case the device driver must be able to monitor
>> +data activity. The role of the device driver is to encapsulate such power control in a single place.
>> +
>> +This patch series allows to support such drivers by providing:
>> +* a mechanism that a slave driver can identify the UART instance it is connected to
>> +* a mechanism that UART slave drivers can register to be notified
>> +* notfications for DTR (and other modem control) state changes
> 
> This has nothing to do with the binding really, but is rather a Linux
> driver feature.

Yes, we want to describe the linux driver features here.

> 
>> +* notifications that the UART has received some data from the UART
>> +
>> +A slave device simply adds a phandle reference to the UART it is connected to, e.g.
> 
> By default I think this should be a sub-node of the uart.

We want to show with this implementation that uart-subnodes would be the wrong
way to go.

> There are
> more complicated cases of combo devices which we may need to support
> with phandles, but by default we should use sub-nodes to describe
> connections.

There can be serial devices which are power-controlled by I2C and then
we would have a problem to describe the connection as both I2C and UART
subnodes.

For us the "main interface“ of a chip is the one that allows to turn the device
on and off. And not the one that carries data provided by/to user space („payload“).

This is rarely the serial data stream. Hence describing the subdevice by a
subnode of the uart is describing something different from what we need.

> 
>> +
>> +       gps {
>> +               compatible = "wi2wi,w2sg0004";
>> +               uart = <&uart1>;
> 
> What if you have a device with 2 uart connections? Do you have 2 items
> in the list or do multiple "<name>-uart" entries? The former is
> probably sufficient and easier to parse.

It is up to the subdevice driver how it is implemented. So view this as an example
that has only one uart connection.

For two uart connections you can choose either of your proposals since the translator
function has both a phandle node name and an index.

> devm_serial_get_uart_by_phandle(struct device *dev,
> +		const char *phandle, u8 index)


You can call this function twice as:

> uart1 = devm_serial_get_uart_by_phandle(dev, „uart“, 0)
> uart2 = devm_serial_get_uart_by_phandle(dev, „uart“, 1)

or 

> uart1 = devm_serial_get_uart_by_phandle(dev, „uart1“, 0)
> uart2 = devm_serial_get_uart_by_phandle(dev, „uart2“, 0)

whatever better fits. But this is up to the device driver where it IMHO belongs to.

Our serial core driver does not predefine anything. It just provides a mechanism
that a driver can translate phandles into struct uart.

What we should do is to mark this DT fragment in this driver documentation as
an example to avoid misinterpretations.

> 
>> +       };
>> +
>> +The slave driver calls devm_serial_get_uart_by_phandle() to identify the uart driver.
>> +This API follows the concept of devm_usb_get_phy_by_phandle().
>> +
>> +A slave device driver registers itself with serial_register_slave() to receive notifications.
>> +Notification handler callbacks can be registered by serial_register_mctrl_notification() and
>> +serial_register_rx_notification(). If an UART has registered a NULL slave or a NULL handler,
>> +no notifications are sent.
>> +
>> +RX notification handlers can define a ktermios during setup and the handler function can modify
>> +or decide to throw away each character that is passed upwards.
> 
> All these 3 paragraphs should not be in the binding doc.

Maybe you have compared too much with Neil’s implementation.

This is not a binding doc, but a description/documentation of our proposed extension
of the serial core driver.

BR and thanks,
Nikolaus

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists