Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8;
 format=flowed
Date:   Sun, 22 Oct 2023 12:24:27 +0200
From:   Florian Eckert <fe@dev.tdt.de>
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Eckert.Florian@googlemail.com, jirislaby@kernel.org, pavel@ucw.cz,
        lee@kernel.org, kabel@kernel.org, u.kleine-koenig@pengutronix.de,
        ansuelsmth@gmail.com, m.brock@vanmierlo.com,
        linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
        linux-leds@vger.kernel.org
Subject: Re: [PATCH v4 3/3] leds: ledtrig-tty: add new line mode evaluation
In-Reply-To: <2023102136-reenact-cash-7295@gregkh>
References: <20231019112809.881730-1-fe@dev.tdt.de>
 <20231019112809.881730-4-fe@dev.tdt.de>
 <2023102136-reenact-cash-7295@gregkh>
Message-ID: <72be6923ff6dd03a5d02d04ee1c5796f@dev.tdt.de>
User-Agent: Roundcube Webmail/1.3.17
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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/