[PATCH v4 0/3] ledtrig-tty: add additional tty state evaluation

[PATCH v4 0/3] ledtrig-tty: add additional tty state evaluation

[Florian Eckert]

Changes in v4:
- Merging patch 3/4 into patch number 4/4 from previous series, because
  it fixes a problem that does not exist upstream. This was a note from
  the build robot regarding my change that I added with previous series.
  This change was never upstream and therefore this is not relevant.
- Update the commit message of patch 1/3 of this series, that this commit
  also changes the 'ndashes' to simple dashes. There were no changes, so
  I add the 'Reviewed-by' that the commit received before.
- With this patchset version I have reworked my implementation for the
  evaluation of the additional line state, so that this changes becomes
  smaller. As basis I have used the staged commits from Christian Marangi
  that makes this changes to the netdev trigger. This has already been
  applied to 'for-leds-next-next' by Lee Jones. I adapted this to the
  tty trigger.
  Convert device attr to macro:
  https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=509412749002f4bac4c29f2012fff90c08d8afca
  Unify sysfs and state handling:
  https://git.kernel.org/pub/scm/linux/kernel/git/lee/leds.git/commit/drivers/leds/trigger?h=for-leds-next-next&id=0fd93ac8582627bee9a3c824489f302dff722881

Changes in v3:
- Add missing 'kernel test robot' information to the commit message.
- Additional information added to the commit message

Changes in v2:
- rename new function from tty_get_mget() to tty_get_tiocm() as
  requested by 'Jiri Slaby'.
- As suggested by 'Jiri Slaby', fixed tabs in function documentation
  throughout the file '/drivers/tty/tty_io.c' in a separate commit.
- Move the variable definition to the top in function
  'ledtrig_tty_work()'.
  This was reported by the 'kernel test robot' after my change in v1.
- Also set the 'max_brightness' to 'blink_brightness' if no
  'blink_brightness' was set. This fixes a problem at startup when the
  brightness is still set to 0 and only 'line_*' is evaluated. I looked
  in the netdev trigger and that's exactly how it's done there.

v1:
This is a follow-up patchset, based on the mailing list discussion from
March 2023 based on the old patchset v8 [1]. I have changed, the LED
trigger handling via the sysfs interfaces as suggested by Uwe Kleine-König.
[1] https://lore.kernel.org/linux-leds/20230306094113.273988-1-fe@dev.tdt.de/

Florian Eckert (3):
  tty: whitespaces in descriptions corrected by replacing tabs with
    spaces
  tty: add new helper function tty_get_tiocm
  leds: ledtrig-tty: add new line mode evaluation

 .../ABI/testing/sysfs-class-led-trigger-tty   |  54 +++++
 drivers/leds/trigger/ledtrig-tty.c            | 192 +++++++++++++++++-
 drivers/tty/tty_io.c                          | 130 ++++++------
 include/linux/tty.h                           |   1 +
 4 files changed, 309 insertions(+), 68 deletions(-)

-- 
2.30.2

[PATCH v4 1/3] tty: whitespaces in descriptions corrected by replacing tabs with spaces

[Florian Eckert]

Tabs were used in the function description, to make this look more
uniform, the tabs were replaced by spaces where necessary.

While we're at it, I also replaced the 'ndashes' with simple dashes, since
only those are supported by sphinx.

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/tty/tty_io.c | 102 +++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 51 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 8a94e5a43c6d..3299a5d50727 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -159,7 +159,7 @@ static int tty_fasync(int fd, struct file *filp, int on);
 static void release_tty(struct tty_struct *tty, int idx);
 
 /**
- * free_tty_struct	-	free a disused tty
+ * free_tty_struct - free a disused tty
  * @tty: tty struct to free
  *
  * Free the write buffers, tty queue and tty memory itself.
@@ -233,7 +233,7 @@ static void tty_del_file(struct file *file)
 }
 
 /**
- * tty_name	-	return tty naming
+ * tty_name - return tty naming
  * @tty: tty structure
  *
  * Convert a tty structure into a name. The name reflects the kernel naming
@@ -295,7 +295,7 @@ static void check_tty_count(struct tty_struct *tty, const char *routine)
 }
 
 /**
- * get_tty_driver		-	find device of a tty
+ * get_tty_driver - find device of a tty
  * @device: device identifier
  * @index: returns the index of the tty
  *
@@ -320,7 +320,7 @@ static struct tty_driver *get_tty_driver(dev_t device, int *index)
 }
 
 /**
- * tty_dev_name_to_number	-	return dev_t for device name
+ * tty_dev_name_to_number - return dev_t for device name
  * @name: user space name of device under /dev
  * @number: pointer to dev_t that this function will populate
  *
@@ -372,7 +372,7 @@ EXPORT_SYMBOL_GPL(tty_dev_name_to_number);
 #ifdef CONFIG_CONSOLE_POLL
 
 /**
- * tty_find_polling_driver	-	find device of a polled tty
+ * tty_find_polling_driver - find device of a polled tty
  * @name: name string to match
  * @line: pointer to resulting tty line nr
  *
@@ -505,7 +505,7 @@ static DEFINE_SPINLOCK(redirect_lock);
 static struct file *redirect;
 
 /**
- * tty_wakeup	-	request more data
+ * tty_wakeup - request more data
  * @tty: terminal
  *
  * Internal and external helper for wakeups of tty. This function informs the
@@ -529,7 +529,7 @@ void tty_wakeup(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_wakeup);
 
 /**
- * tty_release_redirect	-	Release a redirect on a pty if present
+ * tty_release_redirect - Release a redirect on a pty if present
  * @tty: tty device
  *
  * This is available to the pty code so if the master closes, if the slave is a
@@ -550,7 +550,7 @@ static struct file *tty_release_redirect(struct tty_struct *tty)
 }
 
 /**
- * __tty_hangup		-	actual handler for hangup events
+ * __tty_hangup - actual handler for hangup events
  * @tty: tty device
  * @exit_session: if non-zero, signal all foreground group processes
  *
@@ -673,7 +673,7 @@ static void do_tty_hangup(struct work_struct *work)
 }
 
 /**
- * tty_hangup		-	trigger a hangup event
+ * tty_hangup - trigger a hangup event
  * @tty: tty to hangup
  *
  * A carrier loss (virtual or otherwise) has occurred on @tty. Schedule a
@@ -687,7 +687,7 @@ void tty_hangup(struct tty_struct *tty)
 EXPORT_SYMBOL(tty_hangup);
 
 /**
- * tty_vhangup		-	process vhangup
+ * tty_vhangup - process vhangup
  * @tty: tty to hangup
  *
  * The user has asked via system call for the terminal to be hung up. We do
@@ -703,7 +703,7 @@ EXPORT_SYMBOL(tty_vhangup);
 
 
 /**
- * tty_vhangup_self	-	process vhangup for own ctty
+ * tty_vhangup_self - process vhangup for own ctty
  *
  * Perform a vhangup on the current controlling tty
  */
