diff mbox series

[v2,09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()

Message ID 20200827192642.1725-9-krzk@kernel.org (mailing list archive)
State New, archived
Headers show
Series [v2,01/18] iio: accel: bma180: Simplify with dev_err_probe() | expand

Commit Message

Krzysztof Kozlowski Aug. 27, 2020, 7:26 p.m. UTC
Common pattern of handling deferred probe can be simplified with
dev_err_probe().  Less code and also it prints the error value.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>

---

Changes since v1:
1. Wrap dev_err_probe() lines at 100 character
---
 drivers/iio/afe/iio-rescale.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

Comments

Andy Shevchenko Aug. 27, 2020, 7:54 p.m. UTC | #1
On Thu, Aug 27, 2020 at 10:28 PM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe().  Less code and also it prints the error value.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>
> ---
>
> Changes since v1:
> 1. Wrap dev_err_probe() lines at 100 character
> ---
>  drivers/iio/afe/iio-rescale.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 69c0f277ada0..8cd9645c50e8 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -276,11 +276,8 @@ static int rescale_probe(struct platform_device *pdev)
>         int ret;
>
>         source = devm_iio_channel_get(dev, NULL);
> -       if (IS_ERR(source)) {
> -               if (PTR_ERR(source) != -EPROBE_DEFER)
> -                       dev_err(dev, "failed to get source channel\n");
> -               return PTR_ERR(source);
> -       }
> +       if (IS_ERR(source))
> +               return dev_err_probe(dev, PTR_ERR(source), "failed to get source channel\n");
>
>         sizeof_ext_info = iio_get_channel_ext_info_count(source);
>         if (sizeof_ext_info) {
> --
> 2.17.1
>
Peter Rosin Aug. 27, 2020, 9:46 p.m. UTC | #2
On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
> Common pattern of handling deferred probe can be simplified with
> dev_err_probe().  Less code and also it prints the error value.
> 
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> ---
> 
> Changes since v1:
> 1. Wrap dev_err_probe() lines at 100 character
> ---
>  drivers/iio/afe/iio-rescale.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> index 69c0f277ada0..8cd9645c50e8 100644
> --- a/drivers/iio/afe/iio-rescale.c
> +++ b/drivers/iio/afe/iio-rescale.c
> @@ -276,11 +276,8 @@ static int rescale_probe(struct platform_device *pdev)
>  	int ret;
>  
>  	source = devm_iio_channel_get(dev, NULL);
> -	if (IS_ERR(source)) {
> -		if (PTR_ERR(source) != -EPROBE_DEFER)
> -			dev_err(dev, "failed to get source channel\n");
> -		return PTR_ERR(source);
> -	}
> +	if (IS_ERR(source))
> +		return dev_err_probe(dev, PTR_ERR(source), "failed to get source channel\n");

Hi!

Sorry to be a pain...but...

I'm not a huge fan of adding *one* odd line breaking the 80 column
recommendation to any file. I like to be able to fit multiple
windows side by side in a meaningful way. Also, I don't like having
a shitload of emptiness on my screen, which is what happens when some
lines are longer and you want to see it all. I strongly believe that
the 80 column rule/recommendation is still as valid as it ever was.
It's just hard to read longish lines; there's a reason newspapers
columns are quite narrow...

Same comment for the envelope-detector (3/18).

You will probably never look at these files again, but *I* might have
to revisit them for one reason or another, and these long lines will
annoy me when that happens.

You did wrap the lines for dpot-dac (12/18) - which is excellent - but
that makes me curious as to what the difference is?

Cheers,
Peter

>  
>  	sizeof_ext_info = iio_get_channel_ext_info_count(source);
>  	if (sizeof_ext_info) {
>
Krzysztof Kozlowski Aug. 28, 2020, 6:24 a.m. UTC | #3
On Thu, Aug 27, 2020 at 11:46:40PM +0200, Peter Rosin wrote:
> On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
> > Common pattern of handling deferred probe can be simplified with
> > dev_err_probe().  Less code and also it prints the error value.
> > 
> > Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
> > 
> > ---
> > 
> > Changes since v1:
> > 1. Wrap dev_err_probe() lines at 100 character
> > ---
> >  drivers/iio/afe/iio-rescale.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
> > index 69c0f277ada0..8cd9645c50e8 100644
> > --- a/drivers/iio/afe/iio-rescale.c
> > +++ b/drivers/iio/afe/iio-rescale.c
> > @@ -276,11 +276,8 @@ static int rescale_probe(struct platform_device *pdev)
> >  	int ret;
> >  
> >  	source = devm_iio_channel_get(dev, NULL);
> > -	if (IS_ERR(source)) {
> > -		if (PTR_ERR(source) != -EPROBE_DEFER)
> > -			dev_err(dev, "failed to get source channel\n");
> > -		return PTR_ERR(source);
> > -	}
> > +	if (IS_ERR(source))
> > +		return dev_err_probe(dev, PTR_ERR(source), "failed to get source channel\n");
> 
> Hi!
> 
> Sorry to be a pain...but...
> 
> I'm not a huge fan of adding *one* odd line breaking the 80 column
> recommendation to any file. I like to be able to fit multiple
> windows side by side in a meaningful way. Also, I don't like having
> a shitload of emptiness on my screen, which is what happens when some
> lines are longer and you want to see it all. I strongly believe that
> the 80 column rule/recommendation is still as valid as it ever was.
> It's just hard to read longish lines; there's a reason newspapers
> columns are quite narrow...
> 
> Same comment for the envelope-detector (3/18).
> 
> You will probably never look at these files again, but *I* might have
> to revisit them for one reason or another, and these long lines will
> annoy me when that happens.

Initially I posted it with 80-characters wrap. Then I received a comment
- better to stick to the new 100, as checkpatch accepts it.

Now you write, better to go back to 80.

Maybe then someone else will write to me, better to go to 100.

And another person will reply, no, coding style still mentions 80, so
keep it at 80.

Sure guys, please first decide which one you prefer, then I will wrap it
accordingly. :)

Otherwise I will just jump from one to another depending on one person's
personal preference.

If there is no consensus among discussing people, I find this 100 line
more readable, already got review, checkpatch accepts it so if subsystem
maintainer likes it, I prefer to leave it like this.

> You did wrap the lines for dpot-dac (12/18) - which is excellent - but
> that makes me curious as to what the difference is?

Didn't fit into limit of 100.

Best regards,
Krzysztof
Peter Rosin Aug. 28, 2020, 6:57 a.m. UTC | #4
On 2020-08-28 08:24, Krzysztof Kozlowski wrote:
> On Thu, Aug 27, 2020 at 11:46:40PM +0200, Peter Rosin wrote:
>> On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
>>> Common pattern of handling deferred probe can be simplified with
>>> dev_err_probe().  Less code and also it prints the error value.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
>>>
>>> ---
>>>
>>> Changes since v1:
>>> 1. Wrap dev_err_probe() lines at 100 character
>>> ---
>>>  drivers/iio/afe/iio-rescale.c | 7 ++-----
>>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
>>> index 69c0f277ada0..8cd9645c50e8 100644
>>> --- a/drivers/iio/afe/iio-rescale.c
>>> +++ b/drivers/iio/afe/iio-rescale.c
>>> @@ -276,11 +276,8 @@ static int rescale_probe(struct platform_device *pdev)
>>>  	int ret;
>>>  
>>>  	source = devm_iio_channel_get(dev, NULL);
>>> -	if (IS_ERR(source)) {
>>> -		if (PTR_ERR(source) != -EPROBE_DEFER)
>>> -			dev_err(dev, "failed to get source channel\n");
>>> -		return PTR_ERR(source);
>>> -	}
>>> +	if (IS_ERR(source))
>>> +		return dev_err_probe(dev, PTR_ERR(source), "failed to get source channel\n");
>>
>> Hi!
>>
>> Sorry to be a pain...but...
>>
>> I'm not a huge fan of adding *one* odd line breaking the 80 column
>> recommendation to any file. I like to be able to fit multiple
>> windows side by side in a meaningful way. Also, I don't like having
>> a shitload of emptiness on my screen, which is what happens when some
>> lines are longer and you want to see it all. I strongly believe that
>> the 80 column rule/recommendation is still as valid as it ever was.
>> It's just hard to read longish lines; there's a reason newspapers
>> columns are quite narrow...
>>
>> Same comment for the envelope-detector (3/18).
>>
>> You will probably never look at these files again, but *I* might have
>> to revisit them for one reason or another, and these long lines will
>> annoy me when that happens.
> 
> Initially I posted it with 80-characters wrap. Then I received a comment
> - better to stick to the new 100, as checkpatch accepts it.
> 
> Now you write, better to go back to 80.
> 
> Maybe then someone else will write to me, better to go to 100.
> 
> And another person will reply, no, coding style still mentions 80, so
> keep it at 80.
> 
> Sure guys, please first decide which one you prefer, then I will wrap it
> accordingly. :)
> 
> Otherwise I will just jump from one to another depending on one person's
> personal preference.
> 
> If there is no consensus among discussing people, I find this 100 line
> more readable, already got review, checkpatch accepts it so if subsystem
> maintainer likes it, I prefer to leave it like this.

I'm not impressed by that argument. For the files I have mentioned, it
does not matter very much to me if you and some random person think that
100 columns might *slightly* improve readability.

Quoting coding-style

  Statements longer than 80 columns should be broken into sensible chunks,
  unless exceeding 80 columns significantly increases readability and does
  not hide information.

Notice that word? *significantly*

Why do I even have to speak up about this? WTF?

For the patches that touch files that I originally wrote [1], my
preference should be clear by now.

Cheers,
Peter

[1]

drivers/iio/adc/envelope-detector.c
drivers/iio/afe/iio-rescale.c
drivers/iio/dac/dpot-dac.c
drivers/iio/multiplexer/iio-mux.c

>> You did wrap the lines for dpot-dac (12/18) - which is excellent - but
>> that makes me curious as to what the difference is?
> 
> Didn't fit into limit of 100.
Krzysztof Kozlowski Aug. 28, 2020, 7:03 a.m. UTC | #5
On Fri, 28 Aug 2020 at 08:58, Peter Rosin <peda@axentia.se> wrote:
> >> I'm not a huge fan of adding *one* odd line breaking the 80 column
> >> recommendation to any file. I like to be able to fit multiple
> >> windows side by side in a meaningful way. Also, I don't like having
> >> a shitload of emptiness on my screen, which is what happens when some
> >> lines are longer and you want to see it all. I strongly believe that
> >> the 80 column rule/recommendation is still as valid as it ever was.
> >> It's just hard to read longish lines; there's a reason newspapers
> >> columns are quite narrow...
> >>
> >> Same comment for the envelope-detector (3/18).
> >>
> >> You will probably never look at these files again, but *I* might have
> >> to revisit them for one reason or another, and these long lines will
> >> annoy me when that happens.
> >
> > Initially I posted it with 80-characters wrap. Then I received a comment
> > - better to stick to the new 100, as checkpatch accepts it.
> >
> > Now you write, better to go back to 80.
> >
> > Maybe then someone else will write to me, better to go to 100.
> >
> > And another person will reply, no, coding style still mentions 80, so
> > keep it at 80.
> >
> > Sure guys, please first decide which one you prefer, then I will wrap it
> > accordingly. :)
> >
> > Otherwise I will just jump from one to another depending on one person's
> > personal preference.
> >
> > If there is no consensus among discussing people, I find this 100 line
> > more readable, already got review, checkpatch accepts it so if subsystem
> > maintainer likes it, I prefer to leave it like this.
>
> I'm not impressed by that argument. For the files I have mentioned, it
> does not matter very much to me if you and some random person think that
> 100 columns might *slightly* improve readability.
>
> Quoting coding-style
>
>   Statements longer than 80 columns should be broken into sensible chunks,
>   unless exceeding 80 columns significantly increases readability and does
>   not hide information.
>
> Notice that word? *significantly*

