| Message ID | 20180831180507.28867-1-afd@ti.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ASoC: tlv320aic31xx: Add MICBIAS off setting | expand |
On Fri, Aug 31, 2018 at 01:05:07PM -0500, Andrew F. Davis wrote: > Leaving microphone bias off is a valid setting and even used in the DT > binding document example. Add this setting here and document the same. > Although it may not make much sense to enable a microphone here without > any bias, it is a valid setting that can be chosen by DT and may be > needed for some boards. If we really want to pay attention to something setting this up we'd need to completely remove the widget - what the code is doing at the minute is setting the voltage that the bias will go to when enabled, there's still a widget for turning it on and off. There's some chance that this will break existing boards.
On 09/03/2018 06:26 AM, Mark Brown wrote: > On Fri, Aug 31, 2018 at 01:05:07PM -0500, Andrew F. Davis wrote: >> Leaving microphone bias off is a valid setting and even used in the DT >> binding document example. Add this setting here and document the same. >> Although it may not make much sense to enable a microphone here without >> any bias, it is a valid setting that can be chosen by DT and may be >> needed for some boards. > > If we really want to pay attention to something setting this up we'd > need to completely remove the widget - what the code is doing at the > minute is setting the voltage that the bias will go to when enabled, > there's still a widget for turning it on and off. There's some chance > that this will break existing boards. > Turning on bias is controlled separately, automatically in user-space in many cases, not based on the board. The DT needs to have a way to state that 0v is the needed bias on this board, without this you can not set 0v bias and 2v is chosen by default (which is IMHO should be 0v but that would change existing behavior so I won't touch that).
On Tue, Sep 04, 2018 at 08:55:06AM -0500, Andrew F. Davis wrote: > On 09/03/2018 06:26 AM, Mark Brown wrote: > > If we really want to pay attention to something setting this up we'd > > need to completely remove the widget - what the code is doing at the > > minute is setting the voltage that the bias will go to when enabled, > > there's still a widget for turning it on and off. There's some chance > > that this will break existing boards. > Turning on bias is controlled separately, automatically in user-space in > many cases, not based on the board. The DT needs to have a way to state > that 0v is the needed bias on this board, without this you can not set > 0v bias and 2v is chosen by default (which is IMHO should be 0v but that > would change existing behavior so I won't touch that). Surely turning on MICBIAS at 0V is equivalent to leaving the bias off?
On 09/04/2018 09:41 AM, Mark Brown wrote: > On Tue, Sep 04, 2018 at 08:55:06AM -0500, Andrew F. Davis wrote: >> On 09/03/2018 06:26 AM, Mark Brown wrote: > >>> If we really want to pay attention to something setting this up we'd >>> need to completely remove the widget - what the code is doing at the >>> minute is setting the voltage that the bias will go to when enabled, >>> there's still a widget for turning it on and off. There's some chance >>> that this will break existing boards. > >> Turning on bias is controlled separately, automatically in user-space in >> many cases, not based on the board. The DT needs to have a way to state >> that 0v is the needed bias on this board, without this you can not set >> 0v bias and 2v is chosen by default (which is IMHO should be 0v but that >> would change existing behavior so I won't touch that). > > Surely turning on MICBIAS at 0V is equivalent to leaving the bias off? > It is, but DT doesn't control when or why MICBIAS is turned on or off, only what voltage on should be.
On Tue, Sep 04, 2018 at 09:43:03AM -0500, Andrew F. Davis wrote: > On 09/04/2018 09:41 AM, Mark Brown wrote: > >>> If we really want to pay attention to something setting this up we'd > >>> need to completely remove the widget - what the code is doing at the > >>> minute is setting the voltage that the bias will go to when enabled, > >>> there's still a widget for turning it on and off. There's some chance > >>> that this will break existing boards. > >> Turning on bias is controlled separately, automatically in user-space in > >> many cases, not based on the board. The DT needs to have a way to state > >> that 0v is the needed bias on this board, without this you can not set > >> 0v bias and 2v is chosen by default (which is IMHO should be 0v but that > >> would change existing behavior so I won't touch that). > > Surely turning on MICBIAS at 0V is equivalent to leaving the bias off? > It is, but DT doesn't control when or why MICBIAS is turned on or off, > only what voltage on should be. The DT controls this in so far as it will arrange for the bias to be connected to something, if it's not connected to anything then it will only be turned on if the machine driver explicitly forces it on which is a potential source of problems but seems comfortably in "you broke it, you get to keep the pieces" territory. If we really want to have a way of explicitly specifying that some widgets should never be turned on then it feels like rather than have something device and widget specific like this we should instead have a higher level way of doing that which can be applied to any widget, that is something that could be useful especially with things like speaker drivers where there's real potential for physical damage to the system. Like I said in my original reply I'm also worried that this will break existing boards by causing them to change to a voltage of 0 when they had managed to end up with the default of 2V which happened to work for them.
On 09/04/2018 09:55 AM, Mark Brown wrote: > On Tue, Sep 04, 2018 at 09:43:03AM -0500, Andrew F. Davis wrote: >> On 09/04/2018 09:41 AM, Mark Brown wrote: > >>>>> If we really want to pay attention to something setting this up we'd >>>>> need to completely remove the widget - what the code is doing at the >>>>> minute is setting the voltage that the bias will go to when enabled, >>>>> there's still a widget for turning it on and off. There's some chance >>>>> that this will break existing boards. > >>>> Turning on bias is controlled separately, automatically in user-space in >>>> many cases, not based on the board. The DT needs to have a way to state >>>> that 0v is the needed bias on this board, without this you can not set >>>> 0v bias and 2v is chosen by default (which is IMHO should be 0v but that >>>> would change existing behavior so I won't touch that). > >>> Surely turning on MICBIAS at 0V is equivalent to leaving the bias off? > >> It is, but DT doesn't control when or why MICBIAS is turned on or off, >> only what voltage on should be. > > The DT controls this in so far as it will arrange for the bias to be > connected to something, if it's not connected to anything then it will > only be turned on if the machine driver explicitly forces it on which is > a potential source of problems but seems comfortably in "you broke it, > you get to keep the pieces" territory. If we really want to have a way > of explicitly specifying that some widgets should never be turned on > then it feels like rather than have something device and widget specific > like this we should instead have a higher level way of doing that which > can be applied to any widget, that is something that could be useful > especially with things like speaker drivers where there's real potential > for physical damage to the system. > Not sure how that could even look, it would need to be in DT as only that layer knows the connections, but then the it would also have to have internal knowledge of the widgets used in the driver.. > Like I said in my original reply I'm also worried that this will break > existing boards by causing them to change to a voltage of 0 when they > had managed to end up with the default of 2V which happened to work for > them. > Then only time this could happen is if they specified MICBIAS_OFF in DT but got the default 2v instead. In which case, yes, the behavior would change, but it would also be changing to the correct behavior. We can't avoid fixing code on the off chance someone depended on the broken behavior, no progress could ever be made.
On Tue, Sep 04, 2018 at 10:10:24AM -0500, Andrew F. Davis wrote: > On 09/04/2018 09:55 AM, Mark Brown wrote: > > you get to keep the pieces" territory. If we really want to have a way > > of explicitly specifying that some widgets should never be turned on > > then it feels like rather than have something device and widget specific > > like this we should instead have a higher level way of doing that which > > can be applied to any widget, that is something that could be useful > > especially with things like speaker drivers where there's real potential > > for physical damage to the system. > Not sure how that could even look, it would need to be in DT as only > that layer knows the connections, but then the it would also have to > have internal knowledge of the widgets used in the driver.. We already have a bunch of DT code that knows about and uses the external widgets the device has (which are I'd guess the only ones where this is an issue)? I was thinking just have a property that lists the widgets that should never be turned on. > > Like I said in my original reply I'm also worried that this will break > > existing boards by causing them to change to a voltage of 0 when they > > had managed to end up with the default of 2V which happened to work for > > them. > Then only time this could happen is if they specified MICBIAS_OFF in DT > but got the default 2v instead. In which case, yes, the behavior would > change, but it would also be changing to the correct behavior. We can't > avoid fixing code on the off chance someone depended on the broken > behavior, no progress could ever be made. I'm really having a lot of trouble seeing MICBIAS_OFF as a useful voltage to specify in DT in the first place.
On 09/04/2018 10:56 AM, Mark Brown wrote: > On Tue, Sep 04, 2018 at 10:10:24AM -0500, Andrew F. Davis wrote: >> On 09/04/2018 09:55 AM, Mark Brown wrote: > >>> you get to keep the pieces" territory. If we really want to have a way >>> of explicitly specifying that some widgets should never be turned on >>> then it feels like rather than have something device and widget specific >>> like this we should instead have a higher level way of doing that which >>> can be applied to any widget, that is something that could be useful >>> especially with things like speaker drivers where there's real potential >>> for physical damage to the system. > >> Not sure how that could even look, it would need to be in DT as only >> that layer knows the connections, but then the it would also have to >> have internal knowledge of the widgets used in the driver.. > > We already have a bunch of DT code that knows about and uses the > external widgets the device has (which are I'd guess the only ones where > this is an issue)? I was thinking just have a property that lists the > widgets that should never be turned on. > >>> Like I said in my original reply I'm also worried that this will break >>> existing boards by causing them to change to a voltage of 0 when they >>> had managed to end up with the default of 2V which happened to work for >>> them. > >> Then only time this could happen is if they specified MICBIAS_OFF in DT >> but got the default 2v instead. In which case, yes, the behavior would >> change, but it would also be changing to the correct behavior. We can't >> avoid fixing code on the off chance someone depended on the broken >> behavior, no progress could ever be made. > > I'm really having a lot of trouble seeing MICBIAS_OFF as a useful > voltage to specify in DT in the first place. > I don't see the usefulness in specifying any bias voltage in DT at all, it is a configuration and can be made at runtime, it has no place in DT. But it is already here, so lets allow all available voltages a board may need and the CODEC can supply, even 0.
On Tue, Sep 04, 2018 at 11:02:14AM -0500, Andrew F. Davis wrote: > On 09/04/2018 10:56 AM, Mark Brown wrote: > > I'm really having a lot of trouble seeing MICBIAS_OFF as a useful > > voltage to specify in DT in the first place. > I don't see the usefulness in specifying any bias voltage in DT at all, > it is a configuration and can be made at runtime, it has no place in DT. > But it is already here, so lets allow all available voltages a board may > need and the CODEC can supply, even 0. It is very rare for it to be useful to select the bias voltage at runtime - it's usually something that's decided by the electrical engineering at system design time and linked to selection of passive components rather than something that a user could usefully vary. I suspect in the situations where it is useful to vary it you'd want a layer of indirection mapping it onto some user observable behaviour (or the system should just do this autonomously as with the various low power mic detect solutions out there). What situations are you aware of where runtime configuration is useful?
diff --git a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt index 5b3c33bb99e5..411cc46a2c58 100644 --- a/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt +++ b/Documentation/devicetree/bindings/sound/tlv320aic31xx.txt @@ -24,6 +24,7 @@ Optional properties: - reset-gpios - GPIO specification for the active low RESET input. - ai31xx-micbias-vg - MicBias Voltage setting + 0 or MICBIAS_OFF - MICBIAS output is powered off 1 or MICBIAS_2_0V - MICBIAS output is powered to 2.0V 2 or MICBIAS_2_5V - MICBIAS output is powered to 2.5V 3 or MICBIAS_AVDD - MICBIAS output is connected to AVDD diff --git a/include/dt-bindings/sound/tlv320aic31xx-micbias.h b/include/dt-bindings/sound/tlv320aic31xx-micbias.h index c6895a18a455..069484070fcf 100644 --- a/include/dt-bindings/sound/tlv320aic31xx-micbias.h +++ b/include/dt-bindings/sound/tlv320aic31xx-micbias.h @@ -2,6 +2,7 @@ #ifndef __DT_TLV320AIC31XX_MICBIAS_H #define __DT_TLV320AIC31XX_MICBIAS_H +#define MICBIAS_OFF 0 #define MICBIAS_2_0V 1 #define MICBIAS_2_5V 2 #define MICBIAS_AVDDV 3 diff --git a/sound/soc/codecs/tlv320aic31xx.c b/sound/soc/codecs/tlv320aic31xx.c index bf92d36b8f8a..7d87df518fed 100644 --- a/sound/soc/codecs/tlv320aic31xx.c +++ b/sound/soc/codecs/tlv320aic31xx.c @@ -1421,6 +1421,7 @@ static int aic31xx_i2c_probe(struct i2c_client *i2c, fwnode_property_read_u32(aic31xx->dev->fwnode, "ai31xx-micbias-vg", &micbias_value); switch (micbias_value) { + case MICBIAS_OFF: case MICBIAS_2_0V: case MICBIAS_2_5V: case MICBIAS_AVDDV: