| Message ID | 068836c7-a116-7e6f-cbb2-f2176fbb9a38@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] media: dvb: Provide 4 devm variants | expand |
Hi Marc, Thanks for your patch. On Thu, 2019-07-18 at 15:03 +0200, Marc Gonzalez wrote: > Provide devm variants for automatic resource release on device removal. > Makes error-handling in probe() simpler, thus less error-prone. > Once all resources are devmanaged, remove() is no longer needed. > I think it would be better to add this API as part of a series that also uses it. Regards, Eze
On 21/07/2019 17:02, Ezequiel Garcia wrote: > On Thu, 2019-07-18 at 15:03 +0200, Marc Gonzalez wrote: > >> Provide devm variants for automatic resource release on device removal. >> Makes error-handling in probe() simpler, thus less error-prone. >> Once all resources are devmanaged, remove() is no longer needed. > > I think it would be better to add this API as part of a series > that also uses it. I have an outstanding RFC for a new driver: https://patchwork.kernel.org/patch/11040435/ Locally, I've updated the probe function to use the proposed devm functions. I was mainly hoping to get initial feedback from the maintainers. 1) See if they're OK with devm APIs 2) See if they're OK with the way I implemented devm APIs 3) See what's missing to get an Ack static int tsif_probe(struct platform_device *pdev) { int err, virq; struct tsif *tsif; struct resource *res; struct dvb_frontend *fe; struct dvb_adapter *adap; struct device *dev = &pdev->dev; short num = DVB_UNSET; fe = dvb_get_demod_fe(dev->of_node); if (!fe) return -ENXIO; tsif = devm_kzalloc(dev, sizeof(*tsif), GFP_KERNEL); if (!tsif) return -ENOMEM; res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsif0"); tsif->base = devm_ioremap_resource(dev, res); if (IS_ERR(tsif->base)) return PTR_ERR(tsif->base); virq = platform_get_irq_byname(pdev, "tsif0"); err = devm_request_irq(dev, virq, tsif_isr, IRQF_SHARED, "tsif", tsif); if (err) return err; tsif->clk = devm_clk_get(dev, "iface"); if (IS_ERR(tsif->clk)) return PTR_ERR(tsif->clk); err = devm_clk_prepare_enable(dev, tsif->clk); if (err) return err; adap = &tsif->adapter; err = devm_dvb_register_adapter(dev, adap, "tsif", THIS_MODULE, &num); if (err) return err; /* Not sure the diff between filter and feed? */ tsif->demux.filternum = 16; /*** Dunno what to put here ***/ tsif->demux.feednum = 16; /*** Dunno what to put here ***/ tsif->demux.start_feed = noop; tsif->demux.stop_feed = noop; err = devm_dvb_dmx_init(dev, &tsif->demux); if (err) return err; /* What relation to dvb_demux.filternum??? */ /* Do I need this object?? */ tsif->dmxdev.filternum = 16; tsif->dmxdev.demux = &tsif->demux.dmx; err = devm_dvb_dmxdev_init(dev, &tsif->dmxdev, adap); if (err) return err; err = devm_dvb_register_frontend(dev, adap, fe); if (err) return err; writel_relaxed(TSIF_START + ENABLE_IRQ, tsif->base + TSIF_STS_CTL); devm_add_action(dev, stop_tsif, tsif); INIT_WORK(&tsif->work, handle_pkt); return 0; }
On Mon, Jul 22, 2019 at 10:13:56AM +0200, Marc Gonzalez wrote: > On 21/07/2019 17:02, Ezequiel Garcia wrote: > > > On Thu, 2019-07-18 at 15:03 +0200, Marc Gonzalez wrote: > > > >> Provide devm variants for automatic resource release on device removal. > >> Makes error-handling in probe() simpler, thus less error-prone. > >> Once all resources are devmanaged, remove() is no longer needed. > > > > I think it would be better to add this API as part of a series > > that also uses it. > > I have an outstanding RFC for a new driver: > > https://patchwork.kernel.org/patch/11040435/ > > Locally, I've updated the probe function to use the proposed devm functions. > > I was mainly hoping to get initial feedback from the maintainers. > 1) See if they're OK with devm APIs devm_ has some lifetime issues, wrt to disconnect. However, for a platform device it is fine. > 2) See if they're OK with the way I implemented devm APIs Looks good to me. > 3) See what's missing to get an Ack Please submit both this patch and a the driver as a series. Thanks, Sean > > static int tsif_probe(struct platform_device *pdev) > { > int err, virq; > struct tsif *tsif; > struct resource *res; > struct dvb_frontend *fe; > struct dvb_adapter *adap; > struct device *dev = &pdev->dev; > short num = DVB_UNSET; > > fe = dvb_get_demod_fe(dev->of_node); > if (!fe) > return -ENXIO; > > tsif = devm_kzalloc(dev, sizeof(*tsif), GFP_KERNEL); > if (!tsif) > return -ENOMEM; > > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "tsif0"); > tsif->base = devm_ioremap_resource(dev, res); > if (IS_ERR(tsif->base)) > return PTR_ERR(tsif->base); > > virq = platform_get_irq_byname(pdev, "tsif0"); > err = devm_request_irq(dev, virq, tsif_isr, IRQF_SHARED, "tsif", tsif); > if (err) > return err; > > tsif->clk = devm_clk_get(dev, "iface"); > if (IS_ERR(tsif->clk)) > return PTR_ERR(tsif->clk); > > err = devm_clk_prepare_enable(dev, tsif->clk); > if (err) > return err; > > adap = &tsif->adapter; > err = devm_dvb_register_adapter(dev, adap, "tsif", THIS_MODULE, &num); > if (err) return err; > > /* Not sure the diff between filter and feed? */ > tsif->demux.filternum = 16; /*** Dunno what to put here ***/ > tsif->demux.feednum = 16; /*** Dunno what to put here ***/ > tsif->demux.start_feed = noop; > tsif->demux.stop_feed = noop; > err = devm_dvb_dmx_init(dev, &tsif->demux); > if (err) return err; > > /* What relation to dvb_demux.filternum??? */ > /* Do I need this object?? */ > tsif->dmxdev.filternum = 16; > tsif->dmxdev.demux = &tsif->demux.dmx; > err = devm_dvb_dmxdev_init(dev, &tsif->dmxdev, adap); > if (err) return err; > > err = devm_dvb_register_frontend(dev, adap, fe); > if (err) return err; > > writel_relaxed(TSIF_START + ENABLE_IRQ, tsif->base + TSIF_STS_CTL); > devm_add_action(dev, stop_tsif, tsif); > INIT_WORK(&tsif->work, handle_pkt); > > return 0; > }
diff --git a/Documentation/driver-model/devres.rst b/Documentation/driver-model/devres.rst index 6af41a6a4bb6..84bdb61f27a3 100644 --- a/Documentation/driver-model/devres.rst +++ b/Documentation/driver-model/devres.rst @@ -268,6 +268,12 @@ DMA DRM devm_drm_dev_init() +DVB + devm_dvb_dmx_init() + devm_dvb_dmxdev_init() + devm_dvb_register_adapter() + devm_dvb_register_frontend() + GPIO devm_gpiod_get() devm_gpiod_get_index() diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c index f14a872d1268..f38b65d1b430 100644 --- a/drivers/media/dvb-core/dmxdev.c +++ b/drivers/media/dvb-core/dmxdev.c @@ -1444,6 +1444,19 @@ int dvb_dmxdev_init(struct dmxdev *dmxdev, struct dvb_adapter *dvb_adapter) EXPORT_SYMBOL(dvb_dmxdev_init); +static void dmxdev_release(void *dmxdev) +{ + dvb_dmxdev_release(dmxdev); +} + +int devm_dvb_dmxdev_init(struct device *dev, struct dmxdev *dmxdev, + struct dvb_adapter *dvb_adapter) +{ + int rc = dvb_dmxdev_init(dmxdev, dvb_adapter); + return rc ? : devm_add_action_or_reset(dev, dmxdev_release, dmxdev); +} +EXPORT_SYMBOL(devm_dvb_dmxdev_init); + void dvb_dmxdev_release(struct dmxdev *dmxdev) { dmxdev->exit = 1; diff --git a/drivers/media/dvb-core/dvb_demux.c b/drivers/media/dvb-core/dvb_demux.c index 39a2c6ccf31d..55b9675b99c5 100644 --- a/drivers/media/dvb-core/dvb_demux.c +++ b/drivers/media/dvb-core/dvb_demux.c @@ -28,6 +28,7 @@ #include <linux/string.h> #include <linux/crc32.h> #include <linux/uaccess.h> +#include <linux/device.h> #include <asm/div64.h> #include <media/dvb_demux.h> @@ -1317,6 +1318,18 @@ int dvb_dmx_init(struct dvb_demux *dvbdemux) EXPORT_SYMBOL(dvb_dmx_init); +static void dmx_release(void *dvbdemux) +{ + dvb_dmx_release(dvbdemux); +} + +int devm_dvb_dmx_init(struct device *dev, struct dvb_demux *dvbdemux) +{ + int rc = dvb_dmx_init(dvbdemux); + return rc ? : devm_add_action_or_reset(dev, dmx_release, dvbdemux); +} +EXPORT_SYMBOL(devm_dvb_dmx_init); + void dvb_dmx_release(struct dvb_demux *dvbdemux) { vfree(dvbdemux->cnt_storage); diff --git a/drivers/media/dvb-core/dvb_frontend.c b/drivers/media/dvb-core/dvb_frontend.c index 209186c5cd9b..4a281cd8c064 100644 --- a/drivers/media/dvb-core/dvb_frontend.c +++ b/drivers/media/dvb-core/dvb_frontend.c @@ -3007,6 +3007,19 @@ int dvb_register_frontend(struct dvb_adapter *dvb, } EXPORT_SYMBOL(dvb_register_frontend); +static void unregister_frontend(void *fe) +{ + dvb_unregister_frontend(fe); +} + +int devm_dvb_register_frontend(struct device *dev, + struct dvb_adapter *dvb, struct dvb_frontend *fe) +{ + int rc = dvb_register_frontend(dvb, fe); + return rc ? : devm_add_action_or_reset(dev, unregister_frontend, fe); +} +EXPORT_SYMBOL(devm_dvb_register_frontend); + int dvb_unregister_frontend(struct dvb_frontend *fe) { struct dvb_frontend_private *fepriv = fe->frontend_priv; diff --git a/drivers/media/dvb-core/dvbdev.c b/drivers/media/dvb-core/dvbdev.c index a3393cd4e584..0fc8dbf65045 100644 --- a/drivers/media/dvb-core/dvbdev.c +++ b/drivers/media/dvb-core/dvbdev.c @@ -885,6 +885,18 @@ int dvb_register_adapter(struct dvb_adapter *adap, const char *name, } EXPORT_SYMBOL(dvb_register_adapter); +static void unreg_adapter(void *adap) +{ + dvb_unregister_adapter(adap); +} + +int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap, + const char *name, struct module *module, short *adapter_nums) +{ + int rc = dvb_register_adapter(adap, name, module, dev, adapter_nums); + return rc < 0 ? rc : devm_add_action_or_reset(dev, unreg_adapter, adap); +} +EXPORT_SYMBOL(devm_dvb_register_adapter); int dvb_unregister_adapter(struct dvb_adapter *adap) { diff --git a/include/media/dmxdev.h b/include/media/dmxdev.h index baafa3b8aca4..0300d829aeea 100644 --- a/include/media/dmxdev.h +++ b/include/media/dmxdev.h @@ -204,6 +204,9 @@ struct dmxdev { */ int dvb_dmxdev_init(struct dmxdev *dmxdev, struct dvb_adapter *adap); +int devm_dvb_dmxdev_init(struct device *dev, struct dmxdev *dmxdev, + struct dvb_adapter *dvb_adapter); + /** * dvb_dmxdev_release - releases a digital TV demux and unregisters it. * diff --git a/include/media/dvb_demux.h b/include/media/dvb_demux.h index 3b6aeca7a49e..95fe901076d7 100644 --- a/include/media/dvb_demux.h +++ b/include/media/dvb_demux.h @@ -266,6 +266,8 @@ struct dvb_demux { */ int dvb_dmx_init(struct dvb_demux *demux); +int devm_dvb_dmx_init(struct device *dev, struct dvb_demux *demux); + /** * dvb_dmx_release - releases a digital TV demux internal buffers. * diff --git a/include/media/dvb_frontend.h b/include/media/dvb_frontend.h index f05cd7b94a2c..56dbaf3cc7a9 100644 --- a/include/media/dvb_frontend.h +++ b/include/media/dvb_frontend.h @@ -713,6 +713,9 @@ struct dvb_frontend { int dvb_register_frontend(struct dvb_adapter *dvb, struct dvb_frontend *fe); +int devm_dvb_register_frontend(struct device *dev, + struct dvb_adapter *dvb, struct dvb_frontend *fe); + /** * dvb_unregister_frontend() - Unregisters a DVB frontend * diff --git a/include/media/dvbdev.h b/include/media/dvbdev.h index 551325858de3..32f8d1626319 100644 --- a/include/media/dvbdev.h +++ b/include/media/dvbdev.h @@ -202,6 +202,9 @@ int dvb_register_adapter(struct dvb_adapter *adap, const char *name, struct module *module, struct device *device, short *adapter_nums); +int devm_dvb_register_adapter(struct device *dev, struct dvb_adapter *adap, + const char *name, struct module *module, short *adapter_nums); + /** * dvb_unregister_adapter - Unregisters a DVB adapter *
Provide devm variants for automatic resource release on device removal. Makes error-handling in probe() simpler, thus less error-prone. Once all resources are devmanaged, remove() is no longer needed. Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- Changes from v2: x Handle dvb_register_adapter() return code correctly Changes from v1: x Simplify devm_dvb_register_adapter() implementation x Add 3 more devm APIs x Document them in devres.rst --- Documentation/driver-model/devres.rst | 6 ++++++ drivers/media/dvb-core/dmxdev.c | 13 +++++++++++++ drivers/media/dvb-core/dvb_demux.c | 13 +++++++++++++ drivers/media/dvb-core/dvb_frontend.c | 13 +++++++++++++ drivers/media/dvb-core/dvbdev.c | 12 ++++++++++++ include/media/dmxdev.h | 3 +++ include/media/dvb_demux.h | 2 ++ include/media/dvb_frontend.h | 3 +++ include/media/dvbdev.h | 3 +++ 9 files changed, 68 insertions(+)