Notice also checkpatch change...

First of all, I don't have a preference over wrapping here. As I said,
I sent v1 with 80 and got a response to change it to 100. You want me
basically to bounce from A to B to A to B.

> Why do I even have to speak up about this? WTF?

Because we all share here our ideas...

> For the patches that touch files that I originally wrote [1], my
> preference should be clear by now.

I understood your preference. There is nothing unclear here. Other
person had different preference. I told you my arguments that it is
not reasonable to jump A->B->A->B just because each person has a
different view. At the end it's the subsystem maintainer's decision as
he wants to keep his subsystem clean.

Best regards,
Krzysztof
Andy Shevchenko Aug. 28, 2020, 8:25 a.m. UTC | #6
On Fri, Aug 28, 2020 at 12:46 AM Peter Rosin <peda@axentia.se> wrote:
> On 2020-08-27 21:26, Krzysztof Kozlowski wrote:

...

> I'm not a huge fan of adding *one* odd line breaking the 80 column
> recommendation to any file. I like to be able to fit multiple
> windows side by side in a meaningful way. Also, I don't like having
> a shitload of emptiness on my screen, which is what happens when some
> lines are longer and you want to see it all. I strongly believe that
> the 80 column rule/recommendation is still as valid as it ever was.
> It's just hard to read longish lines; there's a reason newspapers
> columns are quite narrow...

