| Message ID | 20220707132613.3150931-3-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 0de876c125188e502d2899de4bcba8d4a6b1f98c |
| Headers | show |
| Series | [1/3] ASoC: amd: remove unused header file inclusion | expand |
Le 07/07/2022 à 15:26, Vijendar Mukunda a écrit : > Fix below kernel warning. >>>> sound/soc/amd/acp-es8336.c:200:13: warning: variable 'ret' set but >>>> not used [-Wunused-but-set-variable] > Hi, this patch, looks odd to me. > Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > sound/soc/amd/acp-es8336.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/sound/soc/amd/acp-es8336.c b/sound/soc/amd/acp-es8336.c > index 90f4e5809c72..e1479ae684e9 100644 > --- a/sound/soc/amd/acp-es8336.c > +++ b/sound/soc/amd/acp-es8336.c > @@ -206,6 +206,8 @@ static int st_es8336_late_probe(struct snd_soc_card *card) > dev_err(card->dev, "can not find codec dev\n"); The next devm_acpi_dev_add_driver_gpios() will fail, should we return immediately? > > ret = devm_acpi_dev_add_driver_gpios(codec_dev, acpi_es8336_gpios); > + if (ret) > + dev_warn(card->dev, "Failed to add driver gpios\n"); Should we return immediately? > > gpio_pa = gpiod_get_optional(codec_dev, "pa-enable", GPIOD_OUT_LOW); > if (IS_ERR(gpio_pa)) { > @@ -213,6 +215,7 @@ static int st_es8336_late_probe(struct snd_soc_card *card) > "could not get pa-enable GPIO\n"); > gpiod_put(gpio_pa); > put_device(codec_dev); > + return ret; Here, we return 'ret' which is what is returned by devm_acpi_dev_add_driver_gpios(). So there doesn't seem to be a link with this block which checks for gpiod_get_optional() errors. Moreover, returning an error for something that is optional (gpiod_get_optional()) looks strange. Finally, should an error be returned, maybe PTR_ERR(gpio_pa) would be better here. Just my 2c. CJ > } > return 0; > }
On 7/9/22 1:03 PM, Christophe JAILLET wrote: > Le 07/07/2022 à 15:26, Vijendar Mukunda a écrit : >> Fix below kernel warning. >>>>> sound/soc/amd/acp-es8336.c:200:13: warning: variable 'ret' set but >>>>> not used [-Wunused-but-set-variable] >> > > Hi, > this patch, looks odd to me. > > >> Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> >> Reported-by: kernel test robot <lkp@intel.com> >> --- >> sound/soc/amd/acp-es8336.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/sound/soc/amd/acp-es8336.c b/sound/soc/amd/acp-es8336.c >> index 90f4e5809c72..e1479ae684e9 100644 >> --- a/sound/soc/amd/acp-es8336.c >> +++ b/sound/soc/amd/acp-es8336.c >> @@ -206,6 +206,8 @@ static int st_es8336_late_probe(struct >> snd_soc_card *card) >> dev_err(card->dev, "can not find codec dev\n"); > > The next devm_acpi_dev_add_driver_gpios() will fail, should we return > immediately? > >> ret = devm_acpi_dev_add_driver_gpios(codec_dev, >> acpi_es8336_gpios); >> + if (ret) >> + dev_warn(card->dev, "Failed to add driver gpios\n"); > > Should we return immediately? As it required to support Machine driver differed probe , we shouldn't return immediately. We are checking gpiod_get_optional() return status. If still error is reported, then return is invoked after checking whether return error code is -EPROBE_DEFER. We found similar implementation in other platforms machine driver code as well. >> gpio_pa = gpiod_get_optional(codec_dev, "pa-enable", >> GPIOD_OUT_LOW); >> if (IS_ERR(gpio_pa)) { >> @@ -213,6 +215,7 @@ static int st_es8336_late_probe(struct >> snd_soc_card *card) >> "could not get pa-enable GPIO\n"); >> gpiod_put(gpio_pa); >> put_device(codec_dev); >> + return ret; > > Here, we return 'ret' which is what is returned by > devm_acpi_dev_add_driver_gpios(). So there doesn't seem to be a link > with this block which checks for gpiod_get_optional() errors. > > Moreover, returning an error for something that is optional > (gpiod_get_optional()) looks strange. > > Finally, should an error be returned, maybe PTR_ERR(gpio_pa) would be > better here. Machine driver deferred probing should be supported. err code PTR_ERR(gpio_pa) is compared with -EPROBE_DIFFER and same err returned from dev_err_probe() API. same code can also be modified as below if (IS_ERR(gpio_pa)) { gpiod_put(gpio_pa); put_device(codec_dev); return dev_err_probe(card->dev, PTR_ERR(gpio_pa), "could not get pa-enable GPIO\n"); } > > > Just my 2c. > > CJ > >> } >> return 0; >> } >
This code is ok. The machine driver should still function well without gpio.
if (IS_ERR(gpio_pa)) {
gpiod_put(gpio_pa);
put_device(codec_dev);
return dev_err_probe(card->dev, PTR_ERR(gpio_pa),
"could not get pa-enable GPIO\n");
}
You donnot need to handle null gpio_pa gpio.
if (!(IS_ERR_OR_NULL(gpio_pa)))
gpiod_set_value_cansleep(gpio_pa, true);
diff --git a/sound/soc/amd/acp-es8336.c b/sound/soc/amd/acp-es8336.c index 90f4e5809c72..e1479ae684e9 100644 --- a/sound/soc/amd/acp-es8336.c +++ b/sound/soc/amd/acp-es8336.c @@ -206,6 +206,8 @@ static int st_es8336_late_probe(struct snd_soc_card *card) dev_err(card->dev, "can not find codec dev\n"); ret = devm_acpi_dev_add_driver_gpios(codec_dev, acpi_es8336_gpios); + if (ret) + dev_warn(card->dev, "Failed to add driver gpios\n"); gpio_pa = gpiod_get_optional(codec_dev, "pa-enable", GPIOD_OUT_LOW); if (IS_ERR(gpio_pa)) { @@ -213,6 +215,7 @@ static int st_es8336_late_probe(struct snd_soc_card *card) "could not get pa-enable GPIO\n"); gpiod_put(gpio_pa); put_device(codec_dev); + return ret; } return 0; }