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: <20250508-unrefined-outhouse-3ff09d1e46b5@spud>
Date: Thu, 8 May 2025 12:39:35 +0100
From: Conor Dooley <conor@...nel.org>
To: Andi Shyti <andi.shyti@...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

On Tue, May 06, 2025 at 02:04:55PM +0200, Andi Shyti wrote:
> 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?

I dunno, that's how it was provided, I'm a fan of self-determination in
that regard.

> > > > > 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.

Right, I wouldn't either. Sure, it could have been better and I
probably should have rewritten it when I sent it on - but I get more
than a bit pissed off and opt to push back when people who aren't
maintainers for some code come along with a review entirely about
cosmetic parts of a commit message.

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

I was just about to do this, but noticed you picked the patch up
already. Sorry for the delay there, I meant to do it yesterday but
crashed out early. I'd just have changed it to
"Add hardware support for the SMBUS commands smbus_quick, smbus_byte,
smbus_byte_data, smbus_word_data and smbus_block_data, replacing the
fallback to software emulation"
or similar. If you fancy rebasing, maybe use that?

> > > > 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? :-)

I wouldn't say no one, just neither Prashanth or I :-)

> > 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.

NW, thanks for picking it up.
Conor.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)