Re: [RFC] [PATCH] mfd: Fetch cell pointer from platform_device->mfd_cell

[Andres Salomon <dilinger@queued.net>]

This looks fine to me; just some minor points below.

On
Fri, 8 Apr 2011 02:40:09 +0200 Samuel Ortiz <sameo@linux.intel.com>
wrote:

> 
> In order for MFD drivers to fetch their cell pointer but also their
> platform data one, an mfd cell pointer is t aed to the
> platform_device structure.
> That allows all MFD sub devices drivers to be MFD agnostic, unless
> they really need to access their MFD cell data. Most of them don't,
> especially the ones for IPs used by both MFD and non MFD SoCs.
> 
> Cc: Greg KH <gregkh@suse.de>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Signed-off-by: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  drivers/base/platform.c         |    1 +
>  drivers/mfd/mfd-core.c          |   16 ++++++++++++++--
>  include/linux/mfd/core.h        |    8 ++++++--
>  include/linux/platform_device.h |    5 +++++
>  4 files changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index f051cff..6c3a2bd 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -149,6 +149,7 @@ static void platform_device_release(struct device
> *dev) 
>  	of_device_node_put(&pa->pdev.dev);
>  	kfree(pa->pdev.dev.platform_data);
> +	kfree(pa->pdev.mfd_cell);

Hm, given that most platform devices won't be mfd devices (and thus
mfd_cell will be NULL), is it better to rely on kfree's
unlikely(ZERO_OR_NULL_PTR(...)), or have this be "if
(pa->pdev.mfd_cell) kfree(pa->pdev.mfd_cell);"?

>  	kfree(pa->pdev.resource);
>  	kfree(pa);
>  }
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index d01574d..f4c8c84 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -55,6 +55,19 @@ int mfd_cell_disable(struct platform_device *pdev)
>  }
>  EXPORT_SYMBOL(mfd_cell_disable);
>  
> +static int mfd_platform_add_cell(struct platform_device *pdev,
> +				 const struct mfd_cell *cell)
> +{
> +	if (!cell)
> +		return 0;
> +
> +	pdev->mfd_cell = kmemdup(cell, sizeof(*cell), GFP_KERNEL);
> +	if (!pdev->mfd_cell)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
>  static int mfd_add_device(struct device *parent, int id,
>  			  const struct mfd_cell *cell,
>  			  struct resource *mem_base,
> @@ -75,7 +88,7 @@ static int mfd_add_device(struct device *parent,
> int id, 
>  	pdev->dev.parent = parent;
>  
> -	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
> +	ret = mfd_platform_add_cell(pdev, cell);
>  	if (ret)
>  		goto fail_res;
>  
> @@ -123,7 +136,6 @@ static int mfd_add_device(struct device *parent,
> int id, 
>  	return 0;
>  
> -/*	platform_device_del(pdev); */
>  fail_res:
>  	kfree(res);
>  fail_device:
> diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
> index ad1b19a..ee4731b 100644
> --- a/include/linux/mfd/core.h
> +++ b/include/linux/mfd/core.h
> @@ -86,16 +86,20 @@ extern int mfd_clone_cell(const char *cell, const
> char **clones, */
>  static inline const struct mfd_cell *mfd_get_cell(struct
> platform_device *pdev) {
> -	return pdev->dev.platform_data;
> +	return pdev->mfd_cell;
>  }
>  
>  /*
>   * Given a platform device that's been created by mfd_add_devices(),
> fetch
>   * the .mfd_data entry from the mfd_cell that created it.
> + * Otherwise just return the platform_data pointer.

I'd also suggest describing why we fall back to
platform_data; to the casual reader, it would be confusing.  Perhaps
something to the effect of, "This maintains compatibility with
platform drivers whose devices aren't created by the mfd layer, and
expect platform_data to contain what would've otherwise been in
mfd_data."

>   */
>  static inline void *mfd_get_data(struct platform_device *pdev)
>  {
> -	return mfd_get_cell(pdev)->mfd_data;
> +	if (pdev->mfd_cell)
> +		return mfd_get_cell(pdev)->mfd_data;
> +	else
> +		return pdev->dev.platform_data;

Not much point checking pdev->mfd_cell and then using an abstraction.
I'd just do "const struct mfd_cell *cell = mfd_get_cell(pdev); if
(cell) return cell->mfd_data;" or "if (pdev->mfd_cell) return
pdev->mfd_cell->mfd_data;"


>  }
>  
>  extern int mfd_add_devices(struct device *parent, int id,
> diff --git a/include/linux/platform_device.h
> b/include/linux/platform_device.h index d96db98..744942c 100644
> --- a/include/linux/platform_device.h
> +++ b/include/linux/platform_device.h
> @@ -14,6 +14,8 @@
>  #include <linux/device.h>
>  #include <linux/mod_devicetable.h>
>  
> +struct mfd_cell;
> +
>  struct platform_device {
>  	const char	* name;
>  	int		id;
> @@ -23,6 +25,9 @@ struct platform_device {
>  
>  	const struct platform_device_id	*id_entry;
>  
> +	/* MFD cell pointer */
> +	struct mfd_cell *mfd_cell;
> +
>  	/* arch specific additions */
>  	struct pdev_archdata	archdata;
>  };