Re: [PATCH] OMAP: I2C driver for TI OMAP boards #2

[Jean Delvare]

Hi David,

> On Monday 31 July 2006 9:13 am, Jean Delvare wrote:
> >	 If you want things to improve, please help by
> > reviewing Komal's driver. I think I understand you already commented on
> > it, but I'd like you to really review it, and add a formal approval to
> > it (e.g. Signed-off-by or Acked-by). Then I'll review it for merge.
> 
> The issues noted in the code are still almost all low priority
> (non-blocking).
> 
>  - The FIXME about choosing the address is very low priority,
>    and would affect only multi-master systems.  The fix would
>    involve defining a new i2c-specific struct for platform_data,
>    updating various boards to use it (e.g. OSK can use 400 MHz),
>    and wouldn't change behavior for any board I've ever seen.

Given that the slave mode isn't supported by Linux at the moment, I
don't even see how this is relevant. (So I agree with you that this is
very low priority - I would even kill that part of the code.)

> 
>  - Likewise with the REVISIT for the bus speed to use.  They'd
>    be fixed with the same patch.

I don't see any relation with the slave address mechanism. The bus
speed is a simple parameter, internal to the device (i2c-core doesn't
care) so it should be very easy to move it to platform data. Not that I
really care, though.

>  - The REVISIT about maybe a better way to probe is also low
>    priority; someone with a board that needs better probing
>    could address it at that time.  (Then restest any changes
>    on multiple generations of silicion ... which IMO is the
>    role the linux-omap tree should play.)

No, it's not low priority, it's a blocker. I can't let that code go in.
The driver must do what it is told to do. If it can't, it must fail.
Yes, this means no (in-kernel) probing on this bus at the moment. Blame
it on the hardware manufacturer (if the chip actually can't do it.) In
user-space, i2cdetect can be told to use byte reads for probing.

>  - The revisit about adap->retries is still up in the air,
>    and was a question in my submission from last year.
>    How exactly is that supposed to be used?  Right now
>    it's neither initialized (except to zero) nor tested.

This is an optional feature, most i2c adapter drivers don't implement
it. I'm fine without it, this can always be implemented later if needed.

I just sent my own review of the code. As you can see, I had quite a
few comments. Hopefully you now agree that pushing that code to Linus
right away wouldn't have been wise.

Thanks,
-- 
Jean Delvare
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/