| Message ID | 1459856912-17859-2-git-send-email-ldewangan@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 05 Apr 2016, Laxman Dewangan wrote: > Add device managed APIs devm_mfd_add_devices() and > devm_mfd_remove_devices() for the APIs mfd_add_devices() > and mfd_remove_devices(). Nit: Line wrap after "devm_mfd_remove_devices()" instead. > This helps in reducing code in error path and sometimes > removal of .remove callback for driver unbind. s/for/during/ > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > --- > drivers/mfd/mfd-core.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/mfd/core.h | 7 ++++++ > 2 files changed, 69 insertions(+) > > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c > index 409da01..145712e 100644 > --- a/drivers/mfd/mfd-core.c > +++ b/drivers/mfd/mfd-core.c > @@ -334,6 +334,68 @@ void mfd_remove_devices(struct device *parent) > } > EXPORT_SYMBOL(mfd_remove_devices); > > +static void devm_mfd_dev_release(struct device *dev, void *res) > +{ > + mfd_remove_devices(dev); > +} > + > +/** > + * devm_mfd_add_devices - Resource managed version of mfd_add_devices() > + * > + * This returns the 0 on success otherwise error number in the failure. "Returns 0 on success or an appropriate negative error number on failure." > + * The allocated mfd devices will automatically be released when the s/mfd/MFD/ > + * device is unbound. > + */ > +int devm_mfd_add_devices(struct device *dev, int id, > + const struct mfd_cell *cells, int n_devs, > + struct resource *mem_base, > + int irq_base, struct irq_domain *domain) > +{ > + struct device **ptr; > + int ret; > + > + ptr = devres_alloc(devm_mfd_dev_release, sizeof(*ptr), GFP_KERNEL); > + if (!ptr) > + return -ENOMEM; > + > + ret = mfd_add_devices(dev, id, cells, n_devs, mem_base, > + irq_base, domain); > + if (!ret) { > + *ptr = dev; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } Switch these round. If you encounter a problem, free and return. If not, skip the error handling and add the device outside of the if(). > + return ret; > +} > +EXPORT_SYMBOL(devm_mfd_add_devices); > + > +static int devm_mfd_devs_match(struct device *dev, void *res, void *data) > +{ > + struct device **r = res; > + > + if (WARN_ON(!r || !*r)) > + return 0; > + > + return *r == data; > +} > + > +/** > + * devm_mfd_remove_device - Resource managed version of mfd_remove_devices() > + * @dev: Device for which which resource was allocated. Did you proof read your own writing? Pleaes re-word. > + * Remove all mfd devices added on the device. s/mfd/MFD/ 'D' already means devices, so here you are saying "devices devices". Please re-word. Besides, you need to be more specific as to which "devices on the devices" you are detailing, since this sentence doesn't really make a great deal of sense. > + * Normally this function will not need to be called and the resource > + * management code will ensure that the resource is freed. Then what is the purpose of providing it? Do you have a user? > + */ > +void devm_mfd_remove_devices(struct device *dev) > +{ > + WARN_ON(devres_release(dev, devm_mfd_dev_release, > + devm_mfd_devs_match, dev)); > +} > +EXPORT_SYMBOL(devm_mfd_remove_devices); > + > int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones) > { > struct mfd_cell cell_entry; > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h > index bc6f7e0..d658d7f 100644 > --- a/include/linux/mfd/core.h > +++ b/include/linux/mfd/core.h > @@ -131,4 +131,11 @@ static inline int mfd_add_hotplug_devices(struct device *parent, > > extern void mfd_remove_devices(struct device *parent); > > +extern int devm_mfd_add_devices(struct device *dev, int id, > + const struct mfd_cell *cells, int n_devs, > + struct resource *mem_base, > + int irq_base, struct irq_domain *irq_domain); > + > +extern void devm_mfd_remove_devices(struct device *dev); > + > #endif
Hi Lee, Thanks for review. I will send another patch with incorporating your comments. On Thursday 07 April 2016 04:14 PM, Lee Jones wrote: > On Tue, 05 Apr 2016, Laxman Dewangan wrote: > > + if (!ret) { > + *ptr = dev; > + devres_add(dev, ptr); > + } else { > + devres_free(ptr); > + } > Switch these round. If you encounter a problem, free and return. If > not, skip the error handling and add the device outside of the if(). Like below? if (ret) { devres_free(ptr); return ret; } *ptr = dev; devres_add(dev, ptr); return ret; >> + * Remove all mfd devices added on the device. > s/mfd/MFD/ > > 'D' already means devices, so here you are saying "devices devices". > Please re-word. Besides, you need to be more specific as to which > "devices on the devices" you are detailing, since this sentence > doesn't really make a great deal of sense. Wanted to say Remove all devices added by mfd_add_devices() from parent device. >> + * Normally this function will not need to be called and the resource >> + * management code will ensure that the resource is freed. > Then what is the purpose of providing it? Do you have a user? To have pair of release. I have not seen the usage of most of devm_*_release() function other than devm_kfree(). -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 07 Apr 2016, Laxman Dewangan wrote: > Hi Lee, > Thanks for review. > I will send another patch with incorporating your comments. > > > On Thursday 07 April 2016 04:14 PM, Lee Jones wrote: > >On Tue, 05 Apr 2016, Laxman Dewangan wrote: > > > >+ if (!ret) { > >+ *ptr = dev; > >+ devres_add(dev, ptr); > >+ } else { > >+ devres_free(ptr); > >+ } > >Switch these round. If you encounter a problem, free and return. If > >not, skip the error handling and add the device outside of the if(). > > Like below? > > if (ret) { > devres_free(ptr); > return ret; > } > > *ptr = dev; > devres_add(dev, ptr); > > return ret; This is more in line with what I expect, yes. > >>+ * Remove all mfd devices added on the device. > >s/mfd/MFD/ > > > >'D' already means devices, so here you are saying "devices devices". > >Please re-word. Besides, you need to be more specific as to which > >"devices on the devices" you are detailing, since this sentence > >doesn't really make a great deal of sense. > Wanted to say > Remove all devices added by mfd_add_devices() from parent device. Remove all chlid-devices? > >>+ * Normally this function will not need to be called and the resource > >>+ * management code will ensure that the resource is freed. > >Then what is the purpose of providing it? Do you have a user? > > To have pair of release. I have not seen the usage of most of > devm_*_release() function other than devm_kfree(). Unless you have a need or a user, I would omit this for now.
On Thursday 07 April 2016 05:12 PM, Lee Jones wrote: > On Thu, 07 Apr 2016, Laxman Dewangan wrote: > > + * Normally this function will not need to be called and the resource > + * management code will ensure that the resource is freed. >>> Then what is the purpose of providing it? Do you have a user? >> To have pair of release. I have not seen the usage of most of >> devm_*_release() function other than devm_kfree(). > Unless you have a need or a user, I would omit this for now. > OK, I will remove it. BTW, I have collected all acks and reviewed by which I got from this series. So I can send this series with all acks/reviewed by. If you want me to provide the git location for these patchs then I can do it also. For this I will need base from where I need to take branch. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c index 409da01..145712e 100644 --- a/drivers/mfd/mfd-core.c +++ b/drivers/mfd/mfd-core.c @@ -334,6 +334,68 @@ void mfd_remove_devices(struct device *parent) } EXPORT_SYMBOL(mfd_remove_devices); +static void devm_mfd_dev_release(struct device *dev, void *res) +{ + mfd_remove_devices(dev); +} + +/** + * devm_mfd_add_devices - Resource managed version of mfd_add_devices() + * + * This returns the 0 on success otherwise error number in the failure. + * The allocated mfd devices will automatically be released when the + * device is unbound. + */ +int devm_mfd_add_devices(struct device *dev, int id, + const struct mfd_cell *cells, int n_devs, + struct resource *mem_base, + int irq_base, struct irq_domain *domain) +{ + struct device **ptr; + int ret; + + ptr = devres_alloc(devm_mfd_dev_release, sizeof(*ptr), GFP_KERNEL); + if (!ptr) + return -ENOMEM; + + ret = mfd_add_devices(dev, id, cells, n_devs, mem_base, + irq_base, domain); + if (!ret) { + *ptr = dev; + devres_add(dev, ptr); + } else { + devres_free(ptr); + } + + return ret; +} +EXPORT_SYMBOL(devm_mfd_add_devices); + +static int devm_mfd_devs_match(struct device *dev, void *res, void *data) +{ + struct device **r = res; + + if (WARN_ON(!r || !*r)) + return 0; + + return *r == data; +} + +/** + * devm_mfd_remove_device - Resource managed version of mfd_remove_devices() + * @dev: Device for which which resource was allocated. + * + * Remove all mfd devices added on the device. + * Normally this function will not need to be called and the resource + * management code will ensure that the resource is freed. + */ +void devm_mfd_remove_devices(struct device *dev) +{ + WARN_ON(devres_release(dev, devm_mfd_dev_release, + devm_mfd_devs_match, dev)); +} +EXPORT_SYMBOL(devm_mfd_remove_devices); + int mfd_clone_cell(const char *cell, const char **clones, size_t n_clones) { struct mfd_cell cell_entry; diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h index bc6f7e0..d658d7f 100644 --- a/include/linux/mfd/core.h +++ b/include/linux/mfd/core.h @@ -131,4 +131,11 @@ static inline int mfd_add_hotplug_devices(struct device *parent, extern void mfd_remove_devices(struct device *parent); +extern int devm_mfd_add_devices(struct device *dev, int id, + const struct mfd_cell *cells, int n_devs, + struct resource *mem_base, + int irq_base, struct irq_domain *irq_domain); + +extern void devm_mfd_remove_devices(struct device *dev); + #endif
Add device managed APIs devm_mfd_add_devices() and devm_mfd_remove_devices() for the APIs mfd_add_devices() and mfd_remove_devices(). This helps in reducing code in error path and sometimes removal of .remove callback for driver unbind. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- drivers/mfd/mfd-core.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/mfd/core.h | 7 ++++++ 2 files changed, 69 insertions(+)