[PATCH 0/2] dmaengine: s3c24xx: Fine-tuning for s3c24xx_dma_probe()

[PATCH 0/2] dmaengine: s3c24xx: Fine-tuning for s3c24xx_dma_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 22 Apr 2017 23:14:32 +0200

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Use devm_kcalloc() in s3c24xx_dma_probe()
  Fix a typo in a comment line

 drivers/dma/s3c24xx-dma.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

-- 
2.12.2

[PATCH 1/2] dmaengine: s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 22 Apr 2017 23:00:23 +0200

* A multiplication for the size determination of a memory allocation
  indicated that an array data structure should be processed.
  Thus use the corresponding function "devm_kcalloc".

* Replace the specification of a data structure by a pointer dereference
  to make the corresponding size determination a bit safer according to
  the Linux coding style convention.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/dma/s3c24xx-dma.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
index f04c4702d98b..967229829683 100644
--- a/drivers/dma/s3c24xx-dma.c
+++ b/drivers/dma/s3c24xx-dma.c
@@ -1216,10 +1216,10 @@ static int s3c24xx_dma_probe(struct platform_device *pdev)
 	if (IS_ERR(s3cdma->base))
 		return PTR_ERR(s3cdma->base);
 
-	s3cdma->phy_chans = devm_kzalloc(&pdev->dev,
-					      sizeof(struct s3c24xx_dma_phy) *
-							pdata->num_phy_channels,
-					      GFP_KERNEL);
+	s3cdma->phy_chans = devm_kcalloc(&pdev->dev,
+					 pdata->num_phy_channels,
+					 sizeof(*s3cdma->phy_chans),
+					 GFP_KERNEL);
 	if (!s3cdma->phy_chans)
 		return -ENOMEM;
 
-- 
2.12.2

[PATCH 1/2] dmaengine: s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[Tobias Jakobi]

Hello Markus,


SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sat, 22 Apr 2017 23:00:23 +0200
> 
> * A multiplication for the size determination of a memory allocation
>   indicated that an array data structure should be processed.
>   Thus use the corresponding function "devm_kcalloc".
I have trouble parsing that sentences. This looks like the typical
approach of native german speakers to directly transfer sentence
constructions from German to English. Which, in most cases, doesn't work
or is just plain confusing.

With best wishes,
Tobias


> * Replace the specification of a data structure by a pointer dereference
>   to make the corresponding size determination a bit safer according to
>   the Linux coding style convention.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/dma/s3c24xx-dma.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
> index f04c4702d98b..967229829683 100644
> --- a/drivers/dma/s3c24xx-dma.c
> +++ b/drivers/dma/s3c24xx-dma.c
> @@ -1216,10 +1216,10 @@ static int s3c24xx_dma_probe(struct platform_device *pdev)
>  	if (IS_ERR(s3cdma->base))
>  		return PTR_ERR(s3cdma->base);
>  
> -	s3cdma->phy_chans = devm_kzalloc(&pdev->dev,
> -					      sizeof(struct s3c24xx_dma_phy) *
> -							pdata->num_phy_channels,
> -					      GFP_KERNEL);
> +	s3cdma->phy_chans = devm_kcalloc(&pdev->dev,
> +					 pdata->num_phy_channels,
> +					 sizeof(*s3cdma->phy_chans),
> +					 GFP_KERNEL);
>  	if (!s3cdma->phy_chans)
>  		return -ENOMEM;
>  
> 

s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[SF Markus Elfring]

>> * A multiplication for the size determination of a memory allocation
>>   indicated that an array data structure should be processed.
>>   Thus use the corresponding function "devm_kcalloc".
> I have trouble parsing that sentences. This looks like the typical
> approach of native german speakers to directly transfer sentence
> constructions from German to English. Which, in most cases, doesn't work
> or is just plain confusing.

Do you find the following wording better to understand
if it would be presented by a script like ?checkpatch.pl??

WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply


Regards,
Markus

s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[Tobias Jakobi]

SF Markus Elfring wrote:
>>> * A multiplication for the size determination of a memory allocation
>>>   indicated that an array data structure should be processed.
>>>   Thus use the corresponding function "devm_kcalloc".
>> I have trouble parsing that sentences. This looks like the typical
>> approach of native german speakers to directly transfer sentence
>> constructions from German to English. Which, in most cases, doesn't work
>> or is just plain confusing.
> 
> Do you find the following wording better to understand
> if it would be presented by a script like ?checkpatch.pl??
> 
> WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply
For example. Also I just noticed some previous comment by Krzysztof that
pointed that out already.

My suggestion: One sentence describing that the current situation is.
Another sentence explaining why this is bad/undesirable. Last sentence
saying what you're doing to fix this (can sometimes be omitted if it's
clear from the diffstat). In this case, the output of the checkpatch
script would come in handy.

With this, you avoid cramming every information into one long and
complicated sentence.

With best wishes,
Tobias


> 
> 
> Regards,
> Markus
> 

s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[SF Markus Elfring]

>> WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply
> For example. Also I just noticed some previous comment by Krzysztof that
> pointed that out already.
> 
> My suggestion: One sentence describing that the current situation is.

Why do you find the sentence for the multiplication information inappropriate
(or incomplete) at the moment?


> Another sentence explaining why this is bad/undesirable.

Which details do you miss here?


> In this case, the output of the checkpatch script would come in handy.

Its implementation of the check ?ALLOC_WITH_MULTIPLY? considers only an other
search pattern so far.

* Do you find it worthwhile to add a prefix like ?devm_? to the used
  regular expression?

* Would like to improve any related scripts for the semantic patch language
  (Coccinelle software) a bit more?


> With this, you avoid cramming every information into one long and
> complicated sentence.

Thanks for your feedback about other wording preferences.

Regards,
Markus

s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[Tobias Jakobi]

SF Markus Elfring wrote:
>>> WARNING: Prefer devm_kcalloc over devm_kzalloc with multiply
>> For example. Also I just noticed some previous comment by Krzysztof that
>> pointed that out already.
>>
>> My suggestion: One sentence describing that the current situation is.
> 
> Why do you find the sentence for the multiplication information inappropriate
> (or incomplete) at the moment?
I already explained that. It's a 1:1 translation of a german sentence
into English. A native speaker does not write or speak like that. If in
doubt, don't use long sentences (with nesting, etc.) at all, and break
things down into logical blocks.


>> Another sentence explaining why this is bad/undesirable.
> 
> Which details do you miss here?
Pretty much everything.


>> In this case, the output of the checkpatch script would come in handy.
> 
> Its implementation of the check ?ALLOC_WITH_MULTIPLY? considers only an other
> search pattern so far.
> 
> * Do you find it worthwhile to add a prefix like ?devm_? to the used
>   regular expression?
> 
> * Would like to improve any related scripts for the semantic patch language
>   (Coccinelle software) a bit more?
I don't understand why you're asking this. I'm talking about the
_output_ of checkpatch, not about the script itself.

But undoubtedly your patch is motivated by the output of said tool.
Hence you should mention that.

- Tobias


>> With this, you avoid cramming every information into one long and
>> complicated sentence.
> 
> Thanks for your feedback about other wording preferences.
> 
> Regards,
> Markus
> 

s3c24xx: Use devm_kcalloc() in s3c24xx_dma_probe()

[SF Markus Elfring]

>> Its implementation of the check ?ALLOC_WITH_MULTIPLY? considers only an other
>> search pattern so far.
>>
>> * Do you find it worthwhile to add a prefix like ?devm_? to the used
>>   regular expression?
>>
>> * Would like to improve any related scripts for the semantic patch language
>>   (Coccinelle software) a bit more?
> I don't understand why you're asking this.

Software developers and code reviewers have got different opinions
about such checks and their relevance.


> I'm talking about the _output_ of checkpatch,

This information is clear at first glance.


> not about the script itself.

But it will not provide the warning you might be looking for
while you seem to find my source code analysis approach and
notifications improvable.
I assume that you might be interested in corresponding extensions
for the involved search patterns.


> But undoubtedly your patch is motivated by the output of said tool.

This tool implemented some checks.


> Hence you should mention that.

Additional tools take also care for similar software development concerns,
don't they?

Can it be appropriate to omit the reference to only one Perl script
for related use cases?

Regards,
Markus

[PATCH 2/2] dmaengine: s3c24xx: Fix a typo in a comment line

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sat, 22 Apr 2017 23:09:03 +0200

Add a missing character in this description for a data structure member.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/dma/s3c24xx-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/s3c24xx-dma.c b/drivers/dma/s3c24xx-dma.c
index 967229829683..da79b2c6989f 100644
--- a/drivers/dma/s3c24xx-dma.c
+++ b/drivers/dma/s3c24xx-dma.c
@@ -159,7 +159,7 @@ struct s3c24xx_sg {
  * struct s3c24xx_txd - wrapper for struct dma_async_tx_descriptor
  * @vd: virtual DMA descriptor
  * @dsg_list: list of children sg's
- * @at: sg currently being transfered
+ * @at: sg currently being transferred
  * @width: transfer width
  * @disrcc: value for source control register
  * @didstc: value for destination control register
-- 
2.12.2