diff mbox series

gpio: Add support for gpio-line-names reading

Message ID 2ea0889ef7947c7feda0c9327ceed087c0521fed.1595337294.git.michal.simek@xilinx.com
State New
Delegated to: Michal Simek
Headers show
Series gpio: Add support for gpio-line-names reading | expand

Commit Message

Michal Simek July 21, 2020, 1:14 p.m. UTC
The commit 2bd261dd1712 ("gpio: search for gpio label if gpio is not found
through bank name") introduced the option to search gpio via labels (gpio
hog). This patch is just follow up on this and fills pin name based on
gpio-line-names DT property.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

 arch/sandbox/dts/test.dts  |  2 ++
 drivers/gpio/gpio-uclass.c | 31 +++++++++++++++++++++++++++++++
 test/dm/gpio.c             |  6 ++++++
 3 files changed, 39 insertions(+)

Comments

Heiko Schocher July 22, 2020, 4:01 a.m. UTC | #1
Hello Michal,

Am 21.07.2020 um 15:14 schrieb Michal Simek:
> The commit 2bd261dd1712 ("gpio: search for gpio label if gpio is not found
> through bank name") introduced the option to search gpio via labels (gpio
> hog). This patch is just follow up on this and fills pin name based on
> gpio-line-names DT property.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> 
>   arch/sandbox/dts/test.dts  |  2 ++
>   drivers/gpio/gpio-uclass.c | 31 +++++++++++++++++++++++++++++++
>   test/dm/gpio.c             |  6 ++++++
>   3 files changed, 39 insertions(+)

Looks good to me, thanks!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
Simon Glass July 28, 2020, 6:58 p.m. UTC | #2
On Tue, 21 Jul 2020 at 07:15, Michal Simek <michal.simek@xilinx.com> wrote:
>
> The commit 2bd261dd1712 ("gpio: search for gpio label if gpio is not found
> through bank name") introduced the option to search gpio via labels (gpio
> hog). This patch is just follow up on this and fills pin name based on
> gpio-line-names DT property.
>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
>  arch/sandbox/dts/test.dts  |  2 ++
>  drivers/gpio/gpio-uclass.c | 31 +++++++++++++++++++++++++++++++
>  test/dm/gpio.c             |  6 ++++++
>  3 files changed, 39 insertions(+)

Reviewed-by: Simon Glass <sjg@chromium.org>
Michal Simek Aug. 13, 2020, 9:24 a.m. UTC | #3
Hi,

On 28. 07. 20 20:58, Simon Glass wrote:
> On Tue, 21 Jul 2020 at 07:15, Michal Simek <michal.simek@xilinx.com> wrote:
>>
>> The commit 2bd261dd1712 ("gpio: search for gpio label if gpio is not found
>> through bank name") introduced the option to search gpio via labels (gpio
>> hog). This patch is just follow up on this and fills pin name based on
>> gpio-line-names DT property.
>>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>>  arch/sandbox/dts/test.dts  |  2 ++
>>  drivers/gpio/gpio-uclass.c | 31 +++++++++++++++++++++++++++++++
>>  test/dm/gpio.c             |  6 ++++++
>>  3 files changed, 39 insertions(+)
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> 

I have run CI loop with this patch added and I see one issue regarding
this.
Issue is visible when dm_test_gpio_get_dir_flags() is called.

Specifically
	ut_asserteq(6, gpio_request_list_by_name(dev, "test3-gpios", desc_list,
						 ARRAY_SIZE(desc_list), 0));

This function requests gpios and when request passes it assigns name to
gpio_dev_priv->name[] array.
This is done in dm_gpio_request()

Before assignment there is this code which check if name is already
assigned or not.
	uc_priv = dev_get_uclass_priv(dev);
	if (uc_priv->name[desc->offset])
		return -EBUSY;

That means that we are using name as indicator if gpio is used or not.

