Re: w1_ds2408 linux driver

Re: w1_ds2408 linux driver

[Jean-Francois Dagenais]

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

Re: w1_ds2408 linux driver

[Jean-Francois Dagenais]

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.

[PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option

[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

[PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number

[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

Re: [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option

[Evgeniy Polyakov]

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

Re: [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option

[Jean-François Dagenais]

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

Re: [PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option

[Evgeniy Polyakov]

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

[PATCH 1/2] w1: ds2408: make value read-back check a Kconfig option

[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

[PATCH 2/2] w1: ds2408: use ARRAY_SIZE instead of hard-coded number

[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