@@ -719,7 +719,7 @@ void tty_vhangup_self(void)
 }
 
 /**
- * tty_vhangup_session	-	hangup session leader exit
+ * tty_vhangup_session - hangup session leader exit
  * @tty: tty to hangup
  *
  * The session leader is exiting and hanging up its controlling terminal.
@@ -735,7 +735,7 @@ void tty_vhangup_session(struct tty_struct *tty)
 }
 
 /**
- * tty_hung_up_p	-	was tty hung up
+ * tty_hung_up_p - was tty hung up
  * @filp: file pointer of tty
  *
  * Return: true if the tty has been subject to a vhangup or a carrier loss
@@ -756,7 +756,7 @@ void __stop_tty(struct tty_struct *tty)
 }
 
 /**
- * stop_tty	-	propagate flow control
+ * stop_tty - propagate flow control
  * @tty: tty to stop
  *
  * Perform flow control to the driver. May be called on an already stopped
@@ -790,7 +790,7 @@ void __start_tty(struct tty_struct *tty)
 }
 
 /**
- * start_tty	-	propagate flow control
+ * start_tty - propagate flow control
  * @tty: tty to start
  *
  * Start a tty that has been stopped if at all possible. If @tty was previously
@@ -898,7 +898,7 @@ static ssize_t iterate_tty_read(struct tty_ldisc *ld, struct tty_struct *tty,
 
 
 /**
- * tty_read	-	read method for tty device files
+ * tty_read - read method for tty device files
  * @iocb: kernel I/O control block
  * @to: destination for the data read
  *
@@ -1091,7 +1091,7 @@ static ssize_t file_tty_write(struct file *file, struct kiocb *iocb, struct iov_
 }
 
 /**
- * tty_write		-	write method for tty device file
+ * tty_write - write method for tty device file
  * @iocb: kernel I/O control block
  * @from: iov_iter with data to write
  *
@@ -1133,7 +1133,7 @@ ssize_t redirected_tty_write(struct kiocb *iocb, struct iov_iter *iter)
 }
 
 /**
- * tty_send_xchar	-	send priority character
+ * tty_send_xchar - send priority character
  * @tty: the tty to send to
  * @ch: xchar to send
  *
@@ -1167,7 +1167,7 @@ int tty_send_xchar(struct tty_struct *tty, char ch)
 }
 
 /**
- * pty_line_name	-	generate name for a pty
+ * pty_line_name - generate name for a pty
  * @driver: the tty driver in use
  * @index: the minor number
  * @p: output buffer of at least 6 bytes
@@ -1188,7 +1188,7 @@ static void pty_line_name(struct tty_driver *driver, int index, char *p)
 }
 
 /**
- * tty_line_name	-	generate name for a tty
+ * tty_line_name - generate name for a tty
  * @driver: the tty driver in use
  * @index: the minor number
  * @p: output buffer of at least 7 bytes
@@ -1239,7 +1239,7 @@ static struct tty_struct *tty_driver_lookup_tty(struct tty_driver *driver,
 }
 
 /**
- * tty_init_termios	-  helper for termios setup
+ * tty_init_termios - helper for termios setup
  * @tty: the tty to set up
  *
  * Initialise the termios structure for this tty. This runs under the
@@ -1322,7 +1322,7 @@ static void tty_driver_remove_tty(struct tty_driver *driver, struct tty_struct *
 }
 
 /**
- * tty_reopen()	- fast re-open of an open tty
+ * tty_reopen() - fast re-open of an open tty
  * @tty: the tty to open
  *
  * Re-opens on master ptys are not allowed and return -%EIO.
@@ -1366,7 +1366,7 @@ static int tty_reopen(struct tty_struct *tty)
 }
 
 /**
- * tty_init_dev		-	initialise a tty device
+ * tty_init_dev - initialise a tty device
  * @driver: tty driver we are opening a device on
  * @idx: device index
  *
@@ -1488,7 +1488,7 @@ void tty_save_termios(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_save_termios);
 
 /**
- * tty_flush_works	-	flush all works of a tty/pty pair
+ * tty_flush_works - flush all works of a tty/pty pair
  * @tty: tty device to flush works for (or either end of a pty pair)
  *
  * Sync flush all works belonging to @tty (and the 'other' tty).
@@ -1504,7 +1504,7 @@ static void tty_flush_works(struct tty_struct *tty)
 }
 
 /**
- * release_one_tty	-	release tty structure memory
+ * release_one_tty - release tty structure memory
  * @work: work of tty we are obliterating
  *
  * Releases memory associated with a tty structure, and clears out the
@@ -1552,7 +1552,7 @@ static void queue_release_one_tty(struct kref *kref)
 }
 
 /**
- * tty_kref_put		-	release a tty kref
+ * tty_kref_put - release a tty kref
  * @tty: tty device
  *
  * Release a reference to the @tty device and if need be let the kref layer
@@ -1566,7 +1566,7 @@ void tty_kref_put(struct tty_struct *tty)
 EXPORT_SYMBOL(tty_kref_put);
 
 /**
- * release_tty		-	release tty structure memory
+ * release_tty - release tty structure memory
  * @tty: tty device release
  * @idx: index of the tty device release
  *
@@ -1643,7 +1643,7 @@ static int tty_release_checks(struct tty_struct *tty, int idx)
 }
 
 /**
- * tty_kclose      -       closes tty opened by tty_kopen
+ * tty_kclose - closes tty opened by tty_kopen
  * @tty: tty device
  *
  * Performs the final steps to release and free a tty device. It is the same as
@@ -1673,7 +1673,7 @@ void tty_kclose(struct tty_struct *tty)
 EXPORT_SYMBOL_GPL(tty_kclose);
 
 /**
- * tty_release_struct	-	release a tty struct
+ * tty_release_struct - release a tty struct
  * @tty: tty device
  * @idx: index of the tty
  *
@@ -1702,7 +1702,7 @@ void tty_release_struct(struct tty_struct *tty, int idx)
 EXPORT_SYMBOL_GPL(tty_release_struct);
 
 /**
- * tty_release		-	vfs callback for close
+ * tty_release - vfs callback for close
  * @inode: inode of tty
  * @filp: file pointer for handle to tty
  *
@@ -1983,7 +1983,7 @@ static struct tty_struct *tty_kopen(dev_t device, int shared)
 }
 
 /**
- * tty_kopen_exclusive	-	open a tty device for kernel
+ * tty_kopen_exclusive - open a tty device for kernel
  * @device: dev_t of device to open
  *
  * Opens tty exclusively for kernel. Performs the driver lookup, makes sure
@@ -2003,7 +2003,7 @@ struct tty_struct *tty_kopen_exclusive(dev_t device)
 EXPORT_SYMBOL_GPL(tty_kopen_exclusive);
 
 /**
- * tty_kopen_shared	-	open a tty device for shared in-kernel use
+ * tty_kopen_shared - open a tty device for shared in-kernel use
  * @device: dev_t of device to open
  *
  * Opens an already existing tty for in-kernel use. Compared to
@@ -2018,7 +2018,7 @@ struct tty_struct *tty_kopen_shared(dev_t device)
 EXPORT_SYMBOL_GPL(tty_kopen_shared);
 
 /**
- * tty_open_by_driver	-	open a tty device
+ * tty_open_by_driver - open a tty device
  * @device: dev_t of device to open
  * @filp: file pointer to tty
  *
@@ -2086,7 +2086,7 @@ static struct tty_struct *tty_open_by_driver(dev_t device,
 }
 
 /**
- * tty_open	-	open a tty device
+ * tty_open - open a tty device
  * @inode: inode of device file
  * @filp: file pointer to tty
  *
@@ -2180,7 +2180,7 @@ static int tty_open(struct inode *inode, struct file *filp)
 
 
 /**
- * tty_poll	-	check tty status
+ * tty_poll - check tty status
  * @filp: file being polled
  * @wait: poll wait structures to update
  *
@@ -2258,7 +2258,7 @@ static int tty_fasync(int fd, struct file *filp, int on)
 
 static bool tty_legacy_tiocsti __read_mostly = IS_ENABLED(CONFIG_LEGACY_TIOCSTI);
 /**
- * tiocsti		-	fake input character
+ * tiocsti - fake input character
  * @tty: tty to fake input into
  * @p: pointer to character
  *
@@ -2295,7 +2295,7 @@ static int tiocsti(struct tty_struct *tty, char __user *p)
 }
 
 /**
- * tiocgwinsz		-	implement window query ioctl
+ * tiocgwinsz - implement window query ioctl
  * @tty: tty
  * @arg: user buffer for result
  *
@@ -2316,7 +2316,7 @@ static int tiocgwinsz(struct tty_struct *tty, struct winsize __user *arg)
 }
 
 /**
- * tty_do_resize	-	resize event
+ * tty_do_resize - resize event
  * @tty: tty being resized
  * @ws: new dimensions
  *
@@ -2346,7 +2346,7 @@ int tty_do_resize(struct tty_struct *tty, struct winsize *ws)
 EXPORT_SYMBOL(tty_do_resize);
 
 /**
- * tiocswinsz		-	implement window size set ioctl
+ * tiocswinsz - implement window size set ioctl
  * @tty: tty side of tty
  * @arg: user buffer for result
  *
@@ -2373,7 +2373,7 @@ static int tiocswinsz(struct tty_struct *tty, struct winsize __user *arg)
 }
 
 /**
- * tioccons	-	allow admin to move logical console
+ * tioccons - allow admin to move logical console
  * @file: the file to become console
  *
  * Allow the administrator to move the redirected console device.
@@ -2412,7 +2412,7 @@ static int tioccons(struct file *file)
 }
 
 /**
- * tiocsetd	-	set line discipline
+ * tiocsetd - set line discipline
  * @tty: tty device
  * @p: pointer to user data
  *
@@ -2434,7 +2434,7 @@ static int tiocsetd(struct tty_struct *tty, int __user *p)
 }
 
 /**
- * tiocgetd	-	get line discipline
+ * tiocgetd - get line discipline
  * @tty: tty device
  * @p: pointer to user data
  *
@@ -2457,7 +2457,7 @@ static int tiocgetd(struct tty_struct *tty, int __user *p)
 }
 
 /**
- * send_break	-	performed time break
+ * send_break - performed time break
  * @tty: device to break on
  * @duration: timeout in mS
  *
@@ -2495,7 +2495,7 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 }
 
 /**
- * tty_tiocmget		-	get modem status
+ * tty_tiocmget - get modem status
  * @tty: tty device
  * @p: pointer to result
  *
@@ -2518,7 +2518,7 @@ static int tty_tiocmget(struct tty_struct *tty, int __user *p)
 }
 
 /**
- * tty_tiocmset		-	set modem status
+ * tty_tiocmset - set modem status
  * @tty: tty device
  * @cmd: command - clear bits, set bits or set all
  * @p: pointer to desired bits
@@ -2559,7 +2559,7 @@ static int tty_tiocmset(struct tty_struct *tty, unsigned int cmd,
 }
 
 /**
- * tty_get_icount	-	get tty statistics
+ * tty_get_icount - get tty statistics
  * @tty: tty device
  * @icount: output parameter
  *
@@ -3122,7 +3122,7 @@ struct tty_struct *alloc_tty_struct(struct tty_driver *driver, int idx)
 }
 
 /**
- * tty_put_char	- write one character to a tty
+ * tty_put_char - write one character to a tty
  * @tty: tty
  * @ch: character to write
  *
@@ -3300,7 +3300,7 @@ void tty_unregister_device(struct tty_driver *driver, unsigned index)
 EXPORT_SYMBOL(tty_unregister_device);
 
 /**
- * __tty_alloc_driver -- allocate tty driver
+ * __tty_alloc_driver - allocate tty driver
  * @lines: count of lines this driver can handle at most
  * @owner: module which is responsible for this driver
  * @flags: some of %TTY_DRIVER_ flags, will be set in driver->flags
@@ -3393,7 +3393,7 @@ static void destruct_tty_driver(struct kref *kref)
 }
 
 /**
- * tty_driver_kref_put -- drop a reference to a tty driver
+ * tty_driver_kref_put - drop a reference to a tty driver
  * @driver: driver of which to drop the reference
  *
  * The final put will destroy and free up the driver.
@@ -3405,7 +3405,7 @@ void tty_driver_kref_put(struct tty_driver *driver)
 EXPORT_SYMBOL(tty_driver_kref_put);
 
 /**
- * tty_register_driver -- register a tty driver
+ * tty_register_driver - register a tty driver
  * @driver: driver to register
  *
  * Called by a tty driver to register itself.
@@ -3470,7 +3470,7 @@ int tty_register_driver(struct tty_driver *driver)
 EXPORT_SYMBOL(tty_register_driver);
 
 /**
- * tty_unregister_driver -- unregister a tty driver
+ * tty_unregister_driver - unregister a tty driver
  * @driver: driver to unregister
  *
  * Called by a tty driver to unregister itself.
-- 
2.30.2

Re: [PATCH v4 1/3] tty: whitespaces in descriptions corrected by replacing tabs with spaces

[Greg KH]

On Thu, Oct 19, 2023 at 01:28:07PM +0200, Florian Eckert wrote:
> Tabs were used in the function description, to make this look more
> uniform, the tabs were replaced by spaces where necessary.
> 
> While we're at it, I also replaced the 'ndashes' with simple dashes, since
> only those are supported by sphinx.
> 
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>

I'll just take this patch through my tty tree as that's where it
belongs, not through the led tree as there should not be any dependency
on it for any new features as you are only doing comment changes here.

thanks,

greg k-h

Re: [PATCH v4 1/3] tty: whitespaces in descriptions corrected by replacing tabs with spaces

[Greg KH]

On Thu, Oct 19, 2023 at 01:28:07PM +0200, Florian Eckert wrote:
> Tabs were used in the function description, to make this look more
> uniform, the tabs were replaced by spaces where necessary.
> 
> While we're at it, I also replaced the 'ndashes' with simple dashes, since
> only those are supported by sphinx.
> 
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  drivers/tty/tty_io.c | 102 +++++++++++++++++++++----------------------
>  1 file changed, 51 insertions(+), 51 deletions(-)

This didn't apply cleanly as portions of this patch were already in my
tree, what did tree did you make it against?

Anyway, I've fixed it up and taken it now.

thanks,

greg k-h

Re: [PATCH v4 1/3] tty: whitespaces in descriptions corrected by replacing tabs with spaces

[Florian Eckert]

On 2023-10-21 18:28, Greg KH wrote:
> On Thu, Oct 19, 2023 at 01:28:07PM +0200, Florian Eckert wrote:
>> Tabs were used in the function description, to make this look more
>> uniform, the tabs were replaced by spaces where necessary.
>> 
>> While we're at it, I also replaced the 'ndashes' with simple dashes, 
>> since
>> only those are supported by sphinx.
>> 
>> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
>> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
>> ---
>>  drivers/tty/tty_io.c | 102 
>> +++++++++++++++++++++----------------------
>>  1 file changed, 51 insertions(+), 51 deletions(-)
> 
> This didn't apply cleanly as portions of this patch were already in my
> tree, what did tree did you make it against?

I have already seen that I should add a base commit next time.
So I made it against the master from last week.

However, I was not sure which tree to use as I am changing
something in the tty and led subsystem-

> Anyway, I've fixed it up and taken it now.

Thank you for adopting my change and correcting my commit so that
it can be applied cleanly.

I will not add this patch in the next series, as it is already in the
tty-testing branch [1] from you?

---
Florian

Links:
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/tty.git/commit/?h=tty-testing&id=838eb763c3e939a8de8d4c55a17ddcce737685c1

Re: [PATCH v4 1/3] tty: whitespaces in descriptions corrected by replacing tabs with spaces

[Greg KH]

On Sun, Oct 22, 2023 at 12:24:55PM +0200, Florian Eckert wrote:
> On 2023-10-21 18:28, Greg KH wrote:
> > On Thu, Oct 19, 2023 at 01:28:07PM +0200, Florian Eckert wrote:
> > > Tabs were used in the function description, to make this look more
> > > uniform, the tabs were replaced by spaces where necessary.
> > > 
> > > While we're at it, I also replaced the 'ndashes' with simple dashes,
> > > since
> > > only those are supported by sphinx.
> > > 
> > > Reviewed-by: Jiri Slaby <jirislaby@kernel.org>
> > > Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> > > ---
> > >  drivers/tty/tty_io.c | 102
> > > +++++++++++++++++++++----------------------
> > >  1 file changed, 51 insertions(+), 51 deletions(-)
> > 
> > This didn't apply cleanly as portions of this patch were already in my
> > tree, what did tree did you make it against?
> 
> I have already seen that I should add a base commit next time.
> So I made it against the master from last week.
> 
> However, I was not sure which tree to use as I am changing
> something in the tty and led subsystem-
> 
> > Anyway, I've fixed it up and taken it now.
> 
> Thank you for adopting my change and correcting my commit so that
> it can be applied cleanly.
> 
> I will not add this patch in the next series, as it is already in the
> tty-testing branch [1] from you?

That is correct, thanks!

greg k-h

[PATCH v4 2/3] tty: add new helper function tty_get_tiocm

[Florian Eckert]

The struct 'tty_struct' has a callback to read the status flags of the tty
if the tty driver provides them. So fare, the data is transferred directly
to userspace with the function 'tty_tiocmget'. This function cannot be
used to evaluate the status line of the tty interface in the ledtrig-tty
trigger. To make this possible, a new function must be added that does
not immediately pass the data on to userspace.

The new function 'tty_get_tiocm' only returns the status register.
This information can then be processed further in the ledtrig-tty
trigger.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 drivers/tty/tty_io.c | 28 ++++++++++++++++++++++------
 include/linux/tty.h  |  1 +
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c
index 3299a5d50727..a12f63854ac4 100644
--- a/drivers/tty/tty_io.c
+++ b/drivers/tty/tty_io.c
@@ -2494,6 +2494,24 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
 	return retval;
 }
 
+/**
+ * tty_get_tiocm - get tiocm status register
+ * @tty: tty device
+ *
+ * Obtain the modem status bits from the tty driver if the feature
+ * is supported.
+ */
+int tty_get_tiocm(struct tty_struct *tty)
+{
+	int retval = -ENOTTY;
+
+	if (tty->ops->tiocmget)
+		retval = tty->ops->tiocmget(tty);
+
+	return retval;
+}
+EXPORT_SYMBOL_GPL(tty_get_tiocm);
+
 /**
  * tty_tiocmget - get modem status
  * @tty: tty device
@@ -2506,14 +2524,12 @@ static int send_break(struct tty_struct *tty, unsigned int duration)
  */
 static int tty_tiocmget(struct tty_struct *tty, int __user *p)
 {
-	int retval = -ENOTTY;
+	int retval;
 
-	if (tty->ops->tiocmget) {
-		retval = tty->ops->tiocmget(tty);
+	retval = tty_get_tiocm(tty);
+	if (retval >= 0)
+		retval = put_user(retval, p);
 
-		if (retval >= 0)
-			retval = put_user(retval, p);
-	}
 	return retval;
 }
 
