(Also CC'ing in the DT bindings maintainers, hence quoting all of the
binding.)
On 07/26/2013 04:15 AM, Laxman Dewangan wrote:
That field looks odd. From what I can tell in the code, the location of
the pull-up/down/... registers/bits varies depending on which mux
function is selected for a particular pin? That's a very strange HW
design. Are you sure the set of pins/functions this driver exposes is
the correct way to represent the HW?
+ unsigned mux_reg_base;This isn't an issue with this patch, but more with the way palmas_read()
+ unsigned mux_reg_add;
works. Here, the MFD component is telling the MFD top-level object where
the MFD component exists within the top-level address space. That's
backwards. The top-level MFD is what know how the components are put
together to create the whole particular chip, and it's entirely possible
this could vary chip-to-chip. :-(
+ for (i = 0; i < ARRAY_SIZE(g->opt); i++) {So, when I create the Tegra bindings, I created the list of mux
+ if (g->opt[i]->mux_opt == function)
+ break;
+ }
functions by looking at the logical meaning of each register value
(0..3) for each pin, and then made the list of functions have a value
for each logical meaning. This requires a mapping table between the
pinctrl subsystem's mux function values and the HW mux function values,
which is what the loop above implements. Instead, if might be simpler to
just have functions named "0", "1", "2", ... and have all pins support
those functions. This simplifies the driver, and the DT bindings.
Whether doing so would make the DT bindings better probably depends on
exactly how the HW's documentation is written...
+static int palmas_pinconf_get(struct pinctrl_dev *pctldev,Are the pin configuration values applied per pin in HW? If so, you
+ unsigned pin, unsigned long *config)
+{
+ dev_err(pctldev->dev, "pin_config_get op not supported\n");
+ return -ENOTSUPP;
+}
should probably implement palmas_pinconf_get/set() rather than
palmas_pinconf_group_get/set(). You'd also need to change the DT->map
conversion function to use PIN_MAP_TYPE_CONFIGS_PIN rather than
PIN_MAP_TYPE_CONFIGS_GROUP.