| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 23 Jun 2009 16:11:51 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Sascha Hauer <s.hauer@...gutronix.de>
Cc: linux-kernel@...r.kernel.org, david-b@...bell.net,
s.hauer@...gutronix.de
Subject: Re: [PATCH 2/2] SPI: Add SPI driver for most known i.MX SoCs
On Thu, 18 Jun 2009 08:54:32 +0200
Sascha Hauer <s.hauer@...gutronix.de> wrote:
> v2: Updated and tested on i.MX35
>
> While there already is a SPI driver for i.MX1 in the tree there are
> good reasons to replace it:
>
> - The inkernel driver implements a full blown SPI driver, but
> the hardware can be fully supported using a bitbang driver.
> This greatly reduces the size and complexity of the driver.
> - The inkernel driver only works on i.MX1 SoCs. Unfortunately
> Freescale decided to randomly mix the register bits for each
> new SoC, This is quite hard to handle with the current driver
> since it has register accesses in many places.
> - The DMA API of the durrent driver is broken for arch-mx1 (as opposed
> to arch-imx) and nobody cared to fix it yet.
ah, there's some detail. I'l move this into the changelog for
spi-remove-imx-spi-driver.patch.
Still, it seesm that the current driver does work for some people.
Removing it in this manner seems a bit risky. Unnecessarily risky?
> This driver has been tested on i.MX1/i.MX27/i.MX35 with an AT25 type
> EEPROM and on i.MX27/i.MX31 with a Freescale MC13783 PMIC.
>
> Signed-off-by: Sascha Hauer <s.hauer@...gutronix.de>
> Tested-by: Guennadi Liakhovetski <g.liakhovetski@....de>
>
> ...
>
> +#define MXC_SPI_BUF_RX(type) \
> +static void mxc_spi_buf_rx_##type(struct mxc_spi_data *mxc_spi) \
> +{ \
> + unsigned int val = readl(mxc_spi->base + MXC_CSPIRXDATA); \
> + \
> + if (mxc_spi->rx_buf) { \
> + *(type *)mxc_spi->rx_buf = val; \
> + mxc_spi->rx_buf += sizeof(type); \
> + } \
> +}
> +
> +#define MXC_SPI_BUF_TX(type) \
> +static void mxc_spi_buf_tx_##type(struct mxc_spi_data *mxc_spi) \
> +{ \
> + type val = 0; \
> + \
> + if (mxc_spi->tx_buf) { \
> + val = *(type *)mxc_spi->tx_buf; \
Are these operations endian-safe? Seems not. Should they be? It
would be pretty simple to do.
> + mxc_spi->tx_buf += sizeof(type); \
> + } \
> + \
> + mxc_spi->count -= sizeof(type); \
> + \
> + writel(val, mxc_spi->base + MXC_CSPITXDATA); \
> +}
> +
> +MXC_SPI_BUF_RX(u8)
> +MXC_SPI_BUF_TX(u8)
> +MXC_SPI_BUF_RX(u16)
> +MXC_SPI_BUF_TX(u16)
> +MXC_SPI_BUF_RX(u32)
> +MXC_SPI_BUF_TX(u32)
> +
> +/* First entry is reserved, second entry is valid only if SDHC_SPIEN is set
> + * (which is currently not the case in this driver)
> + */
> +static int mxc_clkdivs[] = {0, 3, 4, 6, 8, 12, 16, 24, 32, 48, 64, 96, 128, 192,
> + 256, 384, 512, 768, 1024};
Could be made const - it's slightly preferable to have data in
read-only locations if possible. It means that we'll get a nice oops
if someone accidentally writes to them and sometimes it means that the
data doesn't need to be copied out to read/write memory at boot-time.
Perhaps the compiler does this anyway.
>
> ...
>
> +static void mx27_trigger(struct mxc_spi_data *mxc_spi)
> +{
> + unsigned int reg;
> +
> + reg = readl(mxc_spi->base + MXC_CSPICTRL);
> + reg |= MX27_CSPICTRL_XCH;
> + writel(reg, mxc_spi->base + MXC_CSPICTRL);
> +}
This all looks rather SMP/preempt/interrupt-unsafe. Or does the SPI core
provide the needed locking?
> +static int mx27_config(struct mxc_spi_data *mxc_spi,
> + struct mxc_spi_config *config)
> +{
> + unsigned int reg = MX27_CSPICTRL_ENABLE | MX27_CSPICTRL_MASTER;
> +
> + reg |= mxc_spi_clkdiv_1(mxc_spi->spi_clk, config->speed_hz) <<
> + MX27_CSPICTRL_DR_SHIFT;
> + reg |= config->bpw - 1;
> +
> + if (config->mode & SPI_CPHA)
> + reg |= MX27_CSPICTRL_PHA;
> + if (config->mode & SPI_CPOL)
> + reg |= MX27_CSPICTRL_POL;
> + if (config->mode & SPI_CS_HIGH)
> + reg |= MX27_CSPICTRL_SSPOL;
> + if (config->cs < 0)
> + reg |= (config->cs + 32) << MX27_CSPICTRL_CS_SHIFT;
> +
> + writel(reg, mxc_spi->base + MXC_CSPICTRL);
> +
> + return 0;
> +}
Dittoes everywhere.
> +static int mx27_rx_available(struct mxc_spi_data *mxc_spi)
> +{
> + return readl(mxc_spi->base + MXC_CSPIINT) & MX27_INTREG_RR;
> +}
> +
>
> ...
>
> +static int __init mxc_spi_probe(struct platform_device *pdev)
> +{
> + struct spi_imx_master *mxc_platform_info;
> + struct spi_master *master;
> + struct mxc_spi_data *mxc_spi;
> + struct resource *res;
> + int i, ret;
> +
> + mxc_platform_info = (struct spi_imx_master *)pdev->dev.platform_data;
Unneeded and undesirable cast of a void*.
> + if (!mxc_platform_info) {
> + dev_err(&pdev->dev, "can't get the platform data\n");
> + return -EINVAL;
Can this happen?
> + }
> +
> + master = spi_alloc_master(&pdev->dev, sizeof(struct mxc_spi_data));
> + if (!master)
> + return -ENOMEM;
> +
>
> ...
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists