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

["Luca Ceresoli" <luca.ceresoli@bootlin.com>]

Hello Javier,

On Thu Oct 15, 2026 at 12:47 PM CEST, Javier Martinez Canillas wrote:
> Luca Ceresoli <luca.ceresoli@bootlin.com> 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@bootlin.com
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> ---
>>  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.

OK, will do, together with the change to get_unaligned_le16() suggested
below.

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

I'd rather keep them for documentation if you don't mind.

>> +	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().

Sure, will do. I felt like this patch shoudl probably have been split, but
was unsure how.

Luca

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com