All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.