Re: [PATCH 7/8] rtc: rework rtc_register_device() resource management
On Fri, 27 Nov 2020, Bartosz Golaszewski wrote: > On Fri, Nov 27, 2020 at 10:16 AM Lee Jones wrote: > > > > > > > > On Mon, 9 Nov 2020 at 16:34, Bartosz Golaszewski wrote: > >> > >> From: Bartosz Golaszewski > >> > >> rtc_register_device() is a managed interface but it doesn't use devres > >> by itself - instead it marks an rtc_device as "registered" and the devres > >> callback for devm_rtc_allocate_device() takes care of resource release. > >> > >> This doesn't correspond with the design behind devres where managed > >> structures should not be aware of being managed. The correct solution > >> here is to register a separate devres callback for unregistering the > >> device. > >> > >> While at it: rename rtc_register_device() to devm_rtc_register_device() > >> and add it to the list of managed interfaces in devres.rst. This way we > >> can avoid any potential confusion of driver developers who may expect > >> there to exist a corresponding unregister function. > >> > >> Signed-off-by: Bartosz Golaszewski > >> --- > >> .../driver-api/driver-model/devres.rst| 1 + > >> arch/alpha/kernel/rtc.c | 2 +- > >> drivers/mfd/menelaus.c| 2 +- > > > > > > This patch should have been sent to and Acked by MFD too. > > > > Sorry Lee, I missed the fact that there were changes outside of > drivers/rtc/. Other than skipping the MFD maintainer - do you see > anything wrong in that bit? No real harm done. The patch looks fine from an MFD standpoint. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 7/8] rtc: rework rtc_register_device() resource management
On Fri, Nov 27, 2020 at 10:16 AM Lee Jones wrote: > > > > On Mon, 9 Nov 2020 at 16:34, Bartosz Golaszewski wrote: >> >> From: Bartosz Golaszewski >> >> rtc_register_device() is a managed interface but it doesn't use devres >> by itself - instead it marks an rtc_device as "registered" and the devres >> callback for devm_rtc_allocate_device() takes care of resource release. >> >> This doesn't correspond with the design behind devres where managed >> structures should not be aware of being managed. The correct solution >> here is to register a separate devres callback for unregistering the >> device. >> >> While at it: rename rtc_register_device() to devm_rtc_register_device() >> and add it to the list of managed interfaces in devres.rst. This way we >> can avoid any potential confusion of driver developers who may expect >> there to exist a corresponding unregister function. >> >> Signed-off-by: Bartosz Golaszewski >> --- >> .../driver-api/driver-model/devres.rst| 1 + >> arch/alpha/kernel/rtc.c | 2 +- >> drivers/mfd/menelaus.c| 2 +- > > > This patch should have been sent to and Acked by MFD too. > Sorry Lee, I missed the fact that there were changes outside of drivers/rtc/. Other than skipping the MFD maintainer - do you see anything wrong in that bit? Bartosz
Re: [PATCH 7/8] rtc: rework rtc_register_device() resource management
On Tue, Nov 17, 2020 at 10:35 PM Alexandre Belloni wrote: > > On 09/11/2020 17:34:08+0100, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski > > > > rtc_register_device() is a managed interface but it doesn't use devres > > by itself - instead it marks an rtc_device as "registered" and the devres > > callback for devm_rtc_allocate_device() takes care of resource release. > > > > This doesn't correspond with the design behind devres where managed > > structures should not be aware of being managed. The correct solution > > here is to register a separate devres callback for unregistering the > > device. > > > > While at it: rename rtc_register_device() to devm_rtc_register_device() > > and add it to the list of managed interfaces in devres.rst. This way we > > can avoid any potential confusion of driver developers who may expect > > there to exist a corresponding unregister function. > > > > I'm going to apply that but honestly, I don't like the fact that we now > end up with both devm_rtc_device_register and devm_rtc_register_device. > This was the main reason to not have the devm_ prefix there. I find that > way more confusing than the current situation. > Yes, it's unfortunate that we have two similarly named functions but devm_rtc_device_register() is deprecated and should go away right? In that case it's just temporary. Additionally: since it's just a wrapper around devm_rtc_allocate_device() and devm_rtc_register_device() now, we should be able to just replace the code in the drivers with the code from devm_rtc_device_register() and we may be able to get rid of it soon? Bartosz
Re: [PATCH 7/8] rtc: rework rtc_register_device() resource management
On 09/11/2020 17:34:08+0100, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski > > rtc_register_device() is a managed interface but it doesn't use devres > by itself - instead it marks an rtc_device as "registered" and the devres > callback for devm_rtc_allocate_device() takes care of resource release. > > This doesn't correspond with the design behind devres where managed > structures should not be aware of being managed. The correct solution > here is to register a separate devres callback for unregistering the > device. > > While at it: rename rtc_register_device() to devm_rtc_register_device() > and add it to the list of managed interfaces in devres.rst. This way we > can avoid any potential confusion of driver developers who may expect > there to exist a corresponding unregister function. > I'm going to apply that but honestly, I don't like the fact that we now end up with both devm_rtc_device_register and devm_rtc_register_device. This was the main reason to not have the devm_ prefix there. I find that way more confusing than the current situation. > Signed-off-by: Bartosz Golaszewski > --- > .../driver-api/driver-model/devres.rst| 1 + > arch/alpha/kernel/rtc.c | 2 +- > drivers/mfd/menelaus.c| 2 +- > drivers/rtc/class.c | 19 +-- > drivers/rtc/rtc-88pm80x.c | 2 +- > drivers/rtc/rtc-88pm860x.c| 2 +- > drivers/rtc/rtc-ab-b5ze-s3.c | 2 +- > drivers/rtc/rtc-ab-eoz9.c | 2 +- > drivers/rtc/rtc-ab3100.c | 2 +- > drivers/rtc/rtc-ab8500.c | 2 +- > drivers/rtc/rtc-abx80x.c | 2 +- > drivers/rtc/rtc-ac100.c | 2 +- > drivers/rtc/rtc-armada38x.c | 2 +- > drivers/rtc/rtc-aspeed.c | 2 +- > drivers/rtc/rtc-at91rm9200.c | 2 +- > drivers/rtc/rtc-at91sam9.c| 2 +- > drivers/rtc/rtc-au1xxx.c | 2 +- > drivers/rtc/rtc-bd70528.c | 2 +- > drivers/rtc/rtc-brcmstb-waketimer.c | 2 +- > drivers/rtc/rtc-cadence.c | 2 +- > drivers/rtc/rtc-cmos.c| 2 +- > drivers/rtc/rtc-coh901331.c | 2 +- > drivers/rtc/rtc-cpcap.c | 2 +- > drivers/rtc/rtc-cros-ec.c | 2 +- > drivers/rtc/rtc-da9052.c | 2 +- > drivers/rtc/rtc-da9063.c | 2 +- > drivers/rtc/rtc-davinci.c | 2 +- > drivers/rtc/rtc-digicolor.c | 2 +- > drivers/rtc/rtc-dm355evm.c| 2 +- > drivers/rtc/rtc-ds1305.c | 2 +- > drivers/rtc/rtc-ds1307.c | 2 +- > drivers/rtc/rtc-ds1343.c | 2 +- > drivers/rtc/rtc-ds1347.c | 2 +- > drivers/rtc/rtc-ds1374.c | 2 +- > drivers/rtc/rtc-ds1511.c | 2 +- > drivers/rtc/rtc-ds1553.c | 2 +- > drivers/rtc/rtc-ds1672.c | 2 +- > drivers/rtc/rtc-ds1685.c | 2 +- > drivers/rtc/rtc-ds1742.c | 2 +- > drivers/rtc/rtc-ds2404.c | 2 +- > drivers/rtc/rtc-ep93xx.c | 2 +- > drivers/rtc/rtc-fsl-ftm-alarm.c | 2 +- > drivers/rtc/rtc-ftrtc010.c| 2 +- > drivers/rtc/rtc-goldfish.c| 2 +- > drivers/rtc/rtc-imx-sc.c | 2 +- > drivers/rtc/rtc-imxdi.c | 2 +- > drivers/rtc/rtc-isl12026.c| 2 +- > drivers/rtc/rtc-isl1208.c | 2 +- > drivers/rtc/rtc-jz4740.c | 2 +- > drivers/rtc/rtc-lpc32xx.c | 2 +- > drivers/rtc/rtc-ls1x.c| 2 +- > drivers/rtc/rtc-m41t80.c | 2 +- > drivers/rtc/rtc-m48t59.c | 2 +- > drivers/rtc/rtc-m48t86.c | 2 +- > drivers/rtc/rtc-mc13xxx.c | 2 +- > drivers/rtc/rtc-meson-vrtc.c | 2 +- > drivers/rtc/rtc-meson.c | 2 +- > drivers/rtc/rtc-mpc5121.c | 2 +- > drivers/rtc/rtc-mrst.c| 2 +- > drivers/rtc/rtc-mt2712.c | 2 +- > drivers/rtc/rtc-mt6397.c | 2 +- > drivers/rtc/rtc-mv.c | 2 +- > drivers/rtc/rtc-mxc.c | 2 +- > drivers/rtc/rtc-mxc_v2.c | 2 +- > drivers/rtc/rtc-omap.c| 2 +- > drivers/rtc/rtc-pcap.c| 2 +- > drivers/rtc/rtc-pcf2123.c | 2 +- > drivers/rtc/rtc-pcf2127.c
