[PATCH V4 0/2] pinctrl: tegra: Add support for IO pad control

[PATCH V4 0/2] pinctrl: tegra: Add support for IO pad control

[Laxman Dewangan]

NVIDIA Tegra124 and later SoCs support the multi-voltage level and 
low power state of some of its IO pads. The IO pads can work in
the voltage of the 1.8V and 3.3V of IO power rail sources. When IO
interface are not used then IO pads can be configure in low power
state to reduce the power from that IO pads.

This series add the support of configuration of IO pad via pinctrl
framework. The io pad driver uses the tegra PMC interface.

---
This driver was sent earlier for review along with soc/tegra pmc 
changes. During review, decided to first conclude in soc/tegra pmc 
patches and then review this.
    
Thierry applied the pmc patches in the private tree
        https://github.com/thierryreding/linux/tree/tegra186
and he wanted to have the patches for user of the new APIs so that
it can be pushed to mainline.
    
Sending the pinctrl driver. This needs Ack/reviewed from pinctrl subsystem
i.e. Linus Welleij to apply in the Thierry's T186 branch along with
PMC patches.

---
Changes from V1: 
- use the regulator framework to get the IO voltage instead of table from
  DT. The regulator handle is provided from DT. 

Changes from V2: 
- Nit fixes and variable/allocation optimisation as per review comment from
  V2.

Changes from V3:
 Use devm_regulator_get() instead of devm_regulator_get_optional().

Laxman Dewangan (2):
  pinctrl: tegra: Add DT binding for io pads control
  pinctrl: tegra: Add driver to configure voltage and power of io pads

 .../bindings/pinctrl/nvidia,tegra-io-pad.txt       | 126 +++++
 drivers/pinctrl/tegra/Kconfig                      |  12 +
 drivers/pinctrl/tegra/Makefile                     |   1 +
 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c       | 530 +++++++++++++++++++++
 4 files changed, 669 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
 create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laxman Dewangan]

NVIDIA Tegra124 and later SoCs support the multi-voltage level and
low power state of some of its IO pads. The IO pads can work in
the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
sources. When IO interfaces are not used then IO pads can be
configure in low power state to reduce the power consumption from
that IO pads.

On Tegra124, the voltage level of IO power rail source is auto
detected by hardware(SoC) and hence it is only require to configure
in low power mode if IO pads are not used.

On T210 onwards, the auto-detection of voltage level from IO power
rail is removed from SoC and hence SW need to configure the PMC
register explicitly to set proper voltage in IO pads based on
IO rail power source voltage.

Add DT binding document for detailing the DT properties for
configuring IO pads voltage levels and its power state.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

---
Changes from V1:
 New in series based on pinctrl driver requirement.

Changes from V2:
 Updated the statement to say 1.8V and 3.3V as nominal voltage.
 Corrected DT example by adding -supply and taken care of V1 review
 from Rob.

 .../bindings/pinctrl/nvidia,tegra-io-pad.txt       | 126 +++++++++++++++++++++
 1 file changed, 126 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
new file mode 100644
index 0000000..a88c484
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
@@ -0,0 +1,126 @@
+NVIDIA Tegra PMC IO pad controller
+
+NVIDIA Tegra124 and later SoCs support the multi-voltage level and low power
+state of some of its IO pads. When IO interface are not used then IO pads can
+be configure in low power state to reduce the power from that IO pads. The IO
+pads can work in the nominal IO voltage of 1.8V and 3.3V from power rail
+sources.
+
+On Tegra124, the voltage of IO power rail source is auto detected by SoC and
+hence it is only require to configure in low power mode if IO pads are not
+used.
+
+On T210 onwards, the HW based auto-detection for IO voltage is removed and
+hence SW need to configure the PMC register explicitly, to set proper voltage
+in IO pads, based on IO rail power source voltage.
+
+The voltage configurations and low power state of IO pads should be done in
+boot if it is not going to change otherwise dynamically based on IO rail
+voltage on that IO pads and usage of IO pads
+
+The DT property of the IO pads must be under the node of pmc i.e.
+pmc@7000e400 for Tegra124 onwards.
+
+Please refer to <pinctrl-bindings.txt> in this directory for details of the
+common pinctrl bindings used by client devices, including the meaning of the
+phrase "pin configuration node".
+
+Tegra's pin configuration nodes act as a container for an arbitrary number of
+subnodes. Each of these subnodes represents some desired configuration for an
+IO pads, or a list of IO pads. This configuration can include the voltage and
+power enable/disable control
+
+The name of each subnode is not important; all subnodes should be enumerated
+and processed purely based on their content. Each subnode only affects those
+parameters that are explicitly listed. Unspecified is represented as an absent
+property,
+
+See the TRM to determine which properties and values apply to each IO pads.
+
+Required subnode-properties:
+==========================
+- pins : An array of strings. Each string contains the name of an IO pads. Valid
+	 values for these names are listed below.
+
+Optional subnode-properties:
+==========================
+Following properties are supported from generic pin configuration explained
+in <dt-bindings/pinctrl/pinctrl-binding.txt>.
+low-power-enable:		enable low power mode.
+low-power-disable:		disable low power mode.
+
+Valid values for pin for T124 are:
+	audio, bb, cam, comp, csia, csib, csie, dsi, dsib, dsic, dsid, hdmi,
+	hsic, hv, lvds, mipi-bias, nand, pex-bias, pex-clk1, pex-clk2,
+	pex-ctrl, sdmmc1, sdmmc3, sdmmc4, sys-ddc, uart, usb0, usb1, usb2,
+	usb-bias
+
+Valid values for pin for T210 are:
+	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
+	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
+	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
+	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
+	usb1, usb2, usb3.
+
+To find out the IO rail voltage for setting the voltage of IO pad by SW,
+the regulator supply handle must provided from the DT and it is explained
+in the regulator DT binding document
+	<devicetree/bindings/regulator/regulator.txt>.
+For example, for GPIO rail the supply name is vddio-gpio and regulator
+handle is supplied from DT as
+	vddio-gpio-supply = <&regulator_xyz>;
+
+For T210, following IO pads support the 1.8V/3.3V and the corresponding
+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
+
+Example:
+	i2c@7000d000 {
+		pmic@3c {
+			regulators {
+				vddio_sdmmc1: ldo2 {
+					/* Regulator entries for LDO2 */
+				};
+
+				vdd_cam: ldo3 {
+					/* Regulator entries for LDO3 */
+				};
+			};
+		};
+	};
+
+	pmc@7000e400 {
+		vddio-cam-supply = <&vdd_cam>;
+		vddio-sdmmc1-supply = <&vddio_sdmmc1>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&tegra_io_pad_volt_default>;
+		tegra_io_pad_volt_default: common {
+			audio-hv {
+				pins = "audio-hv";
+				low-power-disable;
+			};
+
+			gpio {
+				pins = "gpio";
+				low-power-disable;
+			};
+
+			audio {
+				pins = "audio", "dmic", "sdmmc3";
+				low-power-enable;
+			};
+		};
+
+	};
-- 
2.1.4

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

[Thierry Reding]

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

On Thu, Nov 24, 2016 at 02:08:53PM +0530, Laxman Dewangan wrote:
> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> low power state of some of its IO pads. The IO pads can work in
> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
> sources. When IO interfaces are not used then IO pads can be
> configure in low power state to reduce the power consumption from
> that IO pads.
> 
> On Tegra124, the voltage level of IO power rail source is auto
> detected by hardware(SoC) and hence it is only require to configure
> in low power mode if IO pads are not used.
> 
> On T210 onwards, the auto-detection of voltage level from IO power

You say Tegra124 above but T210 here. Please use consistent spelling to
make it more obvious that we're talking about chip generations here.
Tegra<gen> is preferred over T<gen>.

> rail is removed from SoC and hence SW need to configure the PMC
> register explicitly to set proper voltage in IO pads based on
> IO rail power source voltage.
> 
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> Changes from V1:
>  New in series based on pinctrl driver requirement.
> 
> Changes from V2:
>  Updated the statement to say 1.8V and 3.3V as nominal voltage.
>  Corrected DT example by adding -supply and taken care of V1 review
>  from Rob.
> 
>  .../bindings/pinctrl/nvidia,tegra-io-pad.txt       | 126 +++++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> new file mode 100644
> index 0000000..a88c484
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> @@ -0,0 +1,126 @@
> +NVIDIA Tegra PMC IO pad controller
> +
> +NVIDIA Tegra124 and later SoCs support the multi-voltage level and low power
> +state of some of its IO pads. When IO interface are not used then IO pads can
> +be configure in low power state to reduce the power from that IO pads. The IO
> +pads can work in the nominal IO voltage of 1.8V and 3.3V from power rail
> +sources.
> +
> +On Tegra124, the voltage of IO power rail source is auto detected by SoC and
> +hence it is only require to configure in low power mode if IO pads are not
> +used.
> +
> +On T210 onwards, the HW based auto-detection for IO voltage is removed and
> +hence SW need to configure the PMC register explicitly, to set proper voltage
> +in IO pads, based on IO rail power source voltage.
> +
> +The voltage configurations and low power state of IO pads should be done in
> +boot if it is not going to change otherwise dynamically based on IO rail
> +voltage on that IO pads and usage of IO pads

Missing fullstop at the end.

> +
> +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.

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?

> +Please refer to <pinctrl-bindings.txt> in this directory for details of the
> +common pinctrl bindings used by client devices, including the meaning of the
> +phrase "pin configuration node".
> +
> +Tegra's pin configuration nodes act as a container for an arbitrary number of
> +subnodes. Each of these subnodes represents some desired configuration for an
> +IO pads, or a list of IO pads. This configuration can include the voltage and
> +power enable/disable control

Missing fullstop.

> +
> +The name of each subnode is not important; all subnodes should be enumerated
> +and processed purely based on their content. Each subnode only affects those
> +parameters that are explicitly listed. Unspecified is represented as an absent
> +property,

Should be fullstop instead of comma at the end of the sentence. Also I'm
not sure the last sentence is even necessary. The previous one already
says that only explicitly listed parameters take effect.

> +
> +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?

> +
> +Required subnode-properties:
> +==========================
> +- pins : An array of strings. Each string contains the name of an IO pads. Valid
> +	 values for these names are listed below.
> +
> +Optional subnode-properties:
> +==========================
> +Following properties are supported from generic pin configuration explained
> +in <dt-bindings/pinctrl/pinctrl-binding.txt>.

I don't think such a directory exists. Maybe make these relative
references, though that might become slightly more difficult if this
whole document is merged with the PMC documentation.

> +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.

> +Valid values for pin for T124 are:

Tegra124

> +	audio, bb, cam, comp, csia, csib, csie, dsi, dsib, dsic, dsid, hdmi,
> +	hsic, hv, lvds, mipi-bias, nand, pex-bias, pex-clk1, pex-clk2,
> +	pex-ctrl, sdmmc1, sdmmc3, sdmmc4, sys-ddc, uart, usb0, usb1, usb2,
> +	usb-bias
> +
> +Valid values for pin for T210 are:

