Re: [RFC] tegra: dpaux: pinctrl proposal

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 01/06/15 20:24, Stephen Warren wrote:
> On 05/29/2015 09:50 AM, Jon Hunter wrote:
>>
>> On 22/05/15 15:37, Thierry Reding wrote:
>>> I'd still clearly prefer to have the pinctrl code live directly in the
>>> DPAUX driver, so I think we should at least give that a shot.
>>
>> I have been working on this more this week and the good news is that by
>> using some of the pinconf-generic handlers I can simplify the code and
>> avoid moving headers and structures around.
>>
>> However, I have ran into another issue. The current binding looks like
>> this ...
>>
>>               dpaux: dpaux@0,545c0000 {
>>                       compatible = "nvidia,tegra124-dpaux";
>>                       reg = <0x0 0x545c0000 0x0 0x40000>;
>>                       interrupts = <GIC_SPI 159 IRQ_TYPE_LEVEL_HIGH>;
>>                       clocks = <&tegra_car TEGRA124_CLK_DPAUX>,
>>                                <&tegra_car TEGRA124_CLK_PLL_DP>;
>>                       clock-names = "dpaux", "parent";
>>                       resets = <&tegra_car 181>;
>>                       reset-names = "dpaux";
>>                       status = "disabled";
>>
>>                       dpaux_state: dpaux_state0 {
>>                               dpaux {
>>                                       groups = "dpaux_io";
>>                                       function = "dpaux";
>>                                       nvidia,dpaux-drvi;
>>                                       nvidia,dpaux-drvz;
>>                                       nvidia,dpaux-cmh;
>>                               };
>>                       };
>>
>>                       i2c_state: i2c_state0 {
>>                               i2c {
>>                                       groups = "dpaux_io";
>>                                       function = "i2c";
>>                               };
>>                       };
>>
>> This all works well, however, because the display-port binding now has
>> child devices which are not i2c clients I see the following messages
>> during kernel boot ...
>>
>> [    1.607606] i2c i2c-6: of_i2c: modalias failure on
>> /host1x@0,50000000/dpaux@0,545c0000/dpaux_state0
>> [    1.616658] i2c i2c-6: of_i2c: modalias failure on
>> /host1x@0,50000000/dpaux@0,545c0000/i2c_state0
> 
> Hmm. The DT binding doc for dpaux doesn't say anything about the device
> being an I2C controller and hence inheriting/aggregating the core I2C
> schema. It should...

Yes that would definitely be clearer.

> Equally, being an I2C controller means the node should have
> #address-cells/#size-cells properties for the I2C address.

Agree.

>> In other words, i2c_add_adapter() (which is called by probing the dpaux)
>> then parses the child nodes looking for i2c client devices. However,
>> these child devices are not i2c client devices and hence the above error
>> messages. I can't find an easy way to avoid this. There is no
>> side-effect from these messages, but I would prefer not to see them.
> 
> If this were a completely new binding, I think the best way would be to
> have the dpaux node contain a child node for each semantic service
> implemented by the device, e.g.:
> 
> dpaux {
>     compatible = "nvidia,tegra124-dpaux";
>     ...
>     pinctrl {
>         dpaux_state: dpaux_state0 {
>             ...
>         i2c_state: i2c_state0 {
>             ...
>     };
>     i2c-bus {
>         // i2c devices go here
>         // I2C core pointed at this sub-node, not the dpaux node
>     };
> };
> 
> I guess we can't change the binding this way since it already exists,
> unless we introduce a new compatible value to distinguish the old and
> new styles.
> 
> Perhaps i2c_add_adapter should only attempt to instantiate devices for
> sub-nodes that contain a compatible and/or a reg property?

It does. However, because I tried to add the pinctrl mappings as
sub-nodes, this causes the i2c-core to dump error messages during
initialisation of the dpaux driver, because these child nodes are not
i2c devices. So what I have works, but I see these error messages from
the i2c-core, which in this case are benign.

Therefore, to avoid the i2c error messages, the i2c-bus and pinctrl
nodes need to be sub-nodes like you have above. I like your above
proposal, but like you said, it does mean adding a new compatibility
string not to break existing dtbs.

Cheers
Jon