[RFC] Add frequency support in trigger core

[RFC] Add frequency support in trigger core

[Maxime Ripard]

Hi List,

A lot of driver add an extra frequency attribute to allow user to tune
triggers frequency from sysfs. This is a lot of code duplication, while most
do the exact same thing.

I added here two extra fields to struct iio_trigger, frequency and info_mask,
which might not be needed at first, but will allow us to add extra parameters
more easily.

I added as well a callback for info_mask changes in trigger_ops so that drivers
can take action when frequency has been changed by the user, but will also be 
easily extendable.

Maxime

[PATCH 1/2] IIO: Add frequency sysfs files to triggers

[Maxime Ripard]

This allows to easily set the frequency for hardware triggers supporting
it.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 drivers/staging/iio/industrialio-trigger.c |   65 ++++++++++++++++++++++++++-
 drivers/staging/iio/trigger.h              |    9 ++++
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/drivers/staging/iio/industrialio-trigger.c b/drivers/staging/iio/industrialio-trigger.c
index 47ecadd..457ab15 100644
--- a/drivers/staging/iio/industrialio-trigger.c
+++ b/drivers/staging/iio/industrialio-trigger.c
@@ -51,6 +51,42 @@ static ssize_t iio_trigger_read_name(struct device *dev,
 
 static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
 
+static ssize_t iio_trigger_read_frequency(struct device *dev,
+					  struct device_attribute *attr,
+					  char *buf)
+{
+	struct iio_trigger *trig = dev_get_drvdata(dev);
+	return sprintf(buf, "%lu\n", trig->frequency);
+}
+
+static ssize_t iio_trigger_write_frequency(struct device *dev,
+					   struct device_attribute *attr,
+					   const char *buf,
+					   size_t len)
+{
+	struct iio_trigger *trig = dev_get_drvdata(dev);
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 10, &val);
+	if (ret)
+		return ret;
+
+	trig->frequency = val;
+
+	if (trig->ops->update_infos) {
+		ret = trig->ops->update_infos(trig, IIO_TRIGGER_INFO_FREQUENCY);
+		if (ret)
+			return ret;
+	}
+
+	return len;
+}
+
+static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
+		   iio_trigger_read_frequency,
+		   iio_trigger_write_frequency);
+
 /**
  * iio_trigger_register_sysfs() - create a device for this trigger
  * @trig_info:	the trigger
@@ -59,9 +95,29 @@ static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
  **/
 static int iio_trigger_register_sysfs(struct iio_trigger *trig_info)
 {
-	return sysfs_add_file_to_group(&trig_info->dev.kobj,
-				       &dev_attr_name.attr,
-				       NULL);
+	int ret;
+	ret = sysfs_add_file_to_group(&trig_info->dev.kobj,
+				      &dev_attr_name.attr,
+				      NULL);
+	if (ret)
+		goto name_error;
+
+	if (trig_info->info_mask & IIO_TRIGGER_INFO_FREQUENCY) {
+		ret = sysfs_add_file_to_group(&trig_info->dev.kobj,
+					      &dev_attr_frequency.attr,
+					      NULL);
+		if (ret)
+			goto frequency_error;
+	}
+
+	return 0;
+
+frequency_error:
+	sysfs_remove_file_from_group(&trig_info->dev.kobj,
+				     &dev_attr_name.attr,
+				     NULL);
+name_error:
+	return ret;
 }
 
 static void iio_trigger_unregister_sysfs(struct iio_trigger *trig_info)
@@ -69,6 +125,9 @@ static void iio_trigger_unregister_sysfs(struct iio_trigger *trig_info)
 	sysfs_remove_file_from_group(&trig_info->dev.kobj,
 					   &dev_attr_name.attr,
 					   NULL);
+	sysfs_remove_file_from_group(&trig_info->dev.kobj,
+					   &dev_attr_frequency.attr,
+					   NULL);
 }
 
 int iio_trigger_register(struct iio_trigger *trig_info)
diff --git a/drivers/staging/iio/trigger.h b/drivers/staging/iio/trigger.h
index 1cfca23..a258ef8 100644
--- a/drivers/staging/iio/trigger.h
+++ b/drivers/staging/iio/trigger.h
@@ -16,6 +16,8 @@ struct iio_subirq {
 	bool enabled;
 };
 
