From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jon Hunter Subject: Re: [RFC] tegra: dpaux: pinctrl proposal Date: Tue, 2 Jun 2015 10:18:36 +0100 Message-ID: <556D74EC.6080501@nvidia.com> References: <1431963229-12867-1-git-send-email-jonathanh@nvidia.com> <20150519144654.GG26748@ulmo.nvidia.com> <555C901F.8090009@nvidia.com> <20150520154022.GB7734@ulmo.nvidia.com> <555CAE47.6070907@nvidia.com> <555CDC82.1010104@wwwdotorg.org> <555DD7B3.6020608@nvidia.com> <20150521140356.GA28021@ulmo.nvidia.com> <555F1E3E.7040500@nvidia.com> <20150522143706.GA19922@ulmo.nvidia.com> <55688AB4.2030505@nvidia.com> <556CB183.5020606@wwwdotorg.org> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <556CB183.5020606-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> Sender: linux-tegra-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Stephen Warren , Thierry Reding Cc: Linus Walleij , Alexandre Courbot , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-tegra@vger.kernel.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 = ; >> 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