| Message ID | 20220920134819.2981033-1-yangyingliang@huawei.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | spi: Switch to use devm_spi_alloc_master() in some drivers | expand |
On Tue, Sep 20, 2022 at 09:48:13PM +0800, Yang Yingliang wrote: > This patchset is trying to replace spi_alloc_master() with > devm_spi_alloc_master() in some spi drivers. With this helper, > spi_master_put() is called in devres_release_all() whenever > the device is unbound, so the spi_master_put() in error path > can be removed. If we're switching please update to the modern naming and use "controller" rather than the old name.
Hi Mark, On 2022/9/21 2:33, Mark Brown wrote: > On Tue, Sep 20, 2022 at 09:48:13PM +0800, Yang Yingliang wrote: >> This patchset is trying to replace spi_alloc_master() with >> devm_spi_alloc_master() in some spi drivers. With this helper, >> spi_master_put() is called in devres_release_all() whenever >> the device is unbound, so the spi_master_put() in error path >> can be removed. > If we're switching please update to the modern naming and use > "controller" rather than the old name. Do you mean to use spi_controller instead of spi_master? Something like this: 'struct spi_controller * ctlr = devm_spi_alloc_master();' Dose spi_master_get_devdata() need be changed to spi_controller_get_devdata() ? Thanks, Yang
On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote: > On 2022/9/21 2:33, Mark Brown wrote: > > If we're switching please update to the modern naming and use > > "controller" rather than the old name. > Do you mean to use spi_controller instead of spi_master? Something like > this: > 'struct spi_controller * ctlr = devm_spi_alloc_master();' Or just use devm_spi_alloc_controller() directly. > Dose spi_master_get_devdata() need be changed to > spi_controller_get_devdata() ? Ideally.
On 2022/9/21 20:37, Mark Brown wrote: > On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote: >> On 2022/9/21 2:33, Mark Brown wrote: >>> If we're switching please update to the modern naming and use >>> "controller" rather than the old name. >> Do you mean to use spi_controller instead of spi_master? Something like >> this: >> 'struct spi_controller * ctlr = devm_spi_alloc_master();' > Or just use devm_spi_alloc_controller() directly. Does __devm_spi_alloc_controller() need be changed to devm_spi_alloc_controller(), then use it, or just use __devm_spi_alloc_controller() directly. Thanks, Yang > >> Dose spi_master_get_devdata() need be changed to >> spi_controller_get_devdata() ? > Ideally.
On Wed, Sep 21, 2022 at 01:37:49PM +0100, Mark Brown wrote: > On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote: > > On 2022/9/21 2:33, Mark Brown wrote: > > > If we're switching please update to the modern naming and use > > > "controller" rather than the old name. > > > Do you mean to use spi_controller instead of spi_master? Something like > > this: > > 'struct spi_controller * ctlr = devm_spi_alloc_master();' > > Or just use devm_spi_alloc_controller() directly. There's no such thing. The driver needs to explicitly allocate a master or slave and that will result in the slave bit being set correctly in struct spi_controller. Yang's v2 series now calls __devm_spi_alloc_controller() but drivers should never call that directly. Thanks, Lukas
On Tue, Sep 20, 2022 at 09:48:13PM +0800, Yang Yingliang wrote: > This patchset is trying to replace spi_alloc_master() with > devm_spi_alloc_master() in some spi drivers. With this helper, > spi_master_put() is called in devres_release_all() whenever > the device is unbound, so the spi_master_put() in error path > can be removed. > > Yang Yingliang (6): > spi: oc-tiny: Switch to use devm_spi_alloc_master() > spi: ath79: Switch to use devm_spi_alloc_master() > spi: omap-uwire: Switch to use devm_spi_alloc_master() > spi: ppc4xx: Switch to use devm_spi_alloc_master() > spi: sh-sci: Switch to use devm_spi_alloc_master() > spi: altera: Switch to use devm_spi_alloc_master() I'm withdrawing my objections to patches 1, 2 and 3: I failed to appreciate that these drivers use spi_bitbang_start(), which takes an extra reference on the controller. Sorry for the noise. Whole series is Reviewed-by: Lukas Wunner <lukas@wunner.de> This pertains to v1 of the series, not v2 (which incorrectly uses __devm_spi_alloc_controller()). Thanks, Lukas
On Wed, Sep 21, 2022 at 09:19:35PM +0800, Yang Yingliang wrote: > On 2022/9/21 20:37, Mark Brown wrote: > > On Wed, Sep 21, 2022 at 10:02:25AM +0800, Yang Yingliang wrote: > > > On 2022/9/21 2:33, Mark Brown wrote: > > > > If we're switching please update to the modern naming and use > > > > "controller" rather than the old name. > > > Do you mean to use spi_controller instead of spi_master? Something like > > > this: > > > 'struct spi_controller * ctlr = devm_spi_alloc_master();' > > > > Or just use devm_spi_alloc_controller() directly. > > Does __devm_spi_alloc_controller() need be changed to > devm_spi_alloc_controller(), then use > it, or just use __devm_spi_alloc_controller() directly. Modern drivers only differentiate between master and slave on allocation. They use struct spi_controller instead of spi_master and generally only call the spi_controller_*() functions (again, except on allocation). I think Mark wanted you to convert the drivers so that they use struct spi_controller everywhere, but that can be done in separate patches. Thanks, Lukas
On Fri, Sep 23, 2022 at 06:42:58AM +0200, Lukas Wunner wrote: > On Wed, Sep 21, 2022 at 01:37:49PM +0100, Mark Brown wrote: > > Or just use devm_spi_alloc_controller() directly. > There's no such thing. The driver needs to explicitly allocate a > master or slave and that will result in the slave bit being set > correctly in struct spi_controller. > Yang's v2 series now calls __devm_spi_alloc_controller() > but drivers should never call that directly. Right, we should probably make the actual function to wrap that though - I'd misremembered that that hadn't been created.
On 2022/9/23 18:12, Mark Brown wrote: > On Fri, Sep 23, 2022 at 06:42:58AM +0200, Lukas Wunner wrote: >> On Wed, Sep 21, 2022 at 01:37:49PM +0100, Mark Brown wrote: >>> Or just use devm_spi_alloc_controller() directly. >> There's no such thing. The driver needs to explicitly allocate a >> master or slave and that will result in the slave bit being set >> correctly in struct spi_controller. >> Yang's v2 series now calls __devm_spi_alloc_controller() >> but drivers should never call that directly. > Right, we should probably make the actual function to wrap that though - > I'd misremembered that that hadn't been created. How about introduce devm_spi_alloc_controller() like this: diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h index f089ee1ead58..67e510c8d15e 100644 --- a/include/linux/spi/spi.h +++ b/include/linux/spi/spi.h @@ -763,6 +763,8 @@ struct spi_controller *__devm_spi_alloc_controller(struct device *dev, unsigned int size, bool slave); +#define devm_spi_alloc_controller devm_spi_alloc_master + Thanks, Yang