+#define IIO_TRIGGER_INFO_FREQUENCY	(1 << 0)
+
 /**
  * struct iio_trigger_ops - operations structure for an iio_trigger.
  * @owner:		used to monitor usage count of the trigger.
@@ -32,6 +34,8 @@ struct iio_trigger_ops {
 	struct module			*owner;
 	int (*set_trigger_state)(struct iio_trigger *trig, bool state);
 	int (*try_reenable)(struct iio_trigger *trig);
+	int (*update_infos)(struct iio_trigger *trig,
+			    const unsigned long info_mask);
 	int (*validate_device)(struct iio_trigger *trig,
 			       struct iio_dev *indio_dev);
 };
@@ -52,6 +56,9 @@ struct iio_trigger_ops {
  * @subirqs:		[INTERN] information about the 'child' irqs.
  * @pool:		[INTERN] bitmap of irqs currently in use.
  * @pool_lock:		[INTERN] protection of the irq pool.
+ * @frequency:		[DRIVER] frequency of the trigger in Hz.
+ * @info_mask:		[DRIVER] What informations are to be exported in the
+ *			sysfs directory: frequency, etc....
  **/
 struct iio_trigger {
 	const struct iio_trigger_ops	*ops;
@@ -70,6 +77,8 @@ struct iio_trigger {
 	struct iio_subirq subirqs[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
 	unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)];
 	struct mutex			pool_lock;
+	unsigned long			frequency;
+	long				info_mask;
 };
 
 
-- 
1.7.5.4

Re: [PATCH 1/2] IIO: Add frequency sysfs files to triggers

[Jonathan Cameron]

On 4/19/2012 5:05 PM, Maxime Ripard wrote:
> This allows to easily set the frequency for hardware triggers supporting
> it.
Other than the missing docs for the new callback this is fine.
I've highlighted some future issues inline but we can tackle them later.

Ultimately from a maintenance point of view I'd like these to look more 
similar
to the ones we have in the core (read_raw and write_raw). Those are far more
flexible than we currently need here but that may change.

I'm happy to have this as a stop gap
>
> Signed-off-by: Maxime Ripard<maxime.ripard@free-electrons.com>
> ---
>   drivers/staging/iio/industrialio-trigger.c |   65 ++++++++++++++++++++++++++-
>   drivers/staging/iio/trigger.h              |    9 ++++
>   2 files changed, 71 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/staging/iio/industrialio-trigger.c b/drivers/staging/iio/industrialio-trigger.c
> index 47ecadd..457ab15 100644
> --- a/drivers/staging/iio/industrialio-trigger.c
> +++ b/drivers/staging/iio/industrialio-trigger.c
> @@ -51,6 +51,42 @@ static ssize_t iio_trigger_read_name(struct device *dev,
>
>   static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
>
> +static ssize_t iio_trigger_read_frequency(struct device *dev,
> +					  struct device_attribute *attr,
> +					  char *buf)
> +{
> +	struct iio_trigger *trig = dev_get_drvdata(dev);
> +	return sprintf(buf, "%lu\n", trig->frequency);
Strong assumption of integer value here.  I'd not be suprised if we get
fractional values on a few devices over time.  Maybe that's something to
fix when we do though!
> +}
> +
> +static ssize_t iio_trigger_write_frequency(struct device *dev,
> +					   struct device_attribute *attr,
> +					   const char *buf,
> +					   size_t len)
> +{
> +	struct iio_trigger *trig = dev_get_drvdata(dev);
> +	unsigned long val;
> +	int ret;
> +
> +	ret = kstrtoul(buf, 10,&val);
> +	if (ret)
> +		return ret;
> +
> +	trig->frequency = val;
> +
> +	if (trig->ops->update_infos) {
> +		ret = trig->ops->update_infos(trig, IIO_TRIGGER_INFO_FREQUENCY);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return len;
> +}
> +
> +static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
> +		   iio_trigger_read_frequency,
> +		   iio_trigger_write_frequency);
> +
>   /**
>    * iio_trigger_register_sysfs() - create a device for this trigger
>    * @trig_info:	the trigger
> @@ -59,9 +95,29 @@ static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
>    **/
>   static int iio_trigger_register_sysfs(struct iio_trigger *trig_info)
>   {
Doesn't change the fundamental path here, so fine.  Note however, that at
somepoint we should ensure these are registered with the device so that we
are sure udev will get notification of their creation...
> -	return sysfs_add_file_to_group(&trig_info->dev.kobj,
> -				&dev_attr_name.attr,
> -				       NULL);
> +	int ret;
> +	ret = sysfs_add_file_to_group(&trig_info->dev.kobj,
> +				&dev_attr_name.attr,
> +				      NULL);
> +	if (ret)
> +		goto name_error;
> +
> +	if (trig_info->info_mask&  IIO_TRIGGER_INFO_FREQUENCY) {
> +		ret = sysfs_add_file_to_group(&trig_info->dev.kobj,
> +					&dev_attr_frequency.attr,
> +					      NULL);
> +		if (ret)
> +			goto frequency_error;
> +	}
> +
> +	return 0;
> +
> +frequency_error:
> +	sysfs_remove_file_from_group(&trig_info->dev.kobj,
> +				&dev_attr_name.attr,
> +				     NULL);
> +name_error:
> +	return ret;
>   }
>
>   static void iio_trigger_unregister_sysfs(struct iio_trigger *trig_info)
> @@ -69,6 +125,9 @@ static void iio_trigger_unregister_sysfs(struct iio_trigger *trig_info)
>   	sysfs_remove_file_from_group(&trig_info->dev.kobj,
>   					&dev_attr_name.attr,
>   					   NULL);
> +	sysfs_remove_file_from_group(&trig_info->dev.kobj,
> +					&dev_attr_frequency.attr,
> +					   NULL);
>   }
>
>   int iio_trigger_register(struct iio_trigger *trig_info)
> diff --git a/drivers/staging/iio/trigger.h b/drivers/staging/iio/trigger.h
> index 1cfca23..a258ef8 100644
> --- a/drivers/staging/iio/trigger.h
> +++ b/drivers/staging/iio/trigger.h
> @@ -16,6 +16,8 @@ struct iio_subirq {
>   	bool enabled;
>   };
>   
Slight preference for BIT(0)
> +#define IIO_TRIGGER_INFO_FREQUENCY	(1<<  0)
> +
>   /**
>    * struct iio_trigger_ops - operations structure for an iio_trigger.
>    * @owner:		used to monitor usage count of the trigger.
Document the new callback please!
> @@ -32,6 +34,8 @@ struct iio_trigger_ops {
>   	struct module			*owner;
>   	int (*set_trigger_state)(struct iio_trigger *trig, bool state);
>   	int (*try_reenable)(struct iio_trigger *trig);
> +	int (*update_infos)(struct iio_trigger *trig,
> +			    const unsigned long info_mask);
>   	int (*validate_device)(struct iio_trigger *trig,
>   			       struct iio_dev *indio_dev);
>   };
> @@ -52,6 +56,9 @@ struct iio_trigger_ops {
>    * @subirqs:		[INTERN] information about the 'child' irqs.
>    * @pool:		[INTERN] bitmap of irqs currently in use.
>    * @pool_lock:		[INTERN] protection of the irq pool.
> + * @frequency:		[DRIVER] frequency of the trigger in Hz.
> + * @info_mask:		[DRIVER] What informations are to be exported in the
> + *			sysfs directory: frequency, etc....
>    **/
>   struct iio_trigger {
>   	const struct iio_trigger_ops	*ops;
> @@ -70,6 +77,8 @@ struct iio_trigger {
>   	struct iio_subirq subirqs[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
>   	unsigned long pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)];
>   	struct mutex			pool_lock;
> +	unsigned long			frequency;
> +	long				info_mask;
>   };
>
>

