| Message ID | 20240618160836.945242-3-olivier.moysan@foss.st.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: dfsdm: add scaling support | expand |
On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote: > Add iio_backend_disable() and iio_backend_enable() APIs to allow > IIO backend consumer to request backend disabling and enabling. > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> > --- Hi Olivier, small notes from me... > drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++ > include/linux/iio/backend.h | 2 ++ > 2 files changed, 28 insertions(+) > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio- > backend.c > index b950e30018ca..d3db048c086b 100644 > --- a/drivers/iio/industrialio-backend.c > +++ b/drivers/iio/industrialio-backend.c > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct > iio_backend *back) > } > EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND); > > +/** > + * iio_backend_enable - Backend enable > + * @dev: Consumer device for the backend > + * @back: Backend device > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_enable(struct device *dev, struct iio_backend *back) > +{ > + return iio_backend_op_call(back, enable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); We do already have devm_iio_backend_enable(). From a correctness stand point and even scalability, that API should now call your new iio_backend_enable() instead of directly call iio_backend_op_call(). I guess that change could be in this patch. > + > +/** > + * iio_backend_disable - Backend disable > + * @dev: Consumer device for the backend > + * @back: Backend device > + * > + */ > +void iio_backend_disable(struct device *dev, struct iio_backend *back) > +{ > + iio_backend_void_op_call(back, disable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); > + We also have __iio_backend_disable() which is static since all users were using devm_iio_backend_enable(). I understand that's not suitable for you but I would instead rename the existing function to iio_backend_disable() and export it. With the above changes: Reviewed-by: Nuno Sa <nuno.sa@analog.com> - Nuno Sá
Hi Nuno, On 6/19/24 07:21, Nuno Sá wrote: > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote: >> Add iio_backend_disable() and iio_backend_enable() APIs to allow >> IIO backend consumer to request backend disabling and enabling. >> >> Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> >> --- > > Hi Olivier, > > small notes from me... > >> drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++ >> include/linux/iio/backend.h | 2 ++ >> 2 files changed, 28 insertions(+) >> >> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio- >> backend.c >> index b950e30018ca..d3db048c086b 100644 >> --- a/drivers/iio/industrialio-backend.c >> +++ b/drivers/iio/industrialio-backend.c >> @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct >> iio_backend *back) >> } >> EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND); >> >> +/** >> + * iio_backend_enable - Backend enable >> + * @dev: Consumer device for the backend >> + * @back: Backend device >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_enable(struct device *dev, struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, enable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); > > We do already have devm_iio_backend_enable(). From a correctness stand point and even > scalability, that API should now call your new iio_backend_enable() instead of > directly call iio_backend_op_call(). I guess that change could be in this patch. > Sure. I have updated the patch. >> + >> +/** >> + * iio_backend_disable - Backend disable >> + * @dev: Consumer device for the backend >> + * @back: Backend device >> + * >> + */ >> +void iio_backend_disable(struct device *dev, struct iio_backend *back) >> +{ >> + iio_backend_void_op_call(back, disable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); >> + > > We also have __iio_backend_disable() which is static since all users were using > devm_iio_backend_enable(). I understand that's not suitable for you but I would > instead rename the existing function to iio_backend_disable() and export it. > Just renaming is not sufficient. The reason is that devm_add_action_or_reset() require an action with action(void *) prototype. So the prototype of iio_backend_disable() has to be changed to void iio_backend_disable(void *back). I placed the same arguments in enable and disable for symmetry, but *dev is not required for time being in disable API. So it can make sense to change iio_backend_disable() prototype. alternatively, we can call __iio_backend_disable() through this API. Please, let me know is you have a preference. Thanks Olivier > With the above changes: > > Reviewed-by: Nuno Sa <nuno.sa@analog.com> > > - Nuno Sá >
On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote: > Hi Nuno, > > On 6/19/24 07:21, Nuno Sá wrote: > > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote: > > > Add iio_backend_disable() and iio_backend_enable() APIs to allow > > > IIO backend consumer to request backend disabling and enabling. > > > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> > > > --- > > > > Hi Olivier, > > > > small notes from me... > > > > > drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++ > > > include/linux/iio/backend.h | 2 ++ > > > 2 files changed, 28 insertions(+) > > > > > > diff --git a/drivers/iio/industrialio-backend.c > > > b/drivers/iio/industrialio- > > > backend.c > > > index b950e30018ca..d3db048c086b 100644 > > > --- a/drivers/iio/industrialio-backend.c > > > +++ b/drivers/iio/industrialio-backend.c > > > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, > > > struct > > > iio_backend *back) > > > } > > > EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND); > > > > > > +/** > > > + * iio_backend_enable - Backend enable > > > + * @dev: Consumer device for the backend > > > + * @back: Backend device > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int iio_backend_enable(struct device *dev, struct iio_backend *back) > > > +{ > > > + return iio_backend_op_call(back, enable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); > > > > We do already have devm_iio_backend_enable(). From a correctness stand point > > and even > > scalability, that API should now call your new iio_backend_enable() instead > > of > > directly call iio_backend_op_call(). I guess that change could be in this > > patch. > > > > Sure. I have updated the patch. > > > > + > > > +/** > > > + * iio_backend_disable - Backend disable > > > + * @dev: Consumer device for the backend > > > + * @back: Backend device > > > + * > > > + */ > > > +void iio_backend_disable(struct device *dev, struct iio_backend *back) > > > +{ > > > + iio_backend_void_op_call(back, disable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); > > > + > > > > We also have __iio_backend_disable() which is static since all users were > > using > > devm_iio_backend_enable(). I understand that's not suitable for you but I > > would > > instead rename the existing function to iio_backend_disable() and export it. > > > > Just renaming is not sufficient. The reason is that > devm_add_action_or_reset() require an action with action(void *) > prototype. So the prototype of iio_backend_disable() has to be changed > to void iio_backend_disable(void *back). > I placed the same arguments in enable and disable for symmetry, but *dev > is not required for time being in disable API. So it can make sense to > change iio_backend_disable() prototype. > alternatively, we can call __iio_backend_disable() through this API. > Please, let me know is you have a preference. > Oh, yes, you're right. I would prefer your later option. Call __iio_backend_disable() from __iio_backend_disable() with a proper typed parameter. I also just realized your 'struct device *dev' parameter. I think it can be removed for these APIs. The only reason for it is for devm_add_action_or_reset() which we don't need- - Nuno Sá > >
On Thu, 20 Jun 2024 12:07:47 +0200 Nuno Sá <noname.nuno@gmail.com> wrote: > On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote: > > Hi Nuno, > > > > On 6/19/24 07:21, Nuno Sá wrote: > > > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote: > > > > Add iio_backend_disable() and iio_backend_enable() APIs to allow > > > > IIO backend consumer to request backend disabling and enabling. > > > > > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> > > > > --- > > > > > > Hi Olivier, > > > > > > small notes from me... > > > > > > > drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++ > > > > include/linux/iio/backend.h | 2 ++ > > > > 2 files changed, 28 insertions(+) > > > > > > > > diff --git a/drivers/iio/industrialio-backend.c > > > > b/drivers/iio/industrialio- > > > > backend.c > > > > index b950e30018ca..d3db048c086b 100644 > > > > --- a/drivers/iio/industrialio-backend.c > > > > +++ b/drivers/iio/industrialio-backend.c > > > > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, > > > > struct > > > > iio_backend *back) > > > > } > > > > EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND); > > > > > > > > +/** > > > > + * iio_backend_enable - Backend enable > > > > + * @dev: Consumer device for the backend > > > > + * @back: Backend device > > > > + * > > > > + * RETURNS: > > > > + * 0 on success, negative error number on failure. > > > > + */ > > > > +int iio_backend_enable(struct device *dev, struct iio_backend *back) > > > > +{ > > > > + return iio_backend_op_call(back, enable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); > > > > > > We do already have devm_iio_backend_enable(). From a correctness stand point > > > and even > > > scalability, that API should now call your new iio_backend_enable() instead > > > of > > > directly call iio_backend_op_call(). I guess that change could be in this > > > patch. > > > > > > > Sure. I have updated the patch. > > > > > > + > > > > +/** > > > > + * iio_backend_disable - Backend disable > > > > + * @dev: Consumer device for the backend > > > > + * @back: Backend device > > > > + * > > > > + */ > > > > +void iio_backend_disable(struct device *dev, struct iio_backend *back) > > > > +{ > > > > + iio_backend_void_op_call(back, disable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); > > > > + > > > > > > We also have __iio_backend_disable() which is static since all users were > > > using > > > devm_iio_backend_enable(). I understand that's not suitable for you but I > > > would > > > instead rename the existing function to iio_backend_disable() and export it. > > > > > > > Just renaming is not sufficient. The reason is that > > devm_add_action_or_reset() require an action with action(void *) > > prototype. So the prototype of iio_backend_disable() has to be changed > > to void iio_backend_disable(void *back). > > I placed the same arguments in enable and disable for symmetry, but *dev > > is not required for time being in disable API. So it can make sense to > > change iio_backend_disable() prototype. > > alternatively, we can call __iio_backend_disable() through this API. > > Please, let me know is you have a preference. > > > > Oh, yes, you're right. I would prefer your later option. Call > __iio_backend_disable() from __iio_backend_disable() with a proper typed ? That looks like an infinite loop :) > parameter. I also just realized your 'struct device *dev' parameter. I think it > can be removed for these APIs. The only reason for it is for > devm_add_action_or_reset() which we don't need- > > - Nuno Sá > > > >
On Sun, 2024-06-23 at 14:56 +0100, Jonathan Cameron wrote: > On Thu, 20 Jun 2024 12:07:47 +0200 > Nuno Sá <noname.nuno@gmail.com> wrote: > > > On Wed, 2024-06-19 at 17:59 +0200, Olivier MOYSAN wrote: > > > Hi Nuno, > > > > > > On 6/19/24 07:21, Nuno Sá wrote: > > > > On Tue, 2024-06-18 at 18:08 +0200, Olivier Moysan wrote: > > > > > Add iio_backend_disable() and iio_backend_enable() APIs to allow > > > > > IIO backend consumer to request backend disabling and enabling. > > > > > > > > > > Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> > > > > > --- > > > > > > > > Hi Olivier, > > > > > > > > small notes from me... > > > > > > > > > drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++ > > > > > include/linux/iio/backend.h | 2 ++ > > > > > 2 files changed, 28 insertions(+) > > > > > > > > > > diff --git a/drivers/iio/industrialio-backend.c > > > > > b/drivers/iio/industrialio- > > > > > backend.c > > > > > index b950e30018ca..d3db048c086b 100644 > > > > > --- a/drivers/iio/industrialio-backend.c > > > > > +++ b/drivers/iio/industrialio-backend.c > > > > > @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, > > > > > struct > > > > > iio_backend *back) > > > > > } > > > > > EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND); > > > > > > > > > > +/** > > > > > + * iio_backend_enable - Backend enable > > > > > + * @dev: Consumer device for the backend > > > > > + * @back: Backend device > > > > > + * > > > > > + * RETURNS: > > > > > + * 0 on success, negative error number on failure. > > > > > + */ > > > > > +int iio_backend_enable(struct device *dev, struct iio_backend *back) > > > > > +{ > > > > > + return iio_backend_op_call(back, enable); > > > > > +} > > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); > > > > > > > > We do already have devm_iio_backend_enable(). From a correctness stand point > > > > and even > > > > scalability, that API should now call your new iio_backend_enable() instead > > > > of > > > > directly call iio_backend_op_call(). I guess that change could be in this > > > > patch. > > > > > > > > > > Sure. I have updated the patch. > > > > > > > > + > > > > > +/** > > > > > + * iio_backend_disable - Backend disable > > > > > + * @dev: Consumer device for the backend > > > > > + * @back: Backend device > > > > > + * > > > > > + */ > > > > > +void iio_backend_disable(struct device *dev, struct iio_backend *back) > > > > > +{ > > > > > + iio_backend_void_op_call(back, disable); > > > > > +} > > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); > > > > > + > > > > > > > > We also have __iio_backend_disable() which is static since all users were > > > > using > > > > devm_iio_backend_enable(). I understand that's not suitable for you but I > > > > would > > > > instead rename the existing function to iio_backend_disable() and export it. > > > > > > > > > > Just renaming is not sufficient. The reason is that > > > devm_add_action_or_reset() require an action with action(void *) > > > prototype. So the prototype of iio_backend_disable() has to be changed > > > to void iio_backend_disable(void *back). > > > I placed the same arguments in enable and disable for symmetry, but *dev > > > is not required for time being in disable API. So it can make sense to > > > change iio_backend_disable() prototype. > > > alternatively, we can call __iio_backend_disable() through this API. > > > Please, let me know is you have a preference. > > > > > > > Oh, yes, you're right. I would prefer your later option. Call > > __iio_backend_disable() from __iio_backend_disable() with a proper typed > ? That looks like an infinite loop :) Ahaha, yes it looks. But hopefully you got what I really meant :) - Nuno Sá >
diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c index b950e30018ca..d3db048c086b 100644 --- a/drivers/iio/industrialio-backend.c +++ b/drivers/iio/industrialio-backend.c @@ -166,6 +166,32 @@ int devm_iio_backend_enable(struct device *dev, struct iio_backend *back) } EXPORT_SYMBOL_NS_GPL(devm_iio_backend_enable, IIO_BACKEND); +/** + * iio_backend_enable - Backend enable + * @dev: Consumer device for the backend + * @back: Backend device + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_enable(struct device *dev, struct iio_backend *back) +{ + return iio_backend_op_call(back, enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_enable, IIO_BACKEND); + +/** + * iio_backend_disable - Backend disable + * @dev: Consumer device for the backend + * @back: Backend device + * + */ +void iio_backend_disable(struct device *dev, struct iio_backend *back) +{ + iio_backend_void_op_call(back, disable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_disable, IIO_BACKEND); + /** * iio_backend_data_format_set - Configure the channel data format * @back: Backend device diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h index cff486699054..81277e5b6160 100644 --- a/include/linux/iio/backend.h +++ b/include/linux/iio/backend.h @@ -120,6 +120,8 @@ struct iio_backend_ops { int iio_backend_chan_enable(struct iio_backend *back, unsigned int chan); int iio_backend_chan_disable(struct iio_backend *back, unsigned int chan); int devm_iio_backend_enable(struct device *dev, struct iio_backend *back); +int iio_backend_enable(struct device *dev, struct iio_backend *back); +void iio_backend_disable(struct device *dev, struct iio_backend *back); int iio_backend_data_format_set(struct iio_backend *back, unsigned int chan, const struct iio_backend_data_fmt *data); int iio_backend_data_source_set(struct iio_backend *back, unsigned int chan,
Add iio_backend_disable() and iio_backend_enable() APIs to allow IIO backend consumer to request backend disabling and enabling. Signed-off-by: Olivier Moysan <olivier.moysan@foss.st.com> --- drivers/iio/industrialio-backend.c | 26 ++++++++++++++++++++++++++ include/linux/iio/backend.h | 2 ++ 2 files changed, 28 insertions(+)