[PATCH v2 0/7] gpio: mockup: updates for 4.13

[PATCH v2 0/7] gpio: mockup: updates for 4.13

[Bartosz Golaszewski]

This series contains a couple bug fixes and other minor updates for
the GPIO testing module.

v1 -> v2:
- omit applied patches
- use kstrtoint_from_user() for debugfs input sanitization
- add a patch improving the code in gpio_mockup_event_write()
- bail out from gpio_mockup_event_write() on invalid input even when
  nobody is listening for events

Bartosz Golaszewski (7):
  gpio: mockup: improve the debugfs input sanitization
  gpio: mockup: tweak gpio_mockup_event_write()
  gpio: mockup: refuse to accept an odd number of GPIO ranges
  gpio: mockup: improve readability
  gpio: mockup: don't return magic numbers from probe()
  gpio: mockup: improve the error message
  gpio: mockup: add myself as author

 drivers/gpio/gpio-mockup.c | 61 ++++++++++++++++++++++++----------------------
 1 file changed, 32 insertions(+), 29 deletions(-)

-- 
2.9.3

[PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization

[Bartosz Golaszewski]

We're currently only checking the first character of the input to the
debugfs event files, so a string like '0sdfdsf' is valid and indicates
a falling edge event.

Be more strict and only allow '0', '1', '0\n' & '1\n'.

While we're at it: move the sanitization code before the irq_enabled
check so that we indicate an error on invalid input even if nobody is
waiting for events.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index ba8d62a..da76267 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -208,8 +208,7 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	struct seq_file *sfile;
 	struct gpio_desc *desc;
 	struct gpio_chip *gc;
-	int val;
-	char buf;
+	int rv, val;
 
 	sfile = file->private_data;
 	priv = sfile->private;
@@ -217,19 +216,15 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	chip = priv->chip;
 	gc = &chip->gc;
 
+	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
+	if (rv)
+		return rv;
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
 	if (!chip->lines[priv->offset].irq_enabled)
 		return size;
 
-	if (copy_from_user(&buf, usr_buf, 1))
-		return -EFAULT;
-
-	if (buf == '0')
-		val = 0;
-	else if (buf == '1')
-		val = 1;
-	else
-		return -EINVAL;
-
 	gpiod_set_value_cansleep(desc, val);
 	priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
 	irq_work_queue(&priv->chip->irq_ctx.work);
-- 
2.9.3

[PATCH v2 2/7] gpio: mockup: tweak gpio_mockup_event_write()

[Bartosz Golaszewski]

Invert the logic of the irq_enabled check and only access the private
data after the input is sanitized.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index da76267..d78e8e0 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -210,24 +210,23 @@ static ssize_t gpio_mockup_event_write(struct file *file,
 	struct gpio_chip *gc;
 	int rv, val;
 
-	sfile = file->private_data;
-	priv = sfile->private;
-	desc = priv->desc;
-	chip = priv->chip;
-	gc = &chip->gc;
-
 	rv = kstrtoint_from_user(usr_buf, size, 0, &val);
 	if (rv)
 		return rv;
 	if (val != 0 && val != 1)
 		return -EINVAL;
 
-	if (!chip->lines[priv->offset].irq_enabled)
-		return size;
+	sfile = file->private_data;
+	priv = sfile->private;
+	desc = priv->desc;
+	chip = priv->chip;
+	gc = &chip->gc;
 
-	gpiod_set_value_cansleep(desc, val);
-	priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
-	irq_work_queue(&priv->chip->irq_ctx.work);
+	if (chip->lines[priv->offset].irq_enabled) {
+		gpiod_set_value_cansleep(desc, val);
+		priv->chip->irq_ctx.irq = gc->irq_base + priv->offset;
+		irq_work_queue(&priv->chip->irq_ctx.work);
+	}
 
 	return size;
 }
-- 
2.9.3

[PATCH v2 3/7] gpio: mockup: refuse to accept an odd number of GPIO ranges

[Bartosz Golaszewski]

Currently we ignore the last odd range value, since each chip is
described by two values. Be more strict and require the user to
pass an even number of ranges.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index d78e8e0..d95d37a 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -334,7 +334,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 	int ret, i, base, ngpio;
 	char *chip_name;
 
-	if (gpio_mockup_params_nr < 2)
+	if (gpio_mockup_params_nr < 2 || (gpio_mockup_params_nr % 2))
 		return -EINVAL;
 
 	chips = devm_kzalloc(dev,
-- 
2.9.3

[PATCH v2 4/7] gpio: mockup: improve readability

[Bartosz Golaszewski]

We currently shift bits here and there without actually explaining
what we're doing. Add some helper variables with names indicating
their purpose to improve the code readability.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index d95d37a..0cb6cba 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -27,6 +27,11 @@
 
 #define GPIO_MOCKUP_NAME	"gpio-mockup"
 #define	GPIO_MOCKUP_MAX_GC	10
+/*
+ * We're storing two values per chip: the GPIO base and the number
+ * of GPIO lines.
+ */
+#define GPIO_MOCKUP_MAX_RANGES	(GPIO_MOCKUP_MAX_GC * 2)
 
 enum {
 	GPIO_MOCKUP_DIR_OUT = 0,
@@ -62,7 +67,7 @@ struct gpio_mockup_dbgfs_private {
 	int offset;
 };
 
-static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_GC << 1];
+static int gpio_mockup_ranges[GPIO_MOCKUP_MAX_RANGES];
 static int gpio_mockup_params_nr;
 module_param_array(gpio_mockup_ranges, int, &gpio_mockup_params_nr, 0400);
 
@@ -329,23 +334,24 @@ static int gpio_mockup_add(struct device *dev,
 
 static int gpio_mockup_probe(struct platform_device *pdev)
 {
-	struct gpio_mockup_chip *chips;
+	int ret, i, base, ngpio, num_chips;
 	struct device *dev = &pdev->dev;
-	int ret, i, base, ngpio;
+	struct gpio_mockup_chip *chips;
 	char *chip_name;
 
 	if (gpio_mockup_params_nr < 2 || (gpio_mockup_params_nr % 2))
 		return -EINVAL;
 
-	chips = devm_kzalloc(dev,
-			     sizeof(*chips) * (gpio_mockup_params_nr >> 1),
-			     GFP_KERNEL);
+	/* Each chip is described by two values. */
+	num_chips = gpio_mockup_params_nr / 2;
+
+	chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);
 	if (!chips)
 		return -ENOMEM;
 
 	platform_set_drvdata(pdev, chips);
 
-	for (i = 0; i < gpio_mockup_params_nr >> 1; i++) {
+	for (i = 0; i < num_chips; i++) {
 		base = gpio_mockup_ranges[i * 2];
 
 		if (base == -1)
-- 
2.9.3

[PATCH v2 5/7] gpio: mockup: don't return magic numbers from probe()

[Bartosz Golaszewski]

When the requested number of GPIO lines is 0, return -EINVAL, not
-1 which is -EPERM.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 0cb6cba..ab2d38e 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -369,7 +369,7 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 			ret = gpio_mockup_add(dev, &chips[i],
 					      chip_name, base, ngpio);
 		} else {
-			ret = -1;
+			ret = -EINVAL;
 		}
 
 		if (ret) {
-- 
2.9.3

[PATCH v2 6/7] gpio: mockup: improve the error message

[Bartosz Golaszewski]

Indicate the error number and make the message a bit more elaborate.

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index ab2d38e..2f4fe41 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -373,8 +373,9 @@ static int gpio_mockup_probe(struct platform_device *pdev)
 		}
 
 		if (ret) {
-			dev_err(dev, "gpio<%d..%d> add failed\n",
-				base, base < 0 ? ngpio : base + ngpio);
+			dev_err(dev,
+				"adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
+				ret, base, base < 0 ? ngpio : base + ngpio);
 
 			return ret;
 		}
-- 
2.9.3

[PATCH v2 7/7] gpio: mockup: add myself as author

[Bartosz Golaszewski]

Just taking credit for the recent changes and new features. :)

Signed-off-by: Bartosz Golaszewski <brgl@bgdev.pl>
---
 drivers/gpio/gpio-mockup.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index 2f4fe41..536a229 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2014  Kamlakant Patel <kamlakant.patel@broadcom.com>
  * Copyright (C) 2015-2016  Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
+ * Copyright (C) 2017 Bartosz Golaszewski <brgl@bgdev.pl>
  *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
@@ -432,5 +433,6 @@ module_exit(mock_device_exit);
 
 MODULE_AUTHOR("Kamlakant Patel <kamlakant.patel@broadcom.com>");
 MODULE_AUTHOR("Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>");
+MODULE_AUTHOR("Bartosz Golaszewski <brgl@bgdev.pl>");
 MODULE_DESCRIPTION("GPIO Testing driver");
 MODULE_LICENSE("GPL v2");
-- 
2.9.3

Re: [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization

[Andy Shevchenko]

On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> We're currently only checking the first character of the input to the
> debugfs event files, so a string like '0sdfdsf' is valid and indicates
> a falling edge event.
>
> Be more strict and only allow '0', '1', '0\n' & '1\n'.
>
> While we're at it: move the sanitization code before the irq_enabled
> check so that we indicate an error on invalid input even if nobody is
> waiting for events.

> -       int val;
> -       char buf;
> +       int rv, val;

> +       rv = kstrtoint_from_user(usr_buf, size, 0, &val);
> +       if (rv)
> +               return rv;

> +       if (val != 0 && val != 1)

Wouldn't be easier to have

u8 rv;

ret = kstrtu8_from_user();
if (ret >= 2)
 return ...;

?

> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/7] gpio: mockup: improve readability

[Andy Shevchenko]

On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> We currently shift bits here and there without actually explaining
> what we're doing. Add some helper variables with names indicating
> their purpose to improve the code readability.

> +       /* Each chip is described by two values. */
> +       num_chips = gpio_mockup_params_nr / 2;
> +
> +       chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);

It's effectively
devm_kcalloc()
or
devm_kmalloc_array()
depending on the requirement of zeroing a memory chunks.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] gpio: mockup: improve the error message

[Andy Shevchenko]

On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> Indicate the error number and make the message a bit more elaborate.

> +                       dev_err(dev,
> +                               "adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
> +                               ret, base, base < 0 ? ngpio : base + ngpio);

You may consider to use
'gpio_mockup_add' instead of 'adding gpiochip'. The latter points the
reader first to gpiochip_add family of functions while you run a
wrapper on top of it.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization

[Bartosz Golaszewski]

2017-05-30 20:52 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> We're currently only checking the first character of the input to the
>> debugfs event files, so a string like '0sdfdsf' is valid and indicates
>> a falling edge event.
>>
>> Be more strict and only allow '0', '1', '0\n' & '1\n'.
>>
>> While we're at it: move the sanitization code before the irq_enabled
>> check so that we indicate an error on invalid input even if nobody is
>> waiting for events.
>
>> -       int val;
>> -       char buf;
>> +       int rv, val;
>
>> +       rv = kstrtoint_from_user(usr_buf, size, 0, &val);
>> +       if (rv)
>> +               return rv;
>
>> +       if (val != 0 && val != 1)
>
> Wouldn't be easier to have
>
> u8 rv;
>
> ret = kstrtu8_from_user();
> if (ret >= 2)
>  return ...;
>
> ?

kstrtu8_from_user() doesn't return the converted value, so you won't
skip an if anyway and by using the int variant, we're avoiding a cast.
I'd prefer it this way frankly.

Thanks,
Bartosz

Re: [PATCH v2 4/7] gpio: mockup: improve readability

[Bartosz Golaszewski]

2017-05-30 20:55 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> We currently shift bits here and there without actually explaining
>> what we're doing. Add some helper variables with names indicating
>> their purpose to improve the code readability.
>
>> +       /* Each chip is described by two values. */
>> +       num_chips = gpio_mockup_params_nr / 2;
>> +
>> +       chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);
>
> It's effectively
> devm_kcalloc()
> or
> devm_kmalloc_array()
> depending on the requirement of zeroing a memory chunks.
>

