Re: [PATCH 1/6] driver-core: platform: Provide helpers for multi-driver modules

[Thierry Reding]

On Thu, Sep 24, 2015 at 07:02:36PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@xxxxxxxxxx>
> 
> Some modules register several sub-drivers. Provide a helper that makes
> it easy to register and unregister a list of sub-drivers, as well as
> unwind properly on error.
> 
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
> ---
>  Documentation/driver-model/platform.txt | 11 ++++++
>  drivers/base/platform.c                 | 60 +++++++++++++++++++++++++++++++++
>  include/linux/platform_device.h         |  5 +++
>  3 files changed, 76 insertions(+)

Hi Greg,

In addition to patches 2-6 I have about two dozen patches across various
subsystems that make use of this. I didn't want to spam everyone with
all of them before you've given an indication about what you think about
this patch.

The diffstat of the conversions I did is this:

	drivers/crypto/n2_core.c                     | 17 +++++++----------
	drivers/edac/mpc85xx_edac.c                  | 16 ++++++++--------
	drivers/edac/mv64x60_edac.c                  | 39 +++++++++++----------------------------
	drivers/gpio/gpio-mpc5200.c                  | 17 +++++++----------
	drivers/gpu/drm/armada/armada_drv.c          | 16 +++++++---------
	drivers/gpu/drm/exynos/exynos_drm_drv.c      | 42 ++++++++----------------------------------
	drivers/gpu/drm/omapdrm/omap_drv.c           | 24 +++++++-----------------
	drivers/gpu/host1x/dev.c                     | 17 +++++++----------
	drivers/input/misc/sparcspkr.c               | 18 +++++++-----------
	drivers/iommu/msm_iommu_dev.c                | 25 +++++++------------------
	drivers/leds/leds-sunfire.c                  | 23 +++++++----------------
	drivers/mfd/sta2x11-mfd.c                    | 36 ++++++++++--------------------------
	drivers/net/ethernet/adi/bfin_mac.c          | 14 +++++++-------
	drivers/net/ethernet/broadcom/bcm63xx_enet.c | 28 ++++++++--------------------
	drivers/net/ethernet/freescale/fec_mpc52xx.c | 22 +++++++++-------------
	drivers/net/ethernet/marvell/mv643xx_eth.c   | 19 +++++++------------
	drivers/pinctrl/pinctrl-adi2.c               | 24 ++++++++----------------
	drivers/pinctrl/pinctrl-at91.c               | 14 +++++++-------
	drivers/regulator/lp8788-ldo.c               | 16 +++++++---------
	drivers/regulator/wm831x-dcdc.c              | 31 +++++++++----------------------
	drivers/regulator/wm831x-ldo.c               | 27 ++++++++-------------------
	drivers/tty/serial/mpsc.c                    | 19 ++++++++-----------
	drivers/usb/gadget/udc/dummy_hcd.c           | 17 ++++++++---------
	drivers/video/fbdev/s3c2410fb.c              | 15 +++++++--------
	24 files changed, 186 insertions(+), 350 deletions(-)

That's not too thrilling but in many cases this fixes a potential bug if
a driver fails to register and the code doesn't properly unregister any
previously registered drivers.

Anyway, if you think this is worth merging, how would you like to go
about it? Do you want Acked-bys on all the patches and merge them
through your tree? Perhaps the easiest would be to just merge this patch
and then take the other patches through the maintainer trees after the
core patch has landed?

Thierry

> diff --git a/Documentation/driver-model/platform.txt b/Documentation/driver-model/platform.txt
> index 07795ec51cde..e80468738ba9 100644
> --- a/Documentation/driver-model/platform.txt
> +++ b/Documentation/driver-model/platform.txt
> @@ -63,6 +63,17 @@ runtime memory footprint:
>  	int platform_driver_probe(struct platform_driver *drv,
>  			  int (*probe)(struct platform_device *))
>  
> +Kernel modules can be composed of several platform drivers. The platform core
> +provides helpers to register and unregister an array of drivers:
> +
> +	int platform_register_drivers(struct platform_driver * const *drivers,
> +				      unsigned int count);
> +	void platform_unregister_drivers(struct platform_driver * const *drivers,
> +					 unsigned int count);
> +
> +If one of the drivers fails to register, all drivers registered up to that
> +point will be unregistered in reverse order.
> +
>  
>  Device Enumeration
>  ~~~~~~~~~~~~~~~~~~
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index f80aaaf9f610..b7d7987fda97 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -711,6 +711,66 @@ err_out:
>  }
>  EXPORT_SYMBOL_GPL(__platform_create_bundle);
>  
> +/**
> + * platform_register_drivers - register an array of platform drivers
> + * @drivers: an array of drivers to register
> + * @count: the number of drivers to register
> + *
> + * Registers platform drivers specified by an array. On failure to register a
> + * driver, all previously registered drivers will be unregistered. Callers of
> + * this API should use platform_unregister_drivers() to unregister drivers in
> + * the reverse order.
> + *
> + * Returns: 0 on success or a negative error code on failure.
> + */
> +int platform_register_drivers(struct platform_driver * const *drivers,
> +			      unsigned int count)
> +{
> +	unsigned int i;
> +	int err;
> +
> +	for (i = 0; i < count; i++) {
> +		pr_debug("registering platform driver %ps\n", drivers[i]);
> +
> +		err = platform_driver_register(drivers[i]);
> +		if (err < 0) {
> +			pr_err("failed to register platform driver %ps: %d\n",
> +			       drivers[i], err);
> +			goto error;
> +		}
> +	}
> +
> +	return 0;
> +
> +error:
> +	while (i--) {
> +		pr_debug("unregistering platform driver %ps\n", drivers[i]);
> +		platform_driver_unregister(drivers[i]);
> +	}
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(platform_register_drivers);
> +
> +/**
> + * platform_unregister_drivers - unregister an array of platform drivers
> + * @drivers: an array of drivers to unregister
> + * @count: the number of drivers to unregister
> + *
> + * Unegisters platform drivers specified by an array. This is typically used
> + * to complement an earlier call to platform_register_drivers(). Drivers are
> + * unregistered in the reverse order in which they were registered.
> + */
> +void platform_unregister_drivers(struct platform_driver * const *drivers,
> +				 unsigned int count)
> +{
> +	while (count--) {
> +		pr_debug("unregistering platform driver %ps\n", drivers[count]);
> +		platform_driver_unregister(drivers[count]);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(platform_unregister_drivers);
> +
>  /* modalias support enables more hands-off userspace setup:
>   * (a) environment variable lets new-style hotplug events work once system is
>   *     fully running:  "modprobe $MODALIAS"
> diff --git a/include/linux/platform_device.h b/include/linux/platform_device.h
> index bba08f44cc97..0c9f16bfdd99 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -270,6 +270,11 @@ extern struct platform_device *__platform_create_bundle(
>  	struct resource *res, unsigned int n_res,
>  	const void *data, size_t size, struct module *module);
>  
> +int platform_register_drivers(struct platform_driver * const *drivers,
> +			      unsigned int count);
> +void platform_unregister_drivers(struct platform_driver * const *drivers,
> +				 unsigned int count);
> +
>  /* early platform driver interface */
>  struct early_platform_driver {
>  	const char *class_str;
> -- 
> 2.5.0
>

Attachment: signature.asc
Description: PGP signature