* [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
[not found] <CAL_JsqKNnfgESG6ON95D7nD8VNrcVy7-x6cGGnae_GbbGKAuPQ@mail.gmail.com>
@ 2018-08-05 23:26 ` Andreas Färber
2018-08-06 16:21 ` Andreas Färber
2018-08-10 17:34 ` Rob Herring
0 siblings, 2 replies; 5+ messages in thread
From: Andreas Färber @ 2018-08-05 23:26 UTC (permalink / raw)
To: linux-serial, Rob Herring
Cc: linux-mips, jringle, allsey87, Jakub Kicinski, Xue Liu,
Andreas Färber, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel
This is to allow using serdev.
Signed-off-by: Andreas Färber <afaerber@suse.de>
---
drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 243c96025053..ad7267274f65 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
SC16IS7XX_IOCONTROL_SRESET_BIT);
for (i = 0; i < devtype->nr_uart; ++i) {
+#ifdef CONFIG_OF
+ struct device_node *np;
+ struct platform_device *pdev;
+ char name[6] = "uartx";
+#endif
+
s->p[i].line = i;
/* Initialize port data */
+#ifdef CONFIG_OF
+ name[4] = '0' + i;
+ np = of_get_child_by_name(dev->of_node, name);
+ if (IS_ERR(np)) {
+ ret = PTR_ERR(np);
+ goto out_ports;
+ }
+ pdev = of_platform_device_create(np, NULL, dev);
+ if (IS_ERR(pdev)) {
+ ret = PTR_ERR(pdev);
+ goto out_ports;
+ }
+ platform_set_drvdata(pdev, dev_get_drvdata(dev));
+ s->p[i].port.dev = &pdev->dev;
+#else
s->p[i].port.dev = dev;
+#endif
s->p[i].port.irq = irq;
s->p[i].port.type = PORT_SC16IS7XX;
s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
@@ -1271,6 +1293,9 @@ static int sc16is7xx_probe(struct device *dev,
for (i--; i >= 0; i--) {
uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
clear_bit(s->p[i].port.line, &sc16is7xx_lines);
+#ifdef CONFIG_OF
+ of_platform_device_destroy(s->p[i].port.dev, NULL);
+#endif
}
#ifdef CONFIG_GPIOLIB
--
2.16.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
2018-08-05 23:26 ` [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports Andreas Färber
@ 2018-08-06 16:21 ` Andreas Färber
2018-08-10 17:34 ` Rob Herring
1 sibling, 0 replies; 5+ messages in thread
From: Andreas Färber @ 2018-08-06 16:21 UTC (permalink / raw)
To: linux-serial, Rob Herring
Cc: linux-mips, jringle, allsey87, Jakub Kicinski, Xue Liu,
Greg Kroah-Hartman, Jiri Slaby, linux-kernel
Am 06.08.2018 um 01:26 schrieb Andreas Färber:
> This is to allow using serdev.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 243c96025053..ad7267274f65 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
> SC16IS7XX_IOCONTROL_SRESET_BIT);
>
> for (i = 0; i < devtype->nr_uart; ++i) {
> +#ifdef CONFIG_OF
Looks like this and below need to be CONFIG_OF_ADDRESS (build failure
reported for sparc).
Regards,
Andreas
> + struct device_node *np;
> + struct platform_device *pdev;
> + char name[6] = "uartx";
> +#endif
> +
> s->p[i].line = i;
> /* Initialize port data */
> +#ifdef CONFIG_OF
> + name[4] = '0' + i;
> + np = of_get_child_by_name(dev->of_node, name);
> + if (IS_ERR(np)) {
> + ret = PTR_ERR(np);
> + goto out_ports;
> + }
> + pdev = of_platform_device_create(np, NULL, dev);
> + if (IS_ERR(pdev)) {
> + ret = PTR_ERR(pdev);
> + goto out_ports;
> + }
> + platform_set_drvdata(pdev, dev_get_drvdata(dev));
> + s->p[i].port.dev = &pdev->dev;
> +#else
> s->p[i].port.dev = dev;
> +#endif
> s->p[i].port.irq = irq;
> s->p[i].port.type = PORT_SC16IS7XX;
> s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
> @@ -1271,6 +1293,9 @@ static int sc16is7xx_probe(struct device *dev,
> for (i--; i >= 0; i--) {
> uart_remove_one_port(&sc16is7xx_uart, &s->p[i].port);
> clear_bit(s->p[i].port.line, &sc16is7xx_lines);
> +#ifdef CONFIG_OF
> + of_platform_device_destroy(s->p[i].port.dev, NULL);
> +#endif
> }
>
> #ifdef CONFIG_GPIOLIB
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
2018-08-05 23:26 ` [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports Andreas Färber
2018-08-06 16:21 ` Andreas Färber
@ 2018-08-10 17:34 ` Rob Herring
2018-08-10 17:45 ` Andreas Färber
1 sibling, 1 reply; 5+ messages in thread
From: Rob Herring @ 2018-08-10 17:34 UTC (permalink / raw)
To: Andreas Färber
Cc: open list:SERIAL DRIVERS, Linux-MIPS, jringle, Michael Allwright,
Jakub Kicinski, liuxuenetmail, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel
On Sun, Aug 5, 2018 at 5:27 PM Andreas Färber <afaerber@suse.de> wrote:
>
> This is to allow using serdev.
>
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 243c96025053..ad7267274f65 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
> SC16IS7XX_IOCONTROL_SRESET_BIT);
>
> for (i = 0; i < devtype->nr_uart; ++i) {
> +#ifdef CONFIG_OF
> + struct device_node *np;
> + struct platform_device *pdev;
> + char name[6] = "uartx";
> +#endif
> +
> s->p[i].line = i;
> /* Initialize port data */
> +#ifdef CONFIG_OF
> + name[4] = '0' + i;
> + np = of_get_child_by_name(dev->of_node, name);
> + if (IS_ERR(np)) {
> + ret = PTR_ERR(np);
> + goto out_ports;
> + }
> + pdev = of_platform_device_create(np, NULL, dev);
Ideally, you would use of_platform_default_populate here. I think
you'd have to add a compatible to the child nodes, but that wouldn't
be a bad thing. I could envision that the child nodes ultimately
become their own driver utilizing the standard 8250 driver and a
compatible string would be needed in that case.
You'd then have to loop over each child of 'dev' instead of the DT nodes.
> + if (IS_ERR(pdev)) {
> + ret = PTR_ERR(pdev);
> + goto out_ports;
> + }
> + platform_set_drvdata(pdev, dev_get_drvdata(dev));
> + s->p[i].port.dev = &pdev->dev;
> +#else
> s->p[i].port.dev = dev;
> +#endif
> s->p[i].port.irq = irq;
> s->p[i].port.type = PORT_SC16IS7XX;
> s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
2018-08-10 17:34 ` Rob Herring
@ 2018-08-10 17:45 ` Andreas Färber
2018-08-10 18:11 ` Rob Herring
0 siblings, 1 reply; 5+ messages in thread
From: Andreas Färber @ 2018-08-10 17:45 UTC (permalink / raw)
To: Rob Herring
Cc: SERIAL DRIVERS, Linux-MIPS, jringle, Michael Allwright,
Jakub Kicinski, liuxuenetmail, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel
Am 10.08.2018 um 19:34 schrieb Rob Herring:
> On Sun, Aug 5, 2018 at 5:27 PM Andreas Färber <afaerber@suse.de> wrote:
>>
>> This is to allow using serdev.
>>
>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>> ---
>> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
>> index 243c96025053..ad7267274f65 100644
>> --- a/drivers/tty/serial/sc16is7xx.c
>> +++ b/drivers/tty/serial/sc16is7xx.c
>> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
>> SC16IS7XX_IOCONTROL_SRESET_BIT);
>>
>> for (i = 0; i < devtype->nr_uart; ++i) {
>> +#ifdef CONFIG_OF
>> + struct device_node *np;
>> + struct platform_device *pdev;
>> + char name[6] = "uartx";
>> +#endif
>> +
>> s->p[i].line = i;
>> /* Initialize port data */
>> +#ifdef CONFIG_OF
>> + name[4] = '0' + i;
>> + np = of_get_child_by_name(dev->of_node, name);
>> + if (IS_ERR(np)) {
>> + ret = PTR_ERR(np);
>> + goto out_ports;
>> + }
>> + pdev = of_platform_device_create(np, NULL, dev);
>
> Ideally, you would use of_platform_default_populate here. I think
> you'd have to add a compatible to the child nodes, but that wouldn't
> be a bad thing. I could envision that the child nodes ultimately
> become their own driver utilizing the standard 8250 driver and a
> compatible string would be needed in that case.
Separate compatibles would mean separate drivers.
Unlike your DUART example this is not an MMIO device that we can easily
split but a SPI slave (well, regmap due to some I2C models).
I don't see how separate drivers could work, given that the whole
spi_device has a single interrupt for all functions of this device.
That left me with this ugly but working construct.
Is the uartX naming correct, or should it be serialX?
Regards,
Andreas
>
> You'd then have to loop over each child of 'dev' instead of the DT nodes.
>
>> + if (IS_ERR(pdev)) {
>> + ret = PTR_ERR(pdev);
>> + goto out_ports;
>> + }
>> + platform_set_drvdata(pdev, dev_get_drvdata(dev));
>> + s->p[i].port.dev = &pdev->dev;
>> +#else
>> s->p[i].port.dev = dev;
>> +#endif
>> s->p[i].port.irq = irq;
>> s->p[i].port.type = PORT_SC16IS7XX;
>> s->p[i].port.fifosize = SC16IS7XX_FIFO_SIZE;
--
SUSE Linux GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports
2018-08-10 17:45 ` Andreas Färber
@ 2018-08-10 18:11 ` Rob Herring
0 siblings, 0 replies; 5+ messages in thread
From: Rob Herring @ 2018-08-10 18:11 UTC (permalink / raw)
To: Andreas Färber
Cc: open list:SERIAL DRIVERS, Linux-MIPS, jringle, Michael Allwright,
Jakub Kicinski, liuxuenetmail, Greg Kroah-Hartman, Jiri Slaby,
linux-kernel
On Fri, Aug 10, 2018 at 11:45 AM Andreas Färber <afaerber@suse.de> wrote:
>
> Am 10.08.2018 um 19:34 schrieb Rob Herring:
> > On Sun, Aug 5, 2018 at 5:27 PM Andreas Färber <afaerber@suse.de> wrote:
> >>
> >> This is to allow using serdev.
> >>
> >> Signed-off-by: Andreas Färber <afaerber@suse.de>
> >> ---
> >> drivers/tty/serial/sc16is7xx.c | 25 +++++++++++++++++++++++++
> >> 1 file changed, 25 insertions(+)
> >>
> >> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> >> index 243c96025053..ad7267274f65 100644
> >> --- a/drivers/tty/serial/sc16is7xx.c
> >> +++ b/drivers/tty/serial/sc16is7xx.c
> >> @@ -1213,9 +1213,31 @@ static int sc16is7xx_probe(struct device *dev,
> >> SC16IS7XX_IOCONTROL_SRESET_BIT);
> >>
> >> for (i = 0; i < devtype->nr_uart; ++i) {
> >> +#ifdef CONFIG_OF
> >> + struct device_node *np;
> >> + struct platform_device *pdev;
> >> + char name[6] = "uartx";
> >> +#endif
> >> +
> >> s->p[i].line = i;
> >> /* Initialize port data */
> >> +#ifdef CONFIG_OF
> >> + name[4] = '0' + i;
> >> + np = of_get_child_by_name(dev->of_node, name);
> >> + if (IS_ERR(np)) {
> >> + ret = PTR_ERR(np);
> >> + goto out_ports;
> >> + }
> >> + pdev = of_platform_device_create(np, NULL, dev);
> >
> > Ideally, you would use of_platform_default_populate here. I think
> > you'd have to add a compatible to the child nodes, but that wouldn't
> > be a bad thing. I could envision that the child nodes ultimately
> > become their own driver utilizing the standard 8250 driver and a
> > compatible string would be needed in that case.
>
> Separate compatibles would mean separate drivers.
No. Having a compatible doesn't mean you have to have a driver.
> Unlike your DUART example this is not an MMIO device that we can easily
> split but a SPI slave (well, regmap due to some I2C models).
A SPI slave could provide a regmap, right?
> I don't see how separate drivers could work, given that the whole
> spi_device has a single interrupt for all functions of this device.
A shared interrupt or a parent driver that creates an irqchip like MFD
drivers often do.
>
> That left me with this ugly but working construct.
In any case, I'm was suggesting that you do any of this now. I just
want the binding to be designed to work either way.
> Is the uartX naming correct, or should it be serialX?
Ah, yes. Should be serial@... I'm fine if both the parent and child
are named serial@...
Rob
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-08-10 18:11 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CAL_JsqKNnfgESG6ON95D7nD8VNrcVy7-x6cGGnae_GbbGKAuPQ@mail.gmail.com>
2018-08-05 23:26 ` [RFC] serial: sc16is7xx: Use DT sub-nodes for UART ports Andreas Färber
2018-08-06 16:21 ` Andreas Färber
2018-08-10 17:34 ` Rob Herring
2018-08-10 17:45 ` Andreas Färber
2018-08-10 18:11 ` Rob Herring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).