Re: [PATCH v2 09/17] spi: add locomo SPI driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Tue, Apr 28, 2015 at 02:55:46AM +0300, Dmitry Eremin-Solenikov wrote:

> +static int locomospi_reg_open(struct locomospi_dev *spidev)
> +{

> +static int locomospi_reg_release(struct locomospi_dev *spidev)
> +{

These are a bit weird - as far as I can tell they're just doing some
init done on probe and release?  I think I'd expect to see them either
inlined there or done as part of the PM operations too (including
runtime ones).

> +	for (j = 0; j < wait; j++) {
> +		regmap_read(spidev->regmap, LOCOMO_SPIST, &r);
> +		if (r & LOCOMO_SPI_RFW)
> +			break;
> +	}
> +	if (j == wait)
> +		dev_err(&spi->dev, "rfw timeout\n");

But we don't return an error if we time out?

> +static int locomo_spi_setup_transfer(struct spi_device *spi,
> +		struct spi_transfer *t)
> +{
> +	struct locomospi_dev *spidev;
> +	u32 hz = 0;
> +
> +	if (t)
> +		hz = t->speed_hz;
> +	if (!hz)
> +		hz = spi->max_speed_hz;

The core will set a speed on every transfer for you.

> +	if (!tx && !rx && t->len)
> +		return -EINVAL;

Put this in the core if it's needed.

> +static int locomo_spi_remove(struct platform_device *pdev)
> +{
> +	struct spi_master *master = platform_get_drvdata(pdev);
> +	struct locomospi_dev *spidev = spi_master_get_devdata(master);
> +
> +	return locomospi_reg_release(spidev);

We're doing this release before we free the master which is potentially
racy - but do we even need to do it at all?

Attachment: signature.asc
Description: Digital signature


[Index of Archives]     [Linux SPI]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux