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

[PATCH V2 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.

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       | 112 +++++++
 drivers/pinctrl/tegra/Kconfig                      |  12 +
 drivers/pinctrl/tegra/Makefile                     |   1 +
 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c       | 369 +++++++++++++++++++++
 include/dt-bindings/pinctrl/pinctrl-tegra-io-pad.h |  21 ++
 5 files changed, 515 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/nvidia,tegra-io-pad.txt
 create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
 create mode 100644 include/dt-bindings/pinctrl/pinctrl-tegra-io-pad.h

-- 
2.1.4

[PATCH V2 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:
 The DT binding document is modified to explain the regulator handle
 for different IOs and how can it be passed from the DT.
---
 .../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..6ca961f
--- /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 voltage
+of the 1.8V and 3.3V of IO voltage 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 other wise 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 = <&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

[PATCH V2 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.
---
 drivers/pinctrl/tegra/Kconfig                |  12 +
 drivers/pinctrl/tegra/Makefile               |   1 +
 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 488 +++++++++++++++++++++++++++
 3 files changed, 501 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..f5cf0d0
--- /dev/null
+++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
@@ -0,0 +1,488 @@
+/*
+ * 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"
+
+/**
+ * Macro for 1.8V, keep 200mV as tolerance for deciding that
+ * IO pads should be set for 3.3V (high voltage) or 1.8V.
+ */
+#define TEGRA_IO_PAD_1800000UV_UPPER_LIMIT 2000000
+
+struct tegra_io_pads_cfg_info {
+	const char *name;
+	const unsigned int pins[1];
+	const char *vsupply;
+	enum tegra_io_pad pad_id;
+	bool support_low_power_state;
+};
+
+struct tegra_io_pad_soc_data {
+	const struct tegra_io_pads_cfg_info *pads_cfg;
+	int num_pads_cfg;
+	const struct pinctrl_pin_desc *pins_desc;
+	int num_pins_desc;
+};
+
+struct tegra_io_pads_regulator_info {
+	struct device *dev;
+	const struct tegra_io_pads_cfg_info *pads_cfg;
+	struct regulator *regulator;
+	struct notifier_block regulator_nb;
+};
+
+struct tegra_io_pads_info {
+	struct device *dev;
+	struct pinctrl_dev *pctl;
+	struct tegra_io_pads_regulator_info *rinfo;
+	const struct tegra_io_pad_soc_data *soc_data;
+};
+
+static int tegra_iop_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
+{
+	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
+
+	return tiopi->soc_data->num_pads_cfg;
+}
+
+static const char *tegra_iop_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->pads_cfg[group].name;
+}
+
+static int tegra_iop_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->pads_cfg[group].pins;
+	*num_pins = 1;
+
+	return 0;
+}
+
+static const struct pinctrl_ops tegra_iop_pinctrl_ops = {
+	.get_groups_count	= tegra_iop_pinctrl_get_groups_count,
+	.get_group_name		= tegra_iop_pinctrl_get_group_name,
+	.get_group_pins		= tegra_iop_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_pad_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_info *pads_cfg =
+					&tiopi->soc_data->pads_cfg[pin];
+	enum tegra_io_pad pad_id = pads_cfg->pad_id;
+	int arg = 0;
+	int ret;
+
+	switch (param) {
+	case PIN_CONFIG_LOW_POWER_MODE:
+		ret = tegra_io_pad_power_get_status(pad_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_pad_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_info *pads_cfg =
+					&tiopi->soc_data->pads_cfg[pin];
+	int pad_id = pads_cfg->pad_id;
+	u16 param_val;
+	int param;
+	int ret;
+	int i;
+
+	for (i = 0; i < num_configs; i++) {
+		param = pinconf_to_config_param(configs[i]);
+		param_val = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_LOW_POWER_MODE:
+			if (param_val)
+				ret = tegra_io_pad_power_disable(pad_id);
+			else
+				ret = tegra_io_pad_power_enable(pad_id);
+			if (ret < 0) {
+				dev_err(tiopi->dev,
+					"Failed to set DPD %d of pin %u: %d\n",
+					param_val, pin, 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_pad_pinconf_ops = {
+	.pin_config_get = tegra_io_pad_pinconf_get,
+	.pin_config_set = tegra_io_pad_pinconf_set,
+};
+
+static struct pinctrl_desc tegra_iop_pinctrl_desc = {
+	.name = "pinctrl-tegra-io-pads",
+	.pctlops = &tegra_iop_pinctrl_ops,
+	.confops = &tegra_io_pad_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, old_uv;
+	enum tegra_io_pad_voltage io_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 ((vdata->old_uV > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
+		    (vdata->min_uV <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT))
+			break;
+
+		ret = tegra_io_pad_set_voltage(rinfo->pads_cfg->pad_id,
+					       TEGRA_IO_PAD_3300000UV);
+		if (ret < 0) {
+			dev_err(rinfo->dev,
+				"Failed to set voltage %lu of pad %s: %d\n",
+				vdata->min_uV, rinfo->pads_cfg->name, ret);
+			return ret;
+		}
+		break;
+
+	case REGULATOR_EVENT_VOLTAGE_CHANGE:
+		io_volt_uv = (unsigned long)data;
+		ret = tegra_io_pad_get_voltage(rinfo->pads_cfg->pad_id);
+		if (ret < 0) {
+			dev_err(rinfo->dev, "Failed to get IO pad voltage: %d\n",
+				ret);
+			return ret;
+		}
+		old_uv = (ret == TEGRA_IO_PAD_1800000UV) ? 1800000 : 3300000;
+		if (((io_volt_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
+		      (old_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT)) ||
+		       ((io_volt_uv > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
+			 (old_uv > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT)))
+			break;
+
+		ret = tegra_io_pad_set_voltage(rinfo->pads_cfg->pad_id,
+					       TEGRA_IO_PAD_1800000UV);
+		if (ret < 0) {
+			dev_err(rinfo->dev,
+				"Failed to set voltage %lu of pad %s: %d\n",
+				vdata->min_uV, rinfo->pads_cfg->name, ret);
+			return ret;
+		}
+		break;
+
+	case REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE:
+		io_volt_uv = (unsigned long)data;
+		io_volt = (io_volt_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) ?
+			   TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV;
+		ret = tegra_io_pad_set_voltage(rinfo->pads_cfg->pad_id,
+					       io_volt);
+		if (ret < 0) {
+			dev_err(rinfo->dev,
+				"Failed to set voltage %lu of pad %s: %d\n",
+				io_volt_uv, rinfo->pads_cfg->name, ret);
+			return ret;
+		}
+		break;
+
+	default:
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int tegra_iop_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_pad_soc_data *soc_data;
+	struct device_node *np_parent = pdev->dev.parent->of_node;
+	struct tegra_io_pads_info *tiopi;
+	int ret, i;
+
+	if (!np_parent) {
+		dev_err(dev, "PMC should be register from DT\n");
+		return -ENODEV;
+	}
+
+	soc_data = (const struct tegra_io_pad_soc_data *)id->driver_data;
+
+	tiopi = devm_kzalloc(dev, sizeof(*tiopi), GFP_KERNEL);
+	if (!tiopi)
+		return -ENOMEM;
+
+	tiopi->rinfo = devm_kzalloc(dev, sizeof(*tiopi->rinfo) *
+				    soc_data->num_pads_cfg, GFP_KERNEL);
+	if (!tiopi->rinfo)
+		return -ENOMEM;
+
+	tiopi->dev = &pdev->dev;
+	pdev->dev.of_node = np_parent;
+	tiopi->soc_data = soc_data;
+
+	for (i = 0; i < soc_data->num_pads_cfg; ++i) {
+		struct tegra_io_pads_regulator_info *rinfo = tiopi->rinfo + i;
+		const struct tegra_io_pads_cfg_info *pads_cfg =
+							&soc_data->pads_cfg[i];
+		struct regulator *regulator;
+		int io_volt_uv;
+		enum tegra_io_pad_voltage io_volt;
+
+		if (!pads_cfg->vsupply)
+			continue;
+
+		regulator = devm_regulator_get_optional(dev, pads_cfg->vsupply);
+		if (IS_ERR(regulator)) {
+			ret = PTR_ERR(regulator);
+			if (ret == -EPROBE_DEFER)
+				return ret;
+			continue;
+		}
+
+		io_volt_uv = regulator_get_voltage(regulator);
+		if (io_volt_uv < 0) {
+			dev_err(dev, "Failed to get voltage for rail %s: %d\n",
+				pads_cfg->vsupply, io_volt_uv);
+			return ret;
+		}
+
+		io_volt = (io_volt_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) ?
+			   TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV;
+
+		ret = tegra_io_pad_set_voltage(pads_cfg->pad_id, io_volt);
+		if (ret < 0) {
+			dev_err(dev, "Failed to set voltage %d of pad %s: %d\n",
+				io_volt_uv, pads_cfg->name, ret);
+			return ret;
+		}
+		rinfo->dev = tiopi->dev;
+		rinfo->regulator = regulator;
+		rinfo->pads_cfg = pads_cfg;
+
+		rinfo->regulator_nb.notifier_call =
+					tegra_io_pads_rail_change_notify_cb;
+		ret = devm_regulator_register_notifier(regulator,
+						       &rinfo->regulator_nb);
+		if (ret < 0) {
+			dev_err(dev, "Failed to register regulator %s notifier: %d\n",
+				pads_cfg->name, ret);
+			return ret;
+		}
+	}
+
+	tegra_iop_pinctrl_desc.pins = tiopi->soc_data->pins_desc;
+	tegra_iop_pinctrl_desc.npins = tiopi->soc_data->num_pins_desc;
+	platform_set_drvdata(pdev, tiopi);
+
+	tiopi->pctl = devm_pinctrl_register(dev, &tegra_iop_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(_id, _name, _pad_id, _lpstate, _vsupply)	\
+	{								\
+		.name = _name,						\
+		.pins = {(_id)},					\
+		.pad_id = TEGRA_IO_PAD_##_pad_id,			\
+		.vsupply = (_vsupply),				\
+		.support_low_power_state = (_lpstate),			\
+	}
+
+static const struct tegra_io_pads_cfg_info tegra124_io_pads_cfg_info[] = {
+	TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
+
+static const struct tegra_io_pads_cfg_info tegra210_io_pads_cfg_info[] = {
+	TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
+
+#define TEGRA_IO_PAD_DESC(_id, _name, _pad_id, _lpstate, _vsupply)	\
+	PINCTRL_PIN(_id, _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_pad_soc_data tegra124_io_pad_soc_data = {
+	.pins_desc	= tegra124_io_pads_pinctrl_desc,
+	.num_pins_desc	= ARRAY_SIZE(tegra124_io_pads_pinctrl_desc),
+	.pads_cfg	= tegra124_io_pads_cfg_info,
+	.num_pads_cfg	= ARRAY_SIZE(tegra124_io_pads_cfg_info),
+};
+
+static const struct tegra_io_pad_soc_data tegra210_io_pad_soc_data = {
+	.pins_desc	= tegra210_io_pads_pinctrl_desc,
+	.num_pins_desc	= ARRAY_SIZE(tegra210_io_pads_pinctrl_desc),
+	.pads_cfg	= tegra210_io_pads_cfg_info,
+	.num_pads_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_iop_pinctrl_driver = {
+	.driver		= {
+		.name	= "pinctrl-tegra-io-pad",
+	},
+	.probe		= tegra_iop_pinctrl_probe,
+	.id_table	= tegra_io_pads_dev_id,
+};
+
+module_platform_driver(tegra_iop_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 V2 1/2] pinctrl: tegra: Add DT binding for io pads control

[Rob Herring]

On Wed, Nov 09, 2016 at 06:36:21PM +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.
> 
> 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:
>  The DT binding document is modified to explain the regulator handle
>  for different IOs and how can it be passed from the DT.
> ---
>  .../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..6ca961f
> --- /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 voltage
> +of the 1.8V and 3.3V of IO voltage 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 other wise dynamically based on IO
> +rail voltage on that IO pads and usage of IO pads

s/other wise/otherwise/

The end of the sentence is not clear and missing a period.

> +
> +The DT property of the io pads must be under the node of pmc i.e.
> +pmc@7000e400 for Tegra124 onwards.

s/io/IO/

> +
> +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 = <&vdd_cam>;

Missing -supply.

> +		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";

What's the purpose of grouping these?

> +				low-power-enable;
> +			};
> +		};
> +
> +	};
> -- 
> 2.1.4
> 

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

[Linus Walleij]

On Wed, Nov 9, 2016 at 2:06 PM, Laxman Dewangan <ldewangan@nvidia.com> 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@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.

I'm waiting for maintainer review on this.
Stephen/Thierry?

Yours,
Linus Walleij

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

[Laxman Dewangan]

On Tuesday 15 November 2016 01:04 AM, Rob Herring wrote:
> On Wed, Nov 09, 2016 at 06:36:21PM +0530, Laxman Dewangan wrote:
> +
> +			audio {
> +				pins = "audio", "dmic", "sdmmc3";
> What's the purpose of grouping these?

Just to show that multiple IO pads can be provided here as this is from 
pinctrl dt binding.

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

[Jon Hunter]

On 09/11/16 13:06, 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@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.
> ---
>  drivers/pinctrl/tegra/Kconfig                |  12 +
>  drivers/pinctrl/tegra/Makefile               |   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 488 +++++++++++++++++++++++++++
>  3 files changed, 501 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..f5cf0d0
> --- /dev/null
> +++ b/drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c
> @@ -0,0 +1,488 @@
> +/*
> + * 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"
> +
> +/**
> + * Macro for 1.8V, keep 200mV as tolerance for deciding that
> + * IO pads should be set for 3.3V (high voltage) or 1.8V.
> + */
> +#define TEGRA_IO_PAD_1800000UV_UPPER_LIMIT 2000000

Is there a reference we could add for the source of this information?

> +
> +struct tegra_io_pads_cfg_info {

Nit-pick do you need the suffix '_info'? May be nice to keep the name
shorter and just have 'tegra_io_pads_cfg'

> +	const char *name;
> +	const unsigned int pins[1];
> +	const char *vsupply;
> +	enum tegra_io_pad pad_id;

Nit-pick, I think "id" would be sufficient here.

> +	bool support_low_power_state;

I don't see where the above is used. I would also shorten to
"supports_low_power".

> +};
> +
> +struct tegra_io_pad_soc_data {

s/tegra_io_pad/tegra_io_pads/

> +	const struct tegra_io_pads_cfg_info *pads_cfg;
> +	int num_pads_cfg;

May be just ...

	const struct tegra_io_pads_cfg_info *cfgs;
	int num_cfgs;

> +	const struct pinctrl_pin_desc *pins_desc;
> +	int num_pins_desc;
> +};
> +
> +struct tegra_io_pads_regulator_info {
> +	struct device *dev;
> +	const struct tegra_io_pads_cfg_info *pads_cfg;
> +	struct regulator *regulator;
> +	struct notifier_block regulator_nb;
> +};

Is this struct necessary? Seems to be a lot of duplicated information
from the other structs. Why not add the regulator and regulator_nb to
the main struct? OK, not all io_pads have a regulator but you are only
saving one pointer.

> +
> +struct tegra_io_pads_info {
> +	struct device *dev;
> +	struct pinctrl_dev *pctl;
> +	struct tegra_io_pads_regulator_info *rinfo;
> +	const struct tegra_io_pad_soc_data *soc_data;
> +};
> +
> +static int tegra_iop_pinctrl_get_groups_count(struct pinctrl_dev *pctldev)
> +{
> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +
> +	return tiopi->soc_data->num_pads_cfg;
> +}
> +
> +static const char *tegra_iop_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->pads_cfg[group].name;
> +}
> +
> +static int tegra_iop_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->pads_cfg[group].pins;
> +	*num_pins = 1;
> +
> +	return 0;
> +}
> +
> +static const struct pinctrl_ops tegra_iop_pinctrl_ops = {
> +	.get_groups_count	= tegra_iop_pinctrl_get_groups_count,
> +	.get_group_name		= tegra_iop_pinctrl_get_group_name,
> +	.get_group_pins		= tegra_iop_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_pad_pinconf_get(struct pinctrl_dev *pctldev,
> +				    unsigned int pin, unsigned long *config)
> +{

Seems to be a mixture of tegra_iop/tegra_io_pad/tegra_io_pads between
various function names. Would be good to be consistent.

> +	struct tegra_io_pads_info *tiopi = pinctrl_dev_get_drvdata(pctldev);
> +	int param = pinconf_to_config_param(*config);
> +	const struct tegra_io_pads_cfg_info *pads_cfg =
> +					&tiopi->soc_data->pads_cfg[pin];
> +	enum tegra_io_pad pad_id = pads_cfg->pad_id;
> +	int arg = 0;

Nit-pick, pad_id and arg seem unnecessary.

> +	int ret;
> +
> +	switch (param) {
> +	case PIN_CONFIG_LOW_POWER_MODE:
> +		ret = tegra_io_pad_power_get_status(pad_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_pad_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_info *pads_cfg =
> +					&tiopi->soc_data->pads_cfg[pin];
> +	int pad_id = pads_cfg->pad_id;

Unnecessary variable?

> +	u16 param_val;
> +	int param;
> +	int ret;
> +	int i;

Nit-pick, can't the above be on one line?

> +
> +	for (i = 0; i < num_configs; i++) {
> +		param = pinconf_to_config_param(configs[i]);
> +		param_val = pinconf_to_config_argument(configs[i]);
> +
> +		switch (param) {
> +		case PIN_CONFIG_LOW_POWER_MODE:
> +			if (param_val)
> +				ret = tegra_io_pad_power_disable(pad_id);
> +			else
> +				ret = tegra_io_pad_power_enable(pad_id);
> +			if (ret < 0) {
> +				dev_err(tiopi->dev,
> +					"Failed to set DPD %d of pin %u: %d\n",
> +					param_val, pin, 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_pad_pinconf_ops = {
> +	.pin_config_get = tegra_io_pad_pinconf_get,
> +	.pin_config_set = tegra_io_pad_pinconf_set,
> +};
> +
> +static struct pinctrl_desc tegra_iop_pinctrl_desc = {
> +	.name = "pinctrl-tegra-io-pads",
> +	.pctlops = &tegra_iop_pinctrl_ops,
> +	.confops = &tegra_io_pad_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, old_uv;
> +	enum tegra_io_pad_voltage io_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 ((vdata->old_uV > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
> +		    (vdata->min_uV <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT))
> +			break;

The data-sheet for Tegra210 only lists 1.8V or 3.3V as supported
options. Do we need to support a range? Or does the h/w support a range
of voltages? I am just wondering why we cannot check explicitly for 1.8V
or 3.3V and treat anything else as an error.

> +
> +		ret = tegra_io_pad_set_voltage(rinfo->pads_cfg->pad_id,
> +					       TEGRA_IO_PAD_3300000UV);
> +		if (ret < 0) {
> +			dev_err(rinfo->dev,
> +				"Failed to set voltage %lu of pad %s: %d\n",
> +				vdata->min_uV, rinfo->pads_cfg->name, ret);
> +			return ret;
> +		}
> +		break;
> +
> +	case REGULATOR_EVENT_VOLTAGE_CHANGE:
> +		io_volt_uv = (unsigned long)data;
> +		ret = tegra_io_pad_get_voltage(rinfo->pads_cfg->pad_id);
> +		if (ret < 0) {
> +			dev_err(rinfo->dev, "Failed to get IO pad voltage: %d\n",
> +				ret);
> +			return ret;
> +		}
> +		old_uv = (ret == TEGRA_IO_PAD_1800000UV) ? 1800000 : 3300000;
> +		if (((io_volt_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
> +		      (old_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT)) ||
> +		       ((io_volt_uv > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
> +			 (old_uv > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT)))
> +			break;

Macro or sub-function? It is hard to read.

> +
> +		ret = tegra_io_pad_set_voltage(rinfo->pads_cfg->pad_id,
> +					       TEGRA_IO_PAD_1800000UV);
> +		if (ret < 0) {
> +			dev_err(rinfo->dev,
> +				"Failed to set voltage %lu of pad %s: %d\n",
> +				vdata->min_uV, rinfo->pads_cfg->name, ret);
> +			return ret;
> +		}
> +		break;
> +
> +	case REGULATOR_EVENT_ABORT_VOLTAGE_CHANGE:
> +		io_volt_uv = (unsigned long)data;
> +		io_volt = (io_volt_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) ?
> +			   TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV;

Macro? I believe this is also used in another place.

> +		ret = tegra_io_pad_set_voltage(rinfo->pads_cfg->pad_id,
> +					       io_volt);
> +		if (ret < 0) {
> +			dev_err(rinfo->dev,
> +				"Failed to set voltage %lu of pad %s: %d\n",
> +				io_volt_uv, rinfo->pads_cfg->name, ret);
> +			return ret;
> +		}
> +		break;
> +
> +	default:
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int tegra_iop_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_pad_soc_data *soc_data;
> +	struct device_node *np_parent = pdev->dev.parent->of_node;

I would get rid of this variable and set pdev->dev.of_node right before
testing it is valid.

> +	struct tegra_io_pads_info *tiopi;
> +	int ret, i;
> +
> +	if (!np_parent) {
> +		dev_err(dev, "PMC should be register from DT\n");
> +		return -ENODEV;
> +	}
> +
> +	soc_data = (const struct tegra_io_pad_soc_data *)id->driver_data;
> +
> +	tiopi = devm_kzalloc(dev, sizeof(*tiopi), GFP_KERNEL);
> +	if (!tiopi)
> +		return -ENOMEM;
> +
> +	tiopi->rinfo = devm_kzalloc(dev, sizeof(*tiopi->rinfo) *
> +				    soc_data->num_pads_cfg, GFP_KERNEL);
> +	if (!tiopi->rinfo)
> +		return -ENOMEM;
> +
> +	tiopi->dev = &pdev->dev;
> +	pdev->dev.of_node = np_parent;
> +	tiopi->soc_data = soc_data;
> +
> +	for (i = 0; i < soc_data->num_pads_cfg; ++i) {
> +		struct tegra_io_pads_regulator_info *rinfo = tiopi->rinfo + i;
> +		const struct tegra_io_pads_cfg_info *pads_cfg =
> +							&soc_data->pads_cfg[i];

Is this variable necessary? Why not set rinfo->pads_cfg directly from
soc_data?

> +		struct regulator *regulator;
> +		int io_volt_uv;
> +		enum tegra_io_pad_voltage io_volt;
> +
> +		if (!pads_cfg->vsupply)
> +			continue;
> +
> +		regulator = devm_regulator_get_optional(dev, pads_cfg->vsupply);
> +		if (IS_ERR(regulator)) {
> +			ret = PTR_ERR(regulator);
> +			if (ret == -EPROBE_DEFER)
> +				return ret;
> +			continue;
> +		}
> +
> +		io_volt_uv = regulator_get_voltage(regulator);
> +		if (io_volt_uv < 0) {
> +			dev_err(dev, "Failed to get voltage for rail %s: %d\n",
> +				pads_cfg->vsupply, io_volt_uv);
> +			return ret;
> +		}
> +
> +		io_volt = (io_volt_uv <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) ?
> +			   TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV;

Macro?

> +		ret = tegra_io_pad_set_voltage(pads_cfg->pad_id, io_volt);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to set voltage %d of pad %s: %d\n",
> +				io_volt_uv, pads_cfg->name, ret);
> +			return ret;
> +		}
> +		rinfo->dev = tiopi->dev;
> +		rinfo->regulator = regulator;
> +		rinfo->pads_cfg = pads_cfg;
> +
> +		rinfo->regulator_nb.notifier_call =
> +					tegra_io_pads_rail_change_notify_cb;
> +		ret = devm_regulator_register_notifier(regulator,
> +						       &rinfo->regulator_nb);
> +		if (ret < 0) {
> +			dev_err(dev, "Failed to register regulator %s notifier: %d\n",
> +				pads_cfg->name, ret);
> +			return ret;
> +		}
> +	}
> +
> +	tegra_iop_pinctrl_desc.pins = tiopi->soc_data->pins_desc;
> +	tegra_iop_pinctrl_desc.npins = tiopi->soc_data->num_pins_desc;
> +	platform_set_drvdata(pdev, tiopi);
> +
> +	tiopi->pctl = devm_pinctrl_register(dev, &tegra_iop_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(_id, _name, _pad_id, _lpstate, _vsupply)	\
> +	{								\
> +		.name = _name,						\

Do we need to store 'name' in this struct as well seeing as it is
already in the pins_desc?

Cheers
Jon

-- 
nvpublic

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

[Jon Hunter]

On 09/11/16 13:06, 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.
> 
> 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:
>  The DT binding document is modified to explain the regulator handle
>  for different IOs and how can it be passed from the DT.
> ---
>  .../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..6ca961f
> --- /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 voltage
> +of the 1.8V and 3.3V of IO voltage from power rail sources.

The last sentence is a bit unclear and does not sound correct. I am not
sure if you are missing the word 'range' somewhere or if you are trying
to say it must be either 1.8V or 3.3V. Looks like you have the same
sentence on the changelog too.

Cheers
Jon

-- 
nvpublic

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

[kbuild test robot]

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

Hi Laxman,

[auto build test ERROR on tegra/for-next]
[also build test ERROR on v4.9-rc6 next-20161117]
[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-support-for-IO-pad-control/20161109-215733
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_pad_pinconf_get':
>> drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:113:9: error: implicit declaration of function 'tegra_io_pad_power_get_status' [-Werror=implicit-function-declaration]
      ret = tegra_io_pad_power_get_status(pad_id);
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   cc1: some warnings being treated as errors

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

   107		enum tegra_io_pad pad_id = pads_cfg->pad_id;
   108		int arg = 0;
   109		int ret;
   110	
   111		switch (param) {
   112		case PIN_CONFIG_LOW_POWER_MODE:
 > 113			ret = tegra_io_pad_power_get_status(pad_id);
   114			if (ret < 0)
   115				return ret;
   116			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: 59433 bytes --]

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

[Laxman Dewangan]

On Wednesday 16 November 2016 12:18 AM, Jon Hunter wrote:
> On 09/11/16 13:06, Laxman Dewangan wrote:
>> +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 voltage
>> +of the 1.8V and 3.3V of IO voltage from power rail sources.
> The last sentence is a bit unclear and does not sound correct. I am not
> sure if you are missing the word 'range' somewhere or if you are trying
> to say it must be either 1.8V or 3.3V. Looks like you have the same
> sentence on the changelog too.
The IO pads are designed to work in two different voltage rail 1.8V 
(nominal) and 3.3V (nominal) for interfacing.
However, the tolerances of the IO pads are:

1.8 V nominal is (1.62V, 1.98V)
3.3 V nominal is (2.97V,3.63V)

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

[Laxman Dewangan]

Hi Jon,
I will update the patch per your comment.
Here is answer for some of the query.

Thanks,
Laxman


On Tuesday 15 November 2016 08:37 PM, Jon Hunter wrote:
> On 09/11/16 13:06, Laxman Dewangan wrote:
>> +/**
>> + * Macro for 1.8V, keep 200mV as tolerance for deciding that
>> + * IO pads should be set for 3.3V (high voltage) or 1.8V.
>> + */
>> +#define TEGRA_IO_PAD_1800000UV_UPPER_LIMIT 2000000
> Is there a reference we could add for the source of this information?

I had a discussion with the ASIC on this and as per them
     1.8 V nominal is (1.62V, 1.98V)
     3.3 V nominal is (2.97V,3.63V)

I am working with them to update the TRM document but we can assume that 
this information will be there in TRM.

>> +	const struct pinctrl_pin_desc *pins_desc;
>> +	int num_pins_desc;
>> +};
>> +
>> +struct tegra_io_pads_regulator_info {
>> +	struct device *dev;
>> +	const struct tegra_io_pads_cfg_info *pads_cfg;
>> +	struct regulator *regulator;
>> +	struct notifier_block regulator_nb;
>> +};
> Is this struct necessary? Seems to be a lot of duplicated information
> from the other structs. Why not add the regulator and regulator_nb to
> the main struct? OK, not all io_pads have a regulator but you are only
> saving one pointer.
Yes, some of IO pads support multi-voltage.


>
> +		if ((vdata->old_uV > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
> +		    (vdata->min_uV <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT))
> +			break;
> The data-sheet for Tegra210 only lists 1.8V or 3.3V as supported
> options. Do we need to support a range? Or does the h/w support a range
> of voltages? I am just wondering why we cannot check explicitly for 1.8V
> or 3.3V and treat anything else as an error.

Two voltage level, not range.

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

[Jon Hunter]

Hi Laxman,

On 21/11/16 09:36, Laxman Dewangan wrote:
> 
> Hi Jon,
> I will update the patch per your comment.

Thanks.

> Here is answer for some of the query.
> 
> Thanks,
> Laxman
> 
> 
> On Tuesday 15 November 2016 08:37 PM, Jon Hunter wrote:
>> On 09/11/16 13:06, Laxman Dewangan wrote:
>>> +/**
>>> + * Macro for 1.8V, keep 200mV as tolerance for deciding that
>>> + * IO pads should be set for 3.3V (high voltage) or 1.8V.
>>> + */
>>> +#define TEGRA_IO_PAD_1800000UV_UPPER_LIMIT 2000000
>> Is there a reference we could add for the source of this information?
> 
> I had a discussion with the ASIC on this and as per them
>     1.8 V nominal is (1.62V, 1.98V)
>     3.3 V nominal is (2.97V,3.63V)
> 
> I am working with them to update the TRM document but we can assume that
> this information will be there in TRM.

My feeling is that if all use-cases today are using either 1.8V or 3.3V,
then may be we should not worry about this and only support either 1.8V
or 3.3V. I would be more in favour of supporting other voltages if there
is a real need.

>>> +    const struct pinctrl_pin_desc *pins_desc;
>>> +    int num_pins_desc;
>>> +};
>>> +
>>> +struct tegra_io_pads_regulator_info {
>>> +    struct device *dev;
>>> +    const struct tegra_io_pads_cfg_info *pads_cfg;
>>> +    struct regulator *regulator;
>>> +    struct notifier_block regulator_nb;
>>> +};
>> Is this struct necessary? Seems to be a lot of duplicated information
>> from the other structs. Why not add the regulator and regulator_nb to
>> the main struct? OK, not all io_pads have a regulator but you are only
>> saving one pointer.
> Yes, some of IO pads support multi-voltage.

Yes, but I am saying why not put this information in the main struct and
not bother having yet another struct where half of the information is
duplicated.

>>
>> +        if ((vdata->old_uV > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
>> +            (vdata->min_uV <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT))
>> +            break;
>> The data-sheet for Tegra210 only lists 1.8V or 3.3V as supported
>> options. Do we need to support a range? Or does the h/w support a range
>> of voltages? I am just wondering why we cannot check explicitly for 1.8V
>> or 3.3V and treat anything else as an error.
> 
> Two voltage level, not range.

Ok, then I think it would be much simpler if we just support the
voltages we are using today.

Cheers
Jon

-- 
nvpublic

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

[Laxman Dewangan]

On Monday 21 November 2016 04:38 PM, Jon Hunter wrote:
>>
>> I had a discussion with the ASIC on this and as per them
>>      1.8 V nominal is (1.62V, 1.98V)
>>      3.3 V nominal is (2.97V,3.63V)
>>
>> I am working with them to update the TRM document but we can assume that
>> this information will be there in TRM.
> My feeling is that if all use-cases today are using either 1.8V or 3.3V,
> then may be we should not worry about this and only support either 1.8V
> or 3.3V. I would be more in favour of supporting other voltages if there
> is a real need.

Sometimes, the regulator will not return exact 1.8V or 3.3V due to the 
PMIC rail resolution. On such cases, it returns nearest voltage to 1.8V 
or 3.3V.
That's why the PMIC resolution is considered through IO pad voltage 
tolerances.


>
>>>> +    const struct pinctrl_pin_desc *pins_desc;
>>>> +    int num_pins_desc;
>>>> +};
>>>> +
>>>> +struct tegra_io_pads_regulator_info {
>>>> +    struct device *dev;
>>>> +    const struct tegra_io_pads_cfg_info *pads_cfg;
>>>> +    struct regulator *regulator;
>>>> +    struct notifier_block regulator_nb;
>>>> +};
>>> Is this struct necessary? Seems to be a lot of duplicated information
>>> from the other structs. Why not add the regulator and regulator_nb to
>>> the main struct? OK, not all io_pads have a regulator but you are only
>>> saving one pointer.
>> Yes, some of IO pads support multi-voltage.
> Yes, but I am saying why not put this information in the main struct and
> not bother having yet another struct where half of the information is
> duplicated.

For regulator notifier callback, we will need the driver data. If I keep 
this in the main structure then I will not able to get proper structure 
until I make that as global.

The notifier registration is
                 ret = devm_regulator_register_notifier(regulator,
&rinfo->regulator_nb);


and from the pointer of rinfo->regulator_nb, I will get the rinfo as

         rinfo = container_of(nb, struct tegra_io_pads_regulator_info,
                                      regulator_nb);

if I use this in main structure then I will not be able to get the 
driver data.


>
>>> +        if ((vdata->old_uV > TEGRA_IO_PAD_1800000UV_UPPER_LIMIT) &&
>>> +            (vdata->min_uV <= TEGRA_IO_PAD_1800000UV_UPPER_LIMIT))
>>> +            break;
>>> The data-sheet for Tegra210 only lists 1.8V or 3.3V as supported
>>> options. Do we need to support a range? Or does the h/w support a range
>>> of voltages? I am just wondering why we cannot check explicitly for 1.8V
>>> or 3.3V and treat anything else as an error.
>> Two voltage level, not range.
> Ok, then I think it would be much simpler if we just support the
> voltages we are using today.
Regulator resolution is only reason here to use tolerance.

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

[Jon Hunter]

On 21/11/16 12:49, Laxman Dewangan wrote:
> 
> On Monday 21 November 2016 04:38 PM, Jon Hunter wrote:
>>>
>>> I had a discussion with the ASIC on this and as per them
>>>      1.8 V nominal is (1.62V, 1.98V)
>>>      3.3 V nominal is (2.97V,3.63V)
>>>
>>> I am working with them to update the TRM document but we can assume that
>>> this information will be there in TRM.
>> My feeling is that if all use-cases today are using either 1.8V or 3.3V,
>> then may be we should not worry about this and only support either 1.8V
>> or 3.3V. I would be more in favour of supporting other voltages if there
>> is a real need.
> 
> Sometimes, the regulator will not return exact 1.8V or 3.3V due to the
> PMIC rail resolution. On such cases, it returns nearest voltage to 1.8V
> or 3.3V.
> That's why the PMIC resolution is considered through IO pad voltage
> tolerances.

Ok, gotcha. I can see that would be the case for when you call
regulator_get_voltage() (ie. in the probe), but what about the notifier?
I imagine that in the notifier, at least for the pre-change case, that
the voltage is the target and not the actual. So I am wondering if we
need to check for a range in the notifier?

>>>>> +    const struct pinctrl_pin_desc *pins_desc;
>>>>> +    int num_pins_desc;
>>>>> +};
>>>>> +
>>>>> +struct tegra_io_pads_regulator_info {
>>>>> +    struct device *dev;
>>>>> +    const struct tegra_io_pads_cfg_info *pads_cfg;
>>>>> +    struct regulator *regulator;
>>>>> +    struct notifier_block regulator_nb;
>>>>> +};
>>>> Is this struct necessary? Seems to be a lot of duplicated information
>>>> from the other structs. Why not add the regulator and regulator_nb to
>>>> the main struct? OK, not all io_pads have a regulator but you are only
>>>> saving one pointer.
>>> Yes, some of IO pads support multi-voltage.
>> Yes, but I am saying why not put this information in the main struct and
>> not bother having yet another struct where half of the information is
>> duplicated.
> 
> For regulator notifier callback, we will need the driver data. If I keep
> this in the main structure then I will not able to get proper structure
> until I make that as global.
> 
> The notifier registration is
>                 ret = devm_regulator_register_notifier(regulator,
> &rinfo->regulator_nb);
> 
> 
> and from the pointer of rinfo->regulator_nb, I will get the rinfo as
> 
>         rinfo = container_of(nb, struct tegra_io_pads_regulator_info,
>                                      regulator_nb);
> 
> if I use this in main structure then I will not be able to get the
> driver data.

Ah yes, you have an array of regulators. Make sense. However, shame to
duplicate some of this data and also would be good to avoid allocating
so much memory in probe for these structs if only a few rails actually
have regulators ...

  tiopi->rinfo = devm_kzalloc(dev, sizeof(*tiopi->rinfo) *
				soc_data->num_pads_cfg, GFP_KERNEL);

Cheers
Jon

-- 
nvpublic

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

[Jon Hunter]

On 09/11/16 13:06, 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@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.
> ---
>  drivers/pinctrl/tegra/Kconfig                |  12 +
>  drivers/pinctrl/tegra/Makefile               |   1 +
>  drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c | 488 +++++++++++++++++++++++++++
>  3 files changed, 501 insertions(+)
>  create mode 100644 drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c

...

> +#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)

Can you also fix these checkpatch errors ...

ERROR: Macros with complex values should be enclosed in parentheses
#424: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:350:

ERROR: Macros with complex values should be enclosed in parentheses
#456: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:382:

Cheers
Jon

-- 
nvpublic

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

[Laxman Dewangan]

On Tuesday 22 November 2016 02:07 AM, Jon Hunter wrote:
> On 21/11/16 12:49, Laxman Dewangan wrote:
>> On Monday 21 November 2016 04:38 PM, Jon Hunter wrote:
>>>> I had a discussion with the ASIC on this and as per them
>>>>       1.8 V nominal is (1.62V, 1.98V)
>>>>       3.3 V nominal is (2.97V,3.63V)
>>>>
>>>> I am working with them to update the TRM document but we can assume that
>>>> this information will be there in TRM.
>>> My feeling is that if all use-cases today are using either 1.8V or 3.3V,
>>> then may be we should not worry about this and only support either 1.8V
>>> or 3.3V. I would be more in favour of supporting other voltages if there
>>> is a real need.
>> Sometimes, the regulator will not return exact 1.8V or 3.3V due to the
>> PMIC rail resolution. On such cases, it returns nearest voltage to 1.8V
>> or 3.3V.
>> That's why the PMIC resolution is considered through IO pad voltage
>> tolerances.
> Ok, gotcha. I can see that would be the case for when you call
> regulator_get_voltage() (ie. in the probe), but what about the notifier?
> I imagine that in the notifier, at least for the pre-change case, that
> the voltage is the target and not the actual. So I am wondering if we
> need to check for a range in the notifier?

I think I can use the fixed voltage 1.8V and 3.3V as the current PMIC 
does not have this issue.

If we come for some tolerance issue in future then we will review this part.
This way it will be simple for now to go ahead with simple implementation.
I am going to validate the IO rail voltage as 1800mV and 3300mV.


>>>>>> +    const struct pinctrl_pin_desc *pins_desc;
>>>>>> +    int num_pins_desc;
>>>>>> +};
>>>>>> +
>>>>>> +struct tegra_io_pads_regulator_info {
>>>>>> +    struct device *dev;
>>>>>> +    const struct tegra_io_pads_cfg_info *pads_cfg;
>>>>>> +    struct regulator *regulator;
>>>>>> +    struct notifier_block regulator_nb;
>>>>>> +};
>>>>> Is this struct necessary? Seems to be a lot of duplicated information
>>>>> from the other structs. Why not add the regulator and regulator_nb to
>>>>> the main struct? OK, not all io_pads have a regulator but you are only
>>>>> saving one pointer.
>>>> Yes, some of IO pads support multi-voltage.
>>> Yes, but I am saying why not put this information in the main struct and
>>> not bother having yet another struct where half of the information is
>>> duplicated.
>> For regulator notifier callback, we will need the driver data. If I keep
>> this in the main structure then I will not able to get proper structure
>> until I make that as global.
>>
>> The notifier registration is
>>                  ret = devm_regulator_register_notifier(regulator,
>> &rinfo->regulator_nb);
>>
>>
>> and from the pointer of rinfo->regulator_nb, I will get the rinfo as
>>
>>          rinfo = container_of(nb, struct tegra_io_pads_regulator_info,
>>                                       regulator_nb);
>>
>> if I use this in main structure then I will not be able to get the
>> driver data.
> Ah yes, you have an array of regulators. Make sense. However, shame to
> duplicate some of this data and also would be good to avoid allocating
> so much memory in probe for these structs if only a few rails actually
> have regulators ...
>
>    tiopi->rinfo = devm_kzalloc(dev, sizeof(*tiopi->rinfo) *
> 				soc_data->num_pads_cfg, GFP_KERNEL);
>
Then allocate inside loop when supply found.
This way we can optimise the size of allocation on cast of the multiple 
memory allocation.

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

[Laxman Dewangan]

On Tuesday 22 November 2016 02:31 AM, Jon Hunter wrote:
> On 09/11/16 13:06, Laxman Dewangan wrote:
> +	_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)
> Can you also fix these checkpatch errors ...
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #424: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:350:
>
> ERROR: Macros with complex values should be enclosed in parentheses
> #456: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:382:

I can fix this but will still have the error as:

CHECK: Macro argument reuse '_entry_' - possible side-effects?
#425: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:425:


And there is no better way to fix this.

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

[Laxman Dewangan]

On Tuesday 22 November 2016 03:15 PM, Joe Perches wrote:
> On Tue, 2016-11-22 at 13:45 +0530, Laxman Dewangan wrote:
>> On Tuesday 22 November 2016 02:31 AM, Jon Hunter wrote:
>>> On 09/11/16 13:06, Laxman Dewangan wrote:
>>> +	_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)
>>> Can you also fix these checkpatch errors ...
>>>
>>> ERROR: Macros with complex values should be enclosed in parentheses
>>> #424: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:350:
>>>
>>> ERROR: Macros with complex values should be enclosed in parentheses
>>> #456: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:382:
>> I can fix this but will still have the error as:
>>
>> CHECK: Macro argument reuse '_entry_' - possible side-effects?
>> #425: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:425:
>>
>>
>> And there is no better way to fix this.
> It's a stupid little script, feel free to ignore it.
And when I tried to add the parenthesis, it failed in compilation.
So in this case, we just need to ignore the error of

Macros with complex values should be enclosed in parentheses

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

[Joe Perches]

On Tue, 2016-11-22 at 13:45 +0530, Laxman Dewangan wrote:
> On Tuesday 22 November 2016 02:31 AM, Jon Hunter wrote:
> > On 09/11/16 13:06, Laxman Dewangan wrote:
> > +	_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)
> > Can you also fix these checkpatch errors ...
> > 
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #424: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:350:
> > 
> > ERROR: Macros with complex values should be enclosed in parentheses
> > #456: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:382:
> 
> I can fix this but will still have the error as:
> 
> CHECK: Macro argument reuse '_entry_' - possible side-effects?
> #425: FILE: drivers/pinctrl/tegra/pinctrl-tegra-io-pad.c:425:
> 
> 
> And there is no better way to fix this.

It's a stupid little script, feel free to ignore it.