On Fri, Sep 27, 2019 at 08:25:18AM EDT, Andrew Lunn wrote:
>> +static s32 idtcm_xfer(struct idtcm *idtcm,
>> + u8 regaddr,
>> + u8 *buf,
>> + u16 count,
>> + bool write)
>> +{
>> + struct i2c_client *client = idtcm->client;
>> + struct i2c_msg msg[2];
>> + s32 cnt;
>> +
>> + msg[0].addr = client->addr;
>> + msg[0].flags = 0;
>> + msg[0].len = 1;
>> + msg[0].buf = ®addr;
>> +
>> + msg[1].addr = client->addr;
>> + msg[1].flags = write ? 0 : I2C_M_RD;
>> + msg[1].len = count;
>> + msg[1].buf = buf;
>> +
>> + cnt = i2c_transfer(client->adapter, msg, 2);
>> +
>> + if (cnt < 0) {
>> + dev_err(&client->dev, "i2c_transfer returned %d\n", cnt);
>> + return cnt;
>> + } else if (cnt != 2) {
>> + dev_err(&client->dev,
>> + "i2c_transfer sent only %d of %d messages\n", cnt, 2);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static s32 idtcm_page_offset(struct idtcm *idtcm, u8 val)
>> +{
>> + u8 buf[4];
>> + s32 err;
>
>Hi Vincent
Hi Andrew,
Thank-you for looking at the patch.
>All your functions return s32, rather than the usual int. err is an
>s32. i2c_transfer() will return an int, which you then assign to an
>s32. I've no idea, but maybe the static code checkers like smatch
>will complain about this, especially on 64 bit systems? I suspect on
>64 bit machines, the compiler will be generating worse code, masking
>registers? Maybe use int, not s32?
Oops. You are correct, I messed up when trying to standardize
on linux types.h. I will go through the code to ensure int is used
for error codes and return values.
>> + case OUTPUT_MASK_PLL2_ADDR + 1:
>> + SET_U16_MSB(idtcm->channel[2].output_mask, val);
>> + break;
>> + case OUTPUT_MASK_PLL3_ADDR:
>> + SET_U16_LSB(idtcm->channel[3].output_mask, val);
>> + break;
>> + case OUTPUT_MASK_PLL3_ADDR + 1:
>> + SET_U16_MSB(idtcm->channel[3].output_mask, val);
>> + break;
>> + default:
>> + err = -1;
>
>EINVAL?
Yes, will replace with -EINVAL. Thanks.
>> +static void set_default_function_pointers(struct idtcm *idtcm)
>> +{
>> + idtcm->_idtcm_gettime = _idtcm_gettime;
>> + idtcm->_idtcm_settime = _idtcm_settime;
>> + idtcm->_idtcm_rdwr = idtcm_rdwr;
>> + idtcm->_sync_pll_output = sync_pll_output;
>> +}
>
>Why does this indirection? Are the SPI versions of the silicon?
The indirection is to enable us to replace those functions in
our unit tests with mocked functions.
I read somewhere that I should leave a week between sending a
revised patch series. Is this a good rule to follow?
Regards,
Vincent
