On Tue, 28 Sept 2021 at 11:36, Kevin Townsend <kevin.towns...@linaro.org> wrote:
>
> Hi Peter,
>
> On Mon, 27 Sept 2021 at 18:39, Peter Maydell <peter.mayd...@linaro.org> wrote:
>>
>> I thought we'd agreed to implement the whole of the auto-increment
>> logic, not just for specific registers ?
>
>
> Thanks again for the feedback. Dealing with one register value at a time
> (versus a buffer of response values) does simplify the code flow.
>
> The following code appears to handle multi-byte reads correctly. I just
> wanted to confirm this is what you were looking for before moving on to
> the test code?
>
> /*
> * Callback handler whenever a 'I2C_START_RECV' (read) event is received.
> */
> static void lsm303dlhc_mag_read(LSM303DLHCMagState *s)
> {
> /*
> * Set the LOCK bit whenever a new read attempt is made. This will be
> * cleared in I2C_FINISH. Note that DRDY is always set to 1 in this
> driver.
> */
> s->sr = 0x3;
> }
>
> /*
> * Callback handler whenever a 'I2C_FINISH' event is received.
> */
> static void lsm303dlhc_mag_finish(LSM303DLHCMagState *s)
> {
> /*
> * Clear the LOCK bit when the read attempt terminates.
> * This bit is initially set in the I2C_START_RECV handler.
> */
> s->sr = 0x1;
> }
I would just inline these in the event lsm303dlhc_mag_event()
function. You might also #define some constants for the
register bits.
>
> /*
> * Low-level slave-to-master transaction handler (read attempts).
> */
> static uint8_t lsm303dlhc_mag_recv(I2CSlave *i2c)
> {
> LSM303DLHCMagState *s = LSM303DLHC_MAG(i2c);
>
> switch (s->pointer) {
> case LSM303DLHC_MAG_REG_CRA:
> s->buf = s->cra;
> break;
> case LSM303DLHC_MAG_REG_CRB:
> s->buf = s->crb;
> break;
> case LSM303DLHC_MAG_REG_MR:
> s->buf = s->mr;
> break;
> case LSM303DLHC_MAG_REG_OUT_X_H:
> s->buf = (uint8_t)(s->x >> 8);
> break;
> case LSM303DLHC_MAG_REG_OUT_X_L:
> s->buf = (uint8_t)(s->x);
> break;
> case LSM303DLHC_MAG_REG_OUT_Z_H:
> s->buf = (uint8_t)(s->z >> 8);
> break;
> case LSM303DLHC_MAG_REG_OUT_Z_L:
> s->buf = (uint8_t)(s->z);
> break;
> case LSM303DLHC_MAG_REG_OUT_Y_H:
> s->buf = (uint8_t)(s->y >> 8);
> break;
> case LSM303DLHC_MAG_REG_OUT_Y_L:
> s->buf = (uint8_t)(s->y);
> break;
> case LSM303DLHC_MAG_REG_SR:
> s->buf = s->sr;
> break;
> case LSM303DLHC_MAG_REG_IRA:
> s->buf = s->ira;
> break;
> case LSM303DLHC_MAG_REG_IRB:
> s->buf = s->irb;
> break;
> case LSM303DLHC_MAG_REG_IRC:
> s->buf = s->irc;
> break;
> case LSM303DLHC_MAG_REG_TEMP_OUT_H:
> /* Check if the temperature sensor is enabled or not (CRA & 0x80). */
> if (s->cra & 0x80) {
> s->buf = (uint8_t)(s->temperature >> 8);
> } else {
> s->buf = 0;
> }
> break;
> case LSM303DLHC_MAG_REG_TEMP_OUT_L:
> if (s->cra & 0x80) {
> s->buf = (uint8_t)(s->temperature & 0xf0);
> } else {
> s->buf = 0;
> }
> break;
> default:
> s->buf = 0;
> break;
> }
>
> /*
> * The address pointer on the LSM303DLHC auto-increments whenever a byte
> * is read, without the master device having to request the next address.
> *
> * The auto-increment process has the following logic:
> *
> * - if (s->pointer == 8) then s->pointer = 3
> * - else: if (s->pointer >= 12) then s->pointer = 0
> * - else: s->pointer += 1
> *
> * Reading an invalid address return 0.
> */
> if (s->pointer == LSM303DLHC_MAG_REG_OUT_Y_L) {
> s->pointer = LSM303DLHC_MAG_REG_OUT_X_H;
> } else if (s->pointer >= LSM303DLHC_MAG_REG_IRC) {
> s->pointer = LSM303DLHC_MAG_REG_CRA;
> } else {
> s->pointer++;
> }
>
> return s->buf;
I think you don't need to write the value to s->buf, you can just
use a local variable and return that. Nothing should be able to read
the value back out of s->buf later. I think you should also implement
the actual lock part, to avoid wrong values in the case of
* read starts, reads X_H
* s->x updated via the QOM property setter
* read continues, reads X_L
Basically just capture x,y,z,temp at the point of lock, and then
return those values in the recv function.
> }
thanks
-- PMM
