Ahoy!

On Thu, Jun 15, 2017 at 12:28:33AM +0200, Pavel Machek wrote:

Hi!

quoted

Thanks for the review!

You are welcome :-).

quoted

On Wed, Jun 14, 2017 at 11:39:41PM +0200, Pavel Machek wrote:

quoted

Hi!

quoted

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? 

That was probably my @maxwell.research.nokia.com address. :-) There are no
occurrences of that in the kernel source anymore.

quoted

quoted

quoted

+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?

quoted

+	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@iki.fi	XMPP: sailus@retiisi.org.uk