| Message ID | 1343298534-13611-21-git-send-email-lee.jones@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 26, 2012 at 11:28:53AM +0100, Lee Jones wrote: > drivers/mfd/ab8500-core.c | 1 + > include/linux/mfd/abx500/ab8500-codec.h | 6 ++- > sound/soc/codecs/ab8500-codec.c | 79 +++++++++++++++++++++++++++++++ > 3 files changed, 84 insertions(+), 2 deletions(-) Yet again no binding documentation.... > { > .name = "ab8500-codec", > + .of_compatible = "stericsson,ab8500-codec", > }, Why are we doing this? The MFD cells are a totally Linux specific thing, there's no reason to represent them in the device tree unless they're in some way reusable and the "ab8500-codec" name suggests that's unlikely. Just put the properties on the parent node and instantiate the MFD cell as normal. > + /* Has a non-standard Vamic been requested? */ > + if(of_get_property(np, "stericsson,amic1a-bias-vamic2", NULL)) Coding style. > + if (!of_property_read_u32(np, "stericsson,earpeice-cmv", &value)) { > + switch (value) { > + case 950 : > + codec->ear_cmv = EAR_CMV_0_95V; > + break; > + case 1100 : > + codec->ear_cmv = EAR_CMV_1_10V; > + break; > + case 1270 : > + codec->ear_cmv = EAR_CMV_1_27V; > + break; > + case 1580 : > + codec->ear_cmv = EAR_CMV_1_58V; > + break; > + default : > + codec->ear_cmv = EAR_CMV_UNKNOWN; > + dev_err(dev, "Unsuitable earpiece voltage found in DT\n"); The platform data code picks a default, can't the DT code do the same?
On 26/07/12 12:50, Mark Brown wrote: > On Thu, Jul 26, 2012 at 11:28:53AM +0100, Lee Jones wrote: > >> drivers/mfd/ab8500-core.c | 1 + >> include/linux/mfd/abx500/ab8500-codec.h | 6 ++- >> sound/soc/codecs/ab8500-codec.c | 79 +++++++++++++++++++++++++++++++ >> 3 files changed, 84 insertions(+), 2 deletions(-) > > Yet again no binding documentation.... RFC. ;) I'll write the documentation when/if the properties are accepted. >> { >> .name = "ab8500-codec", >> + .of_compatible = "stericsson,ab8500-codec", >> }, > > Why are we doing this? The MFD cells are a totally Linux specific > thing, there's no reason to represent them in the device tree unless > they're in some way reusable and the "ab8500-codec" name suggests that's > unlikely. Just put the properties on the parent node and instantiate > the MFD cell as normal. > >> + /* Has a non-standard Vamic been requested? */ >> + if(of_get_property(np, "stericsson,amic1a-bias-vamic2", NULL)) > > Coding style. Missing space? Sorry, typo, I'll change. >> + if (!of_property_read_u32(np, "stericsson,earpeice-cmv", &value)) { >> + switch (value) { >> + case 950 : >> + codec->ear_cmv = EAR_CMV_0_95V; >> + break; >> + case 1100 : >> + codec->ear_cmv = EAR_CMV_1_10V; >> + break; >> + case 1270 : >> + codec->ear_cmv = EAR_CMV_1_27V; >> + break; >> + case 1580 : >> + codec->ear_cmv = EAR_CMV_1_58V; >> + break; >> + default : >> + codec->ear_cmv = EAR_CMV_UNKNOWN; >> + dev_err(dev, "Unsuitable earpiece voltage found in DT\n"); > > The platform data code picks a default, can't the DT code do the same? No, I don't think that it does? The original code returns -EINVAL unless a value is specified. The original author is keen to have a clear error message in case users try to specify non-exact values. I'd rather we fail-out than use incorrect values which would be a great deal harder for a user to debug.
Sorry missed this: >> { >> .name = "ab8500-codec", >> + .of_compatible = "stericsson,ab8500-codec", >> }, > > Why are we doing this? The MFD cells are a totally Linux specific > thing, there's no reason to represent them in the device tree unless > they're in some way reusable and the "ab8500-codec" name suggests that's > unlikely. Just put the properties on the parent node and instantiate > the MFD cell as normal. We have all of the AB8500 devices into the Device Tree to accurately represent the hardware. We will also be passing configuration information into the AB8500 Codec from Device Tree. The only reason we're still registering them using the MFD API is to overcome addressing issues encountered earlier. Each 'device' still belongs in the 'device' tree. If we were to take this Device Tree and use it on something non-Linux, that OS will still need to know about each of the AB8500 devices and every associated configuration option. Only in Linux do we continue to register them though a different API, which doesn't affect any other OS.
On Thu, Jul 26, 2012 at 03:00:17PM +0100, Lee Jones wrote: > On 26/07/12 12:50, Mark Brown wrote: > >Yet again no binding documentation.... > RFC. ;) > I'll write the documentation when/if the properties are accepted. No, write the documentation. It's way too much effort to reverse engineer the bindings from the code. > >>+ default : > >>+ codec->ear_cmv = EAR_CMV_UNKNOWN; > >>+ dev_err(dev, "Unsuitable earpiece voltage found in DT\n"); > >The platform data code picks a default, can't the DT code do the same? > No, I don't think that it does? The original code returns -EINVAL > unless a value is specified. The code doesn't specify values for the enumeration so it ought to default to EAR_CMV_0_95V if nothing is specified. > The original author is keen to have a clear error message in case > users try to specify non-exact values. I'd rather we fail-out than > use incorrect values which would be a great deal harder for a user > to debug. By that argument all the properties should be mandatory but it's only this one IIRC.
On Thu, Jul 26, 2012 at 03:15:01PM +0100, Lee Jones wrote: > Sorry missed this: > >Why are we doing this? The MFD cells are a totally Linux specific > >thing, there's no reason to represent them in the device tree unless > >they're in some way reusable and the "ab8500-codec" name suggests that's > >unlikely. Just put the properties on the parent node and instantiate > >the MFD cell as normal. > We have all of the AB8500 devices into the Device Tree to accurately > represent the hardware. We will also be passing configuration > information into the AB8500 Codec from Device Tree. The only reason > we're still registering them using the MFD API is to overcome > addressing issues encountered earlier. Each 'device' still belongs > in the 'device' tree. The device here is the AB8500. The fact that Linux chooses to represent it as an MFD with a particular set of subdrivers is a Linux specific decision and may well change over time. For example it's likely that we'll want to migrate the clocks out of the audio driver and into the clock API when that becomes useful. Similarly currently a lot of these devices use ASoC level jack detection but that's going to move over to extcon over time. There's no way you're going to be able to reuse this for anything that isn't an AB8500, there's no abstraction of the SoC integration here. If you had clearly identifiable, repeatable IPs which you could reasonably bind to a different bit of silicon then that'd be different but there's nothing like that here. We already know that the functionality covered by the driver is going to be present simply by virtue of knowing that there's an AB8500 and similarly there's no real way this driver could ever be used without the core driver. All the "device" in the device tree is doing is serving as a container to place some of the DT properties into, this needs to be separated out from the instantiation of the Linux device driver. There's nothing stopping the driver from looking at the OF node of its parent here. The goal here isn't just to copy the Linux device model and platform data into device tree bindings, the device tree bindings need to think about what the chip actually is so they can be reused by other OSs and by future versions of Linux. > If we were to take this Device Tree and use it on something > non-Linux, that OS will still need to know about each of the AB8500 > devices and every associated configuration option. Only in Linux do > we continue to register them though a different API, which doesn't > affect any other OS. Another OS might have a different idea about how it's going to split up the chip which better fits with the models which that OS has for the functions present on the device. The reason this is a distinct device in Linux is all to do with how Linux models the hardware.
On 26/07/12 15:28, Mark Brown wrote: > On Thu, Jul 26, 2012 at 03:00:17PM +0100, Lee Jones wrote: >> On 26/07/12 12:50, Mark Brown wrote: > >>> Yet again no binding documentation.... > >> RFC. ;) > >> I'll write the documentation when/if the properties are accepted. > > No, write the documentation. It's way too much effort to reverse > engineer the bindings from the code. > >>>> + default : >>>> + codec->ear_cmv = EAR_CMV_UNKNOWN; >>>> + dev_err(dev, "Unsuitable earpiece voltage found in DT\n"); > >>> The platform data code picks a default, can't the DT code do the same? > >> No, I don't think that it does? The original code returns -EINVAL >> unless a value is specified. > > The code doesn't specify values for the enumeration so it ought to > default to EAR_CMV_0_95V if nothing is specified. Ah, I see what you mean. I guess we could compromise and print a warning _and_ fall back to the 0th original emum. >> The original author is keen to have a clear error message in case >> users try to specify non-exact values. I'd rather we fail-out than >> use incorrect values which would be a great deal harder for a user >> to debug. > > By that argument all the properties should be mandatory but it's only > this one IIRC. This is the only value which the user can pick an obscure value, such as 913, thinking they can pick 913mV. I'm happy to fall-back, as long as Ola is too.
On Thu, Jul 26, 2012 at 04:01:14PM +0100, Lee Jones wrote: > This is the only value which the user can pick an obscure value, > such as 913, thinking they can pick 913mV. I'm happy to fall-back, > as long as Ola is too. Erroring out if they pick an invalid value is fine, I'm more concerned with the case where no property is supplied at all.
On 26/07/12 16:14, Mark Brown wrote: > On Thu, Jul 26, 2012 at 04:01:14PM +0100, Lee Jones wrote: > >> This is the only value which the user can pick an obscure value, >> such as 913, thinking they can pick 913mV. I'm happy to fall-back, >> as long as Ola is too. > > Erroring out if they pick an invalid value is fine, I'm more concerned > with the case where no property is supplied at all. Hmmm... I'll have a think.
On 26/07/12 15:43, Mark Brown wrote: > On Thu, Jul 26, 2012 at 03:15:01PM +0100, Lee Jones wrote: >> Sorry missed this: > >>> Why are we doing this? The MFD cells are a totally Linux specific >>> thing, there's no reason to represent them in the device tree unless >>> they're in some way reusable and the "ab8500-codec" name suggests that's >>> unlikely. Just put the properties on the parent node and instantiate >>> the MFD cell as normal. > >> We have all of the AB8500 devices into the Device Tree to accurately >> represent the hardware. We will also be passing configuration >> information into the AB8500 Codec from Device Tree. The only reason >> we're still registering them using the MFD API is to overcome >> addressing issues encountered earlier. Each 'device' still belongs >> in the 'device' tree. > > The device here is the AB8500. The fact that Linux chooses to represent > it as an MFD with a particular set of subdrivers is a Linux specific > decision and may well change over time. For example it's likely that > we'll want to migrate the clocks out of the audio driver and into the > clock API when that becomes useful. Similarly currently a lot of these > devices use ASoC level jack detection but that's going to move over to > extcon over time. > > There's no way you're going to be able to reuse this for anything that > isn't an AB8500, there's no abstraction of the SoC integration here. If > you had clearly identifiable, repeatable IPs which you could reasonably > bind to a different bit of silicon then that'd be different but there's > nothing like that here. We already know that the functionality covered > by the driver is going to be present simply by virtue of knowing that > there's an AB8500 and similarly there's no real way this driver could > ever be used without the core driver. All the "device" in the device > tree is doing is serving as a container to place some of the DT > properties into, this needs to be separated out from the instantiation > of the Linux device driver. There's nothing stopping the driver from > looking at the OF node of its parent here. > > The goal here isn't just to copy the Linux device model and platform > data into device tree bindings, the device tree bindings need to think > about what the chip actually is so they can be reused by other OSs and > by future versions of Linux. > >> If we were to take this Device Tree and use it on something >> non-Linux, that OS will still need to know about each of the AB8500 >> devices and every associated configuration option. Only in Linux do >> we continue to register them though a different API, which doesn't >> affect any other OS. > > Another OS might have a different idea about how it's going to split up > the chip which better fits with the models which that OS has for the > functions present on the device. The reason this is a distinct device > in Linux is all to do with how Linux models the hardware. Okay, so your suggestion is to strip out all of the sub-devices under the AB8500. It's doable, but will take some restructuring and thinking about. This is a job for another day. I think it's okay to continue with the current semantics for the time-being. The line we're discussing does need to be split out though. I didn't mean to merge it in with the ASoC stuff.
On Thu, Jul 26, 2012 at 04:19:33PM +0100, Lee Jones wrote: > Okay, so your suggestion is to strip out all of the sub-devices > under the AB8500. It's doable, but will take some restructuring and > thinking about. This is a job for another day. I think it's okay to > continue with the current semantics for the time-being. The line > we're discussing does need to be split out though. I didn't mean to > merge it in with the ASoC stuff. Yes, well any that aren't reusable at any rate. If you could relocate the device into another SoC integation that'd be different.
diff --git a/drivers/mfd/ab8500-core.c b/drivers/mfd/ab8500-core.c index 626b4ec..0c5b70f 100644 --- a/drivers/mfd/ab8500-core.c +++ b/drivers/mfd/ab8500-core.c @@ -1076,6 +1076,7 @@ static struct mfd_cell __devinitdata ab8500_devs[] = { }, { .name = "ab8500-codec", + .of_compatible = "stericsson,ab8500-codec", }, }; diff --git a/include/linux/mfd/abx500/ab8500-codec.h b/include/linux/mfd/abx500/ab8500-codec.h index dc65292..d707941 100644 --- a/include/linux/mfd/abx500/ab8500-codec.h +++ b/include/linux/mfd/abx500/ab8500-codec.h @@ -23,7 +23,8 @@ enum amic_type { /* Mic-biases */ enum amic_micbias { AMIC_MICBIAS_VAMIC1, - AMIC_MICBIAS_VAMIC2 + AMIC_MICBIAS_VAMIC2, + AMIC_MICBIAS_UNKNOWN }; /* Bias-voltage */ @@ -31,7 +32,8 @@ enum ear_cm_voltage { EAR_CMV_0_95V, EAR_CMV_1_10V, EAR_CMV_1_27V, - EAR_CMV_1_58V + EAR_CMV_1_58V, + EAR_CMV_UNKNOWN }; /* Analog microphone settings */ diff --git a/sound/soc/codecs/ab8500-codec.c b/sound/soc/codecs/ab8500-codec.c index 3c79592..e3ae9f5 100644 --- a/sound/soc/codecs/ab8500-codec.c +++ b/sound/soc/codecs/ab8500-codec.c @@ -34,6 +34,7 @@ #include <linux/mfd/abx500/ab8500-sysctrl.h> #include <linux/mfd/abx500/ab8500-codec.h> #include <linux/regulator/consumer.h> +#include <linux/of.h> #include <sound/core.h> #include <sound/pcm.h> @@ -2394,9 +2395,62 @@ struct snd_soc_dai_driver ab8500_codec_dai[] = { } }; +static void ab8500_codec_of_probe(struct device *dev, struct device_node *np, + struct ab8500_codec_platform_data *codec) +{ + u32 value; + + if (of_get_property(np, "stericsson,amic1-type-single-ended", NULL)) + codec->amics.mic1_type = AMIC_TYPE_SINGLE_ENDED; + else + codec->amics.mic1_type = AMIC_TYPE_DIFFERENTIAL; + + if (of_get_property(np, "stericsson,amic2-type-single-ended", NULL)) + codec->amics.mic2_type = AMIC_TYPE_SINGLE_ENDED; + else + codec->amics.mic2_type = AMIC_TYPE_DIFFERENTIAL; + + /* Has a non-standard Vamic been requested? */ + if(of_get_property(np, "stericsson,amic1a-bias-vamic2", NULL)) + codec->amics.mic1a_micbias = AMIC_MICBIAS_VAMIC2; + else + codec->amics.mic1a_micbias = AMIC_MICBIAS_VAMIC1; + + if (of_get_property(np, "stericsson,amic1b-bias-vamic2", NULL)) + codec->amics.mic1b_micbias = AMIC_MICBIAS_VAMIC2; + else + codec->amics.mic1b_micbias = AMIC_MICBIAS_VAMIC1; + + if (of_get_property(np, "stericsson,amic2-bias-vamic1", NULL)) + codec->amics.mic2_micbias = AMIC_MICBIAS_VAMIC1; + else + codec->amics.mic2_micbias = AMIC_MICBIAS_VAMIC2; + + if (!of_property_read_u32(np, "stericsson,earpeice-cmv", &value)) { + switch (value) { + case 950 : + codec->ear_cmv = EAR_CMV_0_95V; + break; + case 1100 : + codec->ear_cmv = EAR_CMV_1_10V; + break; + case 1270 : + codec->ear_cmv = EAR_CMV_1_27V; + break; + case 1580 : + codec->ear_cmv = EAR_CMV_1_58V; + break; + default : + codec->ear_cmv = EAR_CMV_UNKNOWN; + dev_err(dev, "Unsuitable earpiece voltage found in DT\n"); + } + } +} + static int ab8500_codec_probe(struct snd_soc_codec *codec) { struct device *dev = codec->dev; + struct device_node *np = dev->of_node; struct ab8500_codec_drvdata *drvdata = dev_get_drvdata(dev); struct ab8500_platform_data *pdata; struct filter_control *fc; @@ -2406,6 +2460,31 @@ static int ab8500_codec_probe(struct snd_soc_codec *codec) /* Setup AB8500 according to board-settings */ pdata = (struct ab8500_platform_data *)dev_get_platdata(dev->parent); + + if (np) { + if (!pdata) + pdata = devm_kzalloc(dev, + sizeof(struct ab8500_platform_data), + GFP_KERNEL); + + if (!pdata->codec) + pdata->codec + = devm_kzalloc(dev, + sizeof(struct ab8500_codec_platform_data), + GFP_KERNEL); + + if (!(pdata && pdata->codec)) + return -ENOMEM; + + ab8500_codec_of_probe(dev, np, pdata->codec); + + } else { + if (!(pdata && pdata->codec)) { + dev_err(dev, "No codec platform data or DT found\n"); + return -EINVAL; + } + } + status = ab8500_audio_setup_mics(codec, &pdata->codec->amics); if (status < 0) { pr_err("%s: Failed to setup mics (%d)!\n", __func__, status);
We continue to allow the AB8500 CODEC to be registered via the AB8500 Multi Functional Device API, only this time we extract its configuration from the Device Tree binary. Signed-off-by: Lee Jones <lee.jones@linaro.org> --- drivers/mfd/ab8500-core.c | 1 + include/linux/mfd/abx500/ab8500-codec.h | 6 ++- sound/soc/codecs/ab8500-codec.c | 79 +++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-)