Hello Johan,
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.
> 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).
> [...]
I agree to all of your comments that I stripped and will address them in
the next round.
> > @@ -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.).
> 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.
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