This logic is also applied in get_function() where you can see
	if (skip_unused && !uc_priv->name[offset])
		return GPIOF_UNUSED;

Where assigned name is just indicator of if device is used or not.

This doesn't sound right to me.
And that's just open a question how Heiko's patch should work. Because
you look for a name and you get that pin but you can't request is
because this request IMHO has to failed because gpio core thinks that it
is already in used and you shouldn't touch that pin.
The whole code which calls dm_gpio_lookup_name for bank name should be
fine because it is different entry in struct gpio_dev_priv.

Definitely this patch needs to be dropped and changed because it can't
work like this and the question is Heiko's patch should still stay in
tree or should be reverted.

Maybe we should create another entry in char **label; in struct
gpio_dev_priv where these labels should be saved and look for instead of
just name which is used for assignment.

Thanks,
Michal
diff mbox series

Patch

diff --git a/arch/sandbox/dts/test.dts b/arch/sandbox/dts/test.dts
index 3744a4660300..1b33cd4c0878 100644
--- a/arch/sandbox/dts/test.dts
+++ b/arch/sandbox/dts/test.dts
@@ -388,6 +388,8 @@ 
 			#gpio-cells = <2>;
 			gpio-bank-name = "c";
 			sandbox,gpio-count = <10>;
+			gpio-line-names = "ZERO", "ONE", "TWO", "THREE", "FOUR",
+					  "FIVE", "SIX", "SEVEL", "EIGHT", "";
 		};
 	};
 
diff --git a/drivers/gpio/gpio-uclass.c b/drivers/gpio/gpio-uclass.c
index 9c53299b6a3b..430c6849f4cd 100644
--- a/drivers/gpio/gpio-uclass.c
+++ b/drivers/gpio/gpio-uclass.c
@@ -1177,6 +1177,37 @@  static int gpio_post_probe(struct udevice *dev)
 	if (!uc_priv->name)
 		return -ENOMEM;
 
+	if (IS_ENABLED(CONFIG_DM_GPIO_LOOKUP_LABEL) &&
+	    dev_read_string(dev, "gpio-line-names")) {
+		int i, ret, count;
+		const char *name;
+		char *str;
+
+		count = dev_read_string_count(dev, "gpio-line-names");
+
+		if (count != uc_priv->gpio_count) {
+			printf("Incorrect gpio-line-names count %d/%d\n",
+			       count, uc_priv->gpio_count);
+			return -EINVAL;
+		}
+
+		for (i = 0; i < uc_priv->gpio_count; i++) {
+			ret = dev_read_string_index(dev, "gpio-line-names", i,
+						    &name);
+			if (ret)
+				return ret;
+
+			if (strlen(name)) {
+				str = strdup(name);
+				if (!str)
+					return -ENOMEM;
+
+				/* All the time there is pointer to name in DT */
+				uc_priv->name[i] = (char *)str;
+			}
+		}
+	}
+
 	return gpio_renumber(NULL);
 }
 
diff --git a/test/dm/gpio.c b/test/dm/gpio.c
index 29701389fcd5..623a157ff1db 100644
--- a/test/dm/gpio.c
+++ b/test/dm/gpio.c
@@ -142,6 +142,12 @@  static int dm_test_gpio(struct unit_test_state *uts)
 	ut_assert(gpio_lookup_name("hog_not_exist", &dev, &offset,
 				   &gpio));
 
+	/* Check if lookup for names filled via gpio-line-names work */
+	ut_assertok(gpio_lookup_name("EIGHT", &dev, &offset, &gpio));
+	ut_asserteq_str(dev->name, "pinmux-gpios");
+	ut_asserteq(8, offset);
+	ut_asserteq(CONFIG_SANDBOX_GPIO_COUNT + 20 + 10 + 8, gpio);
+
 	return 0;
 }
 DM_TEST(dm_test_gpio, DM_TESTF_SCAN_PDATA | DM_TESTF_SCAN_FDT);