Is there any advantage to using one of these here?

Thanks,
Bartosz

Re: [PATCH v2 6/7] gpio: mockup: improve the error message

[Bartosz Golaszewski]

2017-05-30 20:59 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> Indicate the error number and make the message a bit more elaborate.
>
>> +                       dev_err(dev,
>> +                               "adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
>> +                               ret, base, base < 0 ? ngpio : base + ngpio);
>
> You may consider to use
> 'gpio_mockup_add' instead of 'adding gpiochip'. The latter points the
> reader first to gpiochip_add family of functions while you run a
> wrapper on top of it.
>

But this message can also be emitted if the module params are invalid,
in which case we don't even enter gpio_mockup_add().

Thanks,
Bartosz

Re: [PATCH v2 4/7] gpio: mockup: improve readability

[Andy Shevchenko]

On Wed, May 31, 2017 at 1:53 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-05-30 20:55 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>> We currently shift bits here and there without actually explaining
>>> what we're doing. Add some helper variables with names indicating
>>> their purpose to improve the code readability.
>>
>>> +       /* Each chip is described by two values. */
>>> +       num_chips = gpio_mockup_params_nr / 2;
>>> +
>>> +       chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);
>>
>> It's effectively
>> devm_kcalloc()
>> or
>> devm_kmalloc_array()
>> depending on the requirement of zeroing a memory chunks.

