Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
On Sat, Dec 22, 2012 at 12:21:15PM +0100, Federico Vaga wrote: > > I'm cautious about adding operational interfaces to sysfs because it > > can be quite difficult to get the locking right. To begin with it > > splits up a single interface into multiple files, any of which can > > be held open by a process. Then there is the question of ordering > > of operations when there are multiple users. For instance, if there > > were two users, each of which using different transfer parameters, > > a sysfs interface doesn't provide any mechanism to group setting up > > the device with the transfer. > > > > These are lessons learned the hard way with the gpio sysfs abi. I > > don't want to get caught in the same trap for spi. > > > > g. > > I understand the problem, but I think that for very simple test on > devices, sysfs is easier. For example, it happens that a SPI device > does not work correctly with a driver, so I want to verify the SPI > traffic by writing directly the device commands and check with an > oscilloscope. I think that an easy way is to use sysfs like this: > > echo 123456 > speed_hz > [other options if needed] > echo -n -e "\x12\x34" > /dev/spidev1.1 > [oscilloscope] > hexdump -n 2 /dev/spidev1.1 > > This sysfs interface should be used only for testing/debugging, not to > develop an user space driver on it; moreover, the ioctl interface is > still there. Then move it to debugfs, don't put debugging code in sysfs, as once you do, you are stuck with it for life (hint, you forgot to add Documentation/ABI entries for your new sysfs files, which are required). If you move this to debugfs, you can do whatever you want, as it's obvious no one would ever put "real" interfaces in debugfs :) thanks, greg k-h -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: MPC866, Help, SPI blocked at startup
Hi Leroy, Did you find a solution to your problem or did some answer you? I have a similar problem and would like to have some info if you have. Thanks, Yaniv -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
> I'm cautious about adding operational interfaces to sysfs because it > can be quite difficult to get the locking right. To begin with it > splits up a single interface into multiple files, any of which can > be held open by a process. Then there is the question of ordering > of operations when there are multiple users. For instance, if there > were two users, each of which using different transfer parameters, > a sysfs interface doesn't provide any mechanism to group setting up > the device with the transfer. > > These are lessons learned the hard way with the gpio sysfs abi. I > don't want to get caught in the same trap for spi. > > g. I understand the problem, but I think that for very simple test on devices, sysfs is easier. For example, it happens that a SPI device does not work correctly with a driver, so I want to verify the SPI traffic by writing directly the device commands and check with an oscilloscope. I think that an easy way is to use sysfs like this: echo 123456 > speed_hz [other options if needed] echo -n -e "\x12\x34" > /dev/spidev1.1 [oscilloscope] hexdump -n 2 /dev/spidev1.1 This sysfs interface should be used only for testing/debugging, not to develop an user space driver on it; moreover, the ioctl interface is still there. spidev_test and spidev_fdx does not allow me to customize tx buffer and I think (very personal opinion) that for debugging purpose is better sysfs with well known programs (echo, cat, hexdump, od) and oscilloscope. I know that I'm not so persuasive :) I can develop a simple program that can write custom tx buf with ioctl -- Federico Vaga -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: davinci: use request_threaded_irq() to fix deadlock
On Fri, 21 Dec 2012 15:13:26 -0500, Murali Karicheri
wrote:
> With RT pre-empt patch applied to Linux kernel, the irq handler will be
> force converted to an irq thread. spi driver can get back to back messages
> from the slave device. In such cases, IRQ thread doesn't get a chance to
> run to read the slave data. Hence the irq handler must be run in hard irq
> context to read/write data from slave device. Otherwise, the kernel goes
> into a deadlock. This patch fixes this issue when PREEMPT_RT_FULL is
> enabled in the kernel. A dummy thread function is provided to satisfy the
> request_threaded_irq() API. Passing a NULL for function also causes the
> irq handler to be executed in the thread context.
>
> Signed-off-by: Murali Karicheri
Thomas, would you mind taking a look at this for me. My gut feel is that
this is the wrong way to solve the problem that Murali is having and
that it really just hacks something that works.
It seems to me that what the driver should do is disable the irq
in the handler, and then perform the regular work in the thread. After
all the pending data is processed, then it should reenable the interrupts.
Passing in a dummy thread function just looks wrong.
g.
> ---
> drivers/spi/spi-davinci.c | 17 +++--
> 1 file changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-davinci.c b/drivers/spi/spi-davinci.c
> index 50bd2cd..8234d22 100644
> --- a/drivers/spi/spi-davinci.c
> +++ b/drivers/spi/spi-davinci.c
> @@ -702,6 +702,19 @@ err_alloc_dummy_buf:
> }
>
> /**
> + * dummy_thread_fn - dummy thread function
> + * @irq: IRQ number for this SPI Master
> + * @context_data: structure for SPI Master controller davinci_spi
> + *
> + * This is to satisfy the request_threaded_irq() API so that the irq
> + * handler is called in interrupt context.
> + */
> +static irqreturn_t dummy_thread_fn(s32 irq, void *data)
> +{
> + return IRQ_HANDLED;
> +}
> +
> +/**
> * davinci_spi_irq - Interrupt handler for SPI Master Controller
> * @irq: IRQ number for this SPI Master
> * @context_data: structure for SPI Master controller davinci_spi
> @@ -899,8 +912,8 @@ static int davinci_spi_probe(struct platform_device *pdev)
> goto unmap_io;
> }
>
> - ret = request_irq(dspi->irq, davinci_spi_irq, 0, dev_name(&pdev->dev),
> - dspi);
> + ret = request_threaded_irq(dspi->irq, davinci_spi_irq, dummy_thread_fn,
> + 0, dev_name(&pdev->dev), dspi);
> if (ret)
> goto unmap_io;
>
> --
> 1.7.9.5
>
--
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.
--
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 5/5] spi: fix return value check in hspi_probe().
On Wed, 19 Dec 2012 19:39:14 +0300, Dan Carpenter wrote: > On Wed, Dec 19, 2012 at 03:11:54PM +, Grant Likely wrote: > > On Tue, 11 Dec 2012 16:36:27 -0800 (PST), Kuninori Morimoto > > wrote: > > > > > > Hi > > > > > > > According to its documentation, clk_get() returns a "valid IS_ERR() > > > > condition > > > > containing errno", so we should call IS_ERR() rather than a NULL check. > > > > > > > > Signed-off-by: Cyril Roelandt > > > > > > Acked-by: Kuninori Morimoto > > > > Applied, thanks. > > In another thread, we were just talking about who clk_get() can > return a NULL if !CONFIG_HAVE_CLK. That might change to match the > documentation later... Not sure. So what is the solution here? Will the dummy clk_get() be changed, or is there more work needed on the drivers? /me stifles a rant about the PTR_ERR pattern g. -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH RFC] spidev.c: add sysfs attributes for SPI configuration
On Thu, 20 Dec 2012 16:30:36 +0100, Federico Vaga wrote: > On Wednesday 19 December 2012 15:09:25 Grant Likely wrote: > > Not a good idea. sysfs is not a good place for operational > > interfaces. Please use the spi character devices for direct > > manipulation of the SPI configuration. > > Hello, > > Can you explain why it is not a good idea? I do not understand; what > is the advantage of ioctl through char device? Or what it the issue > with sysfs? > > Thank you very much I'm cautious about adding operational interfaces to sysfs because it can be quite difficult to get the locking right. To begin with it splits up a single interface into multiple files, any of which can be held open by a process. Then there is the question of ordering of operations when there are multiple users. For instance, if there were two users, each of which using different transfer parameters, a sysfs interface doesn't provide any mechanism to group setting up the device with the transfer. These are lessons learned the hard way with the gpio sysfs abi. I don't want to get caught in the same trap for spi. g. -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2] spi: remove check for bits_per_word on transfer from low level driver
On Thu, 20 Dec 2012 11:33:47 +0530, Laxman Dewangan
wrote:
> On Wednesday 19 December 2012 09:54 PM, Grant Likely wrote:
> > On Tue, 18 Dec 2012 14:25:43 +0530, Laxman Dewangan
> > wrote:
> >> The spi core make sure that each transfer structure have the proper
> >> setting for bits_per_word before calling low level transfer APIs.
> >>
> >> Hence it is no more require to check again in low level driver for
> >> this field whether this is set correct or not. Removing such code
> >> from low level driver.
> >>
> >> Signed-off-by: Laxman Dewangan
> > [...]
> >> */
> >>
> >>if (prev_speed_hz != speed_hz
> >> @@ -316,9 +315,8 @@ static int txx9spi_transfer(struct spi_device *spi,
> >> struct spi_message *m)
> >>/* check each transfer's parameters */
> >>list_for_each_entry (t,&m->transfers, transfer_list) {
> >>u32 speed_hz = t->speed_hz ? : spi->max_speed_hz;
> >> - u8 bits_per_word = t->bits_per_word ? : spi->bits_per_word;
> >> + u8 bits_per_word = t->bits_per_word;
> >>
> >> - bits_per_word = bits_per_word ? : 8;
> > Have you verified here that bits_per_word can never be '0' here? What is
> > the path that ensures spi->bits_per_word (and hence t->bits_per_word) is
> > set to 8 here?
> >
> > Otherwise the patch looks good. Thanks for doing this work.
>
>
> When we do the spi_add_device(), the spi_setup() get called.
> In spi_setup, it make sure that spi->bits_per_word is not zero.
>
> in spi_setup(spi.c)
>
> if (!spi->bits_per_word)
> spi->bits_per_word = 8;
Applied, thanks.
g.
--
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: Add the flag indicate to registe new device as children of master or not.
On Fri, 21 Dec 2012 12:39:52 -0500, Jun Chen wrote: > On Wed, 2012-12-19 at 16:21 +, Grant Likely wrote: > > On Wed, 19 Dec 2012 09:04:16 +, Mark Brown > > wrote: > > > On Wed, Dec 19, 2012 at 04:44:03AM -0500, Jun Chen wrote: > > > > > > > This spi_alloc_device function will be called in the spi_new_device > > > > function to alloc new device as the master. But other way, it is called > > > > by the of_register_spi_devices function to register new device as the > > > > children of the master. I will update changlog to add it. > > > > > > But why is this a bad thing? You've said what's happening but not why > > > it's a problem. > > > > spi_devices should always be children of the spi_master. If that is not > > the case then it is a bug to be fixed. > > > > When many boards initializing, boards will call function > spi_register_board_info to create bi->board_info,Then spi driver probe > to call spi_register_master to register the driver and in the function > spi_match_master_to_boardinfo To create new spi device, and this cases > the spi_devices are not children of the spi_master. > Many drivers do these steps. If all spi_devices must be children of the > spi_master, Do spi core have plan to delete this way? > Or spi core can hold this way for many drivers. Let me make sure I understand what you're saying... Right now, every spi_device object is registered as a child of an spi_master object. With this proposed patch, spi_devices registered via spi_register_board_info will be siblings of the spi_master instead of children. Do I understand correctly so far? The problem is that I don't understand why this change is necessary. spi_devices should always be children of an spi_master, not siblings. What is the problem you're trying to solve with this change? g. -- LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial Remotely access PCs and mobile devices and provide instant support Improve your efficiency, and focus on delivering more value-add services Discover what IT Professionals Know. Rescue delivers http://p.sf.net/sfu/logmein_12329d2d ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH] spi: spi-mpc512x-psc: don't use obsolet cell-index property
On Fri, 21 Dec 2012 14:48:38 +, Grant Likely
wrote:
> On Fri, Dec 21, 2012 at 2:43 PM, Anatolij Gustschin wrote:
> > Use unique PSCx register base offset to obtain the
> > SPI PSC number used for SPI bus id.
> >
> > Signed-off-by: Anatolij Gustschin
>
> Don't do this. If you really want to assign a specific bus number,
> then use a property in /aliases. cell-index has been deprecated a very
> long time ago now.
Ummm.. I really should read patches before I reply to them. I see you're
removing cell-index, not adding it back in. It is fine.
Aliases would be a more generic solution though. Would this following
change work for you? Try it out and let me know.
g.
---
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 84c2861..de9f6ee 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -983,6 +983,9 @@ int spi_register_master(struct spi_master *master)
if (master->num_chipselect == 0)
return -EINVAL;
+ if ((master->bus_num < 0) && master->dev.of_node)
+ master->bus_num = of_alias_get_id(master->dev.of_node, "spi");
+
/* convention: dynamically assigned bus IDs count down from the max */
if (master->bus_num < 0) {
/* FIXME switch to an IDR based scheme, something like
--
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