Tegra210

> +	audio, audio-hv, cam, csia, csib, csic, csid, csie, csif,
> +	dbg, debug-nonao, dmic, dp, dsi, dsib, dsic, dsid, emmc, emmc2,
> +	gpio, hdmi, hsic, lvds, mipi-bias, pex-bias, pex-clk1, pex-clk2,
> +	pex-ctrl, sdmmc1, sdmmc3, spi, spi-hv, uart, usb-bias, usb0,
> +	usb1, usb2, usb3.
> +
> +To find out the IO rail voltage for setting the voltage of IO pad by SW,
> +the regulator supply handle must provided from the DT and it is explained
> +in the regulator DT binding document
> +	<devicetree/bindings/regulator/regulator.txt>.

Again, maybe make this relative because as it is it isn't obvious as to
what the base of the above path is.

> +For example, for GPIO rail the supply name is vddio-gpio and regulator
> +handle is supplied from DT as
> +	vddio-gpio-supply = <&regulator_xyz>;
> +
> +For T210, following IO pads support the 1.8V/3.3V and the corresponding

Tegra210

> +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. 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.

> +
> +Example:
> +	i2c@7000d000 {
> +		pmic@3c {
> +			regulators {
> +				vddio_sdmmc1: ldo2 {
> +					/* Regulator entries for LDO2 */
> +				};
> +
> +				vdd_cam: ldo3 {
> +					/* Regulator entries for LDO3 */

These comments are somewhat stating the obvious. Perhaps simply use a
... as a placeholder?

> +				};
> +			};
> +		};
> +	};
> +
> +	pmc@7000e400 {
> +		vddio-cam-supply = <&vdd_cam>;
> +		vddio-sdmmc1-supply = <&vddio_sdmmc1>;
> +
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&tegra_io_pad_volt_default>;
> +		tegra_io_pad_volt_default: common {
> +			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?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Laxman Dewangan]

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.

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

[Jon Hunter]

On 24/11/16 08:38, Laxman Dewangan wrote:
> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> low power state of some of its IO pads. The IO pads can work in
> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
> sources.

Can you fix the above sentence?

> When IO interfaces are not used then IO pads can be
> configure in low power state to reduce the power consumption from
> that IO pads.
> 
> On Tegra124, the voltage level of IO power rail source is auto
> detected by hardware(SoC) and hence it is only require to configure
> in low power mode if IO pads are not used.
> 
> On T210 onwards, the auto-detection of voltage level from IO power
> rail is removed from SoC and hence SW need to configure the PMC
> register explicitly to set proper voltage in IO pads based on
> IO rail power source voltage.
> 
> Add DT binding document for detailing the DT properties for
> configuring IO pads voltage levels and its power state.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> Changes from V1:
>  New in series based on pinctrl driver requirement.
> 
> Changes from V2:
>  Updated the statement to say 1.8V and 3.3V as nominal voltage.
>  Corrected DT example by adding -supply and taken care of V1 review
>  from Rob.
> 
>  .../bindings/pinctrl/nvidia,tegra-io-pad.txt       | 126 +++++++++++++++++++++
>  1 file changed, 126 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> new file mode 100644
> index 0000000..a88c484
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
> @@ -0,0 +1,126 @@
> +NVIDIA Tegra PMC IO pad controller
> +
> +NVIDIA Tegra124 and later SoCs support the multi-voltage level and low power
> +state of some of its IO pads. When IO interface are not used then IO pads can
> +be configure in low power state to reduce the power from that IO pads. The IO
> +pads can work in the nominal IO voltage of 1.8V and 3.3V from power rail
> +sources.

I would replace the above sentence with ...

"The IO pads can operate at the nominal IO voltage of either 1.8V or 3.3V".

Cheers
Jon

-- 
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V4 2/2] pinctrl: tegra: Add driver to configure voltage and power of io pads

[Laxman Dewangan]

NVIDIA Tegra124 and later SoCs support the multi-voltage level and
low power state of some of its IO pads. The IO pads can work in
the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
sources. When IO interfaces are not used then IO pads can be
configure in low power state to reduce the power consumption from
that IO pads.

On Tegra124, the voltage level of IO power rail source is auto
detected by hardware(SoC) and hence it is only require to configure
in low power mode if IO pads are not used.

On T210 onwards, the auto-detection of voltage level from IO power
rail is removed from SoC and hence SW need to configure the PMC
register explicitly to set proper voltage in IO pads based on
IO rail power source voltage.

This driver adds the IO pad driver to configure the power state and
IO pad voltage based on the usage and power tree via pincontrol
framework. The configuration can be static and dynamic.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

---
Changes from V1:
- Dropped the custom properties to set pad voltage and use regulator.
- Added support for regulator to get vottage in boot and configure IO
  pad voltage.
- Add support for callback to handle regulator notification and configure
  IO pad voltage based on voltage change.

Changes from V2:
 Mostly nit changes per Jon's feedback i.e. use macros for voltage, added
 comment on macros, reduce the structure and variable name size, optimise
 number of variables, and allocate memory for regulator info when it needed.

Changes from V3:
 Use the devm_regulator_get() instead of devm_regulator_get_optional().

 drivers/pinctrl/tegra/Kconfig                |  12 +
 drivers/pinctrl/tegra/Makefile               |   1 +
 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 530 +++++++++++++++++++++++++++
 3 files changed, 543 insertions(+)
 create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c

diff --git a/drivers/pinctrl/tegra/Kconfig b/drivers/pinctrl/tegra/Kconfig
index 24e20cc..6004e5c 100644
--- a/drivers/pinctrl/tegra/Kconfig
+++ b/drivers/pinctrl/tegra/Kconfig
@@ -23,6 +23,18 @@ config PINCTRL_TEGRA210
 	bool
 	select PINCTRL_TEGRA
 
+config PINCTRL_TEGRA_IO_PAD
+	bool "Tegra IO pad Control Driver"
+	depends on ARCH_TEGRA && REGULATOR
+	select PINCONF
+	select PINMUX
+	help
+	  NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
+	  level of interfacing and deep power down mode of IO pads. The
+	  voltage of IO pads are SW configurable based on IO rail of that
+	  pads on T210. This driver provides the interface to change IO pad
+	  voltage and power state via pincontrol interface.
+
 config PINCTRL_TEGRA_XUSB
 	def_bool y if ARCH_TEGRA
 	select GENERIC_PHY
diff --git a/drivers/pinctrl/tegra/Makefile b/drivers/pinctrl/tegra/Makefile
index d9ea2be..3ebaaa2 100644
--- a/drivers/pinctrl/tegra/Makefile
+++ b/drivers/pinctrl/tegra/Makefile
@@ -4,4 +4,5 @@ obj-$(CONFIG_PINCTRL_TEGRA30)		+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_TEGRA114)		+= pinctrl-tegra114.o
 obj-$(CONFIG_PINCTRL_TEGRA124)		+= pinctrl-tegra124.o
 obj-$(CONFIG_PINCTRL_TEGRA210)		+= pinctrl-tegra210.o
+obj-$(CONFIG_PINCTRL_TEGRA_IO_PAD)	+= pinctrl-tegra-io-pad.o
 obj-$(CONFIG_PINCTRL_TEGRA_XUSB)	+= pinctrl-tegra-xusb.o
diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
new file mode 100644
index 0000000..aab02d0
--- /dev/null
+++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
@@ -0,0 +1,530 @@
+/*
+ * pinctrl-tegra-io-pad: IO PAD driver for configuration of IO rail and deep
+ *			 Power Down mode via pinctrl framework.
+ *
+ * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
+ *
+ * Author: Laxman Dewangan <ldewangan@nvidia.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <soc/tegra/pmc.h>
+
+#include "../core.h"
+#include "../pinconf.h"
+#include "../pinctrl-utils.h"
+
+#define TEGRA_IO_RAIL_1800000UV 1800000
+#define TEGRA_IO_RAIL_3300000UV 3300000
+
+/* Covert IO voltage to IO pad voltage enum */
+#define tegra_io_uv_to_io_pads_uv(io_uv)				\
+		(((io_uv) == TEGRA_IO_RAIL_1800000UV) ?			\
+		  TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV)
+
+#define tegra_io_voltage_is_valid(io_uv)			\
+	({ typeof(io_uv) io_uv_ = (io_uv);			\
+	    ((io_uv_ == TEGRA_IO_RAIL_1800000UV) ||		\
+	     (io_uv_ == TEGRA_IO_RAIL_3300000UV)); })
+
+struct tegra_io_pads_cfg {
+	const char *name;
+	const unsigned int pins[1];
+	const char *vsupply;
+	enum tegra_io_pad id;
+	bool supports_low_power;
+};
+
+struct tegra_io_pads_soc_data {
+	const struct tegra_io_pads_cfg *cfg;
+	int num_cfg;
+	const struct pinctrl_pin_desc *desc;
+	int num_desc;
+};
+
+struct tegra_io_pads_info {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	const struct tegra_io_pads_soc_data *soc_data;
+};
+
+struct tegra_io_pads_regulator_info {
+	struct tegra_io_pads_info *tiopi;
+	const struct tegra_io_pads_cfg *cfg;
+	struct regulator *regulator;
+	struct notifier_block regulator_nb;
+};
+
+static int tegra_io_pads_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	return tiopi->soc_data->num_cfg;
+}
+
+static const char *tegra_io_pads_pinctrl_get_group_name(
+		struct pinctrl_dev *pctldev, unsigned int group)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	return tiopi->soc_data->cfg[group].name;
+}
+
+static int tegra_io_pads_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+						unsigned int group,
+						const unsigned int **pins,
+						unsigned int *num_pins)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	*pins = tiopi->soc_data->cfg[group].pins;
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops tegra_io_pads_pinctrl_ops = {
+	.get_groups_count	= tegra_io_pads_pinctrl_get_groups_count,
+	.get_group_name		= tegra_io_pads_pinctrl_get_group_name,
+	.get_group_pins		= tegra_io_pads_pinctrl_get_group_pins,
+	.dt_node_to_map		= pinconf_generic_dt_node_to_map_pin,
+	.dt_free_map		= pinctrl_utils_free_map,
+};
+
+static int tegra_io_pads_pinconf_get(struct pinctrl_dev *pctldev,
+				     unsigned int pin, unsigned long *config)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+	int param = pinconf_to_config_param(*config);
+	const struct tegra_io_pads_cfg *cfg = &tiopi->soc_data->cfg[pin];
+	int arg = 0;
+	int ret;
+
+	switch (param) {
+	case PIN_CONFIG_LOW_POWER_MODE:
+		if (!cfg->supports_low_power) {
+			dev_err(tiopi->dev,
+				"IO pad %s does not support low power\n",
+				cfg->name);
+			return -EINVAL;
+		}
+
+		ret = tegra_io_pad_power_get_status(cfg->id);
+		if (ret < 0)
+			return ret;
+		arg = !ret;
+		break;
+
+	default:
+		dev_err(tiopi->dev, "The parameter %d not supported\n", param);
+		return -EINVAL;
+	}
+
+	*config = pinconf_to_config_packed(param, (u16)arg);
+
+	return 0;
+}
+
+static int tegra_io_pads_pinconf_set(struct pinctrl_dev *pctldev,
+				     unsigned int pin, unsigned long *configs,
+				    unsigned int num_configs)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+	const struct tegra_io_pads_cfg *cfg = &tiopi->soc_data->cfg[pin];
+	int i;
+
+	for (i = 0; i < num_configs; i++) {
+		int ret;
+		int param = pinconf_to_config_param(configs[i]);
+		u16 param_val = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_LOW_POWER_MODE:
+			if (!cfg->supports_low_power) {
+				dev_err(tiopi->dev,
+					"IO pad %s does not support low power\n",
+					cfg->name);
+				return -EINVAL;
+			}
+			if (param_val)
+				ret = tegra_io_pad_power_disable(cfg->id);
+			else
+				ret = tegra_io_pad_power_enable(cfg->id);
+			if (ret < 0) {
+				dev_err(tiopi->dev,
+					"Failed to set DPD %d of io-pad %s: %d\n",
+					param_val, cfg->name, ret);
+				return ret;
+			}
+			break;
+
+		default:
+			dev_err(tiopi->dev, "The parameter %d not supported\n",
+				param);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+
+static const struct pinconf_ops tegra_io_pads_pinconf_ops = {
+	.pin_config_get = tegra_io_pads_pinconf_get,
+	.pin_config_set = tegra_io_pads_pinconf_set,
+};
+
+static struct pinctrl_desc tegra_io_pads_pinctrl_desc = {
+	.name = "pinctrl-tegra-io-pads",
+	.pctlops = &tegra_io_pads_pinctrl_ops,
+	.confops = &tegra_io_pads_pinconf_ops,
+};
+
+static int tegra_io_pads_rail_change_notify_cb(struct notifier_block *nb,
+					       unsigned long event, void *data)
+{
+	struct tegra_io_pads_regulator_info *rinfo;
+	struct pre_voltage_change_data *vdata;
+	unsigned long int io_volt_uv;
+	enum tegra_io_pad_voltage pad_volt;
+	int ret;
+
+	rinfo = container_of(nb, struct tegra_io_pads_regulator_info,
+			     regulator_nb);
+
+	switch (event) {
+	case REGULATOR_EVENT_PRE_VOLTAGE_CHANGE:
+		vdata = data;
+
+		if (!tegra_io_voltage_is_valid(vdata->old_uV) ||
+		    !tegra_io_voltage_is_valid(vdata->min_uV)) {
+			dev_err(rinfo->tiopi->dev,
+				"IO rail %s voltage is not 1.8/3.3V: %lu:%lu\n",
+				rinfo->cfg->name, vdata->old_uV, vdata->min_uV);
+			return -EINVAL;
+		}
+
+		/**
+		 * Change IO pad voltage before changing IO voltage when it
+		 * changes from 1.8V to 3.3V
+		 */
+		if (vdata->min_uV == TEGRA_IO_RAIL_1800000UV)
+			break;
+
+		ret = tegra_io_pad_set_voltage(rinfo->cfg->id,
+					       TEGRA_IO_PAD_3300000UV);
+		if (ret < 0) {
+			dev_err(rinfo->tiopi->dev,
+				"Failed to set voltage %lu of pad %s: %d\n",
+				vdata->min_uV, rinfo->cfg->name, ret);
+			return ret;
+		}
+		break;
+
+	case REGULATOR_EVENT_VOLTAGE_CHANGE:
+		io_volt_uv = (unsigned long)data;
+		ret = tegra_io_pad_get_voltage(rinfo->cfg->id);
+		if (ret < 0) {
+			dev_err(rinfo->tiopi->dev,
+				"Failed to get IO pad voltage: %d\n", ret);
+			return ret;
+		}
+
+		if (!tegra_io_voltage_is_valid(io_volt_uv)) {
+			dev_err(rinfo->tiopi->dev,
+				"IO rail %s voltage is not 1.8/3.3V: %lu\n",
+				rinfo->cfg->name, io_volt_uv);
+			return -EINVAL;
+		}
+
+		/*
+		 * If IO pad configuration matching with IO rail voltage then
+		 * do nothing.
+		 */
+		if (((io_volt_uv == TEGRA_IO_RAIL_1800000UV) &&
+		     (ret == TEGRA_IO_PAD_1800000UV)) ||
+		     ((io_volt_uv == TEGRA_IO_RAIL_3300000UV) &&
+		      (ret == TEGRA_IO_PAD_3300000UV)))
+			break;
+
+		ret = tegra_io_pad_set_voltage(rinfo->cfg->id,
+					       TEGRA_IO_PAD_1800000UV);
+		if (ret < 0) {
+			dev_err(rinfo->tiopi->dev,
+				"Failed to set voltage %lu of pad %s: %d\n",
+				vdata->min_uV, rinfo->cfg->name, ret);
+			return ret;
+		}
+		break;
+
+	case REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE:
+		io_volt_uv = (unsigned long)data;
+
+		if (!tegra_io_voltage_is_valid(io_volt_uv)) {
+			dev_err(rinfo->tiopi->dev,
+				"IO rail %s voltage is not 1.8/3.3V: %lu\n",
+				rinfo->cfg->name, io_volt_uv);
+			return -EINVAL;
+		}
+
+		pad_volt = tegra_io_uv_to_io_pads_uv(io_volt_uv);
+		ret = tegra_io_pad_set_voltage(rinfo->cfg->id, pad_volt);
+		if (ret < 0) {
+			dev_err(rinfo->tiopi->dev,
+				"Failed to set voltage %lu of pad %s: %d\n",
+				io_volt_uv, rinfo->cfg->name, ret);
+			return ret;
+		}
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int tegra_io_pads_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct platform_device_id *id = platform_get_device_id(pdev);
+	const struct tegra_io_pads_soc_data *soc_data =
+			(const struct tegra_io_pads_soc_data *)id->driver_data;
+	struct tegra_io_pads_info *tiopi;
+	int ret, i;
+
+	if (!pdev->dev.parent->of_node) {
+		dev_err(dev, "PMC should be register from DT\n");
+		return -ENODEV;
+	}
+
+	tiopi = devm_kzalloc(dev, sizeof(*tiopi), GFP_KERNEL);
+	if (!tiopi)
+		return -ENOMEM;
+
+	tiopi->dev = &pdev->dev;
+	pdev->dev.of_node = pdev->dev.parent->of_node;
+	tiopi->soc_data = soc_data;
+
+	for (i = 0; i < soc_data->num_cfg; ++i) {
+		struct tegra_io_pads_regulator_info *rinfo;
+		enum tegra_io_pad_voltage pad_volt;
+		int io_volt_uv;
+
+		if (!soc_data->cfg[i].vsupply)
+			continue;
+
+		rinfo = devm_kzalloc(dev, sizeof(*rinfo), GFP_KERNEL);
+		if (!rinfo)
+			return -ENOMEM;
+
+		rinfo->tiopi = tiopi;
+		rinfo->cfg = &soc_data->cfg[i];
+
+		rinfo->regulator = devm_regulator_get(dev,
+						      soc_data->cfg[i].vsupply);
+		if (IS_ERR(rinfo->regulator)) {
+			ret = PTR_ERR(rinfo->regulator);
+			if (ret == -EPROBE_DEFER)
+				return ret;
+			continue;
+		}
+
+		io_volt_uv = regulator_get_voltage(rinfo->regulator);
+		if (io_volt_uv < 0) {
+			dev_warn(dev, "Failed to get voltage for rail %s: %d\n",
+				 soc_data->cfg[i].vsupply, io_volt_uv);
+			continue;
+		}
+
+		if (!tegra_io_voltage_is_valid(io_volt_uv)) {
+			dev_warn(dev, "IO rail %s voltage is not 1.8/3.3V: %d\n",
+				 soc_data->cfg[i].vsupply, io_volt_uv);
+			continue;
+		}
+
+		pad_volt = tegra_io_uv_to_io_pads_uv(io_volt_uv);
+		ret = tegra_io_pad_set_voltage(soc_data->cfg[i].id, pad_volt);
+		if (ret < 0) {
+			dev_err(dev, "Failed to set voltage %d of pad %s: %d\n",
+				io_volt_uv, soc_data->cfg[i].name, ret);
+			return ret;
+		}
+
+		rinfo->regulator_nb.notifier_call =
+					tegra_io_pads_rail_change_notify_cb;
+		ret = devm_regulator_register_notifier(rinfo->regulator,
+						       &rinfo->regulator_nb);
+		if (ret < 0) {
+			dev_err(dev, "Failed to register regulator %s notifier: %d\n",
+				soc_data->cfg[i].name, ret);
+			return ret;
+		}
+	}
+
+	tegra_io_pads_pinctrl_desc.pins = tiopi->soc_data->desc;
+	tegra_io_pads_pinctrl_desc.npins = tiopi->soc_data->num_desc;
+	platform_set_drvdata(pdev, tiopi);
+
+	tiopi->pctl = devm_pinctrl_register(dev, &tegra_io_pads_pinctrl_desc,
+					    tiopi);
+	if (IS_ERR(tiopi->pctl)) {
+		ret = PTR_ERR(tiopi->pctl);
+		dev_err(dev, "Failed to register io-pad pinctrl driver: %d\n",
+			ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+#define TEGRA124_PAD_INFO_TABLE(_entry_)			\
+	_entry_(0, "audio", AUDIO, true, NULL),			\
+	_entry_(1, "bb", BB, true, NULL),			\
+	_entry_(2, "cam", CAM, true, NULL),			\
+	_entry_(3, "comp", COMP, true, NULL),			\
+	_entry_(4, "csia", CSIA, true, NULL),			\
+	_entry_(5, "csib", CSIB, true, NULL),			\
+	_entry_(6, "csie", CSIE, true, NULL),			\
+	_entry_(7, "dsi", DSI, true, NULL),			\
+	_entry_(8, "dsib", DSIB, true, NULL),			\
+	_entry_(9, "dsic", DSIC, true, NULL),			\
+	_entry_(10, "dsid", DSID, true, NULL),			\
+	_entry_(11, "hdmi", HDMI, true, NULL),			\
+	_entry_(12, "hsic", HSIC, true, NULL),			\
+	_entry_(13, "hv", HV, true, NULL),			\
+	_entry_(14, "lvds", LVDS, true, NULL),			\
+	_entry_(15, "mipi-bias", MIPI_BIAS, true, NULL),	\
+	_entry_(16, "nand", NAND, true, NULL),			\
+	_entry_(17, "pex-bias", PEX_BIAS, true, NULL),		\
+	_entry_(18, "pex-clk1", PEX_CLK1, true, NULL),		\
+	_entry_(19, "pex-clk2", PEX_CLK2, true, NULL),		\
+	_entry_(20, "pex-ctrl", PEX_CNTRL, true, NULL),		\
+	_entry_(21, "sdmmc1", SDMMC1, true, NULL),		\
+	_entry_(22, "sdmmc3", SDMMC3, true, NULL),		\
+	_entry_(23, "sdmmc4", SDMMC4, true, NULL),		\
+	_entry_(24, "sys-ddc", SYS_DDC, true, NULL),		\
+	_entry_(25, "uart", UART, true, NULL),			\
+	_entry_(26, "usb0", USB0, true, NULL),			\
+	_entry_(27, "usb1", USB1, true, NULL),			\
+	_entry_(28, "usb2", USB2, true, NULL),			\
+	_entry_(29, "usb-bias", USB_BIAS, true, NULL)
+
+#define TEGRA210_PAD_INFO_TABLE(_entry_)			\
+	_entry_(0, "audio", AUDIO, true, "vddio-audio"),	\
+	_entry_(1, "audio-hv", AUDIO_HV, true, "vddio-audio-hv"), \
+	_entry_(2, "cam", CAM, true, "vddio-cam"),		\
+	_entry_(3, "csia", CSIA, true, NULL),			\
+	_entry_(4, "csib", CSIB, true, NULL),			\
+	_entry_(5, "csic", CSIC, true, NULL),			\
+	_entry_(6, "csid", CSID, true, NULL),			\
+	_entry_(7, "csie", CSIE, true, NULL),			\
+	_entry_(8, "csif", CSIF, true, NULL),			\
+	_entry_(9, "dbg", DBG, true, "vddio-dbg"),		\
+	_entry_(10, "debug-nonao", DEBUG_NONAO, true, NULL),	\
+	_entry_(11, "dmic", DMIC, true, "vddio-dmic"),		\
+	_entry_(12, "dp", DP, true, NULL),			\
+	_entry_(13, "dsi", DSI, true, NULL),			\
+	_entry_(14, "dsib", DSIB, true, NULL),			\
+	_entry_(15, "dsic", DSIC, true, NULL),			\
+	_entry_(16, "dsid", DSID, true, NULL),			\
+	_entry_(17, "emmc", SDMMC4, true, NULL),		\
+	_entry_(18, "emmc2", EMMC2, true, NULL),		\
+	_entry_(19, "gpio", GPIO, true, "vddio-gpio"),		\
+	_entry_(20, "hdmi", HDMI, true, NULL),			\
+	_entry_(21, "hsic", HSIC, true, NULL),			\
+	_entry_(22, "lvds", LVDS, true, NULL),			\
+	_entry_(23, "mipi-bias", MIPI_BIAS, true, NULL),	\
+	_entry_(24, "pex-bias", PEX_BIAS, true, NULL),		\
+	_entry_(25, "pex-clk1", PEX_CLK1, true, NULL),		\
+	_entry_(26, "pex-clk2", PEX_CLK2, true, NULL),		\
+	_entry_(27, "pex-ctrl", PEX_CNTRL, false, "vddio-pex-ctrl"), \
+	_entry_(28, "sdmmc1", SDMMC1, true, "vddio-sdmmc1"),	\
+	_entry_(29, "sdmmc3", SDMMC3, true, "vddio-sdmmc3"),	\
+	_entry_(30, "spi", SPI, true, "vddio-spi"),		\
+	_entry_(31, "spi-hv", SPI_HV, true, "vddio-spi-hv"),	\
+	_entry_(32, "uart", UART, true, "vddio-uart"),		\
+	_entry_(33, "usb0", USB0, true, NULL),			\
+	_entry_(34, "usb1", USB1, true, NULL),			\
+	_entry_(35, "usb2", USB2, true, NULL),			\
+	_entry_(36, "usb3", USB3, true, NULL),			\
+	_entry_(37, "usb-bias", USB_BIAS, true, NULL)
+
+#define TEGRA_IO_PAD_INFO(_pin, _name, _id, _lpstate, _vsupply)	\
+	{							\
+		.name = _name,					\
+		.pins = {(_pin)},				\
+		.id = TEGRA_IO_PAD_##_id,			\
+		.vsupply = (_vsupply),				\
+		.supports_low_power = (_lpstate),		\
+	}
+
+static const struct tegra_io_pads_cfg tegra124_io_pads_cfg_info[] = {
+	TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
+
+static const struct tegra_io_pads_cfg tegra210_io_pads_cfg_info[] = {
+	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
+
+#define TEGRA_IO_PAD_DESC(_pin, _name, _id, _lpstate, _vsupply)	\
+	PINCTRL_PIN(_pin, _name)
+
+static const struct pinctrl_pin_desc tegra124_io_pads_pinctrl_desc[] = {
+	TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
+};
+
+static const struct pinctrl_pin_desc tegra210_io_pads_pinctrl_desc[] = {
+	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
+};
+
+static const struct tegra_io_pads_soc_data tegra124_io_pad_soc_data = {
+	.desc		= tegra124_io_pads_pinctrl_desc,
+	.num_desc	= ARRAY_SIZE(tegra124_io_pads_pinctrl_desc),
+	.cfg		= tegra124_io_pads_cfg_info,
+	.num_cfg	= ARRAY_SIZE(tegra124_io_pads_cfg_info),
+};
+
+static const struct tegra_io_pads_soc_data tegra210_io_pad_soc_data = {
+	.desc		= tegra210_io_pads_pinctrl_desc,
+	.num_desc	= ARRAY_SIZE(tegra210_io_pads_pinctrl_desc),
+	.cfg		= tegra210_io_pads_cfg_info,
+	.num_cfg	= ARRAY_SIZE(tegra210_io_pads_cfg_info),
+};
+
+static const struct platform_device_id tegra_io_pads_dev_id[] = {
+	{
+		.name = "pinctrl-t124-io-pad",
+		.driver_data = (kernel_ulong_t)&tegra124_io_pad_soc_data,
+	}, {
+		.name = "pinctrl-t210-io-pad",
+		.driver_data = (kernel_ulong_t)&tegra210_io_pad_soc_data,
+	}, {
+	},
+};
+MODULE_DEVICE_TABLE(platform, tegra_io_pads_dev_id);
+
+static struct platform_driver tegra_io_pads_pinctrl_driver = {
+	.driver		= {
+		.name	= "pinctrl-tegra-io-pad",
+	},
+	.probe		= tegra_io_pads_pinctrl_probe,
+	.id_table	= tegra_io_pads_dev_id,
+};
+
+module_platform_driver(tegra_io_pads_pinctrl_driver);
+
+MODULE_DESCRIPTION("NVIDIA TEGRA IO pad Control Driver");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

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

[Thierry Reding]

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

On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
> low power state of some of its IO pads. The IO pads can work in
> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
> sources. When IO interfaces are not used then IO pads can be
> configure in low power state to reduce the power consumption from
> that IO pads.
> 
> On Tegra124, the voltage level of IO power rail source is auto
> detected by hardware(SoC) and hence it is only require to configure
> in low power mode if IO pads are not used.
> 
> On T210 onwards, the auto-detection of voltage level from IO power
> rail is removed from SoC and hence SW need to configure the PMC
> register explicitly to set proper voltage in IO pads based on
> IO rail power source voltage.
> 
> This driver adds the IO pad driver to configure the power state and
> IO pad voltage based on the usage and power tree via pincontrol
> framework. The configuration can be static and dynamic.
> 
> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> ---
> Changes from V1:
> - Dropped the custom properties to set pad voltage and use regulator.
> - Added support for regulator to get vottage in boot and configure IO
>   pad voltage.
> - Add support for callback to handle regulator notification and configure
>   IO pad voltage based on voltage change.
> 
> Changes from V2:
>  Mostly nit changes per Jon's feedback i.e. use macros for voltage, added
>  comment on macros, reduce the structure and variable name size, optimise
>  number of variables, and allocate memory for regulator info when it needed.
> 
> Changes from V3:
>  Use the devm_regulator_get() instead of devm_regulator_get_optional().
> 
>  drivers/pinctrl/tegra/Kconfig                |  12 +
>  drivers/pinctrl/tegra/Makefile               |   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 530 +++++++++++++++++++++++++++
>  3 files changed, 543 insertions(+)
>  create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
> 
> diff --git a/drivers/pinctrl/tegra/Kconfig b/drivers/pinctrl/tegra/Kconfig
> index 24e20cc..6004e5c 100644
> --- a/drivers/pinctrl/tegra/Kconfig
> +++ b/drivers/pinctrl/tegra/Kconfig
> @@ -23,6 +23,18 @@ config PINCTRL_TEGRA210
>  	bool
>  	select PINCTRL_TEGRA
>  
> +config PINCTRL_TEGRA_IO_PAD
> +	bool "Tegra IO pad Control Driver"
> +	depends on ARCH_TEGRA && REGULATOR
> +	select PINCONF
> +	select PINMUX
> +	help
> +	  NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
> +	  level of interfacing and deep power down mode of IO pads. The
> +	  voltage of IO pads are SW configurable based on IO rail of that
> +	  pads on T210. This driver provides the interface to change IO pad
> +	  voltage and power state via pincontrol interface.

This has a lot of chip-specific text. Will all of that have to be
updated if support for new chips is added?

> +
>  config PINCTRL_TEGRA_XUSB
>  	def_bool y if ARCH_TEGRA
>  	select GENERIC_PHY
> diff --git a/drivers/pinctrl/tegra/Makefile b/drivers/pinctrl/tegra/Makefile
> index d9ea2be..3ebaaa2 100644
> --- a/drivers/pinctrl/tegra/Makefile
> +++ b/drivers/pinctrl/tegra/Makefile
> @@ -4,4 +4,5 @@ obj-$(CONFIG_PINCTRL_TEGRA30)		+= pinctrl-tegra30.o
>  obj-$(CONFIG_PINCTRL_TEGRA114)		+= pinctrl-tegra114.o
>  obj-$(CONFIG_PINCTRL_TEGRA124)		+= pinctrl-tegra124.o
>  obj-$(CONFIG_PINCTRL_TEGRA210)		+= pinctrl-tegra210.o
> +obj-$(CONFIG_PINCTRL_TEGRA_IO_PAD)	+= pinctrl-tegra-io-pad.o
>  obj-$(CONFIG_PINCTRL_TEGRA_XUSB)	+= pinctrl-tegra-xusb.o
> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
> new file mode 100644
> index 0000000..aab02d0
> --- /dev/null
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
> @@ -0,0 +1,530 @@
> +/*
> + * pinctrl-tegra-io-pad: IO PAD driver for configuration of IO rail and deep
> + *			 Power Down mode via pinctrl framework.
> + *
> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
> + *
> + * Author: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pinctrl/pinctrl.h>
> +#include <linux/pinctrl/pinconf-generic.h>
> +#include <linux/pinctrl/pinconf.h>
> +#include <linux/pinctrl/pinmux.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +#include <soc/tegra/pmc.h>

Have you considered moving this code into the PMC driver? It seems a
little over the top to go through all of the platform device creation
and driver registration dance only to call into a public API later on.

> +#include "../core.h"
> +#include "../pinconf.h"
> +#include "../pinctrl-utils.h"
> +
> +#define TEGRA_IO_RAIL_1800000UV 1800000
> +#define TEGRA_IO_RAIL_3300000UV 3300000

Is there really a point in having these defines? They are really long
and effectively don't carry more information than just the plain
numbers.

> +
> +/* Covert IO voltage to IO pad voltage enum */

Convert

> +#define tegra_io_uv_to_io_pads_uv(io_uv)				\
> +		(((io_uv) == TEGRA_IO_RAIL_1800000UV) ?			\
> +		  TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV)
> +
> +#define tegra_io_voltage_is_valid(io_uv)			\
> +	({ typeof(io_uv) io_uv_ = (io_uv);			\
> +	    ((io_uv_ == TEGRA_IO_RAIL_1800000UV) ||		\
> +	     (io_uv_ == TEGRA_IO_RAIL_3300000UV)); })
>
Maybe make both of these static inline functions to improve readability?
I find the above very hard to read.

> +struct tegra_io_pads_cfg {
> +	const char *name;
> +	const unsigned int pins[1];
> +	const char *vsupply;
> +	enum tegra_io_pad id;
> +	bool supports_low_power;
> +};

Pretty much everything in this driver operates on single pads, so it's a
little confusing to use the "pads" in the names. I think it would be
more appropriate to name this structure tegra_io_pad_cfg because it is
configuration data associated with a single pad.

> +
> +struct tegra_io_pads_soc_data {

I think the _data suffix is redundant and can be dropped.

The use of "pads" in this structure name is fine because it really does
contain data for multiple pads.

> +	const struct tegra_io_pads_cfg *cfg;
> +	int num_cfg;

This can be unsigned int. Also I think it's more common to use the
plural in these (cfgs, num_cfgs).

> +	const struct pinctrl_pin_desc *desc;
> +	int num_desc;

Same here.

> +};
> +
> +struct tegra_io_pads_info {
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +	const struct tegra_io_pads_soc_data *soc_data;

If you drop the _data suffix from the type I think you can also drop it
from the field name.

"pads" is fine here as well because, again, this deals with multiple
pads.

> +};
> +
> +struct tegra_io_pads_regulator_info {
> +	struct tegra_io_pads_info *tiopi;
> +	const struct tegra_io_pads_cfg *cfg;
> +	struct regulator *regulator;
> +	struct notifier_block regulator_nb;

The regulator_ prefix is redundent here. It's contained within a
structure named tegra_io_pads_regulator_info, so it's fairly obvious
that this is related to regulators.

This deals only with a single pad, so tegra_io_pad_regulator_info would
be less confusing, I think. Perhaps even drop the _info suffix while at
it because it doesn't add anything useful.

> +};
> +
> +static int tegra_io_pads_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return tiopi->soc_data->num_cfg;
> +}
> +
> +static const char *tegra_io_pads_pinctrl_get_group_name(
> +		struct pinctrl_dev *pctldev, unsigned int group)
> +{
> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return tiopi->soc_data->cfg[group].name;
> +}
> +
> +static int tegra_io_pads_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
> +						unsigned int group,
> +						const unsigned int **pins,
> +						unsigned int *num_pins)
> +{
> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> +	*pins = tiopi->soc_data->cfg[group].pins;
> +	*num_pins = 1;
> +
> +	return 0;
> +}
> +
> +static const struct pinctrl_ops tegra_io_pads_pinctrl_ops = {
> +	.get_groups_count	= tegra_io_pads_pinctrl_get_groups_count,
> +	.get_group_name		= tegra_io_pads_pinctrl_get_group_name,
> +	.get_group_pins		= tegra_io_pads_pinctrl_get_group_pins,
> +	.dt_node_to_map		= pinconf_generic_dt_node_to_map_pin,
> +	.dt_free_map		= pinctrl_utils_free_map,
> +};

Nit: I don't think this padding using tabs increases readability.

> +
> +static int tegra_io_pads_pinconf_get(struct pinctrl_dev *pctldev,
> +				     unsigned int pin, unsigned long *config)
> +{
> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +	int param = pinconf_to_config_param(*config);
> +	const struct tegra_io_pads_cfg *cfg = &tiopi->soc_data->cfg[pin];
> +	int arg = 0;

Why not make that a u16...

> +	int ret;
> +
> +	switch (param) {
> +	case PIN_CONFIG_LOW_POWER_MODE:
> +		if (!cfg->supports_low_power) {
> +			dev_err(tiopi->dev,
> +				"IO pad %s does not support low power\n",
> +				cfg->name);
> +			return -EINVAL;
> +		}
> +
> +		ret = tegra_io_pad_power_get_status(cfg->id);
> +		if (ret < 0)
> +			return ret;
> +		arg = !ret;
> +		break;
> +
> +	default:
> +		dev_err(tiopi->dev, "The parameter %d not supported\n", param);
> +		return -EINVAL;
> +	}
> +
> +	*config = pinconf_to_config_packed(param, (u16)arg);

... and avoid the cast here?

> +
> +	return 0;
> +}
> +
> +static int tegra_io_pads_pinconf_set(struct pinctrl_dev *pctldev,
> +				     unsigned int pin, unsigned long *configs,
> +				    unsigned int num_configs)

This last line is not quite properly aligned.

> +{
> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +	const struct tegra_io_pads_cfg *cfg = &tiopi->soc_data->cfg[pin];
> +	int i;

This should be unsigned.

> +
> +	for (i = 0; i < num_configs; i++) {
> +		int ret;
> +		int param = pinconf_to_config_param(configs[i]);

The function returns an enum pin_config_param, why not use that as the
type?

> +		u16 param_val = pinconf_to_config_argument(configs[i]);
> +
> +		switch (param) {
> +		case PIN_CONFIG_LOW_POWER_MODE:
> +			if (!cfg->supports_low_power) {
> +				dev_err(tiopi->dev,
> +					"IO pad %s does not support low power\n",
> +					cfg->name);
> +				return -EINVAL;
> +			}
> +			if (param_val)
> +				ret = tegra_io_pad_power_disable(cfg->id);
> +			else
> +				ret = tegra_io_pad_power_enable(cfg->id);
> +			if (ret < 0) {
> +				dev_err(tiopi->dev,
> +					"Failed to set DPD %d of io-pad %s: %d\n",
> +					param_val, cfg->name, ret);
> +				return ret;
> +			}
> +			break;
> +
> +		default:
> +			dev_err(tiopi->dev, "The parameter %d not supported\n",
> +				param);
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct pinconf_ops tegra_io_pads_pinconf_ops = {
> +	.pin_config_get = tegra_io_pads_pinconf_get,
> +	.pin_config_set = tegra_io_pads_pinconf_set,
> +};
> +
> +static struct pinctrl_desc tegra_io_pads_pinctrl_desc = {
> +	.name = "pinctrl-tegra-io-pads",
> +	.pctlops = &tegra_io_pads_pinctrl_ops,
> +	.confops = &tegra_io_pads_pinconf_ops,
> +};
> +
> +static int tegra_io_pads_rail_change_notify_cb(struct notifier_block *nb,
> +					       unsigned long event, void *data)
> +{
> +	struct tegra_io_pads_regulator_info *rinfo;
> +	struct pre_voltage_change_data *vdata;
> +	unsigned long int io_volt_uv;

You can drop the int, it's implied by long.

> +	enum tegra_io_pad_voltage pad_volt;
> +	int ret;
> +
> +	rinfo = container_of(nb, struct tegra_io_pads_regulator_info,
> +			     regulator_nb);
> +
> +	switch (event) {
> +	case REGULATOR_EVENT_PRE_VOLTAGE_CHANGE:
> +		vdata = data;
> +
> +		if (!tegra_io_voltage_is_valid(vdata->old_uV) ||
> +		    !tegra_io_voltage_is_valid(vdata->min_uV)) {
> +			dev_err(rinfo->tiopi->dev,
> +				"IO rail %s voltage is not 1.8/3.3V: %lu:%lu\n",
> +				rinfo->cfg->name, vdata->old_uV, vdata->min_uV);
> +			return -EINVAL;
> +		}
> +
> +		/**
> +		 * Change IO pad voltage before changing IO voltage when it
> +		 * changes from 1.8V to 3.3V
> +		 */
> +		if (vdata->min_uV == TEGRA_IO_RAIL_1800000UV)
> +			break;
> +
> +		ret = tegra_io_pad_set_voltage(rinfo->cfg->id,
> +					       TEGRA_IO_PAD_3300000UV);
> +		if (ret < 0) {
> +			dev_err(rinfo->tiopi->dev,
> +				"Failed to set voltage %lu of pad %s: %d\n",
> +				vdata->min_uV, rinfo->cfg->name, ret);
> +			return ret;
> +		}
> +		break;
> +
> +	case REGULATOR_EVENT_VOLTAGE_CHANGE:
> +		io_volt_uv = (unsigned long)data;
> +		ret = tegra_io_pad_get_voltage(rinfo->cfg->id);
> +		if (ret < 0) {
> +			dev_err(rinfo->tiopi->dev,
> +				"Failed to get IO pad voltage: %d\n", ret);
> +			return ret;
> +		}

Might be worth reassigning ret to a variable of type enum
tegra_io_pad_voltage because...

> +
> +		if (!tegra_io_voltage_is_valid(io_volt_uv)) {
> +			dev_err(rinfo->tiopi->dev,
> +				"IO rail %s voltage is not 1.8/3.3V: %lu\n",
> +				rinfo->cfg->name, io_volt_uv);
> +			return -EINVAL;
> +		}
> +
> +		/*
> +		 * If IO pad configuration matching with IO rail voltage then
> +		 * do nothing.
> +		 */
> +		if (((io_volt_uv == TEGRA_IO_RAIL_1800000UV) &&
> +		     (ret == TEGRA_IO_PAD_1800000UV)) ||
> +		     ((io_volt_uv == TEGRA_IO_RAIL_3300000UV) &&
> +		      (ret == TEGRA_IO_PAD_3300000UV)))
> +			break;

... if somebody ever inserted code between the above and this, they
might be overwriting ret.

> +
> +		ret = tegra_io_pad_set_voltage(rinfo->cfg->id,
> +					       TEGRA_IO_PAD_1800000UV);
> +		if (ret < 0) {
> +			dev_err(rinfo->tiopi->dev,
> +				"Failed to set voltage %lu of pad %s: %d\n",
> +				vdata->min_uV, rinfo->cfg->name, ret);

You might want to add the units of the voltage to avoid confusion. Same
in a couple more messages above and below.

> +			return ret;
> +		}
> +		break;
> +
> +	case REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE:
> +		io_volt_uv = (unsigned long)data;
> +
> +		if (!tegra_io_voltage_is_valid(io_volt_uv)) {
> +			dev_err(rinfo->tiopi->dev,
> +				"IO rail %s voltage is not 1.8/3.3V: %lu\n",
> +				rinfo->cfg->name, io_volt_uv);
> +			return -EINVAL;
> +		}
> +
> +		pad_volt = tegra_io_uv_to_io_pads_uv(io_volt_uv);
> +		ret = tegra_io_pad_set_voltage(rinfo->cfg->id, pad_volt);
> +		if (ret < 0) {
> +			dev_err(rinfo->tiopi->dev,
> +				"Failed to set voltage %lu of pad %s: %d\n",
> +				io_volt_uv, rinfo->cfg->name, ret);
> +			return ret;
> +		}
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int tegra_io_pads_pinctrl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	const struct tegra_io_pads_soc_data *soc_data =
> +			(const struct tegra_io_pads_soc_data *)id->driver_data;
> +	struct tegra_io_pads_info *tiopi;
> +	int ret, i;
> +
> +	if (!pdev->dev.parent->of_node) {
> +		dev_err(dev, "PMC should be register from DT\n");
> +		return -ENODEV;
> +	}
> +
> +	tiopi = devm_kzalloc(dev, sizeof(*tiopi), GFP_KERNEL);
> +	if (!tiopi)
> +		return -ENOMEM;
> +
> +	tiopi->dev = &pdev->dev;
> +	pdev->dev.of_node = pdev->dev.parent->of_node;
> +	tiopi->soc_data = soc_data;
> +
> +	for (i = 0; i < soc_data->num_cfg; ++i) {
> +		struct tegra_io_pads_regulator_info *rinfo;
> +		enum tegra_io_pad_voltage pad_volt;
> +		int io_volt_uv;
> +
> +		if (!soc_data->cfg[i].vsupply)
> +			continue;
> +
> +		rinfo = devm_kzalloc(dev, sizeof(*rinfo), GFP_KERNEL);
> +		if (!rinfo)
> +			return -ENOMEM;
> +
> +		rinfo->tiopi = tiopi;
> +		rinfo->cfg = &soc_data->cfg[i];
> +
> +		rinfo->regulator = devm_regulator_get(dev,
> +						      soc_data->cfg[i].vsupply);
> +		if (IS_ERR(rinfo->regulator)) {
> +			ret = PTR_ERR(rinfo->regulator);
> +			if (ret == -EPROBE_DEFER)
> +				return ret;
> +			continue;
> +		}
> +
> +		io_volt_uv = regulator_get_voltage(rinfo->regulator);
> +		if (io_volt_uv < 0) {
> +			dev_warn(dev, "Failed to get voltage for rail %s: %d\n",
> +				 soc_data->cfg[i].vsupply, io_volt_uv);
> +			continue;
> +		}
> +
> +		if (!tegra_io_voltage_is_valid(io_volt_uv)) {
> +			dev_warn(dev, "IO rail %s voltage is not 1.8/3.3V: %d\n",
> +				 soc_data->cfg[i].vsupply, io_volt_uv);
> +			continue;
> +		}
> +
> +		pad_volt = tegra_io_uv_to_io_pads_uv(io_volt_uv);
> +		ret = tegra_io_pad_set_voltage(soc_data->cfg[i].id, pad_volt);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set voltage %d of pad %s: %d\n",
> +				io_volt_uv, soc_data->cfg[i].name, ret);
> +			return ret;
> +		}
> +
> +		rinfo->regulator_nb.notifier_call =
> +					tegra_io_pads_rail_change_notify_cb;
> +		ret = devm_regulator_register_notifier(rinfo->regulator,
> +						       &rinfo->regulator_nb);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to register regulator %s notifier: %d\n",
> +				soc_data->cfg[i].name, ret);
> +			return ret;
> +		}
> +	}
> +
> +	tegra_io_pads_pinctrl_desc.pins = tiopi->soc_data->desc;
> +	tegra_io_pads_pinctrl_desc.npins = tiopi->soc_data->num_desc;

This modifies global data. Can we avoid that? I think the easiest would
be to make tegra_io_pads_pinctrl_desc a field of the tegra_io_pads_info
struct.

> +	platform_set_drvdata(pdev, tiopi);
> +
> +	tiopi->pctl = devm_pinctrl_register(dev, &tegra_io_pads_pinctrl_desc,
> +					    tiopi);
> +	if (IS_ERR(tiopi->pctl)) {
> +		ret = PTR_ERR(tiopi->pctl);
> +		dev_err(dev, "Failed to register io-pad pinctrl driver: %d\n",
> +			ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +#define TEGRA124_PAD_INFO_TABLE(_entry_)			\
> +	_entry_(0, "audio", AUDIO, true, NULL),			\
> +	_entry_(1, "bb", BB, true, NULL),			\
> +	_entry_(2, "cam", CAM, true, NULL),			\
> +	_entry_(3, "comp", COMP, true, NULL),			\
> +	_entry_(4, "csia", CSIA, true, NULL),			\
> +	_entry_(5, "csib", CSIB, true, NULL),			\
> +	_entry_(6, "csie", CSIE, true, NULL),			\
> +	_entry_(7, "dsi", DSI, true, NULL),			\
> +	_entry_(8, "dsib", DSIB, true, NULL),			\
> +	_entry_(9, "dsic", DSIC, true, NULL),			\
> +	_entry_(10, "dsid", DSID, true, NULL),			\
> +	_entry_(11, "hdmi", HDMI, true, NULL),			\
> +	_entry_(12, "hsic", HSIC, true, NULL),			\
> +	_entry_(13, "hv", HV, true, NULL),			\
> +	_entry_(14, "lvds", LVDS, true, NULL),			\
> +	_entry_(15, "mipi-bias", MIPI_BIAS, true, NULL),	\
> +	_entry_(16, "nand", NAND, true, NULL),			\
> +	_entry_(17, "pex-bias", PEX_BIAS, true, NULL),		\
> +	_entry_(18, "pex-clk1", PEX_CLK1, true, NULL),		\
> +	_entry_(19, "pex-clk2", PEX_CLK2, true, NULL),		\
> +	_entry_(20, "pex-ctrl", PEX_CNTRL, true, NULL),		\
> +	_entry_(21, "sdmmc1", SDMMC1, true, NULL),		\
> +	_entry_(22, "sdmmc3", SDMMC3, true, NULL),		\
> +	_entry_(23, "sdmmc4", SDMMC4, true, NULL),		\
> +	_entry_(24, "sys-ddc", SYS_DDC, true, NULL),		\
> +	_entry_(25, "uart", UART, true, NULL),			\
> +	_entry_(26, "usb0", USB0, true, NULL),			\
> +	_entry_(27, "usb1", USB1, true, NULL),			\
> +	_entry_(28, "usb2", USB2, true, NULL),			\
> +	_entry_(29, "usb-bias", USB_BIAS, true, NULL)
> +
> +#define TEGRA210_PAD_INFO_TABLE(_entry_)			\
> +	_entry_(0, "audio", AUDIO, true, "vddio-audio"),	\
> +	_entry_(1, "audio-hv", AUDIO_HV, true, "vddio-audio-hv"), \
> +	_entry_(2, "cam", CAM, true, "vddio-cam"),		\
> +	_entry_(3, "csia", CSIA, true, NULL),			\
> +	_entry_(4, "csib", CSIB, true, NULL),			\
> +	_entry_(5, "csic", CSIC, true, NULL),			\
> +	_entry_(6, "csid", CSID, true, NULL),			\
> +	_entry_(7, "csie", CSIE, true, NULL),			\
> +	_entry_(8, "csif", CSIF, true, NULL),			\
> +	_entry_(9, "dbg", DBG, true, "vddio-dbg"),		\
> +	_entry_(10, "debug-nonao", DEBUG_NONAO, true, NULL),	\
> +	_entry_(11, "dmic", DMIC, true, "vddio-dmic"),		\
> +	_entry_(12, "dp", DP, true, NULL),			\
> +	_entry_(13, "dsi", DSI, true, NULL),			\
> +	_entry_(14, "dsib", DSIB, true, NULL),			\
> +	_entry_(15, "dsic", DSIC, true, NULL),			\
> +	_entry_(16, "dsid", DSID, true, NULL),			\
> +	_entry_(17, "emmc", SDMMC4, true, NULL),		\
> +	_entry_(18, "emmc2", EMMC2, true, NULL),		\
> +	_entry_(19, "gpio", GPIO, true, "vddio-gpio"),		\
> +	_entry_(20, "hdmi", HDMI, true, NULL),			\
> +	_entry_(21, "hsic", HSIC, true, NULL),			\
> +	_entry_(22, "lvds", LVDS, true, NULL),			\
> +	_entry_(23, "mipi-bias", MIPI_BIAS, true, NULL),	\
> +	_entry_(24, "pex-bias", PEX_BIAS, true, NULL),		\
> +	_entry_(25, "pex-clk1", PEX_CLK1, true, NULL),		\
> +	_entry_(26, "pex-clk2", PEX_CLK2, true, NULL),		\
> +	_entry_(27, "pex-ctrl", PEX_CNTRL, false, "vddio-pex-ctrl"), \
> +	_entry_(28, "sdmmc1", SDMMC1, true, "vddio-sdmmc1"),	\
> +	_entry_(29, "sdmmc3", SDMMC3, true, "vddio-sdmmc3"),	\
> +	_entry_(30, "spi", SPI, true, "vddio-spi"),		\
> +	_entry_(31, "spi-hv", SPI_HV, true, "vddio-spi-hv"),	\
> +	_entry_(32, "uart", UART, true, "vddio-uart"),		\
> +	_entry_(33, "usb0", USB0, true, NULL),			\
> +	_entry_(34, "usb1", USB1, true, NULL),			\
> +	_entry_(35, "usb2", USB2, true, NULL),			\
> +	_entry_(36, "usb3", USB3, true, NULL),			\
> +	_entry_(37, "usb-bias", USB_BIAS, true, NULL)
> +
> +#define TEGRA_IO_PAD_INFO(_pin, _name, _id, _lpstate, _vsupply)	\
> +	{							\
> +		.name = _name,					\
> +		.pins = {(_pin)},				\
> +		.id = TEGRA_IO_PAD_##_id,			\
> +		.vsupply = (_vsupply),				\
> +		.supports_low_power = (_lpstate),		\
> +	}
> +
> +static const struct tegra_io_pads_cfg tegra124_io_pads_cfg_info[] = {
> +	TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
> +};
> +
> +static const struct tegra_io_pads_cfg tegra210_io_pads_cfg_info[] = {
> +	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
> +};

That's a weird way of writing these tables. Why not do something simpler
and much more common such as:

	#define TEGRA_IO_PAD_INFO(...) ...

	static const struct tegra_io_pads_cfg tegra124_io_pads_cfgs[] = {
		TEGRA_IO_PAD_INFO(...),
		...
	};

	static const struct tegra_io_pads_cfg tegra210_io_pad_cfgs[] = {
		TEGRA_IO_PAD_INFO(...),
		...
	};

> +
> +#define TEGRA_IO_PAD_DESC(_pin, _name, _id, _lpstate, _vsupply)	\
> +	PINCTRL_PIN(_pin, _name)
> +
> +static const struct pinctrl_pin_desc tegra124_io_pads_pinctrl_desc[] = {
> +	TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
> +};
> +
> +static const struct pinctrl_pin_desc tegra210_io_pads_pinctrl_desc[] = {
> +	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_DESC),
> +};
> +
> +static const struct tegra_io_pads_soc_data tegra124_io_pad_soc_data = {
> +	.desc		= tegra124_io_pads_pinctrl_desc,
> +	.num_desc	= ARRAY_SIZE(tegra124_io_pads_pinctrl_desc),
> +	.cfg		= tegra124_io_pads_cfg_info,
> +	.num_cfg	= ARRAY_SIZE(tegra124_io_pads_cfg_info),
> +};
> +
> +static const struct tegra_io_pads_soc_data tegra210_io_pad_soc_data = {
> +	.desc		= tegra210_io_pads_pinctrl_desc,
> +	.num_desc	= ARRAY_SIZE(tegra210_io_pads_pinctrl_desc),
> +	.cfg		= tegra210_io_pads_cfg_info,
> +	.num_cfg	= ARRAY_SIZE(tegra210_io_pads_cfg_info),
> +};
> +
> +static const struct platform_device_id tegra_io_pads_dev_id[] = {
> +	{
> +		.name = "pinctrl-t124-io-pad",
> +		.driver_data = (kernel_ulong_t)&tegra124_io_pad_soc_data,
> +	}, {
> +		.name = "pinctrl-t210-io-pad",
> +		.driver_data = (kernel_ulong_t)&tegra210_io_pad_soc_data,
> +	}, {
> +	},
> +};
> +MODULE_DEVICE_TABLE(platform, tegra_io_pads_dev_id);
> +
> +static struct platform_driver tegra_io_pads_pinctrl_driver = {
> +	.driver		= {
> +		.name	= "pinctrl-tegra-io-pad",
> +	},
> +	.probe		= tegra_io_pads_pinctrl_probe,
> +	.id_table	= tegra_io_pads_dev_id,
> +};
> +
> +module_platform_driver(tegra_io_pads_pinctrl_driver);
> +
> +MODULE_DESCRIPTION("NVIDIA TEGRA IO pad Control Driver");
> +MODULE_AUTHOR("Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> +MODULE_LICENSE("GPL v2");

Like I said above, I think there's a lot of boilerplate in here that's
simply there to create a virtual device and bind a driver to it. All of
that comes with very little to no benefit. I think this could all just
be moved into the PMC driver and be simplified quite a bit.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Laxman Dewangan]

Thanks Thierry for review.

On Friday 25 November 2016 03:27 PM, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
>> +	  NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
>> +	  level of interfacing and deep power down mode of IO pads. The
>> +	  voltage of IO pads are SW configurable based on IO rail of that
>> +	  pads on T210. This driver provides the interface to change IO pad
>> +	  voltage and power state via pincontrol interface.
> This has a lot of chip-specific text. Will all of that have to be
> updated if support for new chips is added?

Then saying that Tegra124 and later..
Hoping, people know our chip releasing sequence as numbering are not in 
sequence.

>
>> +#include <linux/regulator/consumer.h>
>> +#include <soc/tegra/pmc.h>
> Have you considered moving this code into the PMC driver? It seems a
> little over the top to go through all of the platform device creation
> and driver registration dance only to call into a public API later on.

Yes, we had discussion on this and suggestion came to use the pinctrl 
framework.
If we do in the pmc driver then we will need lots of DT processing for 
getting information from DT which we can directly get from the pinctrl 
core framework.
Also client driver may need to have the control dynamically and get the 
IO pads from DT. So implementing all in pmc will be huge duplication 
over already existing framework.



>
>> +#include "../core.h"
>> +#include "../pinconf.h"
>> +#include "../pinctrl-utils.h"
>> +
>> +#define TEGRA_IO_RAIL_1800000UV 1800000
>> +#define TEGRA_IO_RAIL_3300000UV 3300000
> Is there really a point in having these defines? They are really long
> and effectively don't carry more information than just the plain
> numbers.


using macros is always good instead of magic number in code and hence it 
is there.

>
>
>> +#define tegra_io_uv_to_io_pads_uv(io_uv)				\
>> +		(((io_uv) == TEGRA_IO_RAIL_1800000UV) ?			\
>> +		  TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV)
>> +
>> +#define tegra_io_voltage_is_valid(io_uv)			\
>> +	({ typeof(io_uv) io_uv_ = (io_uv);			\
>> +	    ((io_uv_ == TEGRA_IO_RAIL_1800000UV) ||		\
>> +	     (io_uv_ == TEGRA_IO_RAIL_3300000UV)); })
>>
> Maybe make both of these static inline functions to improve readability?
> I find the above very hard to read.

OK, will convert to the inline but dont think it will be less complex.
>> +	enum tegra_io_pad_voltage pad_volt;
>>
>> +
>>
>> +
>> +#define TEGRA_IO_PAD_INFO(_pin, _name, _id, _lpstate, _vsupply)	\
>> +	{							\
>> +		.name = _name,					\
>> +		.pins = {(_pin)},				\
>> +		.id = TEGRA_IO_PAD_##_id,			\
>> +		.vsupply = (_vsupply),				\
>> +		.supports_low_power = (_lpstate),		\
>> +	}
>> +
>> +static const struct tegra_io_pads_cfg tegra124_io_pads_cfg_info[] = {
>> +	TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
>> +};
>> +
>> +static const struct tegra_io_pads_cfg tegra210_io_pads_cfg_info[] = {
>> +	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
>> +};
> That's a weird way of writing these tables. Why not do something simpler
> and much more common such as:
>
> 	#define TEGRA_IO_PAD_INFO(...) ...
>
> 	static const struct tegra_io_pads_cfg tegra124_io_pads_cfgs[] = {
> 		TEGRA_IO_PAD_INFO(...),
> 		...
> 	};
>
> 	static const struct tegra_io_pads_cfg tegra210_io_pad_cfgs[] = {
> 		TEGRA_IO_PAD_INFO(...),
> 		...
> 	};

This is done to use the same table for initialing two different 
structure. If we go as above  then we will endup with the two tables.

or define and then redefine the TEGRA_IO_PAD_INFO.


>
>> +
>>
>> +
>> +module_platform_driver(tegra_io_pads_pinctrl_driver);
>> +
>> +MODULE_DESCRIPTION("NVIDIA TEGRA IO pad Control Driver");
>> +MODULE_AUTHOR("Laxman Dewangan <ldewangan@nvidia.com>");
>> +MODULE_LICENSE("GPL v2");
> Like I said above, I think there's a lot of boilerplate in here that's
> simply there to create a virtual device and bind a driver to it. All of
> that comes with very little to no benefit. I think this could all just
> be moved into the PMC driver and be simplified quite a bit.
>

What did you not like here?
Let me specific to my query to get idea about your view:
1. Do you agree for the interface as pinctrl here?
2. Do you want to call the probe() function implemented here as simple 
function call from pmc's probe?
3. If (2) is yes then how do you want to differentiate T124 or T210? Via 
argument?

It will be really help if you can provide the pseudo code from PMC and 
this driver so that I can implement the same in the next patch.

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

[Jon Hunter]

On 25/11/16 12:04, Laxman Dewangan wrote:
> Thanks Thierry for review.
> 
> On Friday 25 November 2016 03:27 PM, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
>>> +      NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
>>> +      level of interfacing and deep power down mode of IO pads. The
>>> +      voltage of IO pads are SW configurable based on IO rail of that
>>> +      pads on T210. This driver provides the interface to change IO pad
>>> +      voltage and power state via pincontrol interface.
>> This has a lot of chip-specific text. Will all of that have to be
>> updated if support for new chips is added?
> 
> Then saying that Tegra124 and later..
> Hoping, people know our chip releasing sequence as numbering are not in
> sequence.
> 
>>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <soc/tegra/pmc.h>
>> Have you considered moving this code into the PMC driver? It seems a
>> little over the top to go through all of the platform device creation
>> and driver registration dance only to call into a public API later on.
> 
> Yes, we had discussion on this and suggestion came to use the pinctrl
> framework.
> If we do in the pmc driver then we will need lots of DT processing for
> getting information from DT which we can directly get from the pinctrl
> core framework.
> Also client driver may need to have the control dynamically and get the
> IO pads from DT. So implementing all in pmc will be huge duplication
> over already existing framework.

I don't follow. We already did something similar for the Tegra DPAUX
driver [0].

Cheers
Jon

[0]
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/tegra/dpaux.c?id=0751bb5c44fe1aa9494ce259d974c3d249b73a84

-- 
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laxman Dewangan]

On Friday 25 November 2016 10:59 PM, Jon Hunter wrote:
> On 25/11/16 12:04, Laxman Dewangan wrote:
>> Thanks Thierry for review.
>>
>> On Friday 25 November 2016 03:27 PM, Thierry Reding wrote:
>>> * PGP Signed by an unknown key
>>>
>>> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
>>>> +      NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
>>>> +      level of interfacing and deep power down mode of IO pads. The
>>>> +      voltage of IO pads are SW configurable based on IO rail of that
>>>> +      pads on T210. This driver provides the interface to change IO pad
>>>> +      voltage and power state via pincontrol interface.
>>> This has a lot of chip-specific text. Will all of that have to be
>>> updated if support for new chips is added?
>> Then saying that Tegra124 and later..
>> Hoping, people know our chip releasing sequence as numbering are not in
>> sequence.
>>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <soc/tegra/pmc.h>
>>> Have you considered moving this code into the PMC driver? It seems a
>>> little over the top to go through all of the platform device creation
>>> and driver registration dance only to call into a public API later on.
>> Yes, we had discussion on this and suggestion came to use the pinctrl
>> framework.
>> If we do in the pmc driver then we will need lots of DT processing for
>> getting information from DT which we can directly get from the pinctrl
>> core framework.
>> Also client driver may need to have the control dynamically and get the
>> IO pads from DT. So implementing all in pmc will be huge duplication
>> over already existing framework.
> I don't follow. We already did something similar for the Tegra DPAUX
> driver [0].
>
> Cheers
> Jon
>
> [0]
> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/tegra/dpaux.c?id=0751bb5c44fe1aa9494ce259d974c3d249b73a84

In the above dpaux driver,  you used the pinctrl framework and its core 
functionality for the device tree interfacing and client interfacing.

The same thing I am saying here, we should not avoid the pinctrl 
framework. The client driver will use the pinctrl framework for the IO 
pad configurations, not direct PMC APIs.

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

[Jon Hunter]

On 25/11/16 17:49, Laxman Dewangan wrote:
> 
> On Friday 25 November 2016 10:59 PM, Jon Hunter wrote:
>> On 25/11/16 12:04, Laxman Dewangan wrote:
>>> Thanks Thierry for review.
>>>
>>> On Friday 25 November 2016 03:27 PM, Thierry Reding wrote:
>>>> * PGP Signed by an unknown key
>>>>
>>>> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
>>>>> +      NVIDIA Tegra124/210 SoC has IO pads which supports
>>>>> multi-voltage
>>>>> +      level of interfacing and deep power down mode of IO pads. The
>>>>> +      voltage of IO pads are SW configurable based on IO rail of that
>>>>> +      pads on T210. This driver provides the interface to change
>>>>> IO pad
>>>>> +      voltage and power state via pincontrol interface.
>>>> This has a lot of chip-specific text. Will all of that have to be
>>>> updated if support for new chips is added?
>>> Then saying that Tegra124 and later..
>>> Hoping, people know our chip releasing sequence as numbering are not in
>>> sequence.
>>>
>>>>> +#include <linux/regulator/consumer.h>
>>>>> +#include <soc/tegra/pmc.h>
>>>> Have you considered moving this code into the PMC driver? It seems a
>>>> little over the top to go through all of the platform device creation
>>>> and driver registration dance only to call into a public API later on.
>>> Yes, we had discussion on this and suggestion came to use the pinctrl
>>> framework.
>>> If we do in the pmc driver then we will need lots of DT processing for
>>> getting information from DT which we can directly get from the pinctrl
>>> core framework.
>>> Also client driver may need to have the control dynamically and get the
>>> IO pads from DT. So implementing all in pmc will be huge duplication
>>> over already existing framework.
>> I don't follow. We already did something similar for the Tegra DPAUX
>> driver [0].
>>
>> Cheers
>> Jon
>>
>> [0]
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/gpu/drm/tegra/dpaux.c?id=0751bb5c44fe1aa9494ce259d974c3d249b73a84
>>
> 
> In the above dpaux driver,  you used the pinctrl framework and its core
> functionality for the device tree interfacing and client interfacing.
> 
> The same thing I am saying here, we should not avoid the pinctrl
> framework. The client driver will use the pinctrl framework for the IO
> pad configurations, not direct PMC APIs.

Exactly, so why are you saying that by moving the code into the PMC
driver this will "need lots of DT processing for getting information
from DT"? By moving the code, we are not suggesting we don't use the
pinctrl framework, we are just suggesting we move the code. That's all.

Jon

-- 
nvpublic

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

[Laxman Dewangan]

On Monday 28 November 2016 02:56 PM, Jon Hunter wrote:
> On 25/11/16 17:49, Laxman Dewangan wrote:
>>
>> In the above dpaux driver,  you used the pinctrl framework and its core
>> functionality for the device tree interfacing and client interfacing.
>>
>> The same thing I am saying here, we should not avoid the pinctrl
>> framework. The client driver will use the pinctrl framework for the IO
>> pad configurations, not direct PMC APIs.
> Exactly, so why are you saying that by moving the code into the PMC
> driver this will "need lots of DT processing for getting information
> from DT"? By moving the code, we are not suggesting we don't use the
> pinctrl framework, we are just suggesting we move the code. That's all.
>
>
OK, got it. My understanding  from suggestion was that to implement 
everything without using the pinctrl framework.

I dont see much issue to just move this to  drivers/soc/tegra folder
I hope rest of stuff will be same, registering it as subdev of the pmc 
and this io pad driver will be the platform driver.

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

[Jon Hunter]

On 25/11/16 09:57, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
>> NVIDIA Tegra124 and later SoCs support the multi-voltage level and
>> low power state of some of its IO pads. The IO pads can work in
>> the voltage of the 1.8V and 3.3V of IO voltage from IO power rail
>> sources. When IO interfaces are not used then IO pads can be
>> configure in low power state to reduce the power consumption from
>> that IO pads.
>>
>> On Tegra124, the voltage level of IO power rail source is auto
>> detected by hardware(SoC) and hence it is only require to configure
>> in low power mode if IO pads are not used.
>>
>> On T210 onwards, the auto-detection of voltage level from IO power
>> rail is removed from SoC and hence SW need to configure the PMC
>> register explicitly to set proper voltage in IO pads based on
>> IO rail power source voltage.
>>
>> This driver adds the IO pad driver to configure the power state and
>> IO pad voltage based on the usage and power tree via pincontrol
>> framework. The configuration can be static and dynamic.
>>
>> Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> ---
>> Changes from V1:
>> - Dropped the custom properties to set pad voltage and use regulator.
>> - Added support for regulator to get vottage in boot and configure IO
>>   pad voltage.
>> - Add support for callback to handle regulator notification and configure
>>   IO pad voltage based on voltage change.
>>
>> Changes from V2:
>>  Mostly nit changes per Jon's feedback i.e. use macros for voltage, added
>>  comment on macros, reduce the structure and variable name size, optimise
>>  number of variables, and allocate memory for regulator info when it needed.
>>
>> Changes from V3:
>>  Use the devm_regulator_get() instead of devm_regulator_get_optional().
>>
>>  drivers/pinctrl/tegra/Kconfig                |  12 +
>>  drivers/pinctrl/tegra/Makefile               |   1 +
>>  drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 530 +++++++++++++++++++++++++++
>>  3 files changed, 543 insertions(+)
>>  create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c

...

>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>> new file mode 100644
>> index 0000000..aab02d0
>> --- /dev/null
>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>> @@ -0,0 +1,530 @@
>> +/*
>> + * pinctrl-tegra-io-pad: IO PAD driver for configuration of IO rail and deep
>> + *			 Power Down mode via pinctrl framework.
>> + *
>> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
>> + *
>> + * Author: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/pinctrl/pinctrl.h>
>> +#include <linux/pinctrl/pinconf-generic.h>
>> +#include <linux/pinctrl/pinconf.h>
>> +#include <linux/pinctrl/pinmux.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <soc/tegra/pmc.h>
> 
> Have you considered moving this code into the PMC driver? It seems a
> little over the top to go through all of the platform device creation
> and driver registration dance only to call into a public API later on.

I would prefer moving this under driver/soc/tegra as well (even if it is
not in the same source file) so we don't need all this public APIs.

Cheers
Jon

-- 
nvpublic
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Laxman Dewangan]

On Friday 25 November 2016 10:56 PM, Jon Hunter wrote:
> On 25/11/16 09:57, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
> ...
>
>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>>> new file mode 100644
>>> index 0000000..aab02d0
>>> --- /dev/null
>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>>> @@ -0,0 +1,530 @@
>>> +/*
>>> + * pinctrl-tegra-io-pad: IO PAD driver for configuration of IO rail and deep
>>> + *			 Power Down mode via pinctrl framework.
>>> + *
>>> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
>>> + *
>>> + * Author: Laxman Dewangan <ldewangan@nvidia.com>
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License version 2 as
>>> + * published by the Free Software Foundation.
>>> + */
>>> +
>>> +#include <linux/delay.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/pinctrl/pinctrl.h>
>>> +#include <linux/pinctrl/pinconf-generic.h>
>>> +#include <linux/pinctrl/pinconf.h>
>>> +#include <linux/pinctrl/pinmux.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +#include <soc/tegra/pmc.h>
>> Have you considered moving this code into the PMC driver? It seems a
>> little over the top to go through all of the platform device creation
>> and driver registration dance only to call into a public API later on.
> I would prefer moving this under driver/soc/tegra as well (even if it is
> not in the same source file) so we don't need all this public APIs.
>

Do we really gain anything here by moving driver to drivers/soc/tegra?
The folder drivers/pinctrl/tegra is dedicated folder for Tegra specific.

We should keep the related driver in given subsystem until this is 
really hard to do it.
Even if we organise and move this driver to the driver/soc/tegra, we 
will need only 3-4 APIs from the public pmc header to the private header.

I think calling the tegra specific headers/APIs from tegra specific 
driver should be fine here.

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

[Jon Hunter]

On 25/11/16 17:45, Laxman Dewangan wrote:
> 
> On Friday 25 November 2016 10:56 PM, Jon Hunter wrote:
>> On 25/11/16 09:57, Thierry Reding wrote:
>>> * PGP Signed by an unknown key
>>>
>>> On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
>> ...
>>
>>>> diff --git a/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>>>> b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>>>> new file mode 100644
>>>> index 0000000..aab02d0
>>>> --- /dev/null
>>>> +++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
>>>> @@ -0,0 +1,530 @@
>>>> +/*
>>>> + * pinctrl-tegra-io-pad: IO PAD driver for configuration of IO rail
>>>> and deep
>>>> + *             Power Down mode via pinctrl framework.
>>>> + *
>>>> + * Copyright (C) 2016 NVIDIA CORPORATION. All rights reserved.
>>>> + *
>>>> + * Author: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + */
>>>> +
>>>> +#include <linux/delay.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/pinctrl/pinctrl.h>
>>>> +#include <linux/pinctrl/pinconf-generic.h>
>>>> +#include <linux/pinctrl/pinconf.h>
>>>> +#include <linux/pinctrl/pinmux.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +#include <soc/tegra/pmc.h>
>>> Have you considered moving this code into the PMC driver? It seems a
>>> little over the top to go through all of the platform device creation
>>> and driver registration dance only to call into a public API later on.
>> I would prefer moving this under driver/soc/tegra as well (even if it is
>> not in the same source file) so we don't need all this public APIs.
>>
> 
> Do we really gain anything here by moving driver to drivers/soc/tegra?

Only avoid adding these public APIs. By using the pinctrl framework, it
would be nice to avoid having to still have public APIs that someone
could use directly.

> The folder drivers/pinctrl/tegra is dedicated folder for Tegra specific.
> We should keep the related driver in given subsystem until this is
> really hard to do it.
> Even if we organise and move this driver to the driver/soc/tegra, we
> will need only 3-4 APIs from the public pmc header to the private header.
> I think calling the tegra specific headers/APIs from tegra specific
> driver should be fine here.

Its OK, but I still prefer not having them at all.

Jon

-- 
nvpublic

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

[kbuild test robot]

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

Hi Laxman,

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v4.9-rc7 next-20161202]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Laxman-Dewangan/pinctrl-tegra-Add-DT-binding-for-io-pads-control/20161124-185652
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tegra/linux.git for-next
config: arm-allmodconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c: In function 'tegra_io_pads_pinconf_get':
>> drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:124:9: error: implicit declaration of function 'tegra_io_pad_power_get_status' [-Werror=implicit-function-declaration]
      ret = tegra_io_pad_power_get_status(cfg->id);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

vim +/tegra_io_pad_power_get_status +124 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c

   118				dev_err(tiopi->dev,
   119					"IO pad %s does not support low power\n",
   120					cfg->name);
   121				return -EINVAL;
   122			}
   123	
 > 124			ret = tegra_io_pad_power_get_status(cfg->id);
   125			if (ret < 0)
   126				return ret;
   127			arg = !ret;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 59453 bytes --]