Re: [PATCH 1/3] media: i2c: Add ADV761X support

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Thursday 26 September 2013 08:31:58 Hans Verkuil wrote:
> On 09/26/2013 12:57 AM, Laurent Pinchart wrote:
> > On Wednesday 25 September 2013 18:31:51 Guennadi Liakhovetski wrote:
> >> On Wed, 25 Sep 2013, Valentine wrote:
> >>> On 09/25/2013 06:08 PM, Guennadi Liakhovetski wrote:
> > [snip]
> > 
> >>>>>>> +/* I2C I/O operations */
> >>>>>>> +static s32 adv_smbus_read_byte_data(struct i2c_client *client, u8
> >>>>>>> command)
> >>>>>>> +{
> >>>>>>> +	s32 ret, i;
> >>>>>>> +
> >>>>>>> +	for (i = 0; i < 3; i++) {
> >>>>>>> +		ret = i2c_smbus_read_byte_data(client, command);
> >>>>>> 
> >>>>>> Uhm, why do you need to do this 3 times?... I see adv7842 does that
> >>>>>> too... but e.g. adv7604 dies this only for writing and not for
> >>>>>> reading...
> >>>>> 
> >>>>> Just thought it would be safe to retry in case of a failure.
> >>>>> Other drivers seem to retry I2C I/O as well. This is probably related
> >>>>> to the possible cable issues. Not sure. Anyways, retrying doesn't seem
> >>>>> to hurt, does it?
> >>>> 
> >>>> It does, because it means there's something we don't understand. IMHO
> >>>> it should either work from the first time, or there should be an error,
> >>>> that we understand with a following error processing, that _might_
> >>>> include re-trying. Re-trying just in case isn't good. Especially if no
> >>>> error has been observed.
> >>> 
> >>> I have observed very rare errors when reading HDMI status. Just a couple
> >>> of times during this week. I'm not sure of the nature of those errors.
> >>> Just thought it would be OK to retry since other decoder drivers do so
> >>> as well.
> >> 
> >> Ok, understand. As I said, I personally don't like that, but, I think,
> >> Laurent will have a final word on this.
> > 
> > I don't like the idea of blindly retrying, especially for write
> > operations. If a read fails due to a flaky cable, there's equal chances
> > that a write would fail as well, which could result in writing random
> > values to random registers. Given the side effects that this could have,
> > I'd much rather not retry I/O operations and have the users fix
> > electrical issues. Hiding something this serious could be dangerous.
> 
> For a ubiquitous and relatively slow-speed bus the i2c bus is surprisingly
> flaky in my experience. I've seen surprisingly many cases where a retry was
> useful or even necessary. There can be many causes: electrical issues is
> one, and while that shouldn't happen, in practice it does. Interference by
> other devices on the bus is another. And sometimes the device itself is
> flaky: in the case of the adv7511 trying to enable/disable an interrupt
> just fails every so often.

If we enable retries by default, what could happen is that new boards subject 
to electrical issues on their I2C bus will not be seen as broken until much 
later in the development, and possibly too late, after going to production. I 
don't want to blindly hide the problem, if an electrical issue is present it 
should be seen. Now, if we need to support an existing broken board, I'm fine 
with enabling retries, but it shouldn't be enabled by default.

> I have yet to see a product bringup where the i2c bringup 'just works'.
> There are always problems there.

-- 
Regards,

Laurent Pinchart