From: wmb@firmworks•com (Mitch Bradley)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] ARM: vexpress: initial device tree support
Date: Wed, 11 Jan 2012 14:38:45 -1000 [thread overview]
Message-ID: <4F0E2B95.7070402@firmworks.com> (raw)
In-Reply-To: <74CDBE0F657A3D45AFBB94109FB122FF177EE3A848@HQMAIL01.nvidia.com>
On 1/11/2012 2:15 PM, Stephen Warren wrote:
> Mitch Bradley wrote at Wednesday, January 11, 2012 4:16 PM:
>> 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?
>
> The I2C busses are set up like this:
>
> bus 0 bus 1
> I2C controller -------> mux ------> dev a, dev b, dev c, ...
> \------> dev x, dev y, dev z, ...
> bus 2
>
> Thus all devices are on a child I2C bus of the mux and none on the raw
> HW bus exposed by the I2C controller itself.
>
> The I2C core will always call gpiomux_select before each transaction,
> which will set the GPIOs appropriately for bus 1 or bus 2, depending
> on which device is being communicated with.
>
>> 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.
>
> That's true, but the select action happens implicitly inside the I2C
> core for any and all transactions, AIUI, so the two modes are equivalent.
>
>> 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).
>
> That's probably the difficult part.
>
> For an I2C mux that is controlled via I2C, you can just add the mux
> node as a child of the I2C controller, since it has an I2C address,
> and so putting it there makes sense.
>
> But for an I2C mux that's controlled using GPIOs or pinmux, there's no
> I2C address so I guess the mux shouldn't be directly underneath the I2C
> controller.
>
> Perhaps the DT binding for such an I2C mux can refer to the parent I2C
> controller by phandle?
>
> Inside the I2C mux DT node, I think we can have a child node for each
> bus, and then use standard I2C child node addressing for all the nodes
> within these bus nodes.
>
> Perhaps:
The scheme below looks good to me, with minor nits picked...
>
> i2c1: i2c at 7000c000 {
> #address-cells =<1>;
> #size-cells =<0>;
> compatible = "nvidia,tegra20-i2c";
> reg =<0x7000C000 0x100>;
> interrupts =<0 38 0x04>;
> };
>
> mux at 0 {
> #address-cells =<1>;
> #size-cells =<0>;
> compatible = "nvidia,tegra20-i2c";
Shouldn't this compatible value be set up to bind to gpio_i2cmux? The
node doesn't seem to be hardware-specific.
> parent-bus =<&i2c1>;
> gpios =<&gpio 100 0&gpio 101 0>;
> gpio-values-idle =<0>; /* bitmask of values */
>
> bus at 0 {
> #address-cells =<1>;
> #size-cells =<0>;
> /*
> * The GPIO values to set as a bitmask.
> * Formatted like gpio-i2cmux.c's mux->data.values[i].
> * Or name this gpio-values?
> */
Did you mean for the comment above to be associated with the
"gpio-values-idle" property? It seems out of place here.
> reg =<1>;
reg =<0> because this is bus at 0
>
> wm8903: wm8903 at 1a {
> compatible = "wlf,wm8903";
> reg =<0x1a>;
> ...
> };
> };
>
> bus at 1 {
> #address-cells =<1>;
> #size-cells =<0>;
> reg =<2>;
reg =<1> because this is bus at 1
>
> light-sensor at 44 {
> compatible = "isil,isl29018";
> reg =<0x44>;
> ...
> };
> };
> };
>
next prev parent reply other threads:[~2012-01-12 0:38 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-09-21 9:19 [PATCH] ARM: vexpress: initial device tree support Dave Martin
2011-09-21 13:24 ` Rob Herring
2011-09-21 14:24 ` Dave Martin
2011-09-21 14:33 ` Pawel Moll
2011-09-21 15:49 ` Dave Martin
2011-09-21 14:57 ` Grant Likely
2011-09-21 16:01 ` Pawel Moll
2011-09-21 16:17 ` Dave Martin
2011-09-21 16:28 ` Pawel Moll
2011-09-21 16:37 ` Rob Herring
2011-09-21 17:15 ` Dave Martin
2011-09-21 17:47 ` Mitch Bradley
2011-09-22 12:19 ` Dave Martin
2012-01-09 23:26 ` Tabi Timur-B04825
2012-01-10 0:42 ` Mitch Bradley
2012-01-10 2:24 ` Tabi Timur-B04825
2012-01-10 12:22 ` Jamie Lokier
2012-01-10 21:58 ` Timur Tabi
2012-01-10 22:35 ` Mitch Bradley
2012-01-10 23:55 ` Stephen Warren
2012-01-11 0:02 ` Timur Tabi
2012-01-11 0:28 ` Timur Tabi
2012-01-11 6:43 ` Mitch Bradley
2012-01-11 20:17 ` Timur Tabi
2012-01-11 23:20 ` Mitch Bradley
2012-01-11 23:32 ` Timur Tabi
2012-01-11 20:29 ` Stephen Warren
2012-01-11 20:32 ` Timur Tabi
2012-01-11 20:36 ` Stephen Warren
2012-01-11 21:37 ` Timur Tabi
2012-01-11 21:57 ` Stephen Warren
2012-01-12 12:24 ` Jamie Lokier
2012-01-12 16:49 ` Stephen Warren
2012-01-11 23:16 ` Mitch Bradley
2012-01-12 0:15 ` Stephen Warren
2012-01-12 0:38 ` Mitch Bradley [this message]
2012-01-12 0:47 ` Mitch Bradley
2012-01-12 16:45 ` Stephen Warren
2012-01-12 12:09 ` Jamie Lokier
2012-01-12 16:52 ` Stephen Warren
2012-01-10 11:04 ` Dave Martin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4F0E2B95.7070402@firmworks.com \
--to=wmb@firmworks$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox