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.
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.
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
