| Message ID | 1444313768-23970-2-git-send-email-igvtee@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Michael, while merging the series i stumbled across this patch. to me this looks wrong. rather than removing the locking i think we should add more to rt2880_spi_read and rt2880_spi_write. i'll merge the other 7 patches just now if they apply without 2/8 John On 08/10/2015 16:16, Michael Lee wrote: > Signed-off-by: Michael Lee <igvtee@gmail.com> > --- > ...0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch | 23 +++++----------------- > 1 file changed, 5 insertions(+), 18 deletions(-) > > diff --git a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > index ca04a17..d6a462c 100644 > --- a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > +++ b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > @@ -41,7 +41,7 @@ Acked-by: John Crispin <blogic@openwrt.org> > spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o > --- /dev/null > +++ b/drivers/spi/spi-rt2880.c > -@@ -0,0 +1,493 @@ > +@@ -0,0 +1,480 @@ > +/* > + * spi-rt2880.c -- Ralink RT288x/RT305x SPI controller driver > + * > @@ -174,7 +174,6 @@ Acked-by: John Crispin <blogic@openwrt.org> > + unsigned int sys_freq; > + unsigned int speed; > + struct clk *clk; > -+ spinlock_t lock; > +}; > + > +static inline struct rt2880_spi *spidev_to_rt2880_spi(struct spi_device *spi) > @@ -187,7 +186,8 @@ Acked-by: John Crispin <blogic@openwrt.org> > + return ioread32(rs->base + reg); > +} > + > -+static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, u32 val) > ++static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, > ++ const u32 val) > +{ > + iowrite32(val, rs->base + reg); > +} > @@ -195,27 +195,15 @@ Acked-by: John Crispin <blogic@openwrt.org> > +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, u32 mask) > +{ > + void __iomem *addr = rs->base + reg; > -+ unsigned long flags; > -+ u32 val; > + > -+ spin_lock_irqsave(&rs->lock, flags); > -+ val = ioread32(addr); > -+ val |= mask; > -+ iowrite32(val, addr); > -+ spin_unlock_irqrestore(&rs->lock, flags); > ++ iowrite32((ioread32(addr) | mask), addr); > +} > + > +static inline void rt2880_spi_clrbits(struct rt2880_spi *rs, u32 reg, u32 mask) > +{ > + void __iomem *addr = rs->base + reg; > -+ unsigned long flags; > -+ u32 val; > + > -+ spin_lock_irqsave(&rs->lock, flags); > -+ val = ioread32(addr); > -+ val &= ~mask; > -+ iowrite32(val, addr); > -+ spin_unlock_irqrestore(&rs->lock, flags); > ++ iowrite32((ioread32(addr) & ~mask), addr); > +} > + > +static int rt2880_spi_baudrate_set(struct spi_device *spi, unsigned int speed) > @@ -488,7 +476,6 @@ Acked-by: John Crispin <blogic@openwrt.org> > + rs->master = master; > + rs->sys_freq = clk_get_rate(rs->clk); > + dev_dbg(&pdev->dev, "sys_freq: %u\n", rs->sys_freq); > -+ spin_lock_irqsave(&rs->lock, flags); > + > + device_reset(&pdev->dev); > + >
Hi John. i check the code which functions need read/write register. 3 functions (prepare_message, set_cs, transfer_one) are used at same function __spi_pump_messages at spi.c. the __spi_pump_message is protected by spi_master->queue_lock this make sure spi device operation is serialized. the spi bus is protected by spi_master->bus_lock_spinlock. you can found it at spi_async function. it make sure only one spi device work on the same spi bus. so the 3 functions don't have lock problem. about the setup function. it maybe be called by user space to setup a new spi device. but it will not affect other spi device. because it just disable the new spi device by useing cs pin. so i think it is save to remove the lock protect. the most important is the lock on probe function must be wrong. because it only call spin_lock_irqsave. but not call spin_unlock_irqrestore. 2015-10-26 22:26 GMT+08:00 John Crispin <blogic@openwrt.org>: > Hi Michael, > > while merging the series i stumbled across this patch. to me this looks > wrong. > > rather than removing the locking i think we should add more to > rt2880_spi_read and rt2880_spi_write. > > i'll merge the other 7 patches just now if they apply without 2/8 > > > John > > On 08/10/2015 16:16, Michael Lee wrote: > > Signed-off-by: Michael Lee <igvtee@gmail.com> > > --- > > ...0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch | 23 > +++++----------------- > > 1 file changed, 5 insertions(+), 18 deletions(-) > > > > diff --git > a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > > index ca04a17..d6a462c 100644 > > --- > a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > > +++ > b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch > > @@ -41,7 +41,7 @@ Acked-by: John Crispin <blogic@openwrt.org> > > spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o > > --- /dev/null > > +++ b/drivers/spi/spi-rt2880.c > > -@@ -0,0 +1,493 @@ > > +@@ -0,0 +1,480 @@ > > +/* > > + * spi-rt2880.c -- Ralink RT288x/RT305x SPI controller driver > > + * > > @@ -174,7 +174,6 @@ Acked-by: John Crispin <blogic@openwrt.org> > > + unsigned int sys_freq; > > + unsigned int speed; > > + struct clk *clk; > > -+ spinlock_t lock; > > +}; > > + > > +static inline struct rt2880_spi *spidev_to_rt2880_spi(struct > spi_device *spi) > > @@ -187,7 +186,8 @@ Acked-by: John Crispin <blogic@openwrt.org> > > + return ioread32(rs->base + reg); > > +} > > + > > -+static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, > u32 val) > > ++static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, > > ++ const u32 val) > > +{ > > + iowrite32(val, rs->base + reg); > > +} > > @@ -195,27 +195,15 @@ Acked-by: John Crispin <blogic@openwrt.org> > > +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, > u32 mask) > > +{ > > + void __iomem *addr = rs->base + reg; > > -+ unsigned long flags; > > -+ u32 val; > > + > > -+ spin_lock_irqsave(&rs->lock, flags); > > -+ val = ioread32(addr); > > -+ val |= mask; > > -+ iowrite32(val, addr); > > -+ spin_unlock_irqrestore(&rs->lock, flags); > > ++ iowrite32((ioread32(addr) | mask), addr); > > +} > > + > > +static inline void rt2880_spi_clrbits(struct rt2880_spi *rs, u32 reg, > u32 mask) > > +{ > > + void __iomem *addr = rs->base + reg; > > -+ unsigned long flags; > > -+ u32 val; > > + > > -+ spin_lock_irqsave(&rs->lock, flags); > > -+ val = ioread32(addr); > > -+ val &= ~mask; > > -+ iowrite32(val, addr); > > -+ spin_unlock_irqrestore(&rs->lock, flags); > > ++ iowrite32((ioread32(addr) & ~mask), addr); > > +} > > + > > +static int rt2880_spi_baudrate_set(struct spi_device *spi, unsigned > int speed) > > @@ -488,7 +476,6 @@ Acked-by: John Crispin <blogic@openwrt.org> > > + rs->master = master; > > + rs->sys_freq = clk_get_rate(rs->clk); > > + dev_dbg(&pdev->dev, "sys_freq: %u\n", rs->sys_freq); > > -+ spin_lock_irqsave(&rs->lock, flags); > > + > > + device_reset(&pdev->dev); > > + > > >
diff --git a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch index ca04a17..d6a462c 100644 --- a/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch +++ b/target/linux/ramips/patches-3.18/0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch @@ -41,7 +41,7 @@ Acked-by: John Crispin <blogic@openwrt.org> spi-s3c24xx-hw-$(CONFIG_SPI_S3C24XX_FIQ) += spi-s3c24xx-fiq.o --- /dev/null +++ b/drivers/spi/spi-rt2880.c -@@ -0,0 +1,493 @@ +@@ -0,0 +1,480 @@ +/* + * spi-rt2880.c -- Ralink RT288x/RT305x SPI controller driver + * @@ -174,7 +174,6 @@ Acked-by: John Crispin <blogic@openwrt.org> + unsigned int sys_freq; + unsigned int speed; + struct clk *clk; -+ spinlock_t lock; +}; + +static inline struct rt2880_spi *spidev_to_rt2880_spi(struct spi_device *spi) @@ -187,7 +186,8 @@ Acked-by: John Crispin <blogic@openwrt.org> + return ioread32(rs->base + reg); +} + -+static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, u32 val) ++static inline void rt2880_spi_write(struct rt2880_spi *rs, u32 reg, ++ const u32 val) +{ + iowrite32(val, rs->base + reg); +} @@ -195,27 +195,15 @@ Acked-by: John Crispin <blogic@openwrt.org> +static inline void rt2880_spi_setbits(struct rt2880_spi *rs, u32 reg, u32 mask) +{ + void __iomem *addr = rs->base + reg; -+ unsigned long flags; -+ u32 val; + -+ spin_lock_irqsave(&rs->lock, flags); -+ val = ioread32(addr); -+ val |= mask; -+ iowrite32(val, addr); -+ spin_unlock_irqrestore(&rs->lock, flags); ++ iowrite32((ioread32(addr) | mask), addr); +} + +static inline void rt2880_spi_clrbits(struct rt2880_spi *rs, u32 reg, u32 mask) +{ + void __iomem *addr = rs->base + reg; -+ unsigned long flags; -+ u32 val; + -+ spin_lock_irqsave(&rs->lock, flags); -+ val = ioread32(addr); -+ val &= ~mask; -+ iowrite32(val, addr); -+ spin_unlock_irqrestore(&rs->lock, flags); ++ iowrite32((ioread32(addr) & ~mask), addr); +} + +static int rt2880_spi_baudrate_set(struct spi_device *spi, unsigned int speed) @@ -488,7 +476,6 @@ Acked-by: John Crispin <blogic@openwrt.org> + rs->master = master; + rs->sys_freq = clk_get_rate(rs->clk); + dev_dbg(&pdev->dev, "sys_freq: %u\n", rs->sys_freq); -+ spin_lock_irqsave(&rs->lock, flags); + + device_reset(&pdev->dev); +
Signed-off-by: Michael Lee <igvtee@gmail.com> --- ...0050-SPI-ralink-add-Ralink-SoC-spi-driver.patch | 23 +++++----------------- 1 file changed, 5 insertions(+), 18 deletions(-)