Message-ID: <20161125095744.GB11512@ulmo.ba.sec>
Date:   Fri, 25 Nov 2016 10:57:44 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Laxman Dewangan <ldewangan@...dia.com>
Cc:     linus.walleij@...aro.org, robh+dt@...nel.org, mark.rutland@....com,
        swarren@...dotorg.org, gnurou@...il.com, jonathanh@...dia.com,
        joe@...ches.com, yamada.masahiro@...ionext.com,
        linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH V4 2/2] pinctrl: tegra: Add driver to configure voltage
 and power of io pads

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@...dia.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.

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@...dia.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.

> +#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@...dia.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.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (802 bytes)

Powered by blists - more mailing lists