linux-kernel - Re: [i2c] [RFC][PATCH 4/4] RTC: SMBus support for the M41T80,

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: <20080509232146.18638986@hyperion.delvare>
Date:	Fri, 9 May 2008 23:21:46 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	"Maciej W. Rozycki" <macro@...ux-mips.org>
Cc:	David Brownell <david-b@...bell.net>, linux-mips@...ux-mips.org,
	mgreer@...sta.com, rtc-linux@...glegroups.com,
	Atsushi Nemoto <anemo@....ocn.ne.jp>,
	linux-kernel@...r.kernel.org, i2c@...sensors.org, ab@...able.de,
	Alessandro Zummo <alessandro.zummo@...ertech.it>
Subject: Re: [i2c] [RFC][PATCH 4/4] RTC: SMBus support for the M41T80,

Hi Maciej,

On Fri, 9 May 2008 21:55:38 +0100 (BST), Maciej W. Rozycki wrote:
> > This is reimplementing i2c_smbus_write_i2c_block_data().
> 
>  Where does it come from?  I fail to see this type of transfer being 
> defined anywhere in the SMBus spec.

It is indeed not.

>                                      I checked the spec before I referred 
> to the implementation in our I2C core and I hope you agree I may not have 
> expected any extensions beyond what the SMBus spec defines.

The "smbus" in these function names are more about "what SMBus
controllers usually can do" than about the SMBus specification. But I
admit you couldn't guess.

>  That written, you are of course correct WRT the reimplementation and I am 
> eager to remove it -- thanks for the point.  I'll skip all your other 
> comments related as obviously implied by this change.
> 
>  Given the function and friends make use of apparently a non-standard
> SMBus transfer, I think they should be called differently, perhaps
> i2c_smbusext_write_i2c_block_data(), etc. or suchlike.

This was an option when the functions where introduced 9 years ago.
But now that it was done, renaming them would cause even more
confusion, I think. I would be fine with adding comments in i2c-core.c
or improving Documentation/i2c/smbus-protocol to make it more obious,
though.

On a related note, you will notice that the other i2c_smbus_* functions
do not follow the naming of SMBus transactions. Again that's something
I regret but I feel that changing the names now would cause a lot of
confusion amongst developers, so I'm not doing it.

> > Mixing code cleanups with functional changes is a Bad Idea (TM).
> 
>  I am happy to bother you with a separate patch including style fixes.  I
> can even create a handful of them, grouping functionally consistent
> changes.

Just one patch should be enough, if I agree with all the changes. You
might make a separate patch with the things I may not agree with, so
that you don't have to cherry-revert them if I indeed don't agree, and
we just merge them if I do agree.

> > >  	dev_info(&client->dev,
> > > -		 "chip found, driver version " DRV_VERSION "\n");
> > > +		 "%s chip found, driver version " DRV_VERSION "\n",
> > > +		 client->name);
> > 
> > Incorrect change, dev_info() already includes the chip name.
> 
>  My system must be a notable exception then, as this change modifies 
> output:
> 
> rtc-m41t80 1-0068: chip found, driver version 0.05
> 
> to:
> 
> rtc-m41t80 1-0068: m41t81 chip found, driver version 0.05
> 
> here.

My bad, for some reason I thought that dev_printk() included the device
name but it in fact includes the driver name. I was wrong, just ignore
me.

-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/