Hi! > Thanks for the review!
You are welcome :-).
> On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:
> > Hi!
> >
> > > From: Sakari Ailus <sakari.ai...@iki.fi>
> >
> > That address no longer works, right?
>
> Why wouldn't it work? Or... do you know something I don't? :-)
Aha. I thought I was removing it from source files because it was no
longer working, but maybe I'm misremembering?
> > > +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.
Can as3645a_current_to_reg just access struct v4l2_flash_config so
that it does not have to recreate its look-alike on the fly?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures)
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
signature.asc
Description: Digital signature
