* [PATCH 7/33 replaces 7/35] W1: feature, enable hardware strong pullup
2008-04-13 22:56 ` David Fries
@ 2008-04-13 23:14 ` David Fries
2008-04-13 23:14 ` [PATCH 8/33 replaces 8/35 and 9/35] W1: feature, w1_therm.c use strong pullup and documentation David Fries
` (6 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-13 23:14 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 10544 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
Add a strong pullup option to the w1 system. This supplies extra
power for parasite powered devices.
The one wire bus requires at a minimum one wire and ground. The
common wire is used for sending and receiving data as well as
supplying power to devices that are parasite powered. Temperature
sensors are one device that can be parasite powered and require no bus
transactions while the temperature conversion is progress. A bus
transaction would pull the common wire down. In addition to the
normal pullup resistor, which allows a device to pull the bus line
down to send data, some devices support a strong pullup mechanism to
deliver more power. The documentation lists this as useful for larger
networks and when temperature sensors are exposed to higher
temperatures they can exceed the current supplied by a simple pullup
resistor. This patch adds a mechanism to support a strong pullup, and
fall back to sleeping with the line high otherwise. This patch
doesn't enable the strong pullup for any master or slave device, those
will follow as they require this patch.
The hardware USB 2490 one wire bus master has a bit on a few commands
which will enable the strong pullup as soon as the command finishes
executing. That requires the ds2490 driver to be notified if it
should be set before the command is executed. That's why I added the
w1_next_pullup. That will call the master set_pullup just before the
next write, and reset the value to zero after it has completed. If
the master driver doesn't support the set_pullup, it will delay the
appropriate amount of time after the write returns.
w1.h 1.2 1.3 1.4 1.10
Add set_pullup to w1_bus_master, and add pullup_duration to w1_master
to hold the strong pullup duration for the next write operation. Add
enable_pullup to w1_master for a per master enable/disable option.
w1.c 1.20
Add w1_master_pullup sysfs entry.
w1_int.c 1.3 1.9
Add a check to disable set_pullup and print a warning message when the
operation isn't supported. Add enable_pullup module parameter.
w1_io.c 1.2 1.3 1.4 1.6
Add w1_next_pullup, call w1_pre_write and w1_post_write to call
set_pullup before and after, or just sleep after if set_pullup isn't
supported.
Signed-off-by: David Fries <david@fries.net>
---
drivers/w1/w1.c | 30 ++++++++++++++++++++++
drivers/w1/w1.h | 12 +++++++++
drivers/w1/w1_int.c | 16 ++++++++++++
drivers/w1/w1_io.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++++---
4 files changed, 122 insertions(+), 4 deletions(-)
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 5d8c2c7..401c56a 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -270,6 +270,34 @@ static ssize_t w1_master_attribute_show_search(struct device *dev,
return count;
}
+static ssize_t w1_master_attribute_store_pullup(struct device * dev,
+ struct device_attribute *attr,
+ const char * buf, size_t count)
+{
+ struct w1_master *md = dev_to_w1_master(dev);
+
+ mutex_lock(&md->mutex);
+ md->enable_pullup = simple_strtol(buf, NULL, 0);
+ mutex_unlock(&md->mutex);
+ wake_up_process(md->thread);
+
+ return count;
+}
+
+static ssize_t w1_master_attribute_show_pullup(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct w1_master *md = dev_to_w1_master(dev);
+ ssize_t count;
+
+ mutex_lock(&md->mutex);
+ count = sprintf(buf, "%d\n", md->enable_pullup);
+ mutex_unlock(&md->mutex);
+
+ return count;
+}
+
static ssize_t w1_master_attribute_show_pointer(struct device *dev, struct device_attribute *attr, char *buf)
{
struct w1_master *md = dev_to_w1_master(dev);
@@ -365,6 +393,7 @@ static W1_MASTER_ATTR_RO(attempts, S_IRUGO);
static W1_MASTER_ATTR_RO(timeout, S_IRUGO);
static W1_MASTER_ATTR_RO(pointer, S_IRUGO);
static W1_MASTER_ATTR_RW(search, S_IRUGO | S_IWUGO);
+static W1_MASTER_ATTR_RW(pullup, S_IRUGO | S_IWUGO);
static struct attribute *w1_master_default_attrs[] = {
&w1_master_attribute_name.attr,
@@ -375,6 +404,7 @@ static struct attribute *w1_master_default_attrs[] = {
&w1_master_attribute_timeout.attr,
&w1_master_attribute_pointer.attr,
&w1_master_attribute_search.attr,
+ &w1_master_attribute_pullup.attr,
NULL
};
diff --git a/drivers/w1/w1.h b/drivers/w1/w1.h
index c47ca1e..83d8208 100644
--- a/drivers/w1/w1.h
+++ b/drivers/w1/w1.h
@@ -142,6 +142,12 @@ struct w1_bus_master
*/
u8 (*reset_bus)(void *);
+ /**
+ * Put out a strong pull-up pulse of the specified duration.
+ * @return -1=Error, 0=completed
+ */
+ u8 (*set_pullup)(void *, int);
+
/** Really nice hardware can handles the different types of ROM search
* w1_master* is passed to the slave found callback.
*/
@@ -167,6 +173,11 @@ struct w1_master
void *priv;
int priv_size;
+ /** 5V strong pullup enabled flag, 1 enabled, zero disabled. */
+ int enable_pullup;
+ /** 5V strong pullup duration in milliseconds, zero disabled. */
+ int pullup_duration;
+
struct task_struct *thread;
struct mutex mutex;
@@ -201,6 +212,7 @@ u8 w1_calc_crc8(u8 *, int);
void w1_write_block(struct w1_master *, const u8 *, int);
u8 w1_read_block(struct w1_master *, u8 *, int);
int w1_reset_select_slave(struct w1_slave *sl);
+void w1_next_pullup(struct w1_master *, int);
static inline struct w1_slave* dev_to_w1_slave(struct device *dev)
{
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 3388d8f..8deb76c 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -31,6 +31,9 @@
static u32 w1_ids = 1;
+static int w1_enable_pullup = 1;
+module_param_named(enable_pullup, w1_enable_pullup, int, 0);
+
static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
struct device_driver *driver,
struct device *device)
@@ -59,6 +62,7 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
dev->initialized = 0;
dev->id = id;
dev->slave_ttl = slave_ttl;
+ dev->enable_pullup = w1_enable_pullup;
dev->search_count = -1; /* continual scan */
/* 1 for w1_process to decrement
@@ -107,6 +111,18 @@ int w1_add_master_device(struct w1_bus_master *master)
printk(KERN_ERR "w1_add_master_device: invalid function set\n");
return(-EINVAL);
}
+ /* While it would be electrically possible to make a device that
+ * generated a strong pullup in bit bang mode, only hardare that
+ * controls 1-wire time frames are even expected to support a strong
+ * pullup. w1_io.c would need to support calling set_pullup before
+ * the last write_bit operation of a w1_write_8 which it currently
+ * doesn't.
+ */
+ if(!master->write_byte && !master->touch_bit && master->set_pullup) {
+ printk(KERN_ERR "w1_add_master_device: set_pullup requires "
+ "write_byte or touch_bit, disabling\n");
+ master->set_pullup=NULL;
+ }
dev = w1_alloc_dev(w1_ids++, w1_max_slave_count, w1_max_slave_ttl, &w1_master_driver, &w1_master_device);
if (!dev)
diff --git a/drivers/w1/w1_io.c b/drivers/w1/w1_io.c
index 0056ef6..3cdac8b 100644
--- a/drivers/w1/w1_io.c
+++ b/drivers/w1/w1_io.c
@@ -93,6 +93,40 @@ static void w1_write_bit(struct w1_master *dev, int bit)
}
/**
+ * Pre-write operation, currently only supporting strong pullups.
+ * Program the hardware for a strong pullup, if one has been requested and
+ * the hardware supports it.
+ *
+ * @param dev the master device
+ */
+static void w1_pre_write(struct w1_master *dev)
+{
+ if(dev->pullup_duration &&
+ dev->enable_pullup && dev->bus_master->set_pullup) {
+ dev->bus_master->set_pullup(dev->bus_master->data,
+ dev->pullup_duration);
+ }
+}
+
+/**
+ * Post-write operation, currently only supporting strong pullups.
+ * If a strong pullup was requested, clear it if the hardware supports
+ * them, or execute the delay otherwise, in either case clear the request.
+ *
+ * @param dev the master device
+ */
+static void w1_post_write(struct w1_master *dev)
+{
+ if(dev->pullup_duration) {
+ if(dev->enable_pullup && dev->bus_master->set_pullup)
+ dev->bus_master->set_pullup(dev->bus_master->data, 0);
+ else
+ msleep(dev->pullup_duration);
+ dev->pullup_duration=0;
+ }
+}
+
+/**
* Writes 8 bits.
*
* @param dev the master device
@@ -102,11 +136,17 @@ void w1_write_8(struct w1_master *dev, u8 byte)
{
int i;
- if (dev->bus_master->write_byte)
+ if (dev->bus_master->write_byte) {
+ w1_pre_write(dev);
dev->bus_master->write_byte(dev->bus_master->data, byte);
+ }
else
- for (i = 0; i < 8; ++i)
+ for (i = 0; i < 8; ++i) {
+ if(i==7)
+ w1_pre_write(dev);
w1_touch_bit(dev, (byte >> i) & 0x1);
+ }
+ w1_post_write(dev);
}
EXPORT_SYMBOL_GPL(w1_write_8);
@@ -203,11 +243,14 @@ void w1_write_block(struct w1_master *dev, const u8 *buf, int len)
{
int i;
- if (dev->bus_master->write_block)
+ if (dev->bus_master->write_block) {
+ w1_pre_write(dev);
dev->bus_master->write_block(dev->bus_master->data, buf, len);
+ }
else
for (i = 0; i < len; ++i)
- w1_write_8(dev, buf[i]);
+ w1_write_8(dev, buf[i]); // calls w1_pre_write
+ w1_post_write(dev);
}
EXPORT_SYMBOL_GPL(w1_write_block);
@@ -306,3 +349,20 @@ int w1_reset_select_slave(struct w1_slave *sl)
return 0;
}
EXPORT_SYMBOL_GPL(w1_reset_select_slave);
+
+/**
+ * Put out a strong pull-up of the specified duration after the next write
+ * operation. Not all hardware supports strong pullups. Hardware that
+ * doesn't support strong pullups will sleep for the given time after the
+ * write operation without a strong pullup. This is a one shot request for
+ * the next write, specifying zero will clear a previous request.
+ * The w1 master lock must be held.
+ *
+ * @param delay time in milliseconds
+ * @return 0=success, anything else=error
+ */
+void w1_next_pullup(struct w1_master *dev, int delay)
+{
+ dev->pullup_duration=delay;
+}
+EXPORT_SYMBOL_GPL(w1_next_pullup);
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 8/33 replaces 8/35 and 9/35] W1: feature, w1_therm.c use strong pullup and documentation
2008-04-13 22:56 ` David Fries
2008-04-13 23:14 ` [PATCH 7/33 replaces 7/35] W1: feature, enable hardware strong pullup David Fries
@ 2008-04-13 23:14 ` David Fries
2008-04-13 23:14 ` [PATCH 9/33 replaces 10/35] W1: be able to manually add and remove slaves David Fries
` (5 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-13 23:14 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5173 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
w1_therm.c 1.3 1.4 1.10 1.11
Use the strong pullup not the sleep. Note that the pullup goes before
W1_CONVERT_TEMP not after.
+w1_next_pullup(dev, tm);
w1_write_8(dev, W1_CONVERT_TEMP);
-msleep(tm);
Also added a module parameter strong_pullup to disable the strong
pullup and sleep if requested.
New directory for temperature sensor documentation.
Documentation/w1/00-INDEX 1.2
Added new subdirectory slaves
Documentation/w1/slaves/00-INDEX 1.1 1.2
Added index and w1_therm entry.
Documentation/w1/slaves/w1_therm 1.1 1.2 1.3
Initial entry for the temperature sensor.
Signed-off-by: David Fries <david@fries.net>
---
Documentation/w1/00-INDEX | 2 +
Documentation/w1/slaves/00-INDEX | 4 +++
Documentation/w1/slaves/w1_therm | 41 ++++++++++++++++++++++++++++++++++++++
drivers/w1/slaves/w1_therm.c | 15 ++++++++++++-
4 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/Documentation/w1/00-INDEX b/Documentation/w1/00-INDEX
index 5270cf4..cb49802 100644
--- a/Documentation/w1/00-INDEX
+++ b/Documentation/w1/00-INDEX
@@ -1,5 +1,7 @@
00-INDEX
- This file
+slaves/
+ - Drivers that provide support for specific family codes.
masters/
- Individual chips providing 1-wire busses.
w1.generic
diff --git a/Documentation/w1/slaves/00-INDEX b/Documentation/w1/slaves/00-INDEX
new file mode 100644
index 0000000..f8101d6
--- /dev/null
+++ b/Documentation/w1/slaves/00-INDEX
@@ -0,0 +1,4 @@
+00-INDEX
+ - This file
+w1_therm
+ - The Maxim/Dallas Semiconductor ds18*20 temperature sensor.
diff --git a/Documentation/w1/slaves/w1_therm b/Documentation/w1/slaves/w1_therm
new file mode 100644
index 0000000..0403aaa
--- /dev/null
+++ b/Documentation/w1/slaves/w1_therm
@@ -0,0 +1,41 @@
+Kernel driver w1_therm
+====================
+
+Supported chips:
+ * Maxim ds18*20 based temperature sensors.
+
+Author: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
+
+
+Description
+-----------
+
+w1_therm provides basic temperature conversion for ds18*20 devices.
+supported family codes:
+W1_THERM_DS18S20 0x10
+W1_THERM_DS1822 0x22
+W1_THERM_DS18B20 0x28
+
+Support is provided through the sysfs w1_slave file. Each open and
+read sequence will initiate a temperature conversion then provide two
+lines of ASCII output. The first line contains the nine hex bytes
+read along with a calculated crc value and YES or NO if it matched.
+If the crc matched the returned values are retained. The second line
+displays the retained values along with a temperature in millidegrees
+Centigrade after t=.
+
+Parasite powered devices are limited to one slave performing a
+temperature conversion at a time. If none of the devices are parasite
+powered it would be possible to convert all the devices at the same
+time and then go back to read individual sensors. That isn't
+currently supported. The driver also doesn't support reduced
+precision (which would also reduce the conversion time).
+
+The module parameter strong_pullup can be set to 0 to disable the
+strong pullup or 1 to enable. If enabled the 5V strong pullup will be
+enabled when the conversion is taking place provided the master driver
+must support the strong pullup (or it falls back to a pullup
+resistor). The DS18b20 temperature sensor specification lists a
+maximum current draw of 1.5mA and that a 5k pullup resistor is not
+sufficient. The strong pullup is designed to provide the additional
+current required.
diff --git a/drivers/w1/slaves/w1_therm.c b/drivers/w1/slaves/w1_therm.c
index fb28aca..f659380 100644
--- a/drivers/w1/slaves/w1_therm.c
+++ b/drivers/w1/slaves/w1_therm.c
@@ -37,6 +37,14 @@ MODULE_LICENSE("GPL");
MODULE_AUTHOR("Evgeniy Polyakov <johnpol@2ka.mipt.ru>");
MODULE_DESCRIPTION("Driver for 1-wire Dallas network protocol, temperature family.");
+/* Allow the strong pullup to be disabled, but default to enabled.
+ * If it was disabled a parasite powered device might not get the require
+ * current to do a temperature conversion. 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);
+
static u8 bad_roms[][9] = {
{0xaa, 0x00, 0x4b, 0x46, 0xff, 0xff, 0x0c, 0x10, 0x87},
{}
@@ -192,9 +200,12 @@ static ssize_t w1_therm_read_bin(struct kobject *kobj,
int count = 0;
unsigned int tm = 750;
+ /* 750ms strong pullup (or delay) after the convert */
+ if(w1_strong_pullup)
+ w1_next_pullup(dev, tm);
w1_write_8(dev, W1_CONVERT_TEMP);
-
- msleep(tm);
+ if(!w1_strong_pullup)
+ msleep(tm);
if (!w1_reset_select_slave(sl)) {
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 9/33 replaces 10/35] W1: be able to manually add and remove slaves
2008-04-13 22:56 ` David Fries
2008-04-13 23:14 ` [PATCH 7/33 replaces 7/35] W1: feature, enable hardware strong pullup David Fries
2008-04-13 23:14 ` [PATCH 8/33 replaces 8/35 and 9/35] W1: feature, w1_therm.c use strong pullup and documentation David Fries
@ 2008-04-13 23:14 ` David Fries
2008-04-13 23:15 ` [PATCH 11/33 replaces 12/35] W1: new module parameter search_count David Fries
` (4 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-13 23:14 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6540 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
w1.c 1.11 1.12 1.14
I'm intending to be working with a static network and run without the
bus searching, but bus searching is currently the only way to add
slave devices. This allows any slave to be added or removed. There
is one file for adding and another for removing. If the files are
read a short comment including the expected serial number format is
returned.
Signed-off-by: David Fries <david@fries.net>
---
drivers/w1/w1.c | 140 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 137 insertions(+), 3 deletions(-)
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 401c56a..4ed661c 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -56,6 +56,9 @@ module_param_named(slave_ttl, w1_max_slave_ttl, int, 0);
DEFINE_MUTEX(w1_mlock);
LIST_HEAD(w1_masters);
+static int w1_attach_slave_device(struct w1_master *dev, struct w1_reg_num *rn);
+void w1_slave_detach(struct w1_slave *sl);
+
static int w1_master_match(struct device *dev, struct device_driver *drv)
{
return 1;
@@ -349,7 +352,8 @@ static ssize_t w1_master_attribute_show_slave_count(struct device *dev, struct d
return count;
}
-static ssize_t w1_master_attribute_show_slaves(struct device *dev, struct device_attribute *attr, char *buf)
+static ssize_t w1_master_attribute_show_slaves(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
struct w1_master *md = dev_to_w1_master(dev);
int c = PAGE_SIZE;
@@ -374,6 +378,134 @@ static ssize_t w1_master_attribute_show_slaves(struct device *dev, struct device
return PAGE_SIZE - c;
}
+static ssize_t w1_master_attribute_show_add(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int c = PAGE_SIZE;
+ c -= snprintf(buf+PAGE_SIZE - c, c,
+ "write device id xx-xxxxxxxxxxxx to add slave\n");
+ return PAGE_SIZE - c;
+}
+
+static int w1_atoreg_num(struct device * dev, const char *buf, size_t count,
+ struct w1_reg_num *rn)
+{
+ unsigned int family;
+ u64 id;
+ int i;
+ u64 rn64_le;
+ /* The CRC value isn't read from the user because the sysfs directory
+ * doesn't include it and most messages from the bus search don't
+ * print it either. It would be unreasonable for the user to then
+ * provide it.
+ */
+ const char *error_msg="bad slave string format, expecting "
+ "ff-dddddddddddd\n";
+
+ if(buf[2]!='-') {
+ dev_err(dev, "%s", error_msg);
+ return -EINVAL;
+ }
+ i=sscanf(buf, "%02x-%012llx", &family, &id);
+ if(i!=2) {
+ dev_err(dev, "%s", error_msg);
+ return -EINVAL;
+ }
+ rn->family=family;
+ rn->id=id;
+
+ rn64_le=cpu_to_le64(*(u64*)rn);
+ rn->crc=w1_calc_crc8((u8 *)&rn64_le, 7);
+
+ #if 0
+ dev_info(dev, "With CRC device is %02x.%012llx.%02x.\n",
+ rn->family, (unsigned long long)rn->id, rn->crc);
+ #endif
+
+ return 0;
+}
+
+/* Searches the slaves in the w1_master and returns a pointer or NULL.
+ * Note: must hold the mutex
+ */
+static struct w1_slave *w1_slave_search_device(struct w1_master *dev,
+ struct w1_reg_num *rn)
+{
+ struct w1_slave *sl;
+ list_for_each_entry(sl, &dev->slist, w1_slave_entry) {
+ if (sl->reg_num.family == rn->family &&
+ sl->reg_num.id == rn->id &&
+ sl->reg_num.crc == rn->crc) {
+ return sl;
+ }
+ }
+ return NULL;
+}
+
+static ssize_t w1_master_attribute_store_add(struct device * dev,
+ struct device_attribute *attr,
+ const char * buf, size_t count)
+{
+ struct w1_master *md = dev_to_w1_master(dev);
+ struct w1_reg_num rn;
+ struct w1_slave *sl;
+ ssize_t result=count;
+
+ if(w1_atoreg_num(dev, buf, count, &rn))
+ return -EINVAL;
+
+ mutex_lock(&md->mutex);
+ sl=w1_slave_search_device(md, &rn);
+ /* It would be nice to do a targeted search one the one-wire bus
+ * for the new device to see if it is out there or not. But the
+ * current search doesn't support that.
+ */
+ if(sl) {
+ dev_info(dev, "Device %s already exists\n", sl->name);
+ result=-EINVAL;
+ } else {
+ w1_attach_slave_device(md, &rn);
+ }
+ mutex_unlock(&md->mutex);
+
+ return result;
+}
+
+static ssize_t w1_master_attribute_show_remove(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int c = PAGE_SIZE;
+ c -= snprintf(buf+PAGE_SIZE - c, c,
+ "write device id xx-xxxxxxxxxxxx to remove slave\n");
+ return PAGE_SIZE - c;
+}
+
+static ssize_t w1_master_attribute_store_remove(struct device * dev,
+ struct device_attribute *attr,
+ const char * buf, size_t count)
+{
+ struct w1_master *md = dev_to_w1_master(dev);
+ struct w1_reg_num rn;
+ struct w1_slave *sl;
+ ssize_t result=count;
+
+ if(w1_atoreg_num(dev, buf, count, &rn))
+ return -EINVAL;
+
+ mutex_lock(&md->mutex);
+ sl=w1_slave_search_device(md, &rn);
+ if(sl) {
+ w1_slave_detach(sl);
+ } else {
+ dev_info(dev, "Device %02x-%012llx doesn't exists\n", rn.family,
+ (unsigned long long)rn.id);
+ result=-EINVAL;
+ }
+ mutex_unlock(&md->mutex);
+
+ return result;
+}
+
#define W1_MASTER_ATTR_RO(_name, _mode) \
struct device_attribute w1_master_attribute_##_name = \
__ATTR(w1_master_##_name, _mode, \
@@ -394,6 +526,8 @@ static W1_MASTER_ATTR_RO(timeout, S_IRUGO);
static W1_MASTER_ATTR_RO(pointer, S_IRUGO);
static W1_MASTER_ATTR_RW(search, S_IRUGO | S_IWUGO);
static W1_MASTER_ATTR_RW(pullup, S_IRUGO | S_IWUGO);
+static W1_MASTER_ATTR_RW(add, S_IRUGO | S_IWUGO);
+static W1_MASTER_ATTR_RW(remove, S_IRUGO | S_IWUGO);
static struct attribute *w1_master_default_attrs[] = {
&w1_master_attribute_name.attr,
@@ -405,6 +539,8 @@ static struct attribute *w1_master_default_attrs[] = {
&w1_master_attribute_pointer.attr,
&w1_master_attribute_search.attr,
&w1_master_attribute_pullup.attr,
+ &w1_master_attribute_add.attr,
+ &w1_master_attribute_remove.attr,
NULL
};
@@ -700,9 +836,7 @@ void w1_reconnect_slaves(struct w1_family *f, int attach)
static void w1_slave_found(struct w1_master *dev, u64 rn)
{
- int slave_count;
struct w1_slave *sl;
- struct list_head *ent;
struct w1_reg_num *tmp;
u64 rn_le = cpu_to_le64(rn);
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 11/33 replaces 12/35] W1: new module parameter search_count
2008-04-13 22:56 ` David Fries
` (2 preceding siblings ...)
2008-04-13 23:14 ` [PATCH 9/33 replaces 10/35] W1: be able to manually add and remove slaves David Fries
@ 2008-04-13 23:15 ` David Fries
2008-04-13 23:15 ` [PATCH 12/33 replaces 13/35] W1: Document add, remove, search_count, and pullup David Fries
` (3 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-13 23:15 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1539 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
w1_int.c 1.6 1.10
Added a new module parameter search_count which allows overriding the
default search count. -1 continual, 0 disabled, N that many times.
Signed-off-by: David Fries <david@fries.net>
---
drivers/w1/w1_int.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/w1/w1_int.c b/drivers/w1/w1_int.c
index 8deb76c..47e27f5 100644
--- a/drivers/w1/w1_int.c
+++ b/drivers/w1/w1_int.c
@@ -30,6 +30,8 @@
#include "w1_int.h"
static u32 w1_ids = 1;
+static int w1_search_count = -1; /* Default is continual scan */
+module_param_named(search_count, w1_search_count, int, 0);
static int w1_enable_pullup = 1;
module_param_named(enable_pullup, w1_enable_pullup, int, 0);
@@ -62,8 +64,8 @@ static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
dev->initialized = 0;
dev->id = id;
dev->slave_ttl = slave_ttl;
+ dev->search_count = w1_search_count;
dev->enable_pullup = w1_enable_pullup;
- dev->search_count = -1; /* continual scan */
/* 1 for w1_process to decrement
* 1 for __w1_remove_master_device to decrement
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 12/33 replaces 13/35] W1: Document add, remove, search_count, and pullup.
2008-04-13 22:56 ` David Fries
` (3 preceding siblings ...)
2008-04-13 23:15 ` [PATCH 11/33 replaces 12/35] W1: new module parameter search_count David Fries
@ 2008-04-13 23:15 ` David Fries
2008-04-13 23:15 ` [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug David Fries
` (2 subsequent siblings)
7 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-13 23:15 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2445 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
Documentation/w1/w1.generic 1.2 1.3
Document w1_master_add, w1_master_remove, search_count, and pullup.
Signed-off-by: David Fries <david@fries.net>
---
Documentation/w1/w1.generic | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/Documentation/w1/w1.generic b/Documentation/w1/w1.generic
index 4c6509d..e3333ee 100644
--- a/Documentation/w1/w1.generic
+++ b/Documentation/w1/w1.generic
@@ -79,10 +79,13 @@ w1 master sysfs interface
<xx-xxxxxxxxxxxxx> - a directory for a found device. The format is family-serial
bus - (standard) symlink to the w1 bus
driver - (standard) symlink to the w1 driver
+w1_master_add - Manually register a slave device
w1_master_attempts - the number of times a search was attempted
w1_master_max_slave_count
- the maximum slaves that may be attached to a master
w1_master_name - the name of the device (w1_bus_masterX)
+w1_master_pullup - 5V strong pullup 0 enabled, 1 disabled
+w1_master_remove - Manually remove a slave device
w1_master_search - the number of searches left to do, -1=continual (default)
w1_master_slave_count
- the number of slaves found
@@ -90,7 +93,13 @@ w1_master_slaves - the names of the slaves, one per line
w1_master_timeout - the delay in seconds between searches
If you have a w1 bus that never changes (you don't add or remove devices),
-you can set w1_master_search to a positive value to disable searches.
+you can set the module parameter search_count to a small positive number
+for an initially small number of bus searches. Alternatively it could be
+set to zero, then manually add the slave device serial numbers by
+w1_master_add device file. The w1_master_add and w1_master_remove files
+generally only make sense when searching is disabled, as a search will
+redetect manually removed devices that are present and timeout manually
+added devices that aren't on the bus.
w1 slave sysfs interface
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug
2008-04-13 22:56 ` David Fries
` (4 preceding siblings ...)
2008-04-13 23:15 ` [PATCH 12/33 replaces 13/35] W1: Document add, remove, search_count, and pullup David Fries
@ 2008-04-13 23:15 ` David Fries
2008-04-14 11:40 ` Evgeniy Polyakov
2008-04-13 23:15 ` [PATCH 32/33 replaces 34/35] Documentation/w1/masters/ds2490 update David Fries
2008-04-14 12:30 ` [PATCH 0/35] W1: w1 core deadlock and oops fixes, ds2490 updates Evgeniy Polyakov
7 siblings, 1 reply; 13+ messages in thread
From: David Fries @ 2008-04-13 23:15 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3501 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
This is more to point out the bug in w1_slave_read_id, the next patch
rewrites the routine.
w1.c 1.15
Reading at an offset other than zero, ie reading less than 8 bytes at
a time would result in reading the first bytes over and over until 8
bytes were returned. Added the offset to the buffer.
- memcpy(buf, (u8 *)&sl->reg_num, count);
+ memcpy(buf, (u8 *)&sl->reg_num+off, count);
But there is a better way...
w1.c 1.16
Switching w1_slave_read_id from being a bin_attribute to a
device_attribute. It only has to return 8 bytes per read and lets
kobject handle the buffering. That simplifies the logic in
w1_slave_read_id.
Signed-off-by: David Fries <david@fries.net>
---
drivers/w1/w1.c | 35 ++++++++++-------------------------
1 files changed, 10 insertions(+), 25 deletions(-)
diff --git a/drivers/w1/w1.c b/drivers/w1/w1.c
index 5053bc8..c008493 100644
--- a/drivers/w1/w1.c
+++ b/drivers/w1/w1.c
@@ -104,35 +104,20 @@ static ssize_t w1_slave_read_name(struct device *dev, struct device_attribute *a
return sprintf(buf, "%s\n", sl->name);
}
-static ssize_t w1_slave_read_id(struct kobject *kobj,
- struct bin_attribute *bin_attr,
- char *buf, loff_t off, size_t count)
+static ssize_t w1_slave_read_id(struct device *dev,
+ struct device_attribute *attr, char *buf)
{
- struct w1_slave *sl = kobj_to_w1_slave(kobj);
-
- if (off > 8) {
- count = 0;
- } else {
- if (off + count > 8)
- count = 8 - off;
-
- memcpy(buf, (u8 *)&sl->reg_num, count);
- }
+ struct w1_slave *sl = dev_to_w1_slave(dev);
+ ssize_t count=sizeof(sl->reg_num);
+ memcpy(buf, (u8 *)&sl->reg_num, count);
return count;
}
static struct device_attribute w1_slave_attr_name =
__ATTR(name, S_IRUGO, w1_slave_read_name, NULL);
-
-static struct bin_attribute w1_slave_attr_bin_id = {
- .attr = {
- .name = "id",
- .mode = S_IRUGO,
- },
- .size = 8,
- .read = w1_slave_read_id,
-};
+static struct device_attribute w1_slave_attr_id =
+ __ATTR(id, S_IRUGO, w1_slave_read_id, NULL);
/* Default family */
@@ -642,7 +627,7 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
}
/* Create "id" entry */
- err = sysfs_create_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+ err = device_create_file(&sl->dev, &w1_slave_attr_id);
if (err < 0) {
dev_err(&sl->dev,
"sysfs file creation for [%s] failed. err=%d\n",
@@ -664,7 +649,7 @@ static int __w1_attach_slave_device(struct w1_slave *sl)
return 0;
out_rem2:
- sysfs_remove_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+ device_remove_file(&sl->dev, &w1_slave_attr_id);
out_rem1:
device_remove_file(&sl->dev, &w1_slave_attr_name);
out_unreg:
@@ -746,7 +731,7 @@ void w1_slave_detach(struct w1_slave *sl)
msg.type = W1_SLAVE_REMOVE;
w1_netlink_send(sl->master, &msg);
- sysfs_remove_bin_file(&sl->dev.kobj, &w1_slave_attr_bin_id);
+ device_remove_file(&sl->dev, &w1_slave_attr_id);
device_remove_file(&sl->dev, &w1_slave_attr_name);
device_unregister(&sl->dev);
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug
2008-04-13 23:15 ` [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug David Fries
@ 2008-04-14 11:40 ` Evgeniy Polyakov
2008-04-14 22:22 ` David Fries
0 siblings, 1 reply; 13+ messages in thread
From: Evgeniy Polyakov @ 2008-04-14 11:40 UTC (permalink / raw)
To: David Fries; +Cc: linux-kernel
Hi David.
On Sun, Apr 13, 2008 at 06:15:37PM -0500, David Fries (david@fries.net) wrote:
> This is more to point out the bug in w1_slave_read_id, the next patch
> rewrites the routine.
>
> w1.c 1.15
...
> w1.c 1.16
That is what I talked about. I agree with your analysis of the problem
and I agree with fixes. But I do not agree with how you created them:
why do we ever neen 1.15 version, since immediately after that we have
1.16 which completely rewrite what was done in previous one?
This applies to the whole patchset: there are essentially several bug
fixes and several features, so separate them into several patches, which
do not collide with each other in the above way.
--
Evgeniy Polyakov
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug
2008-04-14 11:40 ` Evgeniy Polyakov
@ 2008-04-14 22:22 ` David Fries
0 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-14 22:22 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
On Mon, Apr 14, 2008 at 03:40:55PM +0400, Evgeniy Polyakov wrote:
> Hi David.
>
> On Sun, Apr 13, 2008 at 06:15:37PM -0500, David Fries (david@fries.net) wrote:
> > This is more to point out the bug in w1_slave_read_id, the next patch
> > rewrites the routine.
> >
> > w1.c 1.15
> ...
> > w1.c 1.16
>
>
> That is what I talked about. I agree with your analysis of the problem
> and I agree with fixes. But I do not agree with how you created them:
> why do we ever neen 1.15 version, since immediately after that we have
> 1.16 which completely rewrite what was done in previous one?
> This applies to the whole patchset: there are essentially several bug
> fixes and several features, so separate them into several patches, which
> do not collide with each other in the above way.
The old patch 14/35 and 15/35 was the only place I know of where I
fixed the bug, then replaced it wholesale with something better.
Several bugs, a few features, lots of patches, go figure.
--
David Fries <david@fries.net>
http://fries.net/~david/ (PGP encryption key available)
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 32/33 replaces 34/35] Documentation/w1/masters/ds2490 update
2008-04-13 22:56 ` David Fries
` (5 preceding siblings ...)
2008-04-13 23:15 ` [PATCH 13/33 replaces 14/35 and 15/35] W1: w1_slave_read_id multiple short read bug David Fries
@ 2008-04-13 23:15 ` David Fries
2008-04-14 12:30 ` [PATCH 0/35] W1: w1 core deadlock and oops fixes, ds2490 updates Evgeniy Polyakov
7 siblings, 0 replies; 13+ messages in thread
From: David Fries @ 2008-04-13 23:15 UTC (permalink / raw)
To: Evgeniy Polyakov; +Cc: linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4959 bytes --]
===================================================================
This is an update to the previous set of patches, don't merge them
yet. Some of the context was messed up and I don't expect patch to
apply the updates with the previous set cleanly. I'll resubmit the
entire set if these updates look good.
===================================================================
Documentation/w1/masters/ds2490 1.1 through 1.4
Provide some additional details about the status of the driver.
Signed-off-by: David Fries <david@fries.net>
---
Documentation/w1/masters/ds2490 | 62 +++++++++++++++++++++++++++++++++++++++
1 files changed, 62 insertions(+), 0 deletions(-)
diff --git a/Documentation/w1/masters/ds2490 b/Documentation/w1/masters/ds2490
index 239f9ae..f8648d2 100644
--- a/Documentation/w1/masters/ds2490
+++ b/Documentation/w1/masters/ds2490
@@ -16,3 +16,65 @@ which allows to build USB <-> W1 bridges.
DS9490(R) is a USB <-> W1 bus master device
which has 0x81 family ID integrated chip and DS2490
low-level operational chip.
+
+Notes and limitations.
+- The weak pullup current is a minimum of 0.9mA and maximum of 6.0mA.
+- The 5V strong pullup is supported with a minimum of 5.9mA and a
+ maximum of 30.4 mA. (From DS2490.pdf)
+- While the ds2490 supports a hardware search the code doesn't take
+ advantage of it.
+- The hardware will detect when devices are attached to the bus on the
+ next bus (reset?) operation, however only a message is printed as
+ the core w1 code doesn't make use of the information. Connecting
+ one device tends to give multiple new device notifications.
+- The number of USB bus transactions could be reduced if w1_reset_send
+ was added to the API. The name is just a suggestion. It would take
+ a write buffer and a read buffer (along with sizes) as arguments.
+ It would add match rom and rom to the send buffer, reset the bus,
+ and read the result. The ds2490 block I/O command supports reset,
+ write, read, and strong pullup all in one command. That would
+ reduce the time and overhead for a set of commands.
+
+ The core w1 functions required to do a temperature conversion are,
+ w1_reset_select_slave, w1_next_pullup, and w1_write_8, which turn
+ around and execute the master functions, reset_bus, write_block,
+ set_pullup, write_byte, set_pullup. Each ds2490 function will have
+ multiple USB bus transactions, except set_pullup where it only
+ requires transactions if the if the time value is different. The
+ conversion could be reduced to w1_next_pullup, w1_reset_send which
+ would call set_pullup, reset_send. The reset_send would enable reset
+ and strong pullup (if enabled), write the data buffer, execute the
+ block I/O command, read status, and get data. Currently I count 11
+ transactions required, this would reduce it to 4.
+- The hardware supports normal, flexable, and overdrive bus
+ communication speeds, but only the normal is supported.
+- The registered w1_bus_master functions don't define error
+ conditions. If a bus search is in progress and the ds2490 is
+ removed it can produce a good amount of error output before the bus
+ search finishes.
+- The hardware supports detecting some error conditions, such as
+ short, alarming presence on reset, and no presence on reset, but the
+ driver doesn't query those values.
+- The ds2490 specification doesn't cover short bulk in reads in
+ detail, but my observation is if fewer bytes are requested than are
+ available, the bulk read will return an error and the hardware will
+ clear the entire bulk in buffer. It would be possible to read the
+ maximum buffer size to not run into this error condition, only extra
+ bytes in the buffer is a logic error in the driver. The code should
+ should match reads and writes as well as data sizes. Reads and
+ writes are serialized and the status verifies that the chip is idle
+ (and data is available) before the read is executed, so it should
+ not happen.
+- Running x86_64 2.6.24 UHCI under qemu 0.9.0 under x86_64 2.6.22-rc6
+ with a OHCI controller, ds2490 running in the guest would operate
+ normally the first time the module was loaded after qemu attached
+ the ds2490 hardware, but if the module was unloaded, then reloaded
+ most of the time one of the bulk out or in, and usually the bulk in
+ would fail. qemu sets a 50ms timeout and the bulk in would timeout
+ even when the status shows data available. A bulk out write would
+ show a successful completion, but the ds2490 status register would
+ show 0 bytes written. Detaching qemu from the ds2490 hardware and
+ reattaching would clear the problem. usbmon output in the guest and
+ host did not explain the problem. My guess is a bug in either qemu
+ or the host OS and more likely the host OS.
+-- 03-06-2008 David Fries <David@Fries.net>
--
1.4.4.4
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH 0/35] W1: w1 core deadlock and oops fixes, ds2490 updates
2008-04-13 22:56 ` David Fries
` (6 preceding siblings ...)
2008-04-13 23:15 ` [PATCH 32/33 replaces 34/35] Documentation/w1/masters/ds2490 update David Fries
@ 2008-04-14 12:30 ` Evgeniy Polyakov
7 siblings, 0 replies; 13+ messages in thread
From: Evgeniy Polyakov @ 2008-04-14 12:30 UTC (permalink / raw)
To: David Fries; +Cc: linux-kernel, Andrew Morton
Hi.
On Sun, Apr 13, 2008 at 05:56:32PM -0500, David Fries (david@fries.net) wrote:
> I dug out a panic before I started modifying the driver, see the end
> of the message. There is still a remaining oops. Register the same
> sensor with two masters. The serial id of the sensor is assumed to be
> unique and the sysfs code will cause an oops when the second sensor is
> registered. If you have two master devices that are both doing bus
> searching, let one detect the sensor, the move it to the other master.
> The second master will detect the device before the first one times it
> out. None of my patches address this.
This is first time you show me the oops, so I we can start thinking
about bug itself. Getting that you already have a fix and changes which
I agree with, we can proceed with them. The only stuff I complain about
it how you organized your patchset. There is no need to put multiple
revisions of the same file in several patches when each new change just
overwrite previous one, this complicates code review and digging into
changeset changelogs in case of problems.
> Could you be more specific on "really wrong, like what you call a
> deadlock was intended"? I only use 'intended' once, and that is is
> patch 1 in "The slave reconnect was incorrectly coded." That
> paragraph is describing a infinite loop inserting a slave driver, not
> unloading a device driver.
I had a gpio driver which worked really bad, since current source was
very weak and shared with different resource. It was not pure luck to
get data from it, but it did not work reliably. So there are all those
loops you see in reading path, having them in userspace would provide a
great feeling of really broken hardware (like that broked rom data you
see blacklisted in the driver), while it just did not worked 100%
reliably. Because of that I did not allow to forcedly stop
'transaction', since getting it right took lots of time, so we do want
that data, so no interrupt, so removing module stops...
Yes, this is really ugly, but it works and was added intentionally.
Now I think that for better quality and simplicity of the code we can
drop this behaviour.
> If something is really wrong with the descriptions I'll fix it.
>
> > There is another problem with your submission - please split it into 3-5
> > patches which are logically compound, so that there would not be things
> > like: patch1: replace A with B, patch2: remove B and the like.
>
> You must be talking about patch 14 and patch 15 w1_slave_read_id sort
> read bug. That's the only place it does that.
>
> I debated on how to submit
> patch 14 "w1_slave_read_id multiple short read bug"
> and
> patch 15 "w1_slave_read_id from bin_attribute to device_attribute"
> and I did it that way to first show what he bug was and fix it, then
> show how simple the code was using a different method. I'll submit it
> in one patch.
>
> > You also have lots of codying style issues in the patchset.
> Such as?
if( without space
a=b without spaces and the like. Pretty trivial, but I will be frowned
upon if accept that :)
> This was the overview e-mail, and it was long enough without going
> into details on each of the issues. But here they are.
I agree with what you pointed is not correct, including 'deadlock', but
I already described why I did it that way. It does work and is ugly and
is probably wrong, so I agreed to remove this 'feature'.
The same applies to other bug fixes you provided: I agreew ith them
(even though some of them I see first time)
> > Please reorganize your patches into something like this:
> > 1. threads changes
> > 2. master device removal changes (remove slaves which are on the bus
> > master being removed) and related things
> > 3. ds2490 changes
> > 4. cleanups
> >
> > or something like that, reviewing 35 small patches which are removing
> > things past another is not a really good time...
>
> You want me to take 35 patches and merge them down to 4? There were a
> lot of issues in here, and I don't have an automagic patch reorder
> tool. If you want to review fewer patches apply 1-6, do a diff and
> look at that.
>
> Documentation/SubmittingPatches '3) Separate your changes.' It goes on
> to specifically separate out bug fixes and performance enhancements.
Your changes overwrite each other, and some times adds something or
remove something to accomplish requested goal. I specially pointed to
one of them in your last patchset, which you marked rfc and not for
merge. Change separation does not mean all commits should be sent as
independent patch, logical changes should be combined. When patch 2
overwrite patch1 and does something similar, combine them.
Bug fixes to bug fixes, features to features. If some overwrite fix a
bug, send that overwrite.
> You requested in another e-mail to merge patch 1-6. The deadlocks in
> 1 is clearly a bug. 6 block (instead of sleeping with a timeout) is a
> performance enhancement. 5 might be a deadlock, but it's not at all
> related to 1. 2 and 3 have nothing to do with any of the above.
> Logically this come out to be a bunch of patches.
I roughly pointed how to organize them. My main intention was to
logically separate changes, not sending every single commit in different
patch.
> On Mon, Jan 21, 2008 at 06:56:35PM -0800, Andrew Morton wrote:
> > On Mon, 21 Jan 2008 20:47:19 -0600 David Fries <david@fries.net> wrote:
> > > Thanks. Question, at what point should I just submit these one liner,
> > > but independent bugs in one patch?
> >
> > You shouldn't, unless they are dependent in both directions.
>
> If Andrew Morton is going to say no for independent one line bugs, I'm
> going to say No (for a bunch of complicated sometimes overlapping
> bugs, features, and performance enhancements) as well. Besides I have
> something like 27KB in 664 lines of descriptions, if I tried to merge
> the changes down there's no way I could figure out which patch lines
> went with what descriptions, and no one else could either.
I think I have to note that again: if some feature overwrite fix the
bug, then just send that overwrite, since it was original goal. Instead
you send number of one-line fixes, and then overwrite them. Less changes
we have (presumbly they are logically separated and do not break each
others), less time we will spend reviewing code, searching for wrong
commit, checking for bugs.
I understand that you do not want to do that just because you already
have cvs sommit log which you send to me, but this also means that I
will need to do that for your changes and then resubmit.
> w1_master_driver w1_bus_master1: Family 21 for 21.34c0000039c5.ba is not registered.
> Failed to read 1-wire data from 0x02: err=-110.
> Failed to read 1-wire data from 0x02: err=-110.
> usb 2-3: USB disconnect, address 5
> w1_master_driver w1_bus_master1: Waiting for w1_bus_master1 to become free: refcnt=1.
> w1_slave_release: Releasing 81-00000020bbd0.
> w1_slave_release: Releasing 21-34c0000039c5.
> w1_slave_release: Releasing 28-0000000e84a2.
> w1_slave_release: Releasing 81-00000020bbd0.
> usb 2-3: new full speed USB device using ohci_hcd and address 8
> usb 2-3: device descriptor read/64, error -62
> hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad?
> hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad?
> hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad?
> hub 2-0:1.0: Cannot enable port 3. Maybe the USB cable is bad?
> usb 2-3: new full speed USB device using ohci_hcd and address 12
> usb 2-3: configuration #1 chosen from 1 choice
> usb 2-3: USB disconnect, address 12
> BUG: unable to handle kernel NULL pointer dereference at virtual address 0000000c
Does this happen only when you remove and plug ds9490 back to the
socket? We could fix it rather quickly, but I'm glad you fixed it
yourself :)
--
Evgeniy Polyakov
^ permalink raw reply [flat|nested] 13+ messages in thread