Re: [PATCH 1/2] IIO: Add frequency sysfs files to triggers

[Maxime Ripard]

Le 24/04/2012 10:19, Jonathan Cameron a =E9crit :
> On 4/19/2012 5:05 PM, Maxime Ripard wrote:
>> This allows to easily set the frequency for hardware triggers suppor=
ting
>> it.
> Other than the missing docs for the new callback this is fine.

You're right, I will add it and send a new patch.

> I've highlighted some future issues inline but we can tackle them lat=
er.
>=20
> Ultimately from a maintenance point of view I'd like these to look mo=
re
> similar
> to the ones we have in the core (read_raw and write_raw). Those are f=
ar
> more
> flexible than we currently need here but that may change.

You mean with the various units multipliers ?

> I'm happy to have this as a stop gap

Great :)

>>
>> Signed-off-by: Maxime Ripard<maxime.ripard@free-electrons.com>
>> ---
>>   drivers/staging/iio/industrialio-trigger.c |   65
>> ++++++++++++++++++++++++++-
>>   drivers/staging/iio/trigger.h              |    9 ++++
>>   2 files changed, 71 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/staging/iio/industrialio-trigger.c
>> b/drivers/staging/iio/industrialio-trigger.c
>> index 47ecadd..457ab15 100644
>> --- a/drivers/staging/iio/industrialio-trigger.c
>> +++ b/drivers/staging/iio/industrialio-trigger.c
>> @@ -51,6 +51,42 @@ static ssize_t iio_trigger_read_name(struct devic=
e
>> *dev,
>>
>>   static DEVICE_ATTR(name, S_IRUGO, iio_trigger_read_name, NULL);
>>
>> +static ssize_t iio_trigger_read_frequency(struct device *dev,
>> +                      struct device_attribute *attr,
>> +                      char *buf)
>> +{
>> +    struct iio_trigger *trig =3D dev_get_drvdata(dev);
>> +    return sprintf(buf, "%lu\n", trig->frequency);
> Strong assumption of integer value here.  I'd not be suprised if we g=
et
> fractional values on a few devices over time.  Maybe that's something=
 to
> fix when we do though!

Yeah, I was thinking at first to use Hz here, and that should only impl=
y
having integers because of the difference of scale, but maybe floats
would be nice too. I wonder if someone ever needs to do conversions at
such a low frequency, but I guess that a conversion every 2 seconds
doesn't look too unfamiliar for example.

>> +}
>> +
>> +static ssize_t iio_trigger_write_frequency(struct device *dev,
>> +                       struct device_attribute *attr,
>> +                       const char *buf,
>> +                       size_t len)
>> +{
>> +    struct iio_trigger *trig =3D dev_get_drvdata(dev);
>> +    unsigned long val;
>> +    int ret;
>> +
>> +    ret =3D kstrtoul(buf, 10,&val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    trig->frequency =3D val;
>> +
>> +    if (trig->ops->update_infos) {
>> +        ret =3D trig->ops->update_infos(trig, IIO_TRIGGER_INFO_FREQ=
UENCY);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>> +    return len;
>> +}
>> +
>> +static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
>> +           iio_trigger_read_frequency,
>> +           iio_trigger_write_frequency);
>> +
>>   /**
>>    * iio_trigger_register_sysfs() - create a device for this trigger
>>    * @trig_info:    the trigger
>> @@ -59,9 +95,29 @@ static DEVICE_ATTR(name, S_IRUGO,
>> iio_trigger_read_name, NULL);
>>    **/
>>   static int iio_trigger_register_sysfs(struct iio_trigger *trig_inf=
o)
>>   {
> Doesn't change the fundamental path here, so fine.  Note however, tha=
t at
> somepoint we should ensure these are registered with the device so th=
at we
> are sure udev will get notification of their creation...
>> -    return sysfs_add_file_to_group(&trig_info->dev.kobj,
>> -                &dev_attr_name.attr,
>> -                       NULL);
>> +    int ret;
>> +    ret =3D sysfs_add_file_to_group(&trig_info->dev.kobj,
>> +                &dev_attr_name.attr,
>> +                      NULL);
>> +    if (ret)
>> +        goto name_error;
>> +
>> +    if (trig_info->info_mask&  IIO_TRIGGER_INFO_FREQUENCY) {
>> +        ret =3D sysfs_add_file_to_group(&trig_info->dev.kobj,
>> +                    &dev_attr_frequency.attr,
>> +                          NULL);
>> +        if (ret)
>> +            goto frequency_error;
>> +    }
>> +
>> +    return 0;
>> +
>> +frequency_error:
>> +    sysfs_remove_file_from_group(&trig_info->dev.kobj,
>> +                &dev_attr_name.attr,
>> +                     NULL);
>> +name_error:
>> +    return ret;
>>   }
>>
>>   static void iio_trigger_unregister_sysfs(struct iio_trigger *trig_=
info)
>> @@ -69,6 +125,9 @@ static void iio_trigger_unregister_sysfs(struct
>> iio_trigger *trig_info)
>>       sysfs_remove_file_from_group(&trig_info->dev.kobj,
>>                       &dev_attr_name.attr,
>>                          NULL);
>> +    sysfs_remove_file_from_group(&trig_info->dev.kobj,
>> +                    &dev_attr_frequency.attr,
>> +                       NULL);
>>   }
>>
>>   int iio_trigger_register(struct iio_trigger *trig_info)
>> diff --git a/drivers/staging/iio/trigger.h
>> b/drivers/staging/iio/trigger.h
>> index 1cfca23..a258ef8 100644
>> --- a/drivers/staging/iio/trigger.h
>> +++ b/drivers/staging/iio/trigger.h
>> @@ -16,6 +16,8 @@ struct iio_subirq {
>>       bool enabled;
>>   };
>>  =20
> Slight preference for BIT(0)
ACK.
>> +#define IIO_TRIGGER_INFO_FREQUENCY    (1<<  0)
>> +
>>   /**
>>    * struct iio_trigger_ops - operations structure for an iio_trigge=
r.
>>    * @owner:        used to monitor usage count of the trigger.
> Document the new callback please!
Yes, will do :)
>> @@ -32,6 +34,8 @@ struct iio_trigger_ops {
>>       struct module            *owner;
>>       int (*set_trigger_state)(struct iio_trigger *trig, bool state)=
;
>>       int (*try_reenable)(struct iio_trigger *trig);
>> +    int (*update_infos)(struct iio_trigger *trig,
>> +                const unsigned long info_mask);
>>       int (*validate_device)(struct iio_trigger *trig,
>>                      struct iio_dev *indio_dev);
>>   };
>> @@ -52,6 +56,9 @@ struct iio_trigger_ops {
>>    * @subirqs:        [INTERN] information about the 'child' irqs.
>>    * @pool:        [INTERN] bitmap of irqs currently in use.
>>    * @pool_lock:        [INTERN] protection of the irq pool.
>> + * @frequency:        [DRIVER] frequency of the trigger in Hz.
>> + * @info_mask:        [DRIVER] What informations are to be exported
>> in the
>> + *            sysfs directory: frequency, etc....
>>    **/
>>   struct iio_trigger {
>>       const struct iio_trigger_ops    *ops;
>> @@ -70,6 +77,8 @@ struct iio_trigger {
>>       struct iio_subirq subirqs[CONFIG_IIO_CONSUMERS_PER_TRIGGER];
>>       unsigned long
>> pool[BITS_TO_LONGS(CONFIG_IIO_CONSUMERS_PER_TRIGGER)];
>>       struct mutex            pool_lock;
>> +    unsigned long            frequency;
>> +    long                info_mask;
>>   };
>>
>>
>=20


--=20
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/2] Move RTC trigger to in-core frequency support

[Maxime Ripard]

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 .../staging/iio/trigger/iio-trig-periodic-rtc.c    |   50 ++++----------------
 1 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
index a80cf67..ffabf80 100644
--- a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
+++ b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
@@ -24,7 +24,6 @@ static DEFINE_MUTEX(iio_prtc_trigger_list_lock);
 
 struct iio_prtc_trigger_info {
 	struct rtc_device *rtc;
-	int frequency;
 	struct rtc_task task;
 };
 
@@ -37,50 +36,19 @@ static int iio_trig_periodic_rtc_set_state(struct iio_trigger *trig, bool state)
 	return rtc_irq_set_state(trig_info->rtc, &trig_info->task, state);
 }
 
