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

[Jon Hunter]

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

...

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

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

Cheers
Jon

-- 
nvpublic