> Is there any advantage to using one of these here?

Yes, though subtle one in this case. The caller will not care about
(possible) overflow in multiplication.
Other (micro)optimizations might be in place in the future as well,
but I dunno about this.

I would suggest to change.

P.S. And of course if you don't need zeroed area it would be (slight?)
performance impact I suppose.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] gpio: mockup: improve the error message

[Andy Shevchenko]

On Wed, May 31, 2017 at 1:54 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-05-30 20:59 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>> Indicate the error number and make the message a bit more elaborate.
>>
>>> +                       dev_err(dev,
>>> +                               "adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
>>> +                               ret, base, base < 0 ? ngpio : base + ngpio);
>>
>> You may consider to use
>> 'gpio_mockup_add' instead of 'adding gpiochip'. The latter points the
>> reader first to gpiochip_add family of functions while you run a
>> wrapper on top of it.
>>
>
> But this message can also be emitted if the module params are invalid,
> in which case we don't even enter gpio_mockup_add().

...which unveils bad phrasing in the message. In that case "adding
gpiochip" is also misleading.

I dunno if it requires separate patch to fix the phrasing, though it
would be nice to make it more clear for both cases, or even split to
two cases.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/7] gpio: mockup: improve readability

[Bartosz Golaszewski]

2017-05-31 16:57 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Wed, May 31, 2017 at 1:53 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> 2017-05-30 20:55 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>> We currently shift bits here and there without actually explaining
>>>> what we're doing. Add some helper variables with names indicating
>>>> their purpose to improve the code readability.
>>>
>>>> +       /* Each chip is described by two values. */
>>>> +       num_chips = gpio_mockup_params_nr / 2;
>>>> +
>>>> +       chips = devm_kzalloc(dev, sizeof(*chips) * num_chips, GFP_KERNEL);
>>>
>>> It's effectively
>>> devm_kcalloc()
>>> or
>>> devm_kmalloc_array()
>>> depending on the requirement of zeroing a memory chunks.
>
>> Is there any advantage to using one of these here?
>
> Yes, though subtle one in this case. The caller will not care about
> (possible) overflow in multiplication.
> Other (micro)optimizations might be in place in the future as well,
> but I dunno about this.
>
> I would suggest to change.
>