Why not to introduce 66 (or so, like TeX recommends)? Or even less?
I consider any comparison to news or natural language text is silly.
Programming language is different in many aspects, including helpful
scripts and utilities to browse the source code.
Peter Rosin Aug. 28, 2020, 9:39 a.m. UTC | #7
On 2020-08-28 09:03, Krzysztof Kozlowski wrote:
> On Fri, 28 Aug 2020 at 08:58, Peter Rosin <peda@axentia.se> wrote:
>>>> I'm not a huge fan of adding *one* odd line breaking the 80 column
>>>> recommendation to any file. I like to be able to fit multiple
>>>> windows side by side in a meaningful way. Also, I don't like having
>>>> a shitload of emptiness on my screen, which is what happens when some
>>>> lines are longer and you want to see it all. I strongly believe that
>>>> the 80 column rule/recommendation is still as valid as it ever was.
>>>> It's just hard to read longish lines; there's a reason newspapers
>>>> columns are quite narrow...
>>>>
>>>> Same comment for the envelope-detector (3/18).
>>>>
>>>> You will probably never look at these files again, but *I* might have
>>>> to revisit them for one reason or another, and these long lines will
>>>> annoy me when that happens.
>>>
>>> Initially I posted it with 80-characters wrap. Then I received a comment
>>> - better to stick to the new 100, as checkpatch accepts it.
>>>
>>> Now you write, better to go back to 80.
>>>
>>> Maybe then someone else will write to me, better to go to 100.
>>>
>>> And another person will reply, no, coding style still mentions 80, so
>>> keep it at 80.
>>>
>>> Sure guys, please first decide which one you prefer, then I will wrap it
>>> accordingly. :)
>>>
>>> Otherwise I will just jump from one to another depending on one person's
>>> personal preference.
>>>
>>> If there is no consensus among discussing people, I find this 100 line
>>> more readable, already got review, checkpatch accepts it so if subsystem
>>> maintainer likes it, I prefer to leave it like this.
>>
>> I'm not impressed by that argument. For the files I have mentioned, it
>> does not matter very much to me if you and some random person think that
>> 100 columns might *slightly* improve readability.
>>
>> Quoting coding-style
>>
>>   Statements longer than 80 columns should be broken into sensible chunks,
>>   unless exceeding 80 columns significantly increases readability and does
>>   not hide information.
>>
>> Notice that word? *significantly*
> 
> Notice also checkpatch change...

