Re: [PATCH] spi: Unlock a spinlock before calling into the controller driver.
Hi,
(Adding Linus Walleij and Mark Brown since they were the ones doing
the queue changes).
On Fri, Jun 22, 2012 at 4:53 PM, Bryan Freed bfr...@chromium.org wrote:
spi_pump_messages() calls into a controller driver with
unprepare_transfer_hardware() which is documented as This may sleep.
As in the prepare_transfer_hardware() call below, we should release the
queue_lock spinlock before making the call.
Rework the logic a bit to hold queue_lock to protect the 'busy' flag,
then release it to call unprepare_transfer_hardware().
Signed-off-by: Bryan Freed bfr...@chromium.org
---
drivers/spi/spi.c | 15 +++
1 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index fc0da39..f7f9df9 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -532,17 +532,16 @@ static void spi_pump_messages(struct kthread_work *work)
/* Lock queue and check for queue work */
spin_lock_irqsave(master-queue_lock, flags);
if (list_empty(master-queue) || !master-running) {
- if (master-busy master-unprepare_transfer_hardware) {
- ret = master-unprepare_transfer_hardware(master);
- if (ret) {
- spin_unlock_irqrestore(master-queue_lock,
flags);
- dev_err(master-dev,
- failed to unprepare transfer
hardware\n);
- return;
- }
+ if (!master-busy) {
+ spin_unlock_irqrestore(master-queue_lock, flags);
+ return;
}
master-busy = false;
spin_unlock_irqrestore(master-queue_lock, flags);
+ if (master-unprepare_transfer_hardware
+ master-unprepare_transfer_hardware(master))
+ dev_err(master-dev,
+ failed to unprepare transfer hardware\n);
I'm not sure this is safe to do. The lock is dropped for the prepare
side, but in that case we can be sure that the above code can't come
in and unprepare at the same time since there is per definition a
request on the queue at that time.
On the other hand, between the lock drop and the call to unprepare
above, another code path can come in and queue up a request and either
not do prepare properly, or there will be a prepare that races with
the unprepare.
It might make more sense to use a workqueue here and schedule a
unprepare call that way instead (and cancelling appropriately before
the prepare call if needed). I'm open for other suggestions as well.
-Olof
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 02/10] spi: s3c64xx: move controller information into driver data
Hi,
Some comments below.
On Tue, May 8, 2012 at 3:04 PM, Thomas Abraham
thomas.abra...@linaro.org wrote:
Platform data is used to specify controller hardware specific information
such as the tx/rx fifo level mask and bit offset of rx fifo level. Such
information is not suitable to be supplied from device tree. Instead,
it can be moved into the driver data and removed from platform data.
Cc: Jaswinder Singh jaswinder.si...@linaro.org
Signed-off-by: Thomas Abraham thomas.abra...@linaro.org
---
drivers/spi/spi-s3c64xx.c | 180
++---
1 files changed, 153 insertions(+), 27 deletions(-)
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6a3d51a..f6bc0e3 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -171,6 +198,8 @@ struct s3c64xx_spi_driver_data {
struct s3c64xx_spi_dma_data rx_dma;
struct s3c64xx_spi_dma_data tx_dma;
struct samsung_dma_ops *ops;
+ struct s3c64xx_spi_port_config *port_conf;
+ unsigned port_id;
unsigned int
@@ -942,6 +964,13 @@ static void s3c64xx_spi_hwinit(struct
s3c64xx_spi_driver_data *sdd, int channel)
flush_fifo(sdd);
}
+static inline struct s3c64xx_spi_port_config *s3c64xx_spi_get_port_config(
+ struct platform_device *pdev)
+{
+ return (struct s3c64xx_spi_port_config *)
+ platform_get_device_id(pdev)-driver_data;
+}
+
static int __init s3c64xx_spi_probe(struct platform_device *pdev)
{
struct resource *mem_res, *dmatx_res, *dmarx_res;
@@ -1000,6 +1029,7 @@ static int __init s3c64xx_spi_probe(struct
platform_device *pdev)
platform_set_drvdata(pdev, master);
sdd = spi_master_get_devdata(master);
+ sdd-port_conf = s3c64xx_spi_get_port_config(pdev);
sdd-master = master;
sdd-cntrlr_info = sci;
sdd-pdev = pdev;
Single-use helper? Might as well open code it in this case.
@@ -1227,6 +1258,100 @@ static const struct dev_pm_ops s3c64xx_spi_pm = {
s3c64xx_spi_runtime_resume, NULL)
};
+#if defined(CONFIG_CPU_S3C2416) || defined(CONFIG_CPU_S3C2443)
+struct s3c64xx_spi_port_config s3c2443_spi_port_config = {
+ .fifo_lvl_mask = { 0x7f },
+ .rx_lvl_offset = 13,
+ .tx_st_done = 21,
+ .high_speed = true,
+};
+#define S3C2443_SPI_PORT_CONFIG ((kernel_ulong_t)s3c2443_spi_port_config)
+#else
+#define S3C2443_SPI_PORT_CONFIG ((kernel_ulong_t)NULL)
+#endif
Is it really worth it to do the ifdefs here for just 16 bytes of data
per entry? The table itself below takes more space.
[..]
+#ifdef CONFIG_ARCH_S5PV210
+struct s3c64xx_spi_port_config s5pv210_spi_port_config = {
+ .fifo_lvl_mask = { 0x1ff, 0x7F },
+ .rx_lvl_offset = 15,
+ .tx_st_done = 25,
+ .high_speed = 1,
high_speed = true
+};
+#define S5PV210_SPI_PORT_CONFIG ((kernel_ulong_t)s5pv210_spi_port_config)
+#else
+#define S5PV210_SPI_PORT_CONFIG ((kernel_ulong_t)NULL)
+#endif /* CONFIG_ARCH_S5PV210 */
+
+#ifdef CONFIG_ARCH_EXYNOS4
+struct s3c64xx_spi_port_config exynos4_spi_port_config = {
+ .fifo_lvl_mask = { 0x1ff, 0x7F, 0x7F },
+ .rx_lvl_offset = 15,
+ .tx_st_done = 25,
+ .high_speed = 1,
high_speed = true
-Olof
--
Live Security Virtual Conference
Exclusive live event will cover all the ways today's security and
threat landscape has changed and how IT managers can respond. Discussions
will include endpoint security, mobile security and the latest in malware
threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v2 4/6] ARM: Samsung: Modify s3c64xx_spi{0|1|2}_set_platdata function
On Sun, May 20, 2012 at 2:21 AM, Mark Brown broo...@opensource.wolfsonmicro.com wrote: On Fri, May 18, 2012 at 03:03:31PM +0530, Thomas Abraham wrote: - s3c64xx_spi0_set_platdata(s3c64xx_spi0_pdata, 0, 1); + s3c64xx_spi0_set_platdata(s3c6410-spi, NULL, 0, 1); ... + pd.src_clk_nr = src_clk_nr; + pd.cfg_gpio = (cfg_gpio) ? cfg_gpio : s3c64xx_spi0_cfg_gpio; + s3c64xx_device_spi0.name = dev_name; This looks *really* strange. Why do we need to pass in a name to set in s3c64xx_device_spi0's name, why would we want to use a different name? This dev_name also isn't equivalent to dev_name() which makes matters more confusing than they need to be. There was similar code for the I2C controllers which caused some really hard to debug brittleness. Looks like they use the name as the magic string to pick initialization data. I wonder if it makes more sense to keep the platform_data still around, and just fill it in with the table (from patch spi: s3c64xx: move controller information into driver data) based on the OF compatible field for the device-tree probe case, instead of trying to overload and having to change the name in this way. -Olof -- Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH] spi/tegra: depend instead of select TEGRA_SYSTEM_DMA
It's unlikely that anyone ever wants to turn off SYSTEM_DMA, but just in case, it makes more sense to have the driver depend on it than select it. Signed-off-by: Olof Johansson o...@lixom.net --- drivers/spi/Kconfig |3 +-- 1 files changed, 1 insertions(+), 2 deletions(-) diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig index 3d292be..326f77a 100644 --- a/drivers/spi/Kconfig +++ b/drivers/spi/Kconfig @@ -332,8 +332,7 @@ config SPI_STMP3XXX config SPI_TEGRA tristate Nvidia Tegra SPI controller - depends on ARCH_TEGRA - select TEGRA_SYSTEM_DMA + depends on ARCH_TEGRA TEGRA_SYSTEM_DMA help SPI driver for NVidia Tegra SoCs -- 1.7.8.GIT -- Write once. Port to many. Get the SDK and tools to simplify cross-platform app development. Create new or port existing apps to sell to consumers worldwide. Explore the Intel AppUpSM program developer opportunity. appdeveloper.intel.com/join http://p.sf.net/sfu/intel-appdev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 2/2] gpio/tegra: add devicetree support
On Wed, Jun 15, 2011 at 12:38 PM, Grant Likely grant.lik...@secretlab.ca wrote: Add support for decoding gpios from the device tree Signed-off-by: Grant Likely grant.lik...@secretlab.ca Acked-by: Olof Johansson o...@lixom.net -- EditLive Enterprise is the world's most technically advanced content authoring tool. Experience the power of Track Changes, Inline Image Editing and ensure content is compliant with Accessibility Checking. http://p.sf.net/sfu/ephox-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/2] gpio/tegra: Move Tegra gpio driver to drivers/gpio
On Wed, Jun 15, 2011 at 12:37 PM, Grant Likely grant.lik...@secretlab.ca wrote: As part of the gpio driver consolidation, this patch moves the Tegra driver into drivers/gpio Signed-off-by: Grant Likely grant.lik...@secretlab.ca Acked-by: Olof Johansson o...@lixom.net Colin said he had plans to do it, or I would already have sent this out. Thanks for doing it! :) -Olof -- EditLive Enterprise is the world's most technically advanced content authoring tool. Experience the power of Track Changes, Inline Image Editing and ensure content is compliant with Accessibility Checking. http://p.sf.net/sfu/ephox-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