diff --git a/include/linux/tty.h b/include/linux/tty.h
index f002d0f25db7..8e4d0b3b12b7 100644
--- a/include/linux/tty.h
+++ b/include/linux/tty.h
@@ -421,6 +421,7 @@ int tty_unthrottle_safe(struct tty_struct *tty);
 int tty_do_resize(struct tty_struct *tty, struct winsize *ws);
 int tty_get_icount(struct tty_struct *tty,
 		struct serial_icounter_struct *icount);
+int tty_get_tiocm(struct tty_struct *tty);
 int is_current_pgrp_orphaned(void);
 void tty_hangup(struct tty_struct *tty);
 void tty_vhangup(struct tty_struct *tty);
-- 
2.30.2

Re: [PATCH v4 2/3] tty: add new helper function tty_get_tiocm

[Greg KH]

On Thu, Oct 19, 2023 at 01:28:08PM +0200, Florian Eckert wrote:
> The struct 'tty_struct' has a callback to read the status flags of the tty
> if the tty driver provides them. So fare, the data is transferred directly
> to userspace with the function 'tty_tiocmget'. This function cannot be
> used to evaluate the status line of the tty interface in the ledtrig-tty
> trigger. To make this possible, a new function must be added that does
> not immediately pass the data on to userspace.
> 
> The new function 'tty_get_tiocm' only returns the status register.
> This information can then be processed further in the ledtrig-tty
> trigger.

