Patchwork [U-Boot] spi: Use DIV_ROUND_UP at appropriate places

login
register
mail settings
Submitter Axel Lin
Date June 14, 2013, 7:59 a.m.
Message ID <1371196742.20321.1.camel@phoenix>
Download mbox | patch
Permalink /patch/251257/
State Superseded
Delegated to: Jagannadha Sutradharudu Teki
Headers show

Comments

Axel Lin - June 14, 2013, 7:59 a.m.
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(-)
Jagannadha Sutradharudu Teki - June 14, 2013, 12:20 p.m.
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.
Axel Lin - June 14, 2013, 1:09 p.m.
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.
Jagannadha Sutradharudu Teki - June 14, 2013, 1:18 p.m.
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.
Richard Retanubun - June 14, 2013, 1:24 p.m.
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.
Jagannadha Sutradharudu Teki - June 14, 2013, 1:28 p.m.
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.
Stefano Babic - June 17, 2013, 10:40 a.m.
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

Patch

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(&regs->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;