[PATCH 4/4] serial: sccnxp: Add DT support

[PATCH 4/4] serial: sccnxp: Add DT support

[Alexander Shiyan]

Add DT support to the SCCNCP serial driver.

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 .../bindings/tty/serial/sccnxp-serial.txt          | 53 ++++++++++++++++++++++
 drivers/tty/serial/sccnxp.c                        | 46 +++++++++++++++----
 include/linux/platform_data/serial-sccnxp.h        |  6 +--
 3 files changed, 93 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt

diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
new file mode 100644
index 0000000..d18b169
--- /dev/null
+++ b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
@@ -0,0 +1,53 @@
+* NXP (Philips) SCC+++(SCN+++) serial driver
+
+Required properties:
+- compatible: Should be "nxp,<ictype>". The supported ICs include sc2681,
+  sc2691, sc2692, sc2891, sc2892, sc28202, sc68681 and sc68692.
+- reg: Address and length of the register set for the device.
+- interrupts: Should contain the interrupt number. If omitted,
+  polling mode will be used instead, so "poll-interval" property should
+  be populated in this case.
+
+Optional properties:
+- clocks: Phandle to input clock. If omitted, default IC frequency will be
+  used instead.
+- poll-interval: Poll interval time in nanoseconds.
+- vcc-supply: The regulator supplying the VCC to drive the chip.
+- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
+  The number of values depends on the UART-number in the selected chip.
+  Each value should be composed according to the following rules:
+  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
+   LINE - VALUE:
+    OP0 - 1
+    OP1 - 2
+    OP2 - 3
+    OP3 - 4
+    OP4 - 5
+    OP5 - 6
+    OP6 - 7
+    OP7 - 8
+    IP0 - 9
+    IP1 - 10
+    IP2 - 11
+    IP3 - 12
+    IP4 - 13
+    IP5 - 14
+    IP6 - 15
+   SIGNAL - VALUE:
+    DTR - 0
+    RTS - 4
+    DSR - 8
+    CTS - 12
+    DCD - 16
+    RNG - 20
+    DIR - 24
+
+Example (Dual UART with direction control on OP0 & OP1):
+sc2892@10100000 {
+	compatible = "nxp,sc2892";
+	reg = <0x10100000 0x10>;
+	poll-interval = <10000>;
+	clocks = <&sc2892_clk>;
+	vcc-supply = <&sc2892_reg>;
+	nxp,sccnxp-io-cfg = <0x01000000 0x02000000>;
+};
diff --git a/drivers/tty/serial/sccnxp.c b/drivers/tty/serial/sccnxp.c
index ef53c7a..cf14528 100644
--- a/drivers/tty/serial/sccnxp.c
+++ b/drivers/tty/serial/sccnxp.c
@@ -20,6 +20,8 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/console.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/serial_core.h>
 #include <linux/serial.h>
 #include <linux/io.h>
@@ -853,10 +855,25 @@ static const struct platform_device_id sccnxp_id_table[] = {
 };
 MODULE_DEVICE_TABLE(platform, sccnxp_id_table);
 
