Re: [PATCH 13/21] media: i2c: it6625: decode detected timings via typed register structs

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hermes,

Thank you for the patches.

On Fri, Sep 18, 2026 at 04:57:28PM +0800, Hermes Wu via B4 Relay wrote:
> From: Hermes Wu <Hermes.wu@ite.com.tw>
> 
> it6625_get_detected_timings() manually assembled each 16-bit field from
> raw byte-buffer offsets with a shift-and-add sequence. Define two local
> structs of __be16 fields matching the contiguous REG_H_ACTIVE_1..
> REG_V_ACTIVE_0 and REG_H_FP_1..REG_V_BP_0 register layouts, read
> directly into them, and decode each field with be16_to_cpu(). Guard
> each struct's size with static_assert() against the expected register
> range width.
> 
> Every member is 2 bytes wide and naturally aligned, so the struct is
> laid out with no padding -- this is safe because the struct is the I2C
> read target itself, not a cast over a pre-existing raw buffer.
> 
> Signed-off-by: Hermes Wu <Hermes.wu@ite.com.tw>
> ---
>  drivers/media/i2c/it6625.c | 41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/i2c/it6625.c b/drivers/media/i2c/it6625.c
> index 60c79a2277941621c7aa83189244b706a5505d66..90b87dbf54fcfc7ad2a1245d4594beee24a193ca 100644
> --- a/drivers/media/i2c/it6625.c
> +++ b/drivers/media/i2c/it6625.c
> @@ -769,10 +769,22 @@ static int it6625_get_detected_timings(struct it6625 *it6625,
>  				       struct v4l2_dv_timings *timings)
>  {
>  	struct v4l2_bt_timings *bt = &timings->bt;
> +	struct {
> +		__be16 h_active;
> +		__be16 v_active;
> +	} active;
> +	struct {
> +		__be16 hfrontporch;
> +		__be16 hsync;
> +		__be16 hbackporch;
> +		__be16 vfrontporch;
> +		__be16 vsync;
> +		__be16 vbackporch;
> +	} porch;

The driver accesses many such register areas, I'd define these separate
from the functions that use them. This isn't the only one case.

Alternatively you could define each register separately, which is what most
drivers do, albeit the usage pattern in this driver is a bit atypical so I
think using structs for this indeed could make sense.

>  	int val;
> -	unsigned int width, height;
> -	u8 buffer[4];
> -	u8 buffer2[12];
> +
> +	static_assert(sizeof(active) == 4);
> +	static_assert(sizeof(porch) == 12);
>  
>  	if (no_signal(it6625)) {
>  		dev_err(it6625->dev, "no signal detected");
> @@ -792,24 +804,21 @@ static int it6625_get_detected_timings(struct it6625 *it6625,
>  	bt->interlaced = val & B_INTERLACE ?
>  			 V4L2_DV_INTERLACED : V4L2_DV_PROGRESSIVE;
>  
> -	if (it6625_read_bytes(it6625, REG_H_ACTIVE_1, buffer, 4) < 0)
> +	if (it6625_read_bytes(it6625, REG_H_ACTIVE_1, (u8 *)&active, sizeof(active)) < 0)
>  		return -EIO;
>  
> -	width = ((buffer[0] & 0xff) << 8) + buffer[1];
> -	height = ((buffer[2] & 0xff) << 8) + buffer[3];
> -
> -	bt->width = width;
> -	bt->height = height;
> +	bt->width = be16_to_cpu(active.h_active);
> +	bt->height = be16_to_cpu(active.v_active);
>  
> -	if (it6625_read_bytes(it6625, REG_H_FP_1, buffer2, 12) < 0)
> +	if (it6625_read_bytes(it6625, REG_H_FP_1, (u8 *)&porch, sizeof(porch)) < 0)
>  		return -EIO;
>  
> -	bt->hfrontporch = ((buffer2[0] & 0xff) << 8) + buffer2[1];
> -	bt->hsync = ((buffer2[2] & 0xff) << 8) + buffer2[3];
> -	bt->hbackporch = ((buffer2[4] & 0xff) << 8) + buffer2[5];
> -	bt->vfrontporch = ((buffer2[6] & 0xff) << 8) + buffer2[7];
> -	bt->vsync = ((buffer2[8] & 0xff) << 8) + buffer2[9];
> -	bt->vbackporch = ((buffer2[10] & 0xff) << 8) + buffer2[11];
> +	bt->hfrontporch = be16_to_cpu(porch.hfrontporch);
> +	bt->hsync = be16_to_cpu(porch.hsync);
> +	bt->hbackporch = be16_to_cpu(porch.hbackporch);
> +	bt->vfrontporch = be16_to_cpu(porch.vfrontporch);
> +	bt->vsync = be16_to_cpu(porch.vsync);
> +	bt->vbackporch = be16_to_cpu(porch.vbackporch);
>  
>  	bt->pixelclock = it6625_get_pclk(it6625);
>  	if (bt->interlaced == V4L2_DV_INTERLACED) {
> 

-- 
Kind regards,

Sakari Ailus