| Message ID | 1398265117-11793-10-git-send-email-balbi@ti.com |
|---|---|
| State | Accepted |
| Commit | 985bfd54c826c0ba873ca0adfd5589263e0c6ee2 |
| Headers | show |
+Neil On Wed, Apr 23, 2014 at 09:58:34AM -0500, Felipe Balbi wrote: > nobody passes a DTR_gpio to this driver, so > this code is not necessary. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- > drivers/tty/serial/omap-serial.c | 39 --------------------------------------- > 1 file changed, 39 deletions(-) > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index b46aaf3..6654682 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -163,10 +163,6 @@ struct uart_omap_port { > u8 wakeups_enabled; > u32 features; > > - int DTR_gpio; > - int DTR_inverted; > - int DTR_active; > - > struct serial_rs485 rs485; > int rts_gpio; > > @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl) > serial_out(up, UART_MCR, up->mcr); > pm_runtime_mark_last_busy(up->dev); > pm_runtime_put_autosuspend(up->dev); > - > - if (gpio_is_valid(up->DTR_gpio) && > - !!(mctrl & TIOCM_DTR) != up->DTR_active) { > - up->DTR_active = !up->DTR_active; > - if (gpio_cansleep(up->DTR_gpio)) > - schedule_work(&up->qos_work); > - else > - gpio_set_value(up->DTR_gpio, > - up->DTR_active != up->DTR_inverted); > - } > } > > static void serial_omap_break_ctl(struct uart_port *port, int break_state) > @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work) > qos_work); > > pm_qos_update_request(&up->pm_qos_request, up->latency); > - if (gpio_is_valid(up->DTR_gpio)) > - gpio_set_value_cansleep(up->DTR_gpio, > - up->DTR_active != up->DTR_inverted); > } > > static void > @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev) > if (IS_ERR(base)) > return PTR_ERR(base); > > - if (gpio_is_valid(omap_up_info->DTR_gpio) && > - omap_up_info->DTR_present) { > - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio, > - "omap-serial"); > - if (ret < 0) > - return ret; > - ret = gpio_direction_output(omap_up_info->DTR_gpio, > - omap_up_info->DTR_inverted); > - if (ret < 0) > - return ret; > - } > - > - if (gpio_is_valid(omap_up_info->DTR_gpio) && > - omap_up_info->DTR_present) { > - up->DTR_gpio = omap_up_info->DTR_gpio; > - up->DTR_inverted = omap_up_info->DTR_inverted; > - } else { > - up->DTR_gpio = -EINVAL; > - } > - > - up->DTR_active = 0; > - > up->dev = &pdev->dev; > up->port.dev = &pdev->dev; > up->port.type = PORT_OMAP; > -- > 1.9.2.459.g68773ac >
On 04/23/2014 09:58 AM, Felipe Balbi wrote: > nobody passes a DTR_gpio to this driver, so > this code is not necessary. > > Signed-off-by: Felipe Balbi <balbi@ti.com> > --- Niel, this seems to revert the functionality introduced in commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100 (OMAP/serial: Add support for driving a GPIO as DTR.) would you like to Ack this change? > drivers/tty/serial/omap-serial.c | 39 --------------------------------------- > 1 file changed, 39 deletions(-) > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > index b46aaf3..6654682 100644 > --- a/drivers/tty/serial/omap-serial.c > +++ b/drivers/tty/serial/omap-serial.c > @@ -163,10 +163,6 @@ struct uart_omap_port { > u8 wakeups_enabled; > u32 features; > > - int DTR_gpio; > - int DTR_inverted; > - int DTR_active; > - > struct serial_rs485 rs485; > int rts_gpio; > > @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl) > serial_out(up, UART_MCR, up->mcr); > pm_runtime_mark_last_busy(up->dev); > pm_runtime_put_autosuspend(up->dev); > - > - if (gpio_is_valid(up->DTR_gpio) && > - !!(mctrl & TIOCM_DTR) != up->DTR_active) { > - up->DTR_active = !up->DTR_active; > - if (gpio_cansleep(up->DTR_gpio)) > - schedule_work(&up->qos_work); > - else > - gpio_set_value(up->DTR_gpio, > - up->DTR_active != up->DTR_inverted); > - } > } > > static void serial_omap_break_ctl(struct uart_port *port, int break_state) > @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work) > qos_work); > > pm_qos_update_request(&up->pm_qos_request, up->latency); > - if (gpio_is_valid(up->DTR_gpio)) > - gpio_set_value_cansleep(up->DTR_gpio, > - up->DTR_active != up->DTR_inverted); > } > > static void > @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev) > if (IS_ERR(base)) > return PTR_ERR(base); > > - if (gpio_is_valid(omap_up_info->DTR_gpio) && > - omap_up_info->DTR_present) { > - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio, > - "omap-serial"); > - if (ret < 0) > - return ret; > - ret = gpio_direction_output(omap_up_info->DTR_gpio, > - omap_up_info->DTR_inverted); > - if (ret < 0) > - return ret; > - } > - > - if (gpio_is_valid(omap_up_info->DTR_gpio) && > - omap_up_info->DTR_present) { > - up->DTR_gpio = omap_up_info->DTR_gpio; > - up->DTR_inverted = omap_up_info->DTR_inverted; > - } else { > - up->DTR_gpio = -EINVAL; > - } > - > - up->DTR_active = 0; > - > up->dev = &pdev->dev; > up->port.dev = &pdev->dev; > up->port.type = PORT_OMAP; >
On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote: > On 04/23/2014 09:58 AM, Felipe Balbi wrote: > > nobody passes a DTR_gpio to this driver, so > > this code is not necessary. > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > --- > > Niel, > this seems to revert the functionality introduced in > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100 > (OMAP/serial: Add support for driving a GPIO as DTR.) > > would you like to Ack this change? I have a couple of out-of-tree drivers that use this support. I hope to get back to working on that code one day and even get those drivers upstream. So I would really prefer this code to remain if it isn't causing any actual problems. Of course, I can always re-submit it when I need it again, but that it just extra work all around. Sorry that I have pushed those drivers already, but sometimes life gets in the way :-) Thanks, NeilBrown > > > drivers/tty/serial/omap-serial.c | 39 --------------------------------------- > > 1 file changed, 39 deletions(-) > > > > diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c > > index b46aaf3..6654682 100644 > > --- a/drivers/tty/serial/omap-serial.c > > +++ b/drivers/tty/serial/omap-serial.c > > @@ -163,10 +163,6 @@ struct uart_omap_port { > > u8 wakeups_enabled; > > u32 features; > > > > - int DTR_gpio; > > - int DTR_inverted; > > - int DTR_active; > > - > > struct serial_rs485 rs485; > > int rts_gpio; > > > > @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl) > > serial_out(up, UART_MCR, up->mcr); > > pm_runtime_mark_last_busy(up->dev); > > pm_runtime_put_autosuspend(up->dev); > > - > > - if (gpio_is_valid(up->DTR_gpio) && > > - !!(mctrl & TIOCM_DTR) != up->DTR_active) { > > - up->DTR_active = !up->DTR_active; > > - if (gpio_cansleep(up->DTR_gpio)) > > - schedule_work(&up->qos_work); > > - else > > - gpio_set_value(up->DTR_gpio, > > - up->DTR_active != up->DTR_inverted); > > - } > > } > > > > static void serial_omap_break_ctl(struct uart_port *port, int break_state) > > @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work) > > qos_work); > > > > pm_qos_update_request(&up->pm_qos_request, up->latency); > > - if (gpio_is_valid(up->DTR_gpio)) > > - gpio_set_value_cansleep(up->DTR_gpio, > > - up->DTR_active != up->DTR_inverted); > > } > > > > static void > > @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev) > > if (IS_ERR(base)) > > return PTR_ERR(base); > > > > - if (gpio_is_valid(omap_up_info->DTR_gpio) && > > - omap_up_info->DTR_present) { > > - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio, > > - "omap-serial"); > > - if (ret < 0) > > - return ret; > > - ret = gpio_direction_output(omap_up_info->DTR_gpio, > > - omap_up_info->DTR_inverted); > > - if (ret < 0) > > - return ret; > > - } > > - > > - if (gpio_is_valid(omap_up_info->DTR_gpio) && > > - omap_up_info->DTR_present) { > > - up->DTR_gpio = omap_up_info->DTR_gpio; > > - up->DTR_inverted = omap_up_info->DTR_inverted; > > - } else { > > - up->DTR_gpio = -EINVAL; > > - } > > - > > - up->DTR_active = 0; > > - > > up->dev = &pdev->dev; > > up->port.dev = &pdev->dev; > > up->port.type = PORT_OMAP; > > > >
Hi, On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote: > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote: > > > On 04/23/2014 09:58 AM, Felipe Balbi wrote: > > > nobody passes a DTR_gpio to this driver, so > > > this code is not necessary. > > > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > > --- > > > > Niel, > > this seems to revert the functionality introduced in > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100 > > (OMAP/serial: Add support for driving a GPIO as DTR.) > > > > would you like to Ack this change? > > I have a couple of out-of-tree drivers that use this support. > > I hope to get back to working on that code one day and even get those drivers > upstream. So I would really prefer this code to remain if it isn't causing > any actual problems. it causes problem with DT (not really). That suport is only available on legacy platform_data-based boot, it's not available on DT. I hear Tony is pretty close to turning OMAP3 DT-only. > Of course, I can always re-submit it when I need it again, but that it just > extra work all around. I wonder how you will pass those attributes through DT considering they are *really* SW configuration. Why can't you use the real DTR pin, btw ?
On Wed, 23 Apr 2014 18:01:21 -0500 Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote: > > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote: > > > > > On 04/23/2014 09:58 AM, Felipe Balbi wrote: > > > > nobody passes a DTR_gpio to this driver, so > > > > this code is not necessary. > > > > > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > > > --- > > > > > > Niel, > > > this seems to revert the functionality introduced in > > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100 > > > (OMAP/serial: Add support for driving a GPIO as DTR.) > > > > > > would you like to Ack this change? > > > > I have a couple of out-of-tree drivers that use this support. > > > > I hope to get back to working on that code one day and even get those drivers > > upstream. So I would really prefer this code to remain if it isn't causing > > any actual problems. > > it causes problem with DT (not really). That suport is only available on > legacy platform_data-based boot, it's not available on DT. I hear Tony > is pretty close to turning OMAP3 DT-only. > > > Of course, I can always re-submit it when I need it again, but that it just > > extra work all around. > > I wonder how you will pass those attributes through DT considering they > are *really* SW configuration. Why can't you use the real DTR pin, btw ? > This myth that DT is only about hardware is probably one of the reasons that I haven't got these out-of-tree drivers upstream yet. There is no "real" DTR pin. When I open /dev/ttyWHATEVER, I need the driver for the device that is permanently connected to that serial port to be told that the serial-device has been opened so that it can "power on" or "wake up" the device. I don't much care how that happens, or how I tell DT that it has to happen. I just need it to happen. In discrete hardware devices, the DTR line is what you would use. The RS232 port would raise (or lower or whatever) DTR when /dev/ttyWHATEVER was open, and the device that was plugged in would detect the level change and do stuff. But I don't have discrete hardware. I have a bunch of stuff soldered onto a board with ad-hoc connections chosen to make the life of the hardware builder easy rather than chosen to make the life of the software developer easy (which I think is the correct choice). So I need to tell DT "This device is plugged into this UART, and there is no DTR line, but when the UARTs DTR line would be asserted (if it had one), then I need that regulator of there turned on". or maybe "I need to toggle this GPIO with exactly this pattern, while watching that GPIO to see if it is working". So I thought: 1/ give the UART a "virtual" DTR so it could drive any GPIO 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio and responded to state changes on that GPIO by turning on or off the regulator 3/ create a dedicated driver which knows how to toggle the magic GPIO while watching the other GPIO to convince the silly device to wakeup, or go to sleep, as required, and have this appear as a (virtual) GPIO. Then I can just tell DT that these (virtual) GPIOs are connected together, and it will all just work. And it does. But given the whole "no no, DT is for describing hardware, and you are describing software" attitude, it seems that I lost motivation for a while (that wasn't the only reason, but it didn't help). I have a patch which converts the OMAP serial driver to use DT to configure these virtual DTR lines. I'm very happy to submit that if there is some chance it might be accepted and will keep the current DTR code in place. On the other hand, if you can point out to me what I'm missing, and how I can solve my problem with any virtual GPIOs, I'm all ears. To make my problem simple and explicit: I have a device attached to a UART which has a separate regulator. The regulator should be powered on if and only if the /dev/ttyXX interface to the UART is open. The device is a bluetooth transceiver. (I have another device which is a GPS receiver which has similar but more complicated requirements) Thanks, NeilBrown
Hi, On Thu, Apr 24, 2014 at 10:13:29AM +1000, NeilBrown wrote: > On Wed, 23 Apr 2014 18:01:21 -0500 Felipe Balbi <balbi@ti.com> wrote: > > > Hi, > > > > On Thu, Apr 24, 2014 at 08:43:05AM +1000, NeilBrown wrote: > > > On Wed, 23 Apr 2014 10:35:04 -0500 Nishanth Menon <nm@ti.com> wrote: > > > > > > > On 04/23/2014 09:58 AM, Felipe Balbi wrote: > > > > > nobody passes a DTR_gpio to this driver, so > > > > > this code is not necessary. > > > > > > > > > > Signed-off-by: Felipe Balbi <balbi@ti.com> > > > > > --- > > > > > > > > Niel, > > > > this seems to revert the functionality introduced in > > > > commit 9574f36fb801035f6ab0fbb1b53ce2c12c17d100 > > > > (OMAP/serial: Add support for driving a GPIO as DTR.) > > > > > > > > would you like to Ack this change? > > > > > > I have a couple of out-of-tree drivers that use this support. > > > > > > I hope to get back to working on that code one day and even get those drivers > > > upstream. So I would really prefer this code to remain if it isn't causing > > > any actual problems. > > > > it causes problem with DT (not really). That suport is only available on > > legacy platform_data-based boot, it's not available on DT. I hear Tony > > is pretty close to turning OMAP3 DT-only. > > > > > Of course, I can always re-submit it when I need it again, but that it just > > > extra work all around. > > > > I wonder how you will pass those attributes through DT considering they > > are *really* SW configuration. Why can't you use the real DTR pin, btw ? > > > > This myth that DT is only about hardware is probably one of the > reasons that I haven't got these out-of-tree drivers upstream yet. take that up with DT maintainers > There is no "real" DTR pin. heh, my bad... confused with RTS which is supported in this HW. > When I open /dev/ttyWHATEVER, I need the driver for the device that is > permanently connected to that serial port to be told that the serial-device > has been opened so that it can "power on" or "wake up" the device. > > I don't much care how that happens, or how I tell DT that it has to happen. > I just need it to happen. > > In discrete hardware devices, the DTR line is what you would use. The RS232 > port would raise (or lower or whatever) DTR when /dev/ttyWHATEVER was open, > and the device that was plugged in would detect the level change and do > stuff. > > But I don't have discrete hardware. I have a bunch of stuff soldered onto a > board with ad-hoc connections chosen to make the life of the hardware builder > easy rather than chosen to make the life of the software developer easy > (which I think is the correct choice). > > So I need to tell DT "This device is plugged into this UART, and there is no > DTR line, but when the UARTs DTR line would be asserted (if it had one), then > I need that regulator of there turned on". or maybe "I need to toggle this > GPIO with exactly this pattern, while watching that GPIO to see if it is > working". > > So I thought: > > 1/ give the UART a "virtual" DTR so it could drive any GPIO > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio > and responded to state changes on that GPIO by turning on or off the > regulator > 3/ create a dedicated driver which knows how to toggle the magic GPIO while > watching the other GPIO to convince the silly device to wakeup, or go to > sleep, as required, and have this appear as a (virtual) GPIO. > > Then I can just tell DT that these (virtual) GPIOs are connected together, > and it will all just work. And it does. > > But given the whole "no no, DT is for describing hardware, and you are > describing software" attitude, it seems that I lost motivation for a while > (that wasn't the only reason, but it didn't help). > > I have a patch which converts the OMAP serial driver to use DT to configure > these virtual DTR lines. I'm very happy to submit that if there is some > chance it might be accepted and will keep the current DTR code in place. I have no problem either way, just that unused code doesn't have to be sitting in the tree and I'm not entirely sure this GPIO should be handled by omap-serial.c, perhaps something more generic inside serial-core so other UART drivers can benefit from it. > On the other hand, if you can point out to me what I'm missing, and how I can > solve my problem with any virtual GPIOs, I'm all ears. > > To make my problem simple and explicit: I have a device attached to a UART > which has a separate regulator. The regulator should be powered on if and So you're using DTR to power the GPIO and hoping that the regulator stabilizes quickly enough so that by the time your open() finishes you don't have to add nonsensical msleep() calls before writing to the device. Sounds a bit fragile to me. > only if the /dev/ttyXX interface to the UART is open. The device is a > bluetooth transceiver. considering this is a BTUART device, why didn't you do this at the ldisc level ? hci_uart_open() sounds like a good choice from a quick thinking.
On Wed, 23 Apr 2014 20:21:00 -0500 Felipe Balbi <balbi@ti.com> wrote: > I have no problem either way, just that unused code doesn't have to be > sitting in the tree and I'm not entirely sure this GPIO should be > handled by omap-serial.c, perhaps something more generic inside > serial-core so other UART drivers can benefit from it. Perhaps. But there there are more people I need to convince :-) > > > On the other hand, if you can point out to me what I'm missing, and how I can > > solve my problem with any virtual GPIOs, I'm all ears. > > > > To make my problem simple and explicit: I have a device attached to a UART > > which has a separate regulator. The regulator should be powered on if and > > So you're using DTR to power the GPIO and hoping that the regulator > stabilizes quickly enough so that by the time your open() finishes you > don't have to add nonsensical msleep() calls before writing to the > device. Sounds a bit fragile to me. The gpio_set call is synchronous, and the gpio-regulator driver could add a delay (I think). > > > only if the /dev/ttyXX interface to the UART is open. The device is a > > bluetooth transceiver. > > considering this is a BTUART device, why didn't you do this at the ldisc > level ? hci_uart_open() sounds like a good choice from a quick thinking. > I'll have a look into that, thanks. NeilBrown
Hi, On Thu, Apr 24, 2014 at 11:41:15AM +1000, NeilBrown wrote: > On Wed, 23 Apr 2014 20:21:00 -0500 Felipe Balbi <balbi@ti.com> wrote: > > > I have no problem either way, just that unused code doesn't have to be > > sitting in the tree and I'm not entirely sure this GPIO should be > > handled by omap-serial.c, perhaps something more generic inside > > serial-core so other UART drivers can benefit from it. > > Perhaps. But there there are more people I need to convince :-) heh, Greg is in Cc, that'd be a good start. > > > On the other hand, if you can point out to me what I'm missing, and how I can > > > solve my problem with any virtual GPIOs, I'm all ears. > > > > > > To make my problem simple and explicit: I have a device attached to a UART > > > which has a separate regulator. The regulator should be powered on if and > > > > So you're using DTR to power the GPIO and hoping that the regulator > > stabilizes quickly enough so that by the time your open() finishes you > > don't have to add nonsensical msleep() calls before writing to the > > device. Sounds a bit fragile to me. > > The gpio_set call is synchronous, and the gpio-regulator driver could add a sure, but it's synchronous towards toggling the GPIO, pulling it high. It doesn't guarantee that the far-end regulator's output will be fully changed. > delay (I think). yeah, that'd be part of the regulator-gpio with the startup-delay-ns property (IIRC) > > > only if the /dev/ttyXX interface to the UART is open. The device is a > > > bluetooth transceiver. > > > > considering this is a BTUART device, why didn't you do this at the ldisc > > level ? hci_uart_open() sounds like a good choice from a quick thinking. > > > > I'll have a look into that, thanks. so, Ack for $subject or not ?
On Wed, 23 Apr 2014 20:43:34 -0500 Felipe Balbi <balbi@ti.com> wrote: > so, Ack for $subject or not ? > Just at the moment I'm finding it hard to care. So Acked-by: NeilBrown <neilb@suse.de> Whatever.... NeilBrown
> > But I don't have discrete hardware. I have a bunch of stuff soldered onto a > > board with ad-hoc connections chosen to make the life of the hardware builder > > easy rather than chosen to make the life of the software developer easy > > (which I think is the correct choice). > > > > So I need to tell DT "This device is plugged into this UART, and there is no > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then > > I need that regulator of there turned on". or maybe "I need to toggle this > > GPIO with exactly this pattern, while watching that GPIO to see if it is > > working". > > > > So I thought: > > > > 1/ give the UART a "virtual" DTR so it could drive any GPIO > > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio > > and responded to state changes on that GPIO by turning on or off the > > regulator > > 3/ create a dedicated driver which knows how to toggle the magic GPIO while > > watching the other GPIO to convince the silly device to wakeup, or go to > > sleep, as required, and have this appear as a (virtual) GPIO. Unless you are using it as a "real' DTR line then I think this is the wrong approach. This problem actually is turning up in both PC class and ARM boxes now all over the place for three reasons I am seeing. 1. We are getting a lot of hardware moving to serial attached bluetooth/gps/etc because of the power win. In addition ACPI can describe power relationships and what is on the other end of a UART embedded in the device 2. We've got cheap hardware with modem lines being "retrofitted" via gpio 3. There are a subset of devices that have extra control lines beyond the usual serial port ones. These range from additional control lines for things like smartcard programmers to port muxing. > I have no problem either way, just that unused code doesn't have to be > sitting in the tree and I'm not entirely sure this GPIO should be > handled by omap-serial.c, perhaps something more generic inside > serial-core so other UART drivers can benefit from it. serial-core provides power hooks. If the goal is that this comes on when you power up the uart and goes away on the last close then the hooks are there already. If its ldisc specific then the tty layer also calls the device on ldisc changes precisely so it can make hardware specific twiddles in such cases. A set of gpios on the tty_port object would not go amiss and would also address the PC case. > considering this is a BTUART device, why didn't you do this at the ldisc > level ? hci_uart_open() sounds like a good choice from a quick thinking. ldiscs are hardware independent. Nothing about hardware belongs in an ldisc. Any ldisc should within reason work on any port. What I propsed when it came up ages ago was to expose some gpio settings in the tty, to provide ways for them to be set by default and also ioctls to configure them. I still think this (but moved into the tty_port as its a persistent hardware property) is a good idea now that we are starting to see more use cases than weird dongles and muxes on pre-production reference boards. With my tty and serial hat on I think a power gpio is a no-brainer even without doing the other work and therefore it should probably be described generically for a serial port in the DT as well as in the ACPI data. If you should also be able to give it a regulator to use as well that also seems to make complete sense. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 24 Apr 2014 15:19:14 +0100 One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote: > > > But I don't have discrete hardware. I have a bunch of stuff soldered onto a > > > board with ad-hoc connections chosen to make the life of the hardware builder > > > easy rather than chosen to make the life of the software developer easy > > > (which I think is the correct choice). > > > > > > So I need to tell DT "This device is plugged into this UART, and there is no > > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then > > > I need that regulator of there turned on". or maybe "I need to toggle this > > > GPIO with exactly this pattern, while watching that GPIO to see if it is > > > working". > > > > > > So I thought: > > > > > > 1/ give the UART a "virtual" DTR so it could drive any GPIO > > > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio > > > and responded to state changes on that GPIO by turning on or off the > > > regulator > > > 3/ create a dedicated driver which knows how to toggle the magic GPIO while > > > watching the other GPIO to convince the silly device to wakeup, or go to > > > sleep, as required, and have this appear as a (virtual) GPIO. > > Unless you are using it as a "real' DTR line then I think this is the > wrong approach. This problem actually is turning up in both PC class and > ARM boxes now all over the place for three reasons I am seeing. > > 1. We are getting a lot of hardware moving to serial attached > bluetooth/gps/etc because of the power win. In addition ACPI can describe > power relationships and what is on the other end of a UART embedded in > the device > > 2. We've got cheap hardware with modem lines being "retrofitted" via gpio > > 3. There are a subset of devices that have extra control lines beyond the > usual serial port ones. These range from additional control lines for > things like smartcard programmers to port muxing. > > > I have no problem either way, just that unused code doesn't have to be > > sitting in the tree and I'm not entirely sure this GPIO should be > > handled by omap-serial.c, perhaps something more generic inside > > serial-core so other UART drivers can benefit from it. > > serial-core provides power hooks. If the goal is that this comes on when > you power up the uart and goes away on the last close then the hooks are > there already. Could you be a bit more explicit, or point to an example user of these hooks? I had a quick look and I guess that uart_change_pm() is the most likely candidate for what you are referring to. I can see how that interfaces to the specific piece of uart hardware, such as omap-serial.c But I cannot see how you would plumb that though to the device that was plugged in to the serial port. I guess if that device could be registered as a child of the omap_serial device, then power management inheritance might come in to play, but I cannot see any way to tell a serial port that it has some arbitrary child device. So maybe I'm missing something. > If its ldisc specific then the tty layer also calls the > device on ldisc changes precisely so it can make hardware specific > twiddles in such cases. > > A set of gpios on the tty_port object would not go amiss and would also > address the PC case. > > > considering this is a BTUART device, why didn't you do this at the ldisc > > level ? hci_uart_open() sounds like a good choice from a quick thinking. > > ldiscs are hardware independent. Nothing about hardware belongs in an > ldisc. Any ldisc should within reason work on any port. > > What I propsed when it came up ages ago was to expose some gpio settings > in the tty, to provide ways for them to be set by default and also ioctls > to configure them. I still think this (but moved into the tty_port as its > a persistent hardware property) is a good idea now that we are starting > to see more use cases than weird dongles and muxes on pre-production > reference boards. > > With my tty and serial hat on I think a power gpio is a no-brainer even > without doing the other work and therefore it should probably be described > generically for a serial port in the DT as well as in the ACPI data. If > you should also be able to give it a regulator to use as well that also > seems to make complete sense. In one case the "power on" sequence is substantially more complex that just a gpio or regulator. I would need to write a device driver for the (GPS) chip which could receive a poweron/poweroff signal from the uart and do the required magic. Having serial-core know about gpios and regulators and random other stuff seems wrong. I would really like to see the "runtime interpreted power sequences" become a real thing. Then serial-core could activate a "RIPS", and that would have the flexibility to do whatever is needed without adding complexity to serial-core. Using a virtual GPIO was my poor-mans RIPS. Using gpiolib, and driver can pretend to be a gpio so it is a simple "pipe" to send a power-on/power-off signal over. So ... with your "serial" hat on, if I were to write/test a patch to allow any serial port to have a "power-gpio" described in DT and the gpio would be driven in uart_change_pm(), would you consider accepting that patch, or did I misunderstand? Thanks, NeilBrown
On Fri, Apr 25, 2014 at 11:34 AM, NeilBrown <neilb@suse.de> wrote: > On Thu, 24 Apr 2014 15:19:14 +0100 One Thousand Gnomes > <gnomes@lxorguk.ukuu.org.uk> wrote: > >> > > But I don't have discrete hardware. I have a bunch of stuff soldered onto a >> > > board with ad-hoc connections chosen to make the life of the hardware builder >> > > easy rather than chosen to make the life of the software developer easy >> > > (which I think is the correct choice). >> > > >> > > So I need to tell DT "This device is plugged into this UART, and there is no >> > > DTR line, but when the UARTs DTR line would be asserted (if it had one), then >> > > I need that regulator of there turned on". or maybe "I need to toggle this >> > > GPIO with exactly this pattern, while watching that GPIO to see if it is >> > > working". >> > > >> > > So I thought: >> > > >> > > 1/ give the UART a "virtual" DTR so it could drive any GPIO >> > > 2/ create a "gpio-to-regulator" driver which presented as a (virtual) gpio >> > > and responded to state changes on that GPIO by turning on or off the >> > > regulator >> > > 3/ create a dedicated driver which knows how to toggle the magic GPIO while >> > > watching the other GPIO to convince the silly device to wakeup, or go to >> > > sleep, as required, and have this appear as a (virtual) GPIO. >> >> Unless you are using it as a "real' DTR line then I think this is the >> wrong approach. This problem actually is turning up in both PC class and >> ARM boxes now all over the place for three reasons I am seeing. >> >> 1. We are getting a lot of hardware moving to serial attached >> bluetooth/gps/etc because of the power win. In addition ACPI can describe >> power relationships and what is on the other end of a UART embedded in >> the device >> >> 2. We've got cheap hardware with modem lines being "retrofitted" via gpio >> >> 3. There are a subset of devices that have extra control lines beyond the >> usual serial port ones. These range from additional control lines for >> things like smartcard programmers to port muxing. >> >> > I have no problem either way, just that unused code doesn't have to be >> > sitting in the tree and I'm not entirely sure this GPIO should be >> > handled by omap-serial.c, perhaps something more generic inside >> > serial-core so other UART drivers can benefit from it. >> >> serial-core provides power hooks. If the goal is that this comes on when >> you power up the uart and goes away on the last close then the hooks are >> there already. > > Could you be a bit more explicit, or point to an example user of these hooks? > > I had a quick look and I guess that uart_change_pm() is the most likely > candidate for what you are referring to. > I can see how that interfaces to the specific piece of uart hardware, such as > omap-serial.c > But I cannot see how you would plumb that though to the device that was > plugged in to the serial port. I guess if that device could be registered as > a child of the omap_serial device, then power management inheritance might > come in to play, but I cannot see any way to tell a serial port that it has > some arbitrary child device. > > So maybe I'm missing something. > >> If its ldisc specific then the tty layer also calls the >> device on ldisc changes precisely so it can make hardware specific >> twiddles in such cases. >> >> A set of gpios on the tty_port object would not go amiss and would also >> address the PC case. >> >> > considering this is a BTUART device, why didn't you do this at the ldisc >> > level ? hci_uart_open() sounds like a good choice from a quick thinking. >> >> ldiscs are hardware independent. Nothing about hardware belongs in an >> ldisc. Any ldisc should within reason work on any port. >> >> What I propsed when it came up ages ago was to expose some gpio settings >> in the tty, to provide ways for them to be set by default and also ioctls >> to configure them. I still think this (but moved into the tty_port as its >> a persistent hardware property) is a good idea now that we are starting >> to see more use cases than weird dongles and muxes on pre-production >> reference boards. >> >> With my tty and serial hat on I think a power gpio is a no-brainer even >> without doing the other work and therefore it should probably be described >> generically for a serial port in the DT as well as in the ACPI data. If >> you should also be able to give it a regulator to use as well that also >> seems to make complete sense. > > In one case the "power on" sequence is substantially more complex that just a > gpio or regulator. I would need to write a device driver for the (GPS) chip > which could receive a poweron/poweroff signal from the uart and do the > required magic. > > Having serial-core know about gpios and regulators and random other stuff > seems wrong. > I would really like to see the "runtime interpreted power sequences" become a > real thing. Then serial-core could activate a "RIPS", and that would have > the flexibility to do whatever is needed without adding complexity to > serial-core. > Using a virtual GPIO was my poor-mans RIPS. Using gpiolib, and driver can > pretend to be a gpio so it is a simple "pipe" to send a power-on/power-off > signal over. > > So ... with your "serial" hat on, if I were to write/test a patch to allow > any serial port to have a "power-gpio" described in DT and the gpio would be > driven in uart_change_pm(), would you consider accepting that patch, or did I > misunderstand? As soon as this patch (http://www.spinics.net/lists/arm-kernel/msg325197.html) will be applied, we don't really need this DTR GPIO any more. DTR_active is the only stuff, that is missing. Yegor -- To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> As soon as this patch > (http://www.spinics.net/lists/arm-kernel/msg325197.html) will be > applied, we don't really need this DTR GPIO any more. For the omap specific case, not for the general case of sorting out power hierarchies. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
> I had a quick look and I guess that uart_change_pm() is the most likely > candidate for what you are referring to. > I can see how that interfaces to the specific piece of uart hardware, such as > omap-serial.c > But I cannot see how you would plumb that though to the device that was > plugged in to the serial port. I guess if that device could be registered as > a child of the omap_serial device, then power management inheritance might > come in to play, but I cannot see any way to tell a serial port that it has > some arbitrary child device. If your device is powered by something like a regulator then you can drive the regulator from the uart_pm helpers. > In one case the "power on" sequence is substantially more complex that just a > gpio or regulator. I would need to write a device driver for the (GPS) chip > which could receive a poweron/poweroff signal from the uart and do the > required magic. In which case giving the tty a child device (or devices) would probably be more sensible yes. No problem with that either. > I would really like to see the "runtime interpreted power sequences" become a > real thing. Then serial-core could activate a "RIPS", and that would have > the flexibility to do whatever is needed without adding complexity to > serial-core. Something like ACPI has you mean ? When we put the device into D0 the ACPI methods can do stuff. > So ... with your "serial" hat on, if I were to write/test a patch to allow > any serial port to have a "power-gpio" described in DT and the gpio would be > driven in uart_change_pm(), would you consider accepting that patch, or did I > misunderstand? We are going to need it anyway for other devices fairly soon. It's common for new PC class hardware to have gpio management for the bluetooth, gps and and sometimes sensor devices attached to the tty. That's true irrespective of whether you happen to choose to use it for virtual gpio hacks. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c index b46aaf3..6654682 100644 --- a/drivers/tty/serial/omap-serial.c +++ b/drivers/tty/serial/omap-serial.c @@ -163,10 +163,6 @@ struct uart_omap_port { u8 wakeups_enabled; u32 features; - int DTR_gpio; - int DTR_inverted; - int DTR_active; - struct serial_rs485 rs485; int rts_gpio; @@ -694,16 +690,6 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl) serial_out(up, UART_MCR, up->mcr); pm_runtime_mark_last_busy(up->dev); pm_runtime_put_autosuspend(up->dev); - - if (gpio_is_valid(up->DTR_gpio) && - !!(mctrl & TIOCM_DTR) != up->DTR_active) { - up->DTR_active = !up->DTR_active; - if (gpio_cansleep(up->DTR_gpio)) - schedule_work(&up->qos_work); - else - gpio_set_value(up->DTR_gpio, - up->DTR_active != up->DTR_inverted); - } } static void serial_omap_break_ctl(struct uart_port *port, int break_state) @@ -847,9 +833,6 @@ static void serial_omap_uart_qos_work(struct work_struct *work) qos_work); pm_qos_update_request(&up->pm_qos_request, up->latency); - if (gpio_is_valid(up->DTR_gpio)) - gpio_set_value_cansleep(up->DTR_gpio, - up->DTR_active != up->DTR_inverted); } static void @@ -1672,28 +1655,6 @@ static int serial_omap_probe(struct platform_device *pdev) if (IS_ERR(base)) return PTR_ERR(base); - if (gpio_is_valid(omap_up_info->DTR_gpio) && - omap_up_info->DTR_present) { - ret = devm_gpio_request(&pdev->dev, omap_up_info->DTR_gpio, - "omap-serial"); - if (ret < 0) - return ret; - ret = gpio_direction_output(omap_up_info->DTR_gpio, - omap_up_info->DTR_inverted); - if (ret < 0) - return ret; - } - - if (gpio_is_valid(omap_up_info->DTR_gpio) && - omap_up_info->DTR_present) { - up->DTR_gpio = omap_up_info->DTR_gpio; - up->DTR_inverted = omap_up_info->DTR_inverted; - } else { - up->DTR_gpio = -EINVAL; - } - - up->DTR_active = 0; - up->dev = &pdev->dev; up->port.dev = &pdev->dev; up->port.type = PORT_OMAP;
nobody passes a DTR_gpio to this driver, so this code is not necessary. Signed-off-by: Felipe Balbi <balbi@ti.com> --- drivers/tty/serial/omap-serial.c | 39 --------------------------------------- 1 file changed, 39 deletions(-)