RE: [PATCH v10 2/2] media: i2c: add driver for ITE IT6625/IT6626

[Hermes.Wu]

Hi Sakari,

Thank you for the review. I'll address these in follow-up patches.

> Why is the default to enable printing all debug information? Shouldn't this
> be 0?

Yes, I'll change the default debug level to 0.

> You have a single sub-device. Why not to use the sub-device state lock for
> this? This driver should also fully switch to sub-device state (related
> comments below).

Agreed. I'll use the driver mutex as the sub-device state lock and move the
active pad-format state into the centralized sub-device state.

The separate EDID and InfoFrame locks will remain, as they serialize
independent multi-step transactions rather than pad-format state.

> This seems to be unused.

Correct. I'll remove the unused bus member.

> Avoid storing state information outside sub-devie state.

Agreed. I'll move csi_format/mbus_fmt_code into the active pad format and
derive the hardware CSI data type from the media-bus code when needed.

> You won't be switching PHYs at runtime, will you? You should have two
> arrays in this case, selected based on PHY type.

Correct, the PHY type is fixed by firmware configuration. I'll use separate
single-entry link-frequency arrays for D-PHY and C-PHY.

> Please drop, minimum is 8.1 nowadays.

Will do.

> Ditto.

I'll remove the redundant reserved initializers from both timing capability
structures.

> unsigned int and you can declare it here.

Agreed. I'll update both lookup loops accordingly.

> Please make variable declarations look like a reverse Christmas tree.
> Applies to the rest of the driver, too.

Will do. I'll audit the driver rather than limiting the cleanup to this
function.

> s/1000/USEC_PER_MSEC/
>
> Ditto.

Agreed. I'll use the unit macros and include <linux/units.h>.

> No need to cast rval.
>
> Is this useful? Shouldn't this be dev_dbg()?

Agreed on both points. I'll remove the cast and change the normal status
message to dev_dbg().

> Redundant parentheses.
>
> Ditto.

Agreed. I'll simplify all four boolean helpers.

> static const?

Yes, I'll make the lookup table static const.

> s/4/sizeof(ck)/

Will do.

> pclk = get_unaligned_be32(ck);
>
> And include linux/unaligned.h. I guess it'd be safe to assume this would be
> aligned though.

Agreed. I'll use get_unaligned_be32() and include <linux/unaligned.h>.

> s/1000/KHZ_PER_MHZ/
>
> And include linux/units.h.

Will do.

> unsigned int i...?

Agreed.

> unsigned int except for err?

Agreed. I'll retain a signed type for the returned error and use unsigned
types for the block and bank indices.

> Either align the latter line to start after the opening parenthesis or wrap
> after '=' instead.

Will fix.

> width = get_unaligned_be16(buffer);
>
> height = get_unaligned_be16(buffer + 2);
>
> It'd be nicer to define a struct for this actually.
>
> Same for this one.

Agreed. I'll define structures matching the contiguous timing-register
layouts and use the unaligned big-endian accessors for their fields.

> Don't do bitwise or on error codes!

Agreed. This function also currently discards the accumulated result by
returning 0. I'll execute the updates sequentially and return the first
error.

> You can assign these in declaration as you do below.

Will do.

> { Spaces inside braces, please. }

Will fix.

> Do you need this? V4L2 already has names for formats.

This helper is only used by log_status(). I could not find an in-kernel
helper in the current tree that converts MEDIA_BUS_FMT_* values to printable
names; the fourcc helpers do not apply to media-bus codes.

Did you mean that the status output should use the standard MEDIA_BUS_FMT_*
representation instead of these custom descriptions? If so, I'll drop the
helper and report the media-bus code instead.

> 		return 0;
>
> 		return -ERANGE;
>
> 	return 1;
>
> And drop the else's.

Agreed. I'll convert this to early returns.

> Error checks are missing.
>
> The s_stream() video op is deprecated, please switch to enable_streams and
> disable_streams pad ops.

Agreed. I'll convert the register helpers to return errors, implement the pad
enable_streams and disable_streams operations, and use
v4l2_subdev_s_stream_helper for legacy callers.

> unsigned int i?

Agreed.

> These values come from DT -- see my comments on the 1st patch, too.

Agreed. I'll remove the default lane count, output-port count, and PHY type.
They will be set only from the firmware graph.

> Please don't add this, the driver should assume the configuration *is*
> available in DT. Also the NULL check is redundant;
> v4l2_fwnode_endpoint_alloc_parse() does it.

Agreed. The binding requires port@0 and its data-lanes, so silently
continuing without an endpoint is inconsistent with the binding. I'll remove
the fallback and let endpoint parsing report the error.

> Either combine the error checks or add a label for error handling, use goto
> here and below.

Agreed. I'll consolidate endpoint cleanup through a common error path.

> I wonder if it'd be cleaner to have this in probe -- error handling becomes
> difficult with these intermediate initialisation helpers that do apprently
> unrelated things.

Agreed. I'll move the sub-device, control, media-entity and state
initialization sequence into probe so the unwind path is explicit.

> You'll need to call v4l2_subdev_init_finalize() here to fully initialise
> the sub-device state.
>
> Similarly, call v4l2_subdev_cleanup() on error path after finalize() as
> well as in remove.

Agreed. I'll call v4l2_subdev_init_finalize() after initializing the media
entity and before registering the sub-device. I'll add
v4l2_subdev_cleanup() to the corresponding probe error path and the remove
path.

Kind regards,
Hermes