use pinctrl-single in arch mmp

Marvell's PXA/MMP silicon also match the behavior of pinctrl-single. Each pin binds to one register. A lot of pins could be configured as gpio.

GPIO range is defined as a child node of pinmux in .dtsi file. If those pins are with the same gpio function configuration in the pinmux register, they could be defined in the same GPIO range. For this new child node, two properties are used.

reg = <the start of pinmux register in range, size of range>

pinctrl-single,gpio: <gpio base in range, the gpio function of the range
in the pinmux register>

Signed-off-by Haojian Zhuang <[email protected]>

Marvell's PXA/MMP silicon also match the behavior of pinctrl-single. Each pin binds to one register. A lot of pins could be configured as gpio.

GPIO range is defined as a child node of pinmux in .dtsi file. If those pins are with the same gpio function configuration in the pinmux register, they could be defined in the same GPIO range. For this new child node, two properties are used.

reg = <the start of pinmux register in range, size of range>

pinctrl-single,gpio: <gpio base in range, the gpio function of the range
in the pinmux register>

Signed-off-by Haojian Zhuang <[email protected]>

Quoted 11 lines · Haojian Zhuang

include/linux/pinctrl/consumer.h +4 -0

@@ -36,6 +36,10 @@ extern struct pinctrl_state * __must_check pinctrl_lookup_state(
                                                      struct pinctrl *p,
                                                      const char *name);
 extern int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *s);
+extern const struct pinctrl_map * __must_check pinctrl_lookup_map(
+                                             struct pinctrl *p,
+                                             const char *name,
+                                             unsigned type);
 

