Re: [PATCH] SPI: Add driver for Cadence SPI controller

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3826 bytes --]

On Tue, Mar 18, 2014 at 05:16:26AM +0000, Harini Katakam wrote:

Please fix your mailer to word wrap within paragraphs, this will make
your mail much more legible.

> > > +	if (bits_per_word != 8) {
> > > +		dev_err(&spi->dev, "%s, unsupported bits per word %x\n",
> > > +			__func__, spi->bits_per_word);
> > > +		return -EINVAL;
> > > +	}

> > The core will check this for you.

> I dint find that the core does this.

bpw_mask.

> > It's not clear to me why this has been split into a separate function and the
> > function will write to the hardware which you're not allowed to do in
> > setup() if it might affect an ongoing transfer.

> Are you saying  that there's a chance cdns_spi_setup() will be called
> when there's an ongoing transfer?  In that case I have to remove the
> cdns_setup_transfer() call from here but then there's nothing left to
> do in setup.

yes.

> > I see from further up the file that there are error interrupt states which
> > might be flagged but these are just being ignored.

> I'm not sure I understand what you are referring to -
> The only interrupts enabled (See CNDS_IXR_DEFAULT_MASK) are TXOW and MODF.

The code had:

> +#define CDNS_SPI_IXR_TXOW_MASK 0x00000004 /* SPI TX FIFO Overwater */
> +#define CDNS_SPI_IXR_MODF_MASK 0x00000002 /* SPI Mode Fault */
> +#define CDNS_SPI_IXR_RXNEMTY_MASK 0x00000010 /* SPI RX FIFO Not Empty */

> +#define CDNS_SPI_IXR_TXFULL_MASK       0x00000008 /* SPI TX Full */

and defined:

> +#define CDNS_SPI_IXR_ALL_MASK  0x0000007F /* SPI all interrupts */

> > > +	return IRQ_HANDLED;

> > This should only be returned if an interrupt was actually handled - if the line
> > is shared in some system this will break.

> In this case both possible interrupt conditions are handled.

Are you sure that's the case, and even if you are that's still not
handling the case where the device isn't flagging an interrupt at all.

> > > +	cdns_spi_write(xspi->regs + CDNS_SPI_IER_OFFSET,
> > > +		       CDNS_SPI_IXR_DEFAULT_MASK);
> > > +
> > > +	ret = wait_for_completion_interruptible_timeout(&xspi->done,
> > > +							CDNS_SPI_TIMEOUT);
> > > +	if (ret < 1) {

> > If you return a positive value from transfer_one() the core will wait for you.

> I don't understand. Here, if the ret from
> wait_for_completion_interruptible_timeout is positive, then this
> function (spi_start_transfer) will return (transfer->len) -
> (xspi->remaining_bytes) to cdns_transfer_one_message(the calling
> funtion). Which will just use this return as information on how many
> bytes were transferred (see variable length). 
> cdns_transfer_one_messag() only returns 0 or error value (-ve) (see
> variable status).  It doesn't return positive value to core.

I'm saying you should be implementing a transfer_one() operation and
returning a positive value from it so the core can do the delay for you.

> > > +	ret = of_property_read_u16(pdev->dev.of_node, "num-chip-select",
> > > +				   &master->num_chipselect);
> > > +	if (ret < 0) {
> > > +		dev_err(&pdev->dev, "couldn't determine num-chip-
> > select\n");
> > > +		goto clk_dis_all;
> > > +	}

> > Is there no reasonable default?

> Are you saying if I cant read num-chipselect from dts, 
> I should set it to a default (that will be 4) and proceed?

Yes.

> > > +	/* Set to default valid value */
> > > +	xspi->speed_hz = clk_get_rate(xspi->ref_clk) / 4;

> > The driver should set max_speed_hz as well.

> This is the max_speed_hz - 4 is the lowest divisor possible. 2 is invalid.
> It is set in init_hw (see config register default mask).

I'm saying you should set max_speed_hz, not set speed_hz to the maximum
value.  This tells the core what the maximum speed is so it can error
check the driver operations.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]