+static const struct of_device_id sccnxp_dt_id_table[] = {
+	{ .compatible = "nxp,sc2681",	.data = &sc2681, },
+	{ .compatible = "nxp,sc2691",	.data = &sc2691, },
+	{ .compatible = "nxp,sc2692",	.data = &sc2692, },
+	{ .compatible = "nxp,sc2891",	.data = &sc2891, },
+	{ .compatible = "nxp,sc2892",	.data = &sc2892, },
+	{ .compatible = "nxp,sc28202",	.data = &sc28202, },
+	{ .compatible = "nxp,sc68681",	.data = &sc68681, },
+	{ .compatible = "nxp,sc68692",	.data = &sc68692, },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, sccnxp_dt_id_table);
+
 static int sccnxp_probe(struct platform_device *pdev)
 {
 	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	struct sccnxp_pdata *pdata = dev_get_platdata(&pdev->dev);
+	const struct of_device_id *of_id =
+		of_match_device(sccnxp_dt_id_table, &pdev->dev);
 	int i, ret, uartclk;
 	struct sccnxp_port *s;
 	void __iomem *membase;
@@ -875,7 +892,22 @@ static int sccnxp_probe(struct platform_device *pdev)
 
 	spin_lock_init(&s->lock);
 
-	s->chip = (struct sccnxp_chip *)pdev->id_entry->driver_data;
+	if (of_id) {
+		s->chip = (struct sccnxp_chip *)of_id->data;
+
+		of_property_read_u32(pdev->dev.of_node, "poll-interval",
+				     &s->pdata.poll_time_us);
+		of_property_read_u32(pdev->dev.of_node, "reg-shift",
+				     &s->pdata.reg_shift);
+		of_property_read_u32_array(pdev->dev.of_node,
+					   "nxp,sccnxp-io-cfg",
+					   s->pdata.mctrl_cfg, s->chip->nr);
+	} else {
+		s->chip = (struct sccnxp_chip *)pdev->id_entry->driver_data;
+
+		if (pdata)
+			memcpy(&s->pdata, pdata, sizeof(struct sccnxp_pdata));
+	}
 
 	s->regulator = devm_regulator_get(&pdev->dev, "vcc");
 	if (!IS_ERR(s->regulator)) {
@@ -906,16 +938,11 @@ static int sccnxp_probe(struct platform_device *pdev)
 		goto err_out;
 	}
 
-	if (pdata)
-		memcpy(&s->pdata, pdata, sizeof(struct sccnxp_pdata));
-
 	if (s->pdata.poll_time_us) {
 		dev_info(&pdev->dev, "Using poll mode, resolution %u usecs\n",
 			 s->pdata.poll_time_us);
 		s->poll = 1;
-	}
-
-	if (!s->poll) {
+	} else {
 		s->irq = platform_get_irq(pdev, 0);
 		if (s->irq < 0) {
 			dev_err(&pdev->dev, "Missing irq resource data\n");
@@ -1016,8 +1043,9 @@ static int sccnxp_remove(struct platform_device *pdev)
 
 static struct platform_driver sccnxp_uart_driver = {
 	.driver = {
-		.name	= SCCNXP_NAME,
-		.owner	= THIS_MODULE,
+		.name		= SCCNXP_NAME,
+		.owner		= THIS_MODULE,
+		.of_match_table	= sccnxp_dt_id_table,
 	},
 	.probe		= sccnxp_probe,
 	.remove		= sccnxp_remove,
diff --git a/include/linux/platform_data/serial-sccnxp.h b/include/linux/platform_data/serial-sccnxp.h
index af0c8c3..98373d6 100644
--- a/include/linux/platform_data/serial-sccnxp.h
+++ b/include/linux/platform_data/serial-sccnxp.h
@@ -78,11 +78,11 @@
 /* SCCNXP platform data structure */
 struct sccnxp_pdata {
 	/* Shift for A0 line */
-	const u8		reg_shift;
+	u32	reg_shift;
 	/* Modem control lines configuration */
-	const u32		mctrl_cfg[SCCNXP_MAX_UARTS];
+	u32	mctrl_cfg[SCCNXP_MAX_UARTS];
 	/* Timer value for polling mode (usecs) */
-	const unsigned int	poll_time_us;
+	u32	poll_time_us;
 };
 
 #endif
-- 
1.8.1.5

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Stephen Warren]

On 07/31/2013 04:55 AM, Alexander Shiyan wrote:
> Add DT support to the SCCNCP serial driver.

> diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt


> +Optional properties:

> +- poll-interval: Poll interval time in nanoseconds.

Is this a standard/common property? If not, then it'd be best if the
property name included the vendor-prefix "nxp,".

> +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
> +  The number of values depends on the UART-number in the selected chip.
> +  Each value should be composed according to the following rules:
> +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
> +   LINE - VALUE:
> +    OP0 - 1
...
> +    IP6 - 15
> +   SIGNAL - VALUE:
> +    DTR - 0
...
> +    DIR - 24

I wonder that shouldn't be implemented using standard pinctrl bindings.
I could see someone wanting to create a pinmuxing-based serial port
multiplexing device, which would then need to rely on this node acting
as a standard pin controller...

> +Example (Dual UART with direction control on OP0 & OP1):
> +sc2892@10100000 {
> +	compatible = "nxp,sc2892";
> +	reg = <0x10100000 0x10>;
> +	poll-interval = <10000>;
> +	clocks = <&sc2892_clk>;
> +	vcc-supply = <&sc2892_reg>;
> +	nxp,sccnxp-io-cfg = <0x01000000 0x02000000>;

Why two cells for io-cfg when the shifts above imply everything fits
into a single cell?

Oh I guess that "The number of values depends on the UART-number" means
"number of UARTs in the chip", whereas I read it as "the ID of the UART
within the chip"..

When there are multiple UARTs in the chip, should each UART have its own
separate DT node? If not, how could you refer to each individual UART
e.g. in order to set up the /aliases node?

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Alexander Shiyan]

> On 07/31/2013 04:55 AM, Alexander Shiyan wrote:
> > Add DT support to the SCCNCP serial driver.
> 
> > diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> 
> > +Optional properties:
> 
> > +- poll-interval: Poll interval time in nanoseconds.
> 
> Is this a standard/common property? If not, then it'd be best if the
> property name included the vendor-prefix "nxp,".

I am not sure about this, At least this property is used in gpio-keys-polled.

> > +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
> > +  The number of values depends on the UART-number in the selected chip.
> > +  Each value should be composed according to the following rules:
> > +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
> > +   LINE - VALUE:
> > +    OP0 - 1
> ...
> > +    IP6 - 15
> > +   SIGNAL - VALUE:
> > +    DTR - 0
> ...
> > +    DIR - 24
> 
> I wonder that shouldn't be implemented using standard pinctrl bindings.
> I could see someone wanting to create a pinmuxing-based serial port
> multiplexing device, which would then need to rely on this node acting
> as a standard pin controller...

In this case pin-controller cannot be applied here.
The device has several lines of general purpose inputs and outputs, there is
no functional purpose of these lines as a modem signals. In the driver realized
only an emulation of these signals. That is, this property describes the trick.
I have no idea how to do this trick in another way.

> > +Example (Dual UART with direction control on OP0 & OP1):
> > +sc2892@10100000 {
> > +	compatible = "nxp,sc2892";
> > +	reg = <0x10100000 0x10>;
> > +	poll-interval = <10000>;
> > +	clocks = <&sc2892_clk>;
> > +	vcc-supply = <&sc2892_reg>;
> > +	nxp,sccnxp-io-cfg = <0x01000000 0x02000000>;
> 
> Why two cells for io-cfg when the shifts above imply everything fits
> into a single cell?
> 
> Oh I guess that "The number of values depends on the UART-number" means
> "number of UARTs in the chip", whereas I read it as "the ID of the UART
> within the chip"..
> 
> When there are multiple UARTs in the chip, should each UART have its own
> separate DT node? If not, how could you refer to each individual UART
> e.g. in order to set up the /aliases node?

This is my translation inaccuracies: io-cfg count should be == UART count.
Driver initializes all ports simultaneously (1 or 2). I cannot separate ports,
because IC registers mixed in one address space.

As variant, I can make the io-configuration of a single variable u64.
Or, at this stage, remove this option to add later.
Thanks.

---

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Stephen Warren]

On 08/01/2013 01:51 AM, Alexander Shiyan wrote:
>> On 07/31/2013 04:55 AM, Alexander Shiyan wrote:
>>> Add DT support to the SCCNCP serial driver.
>>
>>> diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
>>
>>> +Optional properties:
>>
>>> +- poll-interval: Poll interval time in nanoseconds.
>>
>> Is this a standard/common property? If not, then it'd be best if the
>> property name included the vendor-prefix "nxp,".
> 
> I am not sure about this, At least this property is used in gpio-keys-polled.
> 
>>> +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
>>> +  The number of values depends on the UART-number in the selected chip.
>>> +  Each value should be composed according to the following rules:
>>> +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
>>> +   LINE - VALUE:
>>> +    OP0 - 1
>> ...
>>> +    IP6 - 15
>>> +   SIGNAL - VALUE:
>>> +    DTR - 0
>> ...
>>> +    DIR - 24
>>
>> I wonder that shouldn't be implemented using standard pinctrl bindings.
>> I could see someone wanting to create a pinmuxing-based serial port
>> multiplexing device, which would then need to rely on this node acting
>> as a standard pin controller...
> 
> In this case pin-controller cannot be applied here.

Why not?

> The device has several lines of general purpose inputs and outputs, there is
> no functional purpose of these lines as a modem signals. In the driver realized
> only an emulation of these signals. That is, this property describes the trick.
> I have no idea how to do this trick in another way.

I assume the HW design is that there are 15 pins, and there is a
register or are multiple registers that define which pins are used for
which purpose.

If so, that's exactly what the pinctrl bindings are for.

However, you're talking about emulation... Is this property really
describing HW? If not, it isn't appropriate for DT.

>>> +Example (Dual UART with direction control on OP0 & OP1):
>>> +sc2892@10100000 {
>>> +	compatible = "nxp,sc2892";
>>> +	reg = <0x10100000 0x10>;
>>> +	poll-interval = <10000>;
>>> +	clocks = <&sc2892_clk>;
>>> +	vcc-supply = <&sc2892_reg>;
>>> +	nxp,sccnxp-io-cfg = <0x01000000 0x02000000>;
>>
>> Why two cells for io-cfg when the shifts above imply everything fits
>> into a single cell?
>>
>> Oh I guess that "The number of values depends on the UART-number" means
>> "number of UARTs in the chip", whereas I read it as "the ID of the UART
>> within the chip"..
>>
>> When there are multiple UARTs in the chip, should each UART have its own
>> separate DT node? If not, how could you refer to each individual UART
>> e.g. in order to set up the /aliases node?
> 
> This is my translation inaccuracies: io-cfg count should be == UART count.
> Driver initializes all ports simultaneously (1 or 2). I cannot separate ports,
> because IC registers mixed in one address space.

If there really are multiple ports, I think there have to be multiple DT
nodes, given the way /aliases works.

> As variant, I can make the io-configuration of a single variable u64.

In DT, cells are U32.

> Or, at this stage, remove this option to add later.
> Thanks.

The binding should be as complete as possible up-front. Adding stuff
later only increases the likelihood of wanting to make incompatible changes.

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Alexander Shiyan]

On Thu, 01 Aug 2013 10:31:13 -0600
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 08/01/2013 01:51 AM, Alexander Shiyan wrote:
> >> On 07/31/2013 04:55 AM, Alexander Shiyan wrote:
> >>> Add DT support to the SCCNCP serial driver.
> >>
> >>> diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
[...]
> >>> +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
> >>> +  The number of values depends on the UART-number in the selected chip.
> >>> +  Each value should be composed according to the following rules:
> >>> +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
> >>> +   LINE - VALUE:
> >>> +    OP0 - 1
> >> ...
> >>> +    IP6 - 15
> >>> +   SIGNAL - VALUE:
> >>> +    DTR - 0
> >> ...
> >>> +    DIR - 24
> >>
> >> I wonder that shouldn't be implemented using standard pinctrl bindings.
> >> I could see someone wanting to create a pinmuxing-based serial port
> >> multiplexing device, which would then need to rely on this node acting
> >> as a standard pin controller...
> > 
> > In this case pin-controller cannot be applied here.
> 
> Why not?
> 
> > The device has several lines of general purpose inputs and outputs, there is
> > no functional purpose of these lines as a modem signals. In the driver realized
> > only an emulation of these signals. That is, this property describes the trick.
> > I have no idea how to do this trick in another way.
> 
> I assume the HW design is that there are 15 pins, and there is a
> register or are multiple registers that define which pins are used for
> which purpose.
> 
> If so, that's exactly what the pinctrl bindings are for.
> 
> However, you're talking about emulation... Is this property really
> describing HW? If not, it isn't appropriate for DT.

What your opinion about replace this property by "quirk" property and hardcode
these values into the driver?
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Mark Rutland]

On Wed, Jul 31, 2013 at 11:55:45AM +0100, Alexander Shiyan wrote:
> Add DT support to the SCCNCP serial driver.
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  .../bindings/tty/serial/sccnxp-serial.txt          | 53 ++++++++++++++++++++++
>  drivers/tty/serial/sccnxp.c                        | 46 +++++++++++++++----
>  include/linux/platform_data/serial-sccnxp.h        |  6 +--
>  3 files changed, 93 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> 
> diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> new file mode 100644
> index 0000000..d18b169
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> @@ -0,0 +1,53 @@
> +* NXP (Philips) SCC+++(SCN+++) serial driver
> +
> +Required properties:
> +- compatible: Should be "nxp,<ictype>". The supported ICs include sc2681,
> +  sc2691, sc2692, sc2891, sc2892, sc28202, sc68681 and sc68692.
> +- reg: Address and length of the register set for the device.
> +- interrupts: Should contain the interrupt number. If omitted,
> +  polling mode will be used instead, so "poll-interval" property should
> +  be populated in this case.

I'm not so keen on bindings describing what drivers *will* do (as
opposed to what they *may* do), but many other bindings already describe
this. It feels like we're describing driver behaviour rather than
describing the hardware and letting a driver try its best to use the
hardware in whatever way it sees fit.

I'm not sure how others feel on this, but I'd prefer a description more
like:

- interrupts: Should contain the interrupt number. If omitted, a driver
  may poll the device, so the "poll-interval" property should be
  populated.

> +
> +Optional properties:
> +- clocks: Phandle to input clock. If omitted, default IC frequency will be
> +  used instead.
> +- poll-interval: Poll interval time in nanoseconds.

Is there any reason this needs to be described at all? Is this interval
a minimum/maximum bound required for some reason, or just a sensible
value?

This feels like driver configuration than hardware description.

> +- vcc-supply: The regulator supplying the VCC to drive the chip.
> +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
> +  The number of values depends on the UART-number in the selected chip.
> +  Each value should be composed according to the following rules:
> +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
> +   LINE - VALUE:
> +    OP0 - 1
> +    OP1 - 2
> +    OP2 - 3
> +    OP3 - 4
> +    OP4 - 5
> +    OP5 - 6
> +    OP6 - 7
> +    OP7 - 8
> +    IP0 - 9
> +    IP1 - 10
> +    IP2 - 11
> +    IP3 - 12
> +    IP4 - 13
> +    IP5 - 14
> +    IP6 - 15
> +   SIGNAL - VALUE:
> +    DTR - 0
> +    RTS - 4
> +    DSR - 8
> +    CTS - 12
> +    DCD - 16
> +    RNG - 20
> +    DIR - 24

I don't really understand what this is describing, but I'm not sure that
the encoding (with an OR of shifted values) is the most sensible. Could
you elaborate on what is being described and how it's used?

Thanks,
Mark.

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Alexander Shiyan]