How is that relevant? checkpatch has *never* had the final say and its
heuristics can never be perfect. Meanwhile, coding style is talking about
exactly the case under discussion, and agrees with me perfectly.

> First of all, I don't have a preference over wrapping here. As I said,
> I sent v1 with 80 and got a response to change it to 100. You want me
> basically to bounce from A to B to A to B.
> 
>> Why do I even have to speak up about this? WTF?
> 
> Because we all share here our ideas...
> 
>> For the patches that touch files that I originally wrote [1], my
>> preference should be clear by now.
> 
> I understood your preference. There is nothing unclear here. Other
> person had different preference. I told you my arguments that it is
> not reasonable to jump A->B->A->B just because each person has a
> different view. At the end it's the subsystem maintainer's decision as
> he wants to keep his subsystem clean.

Yeah, I bet he is thrilled about it.

Cheers,
Peter
Peter Rosin Aug. 28, 2020, 9:39 a.m. UTC | #8
On 2020-08-28 10:25, Andy Shevchenko wrote:
> On Fri, Aug 28, 2020 at 12:46 AM Peter Rosin <peda@axentia.se> wrote:
>> On 2020-08-27 21:26, Krzysztof Kozlowski wrote:
> 
> ...
> 
>> I'm not a huge fan of adding *one* odd line breaking the 80 column
>> recommendation to any file. I like to be able to fit multiple
>> windows side by side in a meaningful way. Also, I don't like having
>> a shitload of emptiness on my screen, which is what happens when some
>> lines are longer and you want to see it all. I strongly believe that
>> the 80 column rule/recommendation is still as valid as it ever was.
>> It's just hard to read longish lines; there's a reason newspapers
>> columns are quite narrow...
> 
> Why not to introduce 66 (or so, like TeX recommends)? Or even less?

