Re: [PATCH 03/10] V4L/DVB: tda18271: Add some hint about what tda18217 reg ID returned

Michael Krufky
2010-09-30T21:03:51Z

On Thu, Sep 30, 2010 at 4:26 PM, Mauro Carvalho Chehab
<mchehab@redhat.com> wrote:
> Em 30-09-2010 16:27, Michael Krufky escreveu:
>> Mauro,
>>
>> I think that's a reasonable explanation.  Would you be open to
>> reworking the patch such that the register contents only show up if
>> the device is not recognized?  (when ret < 0) . In the case where the
>> device is correctly identified (ret == 0), I'd rather preserve the
>> original successful detection message, and not see the ID register
>> contents.
>
> Patch enclosed.
>
> ---
>
> [PATCH] V4L/DVB: tda18271: Add some hint about what tda18217 reg ID returned
>
> Instead of doing:
>
> [   82.581639] tda18271 4-0060: creating new instance
> [   82.588411] Unknown device detected @ 4-0060, device not supported.
> [   82.594695] tda18271_attach: [4-0060|M] error -22 on line 1272
> [   82.600530] tda18271 4-0060: destroying instance
>
> Print:
> [  468.740392] Unknown device (0) detected @ 4-0060, device not supported.
>
> for the error message, to help detecting what's going wrong with the
> device.
>
> This helps to detect when the driver is using the wrong I2C bus (or have
> the i2g gate switch pointing to the wrong place), on devices like cx231xx
> that just return 0 on reads to a non-existent i2c device.
>
> Signed-off-by: Mauro Carvalho Chehab <mchehab@redhat.com>
>
> diff --git a/drivers/media/common/tuners/tda18271-fe.c b/drivers/media/common/tuners/tda18271-fe.c
> index 7955e49..3db8727 100644
> --- a/drivers/media/common/tuners/tda18271-fe.c
> +++ b/drivers/media/common/tuners/tda18271-fe.c
> @@ -1156,7 +1156,6 @@ static int tda18271_get_id(struct dvb_frontend *fe)
>        struct tda18271_priv *priv = fe->tuner_priv;
>        unsigned char *regs = priv->tda18271_regs;
>        char *name;
> -       int ret = 0;
>
>        mutex_lock(&priv->lock);
>        tda18271_read_regs(fe);
> @@ -1172,17 +1171,18 @@ static int tda18271_get_id(struct dvb_frontend *fe)
>                priv->id = TDA18271HDC2;
>                break;
>        default:
> -               name = "Unknown device";
> -               ret = -EINVAL;
> -               break;
> +               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
> +                        regs[R_ID],
> +                        i2c_adapter_id(priv->i2c_props.adap),
> +                        priv->i2c_props.addr);
> +               return -EINVAL;
>        }
>
> -       tda_info("%s detected @ %d-%04x%s\n", name,
> +       tda_info("%s detected @ %d-%04x\n", name,
>                 i2c_adapter_id(priv->i2c_props.adap),
> -                priv->i2c_props.addr,
> -                (0 == ret) ? "" : ", device not supported.");
> +                priv->i2c_props.addr);
>
> -       return ret;
> +       return 0;
>  }
>
>  static int tda18271_setup_configuration(struct dvb_frontend *fe,
>
>

This looks good to me, although you could concatenate these lines together:

> +               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
> +                        regs[R_ID],
> +                        i2c_adapter_id(priv->i2c_props.adap),
> +                        priv->i2c_props.addr);
> +               return -EINVAL;

to be more like this:

> +               tda_info("Unknown device (%i) detected @ %d-%04x, device not supported.\n",
> +                        regs[R_ID], i2c_adapter_id(priv->i2c_props.adap), priv->i2c_props.addr);
> +               return -EINVAL;

...that is, if it fits within an 80-char line.

Anyway,

Reviewed-by: Michael Krufky <mkrufky@kernellabs.com>

Thank you.

Regards,

Mike Krufky