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:

quoted

+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.

quoted

+	dev_dbg(dev, "Resume %s\n", rval < 0 ? "fail" : "ok");
+
+	return rval;
+}

...

quoted

+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@iki.fi	XMPP: sailus@retiisi.org.uk