w1: sysfs, select-slave, ds2433

w1-slave-sysfs.diff
Every slave now has at least two sysfs files:
- id : an 8 byte binary file that holds the 64 bit id
example: [23 6c dc 1c 00 00 00 43]
- name : the name, example: [23-0000001cdc6c]
Family ops is now two callback functions: add_slave() and remove_slave(). The family driver is responsible for creating any other sysfs entries. This patch updates w1-smem (which is pretty much empty) and w1-therm.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: w1-slave-sysfs.diff
Type: text/x-diff
Size: 9934 bytes
Desc: not available
Url : http://lists.lm-sensors.org/pipermail/lm-sensors/attachments/20050630/2e97d7a4/w1-slave-sysfs-0001.bin

Hi Ben,

I see no reason why this should be a binary file. Please make it a text file. Binary files in sysfs are supposed to be an exception, not the rule.

Thanks,

Jean Delvare

Jean,

Is there a preferred format?

"23 6c dc 1c 00 00 00 43"
or
"236cdc1c00000043"
or
"0x23 0x6c 0xdc 0x1c 0x00 0x00 0x00 0x43"
Or, since it can be thought of as a 64 bit number
"0x236cdc1c00000043"

I prefer the last one.

Thanks,
Ben

-----Original Message-----
From: Jean Delvare [mailto:[email protected]]
Sent: Thursday, June 30, 2005 4:09 PM
To: Gardner, Ben
Cc: Evgeniy Polyakov; LM Sensors
Subject: Re: [lm-sensors] [Patch 4/5] w1: sysfs, select-slave, ds2433

Hi Ben,

I see no reason why this should be a binary file. Please make it a text file. Binary files in sysfs are supposed to be an exception, not the rule.

Thanks,

Jean Delvare

Hi Ben,

Last one is fine with me, although #2 would be fine too. I don't think it really makes a difference.

Have you searched for similar use cases in the kernel tree? Not sure where I would start looking for...

Thanks,

Jean Delvare

I have some comments about this patch.

w1.c | 101
+++++++++++++++++++++++++++++++++---------------------------

 w1.h        |    3 -
 w1_family.c |    3 -
 w1_family.h |    6 ++-
 w1_smem.c   |   47 ---------------------------
 w1_therm.c  |   32 +++++++++++++------
 6 files changed, 83 insertions(+), 109 deletions(-)

Index: linux-2.6.12-mm1/drivers/w1/w1.h =================================--- linux-2.6.12-mm1.orig/drivers/w1/w1.h +++ linux-2.6.12-mm1/drivers/w1/w1.h
@@ -77,9 +77,6 @@ struct w1_slave

        struct w1_family        *family;
        struct device           dev;
        struct completion       dev_released;

-
- struct bin_attribute attr_bin;
- struct device_attribute attr_name;
};

typedef void (* w1_slave_found_callback)(unsigned long, u64);
Index: linux-2.6.12-mm1/drivers/w1/w1.c =================================--- linux-2.6.12-mm1.orig/drivers/w1/w1.c +++ linux-2.6.12-mm1/drivers/w1/w1.c
@@ -59,19 +59,6 @@ static pid_t control_thread;
static int control_needs_exit;
static DECLARE_COMPLETION(w1_control_complete);

