Re: [PATCH 1/2] spi: Add MXIC controller driver

[Trent Piepho <tpiepho@xxxxxxxxxx>]

On Thu, 2018-08-02 at 10:07 +0800, masonccyang@xxxxxxxxxxx wrote:
> +
> +#define GPIO			0xc4
> +#define GPIO_PT(x)		BIT(3 + ((x) * 16))
> +#define GPIO_RESET(x)		BIT(2 + ((x) * 16))
> +#define GPIO_HOLDB(x)		BIT(1 + ((x) * 16))
> +#define GPIO_WPB(x)		BIT((x) * 16)

Is this one of the device's registers?  Doesn't seem to be used.  In
fact, most of the registers that were defined here are not used.  The
above look strange.  Do you mean GPIO_RESET(0) is bit 2 and
GPIO_RESET(1) is bit 18?  There are just two gpios?  Or maybe, since
each GPIO has four bits associated with it, that should be x*4 instead
of x*16?

> +
> +#define HC_VER			0xd0
> +
> +#define HW_TEST(x)		(0xe0 + ((x) * 4))
> +
> +struct mxic_spi {
> +	struct clk *ps_clk;
> +	struct clk *send_clk;
> +	struct clk *send_dly_clk;
> +	void __iomem *regs;
> +	struct {
> +		void __iomem *map;
> +		dma_addr_t dma;
> +	} linear;

Nothing here uses linear.

> 
> +
> +static int mxic_spi_data_xfer(struct mxic_spi *mxic, const void *txbuf,
> +			      void *rxbuf, unsigned int len)
> +{
> +	unsigned int pos = 0;
> +
> +	while (pos < len) {
> +		unsigned int nbytes = len - pos;
> +		u32 data = 0xffffffff;
> +		u32 sts;
> +		int ret;
> +
> +		if (nbytes > 4)
> +			nbytes = 4;
> +
> +		if (txbuf)
> +			memcpy(&data, txbuf + pos, nbytes);
> +
> +		ret = readl_poll_timeout(mxic->regs + INT_STS, sts,
> +					 sts & INT_TX_EMPTY, 0, USEC_PER_SEC);
> +		if (ret)
> +			return ret;
> +
> +		writel(data, mxic->regs + TXD(nbytes % 4));

This looks strange.  To send one byte of data, you write a 32-bit word
to the register at 0x18.  To send two bytes, a 32-bit word is written
to 0x1c, three bytes by writing to 0x20, and four bytes to 0x14.  So
there are four TX data registers, each 32 bits wide, and which register
is written to controls how many bytes are sent?

> 
> +
> +int mxic_spi_setup(struct spi_device *spi)
> +{
> +	struct mxic_spi *mxic = spi_master_get_devdata(spi->master);
> +	unsigned long freq = spi->max_speed_hz;
> +	int ret;
> +
> +	ret = clk_set_rate(mxic->send_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_set_rate(mxic->send_dly_clk, freq);
> +	if (ret)
> +		return ret;
> +
> +	mxic_spi_set_input_delay_dqs(mxic, 0xf);
> +	ret = clk_set_phase(mxic->send_dly_clk, 360 / (1000000000 / freq));

Can also be written as freq*9/25000000.

> +	if (ret)
> +		return ret;
> +
> +	clk_prepare_enable(mxic->send_clk);
> +	clk_prepare_enable(mxic->send_dly_clk);
> +
> +	return 0;

Won't changing the clock rate in spi_setup break if there are two
slaves which use different rates?

> ��.n��������+%������w��{.n�����{����)��jg��������ݢj����G�������j:+v���w�m������w�������h�����٥