driver: sensor: adxl372: Bug fix for status2 reg #79105
Conversation
zephyrbot
requested review from
avisconti,
galak,
jeppenodgaard,
MaureenHelm,
microbuilder,
ozersa,
teburd,
tristan-google,
ttmut,
ubieda and
yperess
This is a bug fix for adxl372_get_status function. This function is used to get STATUS1, STATUS2, FIFO_ENTRIES2 and FIFO_ENTRIES1 registers (they are continuously present in the memory in this order). User can choose if it wants STATUS2 and/or FIFO registers by passing pointers where to store received values. Bug was when user doesn't want STATUS2 and want FIFO regs. In that case calculated length would be 3 which will result in reading of STATUS1, STATUS2 and FIFO_ENTRIES2 regs and not expected STATUS1, FIFO_ENTRIES2 and FIFO_ENTRIES1. Signed-off-by: Vladislav Pejic <[email protected]>
vladislav-pejic
force-pushed
the
adi_adxl372_status2_bug_fix
branch
from
518566b
to
f717fda
Compare
| length += 2U; | ||
| if (status2) { | ||
| length += 2U; | ||
| } else { | ||
| /* Registers STATUS1, STATUS2, FIFO_ENTRIES2 and FIFO_ENTRIES1 | ||
| * are one after the other. If user wants fifo_entries and | ||
| * not status2, to get the correct values, STATUS2 register | ||
| * also must be read but read value will be ignored. | ||
| */ | ||
| length += 3U; | ||
| } |
There was a problem hiding this comment.
I'm not sure I understand the rationale for having a variable-length transfer here (I know it was in-tree in the first place; I'm suggesting to simplify it since it had a bug).
Wouldn't it be just easier to always pull the 4-bytes and depending on whether the pointer is valid: pass that config over?
There was a problem hiding this comment.
My rationale was that I wanted to reduce SPI overhead and not read registers that are not necessary (FIFO ENTRIES1/2).
There was a problem hiding this comment.
Ok - I personally find it harder to maintain this way vs the overhead of 1 or 2 extra bytes. It's a subjective thought though. Approving.
| length += 2U; | ||
| if (status2) { | ||
| length += 2U; | ||
| } else { | ||
| /* Registers STATUS1, STATUS2, FIFO_ENTRIES2 and FIFO_ENTRIES1 | ||
| * are one after the other. If user wants fifo_entries and | ||
| * not status2, to get the correct values, STATUS2 register | ||
| * also must be read but read value will be ignored. | ||
| */ | ||
| length += 3U; | ||
| } |
There was a problem hiding this comment.
Ok - I personally find it harder to maintain this way vs the overhead of 1 or 2 extra bytes. It's a subjective thought though. Approving.
This is a bug fix for adxl372_get_status function. This function is used to get STATUS1, STATUS2, FIFO_ENTRIES2 and FIFO_ENTRIES1 registers (they are continuously present in the memory in this order). User can choose if it wants STATUS2 and/or FIFO registers by passing pointers where to store received values. Bug was when user doesn't want STATUS2 and want FIFO regs. In that case calculated length would be 3 which will result in reading of STATUS1, STATUS2 and FIFO_ENTRIES2 regs and not expected STATUS1, FIFO_ENTRIES2 and FIFO_ENTRIES1.
Signed-off-by: Vladislav Pejic [email protected]