| Message ID | 20201126165950.2554997-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] ALSA: ppc: drop if block with always false condition | expand |
Hi Uwe, On Thu, Nov 26, 2020 at 6:03 PM Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > The remove callback is only called for devices that were probed > successfully before. As the matching probe function cannot complete > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to > check this here. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for your patch! Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> Note that there are similar checks in snd_ps3_driver_probe(), which can be removed, too: if (WARN_ON(!firmware_has_feature(FW_FEATURE_PS3_LV1))) return -ENODEV; if (WARN_ON(dev->match_id != PS3_MATCH_ID_SOUND)) return -ENODEV; Gr{oetje,eeting}s, Geert
On Fri, Nov 27, 2020 at 09:35:39AM +0100, Geert Uytterhoeven wrote: > Hi Uwe, > > On Thu, Nov 26, 2020 at 6:03 PM Uwe Kleine-König > <u.kleine-koenig@pengutronix.de> wrote: > > The remove callback is only called for devices that were probed > > successfully before. As the matching probe function cannot complete > > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to > > check this here. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > Thanks for your patch! > > Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org> > > Note that there are similar checks in snd_ps3_driver_probe(), which > can be removed, too: > > if (WARN_ON(!firmware_has_feature(FW_FEATURE_PS3_LV1))) > return -ENODEV; > if (WARN_ON(dev->match_id != PS3_MATCH_ID_SOUND)) > return -ENODEV; I had to invest some brain cycles here. For the first: Assuming firmware_has_feature(FW_FEATURE_PS3_LV1) always returns the same value, snd_ps3_driver_probe is only used after this check succeeds because the driver is registered only after this check in snd_ps3_init(). The second is superflous because ps3_system_bus_match() yields false if this doesn't match the driver's match_id. Best regards Uwe
Hallo Leonard, On Fri, Nov 27, 2020 at 04:22:59PM +0100, Leonard Goehrs wrote: > The check for the FW_FEATURE_PS3_LV1 firmware feature is already performed > in ps3_system_bus_init() before registering the driver. So if the probe > function is actually used, this feature is already known to be available. > > The check for the match id is also superfluous; the condition is always > true because the bus' match function (ps3_system_bus_match()) only > considers this driver for devices having: > dev->match_id == snd_ps3_bus_driver_info.match_id. > > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Leonard Goehrs <l.goehrs@pengutronix.de> Reviewed-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Thanks for picking this up. Best regards and have a nice week-end, Uwe Kleine-König
Hi Uwe, On 11/26/20 8:59 AM, Uwe Kleine-König wrote: > The remove callback is only called for devices that were probed > successfully before. As the matching probe function cannot complete > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to > check this here. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I tested your two patches plus Leonard's patch 'ALSA: ppc: remove redundant checks in PS3 driver probe' applied to v5.9 on the PS3, and they seem to work fine. Thanks for both your efforts. Tested by: Geoff Levand <geoff@infradead.org>
On 11/27/20 7:22 AM, Leonard Goehrs wrote: > The check for the FW_FEATURE_PS3_LV1 firmware feature is already performed > in ps3_system_bus_init() before registering the driver. So if the probe > function is actually used, this feature is already known to be available. > > The check for the match id is also superfluous; the condition is always > true because the bus' match function (ps3_system_bus_match()) only > considers this driver for devices having: > dev->match_id == snd_ps3_bus_driver_info.match_id. > > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Leonard Goehrs <l.goehrs@pengutronix.de> Seems OK with v5.9 on PS3. Tested by: Geoff Levand <geoff@infradead.org>
On Thu, 26 Nov 2020 17:59:49 +0100, Uwe Kleine-König wrote: > > The remove callback is only called for devices that were probed > successfully before. As the matching probe function cannot complete > without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to > check this here. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Applied this one now. Thanks. Takashi
On Fri, 27 Nov 2020 16:22:59 +0100, Leonard Goehrs wrote: > > The check for the FW_FEATURE_PS3_LV1 firmware feature is already performed > in ps3_system_bus_init() before registering the driver. So if the probe > function is actually used, this feature is already known to be available. > > The check for the match id is also superfluous; the condition is always > true because the bus' match function (ps3_system_bus_match()) only > considers this driver for devices having: > dev->match_id == snd_ps3_bus_driver_info.match_id. > > Suggested-by: Geert Uytterhoeven <geert@linux-m68k.org> > Signed-off-by: Leonard Goehrs <l.goehrs@pengutronix.de> Applied now. Thanks. Takashi
diff --git a/sound/ppc/snd_ps3.c b/sound/ppc/snd_ps3.c index 58bb49fff184..6ab796a5d936 100644 --- a/sound/ppc/snd_ps3.c +++ b/sound/ppc/snd_ps3.c @@ -1053,8 +1053,6 @@ static int snd_ps3_driver_remove(struct ps3_system_bus_device *dev) { int ret; pr_info("%s:start id=%d\n", __func__, dev->match_id); - if (dev->match_id != PS3_MATCH_ID_SOUND) - return -ENXIO; /* * ctl and preallocate buffer will be freed in
The remove callback is only called for devices that were probed successfully before. As the matching probe function cannot complete without error if dev->match_id != PS3_MATCH_ID_SOUND, we don't have to check this here. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- sound/ppc/snd_ps3.c | 2 -- 1 file changed, 2 deletions(-)