include/linux/pinctrl/consumer.h extern struct pinctrl_state * __must_check pinctrl_lookup_state( · line 45

Quoted 2 lines

include/linux/pinctrl/consumer.h +0 -0

 extern struct pinctrl * __must_check devm_pinctrl_get(struct device *dev);
 extern void devm_pinctrl_put(struct pinctrl *p);

I think this should be in drivers/pinctrl/core.h, or is there a need for the consumer drivers to use this?

In Marvell PXA/MMP silicons, input schmitt disable value is 0x40, not 0. So append new config parameter -- input schmitt disable.

Signed-off-by Haojian Zhuang <[email protected]>

static int pcs_pinconf_get(struct pinctrl_dev *pctldev,

drivers/pinctrl/pinctrl-single.c +9 -1

 				unsigned pin, unsigned long *config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(*config);
+	union pcs_pinconf_argument_t arg;
+	u32 offset;
+	unsigned data;
+	int ret;
+
+	if (!pcs->conf.nconfs)
+		return -ENOTSUPP;

}

static int pcs_pinconf_set(struct pinctrl_dev *pctldev,

drivers/pinctrl/pinctrl-single.c +14 -1

 				unsigned pin, unsigned long config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param config_param = pinconf_to_config_param(config);
+	union pcs_pinconf_argument_t arg;
+	u32 offset;
+	unsigned data;
+	int ret;
+
+	if (!pcs->conf.nconfs)
+		return -ENOTSUPP;
+
+	ret = pcs_pinconf_get_config(pctldev, pin, config_param);
+	if (ret < 0)
+		return ret;
+

drivers/pinctrl/pinctrl-single.c +25 -3

@@ -765,12 +958,82 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 	if (res < 0)
 		goto free_function;
 
-	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
-	(*map)->data.mux.group = np->name;
-	(*map)->data.mux.function = np->name;
+	m->type = PIN_MAP_TYPE_MUX_GROUP;
+	m->data.mux.group = np->name;
+	m->data.mux.function = np->name;
+
+	if (!nconfs)
+		return 0;
+
+	conf = devm_kzalloc(pcs->dev, sizeof(*conf) * nconfs, GFP_KERNEL);
+	if (!conf) {
+		res = -ENOMEM;
+		goto free_pingroup;
+	}
+	i = 0;
+	if (!of_property_read_u32_array(np, "pinctrl-single,bias",
+					value, 5)) {
+		/* array: bias value, mask, disable, pull down, pull up */
+		data = value[0] & value[1];
+		arg.bits.shift = ffs(value[1]) - 1;
+		arg.bits.mask = value[1] >> arg.bits.shift;
+		if (pcs_config_match(data, value[2])) {
+			arg.bits.value = value[2] >> arg.bits.shift;
+			conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_DISABLE,
+						    arg.data);
+		}
+		if (pcs_config_match(data, value[3])) {

drivers/pinctrl/pinctrl-single.c +15 -0

+	if (!of_property_read_u32_array(np, "pinctrl-single,power-source",
+					value, 2)) {
+		/* array: power source value, mask */
+		data = value[0] & value[1];
+		arg.bits.shift = ffs(value[1]) - 1;
+		arg.bits.mask = value[1] >> arg.bits.shift;
+		arg.bits.value = data >> arg.bits.shift;
+		conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_POWER_SOURCE, arg.data);
+	}
+	if (!of_property_read_u32_array(np, "pinctrl-single,input-schmitt",
+					value, 3)) {
+		/* array: input schmitt value, mask, disable */
+		data = value[0] & value[1];
+		arg.bits.shift = ffs(value[1]) - 1;
+		arg.bits.mask = value[1] >> arg.bits.shift;

Hi,

Quoted 13 lines

static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
unsigned pin, unsigned long *config)
{

drivers/pinctrl/pinctrl-single.c +9 -1

-     return -ENOTSUPP;
+     struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+     enum pin_config_param config_param = pinconf_to_config_param(*config);
+     union pcs_pinconf_argument_t arg;
+     u32 offset;
+     unsigned data;
+     int ret;
+
+     if (!pcs->conf.nconfs)
+             return -ENOTSUPP;

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 548

Quoted 10 lines

drivers/pinctrl/pinctrl-single.c +10 -0

+
+     ret = pcs_pinconf_get_config(pctldev, pin, config_param);
+     if (ret < 0)
+             return ret;
+
+     offset = pin * (pcs->width / BITS_PER_BYTE);
+     data = pcs->read(pcs->base + offset);
+
+     arg.data = pinconf_to_config_argument(ret);
+     data = (data >> arg.bits.shift) & arg.bits.mask;

Shouldn't you just return data here?

        *config = data;

        return 0;

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 551

Quoted 3 lines

drivers/pinctrl/pinctrl-single.c +3 -0

+     arg.bits.value = data;
+     *config = pinconf_to_config_packed(config_param, arg.data);
+     return 0;

Instead of these? Or how else is the consumer driver supposed to use the value?

Quoted 25 lines

}

static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
unsigned pin, unsigned long config)
{

drivers/pinctrl/pinctrl-single.c +19 -1

-     return -ENOTSUPP;
+     struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+     enum pin_config_param config_param = pinconf_to_config_param(config);
+     union pcs_pinconf_argument_t arg;
+     u32 offset;
+     unsigned data;
+     int ret;
+
+     if (!pcs->conf.nconfs)
+             return -ENOTSUPP;
+
+     ret = pcs_pinconf_get_config(pctldev, pin, config_param);
+     if (ret < 0)
+             return ret;
+
+     offset = pin * (pcs->width / BITS_PER_BYTE);
+     data = pcs->read(pcs->base + offset);
+
+     arg.data = pinconf_to_config_argument(config);
+     data = (data >> arg.bits.shift) & arg.bits.mask;

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 580

Quoted 5 lines

drivers/pinctrl/pinctrl-single.c +4 -0

+     arg.bits.value = data;
+     data = pinconf_to_config_packed(config_param, arg.data);
+     pcs->write(data, pcs->base + offset);
+     return 0;
 }

I need to look at this more to understand how this actually relates to what gets written to the register with my test case :)

Quoted 33 lines

drivers/pinctrl/pinctrl-single.c +25 -3

@@ -765,12 +958,82 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
      if (res < 0)
              goto free_function;
 
