On 12/12/2014 12:23 AM, NeilBrown wrote:
> On Thu, 11 Dec 2014 18:18:37 -0500 Peter Hurley <peter@xxxxxxxxxxxxxxxxxx>
> wrote:
>
>> On 12/11/2014 04:59 PM, NeilBrown wrote:
>>> A "tty slave" is a device connected via UART.
>>> It may need a driver to, for example, power the device on
>>> when the tty is opened, and power it off when the tty
>>> is released.
>>>
>>> A "tty slave" is a platform device which is declared as a
>>> child of the uart in device-tree:
>>>
>>> &uart1 {
>>> bluetooth {
>>> compatible = "tty,regulator";
>>> vdd-supply = <&vaux4>;
>>> };
>>> };
>>>
>>> runtime power management is used to power-up the device
>>> on tty_open() and power-down on tty_release().
>>
>> I have a couple of issues with this:
>> 1) why does a child device imply power management and a platform
>> device? iow, what happens when someone else wants to have a
>> child device that does something different?
>
> Why does it imply power management?
> Because it seems to make obvious sense to turn something on when the tty is
> open. If the device has other reason to remain on when the tty is closed,
> it can arrange for extra references to be taken of course.
> Is it conceivable that you would want the device to remain off when the
> tty device is open? In that case just make it a regular platform device.
>
> Why a platform device?
> Things on an i2c bus are i2c devices. Things on a usb bus are usb devices.
> Ideally a thing on a uart 'bus' would be a 'uart device', but no such thing
> exists. I did contemplate the possibility of creating an explicit "uart" or
> "tty" bus type, but I could find no value in that.
> The door is certainly still open to that possibility if a meaning for the
> idea becomes apparent. As far as device tree is concerned it is just a
> child device node. The fact that it is implemented as a "platform" device
> could easily be changed later if needed without device tree noticing.
>
> What could conceivably want to be different? The only (purely hypothetical)
> concept I can come up with which wouldn't fit is a device with both an UART
> port and a USB port, or similar. However device tree, and the device model
> in general, just isn't geared towards devices being on multiple buses so
> if that were real it would have much greater implications that just here.
>
> But maybe I misunderstand...
Yeah, misunderstanding.
This patch is using a very generic abstraction (parent-child device association)
for a really specific purpose (power management for platform devices).
What about PCI, PNP, etc.?
Also, what happens for existing child device associations like bluetooth & serio?
Why not just provide a serial core interface for associating power-managed
child devices?
Which brings up another point: why not do this in the runtime power management;
ie, turn on the slave device when the master device is turned on? Sebastian
recently made the 8250 driver power-managed per access, which would enable
significant power savings over just leaving the slave device on the whole time.
>> 2) why is this tied to the tty core and not the serial core
>> if this is only for UART?
>
> Because the knowledge of "the device is being opened" or "the device is being
> closed" seems to exist in the tty core but not in the serial core.
uart_open() = device file is being opened
uart_close() = device file is being released
> The "of_platform_populate()" call could certainly go in serial_core.c, in
> uart_add_one_port() I think. I did have it there originally. I moved it for
> a reason that I think is no longer relevant. As the on/off code is (and I
> think has to be) in tty_io.c, I left all of it there.
>
> I'm happy to move it to serial_core.c if that is preferred.
> I'll also move the open/close handling if you can point to where it should go.
Yes, please.
The reverse dependency (tty core => of) is undesirable.
Regards,
Peter Hurley
>>> Signed-off-by: NeilBrown <neilb@xxxxxxx>
>>> ---
>>> .../devicetree/bindings/serial/of-serial.txt | 4 ++++
>>> drivers/tty/tty_io.c | 22 ++++++++++++++++++++
>>> 2 files changed, 26 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/serial/of-serial.txt b/Documentation/devicetree/bindings/serial/of-serial.txt
>>> index 8c4fd0332028..b59501ee2f21 100644
>>> --- a/Documentation/devicetree/bindings/serial/of-serial.txt
>>> +++ b/Documentation/devicetree/bindings/serial/of-serial.txt
>>> @@ -39,6 +39,10 @@ Optional properties:
>>> driver is allowed to detect support for the capability even without this
>>> property.
>>>
>>> +Optional child node:
>>> +- a platform device listed as a child node will be probed and
>>> + powered-on whenever the tty is in use (open).
>>> +
>>> Example:
>>>
>>> uart@80230000 {
>>> diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
>>> index 0508a1d8e4cd..7acdc6f093f4 100644
>>> --- a/drivers/tty/tty_io.c
>>> +++ b/drivers/tty/tty_io.c
>>> @@ -95,6 +95,8 @@
>>> #include <linux/seq_file.h>
>>> #include <linux/serial.h>
>>> #include <linux/ratelimit.h>
>>> +#include <linux/pm_runtime.h>
>>> +#include <linux/of_platform.h>
>>>
>>> #include <linux/uaccess.h>
>>>
>>> @@ -1683,6 +1685,17 @@ static int tty_release_checks(struct tty_struct *tty, struct tty_struct *o_tty,
>>> return 0;
>>> }
>>>
>>> +static int open_child(struct device *dev, void *data)
>>> +{
>>> + pm_runtime_get_sync(dev);
>>> + return 0;
>>> +}
>>> +static int release_child(struct device *dev, void *data)
>>> +{
>>> + pm_runtime_put_autosuspend(dev);
>>> + return 0;
>>> +}
>>> +
>>> /**
>>> * tty_release - vfs callback for close
>>> * @inode: inode of tty
>>> @@ -1712,6 +1725,8 @@ int tty_release(struct inode *inode, struct file *filp)
>>> long timeout = 0;
>>> int once = 1;
>>>
>>> + if (tty->dev)
>>> + device_for_each_child(tty->dev, NULL, release_child);
>>> if (tty_paranoia_check(tty, inode, __func__))
>>> return 0;
>>>
>>> @@ -2118,6 +2133,8 @@ retry_open:
>>> __proc_set_tty(current, tty);
>>> spin_unlock_irq(¤t->sighand->siglock);
>>> tty_unlock(tty);
>>> + if (tty->dev)
>>> + device_for_each_child(tty->dev, NULL, open_child);
>>> mutex_unlock(&tty_mutex);
>>> return 0;
>>> err_unlock:
>>> @@ -3207,6 +3224,11 @@ struct device *tty_register_device_attr(struct tty_driver *driver,
>>> retval = device_register(dev);
>>> if (retval)
>>> goto error;
>>> + if (device && device->of_node)
>>> + /* Children are platform devices and will be
>>> + * runtime_pm managed by this tty.
>>> + */
>>> + of_platform_populate(device->of_node, NULL, NULL, dev);
>>>
>>> return dev;
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
