From: Ben Whitten <ben.whitten@gmail.com>
To: Jacek Anaszewski <jacek.anaszewski@gmail.com>
Cc: rpurdie@rpsys.net, pavel@ucw.cz, linux-leds@vger.kernel.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH] leds: trigger: Introduce a NETDEV trigger
Date: Mon, 4 Dec 2017 16:25:57 +0000 [thread overview]
Message-ID: <CAF3==isGXFeyBVQQEj5XpenEWbZv16MPP=bce8uWUEWKs-yucg@mail.gmail.com> (raw)
In-Reply-To: <fdc16216-4acc-b87b-1d75-6db33c3fb389@gmail.com>
Hi Jacek,
Thank you for the review, trimmed and comments inline.
On 3 December 2017 at 21:09, Jacek Anaszewski
<jacek.anaszewski@gmail.com> wrote:
>> + * link - LED's normal state reflects whether the link is up
>> + * (has carrier) or not
>> + * tx - LED blinks on transmitted data
>> + * rx - LED blinks on receive data
>> + *
>> + */
>> +
>> +struct led_netdev_data {
>> + spinlock_t lock;
>> +
>> + struct delayed_work work;
>> + struct notifier_block notifier;
>> +
>> + struct led_classdev *led_cdev;
>> + struct net_device *net_dev;
>> +
>> + char device_name[IFNAMSIZ];
>> + atomic_t interval;
>> + unsigned int last_activity;
>> +
>> + unsigned long mode;
>> +#define LED_BLINK_link 0
>> +#define LED_BLINK_tx 1
>> +#define LED_BLINK_rx 2
>
> LED core already has LED_BLINK* family of macros. Please come up
> with the prefix specific for this trigger e.g. NETDEV_LED.
> Also let's use uppercase for the whole macro name.
>
>> +#define LED_MODE_LINKUP 3
>
> s/LED/NETDEV_LED/
>
Sorted.
>> +};
>> +
>> +static void set_baseline_state(struct led_netdev_data *trigger_data)
>> +{
>> + if (!test_bit(LED_MODE_LINKUP, &trigger_data->mode))
>> + led_set_brightness(trigger_data->led_cdev, LED_OFF);
>> + else {
>> + if (test_bit(LED_BLINK_link, &trigger_data->mode))
>> + led_set_brightness(trigger_data->led_cdev, LED_FULL);
>> +
>> + if (test_bit(LED_BLINK_tx, &trigger_data->mode) ||
>> + test_bit(LED_BLINK_rx, &trigger_data->mode))
>> + schedule_delayed_work(&trigger_data->work,
>> + atomic_read(&trigger_data->interval));
>
> Now we have blink support in the LED core. Please use
> led_blink_set_oneshot() instead.
>
> Generally you can compare how ledtrig-timer is now implemented.
>
I have cleaned up and converted to led_blink_set_oneshot in the work function
as suggested however it doesn't quite 'look' right yet. So there is something
wrong with my implementation.
When setting blink on RX then initiating a download I find these differences:
Old mechanism, an 'interval' setting of 50ms results in a 100ms period with
50% duty cycle, 50ms on 50ms off.
New mechanism, an 'interval' setting of 10ms resulting in 110ms period with
18% duty cycle, 20ms on 90ms off. Appears to be quite a delay getting the
blink queued up again.
The oneshot is set in the worker function tasked with gathering netdev stats.
I'll keep exploring.
>> +#define led_mode_flags_attr(field) \
>> +static ssize_t field##_show(struct device *dev, \
>> + struct device_attribute *attr, char *buf) \
>> +{ \
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev); \
>> + struct led_netdev_data *trigger_data = led_cdev->trigger_data; \
>> + \
>> + return sprintf(buf, "%u\n", test_bit(LED_BLINK_##field, \
>> + &trigger_data->mode)); \
>> +} \
>> +static ssize_t field##_store(struct device *dev, \
>> + struct device_attribute *attr, const char *buf, size_t size) \
>> +{ \
>> + struct led_classdev *led_cdev = dev_get_drvdata(dev); \
>> + struct led_netdev_data *trigger_data = led_cdev->trigger_data; \
>> + unsigned long state; \
>> + int ret; \
>> + \
>> + ret = kstrtoul(buf, 0, &state); \
>> + if (ret) \
>> + return ret; \
>> + \
>> + cancel_delayed_work_sync(&trigger_data->work); \
>> + \
>> + if (state) \
>> + set_bit(LED_BLINK_##field, &trigger_data->mode); \
>> + else \
>> + clear_bit(LED_BLINK_##field, &trigger_data->mode); \
>> + \
>> + set_baseline_state(trigger_data); \
>> + \
>> + return size; \
>> +} \
>> +static DEVICE_ATTR_RW(field);
>
> In order to get rid of this macro and avoid the need for camel case
> macro name we could have one function that would accept an enum e.g.
>
> enum netdev_led_attr {
> NETDEV_ATTR_LINK,
> NETDEV_ATTR_RX,
> NETDEV_ATTR_TX
> };
>
> The function would be called from each sysfs attr callback with the
> related enum, and would alter the state of the related bit.
>
I was aiming for a bit of code de duplication, but it ended up a mess.
Fixed as per your suggestion.
--
Kind regards,
Ben Whitten
prev parent reply other threads:[~2017-12-04 16:26 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-28 21:54 [PATCH] Introduce a NETDEV LED trigger Ben Whitten
2017-11-28 21:54 ` [PATCH] leds: trigger: Introduce a NETDEV trigger Ben Whitten
2017-11-28 22:41 ` Randy Dunlap
2017-11-28 22:50 ` Andrew Lunn
2017-12-03 21:09 ` Jacek Anaszewski
2017-12-04 16:25 ` Ben Whitten [this message]
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='CAF3==isGXFeyBVQQEj5XpenEWbZv16MPP=bce8uWUEWKs-yucg@mail.gmail.com' \
--to=ben.whitten@gmail.com \
--cc=jacek.anaszewski@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-leds@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=rpurdie@rpsys.net \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).