Re: [PATCH V4 1/2] pinctrl: tegra: Add DT binding for io pads control

[Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On Friday 25 November 2016 02:43 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Nov 24, 2016 at 02:08:53PM +0530, Laxman Dewangan wrote:
>> +
>> +The DT property of the IO pads must be under the node of pmc i.e.
>> +pmc@7000e400 for Tegra124 onwards.
> The PMC is at a different address on Tegra186, so I think we should just
> drop this to avoid having to update it whenever a new chip relocates it
> to a different address.

I wanted to provide the example. So let me say that "i.e. pmc@7000e400 
for Tegra124". In this way we will not need to update for every chips 
and also give idea bout the node.

>
> Come to think of it, if these I/O pads are represented as subnodes in
> the PMC device tree node, perhaps this should be merged into the PMC
> documentation?

Based on my MFD and sub devices experience, maintainers prefer to have  
different DT binding document for subsystem child devices.
So if we say the io-pad is sub device of pmc then it will be in 
respective subsystem dt binding.
>
>> +
>> +See the TRM to determine which properties and values apply to each IO pads.
> Perhaps give a reference to where in the TRM this can be found?
Do we have fixed link for the TRM? and do we really need to provide the 
link here? If link get changed then we will need to change all places.


>
>> +low-power-enable:		enable low power mode.
>> +low-power-disable:		disable low power mode.
> Why the extra padding with tabs? I find that difficult to read. Also, no
> need for a fullstop since it's not a proper sentence.

Just to make proper alignment. Regular typing is not preferred in general.

>> +IO voltage pin names are as follows:
>> +	audio -> vddio-audio
>> +	audio-hv -> vddio-audio-hv
>> +	cam ->vddio-cam
>> +	dbg -> vddio-dbg
>> +	dmic -> vddio-dmic
>> +	gpio -> vddio-gpio
>> +	pex-ctrl -> vddio-pex-ctrl
>> +	sdmmc1 -> vddio-sdmmc1
>> +	sdmmc3 -> vddio-sdmmc3
>> +	spi -> vddio-spi
>> +	spi-hv -> vddio-spi-hv
>> +	uart -> vddio-uart
> It's slightly confusing to only have this list for Tegra210. I assume
> that is because on Tegra124 there is no way to control the voltage of
> the pins, but I think that could be made clearer.

I think I made it in first few paragraph of this document but will add 
here also.

>   Also, it might be
> worth explicitly mentioning that this is a subset of the list of pins
> given above and that the other pins (those not in this list) don't
> support 1.8/3.3 V control, but only the low power state.
ok.

>
> +			audio-hv {
> +				pins = "audio-hv";
> +				low-power-disable;
> +			};
> I wonder if this is at all useful. Shouldn't we rather put all pads into
> a low-power state by default and only take them out of the low-power
> state when the driver decides to do so?
>

We can not do this all disable by default and enable by driver. it may 
be possible that some of io-pads need to be enable during boot.

We need to only depends on platform specific data provided from DT here 
for configurations.

However, for dynamic control, driver can use pinctrl framework for 
optimizing the power.