On 1/11/2012 10:29 AM, Stephen Warren wrote:
Mitch Bradley wrote at Tuesday, January 10, 2012 11:43 PM:
On 1/10/2012 2:28 PM, Timur Tabi wrote:
Mitch Bradley wrote:
...
That GPIO pin thing is annoying, but not sufficiently complex or common
that it warrants having a separate EDID driver. You could define a
platform-specific property to tell your framebuffer driver that it needs
to do that GPIO thing. It's a hack, but the GPIO thing is inherently a
hack, so there will be some ugliness somewhere as a result.
I have two platform-specific functions, "enabled_edid" and "disable_edid", that
I call before/after
calling fb_ddc_read(). This seems to work well, and I already have a mechanism
for calling platform-
specific functions from the framebuffer driver.
However, Stephen Warren said I should be using the I2C mux feature instead.
I2C mux is a plausible solution, as is your enable/disable thing. At
some level they are equivalent. I2C mux is a formalization of your
solution, in which the mux device's select method must be written to
perform the function of your enable/disable edid functions.
Either way, you need platform-dependent functions to do the switching,
and you need to select the appropriate channel. Personally, I don't see
the advantage of using the mux device in this case.
The main advantage I see is that you explicitly don't need any platform-
specific functions to do the switching; you end up with platform-agnostic
code (the I2C GPIO mux driver) and platform-specific configuration for
that driver (the GPIO ID to use).
Oh, I didn't know about the I2C GPIO Mux driver. I was looking at
i2c-mux.c . I now see gpio-i2cmux.c, which indeed seems to do the right
thing.
The display driver just talks to the
I2C API for the DDC I2C bus, and doesn't do anything to switch between
the busses; the I2C GPIO mux driver does all that internally. Thus, the
display driver will work fine on boards that don't need this muxing with
zero changes; the board simply wouldn't register the mux driver.
It's just adding
complexity with no payback. If there were several channels that needed
to be accessed in an interspersed fashion, the mux device would be much
cleaner. But in this case, there is a single "back channel" that only
needs to be accessed once and can subsequently be ignored.
Well, the EDID needs to be read on every hotplug event, so it's certainly
not a one-time thing.
The video
driver can grab a lock, call enable_edid(), read out the EDID data into
an array, call disable_edid(), release the lock, and that is it. The
other users of that I2C bus can ignore the hidden EDID.
Other I2C users/devices also shouldn't be impacted by the mux; they
would continue to use the existing I2C APIs for the bus their devices
are attached to, and not know about the mux.
If other devices that are on the same bus as the EDID don't use the mux,
how does one ensure that the GPIO is restored to the non-EDID
setting when the display driver is finished?
Perhaps I'm missing something, but it appears to me that the model is to
set the correct GPIO state before each use, instead of a
save-set-use-restore model.
In any case, if there is a good way to instantiate the GPIO mux device
from the device tree, it certainly provides a ready-made solution. Each
device that is on the bus in question would have a device node that is a
child of the GPIO mux node, and the display driver could have a
phandle-valued property pointing to the mux node, plus a property
declaring the selection value (or perhaps a single 2-cell property with
phandle, selection-value).
The overhead should be
almost zero; the I2C GPIO mux driver could remember the previous bus
selection and only modify it (gpio_set_value) when switching between
busses. I guess there might be a small overhead to taking a lock to
protect the bus selection; I'm not sure whether the I2C core serializes
access already or you'd need to add this; check the existing I2C I2C
bus mux implementation I guess.
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