-     (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
-     (*map)->data.mux.group = np->name;
-     (*map)->data.mux.function = np->name;
+     m->type = PIN_MAP_TYPE_MUX_GROUP;
+     m->data.mux.group = np->name;
+     m->data.mux.function = np->name;
+
+     if (!nconfs)
+             return 0;
+
+     conf = devm_kzalloc(pcs->dev, sizeof(*conf) * nconfs, GFP_KERNEL);
+     if (!conf) {
+             res = -ENOMEM;
+             goto free_pingroup;
+     }
+     i = 0;
+     if (!of_property_read_u32_array(np, "pinctrl-single,bias",
+                                     value, 5)) {
+             /* array: bias value, mask, disable, pull down, pull up */
+             data = value[0] & value[1];
+             arg.bits.shift = ffs(value[1]) - 1;
+             arg.bits.mask = value[1] >> arg.bits.shift;
+             if (pcs_config_match(data, value[2])) {
+                     arg.bits.value = value[2] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_DISABLE,
+                                                 arg.data);
+             }
+             if (pcs_config_match(data, value[3])) {

drivers/pinctrl/pinctrl-single.c static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, · line 995

Quoted 10 lines

drivers/pinctrl/pinctrl-single.c +10 -0

+                     arg.bits.value = value[3] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_DOWN,
+                                                 arg.data);
+             }
+             if (pcs_config_match(data, value[4])) {
+                     arg.bits.value = value[4] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_UP,
+                                                 arg.data);
+             }
+     }

Doesn't this mean you only get one of the above working? What happens if you initially need to set BIAS_DISABLE, but then the consumer driver wants to set BIAS_PULL_DOWN or something?

Maybe we need separate pinctrl-single,bias-disable, pinctrl-single,bias-pull-down and pinctrl-single,bias-pull-up?

Quoted 15 lines

drivers/pinctrl/pinctrl-single.c +15 -0

+     if (!of_property_read_u32_array(np, "pinctrl-single,power-source",
+                                     value, 2)) {
+             /* array: power source value, mask */
+             data = value[0] & value[1];
+             arg.bits.shift = ffs(value[1]) - 1;
+             arg.bits.mask = value[1] >> arg.bits.shift;
+             arg.bits.value = data >> arg.bits.shift;
+             conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_POWER_SOURCE, arg.data);
+     }
+     if (!of_property_read_u32_array(np, "pinctrl-single,input-schmitt",
+                                     value, 3)) {
+             /* array: input schmitt value, mask, disable */
+             data = value[0] & value[1];
+             arg.bits.shift = ffs(value[1]) - 1;
+             arg.bits.mask = value[1] >> arg.bits.shift;

drivers/pinctrl/pinctrl-single.c static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, · line 1020

Quoted 10 lines

drivers/pinctrl/pinctrl-single.c +10 -0

+             if (pcs_config_match(data, value[2])) {
+                     arg.bits.value = value[2] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_INPUT_SCHMITT_DISABLE,
+                                                 arg.data);
+             } else {
+                     arg.bits.value = data >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_INPUT_SCHMITT,
+                                                 arg.data);
+             }
+     }

And I think this has a similar problem? What if the consumer driver wants to set INPUT_SCHMITT with pin_config_set() and the initial value is SCHMITT_DISABLE? Doesn't the consumer driver get -ENOTSUPP in that case?

I'll look at this more early next week and try to get my testcase working with it.

Regards,

Tony

Quoted 28 lines · Tony Lindgren

Hi,

Quoted 13 lines

static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
unsigned pin, unsigned long *config)
{

drivers/pinctrl/pinctrl-single.c +9 -1

-     return -ENOTSUPP;
+     struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+     enum pin_config_param config_param = pinconf_to_config_param(*config);
+     union pcs_pinconf_argument_t arg;
+     u32 offset;
+     unsigned data;
+     int ret;
+
+     if (!pcs->conf.nconfs)
+             return -ENOTSUPP;

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 548

Quoted 10 lines

drivers/pinctrl/pinctrl-single.c +10 -0

+
+     ret = pcs_pinconf_get_config(pctldev, pin, config_param);
+     if (ret < 0)
+             return ret;
+
+     offset = pin * (pcs->width / BITS_PER_BYTE);
+     data = pcs->read(pcs->base + offset);
+
+     arg.data = pinconf_to_config_argument(ret);
+     data = (data >> arg.bits.shift) & arg.bits.mask;

Shouldn't you just return data here?

Quoted 4 lines

        *config = data;

        return 0;

Since we're using pinconf-generic, let's review how pinconf_generic_dump_pin() works.

pinconf_generic_dump_pin():
config = pinconf_to_config_packed(conf_items[i].param, 0); The lower 16-bit values are parameters. and the upper 16-bit
values are pre-set with 0 for argument.

ret = pin_config_get_for_pin(pctldev, pin, &config); It'll invoke pcs_pinconf_get() that we're taking care. I want
to return the value with both argument
and parameters. Since the argument may be any location in 32-bit register, I organize them into packed
structure. If I return the value field directly, it may conflict with the lower 16-bit config parameter.

        if (conf_items[i].format && pinconf_to_config_argument(config) != 0)
            seq_printf(s, " (%u %s)",

pinconf_to_config_argument(config), conf_items[i].format);
At here, it loads the upper 16-bit config argument. It also
means that returning value field directly
will break this.

So how about change it in below.
if (conf_items[i].format)
seq_printf(s, " (%u %s)", config, conf_items[i].format);

Quoted 7 lines

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 551

Quoted 3 lines

drivers/pinctrl/pinctrl-single.c +3 -0

+     arg.bits.value = data;
+     *config = pinconf_to_config_packed(config_param, arg.data);
+     return 0;

Instead of these? Or how else is the consumer driver supposed to use the value?

I agree that returning data directly would be easy for the usage in device driver. Let's define it as "data >> shift".
Is it OK?

Quoted 25 lines

Quoted 25 lines

}

