On Wed, 9 Sep 2020 22:48:15 +0200 Pavel Machek <pavel@xxxxxx> wrote: > Hi! > > > Many an ethernet PHY (and other chips) supports various HW control modes > > for LEDs connected directly to them. > > I guess this should be > > "Many ethernet PHYs (and other chips) support various HW control modes > for LEDs connected directly to them." > I guess it is older English, used mainly in poetry, but I read it in works of contemporary fiction as well. As far as I could find, it is still actually gramatically correct. https://idioms.thefreedictionary.com/many+an https://en.wiktionary.org/wiki/many_a But I will change it if you insist on it. > > This API registers a new private LED trigger called dev-hw-mode. When > > this trigger is enabled for a LED, the various HW control modes which > > are supported by the device for given LED can be get/set via hw_mode > > sysfs file. > > > > Signed-off-by: Marek Behún <marek.behun@xxxxxx> > > --- > > .../sysfs-class-led-trigger-dev-hw-mode | 8 + > > drivers/leds/Kconfig | 10 + > > I guess this should live in drivers/leds/trigger/ledtrig-hw.c . I'd > call the trigger just "hw"... > Will move in next version. Lets see what others think about the trigger name. > > +Contact: Marek Behún <marek.behun@xxxxxx> > > + linux-leds@xxxxxxxxxxxxxxx > > +Description: (W) Set the HW control mode of this LED. The various available HW control modes > > + are specific per device to which the LED is connected to and per LED itself. > > + (R) Show the available HW control modes and the currently selected one. > > 80 columns :-) (and please fix that globally, at least at places where > it is easy, like comments). > Linux is at 100 columns now since commit bdc48fa11e46, commited by Linus. See https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/scripts/checkpatch.pl?h=v5.9-rc4&id=bdc48fa11e46f867ea4d75fa59ee87a7f48be144 There was actually an article about this on Phoronix, I think. > > + return 0; > > +err_free: > > + devm_kfree(dev, led); > > + return ret; > > +} > > No need for explicit free with devm_ infrastructure? No, but if the registration failed, I don't see any reason why the memory should be freed only when the PHY device is destroyed, if the memory is not used... On failures other code in Linux frees even devm_ allocated resources. > > + cur_mode = led->ops->led_get_hw_mode(dev->parent, led); > > + > > + for (mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter); > > + mode; > > + mode = led->ops->led_iter_hw_mode(dev->parent, led, &iter)) { > > + bool sel; > > + > > + sel = cur_mode && !strcmp(mode, cur_mode); > > + > > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s%s%s ", sel ? "[" : "", mode, > > + sel ? "]" : ""); > > + } > > + > > + if (buf[len - 1] == ' ') > > + buf[len - 1] = '\n'; > > Can this ever be false? Are you accessing buf[-1] in such case? > It can be false if whole PAGE_SIZE is written. The code above using scnprintf only writes the first PAGE_SIZE bytes. You are right about the buf[-1] case though. len has to be nonzero. Thanks. > > +int of_register_hw_controlled_leds(struct device *dev, const char *devicename, > > + const struct hw_controlled_led_ops *ops); > > +int hw_controlled_led_brightness_set(struct led_classdev *cdev, enum led_brightness brightness); > > + > > Could we do something like hw_controlled_led -> hw_led to keep > verbosity down and line lengths reasonable? Or hwc_led? > I am not opposed, lets see what Andrew / others think. > > +extern struct led_hw_trigger_type hw_control_led_trig_type; > > +extern struct led_trigger hw_control_led_trig; > > + > > +#else /* !IS_ENABLED(CONFIG_LEDS_HW_CONTROLLED) */ > > CONFIG_LEDS_HWC? Or maybe CONFIG_LEDTRIG_HW? The second option looks more reasonable to me, if we move to drivers/leds/trigger. Marek
