Re: [PATCH] OMAP: board-files: fix i2c_bus for tfp410

[Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

On 2012-11-16 15:51, Felipe Balbi wrote:
> Hi,
> 
> On Fri, Nov 16, 2012 at 02:22:33PM +0200, Tomi Valkeinen wrote:
>> The i2c handling in tfp410 driver, which handles converting parallel RGB
>> to DVI, was changed in 958f2717b84e88bf833d996997fda8f73276f2af. The
> 
> commit summary should be added in () after commit hash. This would look
> like:
> 
> 'was changed in 958f271 (OMAPDSS: TFP410: pdata rewrite).'

Yep.

>> patch changed what value the driver considers as invalid/undefined.
>> Before the patch 0 was the invalid value, but as 0 is a valid bus
>                   ^
> 		  missing comma (,) character here.

Right.

>> number, the patch changed this to -1.
>>
>> However, the fact was missed that many board files do not define the bus
>> number at all, thus it's left to 0. This causes the driver to fail to
>> get the i2c bus, exiting from the driver's probe with an error, meaning
>> that the DVI output does not work for those boards.
>>
>> This patch fixes the issue by changing the i2c_bus number field in the
>> driver's platform data from u16 to int, and setting the bus number to -1
>> in the board files for the boards that did not define the bus. The
>> exception is devkit8000, for which the bus is set to 1, which is the
>> correct bus for that board.
>>
>> The bug exists in v3.5+ kernels.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen-l0cyMroinI0@public.gmane.org>
>> Reported-by: Thomas Weber <thomas-xePHB1Atl/Dsq35pWSNszA@public.gmane.org>
>> [for v3.5, v3.6 stable kernels]
>> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 
> This format is peculiar. Usually people use:
> 
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v3.5 v3.6

Yes, I tried that. But my git send-email (1.7.10.4) rejects that line. I
don't know if it's my setup, that particular git version, or what...

> To be fair, the whole i2c_bus_num looks like a big hackery introduced by
> the way panel drivers are written for OMAP DSS.
> 
> TFP410 is an I2C client, not an OMAPDSS client. After a quick look at
> the driver, there's is no such thing as a DSS bus, so looks like you
> should have an I2C driver for TFP410 and the whole DSS stuff should be
> just a list of clients, but not a struct bus at all.
> 
> The fact that you have to pass the I2C bus number down to the panel
> driver is already a big indication of how wrong this is, IMHO.

Without going deeper in the dss device model problems, I would agree
with you if this was about i2c panel, but this is not quite like that.

A panel controlled via i2c would be an i2c device. But TFP410 is not
controlled via i2c. It's not really controlled at all except via
power-down gpio. TFP410 doesn't need the i2c to be functional at all.

The i2c lines do not even touch TFP410 chip, so to be precise, the i2c
lines should not be TFP410's concern. The i2c lines come from the
monitor and go to OMAP's i2c pins. But TFP410 driver is a convenient
place to manage them.

(As a side note, TFP410 _could_ be controlled via i2c, if it would've
been setup so in the board hardware. That would be totally different
thing, though, than what's discussed here.).

Anyway, this patch wasn't meant to fix dss device model problems. It's
meant to fix a bug in the kernel.

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 897 bytes --]