> On Wed, Jul 31, 2013 at 11:55:45AM +0100, Alexander Shiyan wrote:
> > Add DT support to the SCCNCP serial driver.
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  .../bindings/tty/serial/sccnxp-serial.txt          | 53 ++++++++++++++++++++++
> >  drivers/tty/serial/sccnxp.c                        | 46 +++++++++++++++----
> >  include/linux/platform_data/serial-sccnxp.h        |  6 +--
> >  3 files changed, 93 insertions(+), 12 deletions(-)
> >  create mode 100644 Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
[...]
> > +Optional properties:
> > +- clocks: Phandle to input clock. If omitted, default IC frequency will be
> > +  used instead.
> > +- poll-interval: Poll interval time in nanoseconds.
> 
> Is there any reason this needs to be described at all? Is this interval
> a minimum/maximum bound required for some reason, or just a sensible
> value?
> 
> This feels like driver configuration than hardware description.

This is a exact value for driver in the polling mode.
Depends on desired response time and/or desired UART baudrate.

> > +- vcc-supply: The regulator supplying the VCC to drive the chip.
> > +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
> > +  The number of values depends on the UART-number in the selected chip.
> > +  Each value should be composed according to the following rules:
> > +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
> > +   LINE - VALUE:
> > +    OP0 - 1
[...]
> > +    DIR - 24
> 
> I don't really understand what this is describing, but I'm not sure that
> the encoding (with an OR of shifted values) is the most sensible. Could
> you elaborate on what is being described and how it's used?

