Re: [PATCH] pinctrl: Add one-register-per-pin type device tree based pinctrl driver

Hi,

* Stephen Warren [off-list ref] [120621 15:17]:

On 06/19/2012 07:56 AM, Tony Lindgren wrote:

quoted

+
+- pinctrl-single,pinconf-mask : mask of allowed pinconf bits in the
+  pinmux register; this gets combined with pinconf mask but is a separate
+  mask to allow the option of setting pinconf separatately from the
+  function

Given that this binding doesn't allow describing pin configuration at
present, I would simply remove all mention of that property in the
binding documentation. It can be added back if/when that feature is
added. Any future driver using this binding can refuse to allow pin
configuration if that property is missing.

It might be better to just add support for pin_config_get/set to avoid
changing the binding later:

 static int pcs_pinconf_get(struct pinctrl_dev *pctldev,
                                unsigned pin, unsigned long *config)
 {
-	return -ENOTSUPP;
+	struct pcs_device *pcs;
+	void __iomem *reg;
+	int res;
+
+	pcs = pinctrl_dev_get_drvdata(pctldev);
+	res = pcs_pin_to_reg(pcs, pin, &reg);
+	if (res)
+		return res;
+
+	return pcs->read(reg) & pcs->cmask;
 }

A have not done that yet as currently the pcs_pin_to_reg() would need to be
sorted out in somewhat clean manner.
 

quoted

+- pinctrl-single,function-off : function off mode for disabled state if
+  available and same for all registers; if not, use a value larger than
+  function-mask to ignore disabling of registers

Rather than requiring an invalid value in this property, shouldn't the
lack of a valid function-off value be represented by the property not
being present in the DT?

Heh good point :) Will change.
 

quoted

+This driver assumes that there is only one register for each pin,
+and uses the common pinctrl bindings as specified in the pinctrl-bindings.txt
+document in this directory.

At this point in the file, I think you need to mention that you're
switching from describing the top-level device node to describing pin
configuration nodes.

Will add.
 

quoted

+The pinctrl register offsets and default values are specified as pairs

I thought we were going to remove "default" here?

Oops, looks like one was left, will remove.
 

quoted

+using pinctrl-single,pins. For example, setting a pin for a device
+could be done with:
+
+	pinctrl-single,pins = <0xdc 0x118>;
+
+Where 0xdc is the offset from the pinctrl register base address for the
+device pinctrl register, and 0x118 contains the desired value of the
+pinctrl register. See the device example and static board pins example
+below for more information.

There should be some explanation only the portion of this value covered
by the pinctrl-single,function-mask value is updated in the register.

OK
 

quoted

+This driver tries to avoid understanding pin and function names because of
+the extra bloat they would cause especially in the case of a large number
+of pins. This driver just sets what is specified for the board in the .dts file.
+Further user space debugging tools can be developed to decipher the pin and
+function names using debugfs.

There shouldn't be any discussion of a driver here; the binding is a HW
description.

Will move that to the driver comments.
 

quoted

+Example:

I only reviewed the binding document, not the code.

Thanks,

Tony