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