drivers/w1/: why is dscore.c not ds9490r.c ?

PATCH In review Adrian Bunk 2 participants 13 messages Nov 21, 2004 – Nov 29, 2004 W1 dallas's 1-wire bus

lore

Share & tools

lore

Adrian Bunk

author

2004-11-21 22:02 UTC #0 lore raw

Hi Evgeniy,

drivers/w1/Makefile in recent 2.6 kernels contains:
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o

Is there a reason, why dscore.c isn't simply named ds9490r.c ?

TIA
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Evgeniy Polyakov

2004-11-22 07:28 UTC #1 lore raw

Quoted 1 line · Adrian Bunk

Hi Evgeniy,

Hello, Adrian.

Quoted 5 lines

drivers/w1/Makefile in recent 2.6 kernels contains:
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
Is there a reason, why dscore.c isn't simply named ds9490r.c ?

dscore.c is a core function set to work with ds2490 chip. ds9490* is built on top of it.
Any vendor can create it's own w1 bus master using this chip, not ds9490.

Quoted 2 lines

TIA
Adrian

Signature · 3 lines

	Evgeniy Polyakov

Crash is better than data corruption. -- Art Grabowski

Adrian Bunk

author

2004-11-22 13:35 UTC #2 lore raw

Quoted 4 lines · Evgeniy Polyakov

Quoted 1 line · Adrian Bunk
Hi Evgeniy,
Hello, Adrian.

Hi Evgeniy,

Quoted 10 lines

Quoted 5 lines
drivers/w1/Makefile in recent 2.6 kernels contains:
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
Is there a reason, why dscore.c isn't simply named ds9490r.c ?
dscore.c is a core function set to work with ds2490 chip. ds9490* is built on top of it.
Any vendor can create it's own w1 bus master using this chip, not ds9490.

if it was built on top of it, I'd have expected ds9490r.o to contain additional object files.

How would a different w1 bus master chip look like in drivers/w1/Makefile?

Quoted 1 line

Evgeniy Polyakov

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Evgeniy Polyakov

2004-11-22 16:55 UTC #3 lore raw

Quoted 14 lines · Adrian Bunk

Quoted 4 lines · Evgeniy Polyakov
Quoted 1 line · Adrian Bunk
Hi Evgeniy,
Hello, Adrian.
Hi Evgeniy,
Quoted 6 lines
Quoted 5 lines
drivers/w1/Makefile in recent 2.6 kernels contains:
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
Is there a reason, why dscore.c isn't simply named ds9490r.c ?

Quoted 7 lines

Quoted 4 lines
dscore.c is a core function set to work with ds2490 chip. ds9490* is built on top of it.
Any vendor can create it's own w1 bus master using this chip, not ds9490.
if it was built on top of it, I'd have expected ds9490r.o to contain additional object files.

DS9490 does not have anything except this chip and simple 64bit memory chip,
so it is not needed to have any additional code.

Quoted 2 lines

How would a different w1 bus master chip look like in drivers/w1/Makefile?

obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o

Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.

Quoted 5 lines

Quoted 1 line
Evgeniy Polyakov
cu
Adrian

Adrian Bunk

author

2004-11-22 16:55 UTC #4 lore raw

Quoted 30 lines · Evgeniy Polyakov

Quoted 14 lines · Adrian Bunk
Quoted 4 lines · Evgeniy Polyakov
Quoted 1 line · Adrian Bunk
Hi Evgeniy,
Hello, Adrian.
Hi Evgeniy,
Quoted 6 lines
Quoted 5 lines
drivers/w1/Makefile in recent 2.6 kernels contains:
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
Is there a reason, why dscore.c isn't simply named ds9490r.c ?
Quoted 7 lines
Quoted 4 lines
dscore.c is a core function set to work with ds2490 chip. ds9490* is built on top of it.
Any vendor can create it's own w1 bus master using this chip, not ds9490.
if it was built on top of it, I'd have expected ds9490r.o to contain additional object files.
DS9490 does not have anything except this chip and simple 64bit memory chip,
so it is not needed to have any additional code.
Quoted 2 lines
How would a different w1 bus master chip look like in drivers/w1/Makefile?

Quoted 6 lines

obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o
Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.

Why are you talking about proprietary modules living outside the kernel tree?

The only interesting case is the one of modules shipped with the kernel. And for them, this will break at link time if two such modules are included statically into the kernel.

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Evgeniy Polyakov

2004-11-22 17:12 UTC #5 lore raw

Quoted 38 lines · Adrian Bunk

Quoted 30 lines · Evgeniy Polyakov
Quoted 14 lines · Adrian Bunk
Quoted 4 lines · Evgeniy Polyakov
Quoted 1 line · Adrian Bunk
Hi Evgeniy,
Hello, Adrian.
Hi Evgeniy,
Quoted 6 lines
Quoted 5 lines
drivers/w1/Makefile in recent 2.6 kernels contains:
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
Is there a reason, why dscore.c isn't simply named ds9490r.c ?
Quoted 7 lines
Quoted 4 lines
dscore.c is a core function set to work with ds2490 chip. ds9490* is built on top of it.
Any vendor can create it's own w1 bus master using this chip, not ds9490.
if it was built on top of it, I'd have expected ds9490r.o to contain additional object files.
DS9490 does not have anything except this chip and simple 64bit memory chip,
so it is not needed to have any additional code.
Quoted 2 lines
How would a different w1 bus master chip look like in drivers/w1/Makefile?
Quoted 6 lines
obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o
Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.

Quoted 6 lines

Why are you talking about proprietary modules living outside the kernel tree?
The only interesting case is the one of modules shipped with the kernel. And for them, this will break at link time if two such modules are included statically into the kernel.

If we _currently_ do not have any open hw/module that depends on ds2490 core then it does not
mean that tomorrow noone will add it.

Quoted 3 lines

cu
Adrian

Adrian Bunk

author

2004-11-22 17:26 UTC #6 lore raw

Quoted 16 lines · Evgeniy Polyakov

Quoted 15 lines · Adrian Bunk
Quoted 10 lines · Evgeniy Polyakov
Quoted 3 lines
How would a different w1 bus master chip look like in drivers/w1/Makefile?
obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o
Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.
Why are you talking about proprietary modules living outside the kernel tree?

Quoted 7 lines

Quoted 3 lines
The only interesting case is the one of modules shipped with the kernel. And for them, this will break at link time if two such modules are included statically into the kernel.
If we _currently_ do not have any open hw/module that depends on ds2490 core then it does not
mean that tomorrow noone will add it.

Once again:
_this will break at link time if two such modules are included
statically into the kernel_

obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o

obj-$(CONFIG_W1_FOO) += foo.o
foo-objs := dscore.o

This will break with CONFIG_W1_DS9490=y and CONFIG_W1_FOO=y.

That drivers/w1/ contains many EXPORT_SYMBOL's with no in-kernel users is a different issue I might send a separate patch for (that besides proprietary modules there might come some day open source drivers using them is not a reason).

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Evgeniy Polyakov

2004-11-22 17:38 UTC #7 lore raw

Quoted 29 lines · Adrian Bunk

Quoted 16 lines · Evgeniy Polyakov
Quoted 15 lines · Adrian Bunk
Quoted 10 lines · Evgeniy Polyakov
Quoted 3 lines
How would a different w1 bus master chip look like in drivers/w1/Makefile?
obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o
Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.
Why are you talking about proprietary modules living outside the kernel tree?
Quoted 7 lines
Quoted 3 lines
The only interesting case is the one of modules shipped with the kernel. And for them, this will break at link time if two such modules are included statically into the kernel.
If we _currently_ do not have any open hw/module that depends on ds2490 core then it does not
mean that tomorrow noone will add it.
Once again:
_this will break at link time if two such modules are included
statically into the kernel_

Quoted 6 lines

obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
obj-$(CONFIG_W1_FOO) += foo.o
foo-objs := dscore.o

that should be follwing:

Kconfig:
foo depends on ds9490r

obj-$() += foo.o
foo-objs := foo_1.o foo_2.o

It just happened that ds9490r does not need any other parts but dscore.o.
That is why ds9490r.o have only dscore.o in it's dependency.

Quoted 7 lines

This will break with CONFIG_W1_DS9490=y and CONFIG_W1_FOO=y.
That drivers/w1/ contains many EXPORT_SYMBOL's with no in-kernel users is a different issue I might send a separate patch for (that besides proprietary modules there might come some day open source drivers using them is not a reason).

Why remove existing non disturbing set of exported functions? Are they violate some unknown issues?

Quoted 4 lines

cu
Adrian

Adrian Bunk

author

2004-11-23 00:25 UTC #8 lore raw

Quoted 42 lines · Evgeniy Polyakov

Quoted 29 lines · Adrian Bunk
Quoted 16 lines · Evgeniy Polyakov
Quoted 15 lines · Adrian Bunk
Quoted 10 lines · Evgeniy Polyakov
Quoted 3 lines
How would a different w1 bus master chip look like in drivers/w1/Makefile?
obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o
Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.
Why are you talking about proprietary modules living outside the kernel tree?
Quoted 7 lines
Quoted 3 lines
The only interesting case is the one of modules shipped with the kernel. And for them, this will break at link time if two such modules are included statically into the kernel.
If we _currently_ do not have any open hw/module that depends on ds2490 core then it does not
mean that tomorrow noone will add it.
Once again:
_this will break at link time if two such modules are included
statically into the kernel_
Quoted 6 lines
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
obj-$(CONFIG_W1_FOO) += foo.o
foo-objs := dscore.o
that should be follwing:
Kconfig:
foo depends on ds9490r

Quoted 6 lines

obj-$() += foo.o
foo-objs := foo_1.o foo_2.o
It just happened that ds9490r does not need any other parts but dscore.o.
That is why ds9490r.o have only dscore.o in it's dependency.

If foo_1 or foo_2 is dscore, you get exactly the compile breakage I described.

Quoted 10 lines

Quoted 7 lines
This will break with CONFIG_W1_DS9490=y and CONFIG_W1_FOO=y.
That drivers/w1/ contains many EXPORT_SYMBOL's with no in-kernel users is a different issue I might send a separate patch for (that besides proprietary modules there might come some day open source drivers using them is not a reason).
Why remove existing non disturbing set of exported functions? Are they violate some unknown issues?

If an export is currently unused, there's no need to export it.

If an export is only used for proprietary modules, that's a reason for an immediate removal of this export.

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Evgeniy Polyakov

2004-11-23 10:31 UTC #9 lore raw

Quoted 46 lines · Adrian Bunk

Quoted 45 lines · Evgeniy Polyakov
Quoted 29 lines · Adrian Bunk
Quoted 16 lines · Evgeniy Polyakov
Quoted 15 lines · Adrian Bunk
Quoted 10 lines · Evgeniy Polyakov
Quoted 3 lines
How would a different w1 bus master chip look like in drivers/w1/Makefile?
obj-m: proprietary_module.o
proprietary_module-objs: dscore.o proprietary_module_init.o
Actually it will live outside the kernel tree, but will require ds2490 driver.
It could be called ds2490.c but I think dscore is better name.
Why are you talking about proprietary modules living outside the kernel tree?
Quoted 7 lines
Quoted 3 lines
The only interesting case is the one of modules shipped with the kernel. And for them, this will break at link time if two such modules are included statically into the kernel.
If we _currently_ do not have any open hw/module that depends on ds2490 core then it does not
mean that tomorrow noone will add it.
Once again:
_this will break at link time if two such modules are included
statically into the kernel_
Quoted 6 lines
obj-$(CONFIG_W1_DS9490) += ds9490r.o
ds9490r-objs := dscore.o
obj-$(CONFIG_W1_FOO) += foo.o
foo-objs := dscore.o
that should be follwing:
Kconfig:
foo depends on ds9490r
obj-$() += foo.o
foo-objs := foo_1.o foo_2.o

Quoted 6 lines

Quoted 3 lines
It just happened that ds9490r does not need any other parts but dscore.o.
That is why ds9490r.o have only dscore.o in it's dependency.
If foo_1 or foo_2 is dscore, you get exactly the compile breakage I described.

foo_1 and foo_2 will not be dscore, since foo depends on ds9490 and thus dscore must be already built.
It looks like you were confused by dscore vs. ds9490 names. Probably it was not a good idea to
call it in a such way, but it was done and it works.

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
This will break with CONFIG_W1_DS9490=y and CONFIG_W1_FOO=y.
That drivers/w1/ contains many EXPORT_SYMBOL's with no in-kernel users is a different issue I might send a separate patch for (that besides proprietary modules there might come some day open source drivers using them is not a reason).

Quoted 7 lines

Quoted 2 lines
Why remove existing non disturbing set of exported functions? Are they violate some unknown issues?
If an export is currently unused, there's no need to export it.
If an export is only used for proprietary modules, that's a reason for an immediate removal of this export.

Sigh. It can be used by anyone who want to use it. I will take more carefull look at it later, thank you.

Quoted 3 lines

cu
Adrian

Adrian Bunk

author

2004-11-26 20:20 UTC #10 lore raw

Quoted 15 lines · Evgeniy Polyakov

Quoted 14 lines · Adrian Bunk
Quoted 12 lines · Evgeniy Polyakov
that should be follwing:
Kconfig:
foo depends on ds9490r
obj-$() += foo.o
foo-objs := foo_1.o foo_2.o
It just happened that ds9490r does not need any other parts but dscore.o.
That is why ds9490r.o have only dscore.o in it's dependency.

Quoted 8 lines

Quoted 2 lines
If foo_1 or foo_2 is dscore, you get exactly the compile breakage I described.
foo_1 and foo_2 will not be dscore, since foo depends on ds9490 and thus dscore must be already built.
It looks like you were confused by dscore vs. ds9490 names. Probably it was not a good idea to
call it in a such way, but it was done and it works.

Is there a reason against simply renaming dscore.c to ds9490r.c ?

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
That drivers/w1/ contains many EXPORT_SYMBOL's with no in-kernel users is a different issue I might send a separate patch for (that besides proprietary modules there might come some day open source drivers using them is not a reason).
Why remove existing non disturbing set of exported functions? Are they violate some unknown issues?

Quoted 6 lines

Quoted 4 lines
If an export is currently unused, there's no need to export it.
If an export is only used for proprietary modules, that's a reason for an immediate removal of this export.
Sigh. It can be used by anyone who want to use it.

That's not the way it's handled in the Linux kernel.

Proprietary modules are allowed, but support code only required by them but not used by free software has to be removed.

Quoted 1 line

I will take more carefull look at it later, thank you.

Thanks.

cu
Adrian

Signature · 4 lines

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Adrian Bunk

author

2004-11-29 01:53 UTC #11 lore raw

PATCH 1/1 drivers/w1/: possible cleanups

The patch below includes the following possible cleanups:
- make needlessly global code static
- remove unused code

Please review and comment which parts are correct and which conflict with pending changes that will add in-kernel users for the functions in question.

diffstat output:

 drivers/w1/dscore.c    |  103 +++++++++++------------------------------
 drivers/w1/dscore.h    |    7 --
 drivers/w1/w1.c        |   25 ++++-----
 drivers/w1/w1.h        |    1 
 drivers/w1/w1_family.c |   16 +-----
 drivers/w1/w1_family.h |    2 
 drivers/w1/w1_int.c    |    7 --
 drivers/w1/w1_int.h    |    2 
 drivers/w1/w1_io.c     |   13 ++---
 drivers/w1/w1_io.h     |    3 -
 10 files changed, 51 insertions(+), 128 deletions(-)

Signed-off-by Adrian Bunk <[email protected]>

drivers/w1/dscore.h renamed from drivers/w1/dscore.h.old +0 -7

@@ -156,14 +156,7 @@
 int ds_read_bit(struct ds_device *, u8 *);
 int ds_write_byte(struct ds_device *, u8);
 int ds_write_bit(struct ds_device *, u8);
-int ds_start_pulse(struct ds_device *, int);
-int ds_set_speed(struct ds_device *, int);
 int ds_reset(struct ds_device *, struct ds_status *);
-int ds_detect(struct ds_device *, struct ds_status *);
-int ds_stop_pulse(struct ds_device *, int);
-int ds_send_data(struct ds_device *, unsigned char *, int);
-int ds_recv_data(struct ds_device *, unsigned char *, int);
-int ds_recv_status(struct ds_device *, struct ds_status *);
 struct ds_device * ds_get_device(void);
 void ds_put_device(struct ds_device *);
 int ds_write_block(struct ds_device *, u8 *, int);

drivers/w1/dscore.c renamed from drivers/w1/dscore.c.old +28 ~~-75~~

211 lines not on this page · raw message

drivers/w1/w1_int.h renamed from drivers/w1/w1_int.h.old +0 -2

@@ -27,8 +27,6 @@
 
 #include "w1.h"
 
-struct w1_master * w1_alloc_dev(u32, int, int, struct device_driver *, struct device *);
-void w1_free_dev(struct w1_master *dev);
 int w1_add_master_device(struct w1_bus_master *);
 void w1_remove_master_device(struct w1_bus_master *);
 void __w1_remove_master_device(struct w1_master *);

drivers/w1/w1_int.c renamed from drivers/w1/w1_int.c.old +2 -5

@@ -30,7 +30,6 @@
 static u32 w1_ids = 1;
 
 extern struct device_driver w1_driver;
-extern struct bus_type w1_bus_type;
 extern struct device w1_device;
 extern int w1_max_slave_count;
 extern int w1_max_slave_ttl;

@@ -39,7 +38,7 @@
 
 extern int w1_process(void *);
 
-struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
+static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 	      struct device_driver *driver, struct device *device)
 {
 	struct w1_master *dev;

@@ -109,7 +108,7 @@
 	return dev;
 }
 
-void w1_free_dev(struct w1_master *dev)
+static void w1_free_dev(struct w1_master *dev)
 {
 	device_unregister(&dev->dev);
 	if (dev->nls->sk_socket)

@@ -220,8 +219,6 @@
 	__w1_remove_master_device(dev);
 }
 
-EXPORT_SYMBOL(w1_alloc_dev);
-EXPORT_SYMBOL(w1_free_dev);
 EXPORT_SYMBOL(w1_add_master_device);
 EXPORT_SYMBOL(w1_remove_master_device);
 EXPORT_SYMBOL(__w1_remove_master_device);

drivers/w1/w1.h renamed from drivers/w1/w1.h.old +0 -1

@@ -126,7 +126,6 @@
 };
 
 int w1_create_master_attributes(struct w1_master *);
-void w1_destroy_master_attributes(struct w1_master *);
 
 #endif /* __KERNEL__ */

drivers/w1/w1.c renamed from drivers/w1/w1.c.old +12 ~~-13~~

@@ -101,7 +101,7 @@
 	return sprintf(buf, "No family registered.\n");
 }
 
-struct bus_type w1_bus_type = {
+static struct bus_type w1_bus_type = {
 	.name = "w1",
 	.match = w1_master_match,
 };

@@ -139,7 +139,7 @@
 	.show = &w1_default_read_name,
 };
 
