Ahoy!
On Thu, Jun 15, 2017 at 12:28:33AM +0200, Pavel Machek wrote:
> 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@xxxxxx>
> > >
> > > 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?
That was probably my @maxwell.research.nokia.com address. :-) There are no
occurrences of that in the kernel source anymore.
>
> > > > +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?
struct v4l2_flash_config is only needed as an argument for
v4l2_flash_init(). I'll split that into two functions in this occasion,
it'll be nicer.
We now have more or less the same conversion implemented in three or so
times, there have to be ways to make that easier for drivers. I think that
could be done later, as well as adding support for checking the flash
strobe status.
--
Regards,
Sakari Ailus
e-mail: sakari.ailus@xxxxxx XMPP: sailus@xxxxxxxxxxxxxx
