Hi Jacek,

On Thu, Jun 15, 2017 at 03:01:47PM +0200, Jacek Anaszewski wrote:

Hi Sakari,

On 06/15/2017 12:10 AM, Sakari Ailus wrote:

quoted

Hi Jacek,

Thanks for the review!

You're welcome!

quoted

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. :-)

Nice :-). I'm also surprised that v4l2-flash API is also used in
drivers/staging/greybus/light.c which popped up with kbuild test robot
complaints.

I missed that on the first round of the submission as well. I'll fix that
in v2.

quoted

On Wed, Jun 14, 2017 at 11:15:24PM +0200, Jacek Anaszewski wrote:

quoted

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

quoted

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

...

quoted

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.

It depends on whether the sub-leds are identified by reg property.
In this case usually common prefix is used followed by reg value,
e.g. led@1, led@2 etc.

Is there a device that would use this already? I checked common.txt and
I couldn't find a suggestion of this scheme there.

Otherwise prevailing scheme is e.g.:

        blue-power {
		...
                label = "netxbig:blue:power";
	}

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk