Re: [PATCH 1/3] iio: dummy: convert all simple allocation devm_ variants
On Mon, 7 Dec 2020 09:22:28 +0200
Alexandru Ardelean wrote:
> On Sat, Dec 5, 2020 at 8:46 PM Jonathan Cameron wrote:
> >
> > On Thu, 3 Dec 2020 11:50:03 +0200
> > Alexandru Ardelean wrote:
> >
> > > Since a main requirement for an IIO device is to have a parent device
> > > object, it makes sense to attach more of the IIO device's objects to the
> > > lifetime of the parent object, to clean up the code.
> > > The idea is to also provide a base example that is more up-to-date with
> > > what's going on lately with most IIO drivers.
> >
> > To some degree maybe, it's also very very handy for testing odd corner
> > cases with. I'd definitely not recommend it as a reference driver
> > because it inherently has some odd corners because we need to
> > fake various things that don't exist without hardware.
>
> It's funny because during GSoC it seemed like some people would try to
> use this as a starting point, then shift to another working ADC/DAC
> example.
Hmm. It make sense to use it if you are interested in see what actually
turns up in userspace etc as a gsoc student probably doesn't have much
hardware that is already supported. But code wise it's somewhat odd
and may be less good as an example than a i2c/spi ADC.
> I was also thinking about maybe extending this a bit further, to have
> it a bit more dynamic at load time [being able to load fake IIO
> drivers, load fake data to be read via fake IIO device].
> No idea when this will be ready, but it's on my long todo-list.
Sure. It's always been on my long term list to make this driver do
more complex stuff, but real parts often get in the way unless I'm
proving out something I don't have any real hardware for.
>
> >
> > >
> > > This change tackles the simple allocations, to convert them to
> > > device-managed calls, and tie them to the parent device.
> >
> > I'm confused or maybe I missrecall how this works.
> >
> > IIRC there isn't a parent device...
> >
> > Maybe we could create one via a bit of smoke and magic.
>
> Yep, there isn't one.
> I'll add one through some smoke, magic, some OF and maybe some fake
> i2c/spi device IDs.
Hmm. Whatever is done needs to be both non invasive and not imply
anything that isn't true. Maybe better off with a platform device
than i2c or spi as the parent.
Jonathan
>
> >
> >
> > >
> > > Signed-off-by: Alexandru Ardelean
> > > ---
> > > drivers/iio/dummy/iio_simple_dummy.c | 29
> > > 1 file changed, 8 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/iio/dummy/iio_simple_dummy.c
> > > b/drivers/iio/dummy/iio_simple_dummy.c
> > > index c0b7ef900735..2a2e62f780a1 100644
> > > --- a/drivers/iio/dummy/iio_simple_dummy.c
> > > +++ b/drivers/iio/dummy/iio_simple_dummy.c
> > > @@ -574,11 +574,9 @@ static struct iio_sw_device *iio_dummy_probe(const
> > > char *name)
> > >* parent = >dev;
> > >*/
> > >
> > > - swd = kzalloc(sizeof(*swd), GFP_KERNEL);
> > > - if (!swd) {
> > > - ret = -ENOMEM;
> > > - goto error_kzalloc;
> > > - }
> > > + swd = devm_kzalloc(parent, sizeof(*swd), GFP_KERNEL);
> > > + if (!swd)
> > > + return ERR_PTR(-ENOMEM);
> > > /*
> > >* Allocate an IIO device.
> > >*
> > > @@ -587,11 +585,9 @@ static struct iio_sw_device *iio_dummy_probe(const
> > > char *name)
> > >* It also has a region (accessed by iio_priv()
> > >* for chip specific state information.
> > >*/
> > > - indio_dev = iio_device_alloc(parent, sizeof(*st));
> > > - if (!indio_dev) {
> > > - ret = -ENOMEM;
> > > - goto error_ret;
> > > - }
> > > + indio_dev = devm_iio_device_alloc(parent, sizeof(*st));
> > > + if (!indio_dev)
> > > + return ERR_PTR(-ENOMEM);
> > >
> > > st = iio_priv(indio_dev);
> > > mutex_init(>lock);
> > > @@ -615,7 +611,7 @@ static struct iio_sw_device *iio_dummy_probe(const
> > > char *name)
> > >*indio_dev->name = id->name;
> > >*indio_dev->name = spi_get_device_id(spi)->name;
> > >*/
> > > - indio_dev->name = kstrdup(name, GFP_KERNEL);
> > > + indio_dev->name = devm_kstrdup(parent, name, GFP_KERNEL);
> > >
> > > /* Provide description of available channels */
> > > indio_dev->channels = iio_dummy_channels;
> > > @@ -632,7 +628,7 @@ static struct iio_sw_device *iio_dummy_probe(const
> > > char *name)
> > >
> > > ret = iio_simple_dummy_events_register(indio_dev);
> > > if (ret < 0)
> > > - goto error_free_device;
> > > + return ERR_PTR(ret);
> > >
> > > ret = iio_simple_dummy_configure_buffer(indio_dev);
> > > if (ret < 0)
> > > @@ -649,11 +645,6 @@ static struct iio_sw_device *iio_dummy_probe(const
> > > char *name)
> > > iio_simple_dummy_unconfigure_buffer(indio_dev);
> > > error_unregister_events:
> > > iio_simple_dummy_events_unregister(indio_dev);
Re: [PATCH 1/3] iio: dummy: convert all simple allocation devm_ variants
On Sat, Dec 5, 2020 at 8:46 PM Jonathan Cameron wrote:
>
> On Thu, 3 Dec 2020 11:50:03 +0200
> Alexandru Ardelean wrote:
>
> > Since a main requirement for an IIO device is to have a parent device
> > object, it makes sense to attach more of the IIO device's objects to the
> > lifetime of the parent object, to clean up the code.
> > The idea is to also provide a base example that is more up-to-date with
> > what's going on lately with most IIO drivers.
>
> To some degree maybe, it's also very very handy for testing odd corner
> cases with. I'd definitely not recommend it as a reference driver
> because it inherently has some odd corners because we need to
> fake various things that don't exist without hardware.
It's funny because during GSoC it seemed like some people would try to
use this as a starting point, then shift to another working ADC/DAC
example.
I was also thinking about maybe extending this a bit further, to have
it a bit more dynamic at load time [being able to load fake IIO
drivers, load fake data to be read via fake IIO device].
No idea when this will be ready, but it's on my long todo-list.
>
> >
> > This change tackles the simple allocations, to convert them to
> > device-managed calls, and tie them to the parent device.
>
> I'm confused or maybe I missrecall how this works.
>
> IIRC there isn't a parent device...
>
> Maybe we could create one via a bit of smoke and magic.
Yep, there isn't one.
I'll add one through some smoke, magic, some OF and maybe some fake
i2c/spi device IDs.
>
>
> >
> > Signed-off-by: Alexandru Ardelean
> > ---
> > drivers/iio/dummy/iio_simple_dummy.c | 29
> > 1 file changed, 8 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/iio/dummy/iio_simple_dummy.c
> > b/drivers/iio/dummy/iio_simple_dummy.c
> > index c0b7ef900735..2a2e62f780a1 100644
> > --- a/drivers/iio/dummy/iio_simple_dummy.c
> > +++ b/drivers/iio/dummy/iio_simple_dummy.c
> > @@ -574,11 +574,9 @@ static struct iio_sw_device *iio_dummy_probe(const
> > char *name)
> >* parent = >dev;
> >*/
> >
> > - swd = kzalloc(sizeof(*swd), GFP_KERNEL);
> > - if (!swd) {
> > - ret = -ENOMEM;
> > - goto error_kzalloc;
> > - }
> > + swd = devm_kzalloc(parent, sizeof(*swd), GFP_KERNEL);
> > + if (!swd)
> > + return ERR_PTR(-ENOMEM);
> > /*
> >* Allocate an IIO device.
> >*
> > @@ -587,11 +585,9 @@ static struct iio_sw_device *iio_dummy_probe(const
> > char *name)
> >* It also has a region (accessed by iio_priv()
> >* for chip specific state information.
> >*/
> > - indio_dev = iio_device_alloc(parent, sizeof(*st));
> > - if (!indio_dev) {
> > - ret = -ENOMEM;
> > - goto error_ret;
> > - }
> > + indio_dev = devm_iio_device_alloc(parent, sizeof(*st));
> > + if (!indio_dev)
> > + return ERR_PTR(-ENOMEM);
> >
> > st = iio_priv(indio_dev);
> > mutex_init(>lock);
> > @@ -615,7 +611,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
> > *name)
> >*indio_dev->name = id->name;
> >*indio_dev->name = spi_get_device_id(spi)->name;
> >*/
> > - indio_dev->name = kstrdup(name, GFP_KERNEL);
> > + indio_dev->name = devm_kstrdup(parent, name, GFP_KERNEL);
> >
> > /* Provide description of available channels */
> > indio_dev->channels = iio_dummy_channels;
> > @@ -632,7 +628,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
> > *name)
> >
> > ret = iio_simple_dummy_events_register(indio_dev);
> > if (ret < 0)
> > - goto error_free_device;
> > + return ERR_PTR(ret);
> >
> > ret = iio_simple_dummy_configure_buffer(indio_dev);
> > if (ret < 0)
> > @@ -649,11 +645,6 @@ static struct iio_sw_device *iio_dummy_probe(const
> > char *name)
> > iio_simple_dummy_unconfigure_buffer(indio_dev);
> > error_unregister_events:
> > iio_simple_dummy_events_unregister(indio_dev);
> > -error_free_device:
> > - iio_device_free(indio_dev);
> > -error_ret:
> > - kfree(swd);
> > -error_kzalloc:
> > return ERR_PTR(ret);
> > }
> >
> > @@ -683,10 +674,6 @@ static int iio_dummy_remove(struct iio_sw_device *swd)
> >
> > iio_simple_dummy_events_unregister(indio_dev);
> >
> > - /* Free all structures */
> > - kfree(indio_dev->name);
> > - iio_device_free(indio_dev);
> > -
> > return 0;
> > }
> >
>
Re: [PATCH 1/3] iio: dummy: convert all simple allocation devm_ variants
On Thu, 3 Dec 2020 11:50:03 +0200
Alexandru Ardelean wrote:
> Since a main requirement for an IIO device is to have a parent device
> object, it makes sense to attach more of the IIO device's objects to the
> lifetime of the parent object, to clean up the code.
> The idea is to also provide a base example that is more up-to-date with
> what's going on lately with most IIO drivers.
To some degree maybe, it's also very very handy for testing odd corner
cases with. I'd definitely not recommend it as a reference driver
because it inherently has some odd corners because we need to
fake various things that don't exist without hardware.
>
> This change tackles the simple allocations, to convert them to
> device-managed calls, and tie them to the parent device.
I'm confused or maybe I missrecall how this works.
IIRC there isn't a parent device...
Maybe we could create one via a bit of smoke and magic.
>
> Signed-off-by: Alexandru Ardelean
> ---
> drivers/iio/dummy/iio_simple_dummy.c | 29
> 1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iio/dummy/iio_simple_dummy.c
> b/drivers/iio/dummy/iio_simple_dummy.c
> index c0b7ef900735..2a2e62f780a1 100644
> --- a/drivers/iio/dummy/iio_simple_dummy.c
> +++ b/drivers/iio/dummy/iio_simple_dummy.c
> @@ -574,11 +574,9 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
>* parent = >dev;
>*/
>
> - swd = kzalloc(sizeof(*swd), GFP_KERNEL);
> - if (!swd) {
> - ret = -ENOMEM;
> - goto error_kzalloc;
> - }
> + swd = devm_kzalloc(parent, sizeof(*swd), GFP_KERNEL);
> + if (!swd)
> + return ERR_PTR(-ENOMEM);
> /*
>* Allocate an IIO device.
>*
> @@ -587,11 +585,9 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
>* It also has a region (accessed by iio_priv()
>* for chip specific state information.
>*/
> - indio_dev = iio_device_alloc(parent, sizeof(*st));
> - if (!indio_dev) {
> - ret = -ENOMEM;
> - goto error_ret;
> - }
> + indio_dev = devm_iio_device_alloc(parent, sizeof(*st));
> + if (!indio_dev)
> + return ERR_PTR(-ENOMEM);
>
> st = iio_priv(indio_dev);
> mutex_init(>lock);
> @@ -615,7 +611,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
>*indio_dev->name = id->name;
>*indio_dev->name = spi_get_device_id(spi)->name;
>*/
> - indio_dev->name = kstrdup(name, GFP_KERNEL);
> + indio_dev->name = devm_kstrdup(parent, name, GFP_KERNEL);
>
> /* Provide description of available channels */
> indio_dev->channels = iio_dummy_channels;
> @@ -632,7 +628,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
>
> ret = iio_simple_dummy_events_register(indio_dev);
> if (ret < 0)
> - goto error_free_device;
> + return ERR_PTR(ret);
>
> ret = iio_simple_dummy_configure_buffer(indio_dev);
> if (ret < 0)
> @@ -649,11 +645,6 @@ static struct iio_sw_device *iio_dummy_probe(const char
> *name)
> iio_simple_dummy_unconfigure_buffer(indio_dev);
> error_unregister_events:
> iio_simple_dummy_events_unregister(indio_dev);
> -error_free_device:
> - iio_device_free(indio_dev);
> -error_ret:
> - kfree(swd);
> -error_kzalloc:
> return ERR_PTR(ret);
> }
>
> @@ -683,10 +674,6 @@ static int iio_dummy_remove(struct iio_sw_device *swd)
>
> iio_simple_dummy_events_unregister(indio_dev);
>
> - /* Free all structures */
> - kfree(indio_dev->name);
> - iio_device_free(indio_dev);
> -
> return 0;
> }
>
[PATCH 1/3] iio: dummy: convert all simple allocation devm_ variants
Since a main requirement for an IIO device is to have a parent device
object, it makes sense to attach more of the IIO device's objects to the
lifetime of the parent object, to clean up the code.
The idea is to also provide a base example that is more up-to-date with
what's going on lately with most IIO drivers.
This change tackles the simple allocations, to convert them to
device-managed calls, and tie them to the parent device.
Signed-off-by: Alexandru Ardelean
---
drivers/iio/dummy/iio_simple_dummy.c | 29
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/drivers/iio/dummy/iio_simple_dummy.c
b/drivers/iio/dummy/iio_simple_dummy.c
index c0b7ef900735..2a2e62f780a1 100644
--- a/drivers/iio/dummy/iio_simple_dummy.c
+++ b/drivers/iio/dummy/iio_simple_dummy.c
@@ -574,11 +574,9 @@ static struct iio_sw_device *iio_dummy_probe(const char
*name)
* parent = >dev;
*/
- swd = kzalloc(sizeof(*swd), GFP_KERNEL);
- if (!swd) {
- ret = -ENOMEM;
- goto error_kzalloc;
- }
+ swd = devm_kzalloc(parent, sizeof(*swd), GFP_KERNEL);
+ if (!swd)
+ return ERR_PTR(-ENOMEM);
/*
* Allocate an IIO device.
*
@@ -587,11 +585,9 @@ static struct iio_sw_device *iio_dummy_probe(const char
*name)
* It also has a region (accessed by iio_priv()
* for chip specific state information.
*/
- indio_dev = iio_device_alloc(parent, sizeof(*st));
- if (!indio_dev) {
- ret = -ENOMEM;
- goto error_ret;
- }
+ indio_dev = devm_iio_device_alloc(parent, sizeof(*st));
+ if (!indio_dev)
+ return ERR_PTR(-ENOMEM);
st = iio_priv(indio_dev);
mutex_init(>lock);
@@ -615,7 +611,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
*name)
*indio_dev->name = id->name;
*indio_dev->name = spi_get_device_id(spi)->name;
*/
- indio_dev->name = kstrdup(name, GFP_KERNEL);
+ indio_dev->name = devm_kstrdup(parent, name, GFP_KERNEL);
/* Provide description of available channels */
indio_dev->channels = iio_dummy_channels;
@@ -632,7 +628,7 @@ static struct iio_sw_device *iio_dummy_probe(const char
*name)
ret = iio_simple_dummy_events_register(indio_dev);
if (ret < 0)
- goto error_free_device;
+ return ERR_PTR(ret);
ret = iio_simple_dummy_configure_buffer(indio_dev);
if (ret < 0)
@@ -649,11 +645,6 @@ static struct iio_sw_device *iio_dummy_probe(const char
*name)
iio_simple_dummy_unconfigure_buffer(indio_dev);
error_unregister_events:
iio_simple_dummy_events_unregister(indio_dev);
-error_free_device:
- iio_device_free(indio_dev);
-error_ret:
- kfree(swd);
-error_kzalloc:
return ERR_PTR(ret);
}
@@ -683,10 +674,6 @@ static int iio_dummy_remove(struct iio_sw_device *swd)
iio_simple_dummy_events_unregister(indio_dev);
- /* Free all structures */
- kfree(indio_dev->name);
- iio_device_free(indio_dev);
-
return 0;
}
--
2.27.0
