Re: [PATCH 6/8] leds: as3645a: Add LED flash class driver

[Pavel Machek <pavel@ucw.cz>]

[-- Attachment #1: Type: text/plain, Size: 2409 bytes --]

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.ailus@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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]