static int pcs_pinconf_set(struct pinctrl_dev *pctldev,
unsigned pin, unsigned long config)
{

drivers/pinctrl/pinctrl-single.c +19 -1

-     return -ENOTSUPP;
+     struct pcs_device *pcs = pinctrl_dev_get_drvdata(pctldev);
+     enum pin_config_param config_param = pinconf_to_config_param(config);
+     union pcs_pinconf_argument_t arg;
+     u32 offset;
+     unsigned data;
+     int ret;
+
+     if (!pcs->conf.nconfs)
+             return -ENOTSUPP;
+
+     ret = pcs_pinconf_get_config(pctldev, pin, config_param);
+     if (ret < 0)
+             return ret;
+
+     offset = pin * (pcs->width / BITS_PER_BYTE);
+     data = pcs->read(pcs->base + offset);
+
+     arg.data = pinconf_to_config_argument(config);
+     data = (data >> arg.bits.shift) & arg.bits.mask;

Quoted 8 lines

drivers/pinctrl/pinctrl-single.c static struct pinmux_ops pcs_pinmux_ops = { · line 580

Quoted 5 lines

drivers/pinctrl/pinctrl-single.c +4 -0

+     arg.bits.value = data;
+     data = pinconf_to_config_packed(config_param, arg.data);
+     pcs->write(data, pcs->base + offset);
+     return 0;
 }

I need to look at this more to understand how this actually relates to what gets written to the register with my test case :)

No problem.

Quoted 48 lines

Quoted 33 lines

drivers/pinctrl/pinctrl-single.c +25 -3

@@ -765,12 +958,82 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
      if (res < 0)
              goto free_function;
 
-     (*map)->type = PIN_MAP_TYPE_MUX_GROUP;
-     (*map)->data.mux.group = np->name;
-     (*map)->data.mux.function = np->name;
+     m->type = PIN_MAP_TYPE_MUX_GROUP;
+     m->data.mux.group = np->name;
+     m->data.mux.function = np->name;
+
+     if (!nconfs)
+             return 0;
+
+     conf = devm_kzalloc(pcs->dev, sizeof(*conf) * nconfs, GFP_KERNEL);
+     if (!conf) {
+             res = -ENOMEM;
+             goto free_pingroup;
+     }
+     i = 0;
+     if (!of_property_read_u32_array(np, "pinctrl-single,bias",
+                                     value, 5)) {
+             /* array: bias value, mask, disable, pull down, pull up */
+             data = value[0] & value[1];
+             arg.bits.shift = ffs(value[1]) - 1;
+             arg.bits.mask = value[1] >> arg.bits.shift;
+             if (pcs_config_match(data, value[2])) {
+                     arg.bits.value = value[2] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_DISABLE,
+                                                 arg.data);
+             }
+             if (pcs_config_match(data, value[3])) {

drivers/pinctrl/pinctrl-single.c static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, · line 995

Quoted 10 lines

drivers/pinctrl/pinctrl-single.c +10 -0

+                     arg.bits.value = value[3] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_DOWN,
+                                                 arg.data);
+             }
+             if (pcs_config_match(data, value[4])) {
+                     arg.bits.value = value[4] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_BIAS_PULL_UP,
+                                                 arg.data);
+             }
+     }

