Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Thu, Apr 12, 2012 at 12:40:30AM +0200, Markus Franke wrote:
> This patch adds a 1-wire slave device driver for the DS28E04-100.
> 
> Signed-off-by: Markus Franke <franm@hrz.tu-chemnitz.de>
> Acked-by: Evgeniy Polyakov <zbr@ioremap.net>

Andrew asked me to relook at this patch, and it turns out to be pretty
messy, I should have never applid it.

You have a lot of checkpatch warnings and errors, and you are creating
new sysfs files with no documentation at all as to what you are doing,
and why you are doing it.  sysfs binary files at that, which should not
be used by ANY code that is trying to intrepret the data being sent to
those files, sysfs binary files are for "pass-through" mode only.

So please redo this patch, cleaning up all of the warnings and errors,
and use the standard kernel interfaces for this type of stuff (i.e. not
sysfs binary files.)

I've reverted it from my tree now, sorry.

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Greg,

Am 30.04.2012 04:13, schrieb Greg KH:
> On Thu, Apr 12, 2012 at 12:40:30AM +0200, Markus Franke wrote:
>> This patch adds a 1-wire slave device driver for the DS28E04-100.
>>
>> Signed-off-by: Markus Franke<franm@hrz.tu-chemnitz.de>
>> Acked-by: Evgeniy Polyakov<zbr@ioremap.net>
>
> You have a lot of checkpatch warnings and errors, and you are creating

Don't have a clue what you mean. You were able to apply the patch 
successfully in the past.

> new sysfs files with no documentation at all as to what you are doing,
> and why you are doing it.  sysfs binary files at that, which should not
> be used by ANY code that is trying to intrepret the data being sent to
> those files, sysfs binary files are for "pass-through" mode only.

Well, I just stuck to the way things are done in already existing 
drivers e.g. drivers/w1/slaves/w1_ds2433.c

@Evgeniy: Any comments on this?

Best regards,

Markus Franke

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Wed, May 02, 2012 at 10:12:43PM +0200, Markus Franke wrote:
> Dear Greg,
> 
> Am 30.04.2012 04:13, schrieb Greg KH:
> >On Thu, Apr 12, 2012 at 12:40:30AM +0200, Markus Franke wrote:
> >>This patch adds a 1-wire slave device driver for the DS28E04-100.
> >>
> >>Signed-off-by: Markus Franke<franm@hrz.tu-chemnitz.de>
> >>Acked-by: Evgeniy Polyakov<zbr@ioremap.net>
> >
> >You have a lot of checkpatch warnings and errors, and you are creating
> 
> Don't have a clue what you mean. You were able to apply the patch
> successfully in the past.

Applying it was fine, but the patch created lots of problems if you ran
it through the scripts/checkpatch.pl tool.  Please fix all of those
warnings and errors up.

> >new sysfs files with no documentation at all as to what you are doing,
> >and why you are doing it.  sysfs binary files at that, which should not
> >be used by ANY code that is trying to intrepret the data being sent to
> >those files, sysfs binary files are for "pass-through" mode only.
> 
> Well, I just stuck to the way things are done in already existing
> drivers e.g. drivers/w1/slaves/w1_ds2433.c

Really?  Ick, where are those files documented?

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Evgeniy Polyakov]

On Wed, May 02, 2012 at 01:21:34PM -0700, Greg KH (greg@kroah.com) wrote:
> > Well, I just stuck to the way things are done in already existing
> > drivers e.g. drivers/w1/slaves/w1_ds2433.c
> 
> Really?  Ick, where are those files documented?

I'm kinda lost here.

To clarify things a bit - w1 allows to create 'private' sysfs files for
special functionality which exists only in given driver - like
enable/disable checksum and so on. It has to be documented and must
follow common kernel codying standards of course.

-- 
	Evgeniy Polyakov

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Evgeniy,

I have reworked the patch and now I am not getting any checkpatch
warnings or errors anymore.

How about the binary sysfs files? If I understand correctly I would add
a documentation file under Documentation/w1/slaves which documents both
sysfs files (eeprom & pio).

@Greg: Could you live with those driver specific binary sysfs files?

Best regards,
Markus Franke

Am 03.05.2012 20:00, schrieb Evgeniy Polyakov:

> On Wed, May 02, 2012 at 01:21:34PM -0700, Greg KH (greg@kroah.com) wrote:
>>> Well, I just stuck to the way things are done in already existing
>>> drivers e.g. drivers/w1/slaves/w1_ds2433.c
>>
>> Really?  Ick, where are those files documented?
> 
> I'm kinda lost here.
> 
> To clarify things a bit - w1 allows to create 'private' sysfs files for
> special functionality which exists only in given driver - like
> enable/disable checksum and so on. It has to be documented and must
> follow common kernel codying standards of course.
> 

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Wed, May 09, 2012 at 10:37:24PM +0200, Markus Franke wrote:
> Dear Evgeniy,
> 
> I have reworked the patch and now I am not getting any checkpatch
> warnings or errors anymore.
> 
> How about the binary sysfs files? If I understand correctly I would add
> a documentation file under Documentation/w1/slaves which documents both
> sysfs files (eeprom & pio).
> 
> @Greg: Could you live with those driver specific binary sysfs files?

Why do you need them?  What are they there for?

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Greg,

the file "pio" is used for reading the current PIO state of the
DS28E04-100. The "eeprom" file is for reading/writing the EEPROM memory
of the same device.

As already mentioned before this is exactly the way how other w1 slave
drivers are handling stuff like this. I could also wrap everything
within a character device driver if this the way you would prefer. If
not, then please give me some advise how to exchange these couple of
bytes between user and kernel space.

Best regards,
Markus Franke

Am 10.05.2012 00:06, schrieb Greg KH:

> On Wed, May 09, 2012 at 10:37:24PM +0200, Markus Franke wrote:
>> Dear Evgeniy,
>>
>> I have reworked the patch and now I am not getting any checkpatch
>> warnings or errors anymore.
>>
>> How about the binary sysfs files? If I understand correctly I would add
>> a documentation file under Documentation/w1/slaves which documents both
>> sysfs files (eeprom & pio).
>>
>> @Greg: Could you live with those driver specific binary sysfs files?
> 
> Why do you need them?  What are they there for?
> 
> greg k-h
> 

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Thu, May 10, 2012 at 12:16:23AM +0200, Markus Franke wrote:
> Dear Greg,
> 
> the file "pio" is used for reading the current PIO state of the
> DS28E04-100.

Why is this a binary file?  That seems like it should not be, as a
binary sysfs file should be "pass-through" only.

> The "eeprom" file is for reading/writing the EEPROM memory of the same
> device.

That seems ok.

> As already mentioned before this is exactly the way how other w1 slave
> drivers are handling stuff like this.

Then those drivers are wrong as well, sorry :(

thanks,

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Greg,

Am 10.05.2012 00:24, schrieb Greg KH:

>> the file "pio" is used for reading the current PIO state of the
>> DS28E04-100.
> 
> Why is this a binary file?  That seems like it should not be, as a
> binary sysfs file should be "pass-through" only.


Well, the data read/written through this file is actually directly
"passed" on to the device. There are two bits which reflect the current
state of the PIO pins of the DS28E04. However, I must admit that this
could also be implemented as regular sysfs attributes.

Best regards,
Markus Franke

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Evgeniy Polyakov]

On Thu, May 10, 2012 at 12:37:56AM +0200, Markus Franke (markus.franke@s2002.tu-chemnitz.de) wrote:
> Well, the data read/written through this file is actually directly
> "passed" on to the device. There are two bits which reflect the current
> state of the PIO pins of the DS28E04. However, I must admit that this
> could also be implemented as regular sysfs attributes.

Greg, could you please describe what is exactly wrong with binary sysfs
file? Or am I missing that it is not usual sysfs file but some special
stuff?

-- 
	Evgeniy Polyakov

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Thu, May 10, 2012 at 03:57:23AM +0400, Evgeniy Polyakov wrote:
> On Thu, May 10, 2012 at 12:37:56AM +0200, Markus Franke (markus.franke@s2002.tu-chemnitz.de) wrote:
> > Well, the data read/written through this file is actually directly
> > "passed" on to the device. There are two bits which reflect the current
> > state of the PIO pins of the DS28E04. However, I must admit that this
> > could also be implemented as regular sysfs attributes.
> 
> Greg, could you please describe what is exactly wrong with binary sysfs
> file? Or am I missing that it is not usual sysfs file but some special
> stuff?

Binary sysfs files should be "pass through" only, the kernel should not
touch the data involved in them at all, it is a pipe directly from the
kernel to userspace for binary blob data, like firmware images.  You
should never do any processing of any binary file data at all in the
kernel.

Hope this helps,

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Evgeniy Polyakov]

On Wed, May 09, 2012 at 05:01:26PM -0700, Greg KH (greg@kroah.com) wrote:
> Binary sysfs files should be "pass through" only, the kernel should not
> touch the data involved in them at all, it is a pipe directly from the
> kernel to userspace for binary blob data, like firmware images.  You
> should never do any processing of any binary file data at all in the
> kernel.

And if some of the data should be somehow changed, what interfact should
be used? Also, Markus, does DS28E04 change written/read data when doing
IO?

I must admit, I never heared that binary sysfs files have to follow this
constraint.

-- 
	Evgeniy Polyakov

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Thu, May 10, 2012 at 04:43:50AM +0400, Evgeniy Polyakov wrote:
> On Wed, May 09, 2012 at 05:01:26PM -0700, Greg KH (greg@kroah.com) wrote:
> > Binary sysfs files should be "pass through" only, the kernel should not
> > touch the data involved in them at all, it is a pipe directly from the
> > kernel to userspace for binary blob data, like firmware images.  You
> > should never do any processing of any binary file data at all in the
> > kernel.
> 
> And if some of the data should be somehow changed, what interfact should
> be used? Also, Markus, does DS28E04 change written/read data when doing
> IO?
> 
> I must admit, I never heared that binary sysfs files have to follow this
> constraint.

For some reason I thought I had documented it years ago, but in digging
through the Documentation directory, I don't see it anywhere, sorry
about that.

I'll add it to my ever-growing TODO list...

thanks,

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Evgeniy,

Am 10.05.2012 02:43, schrieb Evgeniy Polyakov:

> be used? Also, Markus, does DS28E04 change written/read data when doing
> IO?


Apart from masking the upper 6 bits within w1_f1C_write_pio() the data
is directly being passed down to the DS28E04. So actually there is no
other processing done within the driver. Furthermore, I added a new
Documentation file to Documentation/w1/slaves directory which describes
the interface of this driver.

@Greg: Is the "pio" binary file now legitimate or do I have to change it
to a regular sysfs attribute?

Best regards,
Markus Franke

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Thu, May 10, 2012 at 06:55:41AM +0200, Markus Franke wrote:
> Dear Evgeniy,
> 
> Am 10.05.2012 02:43, schrieb Evgeniy Polyakov:
> 
> > be used? Also, Markus, does DS28E04 change written/read data when doing
> > IO?
> 
> 
> Apart from masking the upper 6 bits within w1_f1C_write_pio() the data
> is directly being passed down to the DS28E04. So actually there is no
> other processing done within the driver. Furthermore, I added a new
> Documentation file to Documentation/w1/slaves directory which describes
> the interface of this driver.
> 
> @Greg: Is the "pio" binary file now legitimate or do I have to change it
> to a regular sysfs attribute?

I don't remember anything anymore, you will have to send the patch for
me to be able to properly review it and determine this.

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Greg,

Am 10.05.2012 17:16, schrieb Greg KH:

> I don't remember anything anymore, you will have to send the patch for
> me to be able to properly review it and determine this.


here comes the updated patch.

>From 3ef34f86788bfe39fa5bdb07b7e0598d3fda1f4f Mon Sep 17 00:00:00 2001
From: Markus Franke <franm@hrz.tu-chemnitz.de>
Date: Fri, 11 May 2012 00:41:35 +0200
Subject: [PATCH] Add 1-wire slave device driver for DS28E04-100


Signed-off-by: Markus Franke <franm@hrz.tu-chemnitz.de>
---
 Documentation/w1/slaves/w1_ds28e04 |   36 +++
 drivers/w1/slaves/Kconfig          |   13 +
 drivers/w1/slaves/Makefile         |    1 +
 drivers/w1/slaves/w1_ds28e04.c     |  476 ++++++++++++++++++++++++++++++++++++
 drivers/w1/w1_family.h             |    1 +
 5 files changed, 527 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/w1/slaves/w1_ds28e04
 create mode 100644 drivers/w1/slaves/w1_ds28e04.c

diff --git a/Documentation/w1/slaves/w1_ds28e04 b/Documentation/w1/slaves/w1_ds28e04
new file mode 100644
index 0000000..7d5993c
--- /dev/null
+++ b/Documentation/w1/slaves/w1_ds28e04
@@ -0,0 +1,36 @@
+Kernel driver w1_ds28e04
+========================
+
+Supported chips:
+  * Maxim DS28E04-100 4096-Bit Addressable 1-Wire EEPROM with PIO
+
+supported family codes:
+	W1_FAMILY_DS28E04	0x1C
+
+Author: Markus Franke, <franke.m@sebakmt.com> <franm@hrz.tu-chemnitz.de>
+
+Description
+-----------
+
+Support is provided through the sysfs files "eeprom" and "pio". CRC checking
+during memory accesses can optionally be enabled/disabled via the device
+attribute "crccheck". The strong pull-up can optionally be enabled/disabled
+via the module parameter "w1_strong_pullup".
+
+Memory Access
+
+	A read operation on the "eeprom" file reads the given amount of bytes
+	from the EEPROM of the DS28E04.
+
+	A write operation on the "eeprom" file writes the given byte sequence
+	to the EEPROM of the DS28E04. If CRC checking mode is enabled only
+	fully alligned blocks of 32 bytes with valid CRC16 values (in bytes 30
+	and 31) are allowed to be written.
+
+PIO Access
+
+	The 2 PIOs of the DS28E04-100 are accessible via the "pio" sysfs file.
+
+	The current status of the PIO's is returned as an 8 bit value. Bit 0/1
+	represent the state of PIO_0/PIO_1. Bits 2..7 do not care. The PIO's are
+	driven low-active, i.e. the driver delivers/expects low-active values.
diff --git a/drivers/w1/slaves/Kconfig b/drivers/w1/slaves/Kconfig
index eb9e376..6752669 100644
--- a/drivers/w1/slaves/Kconfig
+++ b/drivers/w1/slaves/Kconfig
@@ -94,6 +94,19 @@ config W1_SLAVE_DS2781
 
 	  If you are unsure, say N.
 
+config W1_SLAVE_DS28E04
+	tristate "4096-Bit Addressable 1-Wire EEPROM with PIO (DS28E04-100)"
+	depends on W1
+	select CRC16
+	help
+	  If you enable this you will have the DS28E04-100
+	  chip support.
+
+	  Say Y here if you want to use a 1-wire
+	  4kb EEPROM with PIO family device (DS28E04).
+
+	  If you are unsure, say N.
+
 config W1_SLAVE_BQ27000
 	tristate "BQ27000 slave support"
 	depends on W1
diff --git a/drivers/w1/slaves/Makefile b/drivers/w1/slaves/Makefile
index c4f1859..05188f6 100644
--- a/drivers/w1/slaves/Makefile
+++ b/drivers/w1/slaves/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_W1_SLAVE_DS2760)	+= w1_ds2760.o
 obj-$(CONFIG_W1_SLAVE_DS2780)	+= w1_ds2780.o
 obj-$(CONFIG_W1_SLAVE_DS2781)	+= w1_ds2781.o
 obj-$(CONFIG_W1_SLAVE_BQ27000)	+= w1_bq27000.o
+obj-$(CONFIG_W1_SLAVE_DS28E04)	+= w1_ds28e04.o
diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
new file mode 100644
index 0000000..56bad73
--- /dev/null
+++ b/drivers/w1/slaves/w1_ds28e04.c
@@ -0,0 +1,476 @@
+/*
+ *	w1_ds28e04.c - w1 family 1C (DS28E04) driver
+ *
+ * Copyright (c) 2012 Markus Franke <franke.m@sebakmt.com>
+ *
+ * This source code is licensed under the GNU General Public License,
+ * Version 2. See the file COPYING for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/crc16.h>
+#include <linux/uaccess.h>
+
+#define CRC16_INIT		0
+#define CRC16_VALID		0xb001
+
+#include "../w1.h"
+#include "../w1_int.h"
+#include "../w1_family.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Markus Franke <franke.m@sebakmt.com>, <franm@hrz.tu-chemnitz.de>");
+MODULE_DESCRIPTION("w1 family 1C driver for DS28E04, 4kb EEPROM and PIO");
+
+/* Allow the strong pullup to be disabled, but default to enabled.
+ * If it was disabled a parasite powered device might not get the required
+ * current to copy the data from the scratchpad to EEPROM.  If it is enabled
+ * parasite powered devices have a better chance of getting the current
+ * required.
+ */
+static int w1_strong_pullup = 1;
+module_param_named(strong_pullup, w1_strong_pullup, int, 0);
+
+/* enable/disable CRC checking on DS28E04-100 memory accesses */
+static char w1_enable_crccheck = 1;
+
+#define W1_EEPROM_SIZE		512
+#define W1_PAGE_COUNT		16
+#define W1_PAGE_SIZE		32
+#define W1_PAGE_BITS		5
+#define W1_PAGE_MASK		0x1F
+
+#define W1_F1C_READ_EEPROM	0xF0
+#define W1_F1C_WRITE_SCRATCH	0x0F
+#define W1_F1C_READ_SCRATCH	0xAA
+#define W1_F1C_COPY_SCRATCH	0x55
+#define W1_F1C_ACCESS_WRITE	0x5A
+
+#define W1_1C_REG_LOGIC_STATE	0x220
+
+struct w1_f1C_data {
+	u8	memory[W1_EEPROM_SIZE];
+	u32	validcrc;
+};
+
+/**
+ * Check the file size bounds and adjusts count as needed.
+ * This would not be needed if the file size didn't reset to 0 after a write.
+ */
+static inline size_t w1_f1C_fix_count(loff_t off, size_t count, size_t size)
+{
+	if (off > size)
+		return 0;
+
+	if ((off + count) > size)
+		return size - off;
+
+	return count;
+}
+
+static int w1_f1C_refresh_block(struct w1_slave *sl, struct w1_f1C_data *data,
+				int block)
+{
+	u8	wrbuf[3];
+	int	off = block * W1_PAGE_SIZE;
+
+	if (data->validcrc & (1 << block))
+		return 0;
+
+	if (w1_reset_select_slave(sl)) {
+		data->validcrc = 0;
+		return -EIO;
+	}
+
+	wrbuf[0] = W1_F1C_READ_EEPROM;
+	wrbuf[1] = off & 0xff;
+	wrbuf[2] = off >> 8;
+	w1_write_block(sl->master, wrbuf, 3);
+	w1_read_block(sl->master, &data->memory[off], W1_PAGE_SIZE);
+
+	/* cache the block if the CRC is valid */
+	if (crc16(CRC16_INIT, &data->memory[off], W1_PAGE_SIZE) == CRC16_VALID)
+		data->validcrc |= (1 << block);
+
+	return 0;
+}
+
+static int w1_f1C_read(struct w1_slave *sl, int addr, int len, char *data)
+{
+	u8 wrbuf[3];
+
+	/* read directly from the EEPROM */
+	if (w1_reset_select_slave(sl))
+		return -EIO;
+
+	wrbuf[0] = W1_F1C_READ_EEPROM;
+	wrbuf[1] = addr & 0xff;
+	wrbuf[2] = addr >> 8;
+
+	w1_write_block(sl->master, wrbuf, sizeof(wrbuf));
+	return w1_read_block(sl->master, data, len);
+}
+
+static ssize_t w1_f1C_read_bin(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	struct w1_f1C_data *data = sl->family_data;
+	int i, min_page, max_page;
+
+	count = w1_f1C_fix_count(off, count, W1_EEPROM_SIZE);
+	if (count == 0)
+		return 0;
+
+	mutex_lock(&sl->master->mutex);
+
+	if (w1_enable_crccheck) {
+		min_page = (off >> W1_PAGE_BITS);
+		max_page = (off + count - 1) >> W1_PAGE_BITS;
+		for (i = min_page; i <= max_page; i++) {
+			if (w1_f1C_refresh_block(sl, data, i)) {
+				count = -EIO;
+				goto out_up;
+			}
+		}
+		memcpy(buf, &data->memory[off], count);
+	} else {
+		count = w1_f1C_read(sl, off, count, buf);
+	}
+
+out_up:
+	mutex_unlock(&sl->master->mutex);
+
+	return count;
+}
+
+/**
+ * Writes to the scratchpad and reads it back for verification.
+ * Then copies the scratchpad to EEPROM.
+ * The data must be on one page.
+ * The master must be locked.
+ *
+ * @param sl	The slave structure
+ * @param addr	Address for the write
+ * @param len   length must be <= (W1_PAGE_SIZE - (addr & W1_PAGE_MASK))
+ * @param data	The data to write
+ * @return	0=Success -1=failure
+ */
+static int w1_f1C_write(struct w1_slave *sl, int addr, int len, const u8 *data)
+{
+	u8 wrbuf[4];
+	u8 rdbuf[W1_PAGE_SIZE + 3];
+	u8 es = (addr + len - 1) & 0x1f;
+	unsigned int tm = 10;
+	int i;
+	struct w1_f1C_data *f1C = sl->family_data;
+
+	/* Write the data to the scratchpad */
+	if (w1_reset_select_slave(sl))
+		return -1;
+
+	wrbuf[0] = W1_F1C_WRITE_SCRATCH;
+	wrbuf[1] = addr & 0xff;
+	wrbuf[2] = addr >> 8;
+
+	w1_write_block(sl->master, wrbuf, 3);
+	w1_write_block(sl->master, data, len);
+
+	/* Read the scratchpad and verify */
+	if (w1_reset_select_slave(sl))
+		return -1;
+
+	w1_write_8(sl->master, W1_F1C_READ_SCRATCH);
+	w1_read_block(sl->master, rdbuf, len + 3);
+
+	/* Compare what was read against the data written */
+	if ((rdbuf[0] != wrbuf[1]) || (rdbuf[1] != wrbuf[2]) ||
+	    (rdbuf[2] != es) || (memcmp(data, &rdbuf[3], len) != 0))
+		return -1;
+
+	/* Copy the scratchpad to EEPROM */
+	if (w1_reset_select_slave(sl))
+		return -1;
+
+	wrbuf[0] = W1_F1C_COPY_SCRATCH;
+	wrbuf[3] = es;
+
+	for (i = 0; i < sizeof(wrbuf); ++i) {
+		/* issue 10ms strong pullup (or delay) on the last byte
+		   for writing the data from the scratchpad to EEPROM */
+		if (w1_strong_pullup && i == sizeof(wrbuf)-1)
+			w1_next_pullup(sl->master, tm);
+
+		w1_write_8(sl->master, wrbuf[i]);
+	}
+
+	if (!w1_strong_pullup)
+		msleep(tm);
+
+	if (w1_enable_crccheck) {
+		/* invalidate cached data */
+		f1C->validcrc &= ~(1 << (addr >> W1_PAGE_BITS));
+	}
+
+	/* Reset the bus to wake up the EEPROM (this may not be needed) */
+	w1_reset_bus(sl->master);
+
+	return 0;
+}
+
+static ssize_t w1_f1C_write_bin(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	int addr, len, idx;
+
+	count = w1_f1C_fix_count(off, count, W1_EEPROM_SIZE);
+	if (count == 0)
+		return 0;
+
+	if (w1_enable_crccheck) {
+		/* can only write full blocks in cached mode */
+		if ((off & W1_PAGE_MASK) || (count & W1_PAGE_MASK)) {
+			dev_err(&sl->dev, "invalid offset/count off=%d cnt=%zd\n",
+				(int)off, count);
+			return -EINVAL;
+		}
+
+		/* make sure the block CRCs are valid */
+		for (idx = 0; idx < count; idx += W1_PAGE_SIZE) {
+			if (crc16(CRC16_INIT, &buf[idx], W1_PAGE_SIZE)
+				!= CRC16_VALID) {
+				dev_err(&sl->dev, "bad CRC at offset %d\n",
+					(int)off);
+				return -EINVAL;
+			}
+		}
+	}
+
+	mutex_lock(&sl->master->mutex);
+
+	/* Can only write data to one page at a time */
+	idx = 0;
+	while (idx < count) {
+		addr = off + idx;
+		len = W1_PAGE_SIZE - (addr & W1_PAGE_MASK);
+		if (len > (count - idx))
+			len = count - idx;
+
+		if (w1_f1C_write(sl, addr, len, &buf[idx]) < 0) {
+			count = -EIO;
+			goto out_up;
+		}
+		idx += len;
+	}
+
+out_up:
+	mutex_unlock(&sl->master->mutex);
+
+	return count;
+}
+
+static ssize_t w1_f1C_read_pio(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	int ret;
+
+	/* check arguments */
+	if (off != 0 || count != 1 || buf == NULL)
+		return -EINVAL;
+
+	mutex_lock(&sl->master->mutex);
+	ret = w1_f1C_read(sl, W1_1C_REG_LOGIC_STATE, count, buf);
+	mutex_unlock(&sl->master->mutex);
+
+	return ret;
+}
+
+static ssize_t w1_f1C_write_pio(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *bin_attr,
+				char *buf, loff_t off, size_t count)
+
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	u8 wrbuf[3];
+	u8 ack;
+
+	/* check arguments */
+	if (off != 0 || count != 1 || buf == NULL)
+		return -EINVAL;
+
+	mutex_lock(&sl->master->mutex);
+
+	/* Write the PIO data */
+	if (w1_reset_select_slave(sl)) {
+		mutex_unlock(&sl->master->mutex);
+		return -1;
+	}
+
+	/* set bit 7..2 to value '1' */
+	*buf = *buf | 0xFC;
+
+	wrbuf[0] = W1_F1C_ACCESS_WRITE;
+	wrbuf[1] = *buf;
+	wrbuf[2] = ~(*buf);
+	w1_write_block(sl->master, wrbuf, 3);
+
+	w1_read_block(sl->master, &ack, sizeof(ack));
+
+	mutex_unlock(&sl->master->mutex);
+
+	/* check for acknowledgement */
+	if (ack != 0xAA)
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t w1_f1C_show_crccheck(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	if (put_user(w1_enable_crccheck + 0x30, buf))
+		return -EFAULT;
+
+	return sizeof(w1_enable_crccheck);
+}
+
+static ssize_t w1_f1C_store_crccheck(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	char val;
+
+	if (count != 1 || !buf)
+		return -EINVAL;
+
+	if (get_user(val, buf))
+		return -EFAULT;
+
+	/* convert to decimal */
+	val = val - 0x30;
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	/* set the new value */
+	w1_enable_crccheck = val;
+
+	return sizeof(w1_enable_crccheck);
+}
+
+#define NB_SYSFS_BIN_FILES 2
+static struct bin_attribute w1_f1C_bin_attr[NB_SYSFS_BIN_FILES] = {
+	{
+		.attr = {
+			.name = "eeprom",
+			.mode = S_IRUGO | S_IWUSR,
+		},
+		.size = W1_EEPROM_SIZE,
+		.read = w1_f1C_read_bin,
+		.write = w1_f1C_write_bin,
+	},
+	{
+		.attr = {
+			.name = "pio",
+			.mode = S_IRUGO | S_IWUSR,
+		},
+		.size = 1,
+		.read = w1_f1C_read_pio,
+		.write = w1_f1C_write_pio,
+	}
+};
+
+static DEVICE_ATTR(crccheck, S_IWUSR | S_IRUGO,
+		   w1_f1C_show_crccheck, w1_f1C_store_crccheck);
+
+static int w1_f1C_add_slave(struct w1_slave *sl)
+{
+	int err = 0;
+	int i;
+	struct w1_f1C_data *data = NULL;
+
+	if (w1_enable_crccheck) {
+		data = kzalloc(sizeof(struct w1_f1C_data), GFP_KERNEL);
+		if (!data)
+			return -ENOMEM;
+		sl->family_data = data;
+	}
+
+	/* create binary sysfs attributes */
+	for (i = 0; i < NB_SYSFS_BIN_FILES && !err; ++i)
+		err = sysfs_create_bin_file(
+			&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
+
+	if (err)
+		goto out;
+
+	/* create device attributes */
+	err = device_create_file(&sl->dev, &dev_attr_crccheck);
+
+	if (err) {
+		/* remove binary sysfs attributes */
+		for (i = 0; i < NB_SYSFS_BIN_FILES; ++i)
+			sysfs_remove_bin_file(
+				&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
+	}
+
+out:
+	if (err) {
+		if (w1_enable_crccheck)
+			kfree(data);
+	}
+
+	return err;
+}
+
+static void w1_f1C_remove_slave(struct w1_slave *sl)
+{
+	int i;
+
+	if (w1_enable_crccheck) {
+		kfree(sl->family_data);
+		sl->family_data = NULL;
+	}
+
+	/* remove device attributes */
+	device_remove_file(&sl->dev, &dev_attr_crccheck);
+
+	/* remove binary sysfs attributes */
+	for (i = 0; i < NB_SYSFS_BIN_FILES; ++i)
+		sysfs_remove_bin_file(&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
+}
+
+static struct w1_family_ops w1_f1C_fops = {
+	.add_slave      = w1_f1C_add_slave,
+	.remove_slave   = w1_f1C_remove_slave,
+};
+
+static struct w1_family w1_family_1C = {
+	.fid = W1_FAMILY_DS28E04,
+	.fops = &w1_f1C_fops,
+};
+
+static int __init w1_f1C_init(void)
+{
+	return w1_register_family(&w1_family_1C);
+}
+
+static void __exit w1_f1C_fini(void)
+{
+	w1_unregister_family(&w1_family_1C);
+}
+
+module_init(w1_f1C_init);
+module_exit(w1_f1C_fini);
diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h
index 874aeb0..b00ada4 100644
--- a/drivers/w1/w1_family.h
+++ b/drivers/w1/w1_family.h
@@ -30,6 +30,7 @@
 #define W1_FAMILY_SMEM_01	0x01
 #define W1_FAMILY_SMEM_81	0x81
 #define W1_THERM_DS18S20 	0x10
+#define W1_FAMILY_DS28E04	0x1C
 #define W1_COUNTER_DS2423	0x1D
 #define W1_THERM_DS1822  	0x22
 #define W1_EEPROM_DS2433  	0x23
-- 
1.7.5.4

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Greg KH]

On Fri, May 11, 2012 at 12:57:58AM +0200, Markus Franke wrote:
> Dear Greg,
> 
> Am 10.05.2012 17:16, schrieb Greg KH:
> 
> > I don't remember anything anymore, you will have to send the patch for
> > me to be able to properly review it and determine this.
> 
> 
> here comes the updated patch.
> 
> >From 3ef34f86788bfe39fa5bdb07b7e0598d3fda1f4f Mon Sep 17 00:00:00 2001
> From: Markus Franke <franm@hrz.tu-chemnitz.de>
> Date: Fri, 11 May 2012 00:41:35 +0200
> Subject: [PATCH] Add 1-wire slave device driver for DS28E04-100
> 
> 
> Signed-off-by: Markus Franke <franm@hrz.tu-chemnitz.de>
> ---
>  Documentation/w1/slaves/w1_ds28e04 |   36 +++
>  drivers/w1/slaves/Kconfig          |   13 +
>  drivers/w1/slaves/Makefile         |    1 +
>  drivers/w1/slaves/w1_ds28e04.c     |  476 ++++++++++++++++++++++++++++++++++++
>  drivers/w1/w1_family.h             |    1 +
>  5 files changed, 527 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/w1/slaves/w1_ds28e04
>  create mode 100644 drivers/w1/slaves/w1_ds28e04.c

You forgot a Documentation/ABI/ update that shows your sysfs files and
explains them there.

Care to retry?

greg k-h

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

> You forgot a Documentation/ABI/ update that shows your sysfs files and

> explains them there.


There is not even a single 1-wire driver mentioned within
Documentation/ABI. In which subdirectory shall I put my documentation
file into? stable?

@Evgeniy: Any suggestions?

Best regards,
Markus Franke

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Evgeniy Polyakov]

On Fri, May 11, 2012 at 08:13:45AM +0200, Markus Franke (markus.franke@s2002.tu-chemnitz.de) wrote:
> There is not even a single 1-wire driver mentioned within
> Documentation/ABI. In which subdirectory shall I put my documentation
> file into? stable?

I believe it is ok to add w1 dir there and describe all your ssyfs files
Feel free to document others :)

-- 
	Evgeniy Polyakov

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Greg,

Am 11.05.2012 01:04, schrieb Greg KH:

> You forgot a Documentation/ABI/ update that shows your sysfs files and
> explains them there.


here comes the updated patch including the review comments of Andrew.

>From d2afaf266d09f4c93d38b47ebeb5a5d28e230500 Mon Sep 17 00:00:00 2001
From: Markus Franke <franm@hrz.tu-chemnitz.de>
Date: Sat, 26 May 2012 00:36:58 +0200
Subject: [PATCH 2/2] Add 1-wire slave device driver for DS28E04-100

Signed-off-by: Markus Franke <franm@hrz.tu-chemnitz.de>
---
 Documentation/ABI/stable/sysfs-driver-w1_ds28e04 |   15 +
 Documentation/w1/slaves/w1_ds28e04               |   36 ++
 drivers/w1/slaves/Kconfig                        |   13 +
 drivers/w1/slaves/Makefile                       |    1 +
 drivers/w1/slaves/w1_ds28e04.c                   |  469 ++++++++++++++++++++++
 drivers/w1/w1_family.h                           |    1 +
 6 files changed, 535 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/stable/sysfs-driver-w1_ds28e04
 create mode 100644 Documentation/w1/slaves/w1_ds28e04
 create mode 100644 drivers/w1/slaves/w1_ds28e04.c