-static ssize_t iio_trig_periodic_read_freq(struct device *dev,
-					   struct device_attribute *attr,
-					   char *buf)
+static int update_infos(struct iio_trigger *trig, const unsigned long mask)
 {
-	struct iio_trigger *trig = dev_get_drvdata(dev);
 	struct iio_prtc_trigger_info *trig_info = trig->private_data;
-	return sprintf(buf, "%u\n", trig_info->frequency);
-}
-
-static ssize_t iio_trig_periodic_write_freq(struct device *dev,
-					    struct device_attribute *attr,
-					    const char *buf,
-					    size_t len)
-{
-	struct iio_trigger *trig = dev_get_drvdata(dev);
-	struct iio_prtc_trigger_info *trig_info = trig->private_data;
-	unsigned long val;
-	int ret;
-
-	ret = strict_strtoul(buf, 10, &val);
-	if (ret)
-		goto error_ret;
-
-	ret = rtc_irq_set_freq(trig_info->rtc, &trig_info->task, val);
-	if (ret)
-		goto error_ret;
-
-	trig_info->frequency = val;
 
-	return len;
-
-error_ret:
-	return ret;
+	switch (mask) {
+	case IIO_TRIGGER_INFO_FREQUENCY:
+		return rtc_irq_set_freq(trig_info->rtc, &trig_info->task,
+					trig->frequency);
+	default:
+		return 0;
+	}
 }
 
