diff mbox series

[v2,7/9] gpio: mockup: pass the chip label as device property

Message ID 20200928104155.7385-8-brgl@bgdev.pl
State Superseded
Headers show
Series gpio: mockup: refactoring + documentation | expand

Commit Message

Bartosz Golaszewski Sept. 28, 2020, 10:41 a.m. UTC 
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

While we do check the "chip-name" property in probe(), we never actually
use it. Let's pass the chip label to the driver using device properties
as we'll want to allow users to define their own once dynamically
created chips are supported.

The property is renamed to "chip-label" to not cause any confusion with
the actual chip name which is of the form: "gpiochipX".

If the "chip-label" property is missing, let's do what most devices in
drivers/gpio/ do and use dev_name().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/gpio/gpio-mockup.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko Sept. 28, 2020, 1 p.m. UTC | #1 
On Mon, Sep 28, 2020 at 12:41:53PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> While we do check the "chip-name" property in probe(), we never actually
> use it. Let's pass the chip label to the driver using device properties
> as we'll want to allow users to define their own once dynamically
> created chips are supported.
> 
> The property is renamed to "chip-label" to not cause any confusion with
> the actual chip name which is of the form: "gpiochipX".
> 
> If the "chip-label" property is missing, let's do what most devices in
> drivers/gpio/ do and use dev_name().

...

> +		snprintf(chip_label, sizeof(chip_label),
> +			 "gpio-mockup-%c", i + 'A');
> +		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> +							   chip_label);

You added new property, now count is up to 4. But at the same time

	#define GPIO_MOCKUP_MAX_PROP  4

how do you avoid overflow?
Bartosz Golaszewski Sept. 28, 2020, 1:13 p.m. UTC | #2 
On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>

> On Mon, Sep 28, 2020 at 12:41:53PM +0200, Bartosz Golaszewski wrote:

> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> >

> > While we do check the "chip-name" property in probe(), we never actually

> > use it. Let's pass the chip label to the driver using device properties

> > as we'll want to allow users to define their own once dynamically

> > created chips are supported.

> >

> > The property is renamed to "chip-label" to not cause any confusion with

> > the actual chip name which is of the form: "gpiochipX".

> >


^^^ here, see below

> > If the "chip-label" property is missing, let's do what most devices in

> > drivers/gpio/ do and use dev_name().

>

> ...

>

> > +             snprintf(chip_label, sizeof(chip_label),

> > +                      "gpio-mockup-%c", i + 'A');

> > +             properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",

> > +                                                        chip_label);

>

> You added new property, now count is up to 4. But at the same time

>

>         #define GPIO_MOCKUP_MAX_PROP  4

>

> how do you avoid overflow?

>


I renamed the property, the previous "chip-name" is no longer used. In
fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.

Bart
Andy Shevchenko Sept. 28, 2020, 2 p.m. UTC | #3 
On Mon, Sep 28, 2020 at 03:13:53PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Mon, Sep 28, 2020 at 12:41:53PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > While we do check the "chip-name" property in probe(), we never actually
> > > use it. Let's pass the chip label to the driver using device properties
> > > as we'll want to allow users to define their own once dynamically
> > > created chips are supported.
> > >
> > > The property is renamed to "chip-label" to not cause any confusion with
> > > the actual chip name which is of the form: "gpiochipX".
> > >
> 
> ^^^ here, see below
> 
> > > If the "chip-label" property is missing, let's do what most devices in
> > > drivers/gpio/ do and use dev_name().
> >
> > ...
> >
> > > +             snprintf(chip_label, sizeof(chip_label),
> > > +                      "gpio-mockup-%c", i + 'A');
> > > +             properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
> > > +                                                        chip_label);
> >
> > You added new property, now count is up to 4. But at the same time
> >
> >         #define GPIO_MOCKUP_MAX_PROP  4
> >
> > how do you avoid overflow?
> >
> 
> I renamed the property, the previous "chip-name" is no longer used. In
> fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.

Either I'm missing something or...

Current code in linux-next has 3 properties to be possible

PROPERTY_ENTRY_U32("gpio-base", base);
PROPERTY_ENTRY_U16("nr-gpios", ngpio);
PROPERTY_ENTRY_BOOL("named-gpio-lines");

You adding here
PROPERTY_ENTRY_STRING("chip-label", chip_label);

Altogether after this patch is 4 which is maximum, but since array is passed by
a solely pointer, the terminator is a must.
Bartosz Golaszewski Sept. 28, 2020, 2:52 p.m. UTC | #4 
On Mon, Sep 28, 2020 at 4:00 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>

> On Mon, Sep 28, 2020 at 03:13:53PM +0200, Bartosz Golaszewski wrote:

> > On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko

> > <andriy.shevchenko@linux.intel.com> wrote:

> > >

> > > On Mon, Sep 28, 2020 at 12:41:53PM +0200, Bartosz Golaszewski wrote:

> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

> > > >

> > > > While we do check the "chip-name" property in probe(), we never actually

> > > > use it. Let's pass the chip label to the driver using device properties

> > > > as we'll want to allow users to define their own once dynamically

> > > > created chips are supported.

> > > >

> > > > The property is renamed to "chip-label" to not cause any confusion with