Ok, I'll change it both for the lines and for the chips arrays and add
a separate patch.

Thanks,
Bartosz

Re: [PATCH v2 6/7] gpio: mockup: improve the error message

[Bartosz Golaszewski]

2017-05-31 17:00 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
> On Wed, May 31, 2017 at 1:54 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>> 2017-05-30 20:59 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>> Indicate the error number and make the message a bit more elaborate.
>>>
>>>> +                       dev_err(dev,
>>>> +                               "adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
>>>> +                               ret, base, base < 0 ? ngpio : base + ngpio);
>>>
>>> You may consider to use
>>> 'gpio_mockup_add' instead of 'adding gpiochip'. The latter points the
>>> reader first to gpiochip_add family of functions while you run a
>>> wrapper on top of it.
>>>
>>
>> But this message can also be emitted if the module params are invalid,
>> in which case we don't even enter gpio_mockup_add().
>
> ...which unveils bad phrasing in the message. In that case "adding
> gpiochip" is also misleading.
>

Not really. You can pass an invalid value later in the list which will
only become apparent when it's reached. In that case previous
gpiochips will be added correctly but probe will fail with -EINVAL
after reaching the bad one in which case the message is right. I hope
I'm being clear.

Thanks,
Bartosz

Re: [PATCH v2 1/7] gpio: mockup: improve the debugfs input sanitization