-static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
-	    iio_trig_periodic_read_freq,
-	    iio_trig_periodic_write_freq);
-
-static struct attribute *iio_trig_prtc_attrs[] = {
-	&dev_attr_frequency.attr,
-	NULL,
-};
-
 static const struct attribute_group iio_trig_prtc_attr_group = {
 	.attrs = iio_trig_prtc_attrs,
 };
@@ -99,6 +67,7 @@ static void iio_prtc_trigger_poll(void *private_data)
 static const struct iio_trigger_ops iio_prtc_trigger_ops = {
 	.owner = THIS_MODULE,
 	.set_trigger_state = &iio_trig_periodic_rtc_set_state,
+	.update_infos = &iio_trig_periodic_rtc_update_infos,
 };
 
 static int iio_trig_periodic_rtc_probe(struct platform_device *dev)
@@ -124,6 +93,7 @@ static int iio_trig_periodic_rtc_probe(struct platform_device *dev)
 			ret = -ENOMEM;
 			goto error_put_trigger_and_remove_from_list;
 		}
+		trig->info_mask = IIO_TRIGGER_INFO_FREQUENCY;
 		trig->private_data = trig_info;
 		trig->ops = &iio_prtc_trigger_ops;
 		/* RTC access */
-- 
1.7.5.4

Re: [PATCH 2/2] Move RTC trigger to in-core frequency support

[Jonathan Cameron]

On 4/19/2012 5:05 PM, Maxime Ripard wrote:

The irony here is that I was going to suggest we droped this driver entirely
as I didn't think anyone was using it!  Guess they are given you've gone
to the effort of cleaning it up.