> > > > the actual chip name which is of the form: "gpiochipX".

> > > >

> >

> > ^^^ here, see below

> >

> > > > If the "chip-label" property is missing, let's do what most devices in

> > > > drivers/gpio/ do and use dev_name().

> > >

> > > ...

> > >

> > > > +             snprintf(chip_label, sizeof(chip_label),

> > > > +                      "gpio-mockup-%c", i + 'A');

> > > > +             properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",

> > > > +                                                        chip_label);

> > >

> > > You added new property, now count is up to 4. But at the same time

> > >

> > >         #define GPIO_MOCKUP_MAX_PROP  4

> > >

> > > how do you avoid overflow?

> > >

> >

> > I renamed the property, the previous "chip-name" is no longer used. In

> > fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.

>

> Either I'm missing something or...

>

> Current code in linux-next has 3 properties to be possible

>

> PROPERTY_ENTRY_U32("gpio-base", base);

> PROPERTY_ENTRY_U16("nr-gpios", ngpio);

> PROPERTY_ENTRY_BOOL("named-gpio-lines");

>

> You adding here

> PROPERTY_ENTRY_STRING("chip-label", chip_label);

>

> Altogether after this patch is 4 which is maximum, but since array is passed by

> a solely pointer, the terminator is a must.

>


Thanks for explaining my code to me. Yes you're right and I'm not sure
why I missed this. :)

I'll fix this in v3.

Actually this means the code is wrong even before this series - it's
just that we don't use the "chip-name" property.

Bartosz
Andy Shevchenko Sept. 28, 2020, 4:24 p.m. UTC | #5 
On Mon, Sep 28, 2020 at 04:52:25PM +0200, Bartosz Golaszewski wrote:
> On Mon, Sep 28, 2020 at 4:00 PM Andy Shevchenko

> <andriy.shevchenko@linux.intel.com> wrote:

> > On Mon, Sep 28, 2020 at 03:13:53PM +0200, Bartosz Golaszewski wrote:

> > > On Mon, Sep 28, 2020 at 3:00 PM Andy Shevchenko

> > > <andriy.shevchenko@linux.intel.com> wrote:

> > > > On Mon, Sep 28, 2020 at 12:41:53PM +0200, Bartosz Golaszewski wrote:


...

> > > > how do you avoid overflow?

> > >

> > > I renamed the property, the previous "chip-name" is no longer used. In

> > > fact it was never used but was accounted for in GPIO_MOCKUP_MAX_PROP.

> >

> > Either I'm missing something or...

> >

> > Current code in linux-next has 3 properties to be possible

> >

> > PROPERTY_ENTRY_U32("gpio-base", base);

> > PROPERTY_ENTRY_U16("nr-gpios", ngpio);

> > PROPERTY_ENTRY_BOOL("named-gpio-lines");

> >

> > You adding here

> > PROPERTY_ENTRY_STRING("chip-label", chip_label);

> >

> > Altogether after this patch is 4 which is maximum, but since array is passed by

> > a solely pointer, the terminator is a must.

> >

> 

> Thanks for explaining my code to me. Yes you're right and I'm not sure

> why I missed this. :)

> 

> I'll fix this in v3.

> 

> Actually this means the code is wrong even before this series - it's

> just that we don't use the "chip-name" property.


Right, you patch just exposed it.

-- 
With Best Regards,
Andy Shevchenko
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-mockup.c b/drivers/gpio/gpio-mockup.c
index de778b52f355..5b2686f9e07d 100644
--- a/drivers/gpio/gpio-mockup.c
+++ b/drivers/gpio/gpio-mockup.c
@@ -429,21 +429,14 @@  static int gpio_mockup_probe(struct platform_device *pdev)
 	if (rv)
 		return rv;
 
-	rv = device_property_read_string(dev, "chip-name", &name);
+	rv = device_property_read_string(dev, "chip-label", &name);
 	if (rv)
-		name = NULL;
+		name = dev_name(dev);
 
 	chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
 	if (!chip)
 		return -ENOMEM;
 
-	if (!name) {
-		name = devm_kasprintf(dev, GFP_KERNEL,
-				      "%s-%c", pdev->name, pdev->id + 'A');
-		if (!name)
-			return -ENOMEM;
-	}
-
 	mutex_init(&chip->lock);
 
 	gc = &chip->gc;
@@ -523,6 +516,7 @@  static int __init gpio_mockup_init(void)
 	int i, prop, num_chips, err = 0, base;
 	struct platform_device_info pdevinfo;
 	struct platform_device *pdev;
+	char chip_label[32];
 	u16 ngpio;
 
 	if ((gpio_mockup_num_ranges < 2) ||
@@ -556,6 +550,11 @@  static int __init gpio_mockup_init(void)
 		memset(&pdevinfo, 0, sizeof(pdevinfo));
 		prop = 0;
 
+		snprintf(chip_label, sizeof(chip_label),
+			 "gpio-mockup-%c", i + 'A');
+		properties[prop++] = PROPERTY_ENTRY_STRING("chip-label",
+							   chip_label);
+
 		base = gpio_mockup_range_base(i);
 		if (base >= 0)
 			properties[prop++] = PROPERTY_ENTRY_U32("gpio-base",