| Message ID | 20220610015105.25987-1-yung-chuan.liao@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | soundwire: bus_type: fix remove and shutdown support | expand |
On 10-06-22, 09:51, Bard Liao wrote: > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > The bus sdw_drv_remove() and sdw_drv_shutdown() helpers are used > conditionally, if the driver provides these routines. > > These helpers already test if the driver provides a .remove or > .shutdown callback, so there's no harm in invoking the > sdw_drv_remove() and sdw_drv_shutdown() unconditionally. Okay sounds good > In addition, the current code is imbalanced with > dev_pm_domain_attach() called from sdw_drv_probe(), but > dev_pm_domain_detach() called from sdw_drv_remove() only if the driver > provides a .remove callback. Am not sure I follow what is imbalance, pm_domain_attach/detach? > Fixes: 9251345dca24b ("soundwire: Add SoundWire bus type") How is this a fix is still unclear to me. At best I think this is code optimization and removing checks which maybe redundant. > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > Reviewed-by: Rander Wang <rander.wang@intel.com> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> > --- > drivers/soundwire/bus_type.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c > index 893296f3fe39..b81e04dd3a9f 100644 > --- a/drivers/soundwire/bus_type.c > +++ b/drivers/soundwire/bus_type.c > @@ -193,12 +193,8 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner) > > drv->driver.owner = owner; > drv->driver.probe = sdw_drv_probe; > - > - if (drv->remove) > - drv->driver.remove = sdw_drv_remove; > - > - if (drv->shutdown) > - drv->driver.shutdown = sdw_drv_shutdown; > + drv->driver.remove = sdw_drv_remove; > + drv->driver.shutdown = sdw_drv_shutdown; > > return driver_register(&drv->driver); > } > -- > 2.17.1
On 6/23/22 02:55, Vinod Koul wrote: > On 10-06-22, 09:51, Bard Liao wrote: >> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> >> The bus sdw_drv_remove() and sdw_drv_shutdown() helpers are used >> conditionally, if the driver provides these routines. >> >> These helpers already test if the driver provides a .remove or >> .shutdown callback, so there's no harm in invoking the >> sdw_drv_remove() and sdw_drv_shutdown() unconditionally. > > Okay sounds good > >> In addition, the current code is imbalanced with >> dev_pm_domain_attach() called from sdw_drv_probe(), but >> dev_pm_domain_detach() called from sdw_drv_remove() only if the driver >> provides a .remove callback. > > Am not sure I follow what is imbalance, pm_domain_attach/detach? Yes, the dev_pm_domain_detach() is done conditionally, depending on the presence of a driver .remove callback, that's not so good. >> Fixes: 9251345dca24b ("soundwire: Add SoundWire bus type") > > How is this a fix is still unclear to me. At best I think this is code > optimization and removing checks which maybe redundant. I think the Fixes tag is appropriate for the dev_pm_domain_detach() problem, but if you want to drop it that's fine with me. >> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> Reviewed-by: Rander Wang <rander.wang@intel.com> >> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com> >> --- >> drivers/soundwire/bus_type.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c >> index 893296f3fe39..b81e04dd3a9f 100644 >> --- a/drivers/soundwire/bus_type.c >> +++ b/drivers/soundwire/bus_type.c >> @@ -193,12 +193,8 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner) >> >> drv->driver.owner = owner; >> drv->driver.probe = sdw_drv_probe; >> - >> - if (drv->remove) >> - drv->driver.remove = sdw_drv_remove; >> - >> - if (drv->shutdown) >> - drv->driver.shutdown = sdw_drv_shutdown; >> + drv->driver.remove = sdw_drv_remove; >> + drv->driver.shutdown = sdw_drv_shutdown; >> >> return driver_register(&drv->driver); >> } >> -- >> 2.17.1 >
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c index 893296f3fe39..b81e04dd3a9f 100644 --- a/drivers/soundwire/bus_type.c +++ b/drivers/soundwire/bus_type.c @@ -193,12 +193,8 @@ int __sdw_register_driver(struct sdw_driver *drv, struct module *owner) drv->driver.owner = owner; drv->driver.probe = sdw_drv_probe; - - if (drv->remove) - drv->driver.remove = sdw_drv_remove; - - if (drv->shutdown) - drv->driver.shutdown = sdw_drv_shutdown; + drv->driver.remove = sdw_drv_remove; + drv->driver.shutdown = sdw_drv_shutdown; return driver_register(&drv->driver); }