Hi Pavel,
Thanks for the review!
On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:
> Hi!
>
> > From: Sakari Ailus <sakari.ailus@xxxxxx>
>
> That address no longer works, right?
Why wouldn't it work? Or... do you know something I don't? :-)
>
> > Add a LED flash class driver for the as3654a flash controller. A V4L2 flash
> > driver for it already exists (drivers/media/i2c/as3645a.c), and this driver
> > is based on that.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
>
> > + * Based on drivers/media/i2c/as3645a.c.
> > + *
> > + * Contact: Sakari Ailus <sakari.ailus@xxxxxx>
>
> So I believe it should not be here.
>
> > +/*
> > + * as3645a_set_config - Set flash configuration registers
> > + * @flash: The flash
> > + *
>
> /** for linuxdoc?
Fixed.
>
> > + struct as3645a *flash = fled_to_as3645a(fled);
> > + int rval;
> > +
> > + /* NOTE: reading register clear fault status */
>
> clears.
Fixed.
>
> > +static unsigned int as3645a_current_to_reg(struct as3645a *flash, bool is_flash,
> > + unsigned int ua)
> > +{
> > + struct {
> > + unsigned int min;
> > + unsigned int max;
> > + unsigned int step;
> > + } __mms[] = {
> > + {
> > + AS_TORCH_INTENSITY_MIN,
> > + flash->cfg.assist_max_ua,
> > + AS_TORCH_INTENSITY_STEP
> > + },
> > + {
> > + AS_FLASH_INTENSITY_MIN,
> > + flash->cfg.flash_max_ua,
> > + AS_FLASH_INTENSITY_STEP
> > + },
> > + }, *mms = &__mms[is_flash];
> > +
> > + if (ua < mms->min)
> > + ua = mms->min;
>
> That's some... seriously interesting code. And you are forcing gcc to
> create quite interesting structure on stack. Would it be easier to do
> normal if()... without this magic?
>
> > + struct v4l2_flash_config cfg = {
> > + .torch_intensity = {
> > + .min = AS_TORCH_INTENSITY_MIN,
> > + .max = flash->cfg.assist_max_ua,
> > + .step = AS_TORCH_INTENSITY_STEP,
> > + .val = flash->cfg.assist_max_ua,
> > + },
> > + .indicator_intensity = {
> > + .min = AS_INDICATOR_INTENSITY_MIN,
> > + .max = flash->cfg.indicator_max_ua,
> > + .step = AS_INDICATOR_INTENSITY_STEP,
> > + .val = flash->cfg.indicator_max_ua,
> > + },
> > + };
>
> Ugh. And here you have copy of the above struct, + .val. Can it be
> somehow de-duplicated?
The flash_brightness_set callback uses micro-Amps as the unit and the driver
needs to convert that to its own specific units. Yeah, there would be
probably an easier way, too. But that'd likely require changes to the LED
flash class.
--
Kind regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