One trivial change inline. Otherwise looks good.
> Signed-off-by: Maxime Ripard<maxime.ripard@free-electrons.com>
> ---
>   .../staging/iio/trigger/iio-trig-periodic-rtc.c    |   50 ++++----------------
>   1 files changed, 10 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
> index a80cf67..ffabf80 100644
> --- a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
> +++ b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
> @@ -24,7 +24,6 @@ static DEFINE_MUTEX(iio_prtc_trigger_list_lock);
>
>   struct iio_prtc_trigger_info {
>   	struct rtc_device *rtc;
> -	int frequency;
>   	struct rtc_task task;
>   };
>
> @@ -37,50 +36,19 @@ static int iio_trig_periodic_rtc_set_state(struct iio_trigger *trig, bool state)
>   	return rtc_irq_set_state(trig_info->rtc,&trig_info->task, state);
>   }
>
> -static ssize_t iio_trig_periodic_read_freq(struct device *dev,
> -					   struct device_attribute *attr,
> -					   char *buf)
> +static int update_infos(struct iio_trigger *trig, const unsigned long mask)
>   {
> -	struct iio_trigger *trig = dev_get_drvdata(dev);
>   	struct iio_prtc_trigger_info *trig_info = trig->private_data;
> -	return sprintf(buf, "%u\n", trig_info->frequency);
> -}
> -
> -static ssize_t iio_trig_periodic_write_freq(struct device *dev,
> -					    struct device_attribute *attr,
> -					    const char *buf,
> -					    size_t len)
> -{
> -	struct iio_trigger *trig = dev_get_drvdata(dev);
> -	struct iio_prtc_trigger_info *trig_info = trig->private_data;
> -	unsigned long val;
> -	int ret;
> -
> -	ret = strict_strtoul(buf, 10,&val);
> -	if (ret)
> -		goto error_ret;
> -
> -	ret = rtc_irq_set_freq(trig_info->rtc,&trig_info->task, val);
> -	if (ret)
> -		goto error_ret;
> -
> -	trig_info->frequency = val;
>
> -	return len;
> -
> -error_ret:
> -	return ret;
> +	switch (mask) {
> +	case IIO_TRIGGER_INFO_FREQUENCY:
> +		return rtc_irq_set_freq(trig_info->rtc,&trig_info->task,
> +					trig->frequency);
> +	default:
return -EINVAL.  Any other write is an error, not a clean return. Of course
it can't happen, but still best to be clear we aren't happy if it does!
> +		return 0;
> +	}
>   }
>
> -static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
> -	    iio_trig_periodic_read_freq,
> -	    iio_trig_periodic_write_freq);
> -
> -static struct attribute *iio_trig_prtc_attrs[] = {
> -	&dev_attr_frequency.attr,
> -	NULL,
> -};
> -
>   static const struct attribute_group iio_trig_prtc_attr_group = {
>   	.attrs = iio_trig_prtc_attrs,
>   };
> @@ -99,6 +67,7 @@ static void iio_prtc_trigger_poll(void *private_data)
>   static const struct iio_trigger_ops iio_prtc_trigger_ops = {
>   	.owner = THIS_MODULE,
>   	.set_trigger_state =&iio_trig_periodic_rtc_set_state,
> +	.update_infos =&iio_trig_periodic_rtc_update_infos,
>   };
>
>   static int iio_trig_periodic_rtc_probe(struct platform_device *dev)
> @@ -124,6 +93,7 @@ static int iio_trig_periodic_rtc_probe(struct platform_device *dev)
>   			ret = -ENOMEM;
>   			goto error_put_trigger_and_remove_from_list;
>   		}
> +		trig->info_mask = IIO_TRIGGER_INFO_FREQUENCY;
>   		trig->private_data = trig_info;
>   		trig->ops =&iio_prtc_trigger_ops;
>   		/* RTC access */

Re: [PATCH 2/2] Move RTC trigger to in-core frequency support

[Maxime Ripard]

Hi,

Le 24/04/2012 10:22, Jonathan Cameron a =E9crit :
> On 4/19/2012 5:05 PM, Maxime Ripard wrote:
>=20
> The irony here is that I was going to suggest we droped this driver
> entirely
> as I didn't think anyone was using it!  Guess they are given you've g=
one
> to the effort of cleaning it up.
>=20
> One trivial change inline. Otherwise looks good.

Well, actually, you still can. I picked a random driver that implemente=
d
the frequency stuff to see what I needed and what I thought was a good
driver API.

I could have been any other driver doing this :)

