Re: [PATCH 07/19] timberdale: mfd_cell is now implicitly available to drivers

[Andres Salomon <dilinger@queued.net>]

On Fri, 1 Apr 2011 13:20:31 +0200
Samuel Ortiz <sameo@linux.intel.com> wrote:

> Hi Grant,
> 
> On Thu, Mar 31, 2011 at 05:05:22PM -0600, Grant Likely wrote:
[...]
> > Gah.  Not all devices instantiated via mfd will be an mfd device,
> > which means that the driver may very well expect an *entirely
> > different* platform_device pointer; which further means a very high
> > potential of incorrectly dereferenced structures (as evidenced by a
> > patch series that is not bisectable).  For instance, the xilinx ip
> > cores are used by more than just mfd.
> I agree. Since the vast majority of the MFD subdevices are MFD
> specific IPs, I overlooked that part. The impacted drivers are the
> timberdale and the DaVinci voice codec ones.

Can you please provide pointers to what you're referring to?  The only
code that I could find that created platform devices prefixed with
'timb-' or named 'xilinx_spi' was drivers/mfd/timberdale.c.



> To fix that problem I propose 2 alternatives:
> 
> 1) When declaring the sub devices cells, the MFD driver should
> specify an mfd_data_size value for sub devices that are not MFD
> specific. It's the MFD driver responsibility to set the cell
> properly, and the non MFD specific drivers are kept MFD agnostic.
> See my patch below for the timberdale case.
> 
> 2) Revert the mfd_get_data() call for getting sub devices platform
> data pointers. That was introduced to ease the MFD cell sharing work,
> so if we take this route we'll need the cs5535 MFD driver to pass its
> cells as platform_data pointer. Andres, can you confirm that this
> would be fine for the mfd_clone_cell() routine to keep working ?

It would break mfd_clone_cell, as it uses mfd_get_cell to grab the one
to clone.  We could change it to accept the cell as an argument.  It
would also break mfd_cell_enable/disable, of course.



> 
> Patch for solution 1:
> 
> 
>  drivers/mfd/mfd-core.c          |   13 ++++++++++---
>  drivers/mfd/timberdale.c        |   11 +++++++++++
>  include/linux/mfd/core.h        |    1 +
>  drivers/i2c/busses/i2c-ocores.c |    3 +--
>  drivers/i2c/busses/i2c-xiic.c   |    3 +--
>  drivers/net/ks8842.c            |    3 +--
>  drivers/spi/xilinx_spi.c        |    3 +--
>  7 files changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
> index d01574d..8abe510 100644
> --- a/drivers/mfd/mfd-core.c
> +++ b/drivers/mfd/mfd-core.c
> @@ -75,9 +75,16 @@ static int mfd_add_device(struct device *parent,
> int id, 
>  	pdev->dev.parent = parent;
>  
> -	ret = platform_device_add_data(pdev, cell, sizeof(*cell));
> -	if (ret)
> -		goto fail_res;
> +	if (cell->mfd_data_size > 0) {
> +		ret = platform_device_add_data(pdev,
> +					cell->mfd_data,
> cell->mfd_data_size);
> +		if (ret)
> +			goto fail_res;
> +	} else {
> +		ret = platform_device_add_data(pdev, cell,
> sizeof(*cell));
> +		if (ret)
> +			goto fail_res;
> +	}
>  
>  	for (r = 0; r < cell->num_resources; r++) {
>  		res[r].name = cell->resources[r].name;