Because 80 is what happens to what is recommended in coding style?

> I consider any comparison to news or natural language text is silly.
> Programming language is different in many aspects, including helpful
> scripts and utilities to browse the source code.

So, by all means, scratch that part. You still have a problem getting
around the coding style recommendation.

Cheers,
Peter
Joe Perches Aug. 28, 2020, 5:51 p.m. UTC | #9
On Fri, 2020-08-28 at 11:39 +0200, Peter Rosin wrote:
> On 2020-08-28 09:03, Krzysztof Kozlowski wrote:
> > > > If there is no consensus among discussing people, I find this 100 line
> > > > more readable, already got review, checkpatch accepts it so if subsystem
> > > > maintainer likes it, I prefer to leave it like this.
> > > 
> > > I'm not impressed by that argument. For the files I have mentioned, it
> > > does not matter very much to me if you and some random person think that
> > > 100 columns might *slightly* improve readability.
> > > 
> > > Quoting coding-style
> > > 
> > >   Statements longer than 80 columns should be broken into sensible chunks,
> > >   unless exceeding 80 columns significantly increases readability and does
> > >   not hide information.
> > > 
> > > Notice that word? *significantly*
> > 
> > Notice also checkpatch change...
> 
> How is that relevant? checkpatch has *never* had the final say and its
> heuristics can never be perfect. Meanwhile, coding style is talking about
> exactly the case under discussion, and agrees with me perfectly.

As the checkpatch maintainer, checkpatch is stupid.
Using it as a primary argument should never be acceptable.

But line lengths from 81 to 100 columns should be exceptions
rather than
standard use.

Any named maintainer of actual code determines the style for
that code.

Style consistency and use of kernel standard mechanisms
should be the primary goals here.
diff mbox series

Patch

diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c
index 69c0f277ada0..8cd9645c50e8 100644
--- a/drivers/iio/afe/iio-rescale.c
+++ b/drivers/iio/afe/iio-rescale.c
@@ -276,11 +276,8 @@  static int rescale_probe(struct platform_device *pdev)
 	int ret;
 
 	source = devm_iio_channel_get(dev, NULL);
-	if (IS_ERR(source)) {
-		if (PTR_ERR(source) != -EPROBE_DEFER)
-			dev_err(dev, "failed to get source channel\n");
-		return PTR_ERR(source);
-	}
+	if (IS_ERR(source))
+		return dev_err_probe(dev, PTR_ERR(source), "failed to get source channel\n");
 
 	sizeof_ext_info = iio_get_channel_ext_info_count(source);
 	if (sizeof_ext_info) {