linux-kernel - Re: [PATCH v1] i2c: microchip-corei2c: add smbus support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4xyehpobtsyj2k5xlhupq7x6z7es7bvzek4zsf4roramy5h7kn@duxhfxd4gxsq>
Date: Tue, 6 May 2025 14:04:55 +0200
From: Andi Shyti <andi.shyti@...nel.org>
To: Conor Dooley <conor@...nel.org>
Cc: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>, 
	linux-i2c@...r.kernel.org, 
	prashanth kumar burujukindi <prashanthkumar.burujukindi@...rochip.com>, Conor Dooley <conor.dooley@...rochip.com>, 
	Daire McNamara <daire.mcnamara@...rochip.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1] i2c: microchip-corei2c: add smbus support

Hi Conor,

On Tue, May 06, 2025 at 11:56:19AM +0100, Conor Dooley wrote:
> On Mon, May 05, 2025 at 10:04:27PM +0200, Andi Shyti wrote:
> > On Wed, Apr 30, 2025 at 05:06:09PM +0530, Mukesh Kumar Savaliya wrote:
> > > On 4/30/2025 4:53 PM, Conor Dooley wrote:
> > > > From: prashanth kumar burujukindi <prashanthkumar.burujukindi@...rochip.com>

Do we want to keep lower case for names and surnames? Can I use
upper cases?

> > > > In this driver the supported SMBUS commands are smbus_quick,
> > > > smbus_byte, smbus_byte_data, smbus_word_data and smbus_block_data.
> > > > 
> > > Write completely in imperative mood. something like :
> > > 
> > > Add support for SMBUS commands in driver
> > > 
> > > Add support for SMBUS commands: smbus_quick, smbus_byte, smbus_byte_data,
> > > smbus_word_data, and smbus_block_data.
> > 
> > yes, I agree that the original commit log is a bit lazy written :-)
> 
> I don't personally think the suggested wording makes any meaningful
> difference, but I can rework it if required.

The point of using the imperative form is to clearly state what
the patch does. Saying "the supported commands are..." feels a
bit lazy, in my opinion, and requires a peek into the change to
fully understand what the patch introduces.

To be honest, I wouldn't reject the patch over this, but it
doesn't hurt to expand the log a little.

(No need to resend—you can just reply to this mail with your
updated commit log.)

> > > Also mention below limitations here .
> 
> I actually removed them from the commit message, since they're not
> limitations just what was and was not tested. I can put them back too
> if that's needed.
> 
> > > SMBUS block read is supported by the controller but has not been tested due
> > > to lack of hardware. However, SMBUS I2C block read has been tested.
> > 
> > Smbus i2c block has not been tested? If so, can we leave it out?
> > What is the interest to keep it in?
> 
> What's the interest in adding any feature? Someone might want to use it.

What's the point of adding a feature that no one uses? :-)

> We did not have a piece of hardware that uses it, so didn't do testing
> of that specific command, but a customer may well want to so we included
> it. Again, if you think removing it is the play, I can do that.

No worries, please leave it as it is if you think it will be
useful in the future. I just wanted to clarify.

Thanks,
Andi