| Message ID | 20220610015105.25987-1-yung-chuan.liao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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 >
On 23-06-22, 09:54, Pierre-Louis Bossart wrote: > > > 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. Sorry am bit confused now, this is what I have in sdw-next static int sdw_drv_remove(struct device *dev) { struct sdw_slave *slave = dev_to_sdw_dev(dev); struct sdw_driver *drv = drv_to_sdw_driver(dev->driver); int ret = 0; if (drv->remove) ret = drv->remove(slave); dev_pm_domain_detach(dev, false); return ret; } I see that dev_pm_domain_detach() is called unconditionally and not dependent on remove method which seems right to me. The code seems same since 9251345dca24b
On 04-07-22, 11:13, Vinod Koul wrote: > On 23-06-22, 09:54, Pierre-Louis Bossart wrote: > I see that dev_pm_domain_detach() is called unconditionally and not > dependent on remove method which seems right to me. > > The code seems same since 9251345dca24b And I overlooked the setting of method, which is correct, sorry for the noise. Have applied this now, thanks
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); }