-/* stuff for the default family */
-static ssize_t w1_famdefault_read_name(struct device *dev, struct device_attribute *attr, char *buf)
-{
- struct w1_slave *sl = container_of(dev, struct w1_slave, dev);
- return(sprintf(buf, "%s\n", sl->name));
-}
-static struct w1_family_ops w1_default_fops = {
- .rname = &w1_famdefault_read_name,
-};
-static struct w1_family w1_default_family = {
- .fops = &w1_default_fops,
-};
-
static int w1_master_match(struct device *dev, struct device_driver
*drv)

 {
        return 1;
@@ -99,30 +86,45 @@ static void w1_slave_release(struct devi
        complete(&sl->dev_released);

}

-static ssize_t w1_default_read_name(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t w1_slave_read_name(struct device *dev,
+ struct device_attribute *attr, char
*buf)
{

-       return sprintf(buf, "No family registered.\n");
+       struct w1_slave *sl = dev_to_w1_slave(dev);
+       return sprintf(buf, "%s\n", sl->name);

}

-static ssize_t w1_default_read_bin(struct kobject *kobj, char *buf, loff_t off,
+static ssize_t w1_slave_read_id(struct kobject *kobj, char *buf, loff_t off,
size_t count)
{

-       return sprintf(buf, "No family registered.\n");
+       struct w1_slave *sl = kobj_to_w1_slave(kobj);
+       atomic_inc(&sl->refcnt);
+       if (off > 8) {

+ count = 0;
+ } else {
+ if (off + count > 8)
+ count = 8 - off;
+
+ memcpy(buf, (u8 *)&sl->reg_num, count);

+       }
+       atomic_dec(&sl->refcnt);
+       return count;

}

You write here binary data, is it what you really want or it should be snprintf("%Lx")?

-static struct device_attribute w1_slave_attribute - __ATTR(name, S_IRUGO, w1_default_read_name, NULL);
+static struct device_attribute w1_slave_attr_name + __ATTR(name, S_IRUGO, w1_slave_read_name, NULL);

-static struct bin_attribute w1_slave_bin_attribute = {

+static struct bin_attribute w1_slave_attr_bin_id = {
        .attr = {

- .name = "w1_slave",
+ .name = "id",
.mode = S_IRUGO,
.owner = THIS_MODULE,
},

-       .size = W1_SLAVE_DATA_SIZE,
-       .read = &w1_default_read_bin,
+       .size = 8,
+       .read = w1_slave_read_id,

};

+/* Empty family */
+static struct w1_family w1_default_family;

 static struct bus_type w1_bus_type = {
        .name = "w1",
@@ -356,36 +358,44 @@ static int __w1_attach_slave_device(stru
                return err;
        }

- memcpy(&sl->attr_bin, &w1_slave_bin_attribute,
sizeof(sl->attr_bin));
- memcpy(&sl->attr_name, &w1_slave_attribute,
sizeof(sl->attr_name));
-

-       sl->attr_bin.read = sl->family->fops->rbin;
-       sl->attr_name.show = sl->family->fops->rname;
+       /* Create "name" entry */
+       err = device_create_file(&sl->dev, &w1_slave_attr_name);
+       if (err < 0) {

+ dev_err(&sl->dev,
+ "sysfs file creation for [%s] failed. err=%d\n",
+ sl->dev.bus_id, err);
+ goto out_unreg;
+ }

-       err = device_create_file(&sl->dev, &sl->attr_name);
+       /* Create "id" entry */
+       err = sysfs_create_bin_file(&sl->dev.kobj,
&w1_slave_attr_bin_id);
        if (err < 0) {
                dev_err(&sl->dev,
                        "sysfs file creation for [%s] failed. err=%d\n",
                        sl->dev.bus_id, err);
-               device_unregister(&sl->dev);
-               return err;
+               goto out_rem1;
        }

- if ( sl->attr_bin.read ) {
- err = sysfs_create_bin_file(&sl->dev.kobj,
&sl->attr_bin);
- if (err < 0) {
- dev_err(&sl->dev,
- "sysfs file creation for [%s] failed.
err=%d\n",
- sl->dev.bus_id, err);

-                       device_remove_file(&sl->dev, &sl->attr_name);
-                       device_unregister(&sl->dev);
-                       return err;

- }
+ /* if the family driver needs to initialize something... */
+ if (sl->family->fops && sl->family->fops->add_slave &&
+ ((err = sl->family->fops->add_slave(sl)) < 0)) {
+ dev_err(&sl->dev,
+ "sysfs file creation for [%s] failed. err=%d\n",
+ sl->dev.bus_id, err);

+               goto out_rem2;
        }
 
        list_add_tail(&sl->w1_slave_entry, &sl->master->slist);
 
        return 0;

+
+out_rem2:
+ sysfs_remove_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+out_rem1:
+ device_remove_file(&sl->dev, &w1_slave_attr_name);
+out_unreg:
+ device_unregister(&sl->dev);
+ return err;
}

static int w1_attach_slave_device(struct w1_master *dev, struct
w1_reg_num *rn)
@@ -460,10 +470,11 @@ static void w1_slave_detach(struct w1_sl

                        flush_signals(current);
        }

- if ( sl->attr_bin.read ) {
- sysfs_remove_bin_file (&sl->dev.kobj, &sl->attr_bin);

-       }
-       device_remove_file(&sl->dev, &sl->attr_name);
+       if (sl->family->fops && sl->family->fops->remove_slave)
+               sl->family->fops->remove_slave(sl);
+
+       sysfs_remove_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+       device_remove_file(&sl->dev, &w1_slave_attr_name);
        device_unregister(&sl->dev);
        w1_family_put(sl->family);

Index: linux-2.6.12-mm1/drivers/w1/w1_family.h =================================--- linux-2.6.12-mm1.orig/drivers/w1/w1_family.h +++ linux-2.6.12-mm1/drivers/w1/w1_family.h
@@ -35,10 +35,12 @@

#define MAXNAMELEN 32

+struct w1_slave;
+
struct w1_family_ops
{
- ssize_t (* rname)(struct device *, struct device_attribute *,
char *);

-       ssize_t (* rbin)(struct kobject *, char *, loff_t, size_t);
+       int  (* add_slave)(struct w1_slave *);
+       void (* remove_slave)(struct w1_slave *);

};

struct w1_family
Index: linux-2.6.12-mm1/drivers/w1/w1_smem.c =================================--- linux-2.6.12-mm1.orig/drivers/w1/w1_smem.c +++ linux-2.6.12-mm1/drivers/w1/w1_smem.c
@@ -36,59 +36,12 @@ MODULE_LICENSE("GPL");
MODULE_AUTHOR("Evgeniy Polyakov <[email protected]>"); MODULE_DESCRIPTION("Driver for 1-wire Dallas network protocol, 64bit
memory family.");

-static ssize_t w1_smem_read_name(struct device *, struct device_attribute *attr, char *);
-static ssize_t w1_smem_read_bin(struct kobject *, char *, loff_t, size_t);
-
-static struct w1_family_ops w1_smem_fops = {
- .rname = &w1_smem_read_name,
- .rbin = &w1_smem_read_bin,
-};
-
-static ssize_t w1_smem_read_name(struct device *dev, struct device_attribute *attr, char *buf)
-{
- struct w1_slave *sl = dev_to_w1_slave(dev);
- return sprintf(buf, "%s\n", sl->name);
-}
-
-static ssize_t w1_smem_read_bin(struct kobject *kobj, char *buf, loff_t off, size_t count)
-{
- struct w1_slave *sl = kobj_to_w1_slave(kobj);
- int i;
-
- atomic_inc(&sl->refcnt);
- if (down_interruptible(&sl->master->mutex)) {
- count = 0;
- goto out_dec;
- }
-
- if (off > W1_SLAVE_DATA_SIZE) {
- count = 0;
- goto out;
- }
- if (off + count > W1_SLAVE_DATA_SIZE) {
- count = 0;
- goto out;
- }
- for (i = 0; i < 8; ++i)
- count += sprintf(buf + count, "%02x ", ((u8
*)&sl->reg_num)[i]);
- count += sprintf(buf + count, "\n");
-
-out:
- up(&sl->master->mutex);
-out_dec:
- atomic_dec(&sl->refcnt);
-
- return count;
-}
-

 static struct w1_family w1_smem_family_01 = {
        .fid = W1_FAMILY_SMEM_01,

- .fops = &w1_smem_fops,
};

 static struct w1_family w1_smem_family_81 = {
        .fid = W1_FAMILY_SMEM_81,

- .fops = &w1_smem_fops,
};

static int __init w1_smem_init(void)
Index: linux-2.6.12-mm1/drivers/w1/w1_therm.c =================================--- linux-2.6.12-mm1.orig/drivers/w1/w1_therm.c +++ linux-2.6.12-mm1/drivers/w1/w1_therm.c

@@ -42,12 +42,31 @@ static u8 bad_roms[][9] = {
                                {}
                        };

-static ssize_t w1_therm_read_name(struct device *, struct device_attribute *attr, char *);
static ssize_t w1_therm_read_bin(struct kobject *, char *, loff_t,
size_t);

+static struct bin_attribute w1_therm_bin_attr = {
+ .attr = {

+               .name = "w1_slave",
+               .mode = S_IRUGO,
+               .owner = THIS_MODULE,
+       },
+       .size = W1_SLAVE_DATA_SIZE,
+       .read = w1_therm_read_bin,

+};
+
+static int w1_therm_add_slave(struct w1_slave *sl) +{
+ return sysfs_create_bin_file(&sl->dev.kobj, &w1_therm_bin_attr);
+}
+
+static void w1_therm_remove_slave(struct w1_slave *sl) +{
+ sysfs_remove_bin_file(&sl->dev.kobj, &w1_therm_bin_attr);
+}

I would like to create this files automatically by w1 core and only let slave drivers to create it's "private" files themselves.

static struct w1_family_ops w1_therm_fops = {

-       .rname = &w1_therm_read_name,
-       .rbin = &w1_therm_read_bin,
+       .add_slave      = w1_therm_add_slave,
+       .remove_slave   = w1_therm_remove_slave,

};

static struct w1_family w1_therm_family_DS18S20 = {
@@ -59,6 +78,7 @@ static struct w1_family w1_therm_family_

        .fid = W1_THERM_DS18B20,
        .fops = &w1_therm_fops,
 };
+
 static struct w1_family w1_therm_family_DS1822 = {
        .fid = W1_THERM_DS1822,
        .fops = &w1_therm_fops,
@@ -90,12 +110,6 @@ static struct w1_therm_family_converter 
        },

};

-static ssize_t w1_therm_read_name(struct device *dev, struct device_attribute *attr, char *buf)
-{
- struct w1_slave *sl = dev_to_w1_slave(dev);
- return sprintf(buf, "%s\n", sl->name);
-}
-
static inline int w1_DS18B20_convert_temp(u8 rom[9])

 {
        int t = (rom[1] << 8) | rom[0];

Index: linux-2.6.12-mm1/drivers/w1/w1_family.c =================================--- linux-2.6.12-mm1.orig/drivers/w1/w1_family.c +++ linux-2.6.12-mm1/drivers/w1/w1_family.c
@@ -31,9 +31,6 @@ extern void w1_reconnect_slaves(struct w

static int w1_check_family(struct w1_family *f)
{
- if (!f->fops->rname || !f->fops->rbin)
- return -EINVAL;
-

        return 0;

}

Evgeniy,

Thanks for the feedback.
I will change the id to be a hex number, ie "236cdc1c00000043\n".

(Qoute about the thermal attribute 'w1_slave')

I'm curious as to why.
I've looked at types of families out there
(http://owfs.sourceforge.net/family.html).
As far as I can tell, only a few of them are thermal class. Do we really want a generic name 'w1_slave' that means something different for each family?

I removed 'w1_slave' from the core because w1_therm is the only family that uses it.

IMO, the core should only create entries for attributes that are applicable to all families.
The only attribute that is present for all families is the 64-bit id.

Thanks,
Ben

Please pad it with zeroes at the begining, i.e. to allow %016Lx reading.

Main idea behind w1_slave was t provide the most valuable information from any family in this file. I.e. thermal family will put there a temeperature, simple ID will put there it's number, voltage monitor will store current voltage there and so on, so this file was always created by w1 core with provided in initialistion time family callback.