Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On Fri, 27 Mar 2015 07:09:56 -0400 Peter Hurley wrote: > On 03/25/2015 05:17 PM, NeilBrown wrote: > > On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley > > wrote: > > > >> On 03/18/2015 01:58 AM, NeilBrown wrote: > >> > >>> + * A "tty-slave" is a device permanently attached to a particularly > >>> + * tty, typically wired to a UART. > >> > >> Why "permanently"? > >> Is that a limitation of the implementation or design? > >> > > > > The slave is described in devicetree - that only happens for permanently > > attached devices, doesn't it? > > > > I guess that with device-tree overlays and 'capes' for boards you could have > > a device attached to the uart "for this power session" rather than > > "permanently", but I think it is a rather subtle distinction. > > > > Did you have something else in mind? > > My primary concern is that the abstraction match the scope. > > If the abstraction is at the tty layer, then the scope of the design > should support tty devices, not just hard-wired, devicetree-defined uarts. I think I see your point, and I tend to agree. However there is a limit to how closely we can reach the ideal... I see a lot of conceptual overlap between line discipline and tty_slaves. They both provide extra driver support for the thing which the tty talks to. Line disciplines can be configured at runtime depending on what is found to be attached. tty_slaves are configure at boot time depending what is declared to be attached in device-tree. line disciplines are a lot like device drivers, but aren't implemented that way for sound historical reasons. I think it would be nice if they were, but they aren't. tty_slaves really need to be devices with device drivers so that they can utilise information from devicetree. line disciplines do not and cannot know about any hardware other than the standard UART. tty_slaves exist so that they can know about regulators and GPIOs and anything else that might combine with the UART to control a particular device. So tty_slaves are really specifically for devices which present a tty, but have more hardware controls than just a UART. Anything that doesn't have more hardware controls is probably best handled from user-space or in a line discipline. And if there are more hardware controls, then it is sure to be permanently attached. Does that make sense? Does it allay your concerns? Thanks, NeilBrown pgp1l_jv0mjyc.pgp Description: OpenPGP digital signature
Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On Fri, 27 Mar 2015 07:09:56 -0400 Peter Hurley pe...@hurleysoftware.com wrote: On 03/25/2015 05:17 PM, NeilBrown wrote: On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley pe...@hurleysoftware.com wrote: On 03/18/2015 01:58 AM, NeilBrown wrote: + * A tty-slave is a device permanently attached to a particularly + * tty, typically wired to a UART. Why permanently? Is that a limitation of the implementation or design? The slave is described in devicetree - that only happens for permanently attached devices, doesn't it? I guess that with device-tree overlays and 'capes' for boards you could have a device attached to the uart for this power session rather than permanently, but I think it is a rather subtle distinction. Did you have something else in mind? My primary concern is that the abstraction match the scope. If the abstraction is at the tty layer, then the scope of the design should support tty devices, not just hard-wired, devicetree-defined uarts. I think I see your point, and I tend to agree. However there is a limit to how closely we can reach the ideal... I see a lot of conceptual overlap between line discipline and tty_slaves. They both provide extra driver support for the thing which the tty talks to. Line disciplines can be configured at runtime depending on what is found to be attached. tty_slaves are configure at boot time depending what is declared to be attached in device-tree. line disciplines are a lot like device drivers, but aren't implemented that way for sound historical reasons. I think it would be nice if they were, but they aren't. tty_slaves really need to be devices with device drivers so that they can utilise information from devicetree. line disciplines do not and cannot know about any hardware other than the standard UART. tty_slaves exist so that they can know about regulators and GPIOs and anything else that might combine with the UART to control a particular device. So tty_slaves are really specifically for devices which present a tty, but have more hardware controls than just a UART. Anything that doesn't have more hardware controls is probably best handled from user-space or in a line discipline. And if there are more hardware controls, then it is sure to be permanently attached. Does that make sense? Does it allay your concerns? Thanks, NeilBrown pgp1l_jv0mjyc.pgp Description: OpenPGP digital signature
Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On 03/25/2015 05:17 PM, NeilBrown wrote: > On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley > wrote: > >> On 03/18/2015 01:58 AM, NeilBrown wrote: >> >>> + * A "tty-slave" is a device permanently attached to a particularly >>> + * tty, typically wired to a UART. >> >> Why "permanently"? >> Is that a limitation of the implementation or design? >> > > The slave is described in devicetree - that only happens for permanently > attached devices, doesn't it? > > I guess that with device-tree overlays and 'capes' for boards you could have > a device attached to the uart "for this power session" rather than > "permanently", but I think it is a rather subtle distinction. > > Did you have something else in mind? My primary concern is that the abstraction match the scope. If the abstraction is at the tty layer, then the scope of the design should support tty devices, not just hard-wired, devicetree-defined uarts. Regards, Peter Hurley -- 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: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On 03/25/2015 05:17 PM, NeilBrown wrote: On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley pe...@hurleysoftware.com wrote: On 03/18/2015 01:58 AM, NeilBrown wrote: + * A tty-slave is a device permanently attached to a particularly + * tty, typically wired to a UART. Why permanently? Is that a limitation of the implementation or design? The slave is described in devicetree - that only happens for permanently attached devices, doesn't it? I guess that with device-tree overlays and 'capes' for boards you could have a device attached to the uart for this power session rather than permanently, but I think it is a rather subtle distinction. Did you have something else in mind? My primary concern is that the abstraction match the scope. If the abstraction is at the tty layer, then the scope of the design should support tty devices, not just hard-wired, devicetree-defined uarts. Regards, Peter Hurley -- 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: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley wrote: > On 03/18/2015 01:58 AM, NeilBrown wrote: > > > + * A "tty-slave" is a device permanently attached to a particularly > > + * tty, typically wired to a UART. > > Why "permanently"? > Is that a limitation of the implementation or design? > The slave is described in devicetree - that only happens for permanently attached devices, doesn't it? I guess that with device-tree overlays and 'capes' for boards you could have a device attached to the uart "for this power session" rather than "permanently", but I think it is a rather subtle distinction. Did you have something else in mind? NeilBrown pgpcK6FieBdI1.pgp Description: OpenPGP digital signature
Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On Wed, 25 Mar 2015 12:30:00 -0400 Peter Hurley pe...@hurleysoftware.com wrote: On 03/18/2015 01:58 AM, NeilBrown wrote: + * A tty-slave is a device permanently attached to a particularly + * tty, typically wired to a UART. Why permanently? Is that a limitation of the implementation or design? The slave is described in devicetree - that only happens for permanently attached devices, doesn't it? I guess that with device-tree overlays and 'capes' for boards you could have a device attached to the uart for this power session rather than permanently, but I think it is a rather subtle distinction. Did you have something else in mind? NeilBrown pgpcK6FieBdI1.pgp Description: OpenPGP digital signature
Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
Hi! > > Header files usually have #include guards, and some kind of comment on > > top. > > Of 1996 files in include/linux, 1851 seem to do that. That's enough to > convince me. I've done it too. :-)). Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- 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: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
Hi! Header files usually have #include guards, and some kind of comment on top. Of 1996 files in include/linux, 1851 seem to do that. That's enough to convince me. I've done it too. :-)). Thanks, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- 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: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On Fri, 20 Mar 2015 20:41:50 +0100 Pavel Machek wrote:
> Hi!
>
> (And yes, I now see dts examples, sorry for the noise.)
>
> Acked-by: Pavel Machek
>
> Minor nits below.
>
> > --- /dev/null
> > +++ b/drivers/tty/slave/tty_slave_core.c
> > @@ -0,0 +1,136 @@
> > +/*
> > + * tty-slave-core - device bus for tty slaves
>
> Filename actually uses underscores.
The filename uses underscores because all the filenames in drivers/tty do.
And this isn't a file name, it is more like a module name, and the module
tools treat '-' and '_' as equivalent.
And I prefer hyphen
I looked at other files in drivers/tty and decided noticed that they use
spaces to separate words in this context (novel concept :-) so I've done the
same.
>
> > + container_of(parent, struct tty_slave, dev);
> > + tty->ops = >ops;
> > + }
> > +}
> > +EXPORT_SYMBOL(tty_slave_activate);
>
> Not "_GPL"?
Other exports in the files are just EXPORT_SYMBOL, so I copied.
I don't feel strongly (the code is GPL anyway) so just follow what
surrounding code does.
>
> > +postcore_initcall(tty_slave_init);
> > +module_exit(tty_slave_exit);
>
> Should it have MODULE_LICENSE tag?
Yes. Added.
>
>
> > +int tty_register_finalize(struct tty_driver *driver, struct device *dev)
> > +{
> > + int retval;
> > + bool cdev = false;
> > + int index = dev->devt - MKDEV(driver->major,
> > + driver->minor_start);
> > + printk("REGISTER %d %d 0x%x %d\n", driver->major, driver->minor_start,
> > dev->devt, index);
>
> That printk should probably be removed for merge?
Gone.
>
> > + if (!(driver->flags & TTY_DRIVER_DYNAMIC_ALLOC)) {
> > + retval = tty_cdev_add(driver,
> > + dev->devt,
> > + index, 1);
>
> You can put this on one line.
Indeed. Done.
>
> > --- /dev/null
> > +++ b/include/linux/tty_slave.h
> > @@ -0,0 +1,26 @@
> > +
> > +struct tty_slave {
> > + struct device *tty_dev;
> > + struct tty_driver *tty_drv;
> > + struct tty_operations ops;
> > + struct device dev;
> > +};
>
> Header files usually have #include guards, and some kind of comment on
> top.
>
> Pavel
Of 1996 files in include/linux, 1851 seem to do that. That's enough to
convince me. I've done it too.
Thanks,
NeilBrown
pgpAJHzwlM30_.pgp
Description: OpenPGP digital signature
Re: [Gta04-owner] [PATCH 2/3] TTY: add support for tty_slave devices.
On Fri, 20 Mar 2015 20:41:50 +0100 Pavel Machek pa...@ucw.cz wrote:
Hi!
(And yes, I now see dts examples, sorry for the noise.)
Acked-by: Pavel Machek pa...@ucw.cz
Minor nits below.
--- /dev/null
+++ b/drivers/tty/slave/tty_slave_core.c
@@ -0,0 +1,136 @@
+/*
+ * tty-slave-core - device bus for tty slaves
Filename actually uses underscores.
The filename uses underscores because all the filenames in drivers/tty do.
And this isn't a file name, it is more like a module name, and the module
tools treat '-' and '_' as equivalent.
And I prefer hyphen
I looked at other files in drivers/tty and decided noticed that they use
spaces to separate words in this context (novel concept :-) so I've done the
same.
+ container_of(parent, struct tty_slave, dev);
+ tty-ops = dev-ops;
+ }
+}
+EXPORT_SYMBOL(tty_slave_activate);
Not _GPL?
Other exports in the files are just EXPORT_SYMBOL, so I copied.
I don't feel strongly (the code is GPL anyway) so just follow what
surrounding code does.
+postcore_initcall(tty_slave_init);
+module_exit(tty_slave_exit);
Should it have MODULE_LICENSE tag?
Yes. Added.
+int tty_register_finalize(struct tty_driver *driver, struct device *dev)
+{
+ int retval;
+ bool cdev = false;
+ int index = dev-devt - MKDEV(driver-major,
+ driver-minor_start);
+ printk(REGISTER %d %d 0x%x %d\n, driver-major, driver-minor_start,
dev-devt, index);
That printk should probably be removed for merge?
Gone.
+ if (!(driver-flags TTY_DRIVER_DYNAMIC_ALLOC)) {
+ retval = tty_cdev_add(driver,
+ dev-devt,
+ index, 1);
You can put this on one line.
Indeed. Done.
--- /dev/null
+++ b/include/linux/tty_slave.h
@@ -0,0 +1,26 @@
+
+struct tty_slave {
+ struct device *tty_dev;
+ struct tty_driver *tty_drv;
+ struct tty_operations ops;
+ struct device dev;
+};
Header files usually have #include guards, and some kind of comment on
top.
Pavel
Of 1996 files in include/linux, 1851 seem to do that. That's enough to
convince me. I've done it too.
Thanks,
NeilBrown
pgpAJHzwlM30_.pgp
Description: OpenPGP digital signature
