[PATCH v5 02/23] drm/i2c: tda998x: check more I/O errors (original) (raw)

Russell King - ARM Linux linux at arm.linux.org.uk
Sun Feb 2 08:20:58 PST 2014

On Sat, Jan 25, 2014 at 06:14:45PM +0100, Jean-Francois Moine wrote:

This patch adds more error checking inn I2C I/O functions. In case of I/O error, this permits to avoid writing in bad controller pages, a bad chipset detection or looping when getting the EDID.

I've just looked at this again, and spotted something:

-static uint8t +static int regread(struct tda998xpriv *priv, uint16t reg) { uint8t val = 0; - regreadrange(priv, reg, &val, sizeof(val)); + int ret; + + ret = regreadrange(priv, reg, &val, sizeof(val)); + if (ret < 0) + return ret;

So yes, this can return negative numbers.

@@ -1158,8 +1184,11 @@ tda998xencoderinit(struct i2cclient *client, tda998xreset(priv);

/* read version: */ - priv->rev = regread(priv, REGVERSIONLSB) | - regread(priv, REGVERSIONMSB) << 8;_ _+ ret = regread(priv, REGVERSIONLSB) |_ _+ (regread(priv, REGVERSIONMSB) << 8);_ _+ if (ret < 0)_ _+ goto fail;_ _+ priv->rev = ret;

Two issues here:

  1. The additional parens are /really/ not required.
  2. What if reg_read(priv, REG_VERSION_MSB) returns a negative number?

If we're going to the extent of attempting to make the read/write functions return errors, we should at least handle errors generated by them properly, otherwise it's pointless making them return errors.

ret = reg_read(priv, REG_VERSION_LSB);
if (ret < 0)
    goto fail;

priv->rev = ret;

ret = reg_read(priv, REG_VERSION_MSB);
if (ret < 0)
    goto fail;

priv->rev |= ret << 8;

If you want it to look slightly nicer:

int rev_lo, rev_hi;

rev_lo = reg_read(priv, REG_VERSION_LSB);
rev_hi = reg_read(priv, REG_VERSION_MSB);
if (rev_lo < 0 || rev_hi < 0) {
    ret = rev_lo < 0 ? rev_lo : rev_hi;
    goto fail;
}

priv->rev = rev_lo | rev_hi << 8;

I'm happy to commit such a change after this patch to clean it up, or if you want to regenerate your patch 2 and post /just/ that incorporating this change.

-- FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad. Estimate before purchase was "up to 13.2Mbit".

More information about the dri-devel mailing list