Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On 2018-07-23 09:24, Andy Shevchenko wrote:
> On Mon, Jul 23, 2018 at 9:12 AM, Peter Rosin wrote:
>> On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
>>> + /* Handle the remaining bytes which were not sent */
>>> + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
>>> +OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
>>
>> You moved the OWL_ line to the left instead of right, so this is still
>> misaligned and thus not helping the reader.
>
> Guys, wouldn't be better to
>
>
> while (i2c_dev->msg_ptr < msg->len) {
> u32 fifostat = readl();
>
> if (!(fifostat & ...))
> break;
> ...
> }
>
> ?
>
> Same for the other branch.
>
> Yes, it's more LOCs, but less bikeshedding,
Says the guy arriving late to the table with an incredibly valuable
suggestion... Please explain why my suggestion is bikeshedding and
yours is not?
Aligning with the opening parenthesis is right. Aligning with the wrong
opening parenthesis is of course going to risk confusing the reader.
Cheers,
Peter
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On 2018-07-23 09:24, Andy Shevchenko wrote:
> On Mon, Jul 23, 2018 at 9:12 AM, Peter Rosin wrote:
>> On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
>>> + /* Handle the remaining bytes which were not sent */
>>> + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
>>> +OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
>>
>> You moved the OWL_ line to the left instead of right, so this is still
>> misaligned and thus not helping the reader.
>
> Guys, wouldn't be better to
>
>
> while (i2c_dev->msg_ptr < msg->len) {
> u32 fifostat = readl();
>
> if (!(fifostat & ...))
> break;
> ...
> }
>
> ?
>
> Same for the other branch.
>
> Yes, it's more LOCs, but less bikeshedding,
Says the guy arriving late to the table with an incredibly valuable
suggestion... Please explain why my suggestion is bikeshedding and
yours is not?
Aligning with the opening parenthesis is right. Aligning with the wrong
opening parenthesis is of course going to risk confusing the reader.
Cheers,
Peter
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On Mon, Jul 23, 2018 at 9:12 AM, Peter Rosin wrote:
> On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
>> + /* Handle the remaining bytes which were not sent */
>> + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
>> +OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
>
> You moved the OWL_ line to the left instead of right, so this is still
> misaligned and thus not helping the reader.
Guys, wouldn't be better to
while (i2c_dev->msg_ptr < msg->len) {
u32 fifostat = readl();
if (!(fifostat & ...))
break;
...
}
?
Same for the other branch.
Yes, it's more LOCs, but less bikeshedding,
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
Hi Peter,
On Mon, Jul 23, 2018 at 08:12:17AM +0200, Peter Rosin wrote:
> On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
> > + /* Handle the remaining bytes which were not sent */
> > + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> > + OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
>
> You moved the OWL_ line to the left instead of right, so this is still
> misaligned and thus not helping the reader.
>
Yes. I'm not sure which side I should shift but since the recommendation
states that we should start from the first character after parenthesis, so I
went for left.
But yeah, this could be more readable by shifting right.
Anyway, thanks a lot for helping me to cleanup/structure the code.
Regards,
Mani
> Cheers,
> Peter
>
> > + writel(msg->buf[i2c_dev->msg_ptr++],
> > + i2c_dev->base + OWL_I2C_REG_TXDAT);
> > + }
>
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On Mon, Jul 23, 2018 at 9:12 AM, Peter Rosin wrote:
> On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
>> + /* Handle the remaining bytes which were not sent */
>> + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
>> +OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
>
> You moved the OWL_ line to the left instead of right, so this is still
> misaligned and thus not helping the reader.
Guys, wouldn't be better to
while (i2c_dev->msg_ptr < msg->len) {
u32 fifostat = readl();
if (!(fifostat & ...))
break;
...
}
?
Same for the other branch.
Yes, it's more LOCs, but less bikeshedding,
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
Hi Peter,
On Mon, Jul 23, 2018 at 08:12:17AM +0200, Peter Rosin wrote:
> On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
> > + /* Handle the remaining bytes which were not sent */
> > + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> > + OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
>
> You moved the OWL_ line to the left instead of right, so this is still
> misaligned and thus not helping the reader.
>
Yes. I'm not sure which side I should shift but since the recommendation
states that we should start from the first character after parenthesis, so I
went for left.
But yeah, this could be more readable by shifting right.
Anyway, thanks a lot for helping me to cleanup/structure the code.
Regards,
Mani
> Cheers,
> Peter
>
> > + writel(msg->buf[i2c_dev->msg_ptr++],
> > + i2c_dev->base + OWL_I2C_REG_TXDAT);
> > + }
>
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
> + /* Handle the remaining bytes which were not sent */
> + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> +OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
You moved the OWL_ line to the left instead of right, so this is still
misaligned and thus not helping the reader.
Cheers,
Peter
> + writel(msg->buf[i2c_dev->msg_ptr++],
> +i2c_dev->base + OWL_I2C_REG_TXDAT);
> + }
Re: [PATCH v6 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On 2018-07-23 05:40, Manivannan Sadhasivam wrote:
> + /* Handle the remaining bytes which were not sent */
> + while (!(readl(i2c_dev->base + OWL_I2C_REG_FIFOSTAT) &
> +OWL_I2C_FIFOSTAT_TFF) && i2c_dev->msg_ptr < msg->len) {
You moved the OWL_ line to the left instead of right, so this is still
misaligned and thus not helping the reader.
Cheers,
Peter
> + writel(msg->buf[i2c_dev->msg_ptr++],
> +i2c_dev->base + OWL_I2C_REG_TXDAT);
> + }
