Re: [PATCH 1/3] drm/bridge: ite-it66121: improve reading IDs and log on errors

[Javier Martinez Canillas]

Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx> writes:

Hello Luca,

> it66121_probe() calls regmap_read() four times to read the vendor and
> device ID registers, without error checking. This leads to silently
> continuing using invalid ID values when any of the reads fail (which has
> happened and is being fixed [0]).
>
> Adding error checking on all 4 regmap_read()s would be annoying, so switch
> to regmap_bulk_read().
>
> In turn a bulk read requires a differen data structure, so take that as an
> opportunity to make the vendor/device ID computation more explicit.
>
> [0] https://lore.kernel.org/r/20261015-drm-ite-it66121-fix-reset-v1-1-473038e06abc@xxxxxxxxxxx
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
> ---
>  drivers/gpu/drm/bridge/ite-it66121.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> index 692a85e1e2e8..9b43456c67a1 100644
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -1580,7 +1580,16 @@ static const struct it66121_chip_info it66xx_chip_info[] = {
>  
>  static int it66121_probe(struct i2c_client *client)
>  {
> -	u32 revision_id, vendor_ids[2] = { 0 }, device_ids[2] = { 0 };
> +	union {
> +		__u8 byte[4];
> +		struct {
> +			__u16 vendor;
> +			__u16 device;
> +		};
> +	} chip_id_regs;
> +	BUILD_BUG_ON(sizeof(chip_id_regs) != 4);

I think that would be simpler and easier to read if you just use a
u8 chip_id_regs[4] variable, instead of the union.

> +	u16 vendor_id, device_id;
> +	u32 revision_id;
>  	struct device_node *ep;
>  	int ret, i;
>  	struct it66121_ctx *ctx;
> @@ -1644,19 +1653,22 @@ static int it66121_probe(struct i2c_client *client)
>  	if (IS_ERR(ctx->regmap))
>  		return PTR_ERR(ctx->regmap);
>  
> -	regmap_read(ctx->regmap, IT66121_VENDOR_ID0_REG, &vendor_ids[0]);
> -	regmap_read(ctx->regmap, IT66121_VENDOR_ID1_REG, &vendor_ids[1]);
> -	regmap_read(ctx->regmap, IT66121_DEVICE_ID0_REG, &device_ids[0]);
> -	regmap_read(ctx->regmap, IT66121_DEVICE_ID1_REG, &device_ids[1]);

You should probably drop these defines if will no longer be used. Instead,
maybe you could define a IT66121_CHIP_ID_REGS_START or something like that?

Although maybe is good to keep them as documentation? Not sure about this.

> +	ret = regmap_bulk_read(ctx->regmap, IT66121_VENDOR_ID0_REG,
> +			       &chip_id_regs, sizeof(chip_id_regs));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Error reading device IDs\n");
>  
>  	/* Revision is shared with DEVICE_ID1 */
> -	revision_id = FIELD_GET(IT66121_REVISION_MASK, device_ids[1]);
> -	device_ids[1] &= IT66121_DEVICE_ID1_MASK;
> +	revision_id = FIELD_GET(IT66121_REVISION_MASK, chip_id_regs.byte[3]);
> +	chip_id_regs.byte[3] &= IT66121_DEVICE_ID1_MASK;
> +
> +	vendor_id = le16_to_cpu(chip_id_regs.vendor);
> +	device_id = le16_to_cpu(chip_id_regs.device);
>

And here you could use get_unaligned_le16() instead, to not have to cast
when using just a u8 array.

>  	for (i = 0; i < ARRAY_SIZE(it66xx_chip_info); i++) {
>  		chip_info = &it66xx_chip_info[i];
> -		if ((vendor_ids[1] << 8 | vendor_ids[0]) == chip_info->vid &&
> -		    (device_ids[1] << 8 | device_ids[0]) == chip_info->pid) {
> +		if (vendor_id == chip_info->vid &&
> +		    device_id == chip_info->pid) {

I wonder if this could be done in a separate patch.

That is, keep the << 8 | for this patch and just do the regmap_read() to
regmap_bulk_read() conversion. Then as a separate patch you could replace
it by converting with le16_to_cpu() or get_unaligned_le16().

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat