On Sun, Oct 02, 2016 at 11:44:08PM +0200, Hauke Mehrtens wrote:
> From: Daniel Schwierzeck <daniel.schwierzeck@xxxxxxxxx>
>
> This driver supports the Intel SSC SPI controller in master
> mode. This controller is found on Intel (former Lantiq) SoCs like
> the Danube, Falcon, xRX200, xRX300.
Can we please just call this Lantiq like all the other Lantiq IPs?
Intel has a number of other SPI controllers in use (there's at least the
Intel designed pxa2xx one, plus DesignWare) so just calling it the Intel
SPI controller isn't very clear and most people would assume it's for
x86 chips.
Overall this looks good, most of the comments below are about use of
more modern APIs.
> +config SPI_INTEL_SSC
> + tristate "Intel SSC SPI controller"
> + depends on LANTIQ
Why no || COMPILE_TEST? Not being able to build on other architectures
makes the driver much harder to maintain. It looks like this is just
for the fpi clock which is exported by the architecture in a very
unusual way.
> +static void hw_chipselect_set(struct intel_ssc_spi *spi, unsigned int cs)
> +{
> + u32 fgpo = (1 << (cs - spi->base_cs + SPI_FGPO_SETOUTN_S));
> + intel_ssc_spi_writel(spi, fgpo, SPI_FPGO);
> +}
> +
> +static void hw_chipselect_clear(struct intel_ssc_spi *spi, unsigned int cs)
> +{
> + u32 fgpo = (1 << (cs - spi->base_cs));
> + intel_ssc_spi_writel(spi, fgpo, SPI_FPGO);
> +}
We really need separate functions for this?
> +static void chipselect_enable(struct spi_device *spidev)
> +{
> + struct intel_ssc_spi *spi = spi_master_get_devdata(spidev->master);
> + struct intel_ssc_spi_cstate *cstate = spi_get_ctldata(spidev);
> +
> + if (cstate->cs_gpio >= 0)
> + gpio_set_value(cstate->cs_gpio, spidev->mode & SPI_CS_HIGH);
> + else
> + hw_chipselect_clear(spi, spidev->chip_select);
Please use the core support for GPIO chip selects rather than open
coding.
> +
> + /* CS setup/recovery time */
> + if (spi->cs_delay)
> + ndelay(spi->cs_delay);
Please add this to the core, the idea of needing a settling time after
asserting chip select doesn't seem device specific.
> + err = gpio_request(spidev->cs_gpio, dev_name(spi->dev));
> + if (err)
> + return err;
> +
> + gpio_direction_output(spidev->cs_gpio,
> + !(spidev->mode & SPI_CS_HIGH));
Use gpio_request_one() to combine these.
> +static void hw_finish_message(const struct intel_ssc_spi *spi)
> +{
> + /* Disable interrupts */
> + intel_ssc_spi_writel(spi, 0, SPI_IRNEN);
> +
> + /* Disable transmitter and receiver */
> + intel_ssc_spi_maskl(spi, 0, SPI_CON_TXOFF | SPI_CON_RXOFF, SPI_CON);
> +}
This needs to be done per message or just when the hardware idles?
> + switch (spi->bits_per_word) {
> + default:
> + WARN_ON(1);
> + }
The driver hasn't set bits_per_word_mask, it should do so so that we've
got error checking for this further up the stack where it can be
usefully reported and so client drivers can discover what's supported.
> +static irqreturn_t intel_ssc_spi_xmit_interrupt(int irq, void *data)
> +{
> + struct intel_ssc_spi *spi = data;
> +
> + /* handle possible interrupts after device initialization */
> + if (!spi->rx && !spi->tx)
> + return IRQ_HANDLED;
This is broken if the interrupt is ever shared - the driver should
really quiesce the hardware before requesting interrupts. It also
prevents the interrupt core from handling things if there is ever a
problem which causes lots of spurious interrupts. Why do we *need* to
do this? It'd also be clearer if this were part of the if/else chain
below.
> + goto completed;
> + } else
> + goto completed;
Coding style, if one side of the if statement has brackets both should.
> +static int transfer_wait_finished(struct intel_ssc_spi *spi)
> +{
> + unsigned long timeout;
The core already supports waiting for transfer completion?
> +static int intel_ssc_spi_transfer_one_message(struct spi_master *master,
> + struct spi_message *msg)
Why not implement transfer_one()? This seems to just duplicate the
standard implementation.
> + status = spi->status;
> + if (status) {
> + dev_err(spi->dev, "transfer failed\n");
> + goto done;
> + }
There's no need to log this, let the generic code do it if it's useful
since we're not adding any detail here (or incuding the error value).
> +static int intel_ssc_spi_prepare_transfer(struct spi_master *master)
> +{
> + struct intel_ssc_spi *spi = spi_master_get_devdata(master);
> +
> + pm_runtime_get_sync(spi->dev);
Use the core runtime PM support, it avoids duplication and checks the
return values. auto_runtime_pm.
> + tx_irq = platform_get_irq_byname(pdev, SPI_TX_IRQ_NAME);
I'm not sure the define is helping readabiliy here.
> + spi->spi_clk = devm_clk_get(&pdev->dev, NULL);
> + if (IS_ERR(spi->spi_clk)) {
> + err = PTR_ERR(spi->spi_clk);
> + goto err_master_put;
> + }
> + clk_prepare_enable(spi->spi_clk);
Check the return code here.
> + spi->fpi_clk = clk_get_fpi();
> + if (IS_ERR(spi->fpi_clk)) {
> + err = PTR_ERR(spi->fpi_clk);
> + goto err_clk_disable;
> + }
This is pretty horrible, why does the platform not just export the FPI
clock as a normal clock? clk_get(NULL, "fpi") would work just fine as
an interface and avoid the needless platform dependency here (as would
explicitly mapping it in per device). It looks like there's only one
existing user so this would be easy to fix.
> + clk_disable_unprepare(spi->spi_clk);
> + clk_put(spi->fpi_clk);
You used devm_clk_get() but you're calling clk_put() here. Not only is
this redundant but if you do explicitly need to free a devm_ allocated
thing you should use the matching devm_ function (devm_clk_put() here)
to ensure that the managed deallocation is removed.
Attachment:
signature.asc
Description: PGP signature
