How to get driver_data of struct ieee1394_device_id in kernel driver module?

PATCH In review Stefan Richter 4 participants 12 messages May 26, 2013 – Jun 9, 2013 1 message not in the archive

Subsystems (6)

Media (V4L/DVB) Sound Staging Firewire +2 more subsystems

lore

Share & tools

lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Stefan Richter

author maintainer

2013-05-26 21:35 UTC #1 lore raw

(Adding LKML and Greg KH, in case that there are general opinions about how a bus_type should work.)

Quoted 21 lines · On May 26 Takashi Sakamoto wrote:

Hi all,
I'm a newbile for Linux Firewire subsystem and not used to IEEE 1394 bus programing in Linux.
So I happy to get your advices.
Currently I'm working for some drivers of ALSA in kernel land. Then some devices to which the same module applies need to handle its own operations.
To implement these operations, I think it good to utilize driver_data of struct ieee1394_evice_id entry.
This is example.
For entry:
https://github.com/takaswie/snd-firewire-improve/blob/f509e4587c3f7b18eda70d07b616ef01be3546ef/bebob/bebob.c#L462
For usage:
https://github.com/takaswie/snd-firewire-improve/blob/f509e4587c3f7b18eda70d07b616ef01be3546ef/bebob/bebob.c#L362
(In this case, I use the cast between (kernel_ulong_t) and (struct snd_bebob_ops *) but I have no confidence that this is better...)

Quoted 8 lines

Anyway, this module need to know the id for the current device in device_id table but I have no idea to get the id. Do you know good way to get it in module's probing process?
Regards
Takashi Sakamoto

I think your approach is sensible. There is of course just the little problem that firewire-core keeps the matching device_id table entry as a secret to itself. Therefore, struct ieee1394_device_id.driver_data is currently totally useless.

How about we make it available like in the following patch?

Besides being useful to your presently out-of-tree work, the in-tree sound/firewire/speakers.c::fwspk_detect() could be rewritten to use this approach. Maybe I will post an expanded version of this patch which incorporates such a first in-tree usage.

-------- 8< --------

From: Stefan Richter <[email protected]>
Subject: [PATCH] firewire: pass matching ieee1394_device_id to drivers via struct fw_unit

FireWire audio unit drivers will need to apply various model-specific operations. The struct ieee1394_device_id.driver_data member can be used to store respective flags or function pointer tables.

But until now drivers had no way to know which driver->id_table index was matched with an fw_unit instance. Other bus subsystems like pci or usb pass this information via an own driver probe method, whereas the firewire subsystem uses the stock struct device_driver.probe().

We could introduce a struct fw_driver.probe() which works similarly to struct pci_driver.probe() or struct usb_driver.probe(). But it seems simpler to just add a new member to struct fw_unit which caches the matching ieee1394_device_id, so this is what is done here.

Signed-off-by Stefan Richter <[email protected]>

---
 drivers/firewire/core-device.c |    7 +++++--
 include/linux/firewire.h       |    5 +++--
 2 files changed, 8 insertions(+), 4 deletions(-)

drivers/firewire/core-device.c +5 -2

@@ -169,6 +169,7 @@ static bool is_fw_unit(struct device *de
 
 static int fw_unit_match(struct device *dev, struct device_driver *drv)
 {
+	struct fw_unit *unit = fw_unit(dev);
 	const struct ieee1394_device_id *id_table =
 			container_of(drv, struct fw_driver, driver)->id_table;
 	int id[] = {0, 0, 0, 0};

@@ -177,11 +178,13 @@ static int fw_unit_match(struct device *
 	if (!is_fw_unit(dev))
 		return 0;
 
-	get_modalias_ids(fw_unit(dev), id);
+	get_modalias_ids(unit, id);
 
 	for (; id_table->match_flags != 0; id_table++)
-		if (match_ids(id_table, id))
+		if (match_ids(id_table, id)) {
+			unit->device_id = id_table;
 			return 1;
+		}
 
 	return 0;
 }

include/linux/firewire.h +3 -2

@@ -216,12 +216,15 @@ static inline int fw_device_is_shutdown(
 
 int fw_device_enable_phys_dma(struct fw_device *device);
 
+struct ieee1394_device_id;
+
 /*
  * fw_unit.directory must not be accessed after device_del(&fw_unit.device).
  */
 struct fw_unit {
 	struct device device;
 	const u32 *directory;
+	const struct ieee1394_device_id *device_id;
 	struct fw_attribute_group attribute_group;
 };

@@ -247,8 +250,6 @@ static inline struct fw_device *fw_paren
 	return fw_device(unit->device.parent);
 }
 
-struct ieee1394_device_id;
-
 struct fw_driver {
 	struct device_driver driver;
 	/* Called when the parent device sits through a bus reset. */

Signature · 3 lines

Stefan Richter
-=====-===-= -=-= ==-=-
http://arcgraph.de/sr/

Stefan Richter

author maintainer

2013-05-26 22:16 UTC #2 lore raw

Quoted 32 lines · On May 26 Stefan Richter wrote:

(Adding LKML and Greg KH, in case that there are general opinions about how a bus_type should work.)
Quoted 28 lines · On May 26 Takashi Sakamoto wrote:
Hi all,
I'm a newbile for Linux Firewire subsystem and not used to IEEE 1394 bus programing in Linux.
So I happy to get your advices.
Currently I'm working for some drivers of ALSA in kernel land. Then some devices to which the same module applies need to handle its own operations.
To implement these operations, I think it good to utilize driver_data of struct ieee1394_evice_id entry.
This is example.
For entry:
https://github.com/takaswie/snd-firewire-improve/blob/f509e4587c3f7b18eda70d07b616ef01be3546ef/bebob/bebob.c#L462
For usage:
https://github.com/takaswie/snd-firewire-improve/blob/f509e4587c3f7b18eda70d07b616ef01be3546ef/bebob/bebob.c#L362
(In this case, I use the cast between (kernel_ulong_t) and (struct snd_bebob_ops *) but I have no confidence that this is better...)
Anyway, this module need to know the id for the current device in device_id table but I have no idea to get the id. Do you know good way to get it in module's probing process?
Regards

Quoted 8 lines

Quoted 1 line
Takashi Sakamoto
I think your approach is sensible. There is of course just the little problem that firewire-core keeps the matching device_id table entry as a secret to itself. Therefore, struct ieee1394_device_id.driver_data is currently totally useless.
How about we make it available like in the following patch?

From: Stefan Richter <[email protected]>
Subject: firewire: pass matching ieee1394_device_id to drivers via struct fw_unit

FireWire audio unit drivers will need to apply various model-specific operations. The struct ieee1394_device_id.driver_data member can be used to store respective flags or function pointer tables.

Signed-off-by Stefan Richter <[email protected]>

---
 drivers/firewire/core-device.c |    7 +++-
 include/linux/firewire.h       |    5 ++-
 sound/firewire/speakers.c      |   65 ++++++++++++---------------------
 3 files changed, 32 insertions(+), 45 deletions(-)

drivers/firewire/core-device.c +5 -2

@@ -168,21 +168,24 @@ static bool match_ids(const struct ieee1
 static bool is_fw_unit(struct device *dev);
 
 static int fw_unit_match(struct device *dev, struct device_driver *drv)
 {
+	struct fw_unit *unit = fw_unit(dev);
 	const struct ieee1394_device_id *id_table =
 			container_of(drv, struct fw_driver, driver)->id_table;
 	int id[] = {0, 0, 0, 0};
 
 	/* We only allow binding to fw_units. */
 	if (!is_fw_unit(dev))
 		return 0;
 
-	get_modalias_ids(fw_unit(dev), id);
+	get_modalias_ids(unit, id);
 
 	for (; id_table->match_flags != 0; id_table++)
-		if (match_ids(id_table, id))
+		if (match_ids(id_table, id)) {
+			unit->device_id = id_table;
 			return 1;
+		}
 
 	return 0;
 }

include/linux/firewire.h +3 -2

@@ -215,14 +215,17 @@ static inline int fw_device_is_shutdown(
 }
 
 int fw_device_enable_phys_dma(struct fw_device *device);
 
+struct ieee1394_device_id;
+
 /*
  * fw_unit.directory must not be accessed after device_del(&fw_unit.device).
  */
 struct fw_unit {
 	struct device device;
 	const u32 *directory;
+	const struct ieee1394_device_id *device_id;
 	struct fw_attribute_group attribute_group;
 };
 
 static inline struct fw_unit *fw_unit(struct device *dev)

@@ -246,10 +249,8 @@ static inline struct fw_device *fw_paren
 {
 	return fw_device(unit->device.parent);
 }
 
-struct ieee1394_device_id;
-
 struct fw_driver {
 	struct device_driver driver;
 	/* Called when the parent device sits through a bus reset. */
 	void (*update)(struct fw_unit *unit);

sound/firewire/speakers.c +24 ~~-41~~

105 lines not on this page · raw message

Signature · 3 lines

Stefan Richter
-=====-===-= -=-= ==-==
http://arcgraph.de/sr/

Greg Kroah-Hartman

maintainer

2013-05-26 22:57 UTC #3 lore raw

In reply to Stefan Richter

Quoted 11 lines · Stefan Richter

I think your approach is sensible. There is of course just the little problem that firewire-core keeps the matching device_id table entry as a secret to itself. Therefore, struct ieee1394_device_id.driver_data is currently totally useless.
How about we make it available like in the following patch?
Besides being useful to your presently out-of-tree work, the in-tree sound/firewire/speakers.c::fwspk_detect() could be rewritten to use this approach. Maybe I will post an expanded version of this patch which incorporates such a first in-tree usage.

Why not pass it in the probe() function, like USB and PCI does? That way, if the driver wants to save it for that device, it can.

thanks,

greg k-h

Stefan Richter

author maintainer

2013-05-29 18:10 UTC #4 lore raw

Quoted 2 lines · On May 27 Greg Kroah-Hartman wrote:

Why not pass it in the probe() function, like USB and PCI does? That way, if the driver wants to save it for that device, it can.

I will try that, probably at the weekend.

Signature · 3 lines

Stefan Richter
-=====-===-= -=-= ===-=
http://arcgraph.de/sr/

Takashi Sakamoto

maintainer

2013-05-30 12:24 UTC #5 lore raw

In reply to Greg Kroah-Hartman

Hi Stefan and Greg,

I'm glad to contact with you.

I'm quite a beginner of Linux Firewire subsystem (Juju) and the others like PCI and USB. So it's hard for me to realize what Juju should be.
But I can rebuild Juju and test it with my devices and modules for which I'm working. I'm pleased to test your patches in this way if you need to check probe() function in driver module.

Thanks

Takashi Sakamoto

Quoted 19 lines · Greg Kroah-Hartman

Quoted 11 lines · Stefan Richter
I think your approach is sensible. There is of course just the little problem that firewire-core keeps the matching device_id table entry as a secret to itself. Therefore, struct ieee1394_device_id.driver_data is currently totally useless.
How about we make it available like in the following patch?
Besides being useful to your presently out-of-tree work, the in-tree sound/firewire/speakers.c::fwspk_detect() could be rewritten to use this approach. Maybe I will post an expanded version of this patch which incorporates such a first in-tree usage.
Why not pass it in the probe() function, like USB and PCI does? That way, if the driver wants to save it for that device, it can.
thanks,
greg k-h

Stefan Richter

author maintainer

2013-06-02 22:28 UTC #6 lore raw

PATCH 1/1 firewire: introduce fw_driver.probe and .remove methods

FireWire upper layer drivers are converted from generic
struct driver.probe() and .remove()
to bus-specific
struct fw_driver.probe() and .remove().

The new .probe() adds a const struct ieee1394_device_id *id argument, indicating the entry in the driver's device identifiers table which matched the fw_unit to be probed. This new argument is used by the snd-firewire-speakers driver to look up device-specific parameters and methods. There is at least one other FireWire audio driver currently in development in which this will be useful too.

The new .remove() drops the unused error return code.

Although all in-tree drivers are being converted to the new methods, support for the old methods is left in place for the time being, aiming to avoid conflicts if/when new drivers are being merged into the mainline via other trees.

Signed-off-by Stefan Richter <[email protected]>

Cc Clemens Ladisch <[email protected]>

Cc Peter Hurley <[email protected]>

---
 drivers/firewire/core-device.c      |   42 +++++++++--
 drivers/firewire/net.c              |   50 ++++++-------
 drivers/firewire/sbp2.c             |   17 ++--
 drivers/media/firewire/firedtv-fw.c |   19 ++---
 drivers/staging/fwserial/fwserial.c |   14 ++--
 include/linux/firewire.h            |    2 +
 sound/firewire/isight.c             |   44 ++++++------
 sound/firewire/scs1x.c              |   39 +++++-----
 sound/firewire/speakers.c           |  106 +++++++++++-----------------
 9 files changed, 166 insertions(+), 167 deletions(-)

drivers/firewire/core-device.c +36 -6

@@ -167,23 +167,51 @@ static bool match_ids(const struct ieee1
 
 static bool is_fw_unit(struct device *dev);
 
-static int fw_unit_match(struct device *dev, struct device_driver *drv)
+static const struct ieee1394_device_id *unit_match(struct device *dev,
+						   struct device_driver *drv)
 {
 	const struct ieee1394_device_id *id_table =
 			container_of(drv, struct fw_driver, driver)->id_table;
 	int id[] = {0, 0, 0, 0};
 
+	get_modalias_ids(fw_unit(dev), id);
+
+	for (; id_table->match_flags != 0; id_table++)
+		if (match_ids(id_table, id))
+			return id_table;
+
+	return NULL;
+}
+
+static int fw_unit_match(struct device *dev, struct device_driver *drv)
+{
 	/* We only allow binding to fw_units. */
 	if (!is_fw_unit(dev))
 		return 0;
 
-	get_modalias_ids(fw_unit(dev), id);
+	return !!unit_match(dev, drv);
+}
 
-	for (; id_table->match_flags != 0; id_table++)
-		if (match_ids(id_table, id))
-			return 1;
+static int fw_unit_probe(struct device *dev)
+{
+	struct fw_driver *driver =
+			container_of(dev->driver, struct fw_driver, driver);
 
-	return 0;
+	if (driver->probe)
+		return driver->probe(fw_unit(dev), unit_match(dev, dev->driver));
+	else
+		return driver->driver.probe(dev);
+}
+
+static int fw_unit_remove(struct device *dev)
+{
+	struct fw_driver *driver =
+			container_of(dev->driver, struct fw_driver, driver);
+
+	if (driver->remove)
+		return driver->remove(fw_unit(dev)), 0;
+	else
+		return driver->driver.remove(dev);
 }
 
 static int get_modalias(struct fw_unit *unit, char *buffer, size_t buffer_size)

@@ -213,6 +241,8 @@ static int fw_unit_uevent(struct device
 struct bus_type fw_bus_type = {
 	.name = "firewire",
 	.match = fw_unit_match,
+	.probe = fw_unit_probe,
+	.remove = fw_unit_remove,
 };
 EXPORT_SYMBOL(fw_bus_type);

drivers/firewire/net.c +24 ~~-26~~

84 lines not on this page · raw message

drivers/firewire/sbp2.c +7 ~~-10~~

@@ -1128,11 +1128,10 @@ static void sbp2_init_workarounds(struct
 }
 
 static struct scsi_host_template scsi_driver_template;
-static int sbp2_remove(struct device *dev);
+static void sbp2_remove(struct fw_unit *unit);
 
-static int sbp2_probe(struct device *dev)
+static int sbp2_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 {
-	struct fw_unit *unit = fw_unit(dev);
 	struct fw_device *device = fw_parent_device(unit);
 	struct sbp2_target *tgt;
 	struct sbp2_logical_unit *lu;

@@ -1196,7 +1195,7 @@ static int sbp2_probe(struct device *dev
 	return 0;
 
  fail_remove:
-	sbp2_remove(dev);
+	sbp2_remove(unit);
 	return -ENOMEM;
 
  fail_shost_put:

@@ -1222,9 +1221,8 @@ static void sbp2_update(struct fw_unit *
 	}
 }
 
-static int sbp2_remove(struct device *dev)
+static void sbp2_remove(struct fw_unit *unit)
 {
-	struct fw_unit *unit = fw_unit(dev);
 	struct fw_device *device = fw_parent_device(unit);
 	struct sbp2_target *tgt = dev_get_drvdata(&unit->device);
 	struct sbp2_logical_unit *lu, *next;

@@ -1261,10 +1259,9 @@ static int sbp2_remove(struct device *de
 		kfree(lu);
 	}
 	scsi_remove_host(shost);
-	dev_notice(dev, "released target %d:0:0\n", shost->host_no);
+	dev_notice(&unit->device, "released target %d:0:0\n", shost->host_no);
 
 	scsi_host_put(shost);
-	return 0;
 }
 
 #define SBP2_UNIT_SPEC_ID_ENTRY	0x0000609e

@@ -1285,10 +1282,10 @@ static struct fw_driver sbp2_driver = {
 		.owner  = THIS_MODULE,
 		.name   = KBUILD_MODNAME,
 		.bus    = &fw_bus_type,
-		.probe  = sbp2_probe,
-		.remove = sbp2_remove,
 	},
+	.probe    = sbp2_probe,
 	.update   = sbp2_update,
+	.remove   = sbp2_remove,
 	.id_table = sbp2_id_table,
 };

drivers/media/firewire/firedtv-fw.c +9 ~~-10~~

@@ -248,7 +248,7 @@ static const char * const model_names[]
 /* Adjust the template string if models with longer names appear. */
 #define MAX_MODEL_NAME_LEN sizeof("FireDTV ????")
 
-static int node_probe(struct device *dev)
+static int node_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 {
 	struct firedtv *fdtv;
 	char name[MAX_MODEL_NAME_LEN];

@@ -258,8 +258,8 @@ static int node_probe(struct device *dev
 	if (!fdtv)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, fdtv);
-	fdtv->device		= dev;
+	dev_set_drvdata(&unit->device, fdtv);
+	fdtv->device		= &unit->device;
 	fdtv->isochannel	= -1;
 	fdtv->voltage		= 0xff;
 	fdtv->tone		= 0xff;

@@ -269,7 +269,7 @@ static int node_probe(struct device *dev
 	mutex_init(&fdtv->demux_mutex);
 	INIT_WORK(&fdtv->remote_ctrl_work, avc_remote_ctrl_work);
 
-	name_len = fw_csr_string(fw_unit(dev)->directory, CSR_MODEL,
+	name_len = fw_csr_string(unit->directory, CSR_MODEL,
 				 name, sizeof(name));
 	for (i = ARRAY_SIZE(model_names); --i; )
 		if (strlen(model_names[i]) <= name_len &&

@@ -277,7 +277,7 @@ static int node_probe(struct device *dev
 			break;
 	fdtv->type = i;
 
-	err = fdtv_register_rc(fdtv, dev);
+	err = fdtv_register_rc(fdtv, &unit->device);
 	if (err)
 		goto fail_free;

@@ -307,9 +307,9 @@ fail_free:
 	return err;
 }
 
-static int node_remove(struct device *dev)
+static void node_remove(struct fw_unit *unit)
 {
-	struct firedtv *fdtv = dev_get_drvdata(dev);
+	struct firedtv *fdtv = dev_get_drvdata(&unit->device);
 
 	fdtv_dvb_unregister(fdtv);

@@ -320,7 +320,6 @@ static int node_remove(struct device *de
 	fdtv_unregister_rc(fdtv);
 
 	kfree(fdtv);
-	return 0;
 }
 
 static void node_update(struct fw_unit *unit)

@@ -391,10 +390,10 @@ static struct fw_driver fdtv_driver = {
 		.owner  = THIS_MODULE,
 		.name   = "firedtv",
 		.bus    = &fw_bus_type,
-		.probe  = node_probe,
-		.remove = node_remove,
 	},
+	.probe    = node_probe,
 	.update   = node_update,
+	.remove   = node_remove,
 	.id_table = fdtv_id_table,
 };

drivers/staging/fwserial/fwserial.c +6 -8

@@ -2438,9 +2438,9 @@ free_ports:
  * last peer for a given fw_card triggering the destruction of the same
  * fw_serial for the same fw_card.
  */
-static int fwserial_probe(struct device *dev)
+static int fwserial_probe(struct fw_unit *unit,
+			  const struct ieee1394_device_id *id)
 {
-	struct fw_unit *unit = fw_unit(dev);
 	struct fw_serial *serial;
 	int err;

@@ -2462,9 +2462,9 @@ static int fwserial_probe(struct device
  * specific fw_card). If this is the last peer being removed, then trigger
  * the destruction of the underlying TTYs.
  */
-static int fwserial_remove(struct device *dev)
+static void fwserial_remove(struct fw_unit *unit)
 {
-	struct fwtty_peer *peer = dev_get_drvdata(dev);
+	struct fwtty_peer *peer = dev_get_drvdata(&unit->device);
 	struct fw_serial *serial = peer->serial;
 	int i;

@@ -2484,8 +2484,6 @@ static int fwserial_remove(struct device
 		kref_put(&serial->kref, fwserial_destroy);
 	}
 	mutex_unlock(&fwserial_list_mutex);
-
-	return 0;
 }
 
 /**

@@ -2530,10 +2528,10 @@ static struct fw_driver fwserial_driver
 		.owner  = THIS_MODULE,
 		.name   = KBUILD_MODNAME,
 		.bus    = &fw_bus_type,
-		.probe  = fwserial_probe,
-		.remove = fwserial_remove,
 	},
+	.probe    = fwserial_probe,
 	.update   = fwserial_update,
+	.remove   = fwserial_remove,
 	.id_table = fwserial_id_table,
 };

include/linux/firewire.h +2 -0

@@ -251,8 +251,10 @@ struct ieee1394_device_id;
 
 struct fw_driver {
 	struct device_driver driver;
+	int (*probe)(struct fw_unit *unit, const struct ieee1394_device_id *id);
 	/* Called when the parent device sits through a bus reset. */
 	void (*update)(struct fw_unit *unit);
+	void (*remove)(struct fw_unit *unit);
 	const struct ieee1394_device_id *id_table;
 };

sound/firewire/isight.c +21 ~~-23~~

83 lines not on this page · raw message

sound/firewire/scs1x.c +18 ~~-21~~

@@ -384,9 +384,8 @@ static void scs_card_free(struct snd_car
 	kfree(scs->buffer);
 }
 
-static int scs_probe(struct device *unit_dev)
+static int scs_probe(struct fw_unit *unit, const struct ieee1394_device_id *id)
 {
-	struct fw_unit *unit = fw_unit(unit_dev);
 	struct fw_device *fw_dev = fw_parent_device(unit);
 	struct snd_card *card;
 	struct scs *scs;

@@ -395,7 +394,7 @@ static int scs_probe(struct device *unit
 	err = snd_card_create(-16, NULL, THIS_MODULE, sizeof(*scs), &card);
 	if (err < 0)
 		return err;
-	snd_card_set_dev(card, unit_dev);
+	snd_card_set_dev(card, &unit->device);
 
 	scs = card->private_data;
 	scs->card = card;

@@ -440,7 +439,7 @@ static int scs_probe(struct device *unit
 	if (err < 0)
 		goto err_card;
 
-	dev_set_drvdata(unit_dev, scs);
+	dev_set_drvdata(&unit->device, scs);
 
 	return 0;

@@ -451,9 +450,20 @@ err_card:
 	return err;
 }
 
-static int scs_remove(struct device *dev)
+static void scs_update(struct fw_unit *unit)
 {
-	struct scs *scs = dev_get_drvdata(dev);
+	struct scs *scs = dev_get_drvdata(&unit->device);
+	__be64 data;
+
+	data = cpu_to_be64(((u64)HSS1394_TAG_CHANGE_ADDRESS << 56) |
+			   scs->hss_handler.offset);
+	snd_fw_transaction(scs->unit, TCODE_WRITE_BLOCK_REQUEST,
+			   HSS1394_ADDRESS, &data, 8);
+}
+
+static void scs_remove(struct fw_unit *unit)
+{
+	struct scs *scs = dev_get_drvdata(&unit->device);
 
 	snd_card_disconnect(scs->card);

@@ -465,19 +475,6 @@ static int scs_remove(struct device *dev
 	tasklet_kill(&scs->tasklet);
 
 	snd_card_free_when_closed(scs->card);
-
-	return 0;
-}
-
-static void scs_update(struct fw_unit *unit)
-{
-	struct scs *scs = dev_get_drvdata(&unit->device);
-	__be64 data;
-
-	data = cpu_to_be64(((u64)HSS1394_TAG_CHANGE_ADDRESS << 56) |
-			   scs->hss_handler.offset);
-	snd_fw_transaction(scs->unit, TCODE_WRITE_BLOCK_REQUEST,
-			   HSS1394_ADDRESS, &data, 8);
 }
 
 static const struct ieee1394_device_id scs_id_table[] = {

@@ -506,10 +503,10 @@ static struct fw_driver scs_driver = {
 		.owner  = THIS_MODULE,
 		.name   = KBUILD_MODNAME,
 		.bus    = &fw_bus_type,
-		.probe  = scs_probe,
-		.remove = scs_remove,
 	},
+	.probe    = scs_probe,
 	.update   = scs_update,
+	.remove   = scs_remove,
 	.id_table = scs_id_table,
 };

sound/firewire/speakers.c +43 ~~-63~~

162 lines not on this page · raw message

Signature · 3 lines

Stefan Richter
-=====-===-= -==- ---==
http://arcgraph.de/sr/

Stefan Richter

author maintainer

2013-06-02 22:33 UTC #7 lore raw

PATCH 1/1 firewire: remove support of fw_driver.driver.probe and .remove methods

After all drivers being converted to bus-specific .probe/.remove methods, remove support of the obsolete generic methods.

Signed-off-by Stefan Richter <[email protected]>

---
To be merged maybe one release cycle after patch "firewire: introduce fw_driver.probe and .remove methods".

 drivers/firewire/core-device.c |   12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

drivers/firewire/core-device.c +4 -8

@@ -197,10 +197,7 @@ static int fw_unit_probe(struct device *
 	struct fw_driver *driver =
 			container_of(dev->driver, struct fw_driver, driver);
 
-	if (driver->probe)
-		return driver->probe(fw_unit(dev), unit_match(dev, dev->driver));
-	else
-		return driver->driver.probe(dev);
+	return driver->probe(fw_unit(dev), unit_match(dev, dev->driver));
 }
 
 static int fw_unit_remove(struct device *dev)

@@ -208,10 +205,9 @@ static int fw_unit_remove(struct device
 	struct fw_driver *driver =
 			container_of(dev->driver, struct fw_driver, driver);
 
-	if (driver->remove)
-		return driver->remove(fw_unit(dev)), 0;
-	else
-		return driver->driver.remove(dev);
+	driver->remove(fw_unit(dev));
+
+	return 0;
 }
 
 static int get_modalias(struct fw_unit *unit, char *buffer, size_t buffer_size)

Signature · 3 lines

Stefan Richter
-=====-===-= -==- ---==
http://arcgraph.de/sr/

Greg Kroah-Hartman

maintainer

2013-06-02 22:38 UTC #8 lore raw

Re: [PATCH] firewire: introduce fw_driver.probe and .remove methods

In reply to Stefan Richter

Quoted 12 lines · Stefan Richter

FireWire upper layer drivers are converted from generic
struct driver.probe() and .remove()
to bus-specific
struct fw_driver.probe() and .remove().
The new .probe() adds a const struct ieee1394_device_id *id argument, indicating the entry in the driver's device identifiers table which matched the fw_unit to be probed. This new argument is used by the snd-firewire-speakers driver to look up device-specific parameters and methods. There is at least one other FireWire audio driver currently in development in which this will be useful too.

Quoted 6 lines

The new .remove() drops the unused error return code.
Although all in-tree drivers are being converted to the new methods, support for the old methods is left in place for the time being, aiming to avoid conflicts if/when new drivers are being merged into the mainline via other trees.

Are there other firewire drivers in other trees in linux-next right now? If not, I'd just recommend converting everything over and not using the old functions at all. Especially as there really isn't that many firewire drivers, and new ones are pretty rare these days, right?

Other than that, your patch looks good.

thanks,

greg k-h

Stefan Richter

author maintainer

2013-06-02 23:35 UTC #9 lore raw

Re: [PATCH] firewire: introduce fw_driver.probe and .remove methods

Quoted 10 lines · On Jun 02 Greg Kroah-Hartman wrote:

Quoted 4 lines · Stefan Richter
Although all in-tree drivers are being converted to the new methods, support for the old methods is left in place for the time being, aiming to avoid conflicts if/when new drivers are being merged into the mainline via other trees.
Are there other firewire drivers in other trees in linux-next right now? If not, I'd just recommend converting everything over and not using the old functions at all. Especially as there really isn't that many firewire drivers, and new ones are pretty rare these days, right?

I think there is none in -next at the moment. But Sakamoto-san posted an RFC just two days ago:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-June/062614.html

This is one of two(?) drivers on which he is currently working on; the other one is the one which is going to need the new id argument in .probe(). And then there is another person working on yet another sound/firewire/ driver, but upstreaming of that driver might take a little more time for some more integration steps, from what I heard.

So, alternatively I could fold "firewire: remove support of fw_driver.driver.probe and .remove methods" into this one and commit it to a stable topic branch in linux1394.git (to be merged early after 3.10), and the audio driver developers could pull this topic branch if they want to and when they want to.

Quoted 1 line

Other than that, your patch looks good.

Thanks for review.

Signature · 3 lines

Stefan Richter
-=====-===-= -==- ---==
http://arcgraph.de/sr/

Takashi Sakamoto

maintainer

2013-06-03 03:59 UTC #10 lore raw

Re: [PATCH] firewire: introduce fw_driver.probe and .remove methods

Hi Stefan and Greg,

I test Stefan's patches and it works well with my devices.

Quoted 1 line

This is one of two(?) drivers on which he is currently working on;

Yes. I'm developing for two driver modules for some firewire sound device. One is "snd-fireworks" as Stefan mentioned and another is "snd-bebob" for BridgeCo.'s BeBoB based firewire sound devices. Both of them is now out of tree and in my github repository.

Quoted 2 lines

Quoted 2 lines
Are there other firewire drivers in other trees in linux-next right now?

I know the persons who work for "snd-dice", an driver module is for TC Applied Technologies' Dice based firewire sound devices. I'll inform them about your patches and topic branch.

Thanks a lot!

Takashi Sakamoto
[email protected]

Quoted 31 lines · Stefan Richter

Quoted 10 lines · On Jun 02 Greg Kroah-Hartman wrote:
Quoted 4 lines · Stefan Richter
Although all in-tree drivers are being converted to the new methods, support for the old methods is left in place for the time being, aiming to avoid conflicts if/when new drivers are being merged into the mainline via other trees.
Are there other firewire drivers in other trees in linux-next right now? If not, I'd just recommend converting everything over and not using the old functions at all. Especially as there really isn't that many firewire drivers, and new ones are pretty rare these days, right?
I think there is none in -next at the moment. But Sakamoto-san posted an RFC just two days ago:
http://mailman.alsa-project.org/pipermail/alsa-devel/2013-June/062614.html
This is one of two(?) drivers on which he is currently working on; the other one is the one which is going to need the new id argument in .probe(). And then there is another person working on yet another sound/firewire/ driver, but upstreaming of that driver might take a little more time for some more integration steps, from what I heard.
So, alternatively I could fold "firewire: remove support of fw_driver.driver.probe and .remove methods" into this one and commit it to a stable topic branch in linux1394.git (to be merged early after 3.10), and the audio driver developers could pull this topic branch if they want to and when they want to.
Quoted 1 line
Other than that, your patch looks good.
Thanks for review.

Stefan Richter

author maintainer

2013-06-09 17:04 UTC #11 lore raw

Re: [PATCH] firewire: introduce fw_driver.probe and .remove methods

I pushed the branch "fw_driver-api-transition" out to linux1394.git at

    git://git.kernel.org/pub/scm/linux/kernel/git/ieee1394/linux1394.git fw_driver-api-transition

This branch originates from v3.10-rc5 and contains only the single patch "firewire: introduce fw_driver.probe and .remove methods".

If you pull this branch, you _can_ move your drivers to the new methods.

Furthermore, the "master" and "for-next" branches at linux1394.git are now based on "fw_driver-api-transition" and contain patch "firewire:
remove support of fw_driver.driver.probe and .remove methods".

This means that starting from tomorrow, any FireWire driver which goes into linux-next _must_ implement the new methods.

Signature · 3 lines

Stefan Richter
-=====-===-= -==- -=--=
http://arcgraph.de/sr/

Clemens Ladisch

maintainer Acked-by on v1 1/1

2013-06-03 07:17 UTC #12 lore raw

Re: [PATCH] firewire: introduce fw_driver.probe and .remove methods

In reply to Stefan Richter

Quoted 4 lines · Stefan Richter

FireWire upper layer drivers are converted from generic
struct driver.probe() and .remove()
to bus-specific
struct fw_driver.probe() and .remove().

Acked-by Clemens Ladisch <[email protected]>

for these:

Quoted 8 lines

 sound/firewire/isight.c             |   44 ++++++------
 sound/firewire/scs1x.c              |   39 +++++-----
 sound/firewire/speakers.c           |  106 +++++++++++-----------------
Although all in-tree drivers are being converted to the new methods, support for the old methods is left in place for the time being, aiming to avoid conflicts if/when new drivers are being merged into the mainline via other trees.

AFAICS it's unlikely that any of the new sound drivers will be merged soon enough for this to matter.

Regards,
Clemens