I have already described this property in a reply to Stephen Warren.
I'd like to do it differently, but I have no idea of another implementation of this.
Thanks.

---

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Mark Rutland]

On Thu, Aug 01, 2013 at 11:53:43AM +0100, Alexander Shiyan wrote:
> > On Wed, Jul 31, 2013 at 11:55:45AM +0100, Alexander Shiyan wrote:
> > > Add DT support to the SCCNCP serial driver.
> > > 
> > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > ---
> > >  .../bindings/tty/serial/sccnxp-serial.txt          | 53 ++++++++++++++++++++++
> > >  drivers/tty/serial/sccnxp.c                        | 46 +++++++++++++++----
> > >  include/linux/platform_data/serial-sccnxp.h        |  6 +--
> > >  3 files changed, 93 insertions(+), 12 deletions(-)
> > >  create mode 100644 Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> [...]
> > > +Optional properties:
> > > +- clocks: Phandle to input clock. If omitted, default IC frequency will be
> > > +  used instead.

Come to think of it, what is "default IC frequency" likely to be, and
how does it influence the usable baud rates?

> > > +- poll-interval: Poll interval time in nanoseconds.
> > 
> > Is there any reason this needs to be described at all? Is this interval
> > a minimum/maximum bound required for some reason, or just a sensible
> > value?
> > 
> > This feels like driver configuration than hardware description.
> 
> This is a exact value for driver in the polling mode.