Writing changelogs are hard.  You are including a lot of information in
here that really doesn't need to be, as you are focusing on your
specific use case, which is fine, but you are creating a generic
function.

This can be simpler, how about something like this:

	There is no in-kernel function to get the status register of a
	tty device like the TIOCMGET ioctl returns to userspace.  Create
	a new function, tty_get_tiocm(), to obtain the status register
	that other portions of the kernel can call if they need this
	information, and move the existing internal tty_tiocmget()
	function to use this interface.

Sound good?

The code portion looks fine to me, thanks for doing this.

greg k-h

Re: [PATCH v4 2/3] tty: add new helper function tty_get_tiocm

[Florian Eckert]

On 2023-10-21 18:15, Greg KH wrote:
> On Thu, Oct 19, 2023 at 01:28:08PM +0200, Florian Eckert wrote:
>> The struct 'tty_struct' has a callback to read the status flags of the 
>> tty
>> if the tty driver provides them. So fare, the data is transferred 
>> directly
>> to userspace with the function 'tty_tiocmget'. This function cannot be
>> used to evaluate the status line of the tty interface in the 
>> ledtrig-tty
>> trigger. To make this possible, a new function must be added that does
>> not immediately pass the data on to userspace.
>> 
>> The new function 'tty_get_tiocm' only returns the status register.
>> This information can then be processed further in the ledtrig-tty
>> trigger.
> 
> Writing changelogs are hard.  You are including a lot of information in
> here that really doesn't need to be, as you are focusing on your
> specific use case, which is fine, but you are creating a generic
> function.

Yes, that is absolutely right. I'll try to take that into account next
time, thanks for your advice.

> This can be simpler, how about something like this:
> 
> 	There is no in-kernel function to get the status register of a
> 	tty device like the TIOCMGET ioctl returns to userspace.  Create
> 	a new function, tty_get_tiocm(), to obtain the status register
> 	that other portions of the kernel can call if they need this
> 	information, and move the existing internal tty_tiocmget()
> 	function to use this interface.

I will replace the commit message with your suggestion in the next round 
of
the patch series. Thanks!

---
Florian

[PATCH v4 3/3] leds: ledtrig-tty: add new line mode evaluation

[Florian Eckert]

Until now, the LED blinks when data is sent via the tty (rx/tx).
The serial tty interface also supports additional input signals, that can
also be evaluated within this trigger. This change is adding the following
additional input sources, which could be controlled
via the '/sys/class/<leds>/' sysfs interface.

- line_cts:
  DCE is ready to accept data from the DTE (CTS = Clear To  Send). If the
  line state is detected, the LED is switched on.
  If set to 0 (default), the LED will not evaluate CTS.
  If set to 1, the LED will evaluate CTS.

- line_dsr:
  DCE is ready to receive and send data (DSR = Data Set Ready). If the line
  state is detected, the LED is switched on.
  If set to 0 (default), the LED will not evaluate DSR.
  If set to 1, the LED will evaluate DSR.

- line_car:
  DTE is receiving a carrier from the DCE (DCD = Data Carrier Detect). If
  the line state is detected, the LED is switched on.
  If set to 0 (default), the LED will not evaluate CAR (DCD).
  If set to 1, the LED will evaluate CAR (DCD).

