[PATCH 0/3] drm/bridge: ite-it66121: use bulk read, improve probe logging

[PATCH 0/3] drm/bridge: ite-it66121: use bulk read, improve probe logging

[Luca Ceresoli]

This small series improves it66121_probe() in various ways:

 * use a bulk read for reading the vendor and device ID registers
 * log errors in ID reading and matching instead of failing silently
 * on successful probe be quiet on !DEBUG, more informative on DEBUG

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
Luca Ceresoli (3):
      drm/bridge: ite-it66121: improve reading IDs and log on errors
      drm/bridge: ite-it66121: log an error if the IDs do not match
      drm/bridge: ite-it66121: make successful probe quiet

 drivers/gpu/drm/bridge/ite-it66121.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)
---
base-commit: 49477cc277e0a432a8c08766b8ec764ee69622a7
change-id: 20261015-drm-ite-it66121-log-probe-errors-9c8cfef8a7f7

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

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

[Luca Ceresoli]

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);
+	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]);
+	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);
 
 	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) {
 			ctx->id = chip_info->id;
 			break;
 		}

-- 
2.56.0

[PATCH 2/3] drm/bridge: ite-it66121: log an error if the IDs do not match

[Luca Ceresoli]

If the IDs read from the chip do not match any of the known models,
it66121_probe() fails silently, making troubleshooting unnecessarily
difficult. Log an error to clarify the failure reason, showing the IDs read
from the hardware.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 9b43456c67a1..49eb8318e974 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -1675,7 +1675,8 @@ static int it66121_probe(struct i2c_client *client)
 	}
 
 	if (i == ARRAY_SIZE(it66xx_chip_info))
-		return -ENODEV;
+		return dev_err_probe(dev, -ENODEV, "Unknown vendor %04x device %04x (rev %01x)\n",
+				     vendor_id, device_id, revision_id);
 
 	ctx->bridge.of_node = dev->of_node;
 	ctx->bridge.type = DRM_MODE_CONNECTOR_HDMIA;

-- 
2.56.0

[PATCH 3/3] drm/bridge: ite-it66121: make successful probe quiet

[Luca Ceresoli]

When drivers are working properly they should be quiet. Turn the dev_info()
into a dev_dbg(). While there, also make it more informative.

Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
---
 drivers/gpu/drm/bridge/ite-it66121.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
index 49eb8318e974..11647b1ac735 100644
--- a/drivers/gpu/drm/bridge/ite-it66121.c
+++ b/drivers/gpu/drm/bridge/ite-it66121.c
@@ -1709,7 +1709,8 @@ static int it66121_probe(struct i2c_client *client)
 
 	drm_bridge_add(&ctx->bridge);
 
-	dev_info(ctx->dev, "IT66121 revision %d probed\n", revision_id);
+	dev_dbg(ctx->dev, "IT66121 vendor %04x device %04x revision %d probed\n",
+		vendor_id, device_id, revision_id);
 
 	return 0;
 }

-- 
2.56.0

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

[Javier Martinez Canillas]

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.

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

Re: [PATCH 2/3] drm/bridge: ite-it66121: log an error if the IDs do not match

[Javier Martinez Canillas]

Luca Ceresoli <luca.ceresoli@bootlin.com> writes:

> If the IDs read from the chip do not match any of the known models,
> it66121_probe() fails silently, making troubleshooting unnecessarily
> difficult. Log an error to clarify the failure reason, showing the IDs read
> from the hardware.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/3] drm/bridge: ite-it66121: make successful probe quiet

[Javier Martinez Canillas]

Luca Ceresoli <luca.ceresoli@bootlin.com> writes:

> When drivers are working properly they should be quiet. Turn the dev_info()
> into a dev_dbg(). While there, also make it more informative.
>
> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> ---



-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 3/3] drm/bridge: ite-it66121: make successful probe quiet

[Javier Martinez Canillas]

Javier Martinez Canillas <javierm@redhat.com> writes:

> Luca Ceresoli <luca.ceresoli@bootlin.com> writes:
>
>> When drivers are working properly they should be quiet. Turn the dev_info()
>> into a dev_dbg(). While there, also make it more informative.
>>
>> Signed-off-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> ---

Ups, forgot to add:

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[Luca Ceresoli]

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

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

[Javier Martinez Canillas]

"Luca Ceresoli" <luca.ceresoli@bootlin.com> writes:

> Hello Javier,
>
> On Thu Oct 15, 2026 at 12:47 PM CEST, Javier Martinez Canillas wrote:

[...]

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

Sure, I'm Ok with that.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat