Hi Jacek, Thanks for the review! I have to say I found the v4l2-flash-led-class framework quite useful, now that I refactored a driver for using it. Now we have a user for the indicator, too. :-) On Wed, Jun 14, 2017 at 11:15:24PM +0200, Jacek Anaszewski wrote: > > +static __maybe_unused int as3645a_suspend(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct as3645a *flash = i2c_get_clientdata(client); > > + int rval; > > + > > + rval = as3645a_set_control(flash, AS_MODE_EXT_TORCH, false); > > + dev_dbg(dev, "Suspend %s\n", rval < 0 ? "failed" : "ok"); > > + > > + return rval; > > +} > > + > > +static __maybe_unused int as3645a_resume(struct device *dev) > > +{ > > + struct i2c_client *client = to_i2c_client(dev); > > + struct as3645a *flash = i2c_get_clientdata(client); > > + int rval; > > + > > + rval = as3645a_setup(flash); > > + > > nitpicking: inconsistent coding style - there is no empty line before > dev_dbg() in the as3645a_suspend(). Added one for as3645a_suspend() --- it should have been there. > > > + dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok"); > > + > > + return rval; > > +} ... > > +static int as3645a_led_class_setup(struct as3645a *flash) > > +{ > > + struct led_classdev *fled_cdev = &flash->fled.led_cdev; > > + struct led_classdev *iled_cdev = &flash->iled_cdev; > > + struct led_flash_setting *cfg; > > + int rval; > > + > > + iled_cdev->name = "as3645a indicator"; > > + iled_cdev->brightness_set_blocking = as3645a_set_indicator_brightness; > > + iled_cdev->max_brightness = > > + flash->cfg.indicator_max_ua / AS_INDICATOR_INTENSITY_STEP; > > + > > + rval = led_classdev_register(&flash->client->dev, iled_cdev); > > + if (rval < 0) > > + return rval; > > + > > + cfg = &flash->fled.brightness; > > + cfg->min = AS_FLASH_INTENSITY_MIN; > > + cfg->max = flash->cfg.flash_max_ua; > > + cfg->step = AS_FLASH_INTENSITY_STEP; > > + cfg->val = flash->cfg.flash_max_ua; > > + > > + cfg = &flash->fled.timeout; > > + cfg->min = AS_FLASH_TIMEOUT_MIN; > > + cfg->max = flash->cfg.flash_timeout_us; > > + cfg->step = AS_FLASH_TIMEOUT_STEP; > > + cfg->val = flash->cfg.flash_timeout_us; > > + > > + flash->fled.ops = &as3645a_led_flash_ops; > > + > > + fled_cdev->name = "as3645a flash"; > > LED class device name should be taken from label DT property, > or DT node name if the former wasn't defined. > > Also LED device naming convention defines colon as a separator > between name segments. Right. I'll fix that. I just realised I'm missing DT binding documentation for this device; I'll add that, too. Is the preference to allow freely chosen node names for the LEDs? Now that there's the label, too, this appears to be somewhat duplicated information. -- Kind regards, Sakari Ailus e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html