LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Johan Hovold <johan@kernel.org>
To: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Cc: Johan Hovold <johan@kernel.org>,
	linux-serial@vger.kernel.org,
	One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>,
	Florian Fainelli <f.fainelli@gmail.com>,
	Pavel Machek <pavel@ucw.cz>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-kernel@vger.kernel.org, kernel@pengutronix.de,
	linux-arm-kernel@lists.infradead.org,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v2] tty: implement led triggers
Date: Mon, 7 May 2018 11:27:10 +0200	[thread overview]
Message-ID: <20180507092710.GQ2285@localhost> (raw)
In-Reply-To: <20180507084127.ekpd3ze2itkzo7fd@pengutronix.de>

On Mon, May 07, 2018 at 10:41:27AM +0200, Uwe Kleine-König wrote:
> Hello Johan,
> 
> thanks for your feedback.
> 
> On Mon, May 07, 2018 at 10:02:52AM +0200, Johan Hovold wrote:
> > On Thu, May 03, 2018 at 10:19:52PM +0200, Uwe Kleine-König wrote:
> > > The rx trigger fires when data is pushed to the ldisc. 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.
> > > 
> > > Similarily the tx trigger fires when data taken from the ldisc.
> > 
> > You meant copied from user space, or written to the ldisc, here.
> 
> ack.
> 
> > Note that the rx-path is shared with serdev, but the write path is not.
> > So with this series, serdev devices would only trigger the rx-led.
> 
> Where would be the right place to put the tx trigger to catch serdev,
> too?

I haven't given this much thought, but do we really want this for serdev
at all? I'm thinking whatever driver or subsystem is using serdev should
have their own triggers (e.g. bluetooth or net).

So it might be better to move the rx-blinking to the default tty-port
client receive_buf callback instead (i.e.
tty_port_default_receive_buf()).

And then the resource allocations would need to go after the serdev
registration in tty_port_register_device_attr_serdev().

> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Changes since v1, sent with Message-Id: 
> > > 20180503100448.1350-1-u.kleine-koenig@pengutronix.de:
> > > 
> > >  - implement tx trigger;
> > >  - introduce Kconfig symbol for conditional compilation;
> > >  - set trigger to NULL if allocating the name failed to not free random
> > >    pointers in case the port struct wasn't zeroed;
> > >  - use if/else instead of goto
> > 
> > > @@ -499,6 +500,7 @@ static void flush_to_ldisc(struct work_struct *work)
> > >  		struct tty_buffer *head = buf->head;
> > >  		struct tty_buffer *next;
> > >  		int count;
> > > +		unsigned long delay = 50 /* ms */;
> > 
> > Comment after the semicolon?
> 
> Given that this comment is about the 50 and not the delay member, I
> prefer it before the ;.

Hmm. I personally find it hard to read and can only find about 30
instances of this comment style (for assignments) in the kernel. And
arguably the comment applies equally well to the delay variable in this
case too.

> > Besides the ugly ifdefs here, you're leaking the above LED resources in
> > the error paths below (e.g. on probe deferrals).

> ack, the ifdevs are ugly. I'm working on an idea to get rid of them.

Sounds good.

Thanks,
Johan

  reply	other threads:[~2018-05-07  9:27 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-03 10:04 [PATCH] tty: implement a rx led trigger Uwe Kleine-König
2018-05-03 10:10 ` Pavel Machek
2018-05-03 11:52   ` Uwe Kleine-König
2018-05-03 12:33 ` Robin Murphy
2018-05-03 20:19   ` [PATCH v2] tty: implement led triggers Uwe Kleine-König
2018-05-04  0:29     ` kbuild test robot
2018-05-07  8:02     ` Johan Hovold
2018-05-07  8:41       ` Uwe Kleine-König
2018-05-07  9:27         ` Johan Hovold [this message]
2018-05-10 11:14           ` Pavel Machek
2018-05-10 11:25             ` Robin Murphy
2018-05-12 19:00               ` Uwe Kleine-König

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180507092710.GQ2285@localhost \
    --to=johan@kernel.org \
    --cc=f.fainelli@gmail.com \
    --cc=gnomes@lxorguk.ukuu.org.uk \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=pavel@ucw.cz \
    --cc=robin.murphy@arm.com \
    --cc=u.kleine-koenig@pengutronix.de \
    --subject='Re: [PATCH v2] tty: implement led triggers' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).