Hi Sakari,
On 06/15/2017 12:10 AM, Sakari Ailus wrote:
> Hi Jacek,
>
> Thanks for the review!
You're welcome!
> 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.
> 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.
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.
Otherwise prevailing scheme is e.g.:
blue-power {
...
label = "netxbig:blue:power";
}
--
Best regards,
Jacek Anaszewski
--
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
