drivers: video: gc2145: Include set_resolution() in the format caps check

[josuah]

The current GalxyCore GC2145 does not guard against a wrong pixelformat passed.

This usually never happens and might be caught by the gc2145_set_output_format() anyway, but this permits to only set drv_data->fmt if the format is valid.

A very minor change!

[uLipe]

LGTM, it is perfectly reasonable grouping the I/O operations only after making sure the the format is supported.

[pillo79]

ACK, though if you are up for another round... in Zephyr usually failures are handled in the ifs, and the function returns at the end if everything is OK. You could refactor set_fmt so that if it does not find a match it returns an error.

[uLipe]

@pillo79 ,

ACK, though if you are up for another round... in Zephyr usually failures are handled in the ifs, and the function returns at the end if everything is OK. You could refactor set_fmt so that if it does not find a match it returns an error.

@pillo79 , most of these drivers were written and their logic got oversight after merging, is there a proposal of refactoring them to make them consistent of the Zephyr standard, I have backlog here that I plan to start once I finish my work with the Arduino Nicla, and refactor one driver by one.

[pillo79]

ACK, though if you are up for another round... in Zephyr usually failures are handled in the ifs, and the function returns at the end if everything is OK. You could refactor set_fmt so that if it does not find a match it returns an error.

@pillo79 , most of these drivers were written and their logic got oversight after merging, is there a proposal of refactoring them to make them consistent of the Zephyr standard, I have backlog here that I plan to start once I finish my work with the Arduino Nicla, and refactor one driver by one.

Sure, that's great and that's why I approved in the first place 🙂
I just noted that this PR is actually making the above point slightly worse, in the sense that it moves more code inside the if. My POV was simply "if you're gonna touch it, might as well fix it once". Not a deal breaker at all!

[josuah]

is there a proposal of refactoring them to make them consistent of the Zephyr standard

Kind of, maybe the first step is to agree on a "ideal format".
This is the goal of this:

• drivers: video: sensors: add a sensor_skeleton.c to speed-up contribution of simple sensors #73867

I was tempted to start with the GC2145 as it got a review round directly from a TSC member here:

• video: gc2145: add Galaxy Core GC2145 sensor support and basic controls #77770

The first sensors were contributed a long time ago, and it is good practice to pick existing drivers to write new ones.
This leads to a lot of refactoring to do for each and every new sensor to make it look closer to recent Zephyr guidelines.

[josuah]

Force push: the new driver passes the enum resolutions to set_resolution instead of the width/height, so the build did fail. Remove the width and height variables set but not used after the refactoring.

[pillo79]

ACK - though the new for is quirky 🙂
Up to you if you wish to amend with something like my comment.

[pillo79]

Nit: I think a more common way to write this would be:

	enum resolutions res = RESOLUTIONS_MAX;
	
	for (int i = 0; i < ARRAY_SIZE(fmts); i++) {
		if (fmts[i].width_min == fmt->width && fmts[i].height_min == fmt->height &&
		    fmts[i].pixelformat == fmt->pixelformat) {
			res = (enum resolutions)i;
			break;
		}
	}
	if (res == RESOLUTIONS_MAX) {
		LOG_ERR("Image format not supported\n");
		return -ENOTSUP;
	}

[josuah]

Then there is the more obvious for (i = 0; i < N; i++) idiom standing out.
I appreciate the teachings. :)