- line_rng:
  DCE has detected an incoming ring signal on the telephone line
  (RI = Ring Indicator). If the line state is detected, the LED is
  switched on.
  If set to 0 (default), the LED will not evaluate RNG (RI).
  If set to 1, the LED will evaluate RNG (RI).

Explanation:
DCE = Data Communication Equipment (Modem)
DTE = Data Terminal Equipment (Computer)

In addition to the new line_* entries in sysfs, the indication for the
direction of the transmitted data is independently controllable via the
new rx and tx sysfs entrie now too. These are on by default. Thus the
trigger behaves as before this change.

- rx:
  Signal reception (rx) of data on the named tty device.
  If set to 0, the LED will not blink on reception.
  If set to 1 (default), the LED will blink on reception.

- tx:
  Signal transmission (tx) of data on the named tty device.
  If set to 0, the LED will not blink on transmission.
  If set to 1 (default), the LED will blink on transmission.

Signed-off-by: Florian Eckert <fe@dev.tdt.de>
---
 .../ABI/testing/sysfs-class-led-trigger-tty   |  54 +++++
 drivers/leds/trigger/ledtrig-tty.c            | 192 +++++++++++++++++-
 2 files changed, 235 insertions(+), 11 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
index 2bf6b24e781b..08127b1a4602 100644
--- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
+++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
@@ -4,3 +4,57 @@ KernelVersion:	5.10
 Contact:	linux-leds@vger.kernel.org
 Description:
 		Specifies the tty device name of the triggering tty
+
+What:		/sys/class/leds/<led>/rx
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		Signal reception (rx) of data on the named tty device.
+		If set to 0, the LED will not blink on reception.
+		If set to 1 (default), the LED will blink on reception.
+
+What:		/sys/class/leds/<led>/tx
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		Signal transmission (tx) of data on the named tty device.
+		If set to 0, the LED will not blink on transmission.
+		If set to 1 (default), the LED will blink on transmission.
+
+car rng
+What:		/sys/class/leds/<led>/line_cts
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DCE is ready to accept data from the DTE (Clear To Send). If
+		the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate CTS.
+		If set to 1, the LED will evaluate CTS.
+
+What:		/sys/class/leds/<led>/line_dsr
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DCE is ready to receive and send data (Data Set Ready). If
+		the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate DSR.
+		If set to 1, the LED will evaluate DSR.
+
+What:		/sys/class/leds/<led>/line_car
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DTE is receiving a carrier from the DCE (Data Carrier Detect).
+		If the line state is detected, the LED is switched on.
+		If set to 0 (default), the LED will not evaluate CAR (DCD).
+		If set to 1, the LED will evaluate CAR (DCD).
+
+What:		/sys/class/leds/<led>/line_cts
+Date:		October 2023
+KernelVersion:	6.7
+Description:
+		DCE has detected an incoming ring signal on the telephone
+		line (Ring Indicator). If the line state is detected, the
+		LED is switched on.
+		If set to 0 (default), the LED will not evaluate RNG (RI).
+		If set to 1, the LED will evaluate RNG (RI).
diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
index 8ae0d2d284af..6a96439a7e55 100644
--- a/drivers/leds/trigger/ledtrig-tty.c
+++ b/drivers/leds/trigger/ledtrig-tty.c
@@ -16,6 +16,24 @@ struct ledtrig_tty_data {
 	const char *ttyname;
 	struct tty_struct *tty;
 	int rx, tx;
+	unsigned long mode;
+};
+
+enum led_trigger_tty_state {
+	TTY_LED_BLINK,
+	TTY_LED_ENABLE,
+	TTY_LED_DISABLE,
+};
+
+enum led_trigger_tty_modes {
+	TRIGGER_TTY_RX = 0,
+	TRIGGER_TTY_TX,
+	TRIGGER_TTY_CTS,
+	TRIGGER_TTY_DSR,
+	TRIGGER_TTY_CAR,
+	TRIGGER_TTY_RNG,
+	/* Keep last */
+	__TRIGGER_TTY_MAX,
 };
 
 static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
@@ -78,13 +96,106 @@ static ssize_t ttyname_store(struct device *dev,
 }
 static DEVICE_ATTR_RW(ttyname);
 
