linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] gpio: modepin: Add driver support for modepin GPIO controller
@ 2021-06-15  8:05 Piyush Mehta
  2021-06-15  8:05 ` [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin Piyush Mehta
  2021-06-15  8:05 ` [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
  0 siblings, 2 replies; 7+ messages in thread
From: Piyush Mehta @ 2021-06-15  8:05 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, robh+dt, linux-gpio, devicetree,
	linux-kernel
  Cc: linux-arm-kernel, git, sgoud, michal.simek, Piyush Mehta

This patch adds support for the mode pin GPIO controller and documented
for the same. GPIO Modepin driver set and get the value and status of
the PS_MODE pin, based on device-tree pin configuration.
These 4-bits boot-mode pins are dedicated configurable as input/output.
After the stabilization of the system,these mode pins are sampled.

Piyush Mehta (2):
  dt-bindings: gpio: Add binding documentation for modepin
  gpio: modepin: Add driver support for modepin GPIO controller

 .../bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml    |  41 ++++++
 drivers/gpio/Kconfig                               |  12 ++
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-zynqmp-modepin.c                 | 154 +++++++++++++++++++++
 4 files changed, 208 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
 create mode 100644 drivers/gpio/gpio-zynqmp-modepin.c

-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin
  2021-06-15  8:05 [PATCH 0/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
@ 2021-06-15  8:05 ` Piyush Mehta
  2021-06-15 10:28   ` Michal Simek
  2021-06-24 20:50   ` Rob Herring
  2021-06-15  8:05 ` [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
  1 sibling, 2 replies; 7+ messages in thread
From: Piyush Mehta @ 2021-06-15  8:05 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, robh+dt, linux-gpio, devicetree,
	linux-kernel
  Cc: linux-arm-kernel, git, sgoud, michal.simek, Piyush Mehta

Add DT binding document for modepin GPIO controller.

Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
State: pending
---
 .../bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml    | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml

diff --git a/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
new file mode 100644
index 0000000..39d78f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
@@ -0,0 +1,41 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/gpio/xlnx,zynqmp-gpio-modepin.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: ZynqMP Mode Pin GPIO controller
+
+description:
+  PS_MODE is 4-bits boot mode pins sampled on POR deassertion. Mode Pin
+  GPIO controller with configurable from numbers of pins (from 0 to 3 per
+  PS_MODE). Every pin can be configured as input/output.
+
+maintainers:
+  - Piyush Mehta <piyush.mehta@xilinx.com>
+
+properties:
+  compatible:
+    const: xlnx,zynqmp-gpio-modepin
+
+  gpio-controller: true
+
+  "#gpio-cells":
+    const: 2
+
+required:
+  - compatible
+  - gpio-controller
+  - "#gpio-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    modepin_gpio: gpio {
+        compatible = "xlnx,zynqmp-gpio-modepin";
+        gpio-controller;
+        #gpio-cells = <2>;
+    };
+
+...
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller
  2021-06-15  8:05 [PATCH 0/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
  2021-06-15  8:05 ` [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin Piyush Mehta
@ 2021-06-15  8:05 ` Piyush Mehta
  2021-06-18  9:43   ` Linus Walleij
  1 sibling, 1 reply; 7+ messages in thread
From: Piyush Mehta @ 2021-06-15  8:05 UTC (permalink / raw)
  To: linus.walleij, bgolaszewski, robh+dt, linux-gpio, devicetree,
	linux-kernel
  Cc: linux-arm-kernel, git, sgoud, michal.simek, Piyush Mehta

This patch adds support for the mode pin GPIO controller. GPIO Modepin
driver set and get the value and status of the PS_MODE pin, based on
device-tree pin configuration. These 4-bits boot-mode pins are dedicated
configurable as input/output. After the stabilization of the system,
these mode pins are sampled.

Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
---
 drivers/gpio/Kconfig               |  12 +++
 drivers/gpio/Makefile              |   1 +
 drivers/gpio/gpio-zynqmp-modepin.c | 154 +++++++++++++++++++++++++++++++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/gpio/gpio-zynqmp-modepin.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 1dd0ec6..30e0dbf 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -755,6 +755,18 @@ config GPIO_ZYNQ
 	help
 	  Say yes here to support Xilinx Zynq GPIO controller.
 
+config GPIO_ZYNQMP_MODEPIN
+	tristate "ZynqMP ps-mode pin gpio configuration driver"
+	depends on ZYNQMP_FIRMWARE
+	default ZYNQMP_FIRMWARE
+	help
+	  Say yes here to support the ZynqMP ps-mode pin gpio configuration
+	  driver.
+
+	  This ps-mode pin gpio driver is based on GPIO framework, PS_MODE
+	  is 4-bits boot mode pins. It sets and gets the status of
+	  the ps-mode pin. Every pin can be configured as input/output.
+
 config GPIO_LOONGSON1
 	tristate "Loongson1 GPIO support"
 	depends on MACH_LOONGSON32
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d7c81e1..62bfa73 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -182,3 +182,4 @@ obj-$(CONFIG_GPIO_XRA1403)		+= gpio-xra1403.o
 obj-$(CONFIG_GPIO_XTENSA)		+= gpio-xtensa.o
 obj-$(CONFIG_GPIO_ZEVIO)		+= gpio-zevio.o
 obj-$(CONFIG_GPIO_ZYNQ)			+= gpio-zynq.o
+obj-$(CONFIG_GPIO_ZYNQMP_MODEPIN)	+= gpio-zynqmp-modepin.o
diff --git a/drivers/gpio/gpio-zynqmp-modepin.c b/drivers/gpio/gpio-zynqmp-modepin.c
new file mode 100644
index 0000000..27052f0
--- /dev/null
+++ b/drivers/gpio/gpio-zynqmp-modepin.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the ps-mode pin configuration.
+ *
+ * Copyright (c) 2021 Xilinx, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+#define MODE_PINS			4
+#define GET_OUTEN_PIN(pin)		(1U << (pin))
+
+/*
+ * modepin_gpio_get_value - Get the state of the specified pin of GPIO device
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ *
+ * This function reads the state of the specified pin of the GPIO device.
+ *
+ * Return: 0 if the pin is low, 1 if pin is high, -EINVAL wrong pin configured
+ *         or error value.
+ */
+static int modepin_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
+{
+	u32 out_en;
+	u32 regval = 0;
+	int ret;
+
+	out_en = GET_OUTEN_PIN(pin);
+
+	ret = zynqmp_pm_bootmode_read(&regval);
+	if (ret) {
+		pr_err("modepin: get value err %d\n", ret);
+		return ret;
+	}
+
+	return (out_en & (regval >> 8U)) ? 1 : 0;
+}
+
+/*
+ * modepin_gpio_set_value - Modify the state of the pin with specified value
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ * @state:	value used to modify the state of the specified pin
+ *
+ * Return:	None.
+ */
+static void modepin_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
+				   int state)
+{
+	u32 out_en;
+	u32 bootpin_val = 0;
+	int ret;
+
+	out_en = GET_OUTEN_PIN(pin);
+	state = state != 0 ? out_en : 0;
+	bootpin_val = (state << (8U)) | out_en;
+
+	/* Configure bootpin value */
+	ret = zynqmp_pm_bootmode_write(bootpin_val);
+	if (ret)
+		pr_err("modepin: %s failed\n", __func__);
+}
+
+/*
+ * modepin_gpio_dir_in - Set the direction of the specified GPIO pin as input
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ *
+ * Return: 0 always
+ */
+static int modepin_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
+{
+	return 0;
+}
+
+/*
+ * modepin_gpio_dir_out - Set the direction of the specified GPIO pin as output
+ * @chip:	gpio_chip instance to be worked on
+ * @pin:	gpio pin number within the device
+ * @state:	value to be written to specified pin
+ *
+ * Return: 0 always
+ */
+static int modepin_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
+				int state)
+{
+	return 0;
+}
+
+/*
+ * modepin_gpio_probe - Initialization method for modepin_gpio
+ * @pdev:		platform device instance
+ *
+ * Return: 0 on success, negative error otherwise.
+ */
+static int modepin_gpio_probe(struct platform_device *pdev)
+{
+	struct gpio_chip *chip;
+	int status;
+
+	chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+	if (!chip)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, chip);
+
+	/* configure the gpio chip */
+	chip->base = -1;
+	chip->ngpio = MODE_PINS;
+	chip->owner = THIS_MODULE;
+	chip->parent = &pdev->dev;
+	chip->get = modepin_gpio_get_value;
+	chip->set = modepin_gpio_set_value;
+	chip->direction_input = modepin_gpio_dir_in;
+	chip->direction_output = modepin_gpio_dir_out;
+	chip->label = dev_name(&pdev->dev);
+
+	/* modepin gpio registration */
+	status = devm_gpiochip_add_data(&pdev->dev, chip, chip);
+	if (status)
+		dev_err_probe(&pdev->dev, status,
+			      "Failed to add GPIO chip\n");
+
+	return status;
+}
+
+static const struct of_device_id modepin_platform_id[] = {
+	{ .compatible = "xlnx,zynqmp-gpio-modepin", },
+	{ }
+};
+
+static struct platform_driver modepin_platform_driver = {
+	.driver = {
+		.name = "modepin-gpio",
+		.of_match_table = modepin_platform_id,
+	},
+	.probe = modepin_gpio_probe,
+};
+
+module_platform_driver(modepin_platform_driver);
+
+MODULE_AUTHOR("Piyush Mehta <piyush.mehta@xilinx.com>");
+MODULE_DESCRIPTION("ZynqMP Boot PS_MODE Configuration");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin
  2021-06-15  8:05 ` [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin Piyush Mehta
@ 2021-06-15 10:28   ` Michal Simek
  2021-06-24 20:50   ` Rob Herring
  1 sibling, 0 replies; 7+ messages in thread
From: Michal Simek @ 2021-06-15 10:28 UTC (permalink / raw)
  To: Piyush Mehta, linus.walleij, bgolaszewski, robh+dt, linux-gpio,
	devicetree, linux-kernel
  Cc: linux-arm-kernel, git, sgoud, michal.simek



On 6/15/21 10:05 AM, Piyush Mehta wrote:
> Add DT binding document for modepin GPIO controller.
> 
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> State: pending

note: this shouldn't be the part of commit message.
But please wait when dt guys review this file and then send v2.

M

> ---
>  .../bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml    | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
> new file mode 100644
> index 0000000..39d78f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/gpio/xlnx,zynqmp-gpio-modepin.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: ZynqMP Mode Pin GPIO controller
> +
> +description:
> +  PS_MODE is 4-bits boot mode pins sampled on POR deassertion. Mode Pin
> +  GPIO controller with configurable from numbers of pins (from 0 to 3 per
> +  PS_MODE). Every pin can be configured as input/output.
> +
> +maintainers:
> +  - Piyush Mehta <piyush.mehta@xilinx.com>
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-gpio-modepin
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    modepin_gpio: gpio {
> +        compatible = "xlnx,zynqmp-gpio-modepin";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +    };
> +
> +...
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller
  2021-06-15  8:05 ` [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
@ 2021-06-18  9:43   ` Linus Walleij
  2021-06-21 17:09     ` Piyush Mehta
  0 siblings, 1 reply; 7+ messages in thread
From: Linus Walleij @ 2021-06-18  9:43 UTC (permalink / raw)
  To: Piyush Mehta
  Cc: Bartosz Golaszewski, Rob Herring, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux ARM, git, Srinivas Goud, Michal Simek

Hi Piyush!

thanks for your patch!

On Tue, Jun 15, 2021 at 10:06 AM Piyush Mehta <piyush.mehta@xilinx.com> wrote:

> This patch adds support for the mode pin GPIO controller. GPIO Modepin
> driver set and get the value and status of the PS_MODE pin, based on
> device-tree pin configuration. These 4-bits boot-mode pins are dedicated
> configurable as input/output. After the stabilization of the system,
> these mode pins are sampled.
>
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>

OK, sounds interesting!

> +#include <linux/slab.h>

I think I saw somewhere that this is not needed anymore, check if you need it.

> +#define GET_OUTEN_PIN(pin)             (1U << (pin))

Delete this macro and just use BIT(pin) inline.
#include <linux/bits.h>

> +static int modepin_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
> +{
> +       u32 out_en;
> +       u32 regval = 0;
> +       int ret;
> +
> +       out_en = GET_OUTEN_PIN(pin);

Drop this and out_en

> +       ret = zynqmp_pm_bootmode_read(&regval);
> +       if (ret) {
> +               pr_err("modepin: get value err %d\n", ret);
> +               return ret;
> +       }
> +
> +       return (out_en & (regval >> 8U)) ? 1 : 0;

return !!(regval & BIT(pin + 8));

should work and is easier to read IMO. We just check the right
bit immediately.

> +static void modepin_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
> +                                  int state)
> +{
> +       u32 out_en;
> +       u32 bootpin_val = 0;
> +       int ret;
> +
> +       out_en = GET_OUTEN_PIN(pin);

Skip this helper variable.

> +       state = state != 0 ? out_en : 0;

Uh that is really hard to read and modified a parameter. Skip that too.

> +       bootpin_val = (state << (8U)) | out_en;

What you want is mask and set.

bootpin_val = BIT(pin + 8);

> +       /* Configure bootpin value */
> +       ret = zynqmp_pm_bootmode_write(bootpin_val);

This just looks weird.

Why are you not reading the value first since you are using
read/modify/write?

I *think* you want to do this:

ret = zynqmp_pm_bootmode_read(&val);
if (ret)
   /* error handling */
if (state)
    val |= BIT(pin + 8);
else
    val &= ~BIT(pin + 8);
ret = zynqmp_pm_bootmode_write(val);
if (ret)
   /* error handling */

> +/*
> + * modepin_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * Return: 0 always
> + */
> +static int modepin_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
> +{
> +       return 0;
> +}

I think you said this was configurable in the commit message.

Use the define GPIO_LINE_DIRECTION_OUT rather than 0.

> +static int modepin_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                               int state)
> +{
> +       return 0;
> +}

Configurable?

> +       status = devm_gpiochip_add_data(&pdev->dev, chip, chip);
> +       if (status)
> +               dev_err_probe(&pdev->dev, status,
> +                             "Failed to add GPIO chip\n");

just return dev_err_probe(...)

Yours,
Linus Walleij

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* RE: [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller
  2021-06-18  9:43   ` Linus Walleij
@ 2021-06-21 17:09     ` Piyush Mehta
  0 siblings, 0 replies; 7+ messages in thread
From: Piyush Mehta @ 2021-06-21 17:09 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Bartosz Golaszewski, Rob Herring, open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel, Linux ARM, git, Srinivas Goud, Michal Simek

Hello Linus,

Thanks for the review comments.
We will address all the reviews in the next version.

Regards,
Piyush Mehta

-----Original Message-----
From: Linus Walleij <linus.walleij@linaro.org> 
Sent: Friday, June 18, 2021 3:14 PM
To: Piyush Mehta <piyushm@xilinx.com>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>; Rob Herring <robh+dt@kernel.org>; open list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; linux-kernel <linux-kernel@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>; git <git@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; Michal Simek <michals@xilinx.com>
Subject: Re: [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller

Hi Piyush!

thanks for your patch!

On Tue, Jun 15, 2021 at 10:06 AM Piyush Mehta <piyush.mehta@xilinx.com> wrote:

> This patch adds support for the mode pin GPIO controller. GPIO Modepin 
> driver set and get the value and status of the PS_MODE pin, based on 
> device-tree pin configuration. These 4-bits boot-mode pins are 
> dedicated configurable as input/output. After the stabilization of the 
> system, these mode pins are sampled.
>
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>

OK, sounds interesting!

> +#include <linux/slab.h>

I think I saw somewhere that this is not needed anymore, check if you need it.

> +#define GET_OUTEN_PIN(pin)             (1U << (pin))

Delete this macro and just use BIT(pin) inline.
#include <linux/bits.h>

> +static int modepin_gpio_get_value(struct gpio_chip *chip, unsigned 
> +int pin) {
> +       u32 out_en;
> +       u32 regval = 0;
> +       int ret;
> +
> +       out_en = GET_OUTEN_PIN(pin);

Drop this and out_en

> +       ret = zynqmp_pm_bootmode_read(&regval);
> +       if (ret) {
> +               pr_err("modepin: get value err %d\n", ret);
> +               return ret;
> +       }
> +
> +       return (out_en & (regval >> 8U)) ? 1 : 0;

return !!(regval & BIT(pin + 8));

should work and is easier to read IMO. We just check the right bit immediately.

> +static void modepin_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
> +                                  int state) {
> +       u32 out_en;
> +       u32 bootpin_val = 0;
> +       int ret;
> +
> +       out_en = GET_OUTEN_PIN(pin);

Skip this helper variable.

> +       state = state != 0 ? out_en : 0;

Uh that is really hard to read and modified a parameter. Skip that too.

> +       bootpin_val = (state << (8U)) | out_en;

What you want is mask and set.

bootpin_val = BIT(pin + 8);

> +       /* Configure bootpin value */
> +       ret = zynqmp_pm_bootmode_write(bootpin_val);

This just looks weird.

Why are you not reading the value first since you are using read/modify/write?

I *think* you want to do this:

ret = zynqmp_pm_bootmode_read(&val);
if (ret)
   /* error handling */
if (state)
    val |= BIT(pin + 8);
else
    val &= ~BIT(pin + 8);
ret = zynqmp_pm_bootmode_write(val);
if (ret)
   /* error handling */

> +/*
> + * modepin_gpio_dir_in - Set the direction of the specified GPIO pin as input
> + * @chip:      gpio_chip instance to be worked on
> + * @pin:       gpio pin number within the device
> + *
> + * Return: 0 always
> + */
> +static int modepin_gpio_dir_in(struct gpio_chip *chip, unsigned int 
> +pin) {
> +       return 0;
> +}

I think you said this was configurable in the commit message.

Use the define GPIO_LINE_DIRECTION_OUT rather than 0.

> +static int modepin_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
> +                               int state) {
> +       return 0;
> +}

Configurable?

> +       status = devm_gpiochip_add_data(&pdev->dev, chip, chip);
> +       if (status)
> +               dev_err_probe(&pdev->dev, status,
> +                             "Failed to add GPIO chip\n");

just return dev_err_probe(...)

Yours,
Linus Walleij
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin
  2021-06-15  8:05 ` [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin Piyush Mehta
  2021-06-15 10:28   ` Michal Simek
@ 2021-06-24 20:50   ` Rob Herring
  1 sibling, 0 replies; 7+ messages in thread
From: Rob Herring @ 2021-06-24 20:50 UTC (permalink / raw)
  To: Piyush Mehta
  Cc: linus.walleij, bgolaszewski, linux-gpio, devicetree,
	linux-kernel, linux-arm-kernel, git, sgoud, michal.simek

On Tue, Jun 15, 2021 at 01:35:52PM +0530, Piyush Mehta wrote:
> Add DT binding document for modepin GPIO controller.

Please give some indication in the subject this is for Xilinx.

> 
> Signed-off-by: Piyush Mehta <piyush.mehta@xilinx.com>
> State: pending
> ---
>  .../bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml    | 41 ++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
> new file mode 100644
> index 0000000..39d78f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/xlnx,zynqmp-gpio-modepin.yaml
> @@ -0,0 +1,41 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/gpio/xlnx,zynqmp-gpio-modepin.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: ZynqMP Mode Pin GPIO controller
> +
> +description:
> +  PS_MODE is 4-bits boot mode pins sampled on POR deassertion. Mode Pin
> +  GPIO controller with configurable from numbers of pins (from 0 to 3 per
> +  PS_MODE). Every pin can be configured as input/output.

And connected to other functions like GPIOs?

> +
> +maintainers:
> +  - Piyush Mehta <piyush.mehta@xilinx.com>
> +
> +properties:
> +  compatible:
> +    const: xlnx,zynqmp-gpio-modepin
> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    modepin_gpio: gpio {

Drop unused labels.

> +        compatible = "xlnx,zynqmp-gpio-modepin";
> +        gpio-controller;
> +        #gpio-cells = <2>;

How does one access this h/w?

> +    };
> +
> +...
> -- 
> 2.7.4
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-06-24 20:52 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-15  8:05 [PATCH 0/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
2021-06-15  8:05 ` [PATCH 1/2] dt-bindings: gpio: Add binding documentation for modepin Piyush Mehta
2021-06-15 10:28   ` Michal Simek
2021-06-24 20:50   ` Rob Herring
2021-06-15  8:05 ` [PATCH 2/2] gpio: modepin: Add driver support for modepin GPIO controller Piyush Mehta
2021-06-18  9:43   ` Linus Walleij
2021-06-21 17:09     ` Piyush Mehta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).