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