Re: [PATCH 2/4] pinctrl: iproc: Allow certain PINCONF functions to be disabled

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi Linus/Rob,

On 4/15/2016 1:20 AM, Linus Walleij wrote:
On Wed, Apr 13, 2016 at 2:15 AM, Ray Jui <ray.jui@xxxxxxxxxxxx> wrote:

The iProc GPIO controller is shared among multiple iProc based SoCs. In
some of these SoCs, certain PINCONF functions are disabled and registers
associated with these functions are reserved. This patch adds support to
the iProc GPIO/PINCOF driver to allow these unsupported functions to be

^PINCONF


Will fix this. Thanks.

disabled through device tree property 'brcm,pinconf-func-off'. Without
disabling these unsupported PINCONF functions, user can potentially
access these functions through sysfs debug entries; as a result, these
reserved registers can be accessed

This patch is developed based on the initial work from Yendapally Reddy
Dhananjaya <yrdreddy@xxxxxxxxxxxx> who attempted to disable drive
strength configuration for the iProc based NSP chip. In addition,
Pramod Kumar <pramodku@xxxxxxxxxxxx> also contributed to make the
support more generic across all currently supported PINCONF functions in
the iProc GPIO/PINCONF driver

Signed-off-by: Pramod Kumar <pramodku@xxxxxxxxxxxx>
Signed-off-by: Ray Jui <ray.jui@xxxxxxxxxxxx>
Reviewed-by: Jon Mason <jon.mason@xxxxxxxxxxxx>
Reviewed-by: Yendapally Reddy Dhananjaya Reddy <yendapally.reddy@xxxxxxxxxxxx>
Reviewed-by: Scott Branden <scott.branden@xxxxxxxxxxxx>

OK I see. Hm I think it is better to use a per-soc compatible string
like Rob Herring indicates to determine if we should loose these
pin config parameters. Does it seem reasonable to you?

I'm thinking that it is a property of how the hardware was synthesized
in that SoC so since it is a different version of the hardware it should
have a unique compatible string.


We use the same iProc GPIO controller in various iProc SoCs. Like you said, the same iProc GPIO controller is then synthesized slightly differently in these SoCs:

In NSP, for example, the iProc GPIO controller supports multiple PINCONF functions except drive strength.

In an upcoming iProc chip, the iProc GPIO controller only supports GPIO related operations and all PINCONF related operations are supported by a different HW block.

The use of compatible strings "brcm,iproc-gpio-only" and "brcm,iproc-gpio" models the existing pinctrl-single driver:

Required properties:
- compatible : "pinctrl-single" or "pinconf-single".
"pinctrl-single" means that pinconf isn't supported.
"pinconf-single" means that generic pinconf is supported.

Note compatible string "brcm,iproc-gpio" was already accepted and merged upstream. I believe the current debate is that you do not think a compatible string of "brcm,iproc-gpio-only" should be introduced to deal with an iProc SoC whose GPIO controller is only used as GPIO controller without PINCONF functions, correct?

If so, may I suggest the following:

1. keep "brcm,iproc-gpio" for full featured iProc GPIO and PINCONF controller 2. Introduce "brcm,iproc-gpio-v2" for NSP, where GPIO and PINCONF functions are supported but drive strength specific PINCONF feature is disabled 3. Introduce "brcm,iproc-gpio-v3" for upcoming iProc SoCs that move the PINCONF support out of this GPIO controller to a completely separate HW block

It's my understanding that the naming of a compatible string is best tight to the name of the underlying HW block that it is dealing with, instead of the name of a SoC where the HW block belongs to. I'm uncertain whether this particular case falls into the same category, where the same GPIO controller is used but synthesized differently across different SoCs.

Please let me know if the above proposed naming is acceptable?

+/*
+ * Bit position for PINCONF functions to be disabled for a given iProc SoC
+ */
+#define IPROC_PIN_DRIVE_STRENGTH        0
+#define IPROC_PIN_BIAS_DISABLE          1
+#define IPROC_PIN_BIAS_PULL_UP          2
+#define IPROC_PIN_BIAS_PULL_DOWN        3
+
+/*
+ * Bit mask for DT property "brcm,pinconf-func-off"
+ */
+#define IPROC_PIN_OFF_DRIVE_STRENGTH    (1 << IPROC_PIN_DRIVE_STRENGTH)
+#define IPROC_PIN_OFF_BIAS_DISABLE      (1 << IPROC_PIN_BIAS_DISABLE)
+#define IPROC_PIN_OFF_BIAS_PULL_UP      (1 << IPROC_PIN_BIAS_PULL_UP)
+#define IPROC_PIN_OFF_BIAS_PULL_DOWN    (1 << IPROC_PIN_BIAS_PULL_DOWN)
+
+#endif

So this stuff should not be in the device tree, the driver should know,
from the compatible string, what config options that are unavailable.

Okay.


Then if someone tries to use that in the DT, they should get an
error code -ENOTSUPP, but AFAICT that is what the patch achieves.

Yours,
Linus Walleij


Thanks,

Ray
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux