2022-08-29 20:53:36

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v2 0/9] i2c-mlxbf.c: bug fixes and new feature support

On Mon, Aug 22, 2022 at 03:57:16PM -0400, Asmaa Mnebhi wrote:
> This is a series of patches fixing several bugs and implementing
> new features.

What did change since v1 and where did Khalil's Rev-by tags go?

> Bug fixes:
> 1) Fix the frequency calculation
> 2) remove unnecessary IRQF_ONESHOT flag

Is this really a bugfix?

> 3) Fix incorrect base address passed during io write
> 4) prevent stack overflow in mlxbf_i2c_smbus_start_transaction()
> 5) Support lock mechanism

Here, I am also not sure if this is a bugfix.

Thanks for the update,

Wolfram


Attachments:
(No filename) (584.00 B)
signature.asc (849.00 B)
Download all attachments

2022-08-29 21:06:54

by Asmaa Mnebhi

[permalink] [raw]
Subject: RE: [PATCH v2 0/9] i2c-mlxbf.c: bug fixes and new feature support

> This is a series of patches fixing several bugs and implementing new
> features.

What did change since v1 and where did Khalil's Rev-by tags go?

Khalil told me to remove the rev-by : ) since it is supposed to be reserved for the reviewers. I will add it back and describe what has changed in my next set of patches. As a note , this is what changed from v1->v2:
1) moved all the bug fixes to the top commits and left the features for last
2) split the BlueField-3 SoC patch into 2 to address Rob's comment: one for the driver code and another patch for the device tree binding yaml documentation file
3) addressed Rob's comment regarding keeping the device tree/acpi tables backward compatible. So add the new resources at the end of the enum.
4) update the license in a separate patch

> Bug fixes:
> 1) Fix the frequency calculation
> 2) remove unnecessary IRQF_ONESHOT flag

Is this really a bugfix?

This is not a bug fix. We removed it because it is no longer needed.

> 3) Fix incorrect base address passed during io write
> 4) prevent stack overflow in mlxbf_i2c_smbus_start_transaction()
> 5) Support lock mechanism

Here, I am also not sure if this is a bugfix.

I think this is a bug fix because we have an i2c driver also in UEFI so we need this lock mechanism to avoid race conditions over acquiring the i2c bus. Not using this lock resulted in unexpected behavior.

Thanks for the update,

Wolfram

2022-09-07 22:26:12

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v2 0/9] i2c-mlxbf.c: bug fixes and new feature support


> What did change since v1 and where did Khalil's Rev-by tags go?
>
> Khalil told me to remove the rev-by : ) since it is supposed to be reserved for the reviewers.

But he reviewed the patches, or? Which makes him a reviewer? I mean he
is the maintainer of the driver, so I basically a Rev-by from him.

> I will add it back and describe what has changed in my next set of patches.

So, you are planning a v3 already?

> As a note , this is what changed from v1->v2:

Cool, thank you for doing that.

> I think this is a bug fix because we have an i2c driver also in UEFI
> so we need this lock mechanism to avoid race conditions over acquiring
> the i2c bus. Not using this lock resulted in unexpected behavior.

OK, thank you!


Attachments:
(No filename) (755.00 B)
signature.asc (849.00 B)
Download all attachments

2022-09-07 22:40:46

by Asmaa Mnebhi

[permalink] [raw]
Subject: RE: [PATCH v2 0/9] i2c-mlxbf.c: bug fixes and new feature support

> What did change since v1 and where did Khalil's Rev-by tags go?
>
> Khalil told me to remove the rev-by : ) since it is supposed to be reserved for the reviewers.

But he reviewed the patches, or? Which makes him a reviewer? I mean he is the maintainer of the driver, so I basically a Rev-by from him.

Yes he did review the patch internally so I will add the Rev-by in v3

> I will add it back and describe what has changed in my next set of patches.

So, you are planning a v3 already?

I was going to send a v3 patch after I get some feedback on the other patches unless you'd prefer I send it now?

> As a note , this is what changed from v1->v2:

Cool, thank you for doing that.

> I think this is a bug fix because we have an i2c driver also in UEFI
> so we need this lock mechanism to avoid race conditions over acquiring
> the i2c bus. Not using this lock resulted in unexpected behavior.

OK, thank you!