Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
On Wed, Jul 1, 2015 at 2:07 PM, Dr. H. Nikolaus Schaller wrote: > Hi, > > Am 01.07.2015 um 20:16 schrieb Rob Herring : > >> On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko wrote: >>> From: "H. Nikolaus Schaller" >>> >>> 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. PHYs are typically a bit different in that they already have parent/child relationship with MMIO registers which is the primary hierarchy in DT. There's no reason we can't support both cases, but I'm not yet convinced we have a need yet for both. The parent/child approach is simpler to deal with probe ordering issues as the children can be enumerated when the parent has completed enumeration. We should use the DT hierarchy when possible rather than just sticking everything at the top level. > After a long theoretical discussion we were asked to submit code to better > allow > to compare both approaches. What discussion? I don't recall seeing anything related to the binding part. I've not looked closely at the kernel implementations which I guess are where the major differences are. It would be helpful to have some summary comparing the options. The only clue I had that you had looked at Neil's version is he is cc'ed here. > > 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 >>> Signed-off-by: Marek Belisko >>> --- >>> 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. Then where is the property "uart" documented? That is a specific binding like clock or gpio bindings. Bindings for uart slaves is something that should be defined generically. That is not something that should be or has any benefit of being defined per device. > 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 Right, and that needs to refer to a common binding doc. Simply put, I will reject describing any UART devices in DT until that happens. >>> 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 000..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
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
On Wed, Jul 1, 2015 at 2:07 PM, Dr. H. Nikolaus Schaller h...@goldelico.com wrote: Hi, Am 01.07.2015 um 20:16 schrieb Rob Herring robherri...@gmail.com: On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko ma...@goldelico.com wrote: From: H. Nikolaus Schaller h...@goldelico.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. PHYs are typically a bit different in that they already have parent/child relationship with MMIO registers which is the primary hierarchy in DT. There's no reason we can't support both cases, but I'm not yet convinced we have a need yet for both. The parent/child approach is simpler to deal with probe ordering issues as the children can be enumerated when the parent has completed enumeration. We should use the DT hierarchy when possible rather than just sticking everything at the top level. After a long theoretical discussion we were asked to submit code to better allow to compare both approaches. What discussion? I don't recall seeing anything related to the binding part. I've not looked closely at the kernel implementations which I guess are where the major differences are. It would be helpful to have some summary comparing the options. The only clue I had that you had looked at Neil's version is he is cc'ed here. 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 h...@goldelico.com Signed-off-by: Marek Belisko ma...@goldelico.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. Then where is the property uart documented? That is a specific binding like clock or gpio bindings. Bindings for uart slaves is something that should be defined generically. That is not something that should be or has any benefit of being defined per device. 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 Right, and that needs to refer to a common binding doc. Simply put, I will reject describing any UART devices in DT until that happens. 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 000..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
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 :
> On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko wrote:
>> From: "H. Nikolaus Schaller"
>>
>> 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
>> Signed-off-by: Marek Belisko
>> ---
>> 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 000..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 = <>;
>
> What if you have a device with 2 uart connections? Do you have 2 items
> in the list or do multiple "-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
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko wrote:
> From: "H. Nikolaus Schaller"
>
> 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?
> Signed-off-by: H. Nikolaus Schaller
> Signed-off-by: Marek Belisko
> ---
> 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.
> 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 000..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.
> +* 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. 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.
> +
> + gps {
> + compatible = "wi2wi,w2sg0004";
> + uart = <>;
What if you have a device with 2 uart connections? Do you have 2 items
in the list or do multiple "-uart" entries? The former is
probably sufficient and easier to parse.
> + };
> +
> +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.
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko ma...@goldelico.com wrote:
From: H. Nikolaus Schaller h...@goldelico.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?
Signed-off-by: H. Nikolaus Schaller h...@goldelico.com
Signed-off-by: Marek Belisko ma...@goldelico.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.
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 000..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.
+* 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. 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.
+
+ 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.
+ };
+
+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.
Rob
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 robherri...@gmail.com:
On Sun, Jun 28, 2015 at 2:46 PM, Marek Belisko ma...@goldelico.com wrote:
From: H. Nikolaus Schaller h...@goldelico.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 h...@goldelico.com
Signed-off-by: Marek Belisko ma...@goldelico.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 000..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
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
Hi,
On Mon, Jun 29, 2015 at 06:44:23PM +0200, Dr. H. Nikolaus Schaller wrote:
[...]
> >> + list_for_each_entry(uart, _list, head) {
> >> + if (node != uart->dev->of_node)
> >> + continue;
> >> +
> >> + return uart;
> >
> > We can easily save three lines here :)
>
> Hm. We have copied from here:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
>
> So please let us know how you want to save 3 lines.
Sorry for not being specific, I just meant that it could be written as
if (node == uart->dev->of_node)
return uart;
> >> +/**
> >> + * devm_serial_get_uart_by_phandle - find the uart by phandle
> >> + * @dev - device that requests this uart
> >> + * @phandle - name of the property holding the uart phandle value
> >> + * @index - the index of the uart
> >> + *
> >> + * Returns the uart_port associated with the given phandle value,
> >> + * after getting a refcount to it, -ENODEV if there is no such uart or
> >> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
> >> + * not yet loaded. While at that, it also associates the device with
> >> + * the uart using devres. On driver detach, release function is invoked
> >> + * on the devres data, then, devres data is freed.
> >
> > Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
>
> Well, if the device is not loaded it means the caller must return
> -EPROBE_DEFER
> anyways since it can’t complete it’s probe function.
That was my mistake, from the first sight I hadn't found where
-EPROBE_DEFER is actually returned from the code and thus decided that
kernel-doc has an error. But now I see it, thanks.
> >> +
> >> + *ptr = uart;
> >> + devres_add(dev, ptr);
> >
> > What is the point of assigning value to *ptr?
>
> Good question. I think it is necessary to store a copy of the found uart/phy
> instead of a reference.
> Therefore the assignment to *ptr copies into the new memory area allocated
> above by
>
> ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
>
> This makes the dev the owner of the data - instead of unknown ownership
> before.
>
> It is the same as here (but it might be wrong/unnecessary there as well):
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n209
>
> Maybe it has something to do with the unfinished devm_serial_uart_release().
Indeed. I haven't noticed this through the first quick look into the
code. Thank you for explanation.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
Hi,
On Mon, Jun 29, 2015 at 06:44:23PM +0200, Dr. H. Nikolaus Schaller wrote:
[...]
+ list_for_each_entry(uart, uart_list, head) {
+ if (node != uart-dev-of_node)
+ continue;
+
+ return uart;
We can easily save three lines here :)
Hm. We have copied from here:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
So please let us know how you want to save 3 lines.
Sorry for not being specific, I just meant that it could be written as
if (node == uart-dev-of_node)
return uart;
+/**
+ * devm_serial_get_uart_by_phandle - find the uart by phandle
+ * @dev - device that requests this uart
+ * @phandle - name of the property holding the uart phandle value
+ * @index - the index of the uart
+ *
+ * Returns the uart_port associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such uart or
+ * -EPROBE_DEFER if there is a phandle to the uart, but the device is
+ * not yet loaded. While at that, it also associates the device with
+ * the uart using devres. On driver detach, release function is invoked
+ * on the devres data, then, devres data is freed.
Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
Well, if the device is not loaded it means the caller must return
-EPROBE_DEFER
anyways since it can’t complete it’s probe function.
That was my mistake, from the first sight I hadn't found where
-EPROBE_DEFER is actually returned from the code and thus decided that
kernel-doc has an error. But now I see it, thanks.
+
+ *ptr = uart;
+ devres_add(dev, ptr);
What is the point of assigning value to *ptr?
Good question. I think it is necessary to store a copy of the found uart/phy
instead of a reference.
Therefore the assignment to *ptr copies into the new memory area allocated
above by
ptr = devres_alloc(devm_serial_uart_release, sizeof(*ptr), GFP_KERNEL);
This makes the dev the owner of the data - instead of unknown ownership
before.
It is the same as here (but it might be wrong/unnecessary there as well):
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n209
Maybe it has something to do with the unfinished devm_serial_uart_release().
Indeed. I haven't noticed this through the first quick look into the
code. Thank you for explanation.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
Hi,
thanks for the comments.
Am 28.06.2015 um 23:34 schrieb Sergei Zviagintsev :
> Hi,
>
> Some comments below.
>
> On Sun, Jun 28, 2015 at 09:46:24PM +0200, Marek Belisko wrote:
>> From: "H. Nikolaus Schaller"
>>
>> 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
>>
>> Signed-off-by: H. Nikolaus Schaller
>> Signed-off-by: Marek Belisko
>> ---
>> Documentation/serial/slaves.txt | 36 ++
>> 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 000..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
>> +* 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.
>> +
>> +gps {
>> +compatible = "wi2wi,w2sg0004";
>> +uart = <>;
>> +};
>> +
>> +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.
>> diff --git a/drivers/tty/serial/serial_core.c
>> b/drivers/tty/serial/serial_core.c
>> index eec067d..ad61441 100644
>> --- a/drivers/tty/serial/serial_core.c
>> +++ b/drivers/tty/serial/serial_core.c
>> @@ -38,6 +38,33 @@
>> #include
>> #include
>>
>> +static LIST_HEAD(uart_list);
>> +static DEFINE_SPINLOCK(uart_lock);
>> +
>> +/* same concept as __of_usb_find_phy */
>> +static struct uart_port *__of_serial_find_uart(struct device_node *node)
>> +{
>> +struct uart_port *uart;
>> +
>> +if (!of_device_is_available(node))
>> +return ERR_PTR(-ENODEV);
>> +
>> +list_for_each_entry(uart, _list, head) {
>> +if (node != uart->dev->of_node)
>> +continue;
>> +
>> +return uart;
>
> We can easily save three lines here :)
Hm. We have copied from here:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
So please let us know how you want to save 3 lines.
>
>> +}
>> +
>> +return ERR_PTR(-EPROBE_DEFER);
>> +}
>> +
>> +static void devm_serial_uart_release(struct device *dev, void *res)
>> +{
>> +struct uart_port *uart = *(struct uart_port **)res;
>> +/* FIXME: serial_put_uart(uart); */
>> +}
>
> Looks unfinished…
Yes indeed. That is why we submit it as a RFC. Maybe someone can give us a
comment
how memory management of uart nodes works.
>
>> +
>> /*
>> * This is used to lock changes in serial line configuration.
>> */
>> @@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
>> return !!(uport->status & UPSTAT_DCD_ENABLE);
>> }
>>
>> +/**
>> + * devm_serial_get_uart_by_phandle - find the uart by phandle
>> + * @dev - device that requests this uart
>> + * @phandle - name of the property holding the uart phandle value
>> + * @index - the index
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
Hi,
thanks for the comments.
Am 28.06.2015 um 23:34 schrieb Sergei Zviagintsev ser...@s15v.net:
Hi,
Some comments below.
On Sun, Jun 28, 2015 at 09:46:24PM +0200, Marek Belisko wrote:
From: H. Nikolaus Schaller h...@goldelico.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
Signed-off-by: H. Nikolaus Schaller h...@goldelico.com
Signed-off-by: Marek Belisko ma...@goldelico.com
---
Documentation/serial/slaves.txt | 36 ++
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 000..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
+* 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.
+
+gps {
+compatible = wi2wi,w2sg0004;
+uart = uart1;
+};
+
+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.
diff --git a/drivers/tty/serial/serial_core.c
b/drivers/tty/serial/serial_core.c
index eec067d..ad61441 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -38,6 +38,33 @@
#include asm/irq.h
#include asm/uaccess.h
+static LIST_HEAD(uart_list);
+static DEFINE_SPINLOCK(uart_lock);
+
+/* same concept as __of_usb_find_phy */
+static struct uart_port *__of_serial_find_uart(struct device_node *node)
+{
+struct uart_port *uart;
+
+if (!of_device_is_available(node))
+return ERR_PTR(-ENODEV);
+
+list_for_each_entry(uart, uart_list, head) {
+if (node != uart-dev-of_node)
+continue;
+
+return uart;
We can easily save three lines here :)
Hm. We have copied from here:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/phy/phy.c?id=refs/tags/v4.1#n65
So please let us know how you want to save 3 lines.
+}
+
+return ERR_PTR(-EPROBE_DEFER);
+}
+
+static void devm_serial_uart_release(struct device *dev, void *res)
+{
+struct uart_port *uart = *(struct uart_port **)res;
+/* FIXME: serial_put_uart(uart); */
+}
Looks unfinished…
Yes indeed. That is why we submit it as a RFC. Maybe someone can give us a
comment
how memory management of uart nodes works.
+
/*
* This is used to lock changes in serial line configuration.
*/
@@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
return !!(uport-status UPSTAT_DCD_ENABLE);
}
+/**
+ * devm_serial_get_uart_by_phandle - find the uart by phandle
+ * @dev - device that requests this uart
+ * @phandle - name of the property holding the uart phandle value
+ * @index - the index of the uart
+ *
+ * Returns the uart_port associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such uart or
+ *
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
Hi,
Some comments below.
On Sun, Jun 28, 2015 at 09:46:24PM +0200, Marek Belisko wrote:
> From: "H. Nikolaus Schaller"
>
> 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
>
> Signed-off-by: H. Nikolaus Schaller
> Signed-off-by: Marek Belisko
> ---
> Documentation/serial/slaves.txt | 36 ++
> 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 000..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
> +* 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.
> +
> + gps {
> + compatible = "wi2wi,w2sg0004";
> + uart = <>;
> + };
> +
> +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.
> diff --git a/drivers/tty/serial/serial_core.c
> b/drivers/tty/serial/serial_core.c
> index eec067d..ad61441 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -38,6 +38,33 @@
> #include
> #include
>
> +static LIST_HEAD(uart_list);
> +static DEFINE_SPINLOCK(uart_lock);
> +
> +/* same concept as __of_usb_find_phy */
> +static struct uart_port *__of_serial_find_uart(struct device_node *node)
> +{
> + struct uart_port *uart;
> +
> + if (!of_device_is_available(node))
> + return ERR_PTR(-ENODEV);
> +
> + list_for_each_entry(uart, _list, head) {
> + if (node != uart->dev->of_node)
> + continue;
> +
> + return uart;
We can easily save three lines here :)
> + }
> +
> + return ERR_PTR(-EPROBE_DEFER);
> +}
> +
> +static void devm_serial_uart_release(struct device *dev, void *res)
> +{
> + struct uart_port *uart = *(struct uart_port **)res;
> + /* FIXME: serial_put_uart(uart); */
> +}
Looks unfinished...
> +
> /*
> * This is used to lock changes in serial line configuration.
> */
> @@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
> return !!(uport->status & UPSTAT_DCD_ENABLE);
> }
>
> +/**
> + * devm_serial_get_uart_by_phandle - find the uart by phandle
> + * @dev - device that requests this uart
> + * @phandle - name of the property holding the uart phandle value
> + * @index - the index of the uart
> + *
> + * Returns the uart_port associated with the given phandle value,
> + * after getting a refcount to it, -ENODEV if there is no such uart or
> + * -EPROBE_DEFER if there is a phandle to the uart, but the device is
> + * not yet loaded. While at that, it also associates the device with
> + * the uart using devres. On driver detach, release function is invoked
> + * on the devres data, then, devres data is freed.
Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
> + *
> + * For use by tty host and
Re: [PATCH RFC v2 1/3] tty: serial core: provide method to search uart by phandle
Hi,
Some comments below.
On Sun, Jun 28, 2015 at 09:46:24PM +0200, Marek Belisko wrote:
From: H. Nikolaus Schaller h...@goldelico.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
Signed-off-by: H. Nikolaus Schaller h...@goldelico.com
Signed-off-by: Marek Belisko ma...@goldelico.com
---
Documentation/serial/slaves.txt | 36 ++
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 000..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
+* 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.
+
+ gps {
+ compatible = wi2wi,w2sg0004;
+ uart = uart1;
+ };
+
+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.
diff --git a/drivers/tty/serial/serial_core.c
b/drivers/tty/serial/serial_core.c
index eec067d..ad61441 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -38,6 +38,33 @@
#include asm/irq.h
#include asm/uaccess.h
+static LIST_HEAD(uart_list);
+static DEFINE_SPINLOCK(uart_lock);
+
+/* same concept as __of_usb_find_phy */
+static struct uart_port *__of_serial_find_uart(struct device_node *node)
+{
+ struct uart_port *uart;
+
+ if (!of_device_is_available(node))
+ return ERR_PTR(-ENODEV);
+
+ list_for_each_entry(uart, uart_list, head) {
+ if (node != uart-dev-of_node)
+ continue;
+
+ return uart;
We can easily save three lines here :)
+ }
+
+ return ERR_PTR(-EPROBE_DEFER);
+}
+
+static void devm_serial_uart_release(struct device *dev, void *res)
+{
+ struct uart_port *uart = *(struct uart_port **)res;
+ /* FIXME: serial_put_uart(uart); */
+}
Looks unfinished...
+
/*
* This is used to lock changes in serial line configuration.
*/
@@ -64,6 +91,78 @@ static int uart_dcd_enabled(struct uart_port *uport)
return !!(uport-status UPSTAT_DCD_ENABLE);
}
+/**
+ * devm_serial_get_uart_by_phandle - find the uart by phandle
+ * @dev - device that requests this uart
+ * @phandle - name of the property holding the uart phandle value
+ * @index - the index of the uart
+ *
+ * Returns the uart_port associated with the given phandle value,
+ * after getting a refcount to it, -ENODEV if there is no such uart or
+ * -EPROBE_DEFER if there is a phandle to the uart, but the device is
+ * not yet loaded. While at that, it also associates the device with
+ * the uart using devres. On driver detach, release function is invoked
+ * on the devres data, then, devres data is freed.
Add -ENOMEM and -EINVAL, remove -EPROBE_DEFER?
+ *
+ * For use by tty host and peripheral drivers.
+ */
+
+/* same concept as
