Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 27 Sep 2019 14:25:18 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     vincent.cheng.xh@renesas.com
Cc:     robh+dt@kernel.org, mark.rutland@arm.com, richardcochran@gmail.com,
        devicetree@vger.kernel.org, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/2] ptp: Add a ptp clock driver for IDT ClockMatrix.
Message-ID: <20190927122518.GA25474@lunn.ch>
References: <1569556128-22212-1-git-send-email-vincent.cheng.xh@renesas.com>
 <1569556128-22212-2-git-send-email-vincent.cheng.xh@renesas.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1569556128-22212-2-git-send-email-vincent.cheng.xh@renesas.com>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> +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

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?

> +static s32 set_pll_output_mask(struct idtcm *idtcm, u16 addr, u8 val)
> +{
> +	s32 err = 0;
> +
> +	switch (addr) {
> +	case OUTPUT_MASK_PLL0_ADDR:
> +		SET_U16_LSB(idtcm->channel[0].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL0_ADDR + 1:
> +		SET_U16_MSB(idtcm->channel[0].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL1_ADDR:
> +		SET_U16_LSB(idtcm->channel[1].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL1_ADDR + 1:
> +		SET_U16_MSB(idtcm->channel[1].output_mask, val);
> +		break;
> +	case OUTPUT_MASK_PLL2_ADDR:
> +		SET_U16_LSB(idtcm->channel[2].output_mask, val);
> +		break;
> +	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?

> +		break;
> +	}
> +
> +	return err;
> +}

> +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?

    Andrew