That certainly sounds like driver configuration ;)

> Depends on desired response time and/or desired UART baudrate.

If this depends on the desired baud rate, how does this interact with
dynamically changing the baud rate later -- surely you may need to have
different polling rates for high and low baud rates?

> 
> > > +- vcc-supply: The regulator supplying the VCC to drive the chip.
> > > +- nxp,sccnxp-io-cfg: Array contains values for the emulated modem signals.
> > > +  The number of values depends on the UART-number in the selected chip.
> > > +  Each value should be composed according to the following rules:
> > > +  (LINE1 << SIGNAL1) | ... | (LINEX << SIGNALX), where:
> > > +   LINE - VALUE:
> > > +    OP0 - 1
> [...]
> > > +    DIR - 24
> > 
> > I don't really understand what this is describing, but I'm not sure that
> > the encoding (with an OR of shifted values) is the most sensible. Could
> > you elaborate on what is being described and how it's used?
> 
> I have already described this property in a reply to Stephen Warren.
> I'd like to do it differently, but I have no idea of another implementation of this.

Is there any publicly available documentation for the hardware?

Thanks,
Mark.

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Alexander Shiyan]

On Thu, 1 Aug 2013 15:14:20 +0100
Mark Rutland <mark.rutland@arm.com> wrote:

> On Thu, Aug 01, 2013 at 11:53:43AM +0100, Alexander Shiyan wrote:
> > > On Wed, Jul 31, 2013 at 11:55:45AM +0100, Alexander Shiyan wrote:
> > > > Add DT support to the SCCNCP serial driver.
> > > > 
> > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > ---
> > > >  .../bindings/tty/serial/sccnxp-serial.txt          | 53 ++++++++++++++++++++++
> > > >  drivers/tty/serial/sccnxp.c                        | 46 +++++++++++++++----
> > > >  include/linux/platform_data/serial-sccnxp.h        |  6 +--
> > > >  3 files changed, 93 insertions(+), 12 deletions(-)
> > > >  create mode 100644 Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> > [...]
> > > > +Optional properties:
> > > > +- clocks: Phandle to input clock. If omitted, default IC frequency will be
> > > > +  used instead.
> 
> Come to think of it, what is "default IC frequency" likely to be, and
> how does it influence the usable baud rates?
> 
> > > > +- poll-interval: Poll interval time in nanoseconds.
> > > 
> > > Is there any reason this needs to be described at all? Is this interval
> > > a minimum/maximum bound required for some reason, or just a sensible
> > > value?
> > > 
> > > This feels like driver configuration than hardware description.
> > 
> > This is a exact value for driver in the polling mode.
> 
> That certainly sounds like driver configuration ;)
> 
> > Depends on desired response time and/or desired UART baudrate.
> 
> If this depends on the desired baud rate, how does this interact with
> dynamically changing the baud rate later -- surely you may need to have
> different polling rates for high and low baud rates?

