w1: Add 1-wire slave device driver for DS28E04-100

This patch adds a 1-wire slave device driver for the DS28E04-100.

Signed-off-by Markus Franke <[email protected]>

Acked-by Evgeniy Polyakov <[email protected]>

Quoted 4 lines

This patch adds a 1-wire slave device driver for the DS28E04-100.

Signed-off-by Markus Franke<[email protected]>

Acked-by Evgeniy Polyakov<[email protected]>

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.

Dear Greg,

Am 30.04.2012 04:13, schrieb Greg KH:

Quoted 3 lines · Markus Franke

Quoted 2 lines

This patch adds a 1-wire slave device driver for the DS28E04-100.

Quoted 4 lines

Quoted 2 lines

Signed-off-by Markus Franke<[email protected]>

Acked-by Evgeniy Polyakov<[email protected]>

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.

Quoted 4 lines

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

Quoted 2 lines

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?

Quoted 4 lines

Quoted 2 lines

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.

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?

Quoted 10 lines

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

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.

Quoted 2 lines

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.

Quoted 4 lines · Markus Franke

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.

Quoted 5 lines · Greg KH

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.

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

Dear Evgeniy,

Am 10.05.2012 02:43, schrieb Evgeniy Polyakov:

Quoted 2 lines

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.

From 3ef34f86788bfe39fa5bdb07b7e0598d3fda1f4f Mon Sep 17 00:00:00 2001

Dear Greg,

Am 10.05.2012 17:16, schrieb Greg KH:

Quoted 2 lines

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.

Quoted 1 line

From 3ef34f86788bfe39fa5bdb07b7e0598d3fda1f4f Mon Sep 17 00:00:00 2001

From: Markus Franke <[email protected]>
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 <[email protected]>

---
 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.

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

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

From d2afaf266d09f4c93d38b47ebeb5a5d28e230500 Mon Sep 17 00:00:00 2001

drivers/w1/slaves/w1_ds28e04.c new file +10 -0

+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;

drivers/w1/slaves/w1_ds28e04.c new file +4 -0

+	/* create device attributes */
+	err = device_create_file(&sl->dev, &dev_attr_crccheck);
+
+	if (err) {

drivers/w1/slaves/w1_ds28e04.c new file +4 -0

+	}
+
+	return err;
+}

Quoted 14 lines · On Fri, 11 May 2012 00:57:58 +0200 Markus Franke…

drivers/w1/slaves/w1_ds28e04.c new file +14 -0

+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 */

drivers/w1/slaves/w1_ds28e04.c line 417

Quoted 6 lines

drivers/w1/slaves/w1_ds28e04.c new file +6 -0

+	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.

Quoted 8 lines

drivers/w1/slaves/w1_ds28e04.c new file +8 -0

+	/* 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]));

drivers/w1/slaves/w1_ds28e04.c line 432

Quoted 6 lines

drivers/w1/slaves/w1_ds28e04.c new file +6 -0

+	}
+
+out:
+	if (err) {
+		if (w1_enable_crccheck)
+			kfree(data);

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