+static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
+	enum led_trigger_tty_modes attr)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	int bit;
+
+	switch (attr) {
+	case TRIGGER_TTY_RX:
+	case TRIGGER_TTY_TX:
+	case TRIGGER_TTY_CTS:
+	case TRIGGER_TTY_DSR:
+	case TRIGGER_TTY_CAR:
+	case TRIGGER_TTY_RNG:
+		bit = attr;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
+}
+
+static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
+	size_t size, enum led_trigger_tty_modes attr)
+{
+	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
+	unsigned long state;
+	int ret;
+	int bit;
+
+	ret = kstrtoul(buf, 0, &state);
+	if (ret)
+		return ret;
+
+	switch (attr) {
+	case TRIGGER_TTY_RX:
+	case TRIGGER_TTY_TX:
+	case TRIGGER_TTY_CTS:
+	case TRIGGER_TTY_DSR:
+	case TRIGGER_TTY_CAR:
+	case TRIGGER_TTY_RNG:
+		bit = attr;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (state)
+		set_bit(bit, &trigger_data->mode);
+	else
+		clear_bit(bit, &trigger_data->mode);
+
+	return size;
+}
+
+#define DEFINE_TTY_TRIGGER(trigger_name, trigger) \
+	static ssize_t trigger_name##_show(struct device *dev, \
+		struct device_attribute *attr, char *buf) \
+	{ \
+		return ledtrig_tty_attr_show(dev, buf, trigger); \
+	} \
+	static ssize_t trigger_name##_store(struct device *dev, \
+		struct device_attribute *attr, const char *buf, size_t size) \
+	{ \
+		return ledtrig_tty_attr_store(dev, buf, size, trigger); \
+	} \
+	static DEVICE_ATTR_RW(trigger_name)
+
+DEFINE_TTY_TRIGGER(rx, TRIGGER_TTY_RX);
+DEFINE_TTY_TRIGGER(tx, TRIGGER_TTY_TX);
+DEFINE_TTY_TRIGGER(line_cts, TRIGGER_TTY_CTS);
+DEFINE_TTY_TRIGGER(line_dsr, TRIGGER_TTY_DSR);
+DEFINE_TTY_TRIGGER(line_car, TRIGGER_TTY_CAR);
+DEFINE_TTY_TRIGGER(line_rng, TRIGGER_TTY_RNG);
+
+static bool ledtrig_tty_evaluate(struct ledtrig_tty_data *trigger_data,
+		int flag)
+{
+	int status;
+
+	status = tty_get_tiocm(trigger_data->tty);
+
+	if (status >= 0)
+		return (status & flag);
+
+	return false;
+}
+
 static void ledtrig_tty_work(struct work_struct *work)
 {
 	struct ledtrig_tty_data *trigger_data =
 		container_of(work, struct ledtrig_tty_data, dwork.work);
+	struct led_classdev *led_cdev = trigger_data->led_cdev;
+	unsigned long interval = LEDTRIG_TTY_INTERVAL;
 	struct serial_icounter_struct icount;
+	enum led_trigger_tty_state state;
+	int current_brightness;
 	int ret;
 
+	state = TTY_LED_DISABLE;
 	mutex_lock(&trigger_data->mutex);
 
 	if (!trigger_data->ttyname) {
@@ -115,22 +226,71 @@ static void ledtrig_tty_work(struct work_struct *work)
 		trigger_data->tty = tty;
 	}
 
-	ret = tty_get_icount(trigger_data->tty, &icount);
-	if (ret) {
-		dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n");
-		mutex_unlock(&trigger_data->mutex);
-		return;
+	if (test_bit(TRIGGER_TTY_CTS, &trigger_data->mode)) {
+		if (ledtrig_tty_evaluate(trigger_data, TIOCM_CTS))
+			state = TTY_LED_ENABLE;
+	}
+
+	if (test_bit(TRIGGER_TTY_DSR, &trigger_data->mode)) {
+		if (ledtrig_tty_evaluate(trigger_data, TIOCM_DSR))
+			state = TTY_LED_ENABLE;
 	}
 
-	if (icount.rx != trigger_data->rx ||
-	    icount.tx != trigger_data->tx) {
-		unsigned long interval = LEDTRIG_TTY_INTERVAL;
+	if (test_bit(TRIGGER_TTY_CAR, &trigger_data->mode)) {
+		if (ledtrig_tty_evaluate(trigger_data, TIOCM_CAR))
+			state = TTY_LED_ENABLE;
+	}
+
+	if (test_bit(TRIGGER_TTY_RNG, &trigger_data->mode)) {
+		if (ledtrig_tty_evaluate(trigger_data, TIOCM_RNG))
+			state = TTY_LED_ENABLE;
+	}
+
+	/* The rx/tx handling must come after the evaluation of TIOCM_*,
+	 * since the display for rx/tx has priority
+	 */
+	if (test_bit(TRIGGER_TTY_RX, &trigger_data->mode) ||
+	    test_bit(TRIGGER_TTY_TX, &trigger_data->mode)) {
+		ret = tty_get_icount(trigger_data->tty, &icount);
+		if (ret) {
+			dev_info(trigger_data->tty->dev, "Failed to get icount, stopped polling\n");
+			mutex_unlock(&trigger_data->mutex);
+			return;
+		}
+
+		if (test_bit(TRIGGER_TTY_RX, &trigger_data->mode) &&
+		    (icount.tx != trigger_data->tx)) {
+			trigger_data->tx = icount.tx;
+			state = TTY_LED_BLINK;
+		}
+
+		if (test_bit(TRIGGER_TTY_TX, &trigger_data->mode) &&
+		    (icount.rx != trigger_data->rx)) {
+			trigger_data->rx = icount.rx;
+			state = TTY_LED_BLINK;
+		}
+	}
 
+	current_brightness = led_cdev->brightness;
+	if (current_brightness)
+		led_cdev->blink_brightness = current_brightness;
+
+	if (!led_cdev->blink_brightness)
+		led_cdev->blink_brightness = led_cdev->max_brightness;
+
+	switch (state) {
+	case TTY_LED_BLINK:
 		led_blink_set_oneshot(trigger_data->led_cdev, &interval,
 				      &interval, 0);
-
-		trigger_data->rx = icount.rx;
-		trigger_data->tx = icount.tx;
+		break;
+	case TTY_LED_ENABLE:
+		led_set_brightness(led_cdev, led_cdev->blink_brightness);
+		break;
+	case TTY_LED_DISABLE:
+		fallthrough;
+	default:
+		led_set_brightness(led_cdev, LED_OFF);
+		break;
 	}
 
 out:
@@ -141,6 +301,12 @@ static void ledtrig_tty_work(struct work_struct *work)
 
 static struct attribute *ledtrig_tty_attrs[] = {
 	&dev_attr_ttyname.attr,
+	&dev_attr_rx.attr,
+	&dev_attr_tx.attr,
+	&dev_attr_line_cts.attr,
+	&dev_attr_line_dsr.attr,
+	&dev_attr_line_car.attr,
+	&dev_attr_line_rng.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(ledtrig_tty);
@@ -153,6 +319,10 @@ static int ledtrig_tty_activate(struct led_classdev *led_cdev)
 	if (!trigger_data)
 		return -ENOMEM;
 
+	/* Enable default rx/tx LED blink */
+	set_bit(TRIGGER_TTY_TX, &trigger_data->mode);
+	set_bit(TRIGGER_TTY_RX, &trigger_data->mode);
+
 	led_set_trigger_data(led_cdev, trigger_data);
 
 	INIT_DELAYED_WORK(&trigger_data->dwork, ledtrig_tty_work);
-- 
2.30.2

Re: [PATCH v4 3/3] leds: ledtrig-tty: add new line mode evaluation

[Greg KH]

On Thu, Oct 19, 2023 at 01:28:09PM +0200, Florian Eckert wrote:
> Until now, the LED blinks when data is sent via the tty (rx/tx).
> The serial tty interface also supports additional input signals, that can
> also be evaluated within this trigger. This change is adding the following
> additional input sources, which could be controlled
> via the '/sys/class/<leds>/' sysfs interface.
> 
> - line_cts:
>   DCE is ready to accept data from the DTE (CTS = Clear To  Send). If the
>   line state is detected, the LED is switched on.
>   If set to 0 (default), the LED will not evaluate CTS.
>   If set to 1, the LED will evaluate CTS.
> 
> - line_dsr:
>   DCE is ready to receive and send data (DSR = Data Set Ready). If the line
>   state is detected, the LED is switched on.
>   If set to 0 (default), the LED will not evaluate DSR.
>   If set to 1, the LED will evaluate DSR.
> 
> - line_car:
>   DTE is receiving a carrier from the DCE (DCD = Data Carrier Detect). If
>   the line state is detected, the LED is switched on.
>   If set to 0 (default), the LED will not evaluate CAR (DCD).
>   If set to 1, the LED will evaluate CAR (DCD).
> 
> - line_rng:
>   DCE has detected an incoming ring signal on the telephone line
>   (RI = Ring Indicator). If the line state is detected, the LED is
>   switched on.
>   If set to 0 (default), the LED will not evaluate RNG (RI).
>   If set to 1, the LED will evaluate RNG (RI).
> 
> Explanation:
> DCE = Data Communication Equipment (Modem)
> DTE = Data Terminal Equipment (Computer)
> 
> In addition to the new line_* entries in sysfs, the indication for the
> direction of the transmitted data is independently controllable via the
> new rx and tx sysfs entrie now too. These are on by default. Thus the
> trigger behaves as before this change.
> 
> - rx:
>   Signal reception (rx) of data on the named tty device.
>   If set to 0, the LED will not blink on reception.
>   If set to 1 (default), the LED will blink on reception.
> 
> - tx:
>   Signal transmission (tx) of data on the named tty device.
>   If set to 0, the LED will not blink on transmission.
>   If set to 1 (default), the LED will blink on transmission.
> 
> Signed-off-by: Florian Eckert <fe@dev.tdt.de>
> ---
>  .../ABI/testing/sysfs-class-led-trigger-tty   |  54 +++++
>  drivers/leds/trigger/ledtrig-tty.c            | 192 +++++++++++++++++-
>  2 files changed, 235 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-led-trigger-tty b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> index 2bf6b24e781b..08127b1a4602 100644
> --- a/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> +++ b/Documentation/ABI/testing/sysfs-class-led-trigger-tty
> @@ -4,3 +4,57 @@ KernelVersion:	5.10
>  Contact:	linux-leds@vger.kernel.org
>  Description:
>  		Specifies the tty device name of the triggering tty
> +
> +What:		/sys/class/leds/<led>/rx
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		Signal reception (rx) of data on the named tty device.
> +		If set to 0, the LED will not blink on reception.
> +		If set to 1 (default), the LED will blink on reception.
> +
> +What:		/sys/class/leds/<led>/tx
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		Signal transmission (tx) of data on the named tty device.
> +		If set to 0, the LED will not blink on transmission.
> +		If set to 1 (default), the LED will blink on transmission.
> +
> +car rng
> +What:		/sys/class/leds/<led>/line_cts
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DCE is ready to accept data from the DTE (Clear To Send). If
> +		the line state is detected, the LED is switched on.
> +		If set to 0 (default), the LED will not evaluate CTS.
> +		If set to 1, the LED will evaluate CTS.
> +
> +What:		/sys/class/leds/<led>/line_dsr
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DCE is ready to receive and send data (Data Set Ready). If
> +		the line state is detected, the LED is switched on.
> +		If set to 0 (default), the LED will not evaluate DSR.
> +		If set to 1, the LED will evaluate DSR.
> +
> +What:		/sys/class/leds/<led>/line_car
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DTE is receiving a carrier from the DCE (Data Carrier Detect).
> +		If the line state is detected, the LED is switched on.
> +		If set to 0 (default), the LED will not evaluate CAR (DCD).
> +		If set to 1, the LED will evaluate CAR (DCD).
> +
> +What:		/sys/class/leds/<led>/line_cts
> +Date:		October 2023
> +KernelVersion:	6.7
> +Description:
> +		DCE has detected an incoming ring signal on the telephone
> +		line (Ring Indicator). If the line state is detected, the
> +		LED is switched on.
> +		If set to 0 (default), the LED will not evaluate RNG (RI).
> +		If set to 1, the LED will evaluate RNG (RI).
> diff --git a/drivers/leds/trigger/ledtrig-tty.c b/drivers/leds/trigger/ledtrig-tty.c
> index 8ae0d2d284af..6a96439a7e55 100644
> --- a/drivers/leds/trigger/ledtrig-tty.c
> +++ b/drivers/leds/trigger/ledtrig-tty.c
> @@ -16,6 +16,24 @@ struct ledtrig_tty_data {
>  	const char *ttyname;
>  	struct tty_struct *tty;
>  	int rx, tx;
> +	unsigned long mode;

Why is mode "unsigned long" when the tty layer treats it as an int?  And
really, this should be set to an explit size, u32 perhaps?  Or am I
confused as to exactly what this is?

> +};
> +
> +enum led_trigger_tty_state {
> +	TTY_LED_BLINK,
> +	TTY_LED_ENABLE,
> +	TTY_LED_DISABLE,
> +};
> +
> +enum led_trigger_tty_modes {
> +	TRIGGER_TTY_RX = 0,
> +	TRIGGER_TTY_TX,
> +	TRIGGER_TTY_CTS,
> +	TRIGGER_TTY_DSR,
> +	TRIGGER_TTY_CAR,
> +	TRIGGER_TTY_RNG,
> +	/* Keep last */
> +	__TRIGGER_TTY_MAX,
>  };
>

Oh wait, is "mode" this?  If so, why not define it as an enum?  Or if
not, I'm totally confused as to what is going on here, sorry.


>  static void ledtrig_tty_restart(struct ledtrig_tty_data *trigger_data)
> @@ -78,13 +96,106 @@ static ssize_t ttyname_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(ttyname);
>  
> +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
> +	enum led_trigger_tty_modes attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	int bit;
> +
> +	switch (attr) {
> +	case TRIGGER_TTY_RX:
> +	case TRIGGER_TTY_TX:
> +	case TRIGGER_TTY_CTS:
> +	case TRIGGER_TTY_DSR:
> +	case TRIGGER_TTY_CAR:
> +	case TRIGGER_TTY_RNG:
> +		bit = attr;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));

sysfs_emit() for all new sysfs attributes please.

> +}
> +
> +static ssize_t ledtrig_tty_attr_store(struct device *dev, const char *buf,
> +	size_t size, enum led_trigger_tty_modes attr)
> +{
> +	struct ledtrig_tty_data *trigger_data = led_trigger_get_drvdata(dev);
> +	unsigned long state;
> +	int ret;
> +	int bit;
> +
> +	ret = kstrtoul(buf, 0, &state);
> +	if (ret)
> +		return ret;
> +
> +	switch (attr) {
> +	case TRIGGER_TTY_RX:
> +	case TRIGGER_TTY_TX:
> +	case TRIGGER_TTY_CTS:
> +	case TRIGGER_TTY_DSR:
> +	case TRIGGER_TTY_CAR:
> +	case TRIGGER_TTY_RNG:
> +		bit = attr;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (state)
> +		set_bit(bit, &trigger_data->mode);
> +	else
> +		clear_bit(bit, &trigger_data->mode);

I think your test of "state" here is wrong, if you write in "40000" you
are treating it as "1", which I don't think you want, right?

thanks,

greg k-h

Re: [PATCH v4 3/3] leds: ledtrig-tty: add new line mode evaluation

[Florian Eckert]

On 2023-10-21 18:07, Greg KH wrote:
>> diff --git a/drivers/leds/trigger/ledtrig-tty.c 
>> b/drivers/leds/trigger/ledtrig-tty.c
>> index 8ae0d2d284af..6a96439a7e55 100644
>> --- a/drivers/leds/trigger/ledtrig-tty.c
>> +++ b/drivers/leds/trigger/ledtrig-tty.c
>> @@ -16,6 +16,24 @@ struct ledtrig_tty_data {
>>  	const char *ttyname;
>>  	struct tty_struct *tty;
>>  	int rx, tx;
>> +	unsigned long mode;
> 
> Why is mode "unsigned long" when the tty layer treats it as an int?  
> And
> really, this should be set to an explit size, u32 perhaps?  Or am I
> confused as to exactly what this is?

This is about the line state that the LED should show "altogether".
All states that the LED is to display are stored here.

For example:
Via the sysfs of the LED I can set the flags rx, tx and line_cts to
a "not" zero value. That means that the led is enable if the CTS of the
tty ist set, and the LED flashes if rx/tx data are transmitted via
this tty.

Therefore, the bits 0 (TRIGGER_TTY_RX), 1 (TRIGGER_TTY_TX) and
2 (TRIGGER_TTY_CTS) are set in the variable. As defined in the
enum led_trigger_tty_modes

I think I have not chosen the correct name for the variable there.
Maybe line_state, would be a better choice?

>> +};
>> +
>> +enum led_trigger_tty_state {
>> +	TTY_LED_BLINK,
>> +	TTY_LED_ENABLE,
>> +	TTY_LED_DISABLE,
>> +};
>> +
>> +enum led_trigger_tty_modes {
>> +	TRIGGER_TTY_RX = 0,
>> +	TRIGGER_TTY_TX,
>> +	TRIGGER_TTY_CTS,
>> +	TRIGGER_TTY_DSR,
>> +	TRIGGER_TTY_CAR,
>> +	TRIGGER_TTY_RNG,
>> +	/* Keep last */
>> +	__TRIGGER_TTY_MAX,
>>  };
>> 
> 
> Oh wait, is "mode" this?  If so, why not define it as an enum?  Or if
> not, I'm totally confused as to what is going on here, sorry.

See explanation above. I can not set this to an enum because I could
set more then one Flag via the sysfs.

> 
>>  static void ledtrig_tty_restart(struct ledtrig_tty_data 
>> *trigger_data)
>> @@ -78,13 +96,106 @@ static ssize_t ttyname_store(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RW(ttyname);
>> 
>> +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
>> +	enum led_trigger_tty_modes attr)
>> +{
>> +	struct ledtrig_tty_data *trigger_data = 
>> led_trigger_get_drvdata(dev);
>> +	int bit;
>> +
>> +	switch (attr) {
>> +	case TRIGGER_TTY_RX:
>> +	case TRIGGER_TTY_TX:
>> +	case TRIGGER_TTY_CTS:
>> +	case TRIGGER_TTY_DSR:
>> +	case TRIGGER_TTY_CAR:
>> +	case TRIGGER_TTY_RNG:
>> +		bit = attr;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
> 
> sysfs_emit() for all new sysfs attributes please.

Correct. Thanks for the hint will use sysf_emit() function in the next
patchset round.

> 
>> +}
>> +
>> +static ssize_t ledtrig_tty_attr_store(struct device *dev, const char 
>> *buf,
>> +	size_t size, enum led_trigger_tty_modes attr)
>> +{
>> +	struct ledtrig_tty_data *trigger_data = 
>> led_trigger_get_drvdata(dev);
>> +	unsigned long state;
>> +	int ret;
>> +	int bit;
>> +
>> +	ret = kstrtoul(buf, 0, &state);
>> +	if (ret)
>> +		return ret;
>> +
>> +	switch (attr) {
>> +	case TRIGGER_TTY_RX:
>> +	case TRIGGER_TTY_TX:
>> +	case TRIGGER_TTY_CTS:
>> +	case TRIGGER_TTY_DSR:
>> +	case TRIGGER_TTY_CAR:
>> +	case TRIGGER_TTY_RNG:
>> +		bit = attr;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (state)
>> +		set_bit(bit, &trigger_data->mode);
>> +	else
>> +		clear_bit(bit, &trigger_data->mode);
> 
> I think your test of "state" here is wrong, if you write in "40000" you
> are treating it as "1", which I don't think you want, right?

If I have understood your question correctly, then I would say that your
assumption is not correct. I just want to check here whether it is a 
number
greater than zero or not. If the number is greater than zero then the 
bit
should be set in the 'mode' variable of the struct and if it is zero 
then
it should be cleared.

The LED could indicate more then one state there. As described above.
This was requested by Uwe Kleine-König in the old v7 patch series [1].

Thanks for your review!

---
Florian

Links:
[1] 
https://lore.kernel.org/linux-leds/20230306093524.amm7o4ppa7gon4ew@pengutronix.de/

Re: [PATCH v4 3/3] leds: ledtrig-tty: add new line mode evaluation

[Greg KH]

On Sun, Oct 22, 2023 at 12:24:27PM +0200, Florian Eckert wrote:
> On 2023-10-21 18:07, Greg KH wrote:
> > > diff --git a/drivers/leds/trigger/ledtrig-tty.c
> > > b/drivers/leds/trigger/ledtrig-tty.c
> > > index 8ae0d2d284af..6a96439a7e55 100644
> > > --- a/drivers/leds/trigger/ledtrig-tty.c
> > > +++ b/drivers/leds/trigger/ledtrig-tty.c
> > > @@ -16,6 +16,24 @@ struct ledtrig_tty_data {
> > >  	const char *ttyname;
> > >  	struct tty_struct *tty;
> > >  	int rx, tx;
> > > +	unsigned long mode;
> > 
> > Why is mode "unsigned long" when the tty layer treats it as an int?  And
> > really, this should be set to an explit size, u32 perhaps?  Or am I
> > confused as to exactly what this is?
> 
> This is about the line state that the LED should show "altogether".
> All states that the LED is to display are stored here.
> 
> For example:
> Via the sysfs of the LED I can set the flags rx, tx and line_cts to
> a "not" zero value. That means that the led is enable if the CTS of the
> tty ist set, and the LED flashes if rx/tx data are transmitted via
> this tty.
> 
> Therefore, the bits 0 (TRIGGER_TTY_RX), 1 (TRIGGER_TTY_TX) and
> 2 (TRIGGER_TTY_CTS) are set in the variable. As defined in the
> enum led_trigger_tty_modes

So the enum is a bitfield value?  That's not obvious either, a comment
for the enum might be good to help describe that.

> I think I have not chosen the correct name for the variable there.
> Maybe line_state, would be a better choice?

Or "trigger_modes"?  "mode" feels odd, these are values, so maybe just
"triggers"?

Naming is hard :(

> > > +};
> > > +
> > > +enum led_trigger_tty_state {
> > > +	TTY_LED_BLINK,
> > > +	TTY_LED_ENABLE,
> > > +	TTY_LED_DISABLE,
> > > +};
> > > +
> > > +enum led_trigger_tty_modes {
> > > +	TRIGGER_TTY_RX = 0,
> > > +	TRIGGER_TTY_TX,
> > > +	TRIGGER_TTY_CTS,
> > > +	TRIGGER_TTY_DSR,
> > > +	TRIGGER_TTY_CAR,
> > > +	TRIGGER_TTY_RNG,
> > > +	/* Keep last */
> > > +	__TRIGGER_TTY_MAX,
> > >  };
> > > 
> > 
> > Oh wait, is "mode" this?  If so, why not define it as an enum?  Or if
> > not, I'm totally confused as to what is going on here, sorry.
> 
> See explanation above. I can not set this to an enum because I could
> set more then one Flag via the sysfs.

Ah, then say they are bits, enums are usually not used for that, or if
they are, they are documented better :)

> > >  static void ledtrig_tty_restart(struct ledtrig_tty_data
> > > *trigger_data)
> > > @@ -78,13 +96,106 @@ static ssize_t ttyname_store(struct device *dev,
> > >  }
> > >  static DEVICE_ATTR_RW(ttyname);
> > > 
> > > +static ssize_t ledtrig_tty_attr_show(struct device *dev, char *buf,
> > > +	enum led_trigger_tty_modes attr)
> > > +{
> > > +	struct ledtrig_tty_data *trigger_data =
> > > led_trigger_get_drvdata(dev);
> > > +	int bit;
> > > +
> > > +	switch (attr) {
> > > +	case TRIGGER_TTY_RX:
> > > +	case TRIGGER_TTY_TX:
> > > +	case TRIGGER_TTY_CTS:
> > > +	case TRIGGER_TTY_DSR:
> > > +	case TRIGGER_TTY_CAR:
> > > +	case TRIGGER_TTY_RNG:
> > > +		bit = attr;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return sprintf(buf, "%u\n", test_bit(bit, &trigger_data->mode));
> > 
> > sysfs_emit() for all new sysfs attributes please.
> 
> Correct. Thanks for the hint will use sysf_emit() function in the next
> patchset round.
> 
> > 
> > > +}
> > > +
> > > +static ssize_t ledtrig_tty_attr_store(struct device *dev, const
> > > char *buf,
> > > +	size_t size, enum led_trigger_tty_modes attr)
> > > +{
> > > +	struct ledtrig_tty_data *trigger_data =
> > > led_trigger_get_drvdata(dev);
> > > +	unsigned long state;
> > > +	int ret;
> > > +	int bit;
> > > +
> > > +	ret = kstrtoul(buf, 0, &state);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	switch (attr) {
> > > +	case TRIGGER_TTY_RX:
> > > +	case TRIGGER_TTY_TX:
> > > +	case TRIGGER_TTY_CTS:
> > > +	case TRIGGER_TTY_DSR:
> > > +	case TRIGGER_TTY_CAR:
> > > +	case TRIGGER_TTY_RNG:
> > > +		bit = attr;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (state)
> > > +		set_bit(bit, &trigger_data->mode);
> > > +	else
> > > +		clear_bit(bit, &trigger_data->mode);
> > 
> > I think your test of "state" here is wrong, if you write in "40000" you
> > are treating it as "1", which I don't think you want, right?
> 
> If I have understood your question correctly, then I would say that your
> assumption is not correct. I just want to check here whether it is a number
> greater than zero or not. If the number is greater than zero then the bit
> should be set in the 'mode' variable of the struct and if it is zero then
> it should be cleared.

"greater than 0" can be any number, that's not a good api.  Use the
sysfs api that can handle a boolean, it will deal with "y/N" and 0/1 and
all sorts of other options that way for you automatically.

> The LED could indicate more then one state there. As described above.
> This was requested by Uwe Kleine-König in the old v7 patch series [1].

That's fine, but you need to fix up the userspace api a bit here.

thanks,

greg k-h