lm83 driver ported to linux 2.6

[Greg KH]

On Mon, Sep 29, 2003 at 09:36:48PM +0200, Jean Delvare wrote:
> 
> Hi Greg,
> 
> Here is a first try of porting my lm83 driver to linux 2.6. Not sure
> it's all perfect, but it at least compiles and loads/unloads. I have no
> hardware for testing, so I can't do much more. Could you please take a
> look and tell me if it's OK for you?

Comments are below.

> BTW, I'm a bit disappointed by the new sysfs interface. Where I had 3
> callback functions with procfs, I now have 15. They are shorter for
> sure, still 15 is much for such a simple driver. The fact that you need
> macros to define them shows IMHO that there's something wrong there
> (although I don't know what and have no real suggestion for
> improvement).

If you don't like registering all of the different files, look into
using sysfs_create_group() and sysfs_remove_group().  It makes large
numbers of attributes a bit easier to deal with.

And yes, it is a bit bigger than procfs, but the coding interface is a
_lot_ simpler, and we are enforcing the "1 file - 1 value" rule, which
is a good thing :)

> + * Copyright (c) 2003  Jean Delvare <khali at linux-fr.org>
                 ^- should be "(C)" to be legal.  So says the lawyers
		 who know these things.

> +/*
> + * The LM83 registers
> + * Manufacturer ID is 0x01 for National Semiconductor.
> + */
> +
> +#define LM83_REG_R_MAN_ID        0xFE

No tabs used here or in any of the other #defines.

> +static struct i2c_driver lm83_driver = {
> +	.owner          = THIS_MODULE,
> +	.name           = "lm83",
> +	.id             = I2C_DRIVERID_LM83,
> +	.flags          = I2C_DF_NOTIFY,
> +	.attach_adapter = lm83_attach_adapter,
> +	.detach_client  = lm83_detach_client
> +};

Use tabs here before the '=' and put a ',' after lm83_detach_client to
make any future changes not involve changing that line.

> +/*
> + * The following function does more than just detection. If detection
> + * succeeds, it also registers the new chip.
> + */
> +
> +static int lm83_detect(struct i2c_adapter *adapter, int address,
> +	int kind)

No extra line between comments and the function, please.

> +	if (kind < 0) /* detection */
> +	{

Ick, please put the '{' back up on the if line.  Read
Documetation/CodingStyle for more info on this.

> +#ifdef DEBUG
> +			printk("lm83.o: LM83 detection failed at 0x%02x.\n",
> +				address);
> +#endif

Just make this a dev_dbg() call and get rid of the #ifdef.

> +	if (kind <= 0) /* identification */
> +	{
> +		u8 man_id, chip_id;
> +
> +		man_id = i2c_smbus_read_byte_data(new_client, LM83_REG_R_MAN_ID);
> +		chip_id = i2c_smbus_read_byte_data(new_client, LM83_REG_R_CHIP_ID);
> +		if (man_id == 0x01) /* National Semiconductor */
> +		{
> +			if (chip_id == 0x03)
> +			{
> +				kind = lm83;
> +				name = "lm83";
> +			}
> +		}
> +
> +		if (kind <= 0) /* identification failed */
> +		{
> +			dev_info(&adapter->dev,
> +				"Unsupported chip (man_id=0x%02X, chip_id=0x%02X).\n",
> +				man_id, chip_id);
> +			goto exit_free;
> +		}
> +	}

As there is only 1 "kind" supported by this driver, this whole logic can
be simplified a bunch.  Just check the man_id and chip_id and then exit
out if it doesn't match.


> +	device_create_file(&new_client->dev, &dev_attr_temp_input1);
> +	device_create_file(&new_client->dev, &dev_attr_temp_input2);
> +	device_create_file(&new_client->dev, &dev_attr_temp_input3);
> +	device_create_file(&new_client->dev, &dev_attr_temp_input4);
> +	device_create_file(&new_client->dev, &dev_attr_temp_max1);
> +	device_create_file(&new_client->dev, &dev_attr_temp_max2);
> +	device_create_file(&new_client->dev, &dev_attr_temp_max3);
> +	device_create_file(&new_client->dev, &dev_attr_temp_max4);
> +	device_create_file(&new_client->dev, &dev_attr_temp_crit);
> +	device_create_file(&new_client->dev, &dev_attr_alarms);
> +
> +	/*
> +	 * Initialize the LM83 chip
> +	 */
> +
> +	lm83_init_client(new_client);

You should probably initialize the chip before registering it with the
i2c core as it can be queried right then before this call could be
finished.


> diff -ruN linux-2.6.0-test6/include/linux/i2c-id.h linux-2.6.0-test6-khali/include/linux/i2c-id.h
> --- linux-2.6.0-test6/include/linux/i2c-id.h	Sun Sep 28 17:43:40 2003
> +++ linux-2.6.0-test6-khali/include/linux/i2c-id.h	Sun Sep 28 22:54:01 2003
> @@ -153,6 +153,7 @@
>  #define I2C_DRIVERID_FS451 1037
>  #define I2C_DRIVERID_W83627HF 1038
>  #define I2C_DRIVERID_LM85 1039
> +#define I2C_DRIVERID_LM83 1040

What do we do with these ids?

Anyway, very nice job.  With those minor fixes I'd be glad to add it to
the 2.6 kernel tree.

thanks,

greg k-h