* [RFC] Add frequency support in trigger core
@ 2012-04-19 16:05 Maxime Ripard
2012-04-19 16:05 ` [PATCH 1/2] IIO: Add frequency sysfs files to triggers Maxime Ripard
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Maxime Ripard @ 2012-04-19 16:05 UTC (permalink / raw)
To: linux-iio
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] IIO: Add frequency sysfs files to triggers 2012-04-19 16:05 [RFC] Add frequency support in trigger core Maxime Ripard @ 2012-04-19 16:05 ` Maxime Ripard 2012-04-24 8:19 ` Jonathan Cameron 2012-04-19 16:05 ` [PATCH 2/2] Move RTC trigger to in-core frequency support Maxime Ripard 2012-04-24 8:12 ` [RFC] Add frequency support in trigger core Jonathan Cameron 2 siblings, 1 reply; 9+ messages in thread From: Maxime Ripard @ 2012-04-19 16:05 UTC (permalink / raw) To: linux-iio 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] IIO: Add frequency sysfs files to triggers 2012-04-19 16:05 ` [PATCH 1/2] IIO: Add frequency sysfs files to triggers Maxime Ripard @ 2012-04-24 8:19 ` Jonathan Cameron 2012-04-26 13:48 ` Maxime Ripard 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Cameron @ 2012-04-24 8:19 UTC (permalink / raw) To: Maxime Ripard; +Cc: linux-iio 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; > }; > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] IIO: Add frequency sysfs files to triggers 2012-04-24 8:19 ` Jonathan Cameron @ 2012-04-26 13:48 ` Maxime Ripard 0 siblings, 0 replies; 9+ messages in thread From: Maxime Ripard @ 2012-04-26 13:48 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] Move RTC trigger to in-core frequency support 2012-04-19 16:05 [RFC] Add frequency support in trigger core Maxime Ripard 2012-04-19 16:05 ` [PATCH 1/2] IIO: Add frequency sysfs files to triggers Maxime Ripard @ 2012-04-19 16:05 ` Maxime Ripard 2012-04-24 8:22 ` Jonathan Cameron 2012-04-24 8:12 ` [RFC] Add frequency support in trigger core Jonathan Cameron 2 siblings, 1 reply; 9+ messages in thread From: Maxime Ripard @ 2012-04-19 16:05 UTC (permalink / raw) To: linux-iio 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Move RTC trigger to in-core frequency support 2012-04-19 16:05 ` [PATCH 2/2] Move RTC trigger to in-core frequency support Maxime Ripard @ 2012-04-24 8:22 ` Jonathan Cameron 2012-04-24 8:36 ` Maxime Ripard 0 siblings, 1 reply; 9+ messages in thread From: Jonathan Cameron @ 2012-04-24 8:22 UTC (permalink / raw) To: Maxime Ripard; +Cc: linux-iio 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 */ ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Move RTC trigger to in-core frequency support 2012-04-24 8:22 ` Jonathan Cameron @ 2012-04-24 8:36 ` Maxime Ripard 2012-04-24 9:10 ` Jonathan Cameron 0 siblings, 1 reply; 9+ messages in thread From: Maxime Ripard @ 2012-04-24 8:36 UTC (permalink / raw) To: Jonathan Cameron; +Cc: linux-iio 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] Move RTC trigger to in-core frequency support 2012-04-24 8:36 ` Maxime Ripard @ 2012-04-24 9:10 ` Jonathan Cameron 0 siblings, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2012-04-24 9:10 UTC (permalink / raw) To: Maxime Ripard; +Cc: Jonathan Cameron, linux-iio 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 */ > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [RFC] Add frequency support in trigger core 2012-04-19 16:05 [RFC] Add frequency support in trigger core Maxime Ripard 2012-04-19 16:05 ` [PATCH 1/2] IIO: Add frequency sysfs files to triggers Maxime Ripard 2012-04-19 16:05 ` [PATCH 2/2] Move RTC trigger to in-core frequency support Maxime Ripard @ 2012-04-24 8:12 ` Jonathan Cameron 2 siblings, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2012-04-24 8:12 UTC (permalink / raw) To: Maxime Ripard; +Cc: linux-iio 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2012-04-26 13:48 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-19 16:05 [RFC] Add frequency support in trigger core Maxime Ripard 2012-04-19 16:05 ` [PATCH 1/2] IIO: Add frequency sysfs files to triggers Maxime Ripard 2012-04-24 8:19 ` Jonathan Cameron 2012-04-26 13:48 ` Maxime Ripard 2012-04-19 16:05 ` [PATCH 2/2] Move RTC trigger to in-core frequency support Maxime Ripard 2012-04-24 8:22 ` Jonathan Cameron 2012-04-24 8:36 ` Maxime Ripard 2012-04-24 9:10 ` Jonathan Cameron 2012-04-24 8:12 ` [RFC] Add frequency support in trigger core 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.