| Message ID | 20250128120100.205523-1-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: accel: adxl345: add interrupt based sensor events | expand |
On Tue, 28 Jan 2025 12:00:48 +0000 Lothar Rubusch <l.rubusch@gmail.com> wrote: > Add several interrupt based sensor detection events: > - single tap > - double tap > - free fall > - activity > - inactivity > > All the needed parameters for each and methods of adjusting them, and > forward a resulting IIO event for each to the IIO channel. So my main feedback here is to be much more reluctant to add new ABI. Anything you add is unused by all existing code and if it is unique to a driver probably never going to be used by anyone other than you. We have a bunch of accelerometers in tree and as they go wrt to events this one isn't even particularly complex. The existing ABI covers their events reasonably well so take a look at how they do it. Often it's a case of mapping names for the application of an event (free fall detection, activity detection etc) to what what they are actually detecting. Those generalize a lot better across different sensor types. It's almost always a threshold of some type. The tap / double tap are more complex but we put quite a lot of effort into coming up with a general description a year or so back. There may new things but most of the ABI is already there. > > The sensor has further features still not covered: > - g-ranges scaled by different ODRs, especially for activity / inactivity > threshold is not covered so far. There seems to be a particularity with > the ADXL345 as annotated on some analog FAQ. > > - Various thinks like low power, sleep mode, etc. are (still) not covered > here, others (ACDC bit, selftest, etc.) currently are hard coded or not > covered. > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > --- > Questions: > - Do I need a mutex/lock protection as this is the case e.g. in the ADXL367 > or the ADXL380? > Actually, I understand those cases as protecting access to the state > object by different channels, temperature and accelerometer. I'm unsure > if this is a correct understanding, where for the ADXL345 there should > not be any issue. At most, a currently displayed value on sysfs is > (still) not updated. So, IMHO I can rely on the internal protections in > regmap no further mutex is needed. Please, can you give me a feedback > here? If you have an read modify write actions triggered by sysfs writes they can race (not serialization between different file writes). That's why you tend to need the mutex. > > - FIELD_PREP/FIELD_GET: I'd like to use arrays of enum indexed elements > to allow for more generic function implementation passing just a "type" > field, e.g. at single tap/double tap or activity/inactivity handling. > When it comes to filtering out bits using FIELD_GET/FIELD_PREP, it says > that this enum array element is not "const enough". Is there a > work-around? I don't have it to hand but there is a patch set trying to add non const versions of these that went to my other email. For now just carry a local version as we don't want to end up waiting for that patch to merge. > > Lothar Rubusch (12): > iio: accel: adxl345: migrate constants to core > iio: accel: adxl345: reorganize measurement enable > iio: accel: adxl345: add debug register access > iio: accel: adxl345: reorganize irq handler > iio: accel: adxl345: improve access to the interrupt enable register > iio: accel: adxl345: add single tap feature > iio: accel: adxl345: show tap status and direction > iio: accel: adxl345: add double tap feature > iio: accel: adxl345: add double tap suppress bit > iio: accel: adxl345: add freefall feature > iio: accel: adxl345: add activity feature > iio: accel: adxl345: add inactivity feature > > drivers/iio/accel/adxl345.h | 86 --- > drivers/iio/accel/adxl345_core.c | 1150 ++++++++++++++++++++++++++++-- > 2 files changed, 1099 insertions(+), 137 deletions(-) >
Hi Jonathan, please find my answers below. On Sat, Feb 1, 2025 at 6:48 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Tue, 28 Jan 2025 12:00:48 +0000 > Lothar Rubusch <l.rubusch@gmail.com> wrote: > > > Add several interrupt based sensor detection events: > > - single tap > > - double tap > > - free fall > > - activity > > - inactivity > > > > All the needed parameters for each and methods of adjusting them, and > > forward a resulting IIO event for each to the IIO channel. > > So my main feedback here is to be much more reluctant to add new ABI. > Anything you add is unused by all existing code and if it is unique > to a driver probably never going to be used by anyone other than you. > > We have a bunch of accelerometers in tree and as they go wrt to events > this one isn't even particularly complex. The existing ABI covers > their events reasonably well so take a look at how they do it. Often > it's a case of mapping names for the application of an event (free fall > detection, activity detection etc) to what what they are actually detecting. > Those generalize a lot better across different sensor types. It's almost > always a threshold of some type. The tap / double tap are more complex > but we put quite a lot of effort into coming up with a general > description a year or so back. There may new things but most of the > ABI is already there. > Please, understand my patches in "v1" rather as a huge set of questions, than a proposal of reinventing the IIO ABI :) My intention is actually not to extend/rewrite the ABI. It rather shows my lack of knowledge combined with the curiosity of how to actually use the (IIO) APIs for such implementation. I know this is still tedious, but I'm sure it will become better. My dilemma was/is the following: I did an initial implementation more similar to e.g. ADXL380 for such events, and the sca3000.c for the freefall event. IMHO the names for the sysfs handles were not at all intuitively mappable to the fields I liked to operate, such as tap duration, window, latent, etc. The other alternative I saw, was setting up sysfs myself, IMHO clearer naming but actually not really using IIO's event_config/event_value. Personally, I don't have any preference. If there really is no way to change naming of the sysfs handles, then it's probably a question of documentation. If I can make it more intuitive for a user who knows the sensor, but not the internals of IIO, then I'd prefer to use the names referred and documented in the sensor's datasheet. In summary, I see your point. So, I redo this patch set as I did in the initial approach to show better what I mean. Probably I'm just using it in a wrong way. Thank you for the feedback so far, I'll try to use it where it still applies. > > > > The sensor has further features still not covered: > > - g-ranges scaled by different ODRs, especially for activity / inactivity > > threshold is not covered so far. There seems to be a particularity with > > the ADXL345 as annotated on some analog FAQ. > > > > - Various thinks like low power, sleep mode, etc. are (still) not covered > > here, others (ACDC bit, selftest, etc.) currently are hard coded or not > > covered. > > > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > > --- > > Questions: > > - Do I need a mutex/lock protection as this is the case e.g. in the ADXL367 > > or the ADXL380? > > Actually, I understand those cases as protecting access to the state > > object by different channels, temperature and accelerometer. I'm unsure > > if this is a correct understanding, where for the ADXL345 there should > > not be any issue. At most, a currently displayed value on sysfs is > > (still) not updated. So, IMHO I can rely on the internal protections in > > regmap no further mutex is needed. Please, can you give me a feedback > > here? > > If you have an read modify write actions triggered by sysfs writes they > can race (not serialization between different file writes). > That's why you tend to need the mutex. > This explains it clearly, ty. > > > > - FIELD_PREP/FIELD_GET: I'd like to use arrays of enum indexed elements > > to allow for more generic function implementation passing just a "type" > > field, e.g. at single tap/double tap or activity/inactivity handling. > > When it comes to filtering out bits using FIELD_GET/FIELD_PREP, it says > > that this enum array element is not "const enough". Is there a > > work-around? > I don't have it to hand but there is a patch set trying to add non > const versions of these that went to my other email. > > For now just carry a local version as we don't want to end up waiting > for that patch to merge. I understand. I'll do another implementation approach, I'll see. Good to know, anyway. > > > > Lothar Rubusch (12): > > iio: accel: adxl345: migrate constants to core > > iio: accel: adxl345: reorganize measurement enable > > iio: accel: adxl345: add debug register access > > iio: accel: adxl345: reorganize irq handler > > iio: accel: adxl345: improve access to the interrupt enable register > > iio: accel: adxl345: add single tap feature > > iio: accel: adxl345: show tap status and direction > > iio: accel: adxl345: add double tap feature > > iio: accel: adxl345: add double tap suppress bit > > iio: accel: adxl345: add freefall feature > > iio: accel: adxl345: add activity feature > > iio: accel: adxl345: add inactivity feature > > > > drivers/iio/accel/adxl345.h | 86 --- > > drivers/iio/accel/adxl345_core.c | 1150 ++++++++++++++++++++++++++++-- > > 2 files changed, 1099 insertions(+), 137 deletions(-) > > >
Add several interrupt based sensor detection events: - single tap - double tap - free fall - activity - inactivity All the needed parameters for each and methods of adjusting them, and forward a resulting IIO event for each to the IIO channel. The sensor has further features still not covered: - g-ranges scaled by different ODRs, especially for activity / inactivity threshold is not covered so far. There seems to be a particularity with the ADXL345 as annotated on some analog FAQ. - Various thinks like low power, sleep mode, etc. are (still) not covered here, others (ACDC bit, selftest, etc.) currently are hard coded or not covered. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- Questions: - Do I need a mutex/lock protection as this is the case e.g. in the ADXL367 or the ADXL380? Actually, I understand those cases as protecting access to the state object by different channels, temperature and accelerometer. I'm unsure if this is a correct understanding, where for the ADXL345 there should not be any issue. At most, a currently displayed value on sysfs is (still) not updated. So, IMHO I can rely on the internal protections in regmap no further mutex is needed. Please, can you give me a feedback here? - FIELD_PREP/FIELD_GET: I'd like to use arrays of enum indexed elements to allow for more generic function implementation passing just a "type" field, e.g. at single tap/double tap or activity/inactivity handling. When it comes to filtering out bits using FIELD_GET/FIELD_PREP, it says that this enum array element is not "const enough". Is there a work-around? Lothar Rubusch (12): iio: accel: adxl345: migrate constants to core iio: accel: adxl345: reorganize measurement enable iio: accel: adxl345: add debug register access iio: accel: adxl345: reorganize irq handler iio: accel: adxl345: improve access to the interrupt enable register iio: accel: adxl345: add single tap feature iio: accel: adxl345: show tap status and direction iio: accel: adxl345: add double tap feature iio: accel: adxl345: add double tap suppress bit iio: accel: adxl345: add freefall feature iio: accel: adxl345: add activity feature iio: accel: adxl345: add inactivity feature drivers/iio/accel/adxl345.h | 86 --- drivers/iio/accel/adxl345_core.c | 1150 ++++++++++++++++++++++++++++-- 2 files changed, 1099 insertions(+), 137 deletions(-)