linux-kernel - Re: [PATCH] x86/platform: (TS-5500) revised ADC driver

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: <20120121044631.GA4143@ericsson.com>
Date:	Fri, 20 Jan 2012 20:46:31 -0800
From:	Guenter Roeck <guenter.roeck@...csson.com>
To:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
CC:	"x86@...nel.org" <x86@...nel.org>, Ingo Molnar <mingo@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] x86/platform: (TS-5500) revised ADC driver

On Fri, Jan 20, 2012 at 06:43:22PM -0500, Vivien Didelot wrote:
> Update of the ADC driver according to the Guenter Roeck's review.
> 
> Signed-off-by: Vivien Didelot <vivien.didelot@...oirfairelinux.com>
> ---
>  arch/x86/platform/ts5500/ts5500_adc.c |  228 +++++++++++++--------------------
>  1 files changed, 89 insertions(+), 139 deletions(-)
> 
> diff --git a/arch/x86/platform/ts5500/ts5500_adc.c b/arch/x86/platform/ts5500/ts5500_adc.c
> index fc4560f..da6decf 100644
> --- a/arch/x86/platform/ts5500/ts5500_adc.c
> +++ b/arch/x86/platform/ts5500/ts5500_adc.c
> @@ -62,57 +62,62 @@
>  
Hi Vivien,

This isn't really a revised driver ... it is a patch on top of the previous version.

Regarding the location, I'd really like to know from the powers-that-be if
	arch/x86/platform/ts5500/
or
	drivers/platform/x86
or
	drivers/hwmon

would be the appropriate location for a driver like this. As mentioned before,
my strong preference is drivers/hwmon, but I would like to hear from others.

Also, I am not sure if the current approach is appropriate to start with.
Looking at the datasheet as well as into existing kernel code, it appears quite likely 
that some kind of more or less generic MAX197 driver exists somewhere. The existence 
of is_max197_installed() - without any calling code - is a strong indication that
this is the case, as well as the "static" platform data in your original patch.
It might be more appropriate to take this more or less generic driver, move it to
drivers/hwmon, and provide - for example through platform data - a means
to read from and write to the chip on a per-platform basis, ie with per-platform
access functions.

Thanks,
Guenter
--
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/