Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
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.
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-28 09:03, Krzysztof Kozlowski wrote: > On Fri, 28 Aug 2020 at 08:58, Peter Rosin 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
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On 2020-08-28 10:25, Andy Shevchenko wrote: > On Fri, Aug 28, 2020 at 12:46 AM Peter Rosin 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
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On Fri, Aug 28, 2020 at 12:46 AM Peter Rosin 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. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On Fri, 28 Aug 2020 at 08:58, Peter Rosin 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
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
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
>>>
>>> ---
>>>
>>> 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.
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
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
> >
> > ---
> >
> > 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
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
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
>
> ---
>
> 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) {
>
Re: [PATCH v2 09/18] iio: afe: iio-rescale: Simplify with dev_err_probe()
On Thu, Aug 27, 2020 at 10:28 PM 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.
Reviewed-by: Andy Shevchenko
> Signed-off-by: Krzysztof Kozlowski
>
> ---
>
> 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
>
--
With Best Regards,
Andy Shevchenko
