Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.

[Vincent Cheng <vincent.cheng.xh@renesas.com>]

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 = &regaddr;
>> +
>> +	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