[Andy Shevchenko]

On Wed, May 31, 2017 at 1:52 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2017-05-30 20:52 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:

>>> +       if (val != 0 && val != 1)
>>
>> Wouldn't be easier to have
>>
>> u8 rv;
>>
>> ret = kstrtu8_from_user();
>> if (ret >= 2)
>>  return ...;
>>
>> ?
>
> kstrtu8_from_user() doesn't return the converted value, so you won't
> skip an if anyway

Oh, yes.

> and by using the int variant, we're avoiding a cast.
> I'd prefer it this way frankly.

Fair enough. (Though I would go with (val < 0 && val > 1) condition,
of course it's matter of taste)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/7] gpio: mockup: improve the error message

[Bartosz Golaszewski]

2017-05-31 17:26 GMT+02:00 Bartosz Golaszewski <brgl@bgdev.pl>:
> 2017-05-31 17:00 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Wed, May 31, 2017 at 1:54 PM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>> 2017-05-30 20:59 GMT+02:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>>>> On Tue, May 30, 2017 at 11:58 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>>>>> Indicate the error number and make the message a bit more elaborate.
>>>>
>>>>> +                       dev_err(dev,
>>>>> +                               "adding gpiochip failed: %d (base: %d, ngpio: %d)\n",
>>>>> +                               ret, base, base < 0 ? ngpio : base + ngpio);
>>>>
>>>> You may consider to use
>>>> 'gpio_mockup_add' instead of 'adding gpiochip'. The latter points the
>>>> reader first to gpiochip_add family of functions while you run a
>>>> wrapper on top of it.
>>>>
>>>
>>> But this message can also be emitted if the module params are invalid,
>>> in which case we don't even enter gpio_mockup_add().
>>
>> ...which unveils bad phrasing in the message. In that case "adding
>> gpiochip" is also misleading.
>>
>
> Not really. You can pass an invalid value later in the list which will
> only become apparent when it's reached. In that case previous
> gpiochips will be added correctly but probe will fail with -EINVAL
> after reaching the bad one in which case the message is right. I hope
> I'm being clear.
>

Which made me think: maybe the next step would be to parse the
arguments in the module init function and probe each dummy gpiochip
separately...

Best regards,
Bartosz Golaszewski