Re: [RFC] tegra: dpaux: pinctrl proposal

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 8380 bytes --]

On Thu, May 21, 2015 at 02:03:47PM +0100, Jon Hunter wrote:
> 
> On 20/05/15 20:12, Stephen Warren wrote:
> > On 05/20/2015 09:54 AM, Jon Hunter wrote:
> >>
> >> On 20/05/15 16:40, Thierry Reding wrote:
> >>> * PGP Signed by an unknown key
> >>>
> >>> On Wed, May 20, 2015 at 02:46:07PM +0100, Jon Hunter wrote:
> >>>>
> >>>> On 19/05/15 15:46, Thierry Reding wrote:
> >>>>>> Old Signed by an unknown key
> >>>>>
> >>>>> On Mon, May 18, 2015 at 04:33:49PM +0100, Jon Hunter wrote:
> >>>>>> Background:
> >>>>>> ==========
> >>>>>> On tegra124 and tegra132 devices the pads used by the Display Port
> >>>>>> Auxiliary
> >>>>>> (DPAUX) channel are multiplexed such that they can also be used by
> >>>>>> one of the
> >>>>>> internal i2c controllers. Note that this is different from
> >>>>>> i2c-over-AUX
> >>>>>> supported by the DPAUX controller. The register that configures
> >>>>>> these pads is
> >>>>>> part of the DPAUX controllers register set and so requires the
> >>>>>> clock for the
> >>>>>> DPAUX controller to be enabled to access the register as well as
> >>>>>> keeping the
> >>>>>> SOR (serial output resource) power domain enabled.
> >>>>>>
> >>>>>> Currently, there is no pinctrl device for these pads and so cannot
> >>>>>> be easily
> >>>>>> mapped to function as an i2c interface. Furthermore, when using
> >>>>>> the pads for
> >>>>>> the DPAUX channel, the DPAUX driver
> >>>>>> (drivers/gpu/drm/tegra/dpaux.c) directly
> >>>>>> writes the to appropriate register to setup the pads.
> >>>>>>
> >>>>>> There are some products based upon the tegra132 that use these
> >>>>>> pads for an
> >>>>>> internal i2c controller and hence we want to support this
> >>>>>> configuration in the
> >>>>>> kernel.
> >>>>>
> >>>>> Good timing, I was going to (reluctantly) add this to my long TODO
> >>>>> list.
> >>>>> I generally like the proposal.
> >>>>
> >>>> Ok, great.
> >>>>
> >>>>>> Proposal:
> >>>>>> ========
> >>>>>> Add a DPAUX MFD device that consists of a DPAUX controller, for
> >>>>>> the Display
> >>>>>> Port Auxiliary related functionality and a DPAUX pad controller,
> >>>>>> for handling
> >>>>>> the pinctrl for the DPAUX pads. Both the DPAUX controller and
> >>>>>> DPAUX pad
> >>>>>> controller need to access the DPAUX register set and therefore, by
> >>>>>> making the
> >>>>>> MFD compatible with "simple-mfd" and "syscon", a regmap for the
> >>>>>> DPAUX registers
> >>>>>> will be created to synchronise register accesses made by the drivers.
> >>>>>
> >>>>> Can we not do without an MFD here? Not only would it break DT ABI, but
> >>>>> it's also way more complicated than it needs to be in my opinion,
> >>>>> we're
> >>>>> only sharing a single register (or perhaps even two) after all.
> >>>>> Keeping
> >>>>> everything in a single DT node would also make the binding less
> >>>>> awkward
> >>>>> because the power domain doesn't apply to the pad controller part of
> >>>>> DPAUX.
> >>>>>
> >>>>> Can't the dpaux driver simply register the pinmux controller itself?
> >>>>
> >>>> Do you think something that looks like the below?
> >>>>
> >>>> +Example (tegra124 DPAUX):
> >>>> +
> >>>> +/ {
> >>>> +       ...
> >>>> +
> >>>> +       host1x {
> >>>> +               compatible = "nvidia,tegra124-host1x", "simple-bus";
> >>>> +               ...
> >>>> +
> >>>> +               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";
> >>>> +                       pinctrl-0 = <&dpaux_state>;
> >>>> +                       pinctrl-names = "default";
> >>>> +                       status = "disabled";
> >>>> +
> >>>> +                       dpaux_padctl@0,545c0124 {
> >>>> +                               compatible =
> >>>> "nvidia,tegra124-dpaux-padctl";
> >>>> +
> >>>> +                               dpaux_state: dpaux_state0 {
> >>>> +                                       dpaux {
> >>>> +                                               nvidia,function =
> >>>> "dpaux";
> >>>> +                                       };
> >>>> +                               };
> >>>> +
> >>>> +                               i2c_state: i2c_state0 {
> >>>> +                                       i2c {
> >>>> +                                               nvidia,function =
> >>>> "i2c";
> >>>> +                                       };
> >>>> +                               };
> >>>> +                       };
> >>>
> >>> Why even have this subnode? Couldn't we simply have this:
> >>>
> >>>     host1x@... {
> >>>         ...
> >>>
> >>>         dpaux@... {
> >>>             compatible = "nvidia,tegra124-dpaux";
> >>>             ...
> >>>             pinctrl-0 = <&dpaux_aux_state>;
> >>>             pinctrl-1 = <&dpaux_i2c_state>;
> >>>             pinctrl-names = "aux", "i2c";
> >>>             ...
> >>>
> >>>             dpaux_aux_state: pinmux-aux {
> >>>                 ...
> >>>             };
> >>>
> >>>             dpaux_i2c_state: pinmux-i2c {
> >>>                 ...
> >>>             };
> >>>         };
> >>>     };
> >>>
> >>> ?
> >>>
> >>> We might need to add in indices to tell apart DPAUX and DPAUX1, though
> >>> perhaps we could refer to these states by path instead of phandle to
> >>> avoid that. Anyway, I don't see any particular reason why a subnode
> >>> would be necessary.
> >>
> >> My thinking was that we would have a pinctrl driver for dpaux in
> >> drivers/pinctrl/pinctrl-tegra-dpaux.c and therefore, I had assumed that
> >> we would need a sub-node and compatible string to probe the device.
> >>
> >> Are you sugguesting that the pinctrl driver for dpaux lives in
> >> drivers/gpu/drm/tegra/dpaux.c?
> >>
> >> Sorry if I am misunderstanding something here.
> > 
> > I think a single DT node for the single HW block makes sense. IIUC, that
> > would most correctly reflect how the HW is actually structured.
> 
> Yes that would be more aligned with the HW.
> 
> > I don't see any conceptual reason why the driver that binds to that node
> > can't register as both a pinctrl driver plus anything else it needs to.
> > For example, there are plenty of Linux drivers that register as both
> > GPIO and pinctrl drivers already. If having the same "struct device"
> > register with multiple subsystems doesn't work out (IIRC some subsystems
> > attempt to own the struct device's one driver_data field), then the
> > top-level driver can internally create whatever child devices it needs
> > to do its job. Using MFD to do that feels like overkill in this
> > situation since those child devices are unlikely to ever show up with
> > some different parent device or register offset. Either way, the choice
> > of whether to use MFD or not shouldn't affect the DT binding in any way.
> 
> Looking at it there should not be a problem here with regard to the
> driver_data member of the device struct and so I don't see why the
> tegra_dpaux_probe() could not call pinctrl_register() to register the
> device.

Yes, I think that'd be the best solution.

> However, it does mean that all the pinctrl/pinmux/pinconf ops for this
> pinctrl device will need to live in drivers/gpu/drm/tegra/dpaux.c which
> is fine, but I *believe* that would require moving
> drivers/pinctrl/pinctrl-utils.h to include/linux/pinctrl/ in order to
> make use of these functions. May be that is fine too. I could put
> together a patch series and see what everyone thinks.

I guess it depends mostly on whether Linus (Cc'ed) is willing to have
drivers outside of drivers/pinctrl implement pin controllers. If not
it'd still be possible to have the split and expose a custom API that
would allow the DPAUX driver to register the pinctrl subcomponent (much
like we do for the SMMU part of the memory controller).

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]