Re: [PATCH 7/7] pinctrl: tegra: Add driver to configure voltage and power state of io pads

[Laxman Dewangan]

On Friday 15 April 2016 01:38 PM, Linus Walleij wrote:
> On Tue, Apr 12, 2016 at 4:56 PM, Laxman Dewangan <ldewangan@xxxxxxxxxx> wrote:
>
> > NVIDIA Tegra210 supports the IO pads which can operate at 1.8V
> > or 3.3V I/O voltage levels. Also the IO pads can be configured
> > for power down state if it is not used. SW needs to configure the
> > voltage level of IO pads based on IO rail voltage and its power
> > state based on platform usage.
> >
> > The voltage and power state configurations of pads are provided
> > through pin control frameworks. Add pin control driver for Tegra's
> > IO pads' voltage and power state configurations.
> >
> > Signed-off-by: Laxman Dewangan <ldewangan@xxxxxxxxxx>
> (...)
> > +config PINCTRL_TEGRA210_IO_PAD
> Why does this need its own Kconfig option?
> Can't you just unconditionally compile it in if
> PINCTRL_TEGRA210 is selected, you seem to say
> it is there on all these platforms anyway.

Yes, it can be done. The reason I kept is that this driver needed T210 
onwards and not for older generation of SoC.

May be we can select from T210 pincontrol.


> > +static const struct pinconf_generic_params tegra_io_pads_cfg_params[] = {
> > +       {
> > +               .property = "nvidia,io-rail-voltage",
> > +               .param = TEGRA_IO_RAIL_VOLTAGE,
> > +       }, {
> What's so nvidia-specific about this?
> We have power-source in
> Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> which takes a custom argument. This is obviously what you
> are doing (selecting one of two rails), so use that binding.

Yes, I looked for the common property but did not found anything near to 
this.
My understating for power-source is that selecting the source of supply, 
not the voltages.
I am looking something  power-source-voltage-level.
Should we add this?




> > +               .property = "nvidia,io-pad-deep-power-down",
> > +               .param = TEGRA_IO_PAD_DEEP_POWER_DOWN,
> > +       },
> Likewise the generic bindings have low-power-enable and
> low-power-disable, this seems like a copy of low-power-enable;
When writing, I considered this property but was not able to fully 
convinced myself to use this but I think now I am fine to use this as 
you suggested.



> Even if Tegra is not using the generic code for handling the
> standard bindings (GENERIC_PINCONF) it doesn't stop
> you from using the generic bindings and contributing to them.
>
> Historically you have a few custom bindings like these:
>
> nvidia,pins
> nvidia,function
> nvidia,pull
> nvidia,tristate
>
> etc etc, but that is just unfortunate and due to preceding the
> generic bindings. I would appreciate if you started to support
> the generic bindings in parallel, but I'm not gonna push that issue.

Yaah, these are in my plate to cleanup. Let me work with Stephen, what 
he think here.