Re: [RFC] tegra: dpaux: pinctrl proposal

[Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>]

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...

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

> 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?

> Thierry, my understanding is the i2c-over-aux protocol is a simplified
> version of the i2c protocol. From a DT perspective would a dpaux device
> ever have i2c client devices as children like a normal i2c device has? I
> am wondering if it would be valid in this case to tell the i2c-core not
> to search for children. Although that sounds like a hack. I am not sure
> if the i2c folks would allow us to make these dev_dbg() prints.