Re: [alsa-devel] [PATCH] ASoC: add RT5640 CODEC driver
On Tue, Apr 16, 2013 at 04:29:40PM -0600, Stephen Warren wrote:
a) There are many different VDD inputs to the chip. I imagine that some of these only need to be enabled/active under certain conditions. For example, perhaps the SPKVDD or MICVDD inputs are only required if actively using the speakers or microphones, and could be disabled at other times? If so, all of these "optional" or "part-time" VDD inputs should be represented as regulators, and retrieved/manipulated using Linux APIs such as regulator_get(), regulator_enable(), etc.
There's framework support for regulator supplies in ASoC so you just need to specify the name and the core will do the rest.
b) Some VDD inputs are optional; for example, either I believe that a board should either provide DCVDD, or provide LDO1_IN and assert the LDO1_EN input signal. Similarly, I think that either MICVDD should be provided, or SPKVDDL be provided, coupled with LDO2_EN register bit being set. This may require the RT5640 driver to provide two regulator objects (LDO1, LDO2), which the board file or device tree can connect back to the DCVDD and MICVDD inputs if appropriate for the board's HW configuration. In the case where LDO1 is used, a separate fixed regulator with GPIO should be used to control the LDO1_EN GPIO input.
That said if boards generally don't use external supplies and use the built in regulators then it's probably best to at least have the driver assume that by default.
Bard: You are right. The mic type should be defined according to circuit. And it should be change in run time. So, should we just remove this control in codec driver?
Mark's point is that the driver should either/both accept some platform data structure, or parse device tree, to configure the input pins. I don't think completely removing all mechanisms to configure this would be a good idea, since then how would the board/device-tree author configure the CODEC?
Indeed, there definitely should be configuration - however what I was commenting on there was a user-visible ALSA control which should definitely go.
On 04/17/2013 08:01 AM, Mark Brown wrote:
On Tue, Apr 16, 2013 at 04:29:40PM -0600, Stephen Warren wrote:
a) There are many different VDD inputs to the chip. I imagine that some of these only need to be enabled/active under certain conditions. For example, perhaps the SPKVDD or MICVDD inputs are only required if actively using the speakers or microphones, and could be disabled at other times? If so, all of these "optional" or "part-time" VDD inputs should be represented as regulators, and retrieved/manipulated using Linux APIs such as regulator_get(), regulator_enable(), etc.
There's framework support for regulator supplies in ASoC so you just need to specify the name and the core will do the rest.
b) Some VDD inputs are optional; for example, either I believe that a board should either provide DCVDD, or provide LDO1_IN and assert the LDO1_EN input signal. Similarly, I think that either MICVDD should be provided, or SPKVDDL be provided, coupled with LDO2_EN register bit being set. This may require the RT5640 driver to provide two regulator objects (LDO1, LDO2), which the board file or device tree can connect back to the DCVDD and MICVDD inputs if appropriate for the board's HW configuration. In the case where LDO1 is used, a separate fixed regulator with GPIO should be used to control the LDO1_EN GPIO input.
That said if boards generally don't use external supplies and use the built in regulators then it's probably best to at least have the driver assume that by default.
So the issue here is that regulators aren't supposed to be optional, right? So if there's a reasonable chance that regulators would ever be needed, we should add them now.
With board files, we probably could have just added them later, but with device tree (which is my use-case for this CODEC at least), the DT binding needs to specify which regulator(s) the device requires (if any) right from the start, so that all DTs will include the regulator definitions.
On Wed, Apr 17, 2013 at 09:18:30AM -0600, Stephen Warren wrote:
On 04/17/2013 08:01 AM, Mark Brown wrote:
That said if boards generally don't use external supplies and use the built in regulators then it's probably best to at least have the driver assume that by default.
So the issue here is that regulators aren't supposed to be optional, right? So if there's a reasonable chance that regulators would ever be needed, we should add them now.
With board files, we probably could have just added them later, but with device tree (which is my use-case for this CODEC at least), the DT binding needs to specify which regulator(s) the device requires (if any) right from the start, so that all DTs will include the regulator definitions.
You can do the same thing with DT as you do with board files - make those supplies an optional property and then if the property is missing do the default thing.
On 04/17/2013 09:28 AM, Mark Brown wrote:
On Wed, Apr 17, 2013 at 09:18:30AM -0600, Stephen Warren wrote:
On 04/17/2013 08:01 AM, Mark Brown wrote:
That said if boards generally don't use external supplies and use the built in regulators then it's probably best to at least have the driver assume that by default.
So the issue here is that regulators aren't supposed to be optional, right? So if there's a reasonable chance that regulators would ever be needed, we should add them now.
With board files, we probably could have just added them later, but with device tree (which is my use-case for this CODEC at least), the DT binding needs to specify which regulator(s) the device requires (if any) right from the start, so that all DTs will include the regulator definitions.
You can do the same thing with DT as you do with board files - make those supplies an optional property and then if the property is missing do the default thing.
But then, you end up with an optional regulator, and the driver has to do things like:
if (!IS_ERR(x->reg_foo)) regulator_enable(x->reg_foo);
I thought the whole point of the rule that "if a regulator is ever needed, it must always be provided, and if there isn't one on the board, use a 'dummy' fixed-regulator" was to avoid exactly that?
But if that rule is relaxed, and the code above is fine, then indeed one can do as you say.
On Wed, Apr 17, 2013 at 10:25:30AM -0600, Stephen Warren wrote:
On 04/17/2013 09:28 AM, Mark Brown wrote:
You can do the same thing with DT as you do with board files - make those supplies an optional property and then if the property is missing do the default thing.
But then, you end up with an optional regulator, and the driver has to do things like:
if (!IS_ERR(x->reg_foo)) regulator_enable(x->reg_foo);
Not if you do it at the other end - do it during device registration. If nothing is set up then feed the regulator API whatever the default configuration is for the device, otherwise use what you were given. The consumer side can't tell where the configuration came from and will always have one.
This does mean you need to do the regulator driver if you support non-default configurations but that's no bad thing.
On 04/17/2013 12:52 PM, Mark Brown wrote:
On Wed, Apr 17, 2013 at 10:25:30AM -0600, Stephen Warren wrote:
On 04/17/2013 09:28 AM, Mark Brown wrote:
You can do the same thing with DT as you do with board files - make those supplies an optional property and then if the property is missing do the default thing.
But then, you end up with an optional regulator, and the driver has to do things like:
if (!IS_ERR(x->reg_foo)) regulator_enable(x->reg_foo);
Not if you do it at the other end - do it during device registration. If nothing is set up then feed the regulator API whatever the default configuration is for the device, otherwise use what you were given. The consumer side can't tell where the configuration came from and will always have one.
This does mean you need to do the regulator driver if you support non-default configurations but that's no bad thing.
OK, so something like:
During DT parsing:
if (!dt_property_exits()) // regulator API call to set up a dummy regulator for this device
Somewhere later in the probe() path:
x->reg = regulator_get();
The one thing I may have forgotten to mention here for the LDO1 case is that if we don't explicitly support regulators right away, then instead the driver needs to explicitly request a GPIO to control the LDO1_EN input on the chip; without that, the device won't even respond to I2C accesses. And hence, that GPIO also needs to be described in device tree. The other regulators could certainly all work as you're pointing out.
On Wed, Apr 17, 2013 at 12:56:30PM -0600, Stephen Warren wrote:
On 04/17/2013 12:52 PM, Mark Brown wrote:
Not if you do it at the other end - do it during device registration. If nothing is set up then feed the regulator API whatever the default configuration is for the device, otherwise use what you were given. The consumer side can't tell where the configuration came from and will always have one.
This does mean you need to do the regulator driver if you support non-default configurations but that's no bad thing.
OK, so something like:
During DT parsing:
if (!dt_property_exits()) // regulator API call to set up a dummy regulator for this device
You should probably have an actual regulator for the regulator that's physically there and doing things but yes. If (as one should) you're supporting platform data as well then the DT code should just work with no extra effort.
The WM8994 code I posted recently does all this, the regulator side is in -next though Samuel didn't pick up the DT stuff yet and non-default configurations just aren't supported except with platform data since I am not aware of any such designs.
The one thing I may have forgotten to mention here for the LDO1 case is that if we don't explicitly support regulators right away, then instead the driver needs to explicitly request a GPIO to control the LDO1_EN input on the chip; without that, the device won't even respond to I2C accesses. And hence, that GPIO also needs to be described in device tree. The other regulators could certainly all work as you're pointing out.
That's expected, yes - you can just define a property for the GPIO and move it over to being handled by a proper regulator driver when you write one.
participants (3)
-
Mark Brown -
Mark Brown -
Stephen Warren