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

[Guenter Roeck]

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@xxxxxxxxxxxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/