-ssize_t w1_master_attribute_show_name(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_name(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of (dev, struct w1_master, dev);
 	ssize_t count;

@@ -154,7 +154,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_pointer(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_pointer(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -168,14 +168,14 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_timeout(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_timeout(struct device *dev, char *buf)
 {
 	ssize_t count;
 	count = sprintf(buf, "%d\n", w1_timeout);
 	return count;
 }
 
-ssize_t w1_master_attribute_show_max_slave_count(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_max_slave_count(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -189,7 +189,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_attempts(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_attempts(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -203,7 +203,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_slave_count(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_slave_count(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -217,7 +217,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_slaves(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_slaves(struct device *dev, char *buf)
 
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);

@@ -600,7 +600,7 @@
 	return 0;
 }
 
-void w1_destroy_master_attributes(struct w1_master *dev)
+static void w1_destroy_master_attributes(struct w1_master *dev)
 {
 	device_remove_file(&dev->dev, &w1_master_attribute_slaves);
 	device_remove_file(&dev->dev, &w1_master_attribute_slave_count);

@@ -612,7 +612,7 @@
 }
 
 
-int w1_control(void *data)
+static int w1_control(void *data)
 {
 	struct w1_slave *sl;
 	struct w1_master *dev;

@@ -749,7 +749,7 @@
 	return 0;
 }
 
-int w1_init(void)
+static int w1_init(void)
 {
 	int retval;

@@ -789,7 +789,7 @@
 	return retval;
 }
 
-void w1_fini(void)
+static void w1_fini(void)
 {
 	struct w1_master *dev;
 	struct list_head *ent, *n;

@@ -811,4 +811,3 @@
 module_exit(w1_fini);
 
 EXPORT_SYMBOL(w1_create_master_attributes);
-EXPORT_SYMBOL(w1_destroy_master_attributes);

drivers/w1/w1_family.h renamed from drivers/w1/w1_family.h.old +0 -2

@@ -54,10 +54,8 @@
 
 extern spinlock_t w1_flock;
 
-void w1_family_get(struct w1_family *);
 void w1_family_put(struct w1_family *);
 void __w1_family_get(struct w1_family *);
-void __w1_family_put(struct w1_family *);
 struct w1_family * w1_family_registered(u8);
 void w1_unregister_family(struct w1_family *);
 int w1_register_family(struct w1_family *);

drivers/w1/w1_family.c renamed from drivers/w1/w1_family.c.old +3 ~~-13~~

@@ -115,25 +115,17 @@
 	return (ret) ? f : NULL;
 }
 
-void w1_family_put(struct w1_family *f)
-{
-	spin_lock(&w1_flock);
-	__w1_family_put(f);
-	spin_unlock(&w1_flock);
-}
-
-void __w1_family_put(struct w1_family *f)
+static void __w1_family_put(struct w1_family *f)
 {
 	if (atomic_dec_and_test(&f->refcnt))
 		f->need_exit = 1;
 }
 
-void w1_family_get(struct w1_family *f)
+void w1_family_put(struct w1_family *f)
 {
 	spin_lock(&w1_flock);
-	__w1_family_get(f);
+	__w1_family_put(f);
 	spin_unlock(&w1_flock);
-
 }
 
 void __w1_family_get(struct w1_family *f)

@@ -141,10 +133,8 @@
 	atomic_inc(&f->refcnt);
 }
 
-EXPORT_SYMBOL(w1_family_get);
 EXPORT_SYMBOL(w1_family_put);
 EXPORT_SYMBOL(__w1_family_get);
-EXPORT_SYMBOL(__w1_family_put);
 EXPORT_SYMBOL(w1_family_registered);
 EXPORT_SYMBOL(w1_unregister_family);
 EXPORT_SYMBOL(w1_register_family);

drivers/w1/w1_io.h renamed from drivers/w1/w1_io.h.old +0 -3

@@ -24,12 +24,9 @@
 
 #include "w1.h"
 
-void w1_delay(unsigned long);
 u8 w1_touch_bit(struct w1_master *, int);
 void w1_write_bit(struct w1_master *, int);
 void w1_write_8(struct w1_master *, u8);
-u8 w1_read_bit(struct w1_master *);
-u8 w1_read_8(struct w1_master *);
 int w1_reset_bus(struct w1_master *);
 u8 w1_calc_crc8(u8 *, int);
 void w1_write_block(struct w1_master *, u8 *, int);

drivers/w1/w1_io.c renamed from drivers/w1/w1_io.c.old +6 -7

@@ -28,7 +28,9 @@
 #include "w1_log.h"
 #include "w1_io.h"
 
-int w1_delay_parm = 1;
+static u8 w1_read_bit(struct w1_master *dev);
+
+static int w1_delay_parm = 1;
 module_param_named(delay_coef, w1_delay_parm, int, 0);
 
 static u8 w1_crc8_table[] = {

@@ -50,7 +52,7 @@
 	116, 42, 200, 150, 21, 75, 169, 247, 182, 232, 10, 84, 215, 137, 107, 53
 };
 
-void w1_delay(unsigned long tm)
+static void w1_delay(unsigned long tm)
 {
 	udelay(tm * w1_delay_parm);
 }

@@ -89,7 +91,7 @@
 			w1_write_bit(dev, (byte >> i) & 0x1);
 }
 
-u8 w1_read_bit(struct w1_master *dev)
+static u8 w1_read_bit(struct w1_master *dev)
 {
 	int result;

@@ -104,7 +106,7 @@
 	return result & 0x1;
 }
 
-u8 w1_read_8(struct w1_master * dev)
+static u8 w1_read_8(struct w1_master * dev)
 {
 	int i;
 	u8 res = 0;

@@ -176,10 +178,7 @@
 
 EXPORT_SYMBOL(w1_write_bit);
 EXPORT_SYMBOL(w1_write_8);
-EXPORT_SYMBOL(w1_read_bit);
-EXPORT_SYMBOL(w1_read_8);
 EXPORT_SYMBOL(w1_reset_bus);
 EXPORT_SYMBOL(w1_calc_crc8);
-EXPORT_SYMBOL(w1_delay);
 EXPORT_SYMBOL(w1_read_block);
 EXPORT_SYMBOL(w1_write_block);

Evgeniy Polyakov

2004-11-29 05:09 UTC #12 lore raw

Re: [2.6 patch] drivers/w1/: possible cleanups

Quoted 7 lines · Adrian Bunk

The patch below includes the following possible cleanups:
- make needlessly global code static
- remove unused code
Please review and comment which parts are correct and which conflict with pending changes that will add in-kernel users for the functions in question.

Thank you, but I will not apply your patch as is, since many parts of it are generic
and need to be used for any ds2490 based devices.

Comments below.

Quoted 20 lines

diffstat output:

 drivers/w1/dscore.c    |  103 +++++++++++------------------------------
 drivers/w1/dscore.h    |    7 --
 drivers/w1/w1.c        |   25 ++++-----
 drivers/w1/w1.h        |    1 
 drivers/w1/w1_family.c |   16 +-----
 drivers/w1/w1_family.h |    2 
 drivers/w1/w1_int.c    |    7 --
 drivers/w1/w1_int.h    |    2 
 drivers/w1/w1_io.c     |   13 ++---
 drivers/w1/w1_io.h     |    3 -
 10 files changed, 51 insertions(+), 128 deletions(-)

Signed-off-by Adrian Bunk <[email protected]>

drivers/w1/dscore.h renamed from drivers/w1/dscore.h.old +0 -0

@@ -156,14 +156,7 @@
 int ds_read_bit(struct ds_device *, u8 *);

drivers/w1/dscore.h line 166

Quoted 10 lines

drivers/w1/dscore.h renamed from drivers/w1/dscore.h.old +0 -7

 int ds_write_byte(struct ds_device *, u8);
 int ds_write_bit(struct ds_device *, u8);
-int ds_start_pulse(struct ds_device *, int);
-int ds_set_speed(struct ds_device *, int);
 int ds_reset(struct ds_device *, struct ds_status *);
-int ds_detect(struct ds_device *, struct ds_status *);
-int ds_stop_pulse(struct ds_device *, int);
-int ds_send_data(struct ds_device *, unsigned char *, int);
-int ds_recv_data(struct ds_device *, unsigned char *, int);
-int ds_recv_status(struct ds_device *, struct ds_status *);

1. ds_start_pulse() and ds_set_speed() are needed for eeprom programming and setting device speed.
2. ds_{send,recv}_data can be used to receive bulks of data.
3. ds_send_status() is used to obtain status.

You may say that they are used only in ds9490 driver, but not - they are generic functions to control any ds2490 based device.

Here is example: if I want to enable network only for netlink communication
I will get even some qdisk functions exported.
Any above and some below functions _can_ be used to control ds2490 based hardware.
DS9490R is _just_one_ of such devices. ds9490r is built on top of (actually it is only)
dscore.o.
dscore.o - this is _generic_ driver for DS2490 based hardware. Some of it requires above function, others not.
When(if) kernel connector patch will be applied, many of this functions can be accessed by it,
but nevertheless they are needed for some hardware, and thus can be accessed from it's drivers.

drivers/w1/dscore.c line 36

Quoted 13 lines

drivers/w1/dscore.h renamed from drivers/w1/dscore.h.old +0 -0

 struct ds_device * ds_get_device(void);
 void ds_put_device(struct ds_device *);
 int ds_write_block(struct ds_device *, u8 *, int);

drivers/w1/dscore.c renamed from drivers/w1/dscore.c.old +2 -2

@@ -32,22 +32,19 @@
 };
 MODULE_DEVICE_TABLE(usb, ds_id_table);
 
-int ds_probe(struct usb_interface *, const struct usb_device_id *);
-void ds_disconnect(struct usb_interface *);
+static int ds_probe(struct usb_interface *, const struct usb_device_id *);
+static void ds_disconnect(struct usb_interface *);

I agree here - this functions are Linux USB callbacks and can be static.

Quoted 6 lines

drivers/w1/dscore.c renamed from drivers/w1/dscore.c.old +1 -1
-int ds_init(void)
+static int ds_init(void)
 {
 	int err;
 

drivers/w1/dscore.c line 723

Quoted 8 lines

drivers/w1/dscore.c renamed from drivers/w1/dscore.c.old +1 -1

@@ -756,7 +717,7 @@
 	return 0;
 }
 
-void ds_fini(void)
+static void ds_fini(void)
 {
 	usb_deregister(&ds_driver);
 }

Sure, it is right changes.

Quoted 19 lines

drivers/w1/dscore.c renamed from drivers/w1/dscore.c.old +0 -8

@@ -774,15 +735,7 @@
 EXPORT_SYMBOL(ds_write_byte);
 EXPORT_SYMBOL(ds_write_bit);
 EXPORT_SYMBOL(ds_write_block);
-EXPORT_SYMBOL(ds_start_pulse);
-EXPORT_SYMBOL(ds_set_speed);
 EXPORT_SYMBOL(ds_reset);
-EXPORT_SYMBOL(ds_detect);
-EXPORT_SYMBOL(ds_stop_pulse);
-EXPORT_SYMBOL(ds_send_data);
-EXPORT_SYMBOL(ds_recv_data);
-EXPORT_SYMBOL(ds_recv_status);
-EXPORT_SYMBOL(ds_search);
 EXPORT_SYMBOL(ds_get_device);
 EXPORT_SYMBOL(ds_put_device);

drivers/w1/w1_int.h line 31

Quoted 5 lines

drivers/w1/w1_int.h renamed from drivers/w1/w1_int.h.old +0 -2

@@ -27,8 +27,6 @@
 
 #include "w1.h"
 
-struct w1_master * w1_alloc_dev(u32, int, int, struct device_driver *, struct device *);
-void w1_free_dev(struct w1_master *dev);

This can be static too.

drivers/w1/w1_int.c line 33

Quoted 10 lines

drivers/w1/w1_int.h renamed from drivers/w1/w1_int.h.old +0 -0

 int w1_add_master_device(struct w1_bus_master *);
 void w1_remove_master_device(struct w1_bus_master *);
 void __w1_remove_master_device(struct w1_master *);

drivers/w1/w1_int.c renamed from drivers/w1/w1_int.c.old +0 -1

@@ -30,7 +30,6 @@
 static u32 w1_ids = 1;
 
 extern struct device_driver w1_driver;
-extern struct bus_type w1_bus_type;

Ok.

Quoted 22 lines

drivers/w1/w1_int.c renamed from drivers/w1/w1_int.c.old +2 -2

 extern struct device w1_device;
 extern int w1_max_slave_count;
 extern int w1_max_slave_ttl;

@@ -39,7 +38,7 @@
 
 extern int w1_process(void *);
 
-struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
+static struct w1_master * w1_alloc_dev(u32 id, int slave_count, int slave_ttl,
 	      struct device_driver *driver, struct device *device)
 {
 	struct w1_master *dev;

@@ -109,7 +108,7 @@
 	return dev;
 }
 
-void w1_free_dev(struct w1_master *dev)
+static void w1_free_dev(struct w1_master *dev)
 {
 	device_unregister(&dev->dev);
 	if (dev->nls->sk_socket)

drivers/w1/w1_int.c line 224

Quoted 5 lines

drivers/w1/w1_int.c renamed from drivers/w1/w1_int.c.old +0 -2

@@ -220,8 +219,6 @@
 	__w1_remove_master_device(dev);
 }
 
-EXPORT_SYMBOL(w1_alloc_dev);
-EXPORT_SYMBOL(w1_free_dev);

As I say this change is right.

drivers/w1/w1.h line 129

Quoted 10 lines

drivers/w1/w1_int.c renamed from drivers/w1/w1_int.c.old +0 -0

 EXPORT_SYMBOL(w1_add_master_device);
 EXPORT_SYMBOL(w1_remove_master_device);
 EXPORT_SYMBOL(__w1_remove_master_device);

drivers/w1/w1.h renamed from drivers/w1/w1.h.old +0 -1

@@ -126,7 +126,6 @@
 };
 
 int w1_create_master_attributes(struct w1_master *);
-void w1_destroy_master_attributes(struct w1_master *);

I've exported it for completeness - it was used before in error exit path.
I need to change it back - w1_create_master_attributes() must be called before
w1_process() thread creation - this will reduce error recovery path.

Quoted 112 lines

drivers/w1/w1.h renamed from drivers/w1/w1.h.old +0 -0

 #endif /* __KERNEL__ */

drivers/w1/w1.c renamed from drivers/w1/w1.c.old +12 ~~-12~~

@@ -101,7 +101,7 @@
 	return sprintf(buf, "No family registered.\n");
 }
 
-struct bus_type w1_bus_type = {
+static struct bus_type w1_bus_type = {
 	.name = "w1",
 	.match = w1_master_match,
 };

@@ -139,7 +139,7 @@
 	.show = &w1_default_read_name,
 };
 
-ssize_t w1_master_attribute_show_name(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_name(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of (dev, struct w1_master, dev);
 	ssize_t count;

@@ -154,7 +154,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_pointer(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_pointer(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -168,14 +168,14 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_timeout(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_timeout(struct device *dev, char *buf)
 {
 	ssize_t count;
 	count = sprintf(buf, "%d\n", w1_timeout);
 	return count;
 }
 
-ssize_t w1_master_attribute_show_max_slave_count(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_max_slave_count(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -189,7 +189,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_attempts(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_attempts(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -203,7 +203,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_slave_count(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_slave_count(struct device *dev, char *buf)
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);
 	ssize_t count;

@@ -217,7 +217,7 @@
 	return count;
 }
 
-ssize_t w1_master_attribute_show_slaves(struct device *dev, char *buf)
+static ssize_t w1_master_attribute_show_slaves(struct device *dev, char *buf)
 
 {
 	struct w1_master *md = container_of(dev, struct w1_master, dev);

@@ -600,7 +600,7 @@
 	return 0;
 }
 
-void w1_destroy_master_attributes(struct w1_master *dev)
+static void w1_destroy_master_attributes(struct w1_master *dev)
 {
 	device_remove_file(&dev->dev, &w1_master_attribute_slaves);
 	device_remove_file(&dev->dev, &w1_master_attribute_slave_count);

@@ -612,7 +612,7 @@
 }
 
 
-int w1_control(void *data)
+static int w1_control(void *data)
 {
 	struct w1_slave *sl;
 	struct w1_master *dev;

@@ -749,7 +749,7 @@
 	return 0;
 }
 
-int w1_init(void)
+static int w1_init(void)
 {
 	int retval;

@@ -789,7 +789,7 @@
 	return retval;
 }
 
-void w1_fini(void)
+static void w1_fini(void)
 {
 	struct w1_master *dev;
 	struct list_head *ent, *n;

drivers/w1/w1.c line 811

Quoted 1 line

drivers/w1/w1.c renamed from drivers/w1/w1.c.old +0 -0
@@ -811,4 +811,3 @@
 module_exit(w1_fini);

I agree with above changes.

Quoted 47 lines

drivers/w1/w1.c renamed from drivers/w1/w1.c.old +0 -1

 EXPORT_SYMBOL(w1_create_master_attributes);
-EXPORT_SYMBOL(w1_destroy_master_attributes);

drivers/w1/w1_family.h renamed from drivers/w1/w1_family.h.old +0 -2

@@ -54,10 +54,8 @@
 
 extern spinlock_t w1_flock;
 
-void w1_family_get(struct w1_family *);
 void w1_family_put(struct w1_family *);
 void __w1_family_get(struct w1_family *);
-void __w1_family_put(struct w1_family *);
 struct w1_family * w1_family_registered(u8);
 void w1_unregister_family(struct w1_family *);
 int w1_register_family(struct w1_family *);

drivers/w1/w1_family.c renamed from drivers/w1/w1_family.c.old +3 ~~-11~~

@@ -115,25 +115,17 @@
 	return (ret) ? f : NULL;
 }
 
-void w1_family_put(struct w1_family *f)
-{
-	spin_lock(&w1_flock);
-	__w1_family_put(f);
-	spin_unlock(&w1_flock);
-}
-
-void __w1_family_put(struct w1_family *f)
+static void __w1_family_put(struct w1_family *f)
 {
 	if (atomic_dec_and_test(&f->refcnt))
 		f->need_exit = 1;
 }
 
-void w1_family_get(struct w1_family *f)
+void w1_family_put(struct w1_family *f)
 {
 	spin_lock(&w1_flock);
-	__w1_family_get(f);
+	__w1_family_put(f);
 	spin_unlock(&w1_flock);
-
 }
 
 void __w1_family_get(struct w1_family *f)

drivers/w1/w1_family.c line 140

Quoted 10 lines

drivers/w1/w1_family.c renamed from drivers/w1/w1_family.c.old +0 -2

@@ -141,10 +133,8 @@
 	atomic_inc(&f->refcnt);
 }
 
-EXPORT_SYMBOL(w1_family_get);
 EXPORT_SYMBOL(w1_family_put);
 EXPORT_SYMBOL(__w1_family_get);
-EXPORT_SYMBOL(__w1_family_put);
 EXPORT_SYMBOL(w1_family_registered);
 EXPORT_SYMBOL(w1_unregister_family);
 EXPORT_SYMBOL(w1_register_family);

You break good writting style.

Quoted 18 lines

drivers/w1/w1_io.h renamed from drivers/w1/w1_io.h.old +0 -3

@@ -24,12 +24,9 @@
 
 #include "w1.h"
 
-void w1_delay(unsigned long);
 u8 w1_touch_bit(struct w1_master *, int);
 void w1_write_bit(struct w1_master *, int);
 void w1_write_8(struct w1_master *, u8);
-u8 w1_read_bit(struct w1_master *);
-u8 w1_read_8(struct w1_master *);
 int w1_reset_bus(struct w1_master *);
 u8 w1_calc_crc8(u8 *, int);
 void w1_write_block(struct w1_master *, u8 *, int);

drivers/w1/w1_io.c line 34

Quoted 8 lines

drivers/w1/w1_io.c renamed from drivers/w1/w1_io.c.old +3 -1

@@ -28,7 +28,9 @@
 #include "w1_log.h"
 #include "w1_io.h"
 
-int w1_delay_parm = 1;
+static u8 w1_read_bit(struct w1_master *dev);
+
+static int w1_delay_parm = 1;
 module_param_named(delay_coef, w1_delay_parm, int, 0);

This can not be static - embedded setups without modules support with unusual
HZ value need this value to create hacks over udelay() to control protocol timslices.
I have discussion with some arm people about it - this will be changed when kernel
connector will be in mainline.

Quoted 29 lines

drivers/w1/w1_io.c renamed from drivers/w1/w1_io.c.old +3 -3

 static u8 w1_crc8_table[] = {

@@ -50,7 +52,7 @@
 	116, 42, 200, 150, 21, 75, 169, 247, 182, 232, 10, 84, 215, 137, 107, 53
 };
 
-void w1_delay(unsigned long tm)
+static void w1_delay(unsigned long tm)
 {
 	udelay(tm * w1_delay_parm);
 }

@@ -89,7 +91,7 @@
 			w1_write_bit(dev, (byte >> i) & 0x1);
 }
 
-u8 w1_read_bit(struct w1_master *dev)
+static u8 w1_read_bit(struct w1_master *dev)
 {
 	int result;

@@ -104,7 +106,7 @@
 	return result & 0x1;
 }
 
-u8 w1_read_8(struct w1_master * dev)
+static u8 w1_read_8(struct w1_master * dev)
 {
 	int i;
 	u8 res = 0;

drivers/w1/w1_io.c line 184

Quoted 10 lines

drivers/w1/w1_io.c renamed from drivers/w1/w1_io.c.old +0 -3

@@ -176,10 +178,7 @@
 
 EXPORT_SYMBOL(w1_write_bit);
 EXPORT_SYMBOL(w1_write_8);
-EXPORT_SYMBOL(w1_read_bit);
-EXPORT_SYMBOL(w1_read_8);
 EXPORT_SYMBOL(w1_reset_bus);
 EXPORT_SYMBOL(w1_calc_crc8);
-EXPORT_SYMBOL(w1_delay);
 EXPORT_SYMBOL(w1_read_block);
 EXPORT_SYMBOL(w1_write_block);

You break it again.

Resume:
1. Lets wait untill current pending patches will be in mainline - it includes your previous changes.
2. I will push upstream some of the above changes after 1.

I greatly appreciate your help, thank you Adrian.

Signature · 3 lines

        Evgeniy Polyakov

Crash is better than data corruption -- Arthur Grabowski