diff --git a/Documentation/ABI/stable/sysfs-driver-w1_ds28e04 b/Documentation/ABI/stable/sysfs-driver-w1_ds28e04
new file mode 100644
index 0000000..26579ee
--- /dev/null
+++ b/Documentation/ABI/stable/sysfs-driver-w1_ds28e04
@@ -0,0 +1,15 @@
+What:		/sys/bus/w1/devices/.../pio
+Date:		May 2012
+Contact:	Markus Franke <franm@hrz.tu-chemnitz.de>
+Description:	read/write the contents of the two PIO's of the DS28E04-100
+		see Documentation/w1/slaves/w1_ds28e04 for detailed information
+Users:		any user space application which wants to communicate with DS28E04-100
+
+
+
+What:		/sys/bus/w1/devices/.../eeprom
+Date:		May 2012
+Contact:	Markus Franke <franm@hrz.tu-chemnitz.de>
+Description:	read/write the contents of the EEPROM memory of the DS28E04-100
+		see Documentation/w1/slaves/w1_ds28e04 for detailed information
+Users:		any user space application which wants to communicate with DS28E04-100
diff --git a/Documentation/w1/slaves/w1_ds28e04 b/Documentation/w1/slaves/w1_ds28e04
new file mode 100644
index 0000000..7d5993c
--- /dev/null
+++ b/Documentation/w1/slaves/w1_ds28e04
@@ -0,0 +1,36 @@
+Kernel driver w1_ds28e04
+========================
+
+Supported chips:
+  * Maxim DS28E04-100 4096-Bit Addressable 1-Wire EEPROM with PIO
+
+supported family codes:
+	W1_FAMILY_DS28E04	0x1C
+
+Author: Markus Franke, <franke.m@sebakmt.com> <franm@hrz.tu-chemnitz.de>
+
+Description
+-----------
+
+Support is provided through the sysfs files "eeprom" and "pio". CRC checking
+during memory accesses can optionally be enabled/disabled via the device
+attribute "crccheck". The strong pull-up can optionally be enabled/disabled
+via the module parameter "w1_strong_pullup".
+
+Memory Access
+
+	A read operation on the "eeprom" file reads the given amount of bytes
+	from the EEPROM of the DS28E04.
+
+	A write operation on the "eeprom" file writes the given byte sequence
+	to the EEPROM of the DS28E04. If CRC checking mode is enabled only
+	fully alligned blocks of 32 bytes with valid CRC16 values (in bytes 30
+	and 31) are allowed to be written.
+
+PIO Access
+
+	The 2 PIOs of the DS28E04-100 are accessible via the "pio" sysfs file.
+
+	The current status of the PIO's is returned as an 8 bit value. Bit 0/1
+	represent the state of PIO_0/PIO_1. Bits 2..7 do not care. The PIO's are
+	driven low-active, i.e. the driver delivers/expects low-active values.
diff --git a/drivers/w1/slaves/Kconfig b/drivers/w1/slaves/Kconfig
index eb9e376..6752669 100644
--- a/drivers/w1/slaves/Kconfig
+++ b/drivers/w1/slaves/Kconfig
@@ -94,6 +94,19 @@ config W1_SLAVE_DS2781
 
 	  If you are unsure, say N.
 
