[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm: Sparse warnings due to __u16 usage for little-endian data
--

commit 8f50576e49682d9068991d3e7de12fc063b0685f
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>
drm/bridge: ite-it66121: improve reading IDs and log on errors

This commit switches from individual regmap_read() calls to a single
regmap_bulk_read() when fetching vendor and device IDs. It introduces a
union to explicitly handle the data bytes and adds proper error checking
to prevent silently continuing with invalid values.

> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> @@ -1579,7 +1579,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;
> +		};

[Severity: Low]
Should these fields be declared as __le16 instead of __u16?

Since they hold little-endian data populated directly by regmap_bulk_read()
and are later passed to le16_to_cpu(), using natively-typed __u16 here
violates the kernel's endianness typing rules and triggers Sparse warnings
for incorrect types.

> +	} chip_id_regs;
> +	BUILD_BUG_ON(sizeof(chip_id_regs) != 4);
> +	u16 vendor_id, device_id;

[ ... ]

> @@ -1643,19 +1652,22 @@ static int it66121_probe(struct i2c_client *client)

[ ... ]

> +	vendor_id = le16_to_cpu(chip_id_regs.vendor);
> +	device_id = le16_to_cpu(chip_id_regs.device);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-ite-it66121-log-probe-errors-v1-0-7fd6994e0be0@bootlin.com?part=1

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

[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

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

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