Doesn't this mean you only get one of the above working? What happens if you initially need to set BIAS_DISABLE, but then the consumer driver wants to set BIAS_PULL_DOWN or something?

Quoted 3 lines

Maybe we need separate pinctrl-single,bias-disable, pinctrl-single,bias-pull-down and pinctrl-single,bias-pull-up?

Good point. I'll separate them.

Quoted 26 lines

Quoted 15 lines

drivers/pinctrl/pinctrl-single.c +15 -0

+     if (!of_property_read_u32_array(np, "pinctrl-single,power-source",
+                                     value, 2)) {
+             /* array: power source value, mask */
+             data = value[0] & value[1];
+             arg.bits.shift = ffs(value[1]) - 1;
+             arg.bits.mask = value[1] >> arg.bits.shift;
+             arg.bits.value = data >> arg.bits.shift;
+             conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_POWER_SOURCE, arg.data);
+     }
+     if (!of_property_read_u32_array(np, "pinctrl-single,input-schmitt",
+                                     value, 3)) {
+             /* array: input schmitt value, mask, disable */
+             data = value[0] & value[1];
+             arg.bits.shift = ffs(value[1]) - 1;
+             arg.bits.mask = value[1] >> arg.bits.shift;

drivers/pinctrl/pinctrl-single.c static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs, · line 1020

Quoted 10 lines

drivers/pinctrl/pinctrl-single.c +10 -0

+             if (pcs_config_match(data, value[2])) {
+                     arg.bits.value = value[2] >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_INPUT_SCHMITT_DISABLE,
+                                                 arg.data);
+             } else {
+                     arg.bits.value = data >> arg.bits.shift;
+                     conf[i++] = PIN_CONF_PACKED(PIN_CONFIG_INPUT_SCHMITT,
+                                                 arg.data);
+             }
+     }

Quoted 4 lines

And I think this has a similar problem? What if the consumer driver wants to set INPUT_SCHMITT with pin_config_set() and the initial value is SCHMITT_DISABLE? Doesn't the consumer driver get -ENOTSUPP in that case?

OK. I'll update them.

Quoted 6 lines

I'll look at this more early next week and try to get my testcase working with it.

Regards,

Tony

pin_config_set() sets the config value into pinmux register. The upper 16-bit is config argument, and the lower 16-bit is config parameter. The pinmux register
is 32-bit in Marvell's silicon. I think it's 32-bit value in most silicons.

And real field value could be lower 16-bit. So I have to pack mask, shift, config value
& config parameter into 32-bit data. The limitation is mask, shift, value are all 5-bit
value. And this is also the reason that I define pinctrl_lookup_map() into consumer.h.

How could consumer driver know mask & shift? It has to lookup map to get the real
configuration.

We also need to keep pin_config_get() compatible with pin_config_set(). I define the config data as packed mask, shift, config value & config parameter.

The conclusion is that we have two choices.
1. Use packed way. It's a little complex.

2. Change the interface of pin_config_set()/pin_config_get(). We need to use the new
interface in below.
pin_config_set(const char *dev_name, const char *name, unsigned
long parameter,
unsigned long config);
pin_config_get(const char *dev_name, const char *name, unsigned
long parameter,
unsigned long *config);
And the config should be "value >> shift".

What's your opinion? I prefer the second method since it's easy to understand and maintain.
Then I could hide pinctrl_lookup_map() in core.h.

On Sat, Nov 17, 2012 at 8:43 AM, Tony Lindgren <[email protected]> wrote:

I agree that returning data directly would be easy for the usage in device driver. Let's define it as "data >> shift".
Is it OK?

Add comments with pinconf & gpio range in the document of pinctrl-single.

+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt

+- pinctrl-single,power-source : array of value that are used to configure

Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt +9 -0

+  power source in the pinmux register. They're value of power source field
+  and power source mask.
+
+		/* power source, mask */
+		pinctrl-single,power-source = <0x1000 0x1800>;
+
+- pinctrl-single,bias : array of value that are used to configure the input
+  bias in the pinmux register.  They're value of bias field, bias mask,
+  bias disable value, bias pull down value & bias pull up value.