Thanks Thierry for review.
On Friday 25 November 2016 03:27 PM, Thierry Reding wrote:
* PGP Signed by an unknown key
On Thu, Nov 24, 2016 at 02:08:54PM +0530, Laxman Dewangan wrote:
+ NVIDIA Tegra124/210 SoC has IO pads which supports multi-voltage
+ level of interfacing and deep power down mode of IO pads. The
+ voltage of IO pads are SW configurable based on IO rail of that
+ pads on T210. This driver provides the interface to change IO pad
+ voltage and power state via pincontrol interface.
This has a lot of chip-specific text. Will all of that have to be
updated if support for new chips is added?
Then saying that Tegra124 and later..
Hoping, people know our chip releasing sequence as numbering are not in
sequence.
+#include <linux/regulator/consumer.h>
+#include <soc/tegra/pmc.h>
Have you considered moving this code into the PMC driver? It seems a
little over the top to go through all of the platform device creation
and driver registration dance only to call into a public API later on.
Yes, we had discussion on this and suggestion came to use the pinctrl
framework.
If we do in the pmc driver then we will need lots of DT processing for
getting information from DT which we can directly get from the pinctrl
core framework.
Also client driver may need to have the control dynamically and get the
IO pads from DT. So implementing all in pmc will be huge duplication
over already existing framework.
+#include "../core.h"
+#include "../pinconf.h"
+#include "../pinctrl-utils.h"
+
+#define TEGRA_IO_RAIL_1800000UV 1800000
+#define TEGRA_IO_RAIL_3300000UV 3300000
Is there really a point in having these defines? They are really long
and effectively don't carry more information than just the plain
numbers.
using macros is always good instead of magic number in code and hence it
is there.
+#define tegra_io_uv_to_io_pads_uv(io_uv) \
+ (((io_uv) == TEGRA_IO_RAIL_1800000UV) ? \
+ TEGRA_IO_PAD_1800000UV : TEGRA_IO_PAD_3300000UV)
+
+#define tegra_io_voltage_is_valid(io_uv) \
+ ({ typeof(io_uv) io_uv_ = (io_uv); \
+ ((io_uv_ == TEGRA_IO_RAIL_1800000UV) || \
+ (io_uv_ == TEGRA_IO_RAIL_3300000UV)); })
Maybe make both of these static inline functions to improve readability?
I find the above very hard to read.
OK, will convert to the inline but dont think it will be less complex.
+ enum tegra_io_pad_voltage pad_volt;
+
+
+#define TEGRA_IO_PAD_INFO(_pin, _name, _id, _lpstate, _vsupply) \
+ { \
+ .name = _name, \
+ .pins = {(_pin)}, \
+ .id = TEGRA_IO_PAD_##_id, \
+ .vsupply = (_vsupply), \
+ .supports_low_power = (_lpstate), \
+ }
+
+static const struct tegra_io_pads_cfg tegra124_io_pads_cfg_info[] = {
+ TEGRA124_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
+
+static const struct tegra_io_pads_cfg tegra210_io_pads_cfg_info[] = {
+ TEGRA210_PAD_INFO_TABLE(TEGRA_IO_PAD_INFO),
+};
That's a weird way of writing these tables. Why not do something simpler
and much more common such as:
#define TEGRA_IO_PAD_INFO(...) ...
static const struct tegra_io_pads_cfg tegra124_io_pads_cfgs[] = {
TEGRA_IO_PAD_INFO(...),
...
};
static const struct tegra_io_pads_cfg tegra210_io_pad_cfgs[] = {
TEGRA_IO_PAD_INFO(...),
...
};
This is done to use the same table for initialing two different
structure. If we go as above then we will endup with the two tables.
or define and then redefine the TEGRA_IO_PAD_INFO.
+
+
+module_platform_driver(tegra_io_pads_pinctrl_driver);
+
+MODULE_DESCRIPTION("NVIDIA TEGRA IO pad Control Driver");
+MODULE_AUTHOR("Laxman Dewangan <ldewangan@xxxxxxxxxx>");
+MODULE_LICENSE("GPL v2");
Like I said above, I think there's a lot of boilerplate in here that's
simply there to create a virtual device and bind a driver to it. All of
that comes with very little to no benefit. I think this could all just
be moved into the PMC driver and be simplified quite a bit.
What did you not like here?
Let me specific to my query to get idea about your view:
1. Do you agree for the interface as pinctrl here?
2. Do you want to call the probe() function implemented here as simple
function call from pmc's probe?
3. If (2) is yes then how do you want to differentiate T124 or T210? Via
argument?
It will be really help if you can provide the pseudo code from PMC and
this driver so that I can implement the same in the next patch.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
