Re: [RFC 6/8] iio: bmc150: introduce bmc150_accel_trigger

[Hartmut Knaack <knaack.h@gmx.de>]

Octavian Purdila schrieb am 24.11.2014 11:42:
> On Mon, Nov 24, 2014 at 1:06 AM, Hartmut Knaack <knaack.h@gmx.de> wrote:
>>> +
>>> +static void bmc150_accel_unregister_triggers(struct bmc150_accel_data *data,
>>> +                                          int from)
>>> +{
>>> +     int i;
>>> +
>>> +     for (i = from; i >= 0; i++) {
>>> +             if (data->triggers[i].indio_trig) {
>>> +                     iio_trigger_unregister(data->triggers[i].indio_trig);
>>> +                     data->triggers[i].indio_trig = NULL;
>> Better use devm_iio_trigger_free()?
> 
> Wouldn't that be called anyway a little bit later?
I thought it would be cleaner/safer to release it using the opposite of devm_iio_trigger_alloc(), instead of changing its reference to NULL.
> 
>>> +             }
>>> +             i--;
>>> +     }
>> i++ and i-- in the same loop looks like an infinite loop to me. Not to mention what happens on the second attempt to unregister the same trigger.
> 
> Oops.
> 
>>> +}
>>> +
>>> +static int bmc150_accel_triggers_setup(struct iio_dev *indio_dev,
>>> +                                    struct bmc150_accel_data *data)
>>> +{
>>> +     int i, ret;
>>> +
>>> +     for (i = 0; i < BMC150_ACCEL_TRIGGERS; i++) {
>>> +             struct bmc150_accel_trigger *t = &data->triggers[i];
>>> +             const char *name = bmc150_accel_triggers[i].name;
>>> +             int intr = bmc150_accel_triggers[i].intr;
>>> +
>>> +             t->indio_trig = devm_iio_trigger_alloc(&data->client->dev, name,
>>> +                                                    indio_dev->name,
>>> +                                                    indio_dev->id);
>>> +             if (!t->indio_trig) {
>>> +                     ret = -ENOMEM;
>>> +                     break;
>>> +             }
>>> +
>>> +             t->indio_trig->dev.parent = &data->client->dev;
>>> +             t->indio_trig->ops = &bmc150_accel_trigger_ops;
>>> +             t->intr = &data->interrupts[intr];
>>> +             t->data = data;
>>> +             iio_trigger_set_drvdata(t->indio_trig, t);
>>> +
>>> +             ret = iio_trigger_register(t->indio_trig);
>>> +             if (ret)
>>> +                     break;
>>> +     }
>>> +
>>> +     if (ret)
>>> +             bmc150_accel_unregister_triggers(data, i);
>> I think this should be called with i - 1.
> 
> You're right.
> 
> Thanks for the reviews Hartmut, I will send a new version after we
> decide on the hardware buffer approach.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>