>> Signed-off-by: Maxime Ripard<maxime.ripard@free-electrons.com>
>> ---
>>   .../staging/iio/trigger/iio-trig-periodic-rtc.c    |   50
>> ++++----------------
>>   1 files changed, 10 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>> b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>> index a80cf67..ffabf80 100644
>> --- a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>> +++ b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>> @@ -24,7 +24,6 @@ static DEFINE_MUTEX(iio_prtc_trigger_list_lock);
>>
>>   struct iio_prtc_trigger_info {
>>       struct rtc_device *rtc;
>> -    int frequency;
>>       struct rtc_task task;
>>   };
>>
>> @@ -37,50 +36,19 @@ static int iio_trig_periodic_rtc_set_state(struc=
t
>> iio_trigger *trig, bool state)
>>       return rtc_irq_set_state(trig_info->rtc,&trig_info->task, stat=
e);
>>   }
>>
>> -static ssize_t iio_trig_periodic_read_freq(struct device *dev,
>> -                       struct device_attribute *attr,
>> -                       char *buf)
>> +static int update_infos(struct iio_trigger *trig, const unsigned lo=
ng
>> mask)
>>   {
>> -    struct iio_trigger *trig =3D dev_get_drvdata(dev);
>>       struct iio_prtc_trigger_info *trig_info =3D trig->private_data=
;
>> -    return sprintf(buf, "%u\n", trig_info->frequency);
>> -}
>> -
>> -static ssize_t iio_trig_periodic_write_freq(struct device *dev,
>> -                        struct device_attribute *attr,
>> -                        const char *buf,
>> -                        size_t len)
>> -{
>> -    struct iio_trigger *trig =3D dev_get_drvdata(dev);
>> -    struct iio_prtc_trigger_info *trig_info =3D trig->private_data;
>> -    unsigned long val;
>> -    int ret;
>> -
>> -    ret =3D strict_strtoul(buf, 10,&val);
>> -    if (ret)
>> -        goto error_ret;
>> -
>> -    ret =3D rtc_irq_set_freq(trig_info->rtc,&trig_info->task, val);
>> -    if (ret)
>> -        goto error_ret;
>> -
>> -    trig_info->frequency =3D val;
>>
>> -    return len;
>> -
>> -error_ret:
>> -    return ret;
>> +    switch (mask) {
>> +    case IIO_TRIGGER_INFO_FREQUENCY:
>> +        return rtc_irq_set_freq(trig_info->rtc,&trig_info->task,
>> +                    trig->frequency);
>> +    default:
> return -EINVAL.  Any other write is an error, not a clean return. Of =
course
> it can't happen, but still best to be clear we aren't happy if it doe=
s!
>> +        return 0;
>> +    }
>>   }
>>
>> -static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
>> -        iio_trig_periodic_read_freq,
>> -        iio_trig_periodic_write_freq);
>> -
>> -static struct attribute *iio_trig_prtc_attrs[] =3D {
>> -    &dev_attr_frequency.attr,
>> -    NULL,
>> -};
>> -
>>   static const struct attribute_group iio_trig_prtc_attr_group =3D {
>>       .attrs =3D iio_trig_prtc_attrs,
>>   };
>> @@ -99,6 +67,7 @@ static void iio_prtc_trigger_poll(void *private_da=
ta)
>>   static const struct iio_trigger_ops iio_prtc_trigger_ops =3D {
>>       .owner =3D THIS_MODULE,
>>       .set_trigger_state =3D&iio_trig_periodic_rtc_set_state,
>> +    .update_infos =3D&iio_trig_periodic_rtc_update_infos,
>>   };
>>
>>   static int iio_trig_periodic_rtc_probe(struct platform_device *dev=
)
>> @@ -124,6 +93,7 @@ static int iio_trig_periodic_rtc_probe(struct
>> platform_device *dev)
>>               ret =3D -ENOMEM;
>>               goto error_put_trigger_and_remove_from_list;
>>           }
>> +        trig->info_mask =3D IIO_TRIGGER_INFO_FREQUENCY;
>>           trig->private_data =3D trig_info;
>>           trig->ops =3D&iio_prtc_trigger_ops;
>>           /* RTC access */
>=20


--=20
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 2/2] Move RTC trigger to in-core frequency support

[Jonathan Cameron]

