* [IIO] Mistake in assumption about trigger names
@ 2011-09-09 14:01 Manuel Stahl
2011-09-09 14:40 ` Jonathan Cameron
0 siblings, 1 reply; 6+ messages in thread
From: Manuel Stahl @ 2011-09-09 14:01 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
Hi Jonathan,
I'm currently updating the iioutils SF project. By doing so I encountered a
problem in your generic_buffer.c example.
You generate the trigger name from the device name:
> if (trigger_name == NULL) {
> /*
> * Build the trigger name. If it is device associated it's
> * name is <device_name>_dev[n] where n matches the device
> * number found above
> */
> ret = asprintf(&trigger_name,
> "%s-dev%d", device_name, dev_num);
> if (ret < 0) {
> ret = -ENOMEM;
> goto error_ret;
> }
> }
That will not work if a device has no trigger and is enumerated before the
device you are looking at.
I think we need a symlink in the device directory to it's own trigger.
Also the current_trigger attribute should go into the buffer directory, as
this is the place, where it is actually used.
--
Regards,
Manuel Stahl
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [IIO] Mistake in assumption about trigger names 2011-09-09 14:01 [IIO] Mistake in assumption about trigger names Manuel Stahl @ 2011-09-09 14:40 ` Jonathan Cameron 2011-09-09 14:56 ` [PATCH] staging:iio: move id and device name setting to iio_device_allocate Jonathan Cameron 0 siblings, 1 reply; 6+ messages in thread From: Jonathan Cameron @ 2011-09-09 14:40 UTC (permalink / raw) To: Manuel Stahl; +Cc: linux-iio On 09/09/11 15:01, Manuel Stahl wrote: > Hi Jonathan, > > I'm currently updating the iioutils SF project. By doing so I encountered a > problem in your generic_buffer.c example. > > You generate the trigger name from the device name: >> if (trigger_name == NULL) { >> /* >> * Build the trigger name. If it is device associated it's >> * name is <device_name>_dev[n] where n matches the device >> * number found above >> */ >> ret = asprintf(&trigger_name, >> "%s-dev%d", device_name, dev_num); >> if (ret < 0) { >> ret = -ENOMEM; >> goto error_ret; >> } >> } > > That will not work if a device has no trigger and is enumerated before the > device you are looking at. Dratt, that is actually a result of a bug rather than a missassumption. The number should be independent of the enumeration order. That is it should be consistent what ever the ordering. The reorganisation of when the device_register happens has broken this however (which is where the id is allocated). My tests didn't pick it up because I was testing only one device. Yours presumably just have one trigger provider so it looked like it was getting the trigger number whereas it was actually getting 0 on all occasions. The fix is to move the id allocation and deallocation from iio_device_register / unregister to the allocate and free paths. That should put things back the way they were... I'll hammer that out quickly now. > > I think we need a symlink in the device directory to it's own trigger. I'm anti that purely because we then have two different ways of finding a trigger. Currently there is one consistent method. They are both children of the underlying device so one can get to it that way if it's really necessary. > Also the current_trigger attribute should go into the buffer directory, as > this is the place, where it is actually used. This one has come up before. It is a pain to do so as the files in the buffer directory come from the buffer implementation and those in the trigger directory from the device driver calling into the core. Same is true of scan_elements and so I'm afraid the slight improvement in logic of where things are located is not worth the complexity increase in the driver. Jonathan ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] staging:iio: move id and device name setting to iio_device_allocate. 2011-09-09 14:40 ` Jonathan Cameron @ 2011-09-09 14:56 ` Jonathan Cameron 2011-09-09 14:57 ` Jonathan Cameron 2011-09-12 8:27 ` Manuel Stahl 0 siblings, 2 replies; 6+ messages in thread From: Jonathan Cameron @ 2011-09-09 14:56 UTC (permalink / raw) To: linux-iio; +Cc: manuel.stahl, Jonathan Cameron The recent reorganization of the sysfs attribute registration had the side effect of moving iio_device_register after registration of triggers etc. The side effect of this is that the id hadn't been allocated by the time of trigger registration. Thus all triggers based on device got the name <dev_name>-dev0 instead of <dev_name>-devN where N is the iio device id. This should also fix the lack of device name for some error messages that we have been seeing (and I'd been meaning to track down) as that has now moved earlier as well. Reported-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> --- drivers/staging/iio/industrialio-core.c | 25 +++++++++++++------------ 1 files changed, 13 insertions(+), 12 deletions(-) diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c index fd4aada..8486914 100644 --- a/drivers/staging/iio/industrialio-core.c +++ b/drivers/staging/iio/industrialio-core.c @@ -1033,6 +1033,15 @@ struct iio_dev *iio_allocate_device(int sizeof_priv) device_initialize(&dev->dev); dev_set_drvdata(&dev->dev, (void *)dev); mutex_init(&dev->mlock); + + dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); + if (dev->id < 0) { + /* cannot use a dev_err as the name isn't available */ + printk(KERN_ERR "Failed to get id\n"); + kfree(dev); + return NULL; + } + dev_set_name(&dev->dev, "iio:device%d", dev->id); } return dev; @@ -1041,8 +1050,10 @@ EXPORT_SYMBOL(iio_allocate_device); void iio_free_device(struct iio_dev *dev) { - if (dev) + if (dev) { + ida_simple_remove(&iio_ida, dev->id); kfree(dev); + } } EXPORT_SYMBOL(iio_free_device); @@ -1100,14 +1111,6 @@ int iio_device_register(struct iio_dev *dev_info) { int ret; - dev_info->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); - if (dev_info->id < 0) { - ret = dev_info->id; - dev_err(&dev_info->dev, "Failed to get id\n"); - goto error_ret; - } - dev_set_name(&dev_info->dev, "iio:device%d", dev_info->id); - /* configure elements for the chrdev */ dev_info->dev.devt = MKDEV(MAJOR(iio_devt), dev_info->id); @@ -1115,7 +1118,7 @@ int iio_device_register(struct iio_dev *dev_info) if (ret) { dev_err(dev_info->dev.parent, "Failed to register sysfs interfaces\n"); - goto error_free_ida; + goto error_ret; } ret = iio_device_register_eventset(dev_info); if (ret) { @@ -1142,8 +1145,6 @@ error_unreg_eventset: iio_device_unregister_eventset(dev_info); error_free_sysfs: iio_device_unregister_sysfs(dev_info); -error_free_ida: - ida_simple_remove(&iio_ida, dev_info->id); error_ret: return ret; } -- 1.7.3.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] staging:iio: move id and device name setting to iio_device_allocate. 2011-09-09 14:56 ` [PATCH] staging:iio: move id and device name setting to iio_device_allocate Jonathan Cameron @ 2011-09-09 14:57 ` Jonathan Cameron 2011-09-12 8:27 ` Manuel Stahl 1 sibling, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2011-09-09 14:57 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio, manuel.stahl On 09/09/11 15:56, Jonathan Cameron wrote: > The recent reorganization of the sysfs attribute registration had > the side effect of moving iio_device_register after registration of > triggers etc. The side effect of this is that the id hadn't been > allocated by the time of trigger registration. Thus all triggers > based on device got the name <dev_name>-dev0 instead of <dev_name>-devN > where N is the iio device id. > > This should also fix the lack of device name for some error messages > that we have been seeing (and I'd been meaning to track down) as > that has now moved earlier as well. I've done some cursory testing with this and 3 devices where the one with the trigger was registered last. Seems to work as it should have done in the first place. Manuel, can you just verify this works for you before I send it on to Greg? Thanks, Jonathan > > Reported-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de> > Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> > --- > drivers/staging/iio/industrialio-core.c | 25 +++++++++++++------------ > 1 files changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/staging/iio/industrialio-core.c b/drivers/staging/iio/industrialio-core.c > index fd4aada..8486914 100644 > --- a/drivers/staging/iio/industrialio-core.c > +++ b/drivers/staging/iio/industrialio-core.c > @@ -1033,6 +1033,15 @@ struct iio_dev *iio_allocate_device(int sizeof_priv) > device_initialize(&dev->dev); > dev_set_drvdata(&dev->dev, (void *)dev); > mutex_init(&dev->mlock); > + > + dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); > + if (dev->id < 0) { > + /* cannot use a dev_err as the name isn't available */ > + printk(KERN_ERR "Failed to get id\n"); > + kfree(dev); > + return NULL; > + } > + dev_set_name(&dev->dev, "iio:device%d", dev->id); > } > > return dev; > @@ -1041,8 +1050,10 @@ EXPORT_SYMBOL(iio_allocate_device); > > void iio_free_device(struct iio_dev *dev) > { > - if (dev) > + if (dev) { > + ida_simple_remove(&iio_ida, dev->id); > kfree(dev); > + } > } > EXPORT_SYMBOL(iio_free_device); > > @@ -1100,14 +1111,6 @@ int iio_device_register(struct iio_dev *dev_info) > { > int ret; > > - dev_info->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); > - if (dev_info->id < 0) { > - ret = dev_info->id; > - dev_err(&dev_info->dev, "Failed to get id\n"); > - goto error_ret; > - } > - dev_set_name(&dev_info->dev, "iio:device%d", dev_info->id); > - > /* configure elements for the chrdev */ > dev_info->dev.devt = MKDEV(MAJOR(iio_devt), dev_info->id); > > @@ -1115,7 +1118,7 @@ int iio_device_register(struct iio_dev *dev_info) > if (ret) { > dev_err(dev_info->dev.parent, > "Failed to register sysfs interfaces\n"); > - goto error_free_ida; > + goto error_ret; > } > ret = iio_device_register_eventset(dev_info); > if (ret) { > @@ -1142,8 +1145,6 @@ error_unreg_eventset: > iio_device_unregister_eventset(dev_info); > error_free_sysfs: > iio_device_unregister_sysfs(dev_info); > -error_free_ida: > - ida_simple_remove(&iio_ida, dev_info->id); > error_ret: > return ret; > } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging:iio: move id and device name setting to iio_device_allocate. 2011-09-09 14:56 ` [PATCH] staging:iio: move id and device name setting to iio_device_allocate Jonathan Cameron 2011-09-09 14:57 ` Jonathan Cameron @ 2011-09-12 8:27 ` Manuel Stahl 2011-09-12 8:37 ` Jonathan Cameron 1 sibling, 1 reply; 6+ messages in thread From: Manuel Stahl @ 2011-09-12 8:27 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio Hi Jonathan, the patch works for me, but I still have to search through all triggers and= =20 try to match the names to find the trigger belonging to a device as the=20 trigger directories are enumerated differently. Regards, Manuel Am Freitag, 9. September 2011, 16:56:07 schrieb Jonathan Cameron: > The recent reorganization of the sysfs attribute registration had > the side effect of moving iio_device_register after registration of > triggers etc. The side effect of this is that the id hadn't been > allocated by the time of trigger registration. Thus all triggers > based on device got the name <dev_name>-dev0 instead of <dev_name>-devN > where N is the iio device id. >=20 > This should also fix the lack of device name for some error messages > that we have been seeing (and I'd been meaning to track down) as > that has now moved earlier as well. >=20 > Reported-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de> > Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> > --- > drivers/staging/iio/industrialio-core.c | 25 +++++++++++++------------ > 1 files changed, 13 insertions(+), 12 deletions(-) >=20 > diff --git a/drivers/staging/iio/industrialio-core.c > b/drivers/staging/iio/industrialio-core.c index fd4aada..8486914 100644 > --- a/drivers/staging/iio/industrialio-core.c > +++ b/drivers/staging/iio/industrialio-core.c > @@ -1033,6 +1033,15 @@ struct iio_dev *iio_allocate_device(int sizeof_pri= v) > device_initialize(&dev->dev); > dev_set_drvdata(&dev->dev, (void *)dev); > mutex_init(&dev->mlock); > + > + dev->id =3D ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); > + if (dev->id < 0) { > + /* cannot use a dev_err as the name isn't available */ > + printk(KERN_ERR "Failed to get id\n"); > + kfree(dev); > + return NULL; > + } > + dev_set_name(&dev->dev, "iio:device%d", dev->id); > } >=20 > return dev; > @@ -1041,8 +1050,10 @@ EXPORT_SYMBOL(iio_allocate_device); >=20 > void iio_free_device(struct iio_dev *dev) > { > - if (dev) > + if (dev) { > + ida_simple_remove(&iio_ida, dev->id); > kfree(dev); > + } > } > EXPORT_SYMBOL(iio_free_device); >=20 > @@ -1100,14 +1111,6 @@ int iio_device_register(struct iio_dev *dev_info) > { > int ret; >=20 > - dev_info->id =3D ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); > - if (dev_info->id < 0) { > - ret =3D dev_info->id; > - dev_err(&dev_info->dev, "Failed to get id\n"); > - goto error_ret; > - } > - dev_set_name(&dev_info->dev, "iio:device%d", dev_info->id); > - > /* configure elements for the chrdev */ > dev_info->dev.devt =3D MKDEV(MAJOR(iio_devt), dev_info->id); >=20 > @@ -1115,7 +1118,7 @@ int iio_device_register(struct iio_dev *dev_info) > if (ret) { > dev_err(dev_info->dev.parent, > "Failed to register sysfs interfaces\n"); > - goto error_free_ida; > + goto error_ret; > } > ret =3D iio_device_register_eventset(dev_info); > if (ret) { > @@ -1142,8 +1145,6 @@ error_unreg_eventset: > iio_device_unregister_eventset(dev_info); > error_free_sysfs: > iio_device_unregister_sysfs(dev_info); > -error_free_ida: > - ida_simple_remove(&iio_ida, dev_info->id); > error_ret: > return ret; > } =2D-=20 Gru=DF, Manuel Stahl ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] staging:iio: move id and device name setting to iio_device_allocate. 2011-09-12 8:27 ` Manuel Stahl @ 2011-09-12 8:37 ` Jonathan Cameron 0 siblings, 0 replies; 6+ messages in thread From: Jonathan Cameron @ 2011-09-12 8:37 UTC (permalink / raw) To: Manuel Stahl; +Cc: linux-iio On 09/12/11 09:27, Manuel Stahl wrote: > Hi Jonathan, > > the patch works for me, but I still have to search through all triggers and > try to match the names to find the trigger belonging to a device as the > trigger directories are enumerated differently. Yes. That's true whenever you are working with a trigger not associated with with a capture device, so the code has to be there anyway. I'd rather have one path than end up with two, one of which only works sometimes. > > Regards, > Manuel > > Am Freitag, 9. September 2011, 16:56:07 schrieb Jonathan Cameron: >> The recent reorganization of the sysfs attribute registration had >> the side effect of moving iio_device_register after registration of >> triggers etc. The side effect of this is that the id hadn't been >> allocated by the time of trigger registration. Thus all triggers >> based on device got the name <dev_name>-dev0 instead of <dev_name>-devN >> where N is the iio device id. >> >> This should also fix the lack of device name for some error messages >> that we have been seeing (and I'd been meaning to track down) as >> that has now moved earlier as well. >> >> Reported-by: Manuel Stahl <manuel.stahl@iis.fraunhofer.de> >> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk> >> --- >> drivers/staging/iio/industrialio-core.c | 25 +++++++++++++------------ >> 1 files changed, 13 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/staging/iio/industrialio-core.c >> b/drivers/staging/iio/industrialio-core.c index fd4aada..8486914 100644 >> --- a/drivers/staging/iio/industrialio-core.c >> +++ b/drivers/staging/iio/industrialio-core.c >> @@ -1033,6 +1033,15 @@ struct iio_dev *iio_allocate_device(int sizeof_priv) >> device_initialize(&dev->dev); >> dev_set_drvdata(&dev->dev, (void *)dev); >> mutex_init(&dev->mlock); >> + >> + dev->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); >> + if (dev->id < 0) { >> + /* cannot use a dev_err as the name isn't available */ >> + printk(KERN_ERR "Failed to get id\n"); >> + kfree(dev); >> + return NULL; >> + } >> + dev_set_name(&dev->dev, "iio:device%d", dev->id); >> } >> >> return dev; >> @@ -1041,8 +1050,10 @@ EXPORT_SYMBOL(iio_allocate_device); >> >> void iio_free_device(struct iio_dev *dev) >> { >> - if (dev) >> + if (dev) { >> + ida_simple_remove(&iio_ida, dev->id); >> kfree(dev); >> + } >> } >> EXPORT_SYMBOL(iio_free_device); >> >> @@ -1100,14 +1111,6 @@ int iio_device_register(struct iio_dev *dev_info) >> { >> int ret; >> >> - dev_info->id = ida_simple_get(&iio_ida, 0, 0, GFP_KERNEL); >> - if (dev_info->id < 0) { >> - ret = dev_info->id; >> - dev_err(&dev_info->dev, "Failed to get id\n"); >> - goto error_ret; >> - } >> - dev_set_name(&dev_info->dev, "iio:device%d", dev_info->id); >> - >> /* configure elements for the chrdev */ >> dev_info->dev.devt = MKDEV(MAJOR(iio_devt), dev_info->id); >> >> @@ -1115,7 +1118,7 @@ int iio_device_register(struct iio_dev *dev_info) >> if (ret) { >> dev_err(dev_info->dev.parent, >> "Failed to register sysfs interfaces\n"); >> - goto error_free_ida; >> + goto error_ret; >> } >> ret = iio_device_register_eventset(dev_info); >> if (ret) { >> @@ -1142,8 +1145,6 @@ error_unreg_eventset: >> iio_device_unregister_eventset(dev_info); >> error_free_sysfs: >> iio_device_unregister_sysfs(dev_info); >> -error_free_ida: >> - ida_simple_remove(&iio_ida, dev_info->id); >> error_ret: >> return ret; >> } > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-09-12 8:28 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2011-09-09 14:01 [IIO] Mistake in assumption about trigger names Manuel Stahl 2011-09-09 14:40 ` Jonathan Cameron 2011-09-09 14:56 ` [PATCH] staging:iio: move id and device name setting to iio_device_allocate Jonathan Cameron 2011-09-09 14:57 ` Jonathan Cameron 2011-09-12 8:27 ` Manuel Stahl 2011-09-12 8:37 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.