diff mbox

[08/14] ieee802154: mrf24j40: fix some kernel coding style errors

Message ID 20170922121405.31789-9-stefan@osg.samsung.com
State Rejected
Headers show

Commit Message

Stefan Schmidt Sept. 22, 2017, 12:13 p.m. UTC
Fix format of multi-line comments and long lines.

Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
---
 drivers/net/ieee802154/mrf24j40.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Comments

Alan Ott Sept. 22, 2017, 2:12 p.m. UTC | #1
Hi Stefan, it's good to hear from you.

On 09/22/2017 08:13 AM, Stefan Schmidt wrote:
> Fix format of multi-line comments and long lines.
>
> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>

Overall, it's pretty churny. More below:

> ---
>  drivers/net/ieee802154/mrf24j40.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
> index ee7084b2d52d..e951a191e15d 100644
> --- a/drivers/net/ieee802154/mrf24j40.c
> +++ b/drivers/net/ieee802154/mrf24j40.c
> @@ -569,8 +569,9 @@ static void write_tx_buf_complete(void *context)
>  }
>
>  /* This function relies on an undocumented write method. Once a write command
> -   and address is set, as many bytes of data as desired can be clocked into
> -   the device. The datasheet only shows setting one byte at a time. */
> + * and address is set, as many bytes of data as desired can be clocked into
> + * the device. The datasheet only shows setting one byte at a time.
> + */
>  static int write_tx_buf(struct mrf24j40 *devrec, u16 reg,
>  			const u8 *data, size_t length)
>  {
> @@ -578,9 +579,10 @@ static int write_tx_buf(struct mrf24j40 *devrec, u16 reg,
>  	int ret;
>
>  	/* Range check the length. 2 bytes are used for the length fields.*/
> -	if (length > TX_FIFO_SIZE-2) {
> -		dev_err(printdev(devrec), "write_tx_buf() was passed too large a buffer. Performing short write.\n");
> -		length = TX_FIFO_SIZE-2;
> +	if (length > TX_FIFO_SIZE - 2) {
> +		dev_err(printdev(devrec), "%s: passed buffer to large, short write\n",
> +			__func__);
> +		length = TX_FIFO_SIZE - 2;
>  	}

Not breaking printouts is a case where long-ish lines are ok. Changing 
the output to be less clear and useful just to make the line short is a 
regression in my mind.

>
>  	cmd = MRF24J40_WRITELONG(reg);
> @@ -1073,7 +1075,8 @@ static int mrf24j40_hw_init(struct mrf24j40 *devrec)
>  	int ret;
>
>  	/* Initialize the device.
> -		From datasheet section 3.2: Initialization. */
> +	 * From datasheet section 3.2: Initialization.
> +	 */
>  	ret = regmap_write(devrec->regmap_short, REG_SOFTRST, 0x07);
>  	if (ret)
>  		goto err_ret;
> @@ -1374,7 +1377,8 @@ static int mrf24j40_remove(struct spi_device *spi)
>  	ieee802154_unregister_hw(devrec->hw);
>  	ieee802154_free_hw(devrec->hw);
>  	/* TODO: Will ieee802154_free_device() wait until ->xmit() is
> -	 * complete? */
> +	 * complete?
> +	 */
>
>  	return 0;
>  }
>

Regarding the comment format, adding extra lines for */ on a two-line 
comment is a bit overkill.

Sorry to be this way, but I just don't agree with this patch. It just 
feels like churn to me.

Alan.

--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stefan Schmidt Sept. 22, 2017, 2:38 p.m. UTC | #2
Hello.

On 09/22/2017 04:12 PM, Alan Ott wrote:
> Hi Stefan, it's good to hear from you.

Same goes to you. It has been a while. Pretty fast turnaround time for mrf24 patches this time. :P

> On 09/22/2017 08:13 AM, Stefan Schmidt wrote:
>> Fix format of multi-line comments and long lines.
>>
>> Signed-off-by: Stefan Schmidt <stefan@osg.samsung.com>
> 
> Overall, it's pretty churny. More below:
> 
>> ---
>>  drivers/net/ieee802154/mrf24j40.c | 18 +++++++++++-------
>>  1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
>> index ee7084b2d52d..e951a191e15d 100644
>> --- a/drivers/net/ieee802154/mrf24j40.c
>> +++ b/drivers/net/ieee802154/mrf24j40.c
>> @@ -569,8 +569,9 @@ static void write_tx_buf_complete(void *context)
>>  }
>>
>>  /* This function relies on an undocumented write method. Once a write command
>> -   and address is set, as many bytes of data as desired can be clocked into
>> -   the device. The datasheet only shows setting one byte at a time. */
>> + * and address is set, as many bytes of data as desired can be clocked into
>> + * the device. The datasheet only shows setting one byte at a time.
>> + */
>>  static int write_tx_buf(struct mrf24j40 *devrec, u16 reg,
>>  			const u8 *data, size_t length)
>>  {
>> @@ -578,9 +579,10 @@ static int write_tx_buf(struct mrf24j40 *devrec, u16 reg,
>>  	int ret;
>>
>>  	/* Range check the length. 2 bytes are used for the length fields.*/
>> -	if (length > TX_FIFO_SIZE-2) {
>> -		dev_err(printdev(devrec), "write_tx_buf() was passed too large a buffer. Performing short write.\n");
>> -		length = TX_FIFO_SIZE-2;
>> +	if (length > TX_FIFO_SIZE - 2) {
>> +		dev_err(printdev(devrec), "%s: passed buffer to large, short write\n",
>> +			__func__);
>> +		length = TX_FIFO_SIZE - 2;
>>  	}
> 
> Not breaking printouts is a case where long-ish lines are ok. Changing 
> the output to be less clear and useful just to make the line short is a 
> regression in my mind.

You are the native speaker but if I compare the two sentences they do not really look less clear and useful to me.
But well, different perceptions. :)

One thing I wondered when looking at this hunk was if this case can actually happen? Can we really get a length here greater than
TX_FIFO_SIZE-2?

>>
>>  	cmd = MRF24J40_WRITELONG(reg);
>> @@ -1073,7 +1075,8 @@ static int mrf24j40_hw_init(struct mrf24j40 *devrec)
>>  	int ret;
>>
>>  	/* Initialize the device.
>> -		From datasheet section 3.2: Initialization. */
>> +	 * From datasheet section 3.2: Initialization.
>> +	 */
>>  	ret = regmap_write(devrec->regmap_short, REG_SOFTRST, 0x07);
>>  	if (ret)
>>  		goto err_ret;
>> @@ -1374,7 +1377,8 @@ static int mrf24j40_remove(struct spi_device *spi)
>>  	ieee802154_unregister_hw(devrec->hw);
>>  	ieee802154_free_hw(devrec->hw);
>>  	/* TODO: Will ieee802154_free_device() wait until ->xmit() is
>> -	 * complete? */
>> +	 * complete?
>> +	 */
>>
>>  	return 0;
>>  }
>>
> 
> Regarding the comment format, adding extra lines for */ on a two-line 
> comment is a bit overkill.

Well, it simply is the kernel coding style. Imagine multi-line comments outside of net/ and drivers/net/ also have an /* on a new line at
the beginning. :-)

> Sorry to be this way, but I just don't agree with this patch. It just 
> feels like churn to me.

Given the tiny amount of patches mrf24 sees over time I would not go as far as calling this churn as it is highly unlikely to conflict with
anything else pending.

I do not mind dropping this patch if you do not want it, though. No problem from my side.

regards
Stefan Schmidt
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ieee802154/mrf24j40.c b/drivers/net/ieee802154/mrf24j40.c
index ee7084b2d52d..e951a191e15d 100644
--- a/drivers/net/ieee802154/mrf24j40.c
+++ b/drivers/net/ieee802154/mrf24j40.c
@@ -569,8 +569,9 @@  static void write_tx_buf_complete(void *context)
 }
 
 /* This function relies on an undocumented write method. Once a write command
-   and address is set, as many bytes of data as desired can be clocked into
-   the device. The datasheet only shows setting one byte at a time. */
+ * and address is set, as many bytes of data as desired can be clocked into
+ * the device. The datasheet only shows setting one byte at a time.
+ */
 static int write_tx_buf(struct mrf24j40 *devrec, u16 reg,
 			const u8 *data, size_t length)
 {
@@ -578,9 +579,10 @@  static int write_tx_buf(struct mrf24j40 *devrec, u16 reg,
 	int ret;
 
 	/* Range check the length. 2 bytes are used for the length fields.*/
-	if (length > TX_FIFO_SIZE-2) {
-		dev_err(printdev(devrec), "write_tx_buf() was passed too large a buffer. Performing short write.\n");
-		length = TX_FIFO_SIZE-2;
+	if (length > TX_FIFO_SIZE - 2) {
+		dev_err(printdev(devrec), "%s: passed buffer to large, short write\n",
+			__func__);
+		length = TX_FIFO_SIZE - 2;
 	}
 
 	cmd = MRF24J40_WRITELONG(reg);
@@ -1073,7 +1075,8 @@  static int mrf24j40_hw_init(struct mrf24j40 *devrec)
 	int ret;
 
 	/* Initialize the device.
-		From datasheet section 3.2: Initialization. */
+	 * From datasheet section 3.2: Initialization.
+	 */
 	ret = regmap_write(devrec->regmap_short, REG_SOFTRST, 0x07);
 	if (ret)
 		goto err_ret;
@@ -1374,7 +1377,8 @@  static int mrf24j40_remove(struct spi_device *spi)
 	ieee802154_unregister_hw(devrec->hw);
 	ieee802154_free_hw(devrec->hw);
 	/* TODO: Will ieee802154_free_device() wait until ->xmit() is
-	 * complete? */
+	 * complete?
+	 */
 
 	return 0;
 }