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.
Signed-off-by: Jean-Francois Dagenais <[email protected]>
---
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
De-activating this reading back will effectively half the time required
for a write to the output register.
Signed-off-by: Jean-Francois Dagenais <[email protected]>
---
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
Hi
On Thu, Mar 07, 2013 at 04:07:03PM -0500, Jean-Francois Dagenais ([email protected]) 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
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 ([email protected]) 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-
On Tue, Mar 12, 2013 at 02:28:11PM -0400, Jean-François Dagenais ([email protected]) wrote:
> Thoughts?
> /jfd
Well, sounds reasonable.
Please submit patches to Greg Kroah-Hartman <[email protected]>, he will
push them upstream.
Acked-by: Evgeniy Polyakov <[email protected]>
--
Evgeniy Polyakov
Acked-by: Evgeniy Polyakov <[email protected]>
Signed-off-by: Jean-Francois Dagenais <[email protected]>
---
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
De-activating this reading back will effectively half the time required
for a write to the output register.
Acked-by: Evgeniy Polyakov <[email protected]>
Signed-off-by: Jean-Francois Dagenais <[email protected]>
---
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