[PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Aishwarya R]

Use the helper function that wraps the calls to
platform_get_resource() and devm_ioremap_resource()
together.

Signed-off-by: Aishwarya R <aishwaryarj100@gmail.com>
---
 drivers/iio/adc/at91_adc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index abe99856c823..0368b6dc6d60 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -1152,7 +1152,6 @@ static int at91_adc_probe(struct platform_device *pdev)
 	int ret;
 	struct iio_dev *idev;
 	struct at91_adc_state *st;
-	struct resource *res;
 	u32 reg;
 
 	idev = devm_iio_device_alloc(&pdev->dev, sizeof(struct at91_adc_state));
@@ -1182,9 +1181,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 	if (st->irq < 0)
 		return -ENODEV;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	st->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	st->reg_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(st->reg_base))
 		return PTR_ERR(st->reg_base);
 
-- 
2.17.1

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Alexandre Belloni]

On 09/04/2020 20:41:23+0530, Aishwarya R wrote:
> Use the helper function that wraps the calls to
> platform_get_resource() and devm_ioremap_resource()
> together.
> 

Please elaborate the actual value of doing that.

> Signed-off-by: Aishwarya R <aishwaryarj100@gmail.com>
> ---
>  drivers/iio/adc/at91_adc.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index abe99856c823..0368b6dc6d60 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -1152,7 +1152,6 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	int ret;
>  	struct iio_dev *idev;
>  	struct at91_adc_state *st;
> -	struct resource *res;
>  	u32 reg;
>  
>  	idev = devm_iio_device_alloc(&pdev->dev, sizeof(struct at91_adc_state));
> @@ -1182,9 +1181,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	if (st->irq < 0)
>  		return -ENODEV;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -	st->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	st->reg_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(st->reg_base))
>  		return PTR_ERR(st->reg_base);
>  
> -- 
> 2.17.1
> 

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Aishwarya R]

Use helper function to simply the code.
The devm_platform_ioremap_resource() helper in the
kernel which helps to make code cleaner a tad by
replacing few "boilerplate" lines of code with a single line.

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Andy Shevchenko]

On Thu, Apr 9, 2020 at 7:00 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 09/04/2020 20:41:23+0530, Aishwarya R wrote:
> > Use the helper function that wraps the calls to
> > platform_get_resource() and devm_ioremap_resource()
> > together.

> Please elaborate the actual value of doing that.

Please, elaborate actual value of not doing that.

Yes, I know that you are p* off of these changes, but why you not
going further and forbid all clean ups we are doing in the code?

To the point. Above change is reducing code base and showing the new
comers modern APIs to use.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Alexandre Belloni]

On 10/04/2020 13:55:26+0300, Andy Shevchenko wrote:
> On Thu, Apr 9, 2020 at 7:00 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 09/04/2020 20:41:23+0530, Aishwarya R wrote:
> > > Use the helper function that wraps the calls to
> > > platform_get_resource() and devm_ioremap_resource()
> > > together.
> 
> > Please elaborate the actual value of doing that.
> 
> Please, elaborate actual value of not doing that.
> 
> Yes, I know that you are p* off of these changes, but why you not
> going further and forbid all clean ups we are doing in the code?
> 
> To the point. Above change is reducing code base and showing the new
> comers modern APIs to use.
> 

The value of doing it is to reduce the code size by 16 bytes. The same
people doing that will actively ruin that by adding error string for
error that will never ever happen.

Also, the commit message is definitively lacking. A good commit message
would say that the patch has been generated using coccinelle, that no
testing has been done and that no thought has been given.

It would definitively make sense to send one patch per subsystem instead
of having 475 different patches each changing only one location.

The whole "let's let newcomers fix trivial bugs" thing is definitively
not working and it is not leading to an increase of the number of useful
reviewers and contributors

-- 
Alexandre Belloni, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Andy Shevchenko]

On Fri, Apr 10, 2020 at 2:22 PM Alexandre Belloni
<alexandre.belloni@bootlin.com> wrote:
> On 10/04/2020 13:55:26+0300, Andy Shevchenko wrote:
> > On Thu, Apr 9, 2020 at 7:00 PM Alexandre Belloni
> > <alexandre.belloni@bootlin.com> wrote:
> > > On 09/04/2020 20:41:23+0530, Aishwarya R wrote:
> > > > Use the helper function that wraps the calls to
> > > > platform_get_resource() and devm_ioremap_resource()
> > > > together.
> >
> > > Please elaborate the actual value of doing that.
> >
> > Please, elaborate actual value of not doing that.
> >
> > Yes, I know that you are p* off of these changes, but why you not
> > going further and forbid all clean ups we are doing in the code?
> >
> > To the point. Above change is reducing code base and showing the new
> > comers modern APIs to use.

> The value of doing it is to reduce the code size by 16 bytes. The same
> people doing that will actively ruin that by adding error string for
> error that will never ever happen.

I don't see it in the patch we are discussing, so, not an argument.

> Also, the commit message is definitively lacking. A good commit message
> would say that the patch has been generated using coccinelle, that no
> testing has been done and that no thought has been given.

That's I agree with.
But somebody need to teach people how to do it better.

> It would definitively make sense to send one patch per subsystem instead
> of having 475 different patches each changing only one location.

Depends on the maintainer and subsystem. This is arguable argument.
In my subsystems (let's forget about PDx86, where one per subsystem in
principle is not working by nature of the subsystem, but consider
others I'm maintaining) I prefer to have a possibility to track
changes independently on driver basis.

> The whole "let's let newcomers fix trivial bugs" thing is definitively
> not working and it is not leading to an increase of the number of useful
> reviewers and contributors

Semi-agree. That people can be self-organized into a reviewer gang and
try to learn together.
In any case, as for maintainers, the task has not only technical
aspects, but mentoring as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Jonathan Cameron]

On Thu,  9 Apr 2020 20:41:23 +0530
Aishwarya R <aishwaryarj100@gmail.com> wrote:

> Use the helper function that wraps the calls to
> platform_get_resource() and devm_ioremap_resource()
> together.
> 
> Signed-off-by: Aishwarya R <aishwaryarj100@gmail.com>
Hi Aishwarya R,

Thanks for the patch.  Good to add a little more detail to the description
and note that I do prefer one patch per driver for IIO drivers.

One other thing.  Please use full name for the signed-off-by.
I see you use a longer form in your email later in the thread.
The DCO is a formal legal statement 
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin
so we tend to be fussy!

Thanks,

Jonathan


> ---
>  drivers/iio/adc/at91_adc.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index abe99856c823..0368b6dc6d60 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -1152,7 +1152,6 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	int ret;
>  	struct iio_dev *idev;
>  	struct at91_adc_state *st;
> -	struct resource *res;
>  	u32 reg;
>  
>  	idev = devm_iio_device_alloc(&pdev->dev, sizeof(struct at91_adc_state));
> @@ -1182,9 +1181,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	if (st->irq < 0)
>  		return -ENODEV;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -	st->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	st->reg_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(st->reg_base))
>  		return PTR_ERR(st->reg_base);
>  

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Jonathan Cameron]

On Fri, 10 Apr 2020 19:37:25 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Fri, Apr 10, 2020 at 2:22 PM Alexandre Belloni
> <alexandre.belloni@bootlin.com> wrote:
> > On 10/04/2020 13:55:26+0300, Andy Shevchenko wrote:  
> > > On Thu, Apr 9, 2020 at 7:00 PM Alexandre Belloni
> > > <alexandre.belloni@bootlin.com> wrote:  
> > > > On 09/04/2020 20:41:23+0530, Aishwarya R wrote:  
> > > > > Use the helper function that wraps the calls to
> > > > > platform_get_resource() and devm_ioremap_resource()
> > > > > together.  
> > >  
> > > > Please elaborate the actual value of doing that.  
> > >
> > > Please, elaborate actual value of not doing that.
> > >
> > > Yes, I know that you are p* off of these changes, but why you not
> > > going further and forbid all clean ups we are doing in the code?
> > >
> > > To the point. Above change is reducing code base and showing the new
> > > comers modern APIs to use.  
> 
> > The value of doing it is to reduce the code size by 16 bytes. The same
> > people doing that will actively ruin that by adding error string for
> > error that will never ever happen.  
> 
> I don't see it in the patch we are discussing, so, not an argument.
> 
> > Also, the commit message is definitively lacking. A good commit message
> > would say that the patch has been generated using coccinelle, that no
> > testing has been done and that no thought has been given.  
> 
> That's I agree with.
> But somebody need to teach people how to do it better.
> 
> > It would definitively make sense to send one patch per subsystem instead
> > of having 475 different patches each changing only one location.  
> 
> Depends on the maintainer and subsystem. This is arguable argument.
> In my subsystems (let's forget about PDx86, where one per subsystem in
> principle is not working by nature of the subsystem, but consider
> others I'm maintaining) I prefer to have a possibility to track
> changes independently on driver basis.

Likewise.  I often request patches are broken out.

> 
> > The whole "let's let newcomers fix trivial bugs" thing is definitively
> > not working and it is not leading to an increase of the number of useful
> > reviewers and contributors  
> 
> Semi-agree. That people can be self-organized into a reviewer gang and
> try to learn together.
> In any case, as for maintainers, the task has not only technical
> aspects, but mentoring as well.
> 

It would be interesting to see if we do have examples of people who have started
with cleanup patches and moved on to more interesting things.  Some of the
outreach programs that start with simple patches as a means of getting
familiar with the tools have led to more significant improvements but
those are somewhat of a special case.

I'd rather have hundreds of people send me these sorts of patches a year
than loose one potential long term contributor.

Jonathan

p.s. Aishwarya - You've been a bit unlucky in a sense here running into
the latest instance of a very long term debate!

[PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Aishwarya R]

From: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>

Use the helper function that wraps the calls to
platform_get_resource() and devm_ioremap_resource()
together. It reduces boilerplate and suggested by coccinelle.

Signed-off-by: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>
---
 drivers/iio/adc/at91_adc.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index abe99856c823..0368b6dc6d60 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -1152,7 +1152,6 @@ static int at91_adc_probe(struct platform_device *pdev)
 	int ret;
 	struct iio_dev *idev;
 	struct at91_adc_state *st;
-	struct resource *res;
 	u32 reg;
 
 	idev = devm_iio_device_alloc(&pdev->dev, sizeof(struct at91_adc_state));
@@ -1182,9 +1181,7 @@ static int at91_adc_probe(struct platform_device *pdev)
 	if (st->irq < 0)
 		return -ENODEV;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-
-	st->reg_base = devm_ioremap_resource(&pdev->dev, res);
+	st->reg_base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(st->reg_base))
 		return PTR_ERR(st->reg_base);
 
-- 
2.17.1

Re: [PATCH] iio: adc: at91-adc: Use devm_platform_ioremap_resource

[Jonathan Cameron]

On Sun, 12 Apr 2020 19:26:42 +0530
Aishwarya R <aishwaryarj100@gmail.com> wrote:

> From: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>
> 
> Use the helper function that wraps the calls to
> platform_get_resource() and devm_ioremap_resource()
> together. It reduces boilerplate and suggested by coccinelle.
> 
> Signed-off-by: Aishwarya Ramakrishnan <aishwaryarj100@gmail.com>
Same comments as the previous patch.  Applied anyway (this time)
mostly because it's quicker for me than insisting on a v3.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/iio/adc/at91_adc.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index abe99856c823..0368b6dc6d60 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -1152,7 +1152,6 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	int ret;
>  	struct iio_dev *idev;
>  	struct at91_adc_state *st;
> -	struct resource *res;
>  	u32 reg;
>  
>  	idev = devm_iio_device_alloc(&pdev->dev, sizeof(struct at91_adc_state));
> @@ -1182,9 +1181,7 @@ static int at91_adc_probe(struct platform_device *pdev)
>  	if (st->irq < 0)
>  		return -ENODEV;
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -
> -	st->reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	st->reg_base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(st->reg_base))
>  		return PTR_ERR(st->reg_base);
>