| Message ID | 20170922121405.31789-9-stefan@osg.samsung.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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
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 --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; }
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(-)