On 4/24/2012 9:36 AM, Maxime Ripard wrote:
> Hi,
>
> Le 24/04/2012 10:22, Jonathan Cameron a =E9crit :
>> On 4/19/2012 5:05 PM, Maxime Ripard wrote:
>>
>> The irony here is that I was going to suggest we droped this driver
>> entirely
>> as I didn't think anyone was using it!  Guess they are given you've go=
ne
>> to the effort of cleaning it up.
>>
>> One trivial change inline. Otherwise looks good.
> Well, actually, you still can. I picked a random driver that implemente=
d
> the frequency stuff to see what I needed and what I thought was a good
> driver API.
>
> I could have been any other driver doing this :)
Fair enough.  For some parts the periodic rtc stuff is emulated using=20
high resolution
timers anyway hence I'm not sure it serves any real purpose any more.
>
>>> Signed-off-by: Maxime Ripard<maxime.ripard@free-electrons.com>
>>> ---
>>>    .../staging/iio/trigger/iio-trig-periodic-rtc.c    |   50
>>> ++++----------------
>>>    1 files changed, 10 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>>> b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>>> index a80cf67..ffabf80 100644
>>> --- a/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>>> +++ b/drivers/staging/iio/trigger/iio-trig-periodic-rtc.c
>>> @@ -24,7 +24,6 @@ static DEFINE_MUTEX(iio_prtc_trigger_list_lock);
>>>
>>>    struct iio_prtc_trigger_info {
>>>        struct rtc_device *rtc;
>>> -    int frequency;
>>>        struct rtc_task task;
>>>    };
>>>
>>> @@ -37,50 +36,19 @@ static int iio_trig_periodic_rtc_set_state(struct
>>> iio_trigger *trig, bool state)
>>>        return rtc_irq_set_state(trig_info->rtc,&trig_info->task, stat=
e);
>>>    }
>>>
>>> -static ssize_t iio_trig_periodic_read_freq(struct device *dev,
>>> -                       struct device_attribute *attr,
>>> -                       char *buf)
>>> +static int update_infos(struct iio_trigger *trig, const unsigned lon=
g
>>> mask)
>>>    {
>>> -    struct iio_trigger *trig =3D dev_get_drvdata(dev);
>>>        struct iio_prtc_trigger_info *trig_info =3D trig->private_data=
;
>>> -    return sprintf(buf, "%u\n", trig_info->frequency);
>>> -}
>>> -
>>> -static ssize_t iio_trig_periodic_write_freq(struct device *dev,
>>> -                        struct device_attribute *attr,
>>> -                        const char *buf,
>>> -                        size_t len)
>>> -{
>>> -    struct iio_trigger *trig =3D dev_get_drvdata(dev);
>>> -    struct iio_prtc_trigger_info *trig_info =3D trig->private_data;
>>> -    unsigned long val;
>>> -    int ret;
>>> -
>>> -    ret =3D strict_strtoul(buf, 10,&val);
>>> -    if (ret)
>>> -        goto error_ret;
>>> -
>>> -    ret =3D rtc_irq_set_freq(trig_info->rtc,&trig_info->task, val);
>>> -    if (ret)
>>> -        goto error_ret;
>>> -
>>> -    trig_info->frequency =3D val;
>>>
>>> -    return len;
>>> -
>>> -error_ret:
>>> -    return ret;
>>> +    switch (mask) {
>>> +    case IIO_TRIGGER_INFO_FREQUENCY:
>>> +        return rtc_irq_set_freq(trig_info->rtc,&trig_info->task,
>>> +                    trig->frequency);
>>> +    default:
>> return -EINVAL.  Any other write is an error, not a clean return. Of c=
ourse
>> it can't happen, but still best to be clear we aren't happy if it does=
!
>>> +        return 0;
>>> +    }
>>>    }
>>>
>>> -static DEVICE_ATTR(frequency, S_IRUGO | S_IWUSR,
>>> -        iio_trig_periodic_read_freq,
>>> -        iio_trig_periodic_write_freq);
>>> -
>>> -static struct attribute *iio_trig_prtc_attrs[] =3D {
>>> -&dev_attr_frequency.attr,
>>> -    NULL,
>>> -};
>>> -
>>>    static const struct attribute_group iio_trig_prtc_attr_group =3D {
>>>        .attrs =3D iio_trig_prtc_attrs,
>>>    };
>>> @@ -99,6 +67,7 @@ static void iio_prtc_trigger_poll(void *private_dat=
a)
>>>    static const struct iio_trigger_ops iio_prtc_trigger_ops =3D {
>>>        .owner =3D THIS_MODULE,
>>>        .set_trigger_state =3D&iio_trig_periodic_rtc_set_state,
>>> +    .update_infos =3D&iio_trig_periodic_rtc_update_infos,
>>>    };
>>>
>>>    static int iio_trig_periodic_rtc_probe(struct platform_device *dev=
)
>>> @@ -124,6 +93,7 @@ static int iio_trig_periodic_rtc_probe(struct
>>> platform_device *dev)
>>>                ret =3D -ENOMEM;
>>>                goto error_put_trigger_and_remove_from_list;
>>>            }
>>> +        trig->info_mask =3D IIO_TRIGGER_INFO_FREQUENCY;
>>>            trig->private_data =3D trig_info;
>>>            trig->ops =3D&iio_prtc_trigger_ops;
>>>            /* RTC access */
>

Re: [RFC] Add frequency support in trigger core

[Jonathan Cameron]

On 4/19/2012 5:05 PM, Maxime Ripard wrote:
> Hi List,
>
> A lot of driver add an extra frequency attribute to allow user to tune
> triggers frequency from sysfs. This is a lot of code duplication, while most
> do the exact same thing.
>
> I added here two extra fields to struct iio_trigger, frequency and info_mask,
> which might not be needed at first, but will allow us to add extra parameters
> more easily.
>
> I added as well a callback for info_mask changes in trigger_ops so that drivers
> can take action when frequency has been changed by the user, but will also be
> easily extendable.
I'm generally in favour of this as it'll make in kernel accesses to 
these controls easier to add.