Hello Javier,
On Thu Oct 15, 2026 at 12:47 PM CEST, Javier Martinez Canillas wrote:
> Luca Ceresoli <[email protected]> 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/[email protected]
>>
>> Signed-off-by: Luca Ceresoli <[email protected]>
>> ---
>> 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
