linux-kernel - Re: [PATCH 2/3] i2c-piix4: separate registration and probing code

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: <20120614213804.46ffe67d@endymion.delvare>
Date:	Thu, 14 Jun 2012 21:38:04 +0200
From:	Jean Delvare <khali@...ux-fr.org>
To:	Andrew Armenia <andrew@...uaredlabs.com>
Cc:	Ben Dooks <ben-linux@...ff.org>,
	Wolfram Sang <w.sang@...gutronix.de>,
	linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] i2c-piix4: separate registration and probing code

Hi Andrew,

On Wed, 13 Jun 2012 12:59:08 -0400, Andrew Armenia wrote:
> Some chipsets have multiple sets of SMBus registers each controlling a
> separate SMBus. Supporting these chipsets properly will require registering
> multiple I2C adapters for one piix4.
> 
> The code to initialize and register the i2c_adapter structure has been
> separated from piix4_probe and allows registration of a piix4 adapter
> given its base address. Note that the i2c_adapter and i2c_piix4_adapdata
> structures are now dynamically allocated.
> 
> Signed-off-by: Andrew Armenia <andrew@...uaredlabs.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c |  113 ++++++++++++++++++++++++++--------------
>  1 file changed, 73 insertions(+), 40 deletions(-)
> (...)

Applied, with the minor changes below:

> -static int piix4_transaction(unsigned short piix4_smba)
> +static int piix4_transaction(struct i2c_adapter *piix4_adapter)
>  {
>  	int temp;
>  	int result = 0;
>  	int timeout = 0;
>  
> -	dev_dbg(&piix4_adapter.dev, "Transaction (pre): CNT=%02x, CMD=%02x, "
> +	struct i2c_piix4_adapdata *adapdata;
> +	unsigned short piix4_smba;
> +
> +	adapdata = i2c_get_adapdata(piix4_adapter);
> +	piix4_smba = adapdata->smba;

It is customary to declare and assign all at once in this case:

	struct i2c_piix4_adapdata *adapdata = i2c_get_adapdata(piix4_adapter);
	unsigned short piix4_smba = adapdata->smba;

This makes the code more compact and more readable too. I've fixed it
and everywhere else.

> (...)
> +static int __devinit piix4_add_adapter(struct pci_dev *dev,
> +					unsigned short smba,
> +					struct i2c_adapter **padap)
> +{
> +	struct i2c_adapter *adap;
> +	struct i2c_piix4_adapdata *adapdata;
> +
> +	int retval;
> +
> +	adap = kzalloc(sizeof(*adap), GFP_KERNEL);
> +	if (NULL == adap)

There's no point in inverting comparisons this way. Compilers would let
you know if you had it wrong, they do for at least 10 years now.

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