I will change it to automatically calculate a reasonable value for polling.

[...]

Greg, please remove this part of patch from tty-tree.
Thanks.

-- 
Alexander Shiyan <shc_work@mail.ru>

Re: [PATCH 4/4] serial: sccnxp: Add DT support

[Greg Kroah-Hartman]

On Fri, Aug 02, 2013 at 10:27:55AM +0400, Alexander Shiyan wrote:
> On Thu, 1 Aug 2013 15:14:20 +0100
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > On Thu, Aug 01, 2013 at 11:53:43AM +0100, Alexander Shiyan wrote:
> > > > On Wed, Jul 31, 2013 at 11:55:45AM +0100, Alexander Shiyan wrote:
> > > > > Add DT support to the SCCNCP serial driver.
> > > > > 
> > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > > ---
> > > > >  .../bindings/tty/serial/sccnxp-serial.txt          | 53 ++++++++++++++++++++++
> > > > >  drivers/tty/serial/sccnxp.c                        | 46 +++++++++++++++----
> > > > >  include/linux/platform_data/serial-sccnxp.h        |  6 +--
> > > > >  3 files changed, 93 insertions(+), 12 deletions(-)
> > > > >  create mode 100644 Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt b/Documentation/devicetree/bindings/tty/serial/sccnxp-serial.txt
> > > [...]
> > > > > +Optional properties:
> > > > > +- clocks: Phandle to input clock. If omitted, default IC frequency will be
> > > > > +  used instead.
> > 
> > Come to think of it, what is "default IC frequency" likely to be, and
> > how does it influence the usable baud rates?
> > 
> > > > > +- poll-interval: Poll interval time in nanoseconds.
> > > > 
> > > > Is there any reason this needs to be described at all? Is this interval
> > > > a minimum/maximum bound required for some reason, or just a sensible
> > > > value?
> > > > 
> > > > This feels like driver configuration than hardware description.
> > > 
> > > This is a exact value for driver in the polling mode.
> > 
> > That certainly sounds like driver configuration ;)
> > 
> > > Depends on desired response time and/or desired UART baudrate.
> > 
> > If this depends on the desired baud rate, how does this interact with
> > dynamically changing the baud rate later -- surely you may need to have
> > different polling rates for high and low baud rates?
> 
> I will change it to automatically calculate a reasonable value for polling.
> 
> [...]
> 
> Greg, please remove this part of patch from tty-tree.

Now removed.