+config W1_SLAVE_DS28E04
+	tristate "4096-Bit Addressable 1-Wire EEPROM with PIO (DS28E04-100)"
+	depends on W1
+	select CRC16
+	help
+	  If you enable this you will have the DS28E04-100
+	  chip support.
+
+	  Say Y here if you want to use a 1-wire
+	  4kb EEPROM with PIO family device (DS28E04).
+
+	  If you are unsure, say N.
+
 config W1_SLAVE_BQ27000
 	tristate "BQ27000 slave support"
 	depends on W1
diff --git a/drivers/w1/slaves/Makefile b/drivers/w1/slaves/Makefile
index c4f1859..05188f6 100644
--- a/drivers/w1/slaves/Makefile
+++ b/drivers/w1/slaves/Makefile
@@ -12,3 +12,4 @@ obj-$(CONFIG_W1_SLAVE_DS2760)	+= w1_ds2760.o
 obj-$(CONFIG_W1_SLAVE_DS2780)	+= w1_ds2780.o
 obj-$(CONFIG_W1_SLAVE_DS2781)	+= w1_ds2781.o
 obj-$(CONFIG_W1_SLAVE_BQ27000)	+= w1_bq27000.o
+obj-$(CONFIG_W1_SLAVE_DS28E04)	+= w1_ds28e04.o
diff --git a/drivers/w1/slaves/w1_ds28e04.c b/drivers/w1/slaves/w1_ds28e04.c
new file mode 100644
index 0000000..69c1583
--- /dev/null
+++ b/drivers/w1/slaves/w1_ds28e04.c
@@ -0,0 +1,469 @@
+/*
+ *	w1_ds28e04.c - w1 family 1C (DS28E04) driver
+ *
+ * Copyright (c) 2012 Markus Franke <franke.m@sebakmt.com>
+ *
+ * This source code is licensed under the GNU General Public License,
+ * Version 2. See the file COPYING for more details.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/moduleparam.h>
+#include <linux/device.h>
+#include <linux/types.h>
+#include <linux/delay.h>
+#include <linux/slab.h>
+#include <linux/crc16.h>
+#include <linux/uaccess.h>
+
+#define CRC16_INIT		0
+#define CRC16_VALID		0xb001
+
+#include "../w1.h"
+#include "../w1_int.h"
+#include "../w1_family.h"
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Markus Franke <franke.m@sebakmt.com>, <franm@hrz.tu-chemnitz.de>");
+MODULE_DESCRIPTION("w1 family 1C driver for DS28E04, 4kb EEPROM and PIO");
+
+/* Allow the strong pullup to be disabled, but default to enabled.
+ * If it was disabled a parasite powered device might not get the required
+ * current to copy the data from the scratchpad to EEPROM.  If it is enabled
+ * parasite powered devices have a better chance of getting the current
+ * required.
+ */
+static int w1_strong_pullup = 1;
+module_param_named(strong_pullup, w1_strong_pullup, int, 0);
+
+/* enable/disable CRC checking on DS28E04-100 memory accesses */
+static char w1_enable_crccheck = 1;
+
+#define W1_EEPROM_SIZE		512
+#define W1_PAGE_COUNT		16
+#define W1_PAGE_SIZE		32
+#define W1_PAGE_BITS		5
+#define W1_PAGE_MASK		0x1F
+
+#define W1_F1C_READ_EEPROM	0xF0
+#define W1_F1C_WRITE_SCRATCH	0x0F
+#define W1_F1C_READ_SCRATCH	0xAA
+#define W1_F1C_COPY_SCRATCH	0x55
+#define W1_F1C_ACCESS_WRITE	0x5A
+
+#define W1_1C_REG_LOGIC_STATE	0x220
+
+struct w1_f1C_data {
+	u8	memory[W1_EEPROM_SIZE];
+	u32	validcrc;
+};
+
+/**
+ * Check the file size bounds and adjusts count as needed.
+ * This would not be needed if the file size didn't reset to 0 after a write.
+ */
+static inline size_t w1_f1C_fix_count(loff_t off, size_t count, size_t size)
+{
+	if (off > size)
+		return 0;
+
+	if ((off + count) > size)
+		return size - off;
+
+	return count;
+}
+
+static int w1_f1C_refresh_block(struct w1_slave *sl, struct w1_f1C_data *data,
+				int block)
+{
+	u8	wrbuf[3];
+	int	off = block * W1_PAGE_SIZE;
+
+	if (data->validcrc & (1 << block))
+		return 0;
+
+	if (w1_reset_select_slave(sl)) {
+		data->validcrc = 0;
+		return -EIO;
+	}
+
+	wrbuf[0] = W1_F1C_READ_EEPROM;
+	wrbuf[1] = off & 0xff;
+	wrbuf[2] = off >> 8;
+	w1_write_block(sl->master, wrbuf, 3);
+	w1_read_block(sl->master, &data->memory[off], W1_PAGE_SIZE);
+
+	/* cache the block if the CRC is valid */
+	if (crc16(CRC16_INIT, &data->memory[off], W1_PAGE_SIZE) == CRC16_VALID)
+		data->validcrc |= (1 << block);
+
+	return 0;
+}
+
+static int w1_f1C_read(struct w1_slave *sl, int addr, int len, char *data)
+{
+	u8 wrbuf[3];
+
+	/* read directly from the EEPROM */
+	if (w1_reset_select_slave(sl))
+		return -EIO;
+
+	wrbuf[0] = W1_F1C_READ_EEPROM;
+	wrbuf[1] = addr & 0xff;
+	wrbuf[2] = addr >> 8;
+
+	w1_write_block(sl->master, wrbuf, sizeof(wrbuf));
+	return w1_read_block(sl->master, data, len);
+}
+
+static ssize_t w1_f1C_read_bin(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	struct w1_f1C_data *data = sl->family_data;
+	int i, min_page, max_page;
+
+	count = w1_f1C_fix_count(off, count, W1_EEPROM_SIZE);
+	if (count == 0)
+		return 0;
+
+	mutex_lock(&sl->master->mutex);
+
+	if (w1_enable_crccheck) {
+		min_page = (off >> W1_PAGE_BITS);
+		max_page = (off + count - 1) >> W1_PAGE_BITS;
+		for (i = min_page; i <= max_page; i++) {
+			if (w1_f1C_refresh_block(sl, data, i)) {
+				count = -EIO;
+				goto out_up;
+			}
+		}
+		memcpy(buf, &data->memory[off], count);
+	} else {
+		count = w1_f1C_read(sl, off, count, buf);
+	}
+
+out_up:
+	mutex_unlock(&sl->master->mutex);
+
+	return count;
+}
+
+/**
+ * Writes to the scratchpad and reads it back for verification.
+ * Then copies the scratchpad to EEPROM.
+ * The data must be on one page.
+ * The master must be locked.
+ *
+ * @param sl	The slave structure
+ * @param addr	Address for the write
+ * @param len   length must be <= (W1_PAGE_SIZE - (addr & W1_PAGE_MASK))
+ * @param data	The data to write
+ * @return	0=Success -1=failure
+ */
+static int w1_f1C_write(struct w1_slave *sl, int addr, int len, const u8 *data)
+{
+	u8 wrbuf[4];
+	u8 rdbuf[W1_PAGE_SIZE + 3];
+	u8 es = (addr + len - 1) & 0x1f;
+	unsigned int tm = 10;
+	int i;
+	struct w1_f1C_data *f1C = sl->family_data;
+
+	/* Write the data to the scratchpad */
+	if (w1_reset_select_slave(sl))
+		return -1;
+
+	wrbuf[0] = W1_F1C_WRITE_SCRATCH;
+	wrbuf[1] = addr & 0xff;
+	wrbuf[2] = addr >> 8;
+
+	w1_write_block(sl->master, wrbuf, 3);
+	w1_write_block(sl->master, data, len);
+
+	/* Read the scratchpad and verify */
+	if (w1_reset_select_slave(sl))
+		return -1;
+
+	w1_write_8(sl->master, W1_F1C_READ_SCRATCH);
+	w1_read_block(sl->master, rdbuf, len + 3);
+
+	/* Compare what was read against the data written */
+	if ((rdbuf[0] != wrbuf[1]) || (rdbuf[1] != wrbuf[2]) ||
+	    (rdbuf[2] != es) || (memcmp(data, &rdbuf[3], len) != 0))
+		return -1;
+
+	/* Copy the scratchpad to EEPROM */
+	if (w1_reset_select_slave(sl))
+		return -1;
+
+	wrbuf[0] = W1_F1C_COPY_SCRATCH;
+	wrbuf[3] = es;
+
+	for (i = 0; i < sizeof(wrbuf); ++i) {
+		/* issue 10ms strong pullup (or delay) on the last byte
+		   for writing the data from the scratchpad to EEPROM */
+		if (w1_strong_pullup && i == sizeof(wrbuf)-1)
+			w1_next_pullup(sl->master, tm);
+
+		w1_write_8(sl->master, wrbuf[i]);
+	}
+
+	if (!w1_strong_pullup)
+		msleep(tm);
+
+	if (w1_enable_crccheck) {
+		/* invalidate cached data */
+		f1C->validcrc &= ~(1 << (addr >> W1_PAGE_BITS));
+	}
+
+	/* Reset the bus to wake up the EEPROM (this may not be needed) */
+	w1_reset_bus(sl->master);
+
+	return 0;
+}
+
+static ssize_t w1_f1C_write_bin(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	int addr, len, idx;
+
+	count = w1_f1C_fix_count(off, count, W1_EEPROM_SIZE);
+	if (count == 0)
+		return 0;
+
+	if (w1_enable_crccheck) {
+		/* can only write full blocks in cached mode */
+		if ((off & W1_PAGE_MASK) || (count & W1_PAGE_MASK)) {
+			dev_err(&sl->dev, "invalid offset/count off=%d cnt=%zd\n",
+				(int)off, count);
+			return -EINVAL;
+		}
+
+		/* make sure the block CRCs are valid */
+		for (idx = 0; idx < count; idx += W1_PAGE_SIZE) {
+			if (crc16(CRC16_INIT, &buf[idx], W1_PAGE_SIZE)
+				!= CRC16_VALID) {
+				dev_err(&sl->dev, "bad CRC at offset %d\n",
+					(int)off);
+				return -EINVAL;
+			}
+		}
+	}
+
+	mutex_lock(&sl->master->mutex);
+
+	/* Can only write data to one page at a time */
+	idx = 0;
+	while (idx < count) {
+		addr = off + idx;
+		len = W1_PAGE_SIZE - (addr & W1_PAGE_MASK);
+		if (len > (count - idx))
+			len = count - idx;
+
+		if (w1_f1C_write(sl, addr, len, &buf[idx]) < 0) {
+			count = -EIO;
+			goto out_up;
+		}
+		idx += len;
+	}
+
+out_up:
+	mutex_unlock(&sl->master->mutex);
+
+	return count;
+}
+
+static ssize_t w1_f1C_read_pio(struct file *filp, struct kobject *kobj,
+			       struct bin_attribute *bin_attr,
+			       char *buf, loff_t off, size_t count)
+
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	int ret;
+
+	/* check arguments */
+	if (off != 0 || count != 1 || buf == NULL)
+		return -EINVAL;
+
+	mutex_lock(&sl->master->mutex);
+	ret = w1_f1C_read(sl, W1_1C_REG_LOGIC_STATE, count, buf);
+	mutex_unlock(&sl->master->mutex);
+
+	return ret;
+}
+
+static ssize_t w1_f1C_write_pio(struct file *filp, struct kobject *kobj,
+				struct bin_attribute *bin_attr,
+				char *buf, loff_t off, size_t count)
+
+{
+	struct w1_slave *sl = kobj_to_w1_slave(kobj);
+	u8 wrbuf[3];
+	u8 ack;
+
+	/* check arguments */
+	if (off != 0 || count != 1 || buf == NULL)
+		return -EINVAL;
+
+	mutex_lock(&sl->master->mutex);
+
+	/* Write the PIO data */
+	if (w1_reset_select_slave(sl)) {
+		mutex_unlock(&sl->master->mutex);
+		return -1;
+	}
+
+	/* set bit 7..2 to value '1' */
+	*buf = *buf | 0xFC;
+
+	wrbuf[0] = W1_F1C_ACCESS_WRITE;
+	wrbuf[1] = *buf;
+	wrbuf[2] = ~(*buf);
+	w1_write_block(sl->master, wrbuf, 3);
+
+	w1_read_block(sl->master, &ack, sizeof(ack));
+
+	mutex_unlock(&sl->master->mutex);
+
+	/* check for acknowledgement */
+	if (ack != 0xAA)
+		return -EIO;
+
+	return count;
+}
+
+static ssize_t w1_f1C_show_crccheck(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	if (put_user(w1_enable_crccheck + 0x30, buf))
+		return -EFAULT;
+
+	return sizeof(w1_enable_crccheck);
+}
+
+static ssize_t w1_f1C_store_crccheck(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	char val;
+
+	if (count != 1 || !buf)
+		return -EINVAL;
+
+	if (get_user(val, buf))
+		return -EFAULT;
+
+	/* convert to decimal */
+	val = val - 0x30;
+	if (val != 0 && val != 1)
+		return -EINVAL;
+
+	/* set the new value */
+	w1_enable_crccheck = val;
+
+	return sizeof(w1_enable_crccheck);
+}
+
+#define NB_SYSFS_BIN_FILES 2
+static struct bin_attribute w1_f1C_bin_attr[NB_SYSFS_BIN_FILES] = {
+	{
+		.attr = {
+			.name = "eeprom",
+			.mode = S_IRUGO | S_IWUSR,
+		},
+		.size = W1_EEPROM_SIZE,
+		.read = w1_f1C_read_bin,
+		.write = w1_f1C_write_bin,
+	},
+	{
+		.attr = {
+			.name = "pio",
+			.mode = S_IRUGO | S_IWUSR,
+		},
+		.size = 1,
+		.read = w1_f1C_read_pio,
+		.write = w1_f1C_write_pio,
+	}
+};
+
+static DEVICE_ATTR(crccheck, S_IWUSR | S_IRUGO,
+		   w1_f1C_show_crccheck, w1_f1C_store_crccheck);
+
+static int w1_f1C_add_slave(struct w1_slave *sl)
+{
+	int err = 0;
+	int i;
+	struct w1_f1C_data *data = NULL;
+
+	if (w1_enable_crccheck) {
+		data = kzalloc(sizeof(struct w1_f1C_data), GFP_KERNEL);
+		if (!data)
+			return -ENOMEM;
+		sl->family_data = data;
+	}
+
+	/* create binary sysfs attributes */
+	for (i = 0; i < NB_SYSFS_BIN_FILES && !err; ++i)
+		err = sysfs_create_bin_file(
+			&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
+
+	if (!err) {
+		/* create device attributes */
+		err = device_create_file(&sl->dev, &dev_attr_crccheck);
+	}
+
+	if (err) {
+		/* remove binary sysfs attributes */
+		for (i = 0; i < NB_SYSFS_BIN_FILES; ++i)
+			sysfs_remove_bin_file(
+				&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
+
+		kfree(data);
+	}
+
+	return err;
+}
+
+static void w1_f1C_remove_slave(struct w1_slave *sl)
+{
+	int i;
+
+	kfree(sl->family_data);
+	sl->family_data = NULL;
+
+	/* remove device attributes */
+	device_remove_file(&sl->dev, &dev_attr_crccheck);
+
+	/* remove binary sysfs attributes */
+	for (i = 0; i < NB_SYSFS_BIN_FILES; ++i)
+		sysfs_remove_bin_file(&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
+}
+
+static struct w1_family_ops w1_f1C_fops = {
+	.add_slave      = w1_f1C_add_slave,
+	.remove_slave   = w1_f1C_remove_slave,
+};
+
+static struct w1_family w1_family_1C = {
+	.fid = W1_FAMILY_DS28E04,
+	.fops = &w1_f1C_fops,
+};
+
+static int __init w1_f1C_init(void)
+{
+	return w1_register_family(&w1_family_1C);
+}
+
+static void __exit w1_f1C_fini(void)
+{
+	w1_unregister_family(&w1_family_1C);
+}
+
+module_init(w1_f1C_init);
+module_exit(w1_f1C_fini);
diff --git a/drivers/w1/w1_family.h b/drivers/w1/w1_family.h
index 874aeb0..b00ada4 100644
--- a/drivers/w1/w1_family.h
+++ b/drivers/w1/w1_family.h
@@ -30,6 +30,7 @@
 #define W1_FAMILY_SMEM_01	0x01
 #define W1_FAMILY_SMEM_81	0x81
 #define W1_THERM_DS18S20 	0x10
+#define W1_FAMILY_DS28E04	0x1C
 #define W1_COUNTER_DS2423	0x1D
 #define W1_THERM_DS1822  	0x22
 #define W1_EEPROM_DS2433  	0x23
-- 
1.7.5.4

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Andrew Morton]

On Fri, 11 May 2012 00:57:58 +0200
Markus Franke <markus.franke@s2002.tu-chemnitz.de> wrote:

> +static int w1_f1C_add_slave(struct w1_slave *sl)
> +{
> +	int err = 0;
> +	int i;
> +	struct w1_f1C_data *data = NULL;
> +
> +	if (w1_enable_crccheck) {
> +		data = kzalloc(sizeof(struct w1_f1C_data), GFP_KERNEL);
> +		if (!data)
> +			return -ENOMEM;
> +		sl->family_data = data;
> +	}
> +
> +	/* create binary sysfs attributes */
> +	for (i = 0; i < NB_SYSFS_BIN_FILES && !err; ++i)
> +		err = sysfs_create_bin_file(
> +			&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
> +
> +	if (err)
> +		goto out;

If this goto is taken, we will leak 0..i sysfs files.

> +	/* create device attributes */
> +	err = device_create_file(&sl->dev, &dev_attr_crccheck);
> +
> +	if (err) {
> +		/* remove binary sysfs attributes */
> +		for (i = 0; i < NB_SYSFS_BIN_FILES; ++i)
> +			sysfs_remove_bin_file(
> +				&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
> +	}
> +
> +out:
> +	if (err) {
> +		if (w1_enable_crccheck)
> +			kfree(data);

kfree(NULL) is legal - the w1_enable_crccheck test can be removed.

> +	}
> +
> +	return err;
> +}

Re: [PATCH] w1: Add 1-wire slave device driver for DS28E04-100

[Markus Franke]

Dear Andrew,

thanks for the feedback.

Am 11.05.2012 01:22, schrieb Andrew Morton:

> On Fri, 11 May 2012 00:57:58 +0200
> Markus Franke <markus.franke@s2002.tu-chemnitz.de> wrote:
> 
>> +static int w1_f1C_add_slave(struct w1_slave *sl)
>> +{
>> +	int err = 0;
>> +	int i;
>> +	struct w1_f1C_data *data = NULL;
>> +
>> +	if (w1_enable_crccheck) {
>> +		data = kzalloc(sizeof(struct w1_f1C_data), GFP_KERNEL);
>> +		if (!data)
>> +			return -ENOMEM;
>> +		sl->family_data = data;
>> +	}
>> +
>> +	/* create binary sysfs attributes */
>> +	for (i = 0; i < NB_SYSFS_BIN_FILES && !err; ++i)
>> +		err = sysfs_create_bin_file(
>> +			&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
>> +
>> +	if (err)
>> +		goto out;
> 
> If this goto is taken, we will leak 0..i sysfs files.


That's indeed the case. :-(

>> +	/* create device attributes */
>> +	err = device_create_file(&sl->dev, &dev_attr_crccheck);
>> +
>> +	if (err) {
>> +		/* remove binary sysfs attributes */
>> +		for (i = 0; i < NB_SYSFS_BIN_FILES; ++i)
>> +			sysfs_remove_bin_file(
>> +				&sl->dev.kobj, &(w1_f1C_bin_attr[i]));
>> +	}
>> +
>> +out:
>> +	if (err) {
>> +		if (w1_enable_crccheck)
>> +			kfree(data);
> 
> kfree(NULL) is legal - the w1_enable_crccheck test can be removed.


Ok, I'll fix this for the next (hopefully final) version of the patch.

Best regards,
Markus Franke