On Mon, 2019-09-30 at 16:11 +0100, Mark Brown wrote:
On Mon, Sep 30, 2019 at 09:44:00AM +0000, Sa, Nuno wrote:
On Thu, 2019-09-26 at 11:43 -0700, Mark Brown wrote:
- case SND_SOC_DAIFMT_RIGHT_J:
st->right_j = true;break;Don't we need to set any register values here?
The register set is done in adau7118_hw_params(). For right justification the device can delay bclck by 8, 12 or 16. So, We need to know the data_width to check if we can apply the configuration.
OK.
- case SND_SOC_BIAS_STANDBY:
if (snd_soc_component_get_bias_level(component)==
SND_SOC_BIAS_OF F) {
if (!st->iovdd)return 0;This is broken, the device will always require power so it should always control the regulators.
The reason why I made this optional was to let the user assume that, in some cases, the supply can be always present (and not controlled by the kernel) and, in those cases, he would not have to care about giving regulators nodes in devicetree. Furthermore, the driver would not have
Have you tried doing that? Notice how the regulator API subtitutes in a dummy regulator for you and the driver works fine without custom code.
Ok, got it. Looking at `_regulator_get` I can see that a dummy regulator is provided, when `NORMAL_GET` is used, even if one was not given. So I think I get now the usage of `devm_regulator_get_optional`. As you said, supply voltages are not optional, the optional _get_ should be used for things that are really optional like some parts that can use external vs internal vref.
to care about enabling/disabling regulators. Is this not a valid scenario? Or is it that, for this kind of devices it does not really
It's not a valid scenario in driver code - the driver shouldn't be randomly ignoring errors and hoping the errors were deliberate rather than indiciations of real problems.
+static int adau7118_resume(struct snd_soc_component *component) +{
- return snd_soc_component_force_bias_level(component,
SND_SOC_BIAS_STANDBY) ; +}
Let DAPM do this for you, there's no substantial delays on power on so you're probably best just setting idle_bias_off.
So, this means dropping resume/suspend and to not set idle_bias_on, right?
Right. Like I say it looks like your power up path is fast enough for this.
+static int adau7118_regulator_setup(struct adau7118_data *st) +{
- int ret = 0;
- st->iovdd = devm_regulator_get_optional(st->dev,
"IOVDD");
- if (!IS_ERR(st->iovdd)) {
Unless the device can operate with supplies physically absent it should not be requesting regulators as optional, this breaks your error handling especially with probe deferral which is a fairly common case.
Just for my understanding (most likely I'm missing something obvious), why would I have issues with the error handling in probe deferral?
Actually it does look like you handle this correctly further down, the normal pattern would have been to have the error handling inside the if here and not indent the rest of the success path so I misread it.