Hello Johan,
On Tue, May 22, 2018 at 11:51:41AM +0200, Johan Hovold wrote:
> On Thu, May 17, 2018 at 08:16:25AM +0200, Uwe Kleine-König wrote:
> > On Wed, May 16, 2018 at 09:54:54AM +0200, Johan Hovold wrote:
> > > On Mon, May 14, 2018 at 12:15:05PM +0200, Uwe Kleine-König wrote:
> > > > The rx trigger fires when data is pushed to the ldisc by the driver. This
> > > > is a bit later than the actual receiving of data but has the nice benefit
> > > > that it doesn't need adaption for each driver and isn't in the hot path.
> > > >
> > > > Similarly the tx trigger fires when data was copied from userspace and is
> > > > given to the ldisc.
> > >
> > > This also has the negative effect of having the blink rate (and
> > > pattern) be dependent on the size of the buffers provided by userspace
> > > however.
> > >
> > > With buffers of 2048 bytes or more and at 115200 baud you get a 178 ms
> > > period (rather than the "default" 100 ms used for smaller buffers and
> > > high-enough baudrates) and this drops to 2 seconds and 68 second periods
> > > for 9600 and 300 baud respectively.
> >
> > that is something I wouldn't care about. I think nobody will want to
> > determine the transfer rate from the emitted light.
>
> Perhaps not, but it's an unfortunate effect of the choice of moving this
> into the tty layer instead of having the lower level drivers implement
> this, which I at least think you need to mention.
the observability is the same, independent of where the trigger fires,
isn't it?
> Personally I don't understand this obsession with blinking lights at
> all, but if this is to be supported we should at least try to do it the
> right way (after determining what that is).
>
> > > Also note that by hooking into do_tty_write, for UARTs used for consoles
> > > there will be no blink on console output, only for console input...
> >
> > I tested the patch with the trigger for the console device. I didn't
> > test if kernel messages trigger the trigger, but the shell running on
> > the console device does (e.g. when running seq 1 10000).
>
> You're right, a getty would open the tty device directly so then you'd
> get blinks in both directions in a login shell.
>
> But printk messages won't trigger the led as consoles use a different
> write path.
I'm willing to sell this as an advantage :-)
> > > > @@ -163,8 +165,31 @@ struct device *tty_port_register_device_attr_serdev(struct tty_port *port,
> > > > return dev;
> > > > }
> > > >
> > > > - return tty_register_device_attr(driver, index, device, drvdata,
> > > > - attr_grp);
> > > > + if (IS_ENABLED(CONFIG_TTY_LEDS_TRIGGERS)) {
> > >
> > > This doesn't really improve readability much. Use compile guards in the
> > > header file to provide dummy implementations for setup/teardown
> > > functions when the trigger isn't selected. And yes, the actual
> > > implementation may require ifdefs around them in the c-file as well.
> >
> > I like this better than cpp stuff because it respects the indention of
> > the C source. This is also supported by Documentation/process/coding-style.rst.
> > (Well, I have admit that it's not impossible to interpret the document
> > to prefer dummy implementations over IS_ENABLED, but what I learned from
> > reviews to other contributions I expected this to be the commonly
> > accepted and expected way.).
>
> Yeah, and I think the following paragraph is pretty clear on this:
>
> Prefer to compile out entire functions, rather than portions of
> functions or portions of expressions. Rather than putting an
> ifdef in an expression, factor out part or all of the expression
> into a separate helper function and apply the conditional to
> that function.
I understand "Prefer to compile out" as "Prefer to compile out using #if
et al", so it doesn't cover if (IS_ENABLED(...)). The section title is
"Conditional Compilation" which supports my interpretation and there is
no "ifdef in any affected expression". The nice thing about IS_ENABLED
is that it can be indented naturally without looking strange. And
compared to #if it still ensures that the block is looked at by the
compiler even when the condition is false, which is also a nice
property.
> And in this case I really don't want to see whatever magic is done to
> register a led trigger so factoring that out into helper function allows
> the tty code to remain clean. You also shouldn't keep those led triggers
> around in struct tty_port when !CONFIG_TTY_LEDS_TRIGGERS.
Having them even without CONFIG_TTY_LEDS_TRIGGERS helps to have to use
less #ifdef in the code. Without CONFIG_LEDS_TRIGGER sizeof(struct
led_trigger) is 0, which is an IMHO fine construct.
> > > Also note that you need to add this to tty_port_register_device() as
> > > well... Serdev is currently opt-in and only enabled for serial core (due
> > > to some lifetime issues and issues related to hotplug), but there's no
> > > reason to limit these triggers to serial core.
> >
> > I cannot follow this immediatly. Will study the code a bit and then
> > probably understand.
>
> There are (at least) three ways to register a tty device of which two
> set up the port link at registration and one where the tty port link is
> setup when the device is opened:
>
> tty_port_register_device_attr()
> tty_port_register_device_attr_serdev()
> tty_register_device_attr()
>
> The tty_port_register_device_attr_serdev() is supposed to be a temporary
> solution until we can enable serdev for all tty drivers that register
> their devices along the their ports. But for now, you'd probably want to
> register led triggers in both of the tty_port_register-functions.
OK, I will take another look.
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
