| Message ID | 1404214378-24669-1-git-send-email-sachin.kamat@samsung.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | d5471e67229adb31a1e5f026955d006f06315f3d |
| Delegated to: | Takashi Iwai |
| Headers | show |
At Tue, 1 Jul 2014 17:02:54 +0530, Sachin Kamat wrote: > > 'status' is not used in the function. Remove it. > > Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > --- > All patches in this series only compile tested. Removing such a line is unsafe, thus it must be tested. It changes the hardware access pattern and sometimes the read is intentionally there although the value isn't used. That said, without testing, I won't apply such patches. thanks, Takashi > --- > sound/pci/hda/hda_tegra.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > index a366ba9293a8..8cd7b06eecef 100644 > --- a/sound/pci/hda/hda_tegra.c > +++ b/sound/pci/hda/hda_tegra.c > @@ -272,13 +272,9 @@ static int hda_tegra_resume(struct device *dev) > struct snd_card *card = dev_get_drvdata(dev); > struct azx *chip = card->private_data; > struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); > - int status; > > hda_tegra_enable_clocks(hda); > > - /* Read STATESTS before controller reset */ > - status = azx_readw(chip, STATESTS); > - > hda_tegra_init(hda); > > azx_init_chip(chip, 1); > -- > 1.7.9.5 >
On Tue, Jul 1, 2014 at 5:26 PM, Takashi Iwai <tiwai@suse.de> wrote: > At Tue, 1 Jul 2014 17:02:54 +0530, > Sachin Kamat wrote: >> >> 'status' is not used in the function. Remove it. >> >> Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> >> Cc: Stephen Warren <swarren@wwwdotorg.org> >> Cc: Thierry Reding <thierry.reding@gmail.com> >> --- >> All patches in this series only compile tested. > > Removing such a line is unsafe, thus it must be tested. It changes > the hardware access pattern and sometimes the read is intentionally > there although the value isn't used. > > That said, without testing, I won't apply such patches. Right. I understand. That is the reason I explicitly mentioned that patches are untested. However, in the above cases how about removing the local variable (return value) and just keeping the read/write calls? Regards, Sachin.
At Tue, 1 Jul 2014 17:33:25 +0530, Sachin Kamat wrote: > > On Tue, Jul 1, 2014 at 5:26 PM, Takashi Iwai <tiwai@suse.de> wrote: > > At Tue, 1 Jul 2014 17:02:54 +0530, > > Sachin Kamat wrote: > >> > >> 'status' is not used in the function. Remove it. > >> > >> Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> > >> Cc: Stephen Warren <swarren@wwwdotorg.org> > >> Cc: Thierry Reding <thierry.reding@gmail.com> > >> --- > >> All patches in this series only compile tested. > > > > Removing such a line is unsafe, thus it must be tested. It changes > > the hardware access pattern and sometimes the read is intentionally > > there although the value isn't used. > > > > That said, without testing, I won't apply such patches. > > Right. I understand. That is the reason I explicitly mentioned that patches > are untested. > However, in the above cases how about removing the local variable (return value) > and just keeping the read/write calls? If the call is needed, yes, at best with some comments. But until someone really tests that it's unnecessary, there is no big reason to touch the code. Takashi
On 07/01/2014 05:32 AM, Sachin Kamat wrote: > 'status' is not used in the function. Remove it. CCing Dylan Reid for comments/testing since he wrote or upstreamed this code. > Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> > Cc: Stephen Warren <swarren@wwwdotorg.org> > Cc: Thierry Reding <thierry.reding@gmail.com> > --- > All patches in this series only compile tested. > --- > sound/pci/hda/hda_tegra.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > index a366ba9293a8..8cd7b06eecef 100644 > --- a/sound/pci/hda/hda_tegra.c > +++ b/sound/pci/hda/hda_tegra.c > @@ -272,13 +272,9 @@ static int hda_tegra_resume(struct device *dev) > struct snd_card *card = dev_get_drvdata(dev); > struct azx *chip = card->private_data; > struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); > - int status; > > hda_tegra_enable_clocks(hda); > > - /* Read STATESTS before controller reset */ > - status = azx_readw(chip, STATESTS); > - > hda_tegra_init(hda); > > azx_init_chip(chip, 1); >
On Tue, Jul 1, 2014 at 8:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 07/01/2014 05:32 AM, Sachin Kamat wrote: >> 'status' is not used in the function. Remove it. > > CCing Dylan Reid for comments/testing since he wrote or upstreamed this > code. Thanks, I gave this a try on a couple of systems this morning. HDMI audio still works through suspend/resume cycles. > >> Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> >> Cc: Stephen Warren <swarren@wwwdotorg.org> Tested-by: Dylan Reid <dgreid@chromium.org> >> Cc: Thierry Reding <thierry.reding@gmail.com> >> --- >> All patches in this series only compile tested. >> --- >> sound/pci/hda/hda_tegra.c | 4 ---- >> 1 file changed, 4 deletions(-) >> >> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c >> index a366ba9293a8..8cd7b06eecef 100644 >> --- a/sound/pci/hda/hda_tegra.c >> +++ b/sound/pci/hda/hda_tegra.c >> @@ -272,13 +272,9 @@ static int hda_tegra_resume(struct device *dev) >> struct snd_card *card = dev_get_drvdata(dev); >> struct azx *chip = card->private_data; >> struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); >> - int status; >> >> hda_tegra_enable_clocks(hda); >> >> - /* Read STATESTS before controller reset */ >> - status = azx_readw(chip, STATESTS); >> - >> hda_tegra_init(hda); >> >> azx_init_chip(chip, 1); >> >
At Tue, 1 Jul 2014 08:47:39 -0700, Dylan Reid wrote: > > On Tue, Jul 1, 2014 at 8:07 AM, Stephen Warren <swarren@wwwdotorg.org> wrote: > > On 07/01/2014 05:32 AM, Sachin Kamat wrote: > >> 'status' is not used in the function. Remove it. > > > > CCing Dylan Reid for comments/testing since he wrote or upstreamed this > > code. > > Thanks, I gave this a try on a couple of systems this morning. HDMI > audio still works through suspend/resume cycles. > > > > >> Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> > >> Cc: Stephen Warren <swarren@wwwdotorg.org> > > Tested-by: Dylan Reid <dgreid@chromium.org> Great, I applied the patch now. Thanks! Takashi > > >> Cc: Thierry Reding <thierry.reding@gmail.com> > >> --- > >> All patches in this series only compile tested. > >> --- > >> sound/pci/hda/hda_tegra.c | 4 ---- > >> 1 file changed, 4 deletions(-) > >> > >> diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c > >> index a366ba9293a8..8cd7b06eecef 100644 > >> --- a/sound/pci/hda/hda_tegra.c > >> +++ b/sound/pci/hda/hda_tegra.c > >> @@ -272,13 +272,9 @@ static int hda_tegra_resume(struct device *dev) > >> struct snd_card *card = dev_get_drvdata(dev); > >> struct azx *chip = card->private_data; > >> struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); > >> - int status; > >> > >> hda_tegra_enable_clocks(hda); > >> > >> - /* Read STATESTS before controller reset */ > >> - status = azx_readw(chip, STATESTS); > >> - > >> hda_tegra_init(hda); > >> > >> azx_init_chip(chip, 1); > >> > > >
diff --git a/sound/pci/hda/hda_tegra.c b/sound/pci/hda/hda_tegra.c index a366ba9293a8..8cd7b06eecef 100644 --- a/sound/pci/hda/hda_tegra.c +++ b/sound/pci/hda/hda_tegra.c @@ -272,13 +272,9 @@ static int hda_tegra_resume(struct device *dev) struct snd_card *card = dev_get_drvdata(dev); struct azx *chip = card->private_data; struct hda_tegra *hda = container_of(chip, struct hda_tegra, chip); - int status; hda_tegra_enable_clocks(hda); - /* Read STATESTS before controller reset */ - status = azx_readw(chip, STATESTS); - hda_tegra_init(hda); azx_init_chip(chip, 1);
'status' is not used in the function. Remove it. Signed-off-by: Sachin Kamat <sachin.kamat@samsung.com> Cc: Stephen Warren <swarren@wwwdotorg.org> Cc: Thierry Reding <thierry.reding@gmail.com> --- All patches in this series only compile tested. --- sound/pci/hda/hda_tegra.c | 4 ---- 1 file changed, 4 deletions(-)