* Re: w1_ds2408 linux driver [not found] <51238FBA.9050600@okprods.com> @ 2013-02-19 15:20 ` Jean-Francois Dagenais 2013-03-07 20:09 ` Jean-Francois Dagenais 1 sibling, 0 replies; 9+ messages in thread From: Jean-Francois Dagenais @ 2013-02-19 15:20 UTC (permalink / raw) To: Olivier KSIKES; +Cc: zbr@ioremap.net, open list Hi Olivier, I'm forwarding this to Evgeniy and the linux-kernel mailing list as this thread may have historical value. On 2013-02-19, at 9:44 AM, Olivier KSIKES wrote: > Dear Mr Dagenais, > > I have just tried your w1-ds2408 linux driver on a TP link mini router running openwrt with w1-gpio. I was able to successfully update a 2408 outputs by writing a byte to the /output file. > However, I would like to drive an LCD screen, and this method seems limited to 3/4 chars per second. Yikes! > > I had little more luck driving the 2408 thru the generic w1 driver (using the rw file), where I reached around 25 bytes/s. This is a big surprise to me... Are you sure about the result, i.e. that no errors are occurring? > This is however far from the actual 1wire bus capabilities. Indeed. Did you try turning on the "dev_dbg" prints using "#define DEBUG" at the top of the file? This will give you clues as to whether bus errors are occurring and retries are made. > > Thus, I'm wondering if there is a faster way to output a series of bytes to the 2408 using your driver? > Is there any doc available apart from w1_ds2408.c comments? A quick inspection of the w1_ds2408 driver reveals that for each byte write operation, we need to write 3 bytes, then read 1, then a bus reset, then the resume command (1byte) and after this we go back to read back to see if the value we wrote worked, which is again 3 bytes written, and 1 read. This is certainly much longer that doing it the easy way and may be a bit paranoid. I could see a patch that would make this double checking optional, say when the module is build with "#define DEBUG" or perhaps with a module option in the Kconfig which would have the double checking behaviour on by default. The reason I designed it this way is because 1-wire busses tend to be close to flaky. Certainly the environment I developed the driver in was flaky. Finally we resolved the issues with pull-ups and what not, but there are other examples where the 1-wire bus spec is stretched a bit by certain component. Having a Kconfig value could enable the double check code to be present for stabilization purposes, and then turned off once things work so it can run faster. Send a patch! > > With best regards and congratulations for your great work, > > Olivier Ksikes ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: w1_ds2408 linux driver [not found] <51238FBA.9050600@okprods.com> 2013-02-19 15:20 ` w1_ds2408 linux driver Jean-Francois Dagenais @ 2013-03-07 20:09 ` Jean-Francois Dagenais 2013-03-07 21:07 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Jean-Francois Dagenais 1 sibling, 1 reply; 9+ messages in thread From: Jean-Francois Dagenais @ 2013-03-07 20:09 UTC (permalink / raw) To: Olivier KSIKES; +Cc: open list On 2013-02-19, at 9:44 AM, Olivier KSIKES wrote: > Dear Mr Dagenais, > > I have just tried your w1-ds2408 linux driver on a TP link mini router running openwrt with w1-gpio. I was able to successfully update a 2408 outputs by writing a byte to the /output file. > However, I would like to drive an LCD screen, and this method seems limited to 3/4 chars per second. > > I had little more luck driving the 2408 thru the generic w1 driver (using the rw file), where I reached around 25 bytes/s. > This is however far from the actual 1wire bus capabilities. > > Thus, I'm wondering if there is a faster way to output a series of bytes to the 2408 using your driver? > Is there any doc available apart from w1_ds2408.c comments? > > With best regards and congratulations for your great work, > > Olivier Ksikes Here are my test results. I have a simple main() that opens the /sys/bus/w1/devices/29-0000000xyz/output file and keeps it open. I sweep from 0 to 255 in a tight loop that simply does for (int i=0; i <= 1024; ++i) { file.putChar(i); file.seek(0); } return 0; I invoke this using "time" and do 1024/timespent to get: Current driver (w/read-back ): ~48 chars/seconds Improved driver (wo/read-back): ~98 chars/seconds patch will follow... Cheers. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option 2013-03-07 20:09 ` Jean-Francois Dagenais @ 2013-03-07 21:07 ` Jean-Francois Dagenais 2013-03-07 21:07 ` [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number Jean-Francois Dagenais 2013-03-12 2:09 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Evgeniy Polyakov 0 siblings, 2 replies; 9+ messages in thread From: Jean-Francois Dagenais @ 2013-03-07 21:07 UTC (permalink / raw) To: zbr, linux-kernel; +Cc: olivier, Jean-Francois Dagenais De-activating this reading back will effectively half the time required for a write to the output register. Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/slaves/Kconfig | 10 ++++++++++ drivers/w1/slaves/w1_ds2408.c | 18 ++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/w1/slaves/Kconfig b/drivers/w1/slaves/Kconfig index 762561f..5e6a3c9 100644 --- a/drivers/w1/slaves/Kconfig +++ b/drivers/w1/slaves/Kconfig @@ -22,6 +22,16 @@ config W1_SLAVE_DS2408 Say Y here if you want to use a 1-wire DS2408 8-Channel Addressable Switch device support +config W1_SLAVE_DS2408_READBACK + bool "Read-back values written to DS2408's output register" + depends on W1_SLAVE_DS2408 + default y + help + Enabling this will cause the driver to read back the values written + to the chip's output register in order to detect errors. + + This is slower but useful when debugging chips and/or busses. + config W1_SLAVE_DS2413 tristate "Dual Channel Addressable Switch 0x3a family support (DS2413)" help diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index 441ad3a..25a5168 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -178,6 +178,15 @@ static ssize_t w1_f29_write_output( w1_write_block(sl->master, w1_buf, 3); readBack = w1_read_8(sl->master); + + if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) { + if (w1_reset_resume_command(sl->master)) + goto error; + /* try again, the slave is ready for a command */ + continue; + } + +#ifdef CONFIG_W1_SLAVE_DS2408_READBACK /* here the master could read another byte which would be the PIO reg (the actual pin logic state) since in this driver we don't know which pins are @@ -186,11 +195,6 @@ static ssize_t w1_f29_write_output( if (w1_reset_resume_command(sl->master)) goto error; - if (readBack != 0xAA) { - /* try again, the slave is ready for a command */ - continue; - } - /* go read back the output latches */ /* (the direct effect of the write above) */ w1_buf[0] = W1_F29_FUNC_READ_PIO_REGS; @@ -198,7 +202,9 @@ static ssize_t w1_f29_write_output( w1_buf[2] = 0; w1_write_block(sl->master, w1_buf, 3); /* read the result of the READ_PIO_REGS command */ - if (w1_read_8(sl->master) == *buf) { + if (w1_read_8(sl->master) == *buf) +#endif + { /* success! */ mutex_unlock(&sl->master->bus_mutex); dev_dbg(&sl->dev, -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number 2013-03-07 21:07 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Jean-Francois Dagenais @ 2013-03-07 21:07 ` Jean-Francois Dagenais 2013-03-12 2:09 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Evgeniy Polyakov 1 sibling, 0 replies; 9+ messages in thread From: Jean-Francois Dagenais @ 2013-03-07 21:07 UTC (permalink / raw) To: zbr, linux-kernel; +Cc: olivier, Jean-Francois Dagenais Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/slaves/w1_ds2408.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index 25a5168..e45eca1 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -303,8 +303,7 @@ error: -#define NB_SYSFS_BIN_FILES 6 -static struct bin_attribute w1_f29_sysfs_bin_files[NB_SYSFS_BIN_FILES] = { +static struct bin_attribute w1_f29_sysfs_bin_files[] = { { .attr = { .name = "state", @@ -363,7 +362,7 @@ static int w1_f29_add_slave(struct w1_slave *sl) int err = 0; int i; - for (i = 0; i < NB_SYSFS_BIN_FILES && !err; ++i) + for (i = 0; i < ARRAY_SIZE(w1_f29_sysfs_bin_files) && !err; ++i) err = sysfs_create_bin_file( &sl->dev.kobj, &(w1_f29_sysfs_bin_files[i])); @@ -377,7 +376,7 @@ static int w1_f29_add_slave(struct w1_slave *sl) static void w1_f29_remove_slave(struct w1_slave *sl) { int i; - for (i = NB_SYSFS_BIN_FILES - 1; i >= 0; --i) + for (i = ARRAY_SIZE(w1_f29_sysfs_bin_files) - 1; i >= 0; --i) sysfs_remove_bin_file(&sl->dev.kobj, &(w1_f29_sysfs_bin_files[i])); } -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option 2013-03-07 21:07 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Jean-Francois Dagenais 2013-03-07 21:07 ` [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number Jean-Francois Dagenais @ 2013-03-12 2:09 ` Evgeniy Polyakov 2013-03-12 18:28 ` Jean-François Dagenais 1 sibling, 1 reply; 9+ messages in thread From: Evgeniy Polyakov @ 2013-03-12 2:09 UTC (permalink / raw) To: Jean-Francois Dagenais; +Cc: linux-kernel, olivier Hi On Thu, Mar 07, 2013 at 04:07:03PM -0500, Jean-Francois Dagenais (jeff.dagenais@gmail.com) wrote: > De-activating this reading back will effectively half the time required > for a write to the output register. Are you sure you want this to be compile-time option and not module parameter? -- Evgeniy Polyakov ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option 2013-03-12 2:09 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Evgeniy Polyakov @ 2013-03-12 18:28 ` Jean-François Dagenais 2013-03-15 0:15 ` Evgeniy Polyakov 0 siblings, 1 reply; 9+ messages in thread From: Jean-François Dagenais @ 2013-03-12 18:28 UTC (permalink / raw) To: Evgeniy Polyakov; +Cc: linux-kernel, olivier On 2013-03-11, at 10:09 PM, Evgeniy Polyakov wrote: > Hi > > On Thu, Mar 07, 2013 at 04:07:03PM -0500, Jean-Francois Dagenais (jeff.dagenais@gmail.com) wrote: >> De-activating this reading back will effectively half the time required >> for a write to the output register. > > Are you sure you want this to be compile-time option and not module > parameter? My experience showed that having this kind of check was only useful for HW/SW debug development phases, which usually require debug configs. Later in the development process, when HW and SW are stable, we usually optimize for actual production. Removing un-needed debug features and modules. Assuming that, for historical reasons, implementing this new option as a module parameter would imply initializing it to "true" (i.e. read-back check enabled), then user-space or kernel invocation would have to override this to "false" in order to benefit from the enhancement. I felt that this was an awkward burden to propagate and maintain outside the kernel's config (.config). Especially since usually, the debug and release configs are meant exactly for this kind of stuff. Also, it's not so off from the "Protect DS2433 data with a CRC16" option nearby. We could do the "best of both worlds" and have the Kconfig influence the init value of a new module parameter which controls whether we read-back or not... All that said, kernel style and good practices should win here over my own experiences as the extra run-time "if" check is not an issue really an issue on most platform. Thoughts? /jfd ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option 2013-03-12 18:28 ` Jean-François Dagenais @ 2013-03-15 0:15 ` Evgeniy Polyakov 2013-03-15 18:20 ` Jean-Francois Dagenais 0 siblings, 1 reply; 9+ messages in thread From: Evgeniy Polyakov @ 2013-03-15 0:15 UTC (permalink / raw) To: Jean-François Dagenais; +Cc: linux-kernel, olivier On Tue, Mar 12, 2013 at 02:28:11PM -0400, Jean-François Dagenais (jeff.dagenais@gmail.com) wrote: > Thoughts? > /jfd Well, sounds reasonable. Please submit patches to Greg Kroah-Hartman <greg@kroah.com>, he will push them upstream. Acked-by: Evgeniy Polyakov <zbr@ioremap.net> -- Evgeniy Polyakov ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option 2013-03-15 0:15 ` Evgeniy Polyakov @ 2013-03-15 18:20 ` Jean-Francois Dagenais 2013-03-15 18:20 ` [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number Jean-Francois Dagenais 0 siblings, 1 reply; 9+ messages in thread From: Jean-Francois Dagenais @ 2013-03-15 18:20 UTC (permalink / raw) To: greg, linux-kernel; +Cc: zbr, olivier, Jean-Francois Dagenais De-activating this reading back will effectively half the time required for a write to the output register. Acked-by: Evgeniy Polyakov <zbr@ioremap.net> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/slaves/Kconfig | 10 ++++++++++ drivers/w1/slaves/w1_ds2408.c | 18 ++++++++++++------ 2 files changed, 22 insertions(+), 6 deletions(-) diff --git a/drivers/w1/slaves/Kconfig b/drivers/w1/slaves/Kconfig index 762561f..5e6a3c9 100644 --- a/drivers/w1/slaves/Kconfig +++ b/drivers/w1/slaves/Kconfig @@ -22,6 +22,16 @@ config W1_SLAVE_DS2408 Say Y here if you want to use a 1-wire DS2408 8-Channel Addressable Switch device support +config W1_SLAVE_DS2408_READBACK + bool "Read-back values written to DS2408's output register" + depends on W1_SLAVE_DS2408 + default y + help + Enabling this will cause the driver to read back the values written + to the chip's output register in order to detect errors. + + This is slower but useful when debugging chips and/or busses. + config W1_SLAVE_DS2413 tristate "Dual Channel Addressable Switch 0x3a family support (DS2413)" help diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index 441ad3a..25a5168 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -178,6 +178,15 @@ static ssize_t w1_f29_write_output( w1_write_block(sl->master, w1_buf, 3); readBack = w1_read_8(sl->master); + + if (readBack != W1_F29_SUCCESS_CONFIRM_BYTE) { + if (w1_reset_resume_command(sl->master)) + goto error; + /* try again, the slave is ready for a command */ + continue; + } + +#ifdef CONFIG_W1_SLAVE_DS2408_READBACK /* here the master could read another byte which would be the PIO reg (the actual pin logic state) since in this driver we don't know which pins are @@ -186,11 +195,6 @@ static ssize_t w1_f29_write_output( if (w1_reset_resume_command(sl->master)) goto error; - if (readBack != 0xAA) { - /* try again, the slave is ready for a command */ - continue; - } - /* go read back the output latches */ /* (the direct effect of the write above) */ w1_buf[0] = W1_F29_FUNC_READ_PIO_REGS; @@ -198,7 +202,9 @@ static ssize_t w1_f29_write_output( w1_buf[2] = 0; w1_write_block(sl->master, w1_buf, 3); /* read the result of the READ_PIO_REGS command */ - if (w1_read_8(sl->master) == *buf) { + if (w1_read_8(sl->master) == *buf) +#endif + { /* success! */ mutex_unlock(&sl->master->bus_mutex); dev_dbg(&sl->dev, -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number 2013-03-15 18:20 ` Jean-Francois Dagenais @ 2013-03-15 18:20 ` Jean-Francois Dagenais 0 siblings, 0 replies; 9+ messages in thread From: Jean-Francois Dagenais @ 2013-03-15 18:20 UTC (permalink / raw) To: greg, linux-kernel; +Cc: zbr, olivier, Jean-Francois Dagenais Acked-by: Evgeniy Polyakov <zbr@ioremap.net> Signed-off-by: Jean-Francois Dagenais <jeff.dagenais@gmail.com> --- drivers/w1/slaves/w1_ds2408.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/w1/slaves/w1_ds2408.c b/drivers/w1/slaves/w1_ds2408.c index 25a5168..e45eca1 100644 --- a/drivers/w1/slaves/w1_ds2408.c +++ b/drivers/w1/slaves/w1_ds2408.c @@ -303,8 +303,7 @@ error: -#define NB_SYSFS_BIN_FILES 6 -static struct bin_attribute w1_f29_sysfs_bin_files[NB_SYSFS_BIN_FILES] = { +static struct bin_attribute w1_f29_sysfs_bin_files[] = { { .attr = { .name = "state", @@ -363,7 +362,7 @@ static int w1_f29_add_slave(struct w1_slave *sl) int err = 0; int i; - for (i = 0; i < NB_SYSFS_BIN_FILES && !err; ++i) + for (i = 0; i < ARRAY_SIZE(w1_f29_sysfs_bin_files) && !err; ++i) err = sysfs_create_bin_file( &sl->dev.kobj, &(w1_f29_sysfs_bin_files[i])); @@ -377,7 +376,7 @@ static int w1_f29_add_slave(struct w1_slave *sl) static void w1_f29_remove_slave(struct w1_slave *sl) { int i; - for (i = NB_SYSFS_BIN_FILES - 1; i >= 0; --i) + for (i = ARRAY_SIZE(w1_f29_sysfs_bin_files) - 1; i >= 0; --i) sysfs_remove_bin_file(&sl->dev.kobj, &(w1_f29_sysfs_bin_files[i])); } -- 1.8.1.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-03-15 18:21 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <51238FBA.9050600@okprods.com>
2013-02-19 15:20 ` w1_ds2408 linux driver Jean-Francois Dagenais
2013-03-07 20:09 ` Jean-Francois Dagenais
2013-03-07 21:07 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Jean-Francois Dagenais
2013-03-07 21:07 ` [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number Jean-Francois Dagenais
2013-03-12 2:09 ` [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option Evgeniy Polyakov
2013-03-12 18:28 ` Jean-François Dagenais
2013-03-15 0:15 ` Evgeniy Polyakov
2013-03-15 18:20 ` Jean-Francois Dagenais
2013-03-15 18:20 ` [PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number Jean-Francois Dagenais
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox