diff mbox

[PATCHv5,3/5] mfd: motorola-cpcap: Add audio-codec support

Message ID 20180223200254.25685-4-sebastian.reichel@collabora.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Sebastian Reichel Feb. 23, 2018, 8:02 p.m. UTC
From: Sebastian Reichel <sre@kernel.org>

Add support for the audio-codec node by converting from
devm_of_platform_populate() to devm_mfd_add_devices().

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
---
 drivers/mfd/motorola-cpcap.c | 51 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

Comments

Lee Jones March 7, 2018, 4:32 p.m. UTC | #1
On Fri, 23 Feb 2018, Sebastian Reichel wrote:

> From: Sebastian Reichel <sre@kernel.org>
> 
> Add support for the audio-codec node by converting from
> devm_of_platform_populate() to devm_mfd_add_devices().
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk>
> ---
>  drivers/mfd/motorola-cpcap.c | 51 +++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
> index d2cc1eabac05..f6c79a4ccb55 100644
> --- a/drivers/mfd/motorola-cpcap.c
> +++ b/drivers/mfd/motorola-cpcap.c
> @@ -18,6 +18,7 @@
>  #include <linux/regmap.h>
>  #include <linux/sysfs.h>
>  
> +#include <linux/mfd/core.h>
>  #include <linux/mfd/motorola-cpcap.h>
>  #include <linux/spi/spi.h>
>  
> @@ -216,6 +217,53 @@ static const struct regmap_config cpcap_regmap_config = {
>  	.val_format_endian = REGMAP_ENDIAN_LITTLE,
>  };
>  
> +static const struct mfd_cell cpcap_mfd_devices[] = {
> +	{
> +		.name          = "cpcap_adc",
> +		.of_compatible = "motorola,mapphone-cpcap-adc",
> +	}, {
> +		.name          = "cpcap_battery",
> +		.of_compatible = "motorola,cpcap-battery",
> +	}, {
> +		.name          = "cpcap-charger",
> +		.of_compatible = "motorola,mapphone-cpcap-charger",
> +	}, {
> +		.name          = "cpcap-regulator",
> +		.of_compatible = "motorola,mapphone-cpcap-regulator",
> +	}, {
> +		.name          = "cpcap-rtc",
> +		.of_compatible = "motorola,cpcap-rtc",
> +	}, {
> +		.name          = "cpcap-pwrbutton",
> +		.of_compatible = "motorola,cpcap-pwrbutton",
> +	}, {
> +		.name          = "cpcap-usb-phy",
> +		.of_compatible = "motorola,mapphone-cpcap-usb-phy",
> +	}, {
> +		.name          = "cpcap-led",
> +		.id            = 0,
> +		.of_compatible = "motorola,cpcap-led-red",
> +	}, {
> +		.name          = "cpcap-led",
> +		.id            = 1,
> +		.of_compatible = "motorola,cpcap-led-green",
> +	}, {
> +		.name          = "cpcap-led",
> +		.id            = 2,
> +		.of_compatible = "motorola,cpcap-led-blue",
> +	}, {
> +		.name          = "cpcap-led",
> +		.id            = 3,
> +		.of_compatible = "motorola,cpcap-led-adl",
> +	}, {
> +		.name          = "cpcap-led",
> +		.id            = 4,
> +		.of_compatible = "motorola,cpcap-led-cp",
> +	}, {
> +		.name          = "cpcap-codec",
> +	}
> +};

With none of the entries containing platform_data /me wonders why you
can't still use devm_of_platform_populate()?

>  static int cpcap_probe(struct spi_device *spi)
>  {
>  	const struct of_device_id *match;
> @@ -260,7 +308,8 @@ static int cpcap_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	return devm_of_platform_populate(&cpcap->spi->dev);
> +	return devm_mfd_add_devices(&spi->dev, 0, cpcap_mfd_devices,
> +				    ARRAY_SIZE(cpcap_mfd_devices), NULL, 0, NULL);
>  }
>  
>  static struct spi_driver cpcap_driver = {
Sebastian Reichel March 8, 2018, 9:46 a.m. UTC | #2
Hi Lee,

On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > +static const struct mfd_cell cpcap_mfd_devices[] = {

[...]

> > +	}, {
> > +		.name          = "cpcap-led",
> > +		.id            = 4,
> > +		.of_compatible = "motorola,cpcap-led-cp",
> > +	}, {
> > +		.name          = "cpcap-codec",
> > +	}
> > +};
> 
> With none of the entries containing platform_data /me wonders why you
> can't still use devm_of_platform_populate()?

Because devm_of_platform_populate works with compatible properties and
cpcap-codec does not have one after I removed it for Mark.

-- Sebastian

> >  static int cpcap_probe(struct spi_device *spi)
> >  {
> >  	const struct of_device_id *match;
> > @@ -260,7 +308,8 @@ static int cpcap_probe(struct spi_device *spi)
> >  	if (ret)
> >  		return ret;
> >  
> > -	return devm_of_platform_populate(&cpcap->spi->dev);
> > +	return devm_mfd_add_devices(&spi->dev, 0, cpcap_mfd_devices,
> > +				    ARRAY_SIZE(cpcap_mfd_devices), NULL, 0, NULL);
> >  }
> >  
> >  static struct spi_driver cpcap_driver = {
> 
> -- 
> Lee Jones
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
Lee Jones March 8, 2018, 9:53 a.m. UTC | #3
On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> 
> [...]
> 
> > > +	}, {
> > > +		.name          = "cpcap-led",
> > > +		.id            = 4,
> > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > +	}, {
> > > +		.name          = "cpcap-codec",
> > > +	}
> > > +};
> > 
> > With none of the entries containing platform_data /me wonders why you
> > can't still use devm_of_platform_populate()?
> 
> Because devm_of_platform_populate works with compatible properties and
> cpcap-codec does not have one after I removed it for Mark.

Sorry, I missed that conversation.  Why was it removed?

> > >  static int cpcap_probe(struct spi_device *spi)
> > >  {
> > >  	const struct of_device_id *match;
> > > @@ -260,7 +308,8 @@ static int cpcap_probe(struct spi_device *spi)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	return devm_of_platform_populate(&cpcap->spi->dev);
> > > +	return devm_mfd_add_devices(&spi->dev, 0, cpcap_mfd_devices,
> > > +				    ARRAY_SIZE(cpcap_mfd_devices), NULL, 0, NULL);
> > >  }
> > >  
> > >  static struct spi_driver cpcap_driver = {
> >
Sebastian Reichel March 8, 2018, 10:27 a.m. UTC | #4
Hi,

On Thu, Mar 08, 2018 at 09:53:15AM +0000, Lee Jones wrote:
> On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> > On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> > 
> > [...]
> > 
> > > > +	}, {
> > > > +		.name          = "cpcap-led",
> > > > +		.id            = 4,
> > > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > > +	}, {
> > > > +		.name          = "cpcap-codec",
> > > > +	}
> > > > +};
> > > 
> > > With none of the entries containing platform_data /me wonders why you
> > > can't still use devm_of_platform_populate()?
> > 
> > Because devm_of_platform_populate works with compatible properties and
> > cpcap-codec does not have one after I removed it for Mark.
> 
> Sorry, I missed that conversation.  Why was it removed?

I had it in PATCHv1-PATCHv4. It was removed, since Mark didn't want
to have it in the DT ABI.

-- Sebastian

> > > >  static int cpcap_probe(struct spi_device *spi)
> > > >  {
> > > >  	const struct of_device_id *match;
> > > > @@ -260,7 +308,8 @@ static int cpcap_probe(struct spi_device *spi)
> > > >  	if (ret)
> > > >  		return ret;
> > > >  
> > > > -	return devm_of_platform_populate(&cpcap->spi->dev);
> > > > +	return devm_mfd_add_devices(&spi->dev, 0, cpcap_mfd_devices,
> > > > +				    ARRAY_SIZE(cpcap_mfd_devices), NULL, 0, NULL);
> > > >  }
> > > >  
> > > >  static struct spi_driver cpcap_driver = {
> > > 
> 
> 
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
Lee Jones March 8, 2018, 10:48 a.m. UTC | #5
On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> On Thu, Mar 08, 2018 at 09:53:15AM +0000, Lee Jones wrote:
> > On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> > > On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > > > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> > > 
> > > [...]
> > > 
> > > > > +	}, {
> > > > > +		.name          = "cpcap-led",
> > > > > +		.id            = 4,
> > > > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > > > +	}, {
> > > > > +		.name          = "cpcap-codec",
> > > > > +	}
> > > > > +};
> > > > 
> > > > With none of the entries containing platform_data /me wonders why you
> > > > can't still use devm_of_platform_populate()?
> > > 
> > > Because devm_of_platform_populate works with compatible properties and
> > > cpcap-codec does not have one after I removed it for Mark.
> > 
> > Sorry, I missed that conversation.  Why was it removed?
> 
> I had it in PATCHv1-PATCHv4. It was removed, since Mark didn't want
> to have it in the DT ABI.

Right, but why?  Is it not a hardware device?  I think converting from
devm_of_platform_populate() for one sub-device is a bit drastic.
Mark Brown March 8, 2018, 11:25 a.m. UTC | #6
On Thu, Mar 08, 2018 at 10:48:31AM +0000, Lee Jones wrote:
> On Thu, 08 Mar 2018, Sebastian Reichel wrote:

> > I had it in PATCHv1-PATCHv4. It was removed, since Mark didn't want
> > to have it in the DT ABI.

> Right, but why?  Is it not a hardware device?  I think converting from
> devm_of_platform_populate() for one sub-device is a bit drastic.

It's not a separate physical device or IP and doesn't exist outside of
the MFD, it's just how Linux is currently choosing to divide up the chip
right now but that's totally open to change even in future versions of
Linux.  Clocks are a big issue with audio stuff, right now sections of
the clock tree get handled in the CODEC driver but we're going to want
to push them out to a clock driver so we're not reimplementing handling
for clocks.
Sebastian Reichel March 8, 2018, 12:54 p.m. UTC | #7
Hi,

On Thu, Mar 08, 2018 at 10:48:31AM +0000, Lee Jones wrote:
> On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> > On Thu, Mar 08, 2018 at 09:53:15AM +0000, Lee Jones wrote:
> > > On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> > > > On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > > > > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > > > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> > > > 
> > > > [...]
> > > > 
> > > > > > +	}, {
> > > > > > +		.name          = "cpcap-led",
> > > > > > +		.id            = 4,
> > > > > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > > > > +	}, {
> > > > > > +		.name          = "cpcap-codec",
> > > > > > +	}
> > > > > > +};
> > > > > 
> > > > > With none of the entries containing platform_data /me wonders why you
> > > > > can't still use devm_of_platform_populate()?
> > > > 
> > > > Because devm_of_platform_populate works with compatible properties and
> > > > cpcap-codec does not have one after I removed it for Mark.
> > > 
> > > Sorry, I missed that conversation.  Why was it removed?
> > 
> > I had it in PATCHv1-PATCHv4. It was removed, since Mark didn't want
> > to have it in the DT ABI.
> 
> Right, but why?  Is it not a hardware device?  I think converting from
> devm_of_platform_populate() for one sub-device is a bit drastic.

This must be answered by Mark. Personally I think it makes more sense
to have the compatible, since all other cpcap sub-devices have them
and it should be consistent IMHO. I changed it to avoid bikeshedding.

The previous discussion was here: https://patchwork.kernel.org/patch/10220035/

-- Sebastian
Tony Lindgren March 8, 2018, 5:07 p.m. UTC | #8
* Sebastian Reichel <sebastian.reichel@collabora.co.uk> [180308 09:47]:
> Hi Lee,
> 
> On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> 
> [...]
> 
> > > +	}, {
> > > +		.name          = "cpcap-led",
> > > +		.id            = 4,
> > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > +	}, {
> > > +		.name          = "cpcap-codec",
> > > +	}
> > > +};
> > 
> > With none of the entries containing platform_data /me wonders why you
> > can't still use devm_of_platform_populate()?
> 
> Because devm_of_platform_populate works with compatible properties and
> cpcap-codec does not have one after I removed it for Mark.

How about keep devm_of_platform_populate() for the ones that
already have compatible. Then add a table entry for cpcap-codec
only and call devm_mfd_add_devices()?

Regards,

Tony
Lee Jones March 9, 2018, 8:34 a.m. UTC | #9
On Thu, 08 Mar 2018, Mark Brown wrote:

> On Thu, Mar 08, 2018 at 10:48:31AM +0000, Lee Jones wrote:
> > On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> 
> > > I had it in PATCHv1-PATCHv4. It was removed, since Mark didn't want
> > > to have it in the DT ABI.
> 
> > Right, but why?  Is it not a hardware device?  I think converting from
> > devm_of_platform_populate() for one sub-device is a bit drastic.
> 
> It's not a separate physical device or IP and doesn't exist outside of
> the MFD, it's just how Linux is currently choosing to divide up the chip
> right now but that's totally open to change even in future versions of
> Linux.  Clocks are a big issue with audio stuff, right now sections of
> the clock tree get handled in the CODEC driver but we're going to want
> to push them out to a clock driver so we're not reimplementing handling
> for clocks.

How is the CODEC controlled?  Does it have its own registers?  I guess
by "it's not a separate device or IP" you mean that it doesn't.  But
that begs the question, how does this then device differ from all the
other devices (adc, battery, charger, regulator, rtc, pwrbutton,
usb-phy and led)?

I'm asking, not to be awkward, but to avoid 50 lines of potentially
unnecessary static code.  If this is a real (sub-)device, even if it's
part of a larger, single device (MFD) then there is no reason why it
can't be represented as a single, albeit empty node.
Sebastian Reichel March 9, 2018, 11:19 a.m. UTC | #10
On Fri, Mar 09, 2018 at 08:34:14AM +0000, Lee Jones wrote:
> On Thu, 08 Mar 2018, Mark Brown wrote:
> 
> > On Thu, Mar 08, 2018 at 10:48:31AM +0000, Lee Jones wrote:
> > > On Thu, 08 Mar 2018, Sebastian Reichel wrote:
> > 
> > > > I had it in PATCHv1-PATCHv4. It was removed, since Mark didn't want
> > > > to have it in the DT ABI.
> > 
> > > Right, but why?  Is it not a hardware device?  I think converting from
> > > devm_of_platform_populate() for one sub-device is a bit drastic.
> > 
> > It's not a separate physical device or IP and doesn't exist outside of
> > the MFD, it's just how Linux is currently choosing to divide up the chip
> > right now but that's totally open to change even in future versions of
> > Linux.  Clocks are a big issue with audio stuff, right now sections of
> > the clock tree get handled in the CODEC driver but we're going to want
> > to push them out to a clock driver so we're not reimplementing handling
> > for clocks.
> 
> How is the CODEC controlled?  Does it have its own registers?  I guess
> by "it's not a separate device or IP" you mean that it doesn't.  But
> that begs the question, how does this then device differ from all the
> other devices (adc, battery, charger, regulator, rtc, pwrbutton,
> usb-phy and led)?

The audio codec goes from register 0x0800 - 0x0844, with the next
register being defined @ 0x0a00. So it definetly has its own register
range. I should note though, that 0x0800 is the audio regulator
control. In Linux we handle this one in the regulator driver instead
of in the audio driver. But this is mostly hidden in DT (the voltage
ranges for VAUDIO are described in the regulator node now).

> I'm asking, not to be awkward, but to avoid 50 lines of potentially
> unnecessary static code.  If this is a real (sub-)device, even if it's
> part of a larger, single device (MFD) then there is no reason why it
> can't be represented as a single, albeit empty node.

We still have the node, so that it can be used with the ASoC graph
binding. It just has no compatible value. Instead it is identified
by the node's name.

-- Sebastian
Sebastian Reichel March 9, 2018, 11:29 a.m. UTC | #11
Hi,

On Thu, Mar 08, 2018 at 09:07:36AM -0800, Tony Lindgren wrote:
> * Sebastian Reichel <sebastian.reichel@collabora.co.uk> [180308 09:47]:
> > Hi Lee,
> > 
> > On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> > 
> > [...]
> > 
> > > > +	}, {
> > > > +		.name          = "cpcap-led",
> > > > +		.id            = 4,
> > > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > > +	}, {
> > > > +		.name          = "cpcap-codec",
> > > > +	}
> > > > +};
> > > 
> > > With none of the entries containing platform_data /me wonders why you
> > > can't still use devm_of_platform_populate()?
> > 
> > Because devm_of_platform_populate works with compatible properties and
> > cpcap-codec does not have one after I removed it for Mark.
> 
> How about keep devm_of_platform_populate() for the ones that
> already have compatible. Then add a table entry for cpcap-codec
> only and call devm_mfd_add_devices()?

This should work. I think it makes sense to wait for Rob Herring's
feedback on the binding. My understanding is, that he has the last
word on binding questions and I would like to avoid changing this
back and forth.

-- Sebastian
Mark Brown March 9, 2018, 12:40 p.m. UTC | #12
On Fri, Mar 09, 2018 at 08:34:14AM +0000, Lee Jones wrote:
> On Thu, 08 Mar 2018, Mark Brown wrote:

> > Linux.  Clocks are a big issue with audio stuff, right now sections of
> > the clock tree get handled in the CODEC driver but we're going to want
> > to push them out to a clock driver so we're not reimplementing handling
> > for clocks.

> How is the CODEC controlled?  Does it have its own registers?  I guess
> by "it's not a separate device or IP" you mean that it doesn't.  But
> that begs the question, how does this then device differ from all the
> other devices (adc, battery, charger, regulator, rtc, pwrbutton,
> usb-phy and led)?

I'm not convinced that this is a particularly good idea for the other
functions but anyway...  the big thing here is that in these devices the
CODEC is generally not the level that the IP is created at, it's a
collection of interlinked IPs which usually includes not only audio
stuff but also some clocking stuff.  The repeatable blocks that could
get reused independently are generally a level down from the CODEC
level, for example if you look at something like wm8994 there's a couple
of identical FLL IPs which could make sense to enumerate individually in
the DT.  The top level generally doesn't get reused and is purely an
encoding of what Linux is currently doing.

Regulators have a bit of this going on as well - you can see it in the
wm831x series of drivers where the devices have a bunch of consistently
defined regulators that are laid out in various configurations so we
have one platform device per physical regulator (not sure if that ever
got converted to DT).

Most of the other things you list are more at the level of the FLL where
it's a single thing doing a single task that is likely to be repeated
somewhere else as a block so there's more sense, though how often that
actually happens can be questionable.

> I'm asking, not to be awkward, but to avoid 50 lines of potentially
> unnecessary static code.  If this is a real (sub-)device, even if it's
> part of a larger, single device (MFD) then there is no reason why it
> can't be represented as a single, albeit empty node.

It never used to be that complicated to define MFD functions?  It was a
data table and then a function call to register the table en masse.
Certainly not anything it's worth defining an ABI to avoid.
Tony Lindgren March 9, 2018, 3:11 p.m. UTC | #13
* Mark Brown <broonie@kernel.org> [180309 12:41]:
> On Fri, Mar 09, 2018 at 08:34:14AM +0000, Lee Jones wrote:
> > On Thu, 08 Mar 2018, Mark Brown wrote:
> 
> > > Linux.  Clocks are a big issue with audio stuff, right now sections of
> > > the clock tree get handled in the CODEC driver but we're going to want
> > > to push them out to a clock driver so we're not reimplementing handling
> > > for clocks.
> 
> > How is the CODEC controlled?  Does it have its own registers?  I guess
> > by "it's not a separate device or IP" you mean that it doesn't.  But
> > that begs the question, how does this then device differ from all the
> > other devices (adc, battery, charger, regulator, rtc, pwrbutton,
> > usb-phy and led)?
> 
> I'm not convinced that this is a particularly good idea for the other
> functions but anyway...  the big thing here is that in these devices the
> CODEC is generally not the level that the IP is created at, it's a
> collection of interlinked IPs which usually includes not only audio
> stuff but also some clocking stuff.  The repeatable blocks that could
> get reused independently are generally a level down from the CODEC
> level, for example if you look at something like wm8994 there's a couple
> of identical FLL IPs which could make sense to enumerate individually in
> the DT.  The top level generally doesn't get reused and is purely an
> encoding of what Linux is currently doing.

It seems that most of the components in the PMICs are just standard
components packed into the PMIC with a control interface provided
over I2C or SPI.

So using compatible for things like ADC, RTC and so on makes sense
if eventually figure out which ones are shared across various
drivers.

Sounds like audio is a bit more fuzzy like Mark describes above.
And by not using a compatible for audio we can have things working
while not establishing and ABI for something that might change
in the future.

Regards,

Tony
Sebastian Reichel March 9, 2018, 4:48 p.m. UTC | #14
Hi,

On Fri, Mar 09, 2018 at 07:11:53AM -0800, Tony Lindgren wrote:
> * Mark Brown <broonie@kernel.org> [180309 12:41]:
> > On Fri, Mar 09, 2018 at 08:34:14AM +0000, Lee Jones wrote:
> > > On Thu, 08 Mar 2018, Mark Brown wrote:
> > 
> > > > Linux.  Clocks are a big issue with audio stuff, right now sections of
> > > > the clock tree get handled in the CODEC driver but we're going to want
> > > > to push them out to a clock driver so we're not reimplementing handling
> > > > for clocks.
> > 
> > > How is the CODEC controlled?  Does it have its own registers?  I guess
> > > by "it's not a separate device or IP" you mean that it doesn't.  But
> > > that begs the question, how does this then device differ from all the
> > > other devices (adc, battery, charger, regulator, rtc, pwrbutton,
> > > usb-phy and led)?
> > 
> > I'm not convinced that this is a particularly good idea for the other
> > functions but anyway...  the big thing here is that in these devices the
> > CODEC is generally not the level that the IP is created at, it's a
> > collection of interlinked IPs which usually includes not only audio
> > stuff but also some clocking stuff.  The repeatable blocks that could
> > get reused independently are generally a level down from the CODEC
> > level, for example if you look at something like wm8994 there's a couple
> > of identical FLL IPs which could make sense to enumerate individually in
> > the DT.  The top level generally doesn't get reused and is purely an
> > encoding of what Linux is currently doing.
> 
> It seems that most of the components in the PMICs are just standard
> components packed into the PMIC with a control interface provided
> over I2C or SPI.
> 
> So using compatible for things like ADC, RTC and so on makes sense
> if eventually figure out which ones are shared across various
> drivers.
> 
> Sounds like audio is a bit more fuzzy like Mark describes above.
> And by not using a compatible for audio we can have things working
> while not establishing and ABI for something that might change
> in the future.

I would agree, if there was no ABI now, but we still have the
ABI. Instead of specifying the compatible property, we now need
to specify what the node name should look like (see my binding
update). That's just a different ABI.

-- Sebastian
Lee Jones March 12, 2018, 9:08 a.m. UTC | #15
On Fri, 09 Mar 2018, Sebastian Reichel wrote:

> Hi,
> 
> On Thu, Mar 08, 2018 at 09:07:36AM -0800, Tony Lindgren wrote:
> > * Sebastian Reichel <sebastian.reichel@collabora.co.uk> [180308 09:47]:
> > > Hi Lee,
> > > 
> > > On Wed, Mar 07, 2018 at 04:32:11PM +0000, Lee Jones wrote:
> > > > On Fri, 23 Feb 2018, Sebastian Reichel wrote:
> > > > > +static const struct mfd_cell cpcap_mfd_devices[] = {
> > > 
> > > [...]
> > > 
> > > > > +	}, {
> > > > > +		.name          = "cpcap-led",
> > > > > +		.id            = 4,
> > > > > +		.of_compatible = "motorola,cpcap-led-cp",
> > > > > +	}, {
> > > > > +		.name          = "cpcap-codec",
> > > > > +	}
> > > > > +};
> > > > 
> > > > With none of the entries containing platform_data /me wonders why you
> > > > can't still use devm_of_platform_populate()?
> > > 
> > > Because devm_of_platform_populate works with compatible properties and
> > > cpcap-codec does not have one after I removed it for Mark.
> > 
> > How about keep devm_of_platform_populate() for the ones that
> > already have compatible. Then add a table entry for cpcap-codec
> > only and call devm_mfd_add_devices()?
> 
> This should work. I think it makes sense to wait for Rob Herring's
> feedback on the binding. My understanding is, that he has the last
> word on binding questions and I would like to avoid changing this
> back and forth.

I've been avoiding mix-and-matching mfd_*() and of_platform_*() APIs,
else it gets too confusing for new developers.

*If* you make the switch to mfd_*() APIs (not preferable), then I
would like to see of_platform_*() removed.
diff mbox

Patch

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
index d2cc1eabac05..f6c79a4ccb55 100644
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -18,6 +18,7 @@ 
 #include <linux/regmap.h>
 #include <linux/sysfs.h>
 
+#include <linux/mfd/core.h>
 #include <linux/mfd/motorola-cpcap.h>
 #include <linux/spi/spi.h>
 
@@ -216,6 +217,53 @@  static const struct regmap_config cpcap_regmap_config = {
 	.val_format_endian = REGMAP_ENDIAN_LITTLE,
 };
 
+static const struct mfd_cell cpcap_mfd_devices[] = {
+	{
+		.name          = "cpcap_adc",
+		.of_compatible = "motorola,mapphone-cpcap-adc",
+	}, {
+		.name          = "cpcap_battery",
+		.of_compatible = "motorola,cpcap-battery",
+	}, {
+		.name          = "cpcap-charger",
+		.of_compatible = "motorola,mapphone-cpcap-charger",
+	}, {
+		.name          = "cpcap-regulator",
+		.of_compatible = "motorola,mapphone-cpcap-regulator",
+	}, {
+		.name          = "cpcap-rtc",
+		.of_compatible = "motorola,cpcap-rtc",
+	}, {
+		.name          = "cpcap-pwrbutton",
+		.of_compatible = "motorola,cpcap-pwrbutton",
+	}, {
+		.name          = "cpcap-usb-phy",
+		.of_compatible = "motorola,mapphone-cpcap-usb-phy",
+	}, {
+		.name          = "cpcap-led",
+		.id            = 0,
+		.of_compatible = "motorola,cpcap-led-red",
+	}, {
+		.name          = "cpcap-led",
+		.id            = 1,
+		.of_compatible = "motorola,cpcap-led-green",
+	}, {
+		.name          = "cpcap-led",
+		.id            = 2,
+		.of_compatible = "motorola,cpcap-led-blue",
+	}, {
+		.name          = "cpcap-led",
+		.id            = 3,
+		.of_compatible = "motorola,cpcap-led-adl",
+	}, {
+		.name          = "cpcap-led",
+		.id            = 4,
+		.of_compatible = "motorola,cpcap-led-cp",
+	}, {
+		.name          = "cpcap-codec",
+	}
+};
+
 static int cpcap_probe(struct spi_device *spi)
 {
 	const struct of_device_id *match;
@@ -260,7 +308,8 @@  static int cpcap_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	return devm_of_platform_populate(&cpcap->spi->dev);
+	return devm_mfd_add_devices(&spi->dev, 0, cpcap_mfd_devices,
+				    ARRAY_SIZE(cpcap_mfd_devices), NULL, 0, NULL);
 }
 
 static struct spi_driver cpcap_driver = {