On Tue, Feb 19, 2013 at 01:05:42PM +0100, Lars-Peter Clausen wrote:
> On 02/19/2013 02:39 AM, Guenter Roeck wrote:
> > On Mon, Feb 18, 2013 at 02:38:58PM +0100, Lars-Peter Clausen wrote:
> >> This allows a userspace application to poll() on the alarm files to get notified
> >> in case of an temperature threshold event.
> >>
> >> Signed-off-by: Lars-Peter Clausen <lars@xxxxxxxxxx>
> >>
> [...]
> >>
> >> +static irqreturn_t adt7410_irq_handler(int irq, void *private)
> >> +{
> >> + struct device *dev = private;
> >> + int status;
> >> +
> >> + status = adt7410_read_byte(dev, ADT7410_STATUS);
> >> + if (status < 0)
> >> + return IRQ_HANDLED;
> >> +
> >> + if (status & ADT7410_STAT_T_HIGH)
> >> + sysfs_notify(&dev->kobj, NULL, "temp1_max_alarm");
> >> + if (status & ADT7410_STAT_T_LOW)
> >> + sysfs_notify(&dev->kobj, NULL, "temp1_min_alarm");
> >> + if (status & ADT7410_STAT_T_CRIT)
> >> + sysfs_notify(&dev->kobj, NULL, "temp1_crit_alarm");
> >> +
> > Question about semantics: Do you want a notification on all attributes each time
> > one is set during an interrupt, or do you want a notification each time an
> > attribute changes state ? With the above code, the 1->0 transition does not
> > result in a notification. Is that on purpose ?
> >
>
> Now that the code uses an edge triggered IRQ and comparator mode it would
> actually be possible to also generate an event for a 1->0 transition, but
> I'm not sure how much sense that makes. Usually you'd only want to respond
> quickly to the case where a threshold event happens.
>
I am not sure if it would make sense either. I am fine either way - this is
quite new functionality for hwmon, and we can add support for two-way
notification later if it turns out to be needed.
> >> + return IRQ_HANDLED;
> >> +}
> >> +
> >> static int adt7410_temp_ready(struct device *dev)
> >> {
> >> int i, status;
> >> @@ -357,7 +377,7 @@ static const struct attribute_group adt7410_group = {
> >> .attrs = adt7410_attributes,
> >> };
> >>
> >> -int adt7410_probe(struct device *dev, const char *name,
> >> +int adt7410_probe(struct device *dev, const char *name, int irq,
> >> const struct adt7410_ops *ops)
> >> {
> >> struct adt7410_data *data;
> >> @@ -383,11 +403,13 @@ int adt7410_probe(struct device *dev, const char *name,
> >> data->oldconfig = ret;
> >>
> >> /*
> >> - * Set to 16 bit resolution, continous conversion and comparator mode.
> >> + * Set to 16 bit resolution and continous conversion
> >> */
> >> data->config = data->oldconfig;
> >> - data->config &= ~ADT7410_MODE_MASK;
> >> + data->config &= ~(ADT7410_MODE_MASK | ADT7410_CT_POLARITY |
> >> + ADT7410_INT_POLARITY);
> >> data->config |= ADT7410_FULL | ADT7410_RESOLUTION | ADT7410_EVENT_MODE;
> >> +
> >> if (data->config != data->oldconfig) {
> >> ret = adt7410_write_byte(dev, ADT7410_CONFIG, data->config);
> >> if (ret)
> >> @@ -421,8 +443,18 @@ int adt7410_probe(struct device *dev, const char *name,
> >> goto exit_remove_name;
> >> }
> >>
> >> + if (irq > 0) {
> >> + ret = request_threaded_irq(irq, NULL, adt7410_irq_handler,
> >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >> + dev_name(dev), dev);
> >
> > If you use devm_request_threaded_irq, you don't have to release it on remove.
>
> devm_request_threaded_irq is tricky, since the IRQ will only be freed after
> the drivers remove callback has been run. This can cause race conditions,
> since the IRQ handler often access resources already freed in the remove
> callback. In this case it will work fine since sysfs_notify handles the case
> where the files are already gone gracefully and just does nothing in that
> case. I'd still prefer to keep the explicit free, so things are cleaned up
> in the reverse order to the way they were set up.
>
Ok, makes sense.
> >
> >> + if (ret)
> >> + goto exit_hwmon_device_unregister;
> >> + }
> >> +
> >
> > This code should be before hwmon registration.
>
> Why? It doesn't make sense to signal an event before userspace can actually
> discover the device.
>
You are right. Historically we keep hwmon registration as last operation to
avoid user-space confusion if the attributes are gone by the time user-space
tries to access them. Getting a notification prior to registration is just as
bad, and would be "normal" operation instead of error handling, so your code
makes sense.
Thanks,
Guenter
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
