linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Greg KH <greg@kroah.com>
To: linux-kernel@vger.kernel.org, chrisg@0-in.com,
	sensors@stimpy.netroedge.com
Subject: Re: it87 driver converted to sysfs
Date: Thu, 24 Apr 2003 09:04:31 -0700	[thread overview]
Message-ID: <20030424160431.GC18690@kroah.com> (raw)
In-Reply-To: <20030424150113.GJ1069@iucha.net>

On Thu, Apr 24, 2003 at 10:01:13AM -0500, Florin Iucha wrote:
> Greg,
> 
> I have converted it87 i2c driver to use sysfs. Please review it and
> submit it for inclusion.

A few comments:

> +static struct i2c_driver it87_driver = {
> +	.owner		= THIS_MODULE,
> +	.name		= "IT87xx sensor driver",
> +	.id		= I2C_DRIVERID_IT87,
> +	.flags		= I2C_DF_NOTIFY,
> +	.attach_adapter	= it87_attach_adapter,
> +	.detach_client	= it87_detach_client,
> +};

Isn't that name too big for sysfs?  Why not just drop the 
" sensor driver" portion of it, as that is pretty redundant.

> +/* The /proc/sys entries */
> +
> +/* -- SENSORS SYSCTL START -- */

<snip> 

These are no longer needed, right?

> +/* These files are created for each detected IT87. This is just a template;
> +   though at first sight, you might think we could use a statically
> +   allocated list, we need some way to get back to the parent - which
> +   is done through one of the 'extra' fields which are initialized 
> +   when a new copy is allocated. 
> +static ctl_table it87_dir_table_template[] = {

Nor is this table needed anymore either.  Yeah, it's commented out,
might as well just delete the whole thing :)

> +static ssize_t show_in(struct device *dev, char *buf, int nr) {
> +	struct i2c_client *client = to_i2c_client(dev);
> +   struct it87_data *data = i2c_get_clientdata(client);
> +	it87_update_client(client);
> +	return sprintf(buf, "%ld\n", IN_FROM_REG(data->in[nr], nr)*10 );
> +}

Please use the kernel coding style as documented in
Documentation/CodingStyle (hint, use tabs, and the '{' for a function
should be on a new line.)

> +/* OK, this is not exactly good programming practice, usually. But it is
> +   very code-efficient in this case. */

This comment can go, as it is good programming practice.

> diff -Nru linux-2.5.68/drivers/i2c/chips/Kconfig linux-2.5.68-it87/drivers/i2c/chips/Kconfig
> --- linux-2.5.68/drivers/i2c/chips/Kconfig	2003-04-24 09:46:09.000000000 -0500
> +++ linux-2.5.68-it87/drivers/i2c/chips/Kconfig	2003-04-24 09:57:32.000000000 -0500
> @@ -64,10 +64,20 @@
>  	  in the lm_sensors package, which you can download at
>  	  http://www.lm-sensors.nu
>  
> +config SENSORS_IT87
> +	tristate "  ITE IT87 and compatibles"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  The module will be called it87.
> +
> +	  You will also need the latest user-space utilties: you can find them
> +	  in the lm_sensors package, which you can download at 
> +	  http://www.lm-sensors.nu
> +

Can you place this in alphabetical order in the file?  Makes is nicer
looking.

> diff -Nru linux-2.5.68/drivers/i2c/chips/Makefile linux-2.5.68-it87/drivers/i2c/chips/Makefile
> --- linux-2.5.68/drivers/i2c/chips/Makefile	2003-04-24 09:46:03.000000000 -0500
> +++ linux-2.5.68-it87/drivers/i2c/chips/Makefile	2003-04-24 09:55:21.000000000 -0500
> @@ -6,3 +6,4 @@
>  obj-$(CONFIG_SENSORS_LM75)	+= lm75.o
>  obj-$(CONFIG_SENSORS_VIA686A)	+= via686a.o
>  obj-$(CONFIG_SENSORS_W83781D)	+= w83781d.o
> +obj-$(CONFIG_SENSORS_IT87)	+= it87.o


Same thing with the alphabetical order here.

Other than those very minor things the patch looks good.  Mind fixing
them and sending it to me again?

thanks,

greg k-h

  reply	other threads:[~2003-04-24 15:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2003-04-24 15:01 it87 driver converted to sysfs Florin Iucha
2003-04-24 16:04 ` Greg KH [this message]
2003-04-24 16:51   ` Florin Iucha
2003-04-24 17:27     ` Greg KH
2003-04-25 21:53       ` Florin Iucha
2003-04-25 22:13         ` Greg KH
2003-04-25 22:52           ` Florin Iucha

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20030424160431.GC18690@kroah.com \
    --to=greg@kroah.com \
    --cc=chrisg@0-in.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sensors@stimpy.netroedge.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).