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@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
