| Message ID | 1371196742.20321.1.camel@phoenix |
|---|---|
| State | Superseded |
| Delegated to: | Jagannadha Sutradharudu Teki |
| Headers | show |
Hi, IMHO: Can you please use the proper commit header prefix. I am just sharing my thoughts, ignore this if you know it already. I followed below syntax. "<main_module>: <sub_module>: <sub_sub_module>: <COMMIT_HEADER>" Ex: for this commit (seems like this commit changes two drivers) spi: cf_qspi | mxc_spi: Use DIV_ROUND_UP at appropriate places if you send same changes individually. spi: cf_qspi: Use DIV_ROUND_UP at appropriate places spi: mxc_spi: Use DIV_ROUND_UP at appropriate places On Fri, Jun 14, 2013 at 1:29 PM, Axel Lin <axel.lin@ingics.com> wrote: > This change slightly improves readability. > > Signed-off-by: Axel Lin <axel.lin@ingics.com> > --- > drivers/spi/cf_qspi.c | 2 +- > drivers/spi/mxc_spi.c | 6 +++--- > 2 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/spi/cf_qspi.c b/drivers/spi/cf_qspi.c > index a37ac4e..06bcf91 100644 > --- a/drivers/spi/cf_qspi.c > +++ b/drivers/spi/cf_qspi.c > @@ -171,7 +171,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, > volatile qspi_t *qspi = dev->regs; > u8 *txbuf = (u8 *)dout; > u8 *rxbuf = (u8 *)din; > - u32 count = ((bitlen / 8) + (bitlen % 8 ? 1 : 0)); Was this equivalent to (bitlen + 7) / 8 ? -- Thanks, Jagan.
2013/6/14 Jagan Teki <jagannadh.teki@gmail.com>: > Hi, > > IMHO: > Can you please use the proper commit header prefix. > > I am just sharing my thoughts, ignore this if you know it already. > I followed below syntax. > "<main_module>: <sub_module>: <sub_sub_module>: <COMMIT_HEADER>" > > Ex: for this commit (seems like this commit changes two drivers) > spi: cf_qspi | mxc_spi: Use DIV_ROUND_UP at appropriate places This does not scale. What if a (trivial) patch touches 10 drivers? > > if you send same changes individually. > spi: cf_qspi: Use DIV_ROUND_UP at appropriate places > spi: mxc_spi: Use DIV_ROUND_UP at appropriate places I was thinking doing so may add maintainer's burden. (well for this case with 2 patches, it's not a problem) But since you prefer sending a fix per driver, I'll resend the patches. > > On Fri, Jun 14, 2013 at 1:29 PM, Axel Lin <axel.lin@ingics.com> wrote: >> This change slightly improves readability. >> >> Signed-off-by: Axel Lin <axel.lin@ingics.com> >> --- >> drivers/spi/cf_qspi.c | 2 +- >> drivers/spi/mxc_spi.c | 6 +++--- >> 2 files changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/spi/cf_qspi.c b/drivers/spi/cf_qspi.c >> index a37ac4e..06bcf91 100644 >> --- a/drivers/spi/cf_qspi.c >> +++ b/drivers/spi/cf_qspi.c >> @@ -171,7 +171,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, >> volatile qspi_t *qspi = dev->regs; >> u8 *txbuf = (u8 *)dout; >> u8 *rxbuf = (u8 *)din; >> - u32 count = ((bitlen / 8) + (bitlen % 8 ? 1 : 0)); > > Was this equivalent to (bitlen + 7) / 8 ? Yes. And that is the point of this patch ( to handle divide-round-up in a uniform way). > > -- > Thanks, > Jagan.
On Fri, Jun 14, 2013 at 6:39 PM, Axel Lin <axel.lin@ingics.com> wrote: > 2013/6/14 Jagan Teki <jagannadh.teki@gmail.com>: >> Hi, >> >> IMHO: >> Can you please use the proper commit header prefix. >> >> I am just sharing my thoughts, ignore this if you know it already. >> I followed below syntax. >> "<main_module>: <sub_module>: <sub_sub_module>: <COMMIT_HEADER>" >> >> Ex: for this commit (seems like this commit changes two drivers) >> spi: cf_qspi | mxc_spi: Use DIV_ROUND_UP at appropriate places > > This does not scale. > What if a (trivial) patch touches 10 drivers? Yes, you are correct if the changes is common to across the all drivers or If the changes which are more in 10 spi drivers out of 20 or > 10 for your example - use "spi: " and I think it's good to use above syntax as you changesd two drivers. I am just showing the possibilities of using the coding guidelines, nothing more. -- Thanks, Jagan.
On 14/06/13 09:09 AM, Axel Lin wrote: > 2013/6/14 Jagan Teki<jagannadh.teki@gmail.com>: >> Hi, >> >> IMHO: >> Can you please use the proper commit header prefix. >> >> I am just sharing my thoughts, ignore this if you know it already. >> I followed below syntax. >> "<main_module>:<sub_module>:<sub_sub_module>:<COMMIT_HEADER>" >> >> Ex: for this commit (seems like this commit changes two drivers) >> spi: cf_qspi | mxc_spi: Use DIV_ROUND_UP at appropriate places > > This does not scale. > What if a (trivial) patch touches 10 drivers? Depends how trivial it is I guess. I can see benefits to both approaches. This one is simple enough that I am okay if it is grouped. Just for my own education, how do I correctly ACK just the part I know (mcf_qspi in this case) in a grouped patch? Is there a way to automate my "Signed-off-by:" via e-mail? -- Richard Retanubun > >> >> if you send same changes individually. >> spi: cf_qspi: Use DIV_ROUND_UP at appropriate places >> spi: mxc_spi: Use DIV_ROUND_UP at appropriate places > > I was thinking doing so may add maintainer's burden. (well for this > case with 2 patches, it's not a problem) > But since you prefer sending a fix per driver, I'll resend the patches. > >> >> On Fri, Jun 14, 2013 at 1:29 PM, Axel Lin<axel.lin@ingics.com> wrote: >>> This change slightly improves readability. >>> >>> Signed-off-by: Axel Lin<axel.lin@ingics.com> >>> --- >>> drivers/spi/cf_qspi.c | 2 +- >>> drivers/spi/mxc_spi.c | 6 +++--- >>> 2 files changed, 4 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/spi/cf_qspi.c b/drivers/spi/cf_qspi.c >>> index a37ac4e..06bcf91 100644 >>> --- a/drivers/spi/cf_qspi.c >>> +++ b/drivers/spi/cf_qspi.c >>> @@ -171,7 +171,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, >>> volatile qspi_t *qspi = dev->regs; >>> u8 *txbuf = (u8 *)dout; >>> u8 *rxbuf = (u8 *)din; >>> - u32 count = ((bitlen / 8) + (bitlen % 8 ? 1 : 0)); >> >> Was this equivalent to (bitlen + 7) / 8 ? > > Yes. And that is the point of this patch ( to handle divide-round-up > in a uniform way). >> >> -- >> Thanks, >> Jagan.
On Fri, Jun 14, 2013 at 6:54 PM, Richard Retanubun <richardretanubun@ruggedcom.com> wrote: > On 14/06/13 09:09 AM, Axel Lin wrote: >> >> 2013/6/14 Jagan Teki<jagannadh.teki@gmail.com>: >>> >>> Hi, >>> >>> IMHO: >>> Can you please use the proper commit header prefix. >>> >>> I am just sharing my thoughts, ignore this if you know it already. >>> I followed below syntax. >>> "<main_module>:<sub_module>:<sub_sub_module>:<COMMIT_HEADER>" >>> >>> Ex: for this commit (seems like this commit changes two drivers) >>> spi: cf_qspi | mxc_spi: Use DIV_ROUND_UP at appropriate places >> >> >> This does not scale. >> What if a (trivial) patch touches 10 drivers? > > Depends how trivial it is I guess. I can see benefits to both approaches. > > This one is simple enough that I am okay if it is grouped. > > Just for my own education, how do I correctly ACK just the part I know > (mcf_qspi in this case) in a grouped patch? > Is there a way to automate my "Signed-off-by:" via e-mail? Usually normal mail server like gmail or outlook doesn' update these s-o-b or a-b use thunderbread mail logic, that will automate when u did these changes. -- Thanks, Jagan.
On 14/06/2013 15:18, Jagan Teki wrote: >> This does not scale. >> What if a (trivial) patch touches 10 drivers? > Yes, you are correct if the changes is common to across the all drivers or > If the changes which are more in 10 spi drivers out of 20 or > 10 for > your example - use "spi: " > and I think it's good to use above syntax as you changesd two drivers. > > I am just showing the possibilities of using the coding guidelines, > nothing more. There is another issue: patman recognizes each <subpart>: and checks for an alias to find the e-mail to send patches, and stops to go on if it is not found. In this example, you can use patman if there is an entry for spi: (and this is you as SPI custodian) and also for mxc_spi:, and usually there is no maintainer for each driver. I agree that using a long chain does not scale, and I think the hidden rule to use <subsystem:> <subject> works well in most cases, Axel did in this patchset. Regards, Stefano
diff --git a/drivers/spi/cf_qspi.c b/drivers/spi/cf_qspi.c index a37ac4e..06bcf91 100644 --- a/drivers/spi/cf_qspi.c +++ b/drivers/spi/cf_qspi.c @@ -171,7 +171,7 @@ int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, volatile qspi_t *qspi = dev->regs; u8 *txbuf = (u8 *)dout; u8 *rxbuf = (u8 *)din; - u32 count = ((bitlen / 8) + (bitlen % 8 ? 1 : 0)); + u32 count = DIV_ROUND_UP(bitlen, 8); u32 n, i = 0; /* Sanitize arguments */ diff --git a/drivers/spi/mxc_spi.c b/drivers/spi/mxc_spi.c index 5bed858..2ea3228 100644 --- a/drivers/spi/mxc_spi.c +++ b/drivers/spi/mxc_spi.c @@ -224,7 +224,7 @@ int spi_xchg_single(struct spi_slave *slave, unsigned int bitlen, const u8 *dout, u8 *din, unsigned long flags) { struct mxc_spi_slave *mxcs = to_mxc_spi_slave(slave); - int nbytes = (bitlen + 7) / 8; + int nbytes = DIV_ROUND_UP(bitlen, 8); u32 data, cnt, i; struct cspi_regs *regs = (struct cspi_regs *)mxcs->base; @@ -294,7 +294,7 @@ int spi_xchg_single(struct spi_slave *slave, unsigned int bitlen, /* Transfer completed, clear any pending request */ reg_write(®s->stat, MXC_CSPICTRL_TC | MXC_CSPICTRL_RXOVF); - nbytes = (bitlen + 7) / 8; + nbytes = DIV_ROUND_UP(bitlen, 8); cnt = nbytes % 32; @@ -330,7 +330,7 @@ int spi_xchg_single(struct spi_slave *slave, unsigned int bitlen, int spi_xfer(struct spi_slave *slave, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { - int n_bytes = (bitlen + 7) / 8; + int n_bytes = DIV_ROUND_UP(bitlen, 8); int n_bits; int ret; u32 blk_size;
This change slightly improves readability. Signed-off-by: Axel Lin <axel.lin@ingics.com> --- drivers/spi/cf_qspi.c | 2 +- drivers/spi/mxc_spi.c | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)