staging: comedi: Add helper macro for comedi pci driver boilerplate

drivers/staging/comedi/drivers.c void comedi_pci_auto_unconfig(struct pci_dev *pcidev) · line 962

Quoted 10 lines · H Hartley Sweeten

drivers/staging/comedi/drivers.c +10 -0

+int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	int ret;
+
+	ret = comedi_driver_register(comedi_driver);
+	if (ret<  0)
+		return ret;
+
+	pci_driver->name = comedi_driver->driver_name;

I think it would be better if the driver modules that call this function already had pci_driver->name initialized statically.

Quoted 14 lines · Ian Abbott

drivers/staging/comedi/drivers.c void comedi_pci_auto_unconfig(struct pci_dev *pcidev) · line 962

Quoted 10 lines · H Hartley Sweeten

drivers/staging/comedi/drivers.c +10 -0

+int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	int ret;
+
+	ret = comedi_driver_register(comedi_driver);
+	if (ret<   0)
+		return ret;
+
+	pci_driver->name = comedi_driver->driver_name;

I think it would be better if the driver modules that call this function already had pci_driver->name initialized statically.

I agree, but this is how all the pci drivers do it now.

To change this will require auditing all the comedi pci drivers and adding the static name.

Quoted 16 lines · H Hartley Sweeten

Quoted 14 lines · Ian Abbott

drivers/staging/comedi/drivers.c void comedi_pci_auto_unconfig(struct pci_dev *pcidev) · line 962

Quoted 10 lines · H Hartley Sweeten

drivers/staging/comedi/drivers.c +10 -0

+int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	int ret;
+
+	ret = comedi_driver_register(comedi_driver);
+	if (ret<   0)
+		return ret;
+
+	pci_driver->name = comedi_driver->driver_name;

I think it would be better if the driver modules that call this function already had pci_driver->name initialized statically.

Quoted 4 lines

I agree, but this is how all the pci drivers do it now.

To change this will require auditing all the comedi pci drivers and adding the static name.

But if you're going to be calling this function from those drivers anyway, you might as well add the static names at the same time! Saves another round of patches.

Quoted 17 lines · Ian Abbott

Quoted 16 lines · H Hartley Sweeten

Quoted 14 lines · Ian Abbott

drivers/staging/comedi/drivers.c void comedi_pci_auto_unconfig(struct pci_dev *pcidev) · line 962

Quoted 10 lines · H Hartley Sweeten

drivers/staging/comedi/drivers.c +10 -0

+int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	int ret;
+
+	ret = comedi_driver_register(comedi_driver);
+	if (ret<    0)
+		return ret;
+
+	pci_driver->name = comedi_driver->driver_name;

I think it would be better if the driver modules that call this function already had pci_driver->name initialized statically.

Quoted 8 lines

Quoted 4 lines

I agree, but this is how all the pci drivers do it now.

To change this will require auditing all the comedi pci drivers and adding the static name.

But if you're going to be calling this function from those drivers anyway, you might as well add the static names at the same time! Saves another round of patches.

True, but many of the pci drivers still need quite a bit of cleanup. After this patch was accepted I was going to just change the ones that have already had the init/exit stuff cleaned up. Then as I hit all the others I would change them.

I guess I could do it this way:

1) add the "name" to all the pci drivers and remove the "pci_driver->name = comedi_driver->name" from all the module_init functions.

2) add the module_comedi_pci_driver stuff, without the "pci_driver->name = comedi_driver->name"

3) Update all the refactored comedi pci drivers to use the module_comedi_pci_driver macro.

4) As the other comedi pci drivers are refactored, use the module_comedi_pci_driver macro.

How does that sound?

Okay, but I don't think it's worth doing three patches for each driver where one would do. Adding a one-line .name = "foo", to the single patch for each driver you were planning to do anyway seems like a trivial addition that could be slotted into the patch without too much argument from the "one change per patch" folks, especially if you mention the purpose of that line in the commit message. (I'd defer to the better judgement of Greg though.)

Quoted 7 lines · Ian Abbott

Okay, but I don't think it's worth doing three patches for each driver where one would do. Adding a one-line .name = "foo", to the single patch for each driver you were planning to do anyway seems like a trivial addition that could be slotted into the patch without too much argument from the "one change per patch" folks, especially if you mention the purpose of that line in the commit message. (I'd defer to the better judgement of Greg though.)

With this patch my main goal was to not "break" any of the drivers.

As a follow-up to this patch I was going to use the macro in all the pci drivers that have been refactored. Similar to what I did with the module_comedi_driver macro. When I do that I can add the static name field to those drivers.

How about doing this in the register function:

	If (!pci_driver->name)
		pci_driver->name = comedi_driver->driver_name;

This way all the drivers that still need to be refactored will still work.

After all the drivers have been updated, those two lines can be removed.

So we have this patch and one big patch updating all the refactored drivers. Some number of patches refactoring the remaining drivers and using the module_comedi_pci_driver macro. Then one last patch removing the check above.

How does that sound?

Quoted 10 lines · H Hartley Sweeten

How about doing this in the register function:

	If (!pci_driver->name)
		pci_driver->name = comedi_driver->driver_name;

This way all the drivers that still need to be refactored will still work.

After all the drivers have been updated, those two lines can be removed.

Quoted 6 lines

So we have this patch and one big patch updating all the refactored drivers. Some number of patches refactoring the remaining drivers and using the module_comedi_pci_driver macro. Then one last patch removing the check above.

How does that sound?

That sounds reasonable to me.

drivers/staging/comedi/drivers.c +17 -0

@@ -950,6 +950,34 @@ void comedi_pci_auto_unconfig(struct pci_dev *pcidev)
 }
 EXPORT_SYMBOL_GPL(comedi_pci_auto_unconfig);
 
+int comedi_pci_driver_register(struct comedi_driver *comedi_driver,
+		struct pci_driver *pci_driver)
+{
+	int ret;
+
+	ret = comedi_driver_register(comedi_driver);
+	if (ret < 0)
+		return ret;
+
+	pci_driver->name = comedi_driver->driver_name;
+	ret = pci_register_driver(pci_driver);
+	if (ret < 0) {
+		comedi_driver_unregister(comedi_driver);
+		return ret;
+	}
+
+	return 0;

Quoted 2 lines · H Hartley Sweeten

drivers/staging/comedi/drivers.c +0 -0

Quoted 1 line

+EXPORT_SYMBOL(comedi_pci_driver_register);

Quoted 1 line

+EXPORT_SYMBOL(comedi_pci_driver_unregister);

EXPORT_SYMBOL_GPL() for these two functions?

Quoted 4 lines · Greg KH

Quoted 1 line · H Hartley Sweeten

+EXPORT_SYMBOL(comedi_pci_driver_unregister);

EXPORT_SYMBOL_GPL() for these two functions?

I wasn't sure about that...

Question.. when "should" EXPORT_SYMBOL be used and when should it be EXPORT_SYMBOL_GPL?

Quoted 6 lines · H Hartley Sweeten

Quoted 4 lines · Greg KH

Quoted 1 line · H Hartley Sweeten

+EXPORT_SYMBOL(comedi_pci_driver_unregister);

EXPORT_SYMBOL_GPL() for these two functions?

Quoted 4 lines

I wasn't sure about that...

Question.. when "should" EXPORT_SYMBOL be used and when should it be EXPORT_SYMBOL_GPL?

Adding EXPORT_SYMBOL_GPL shouldn't remove previous functionality. It doesn't in this case, so that's okay!