linux-kernel - Re: [lm-sensors] [RFC 5/5] hwmon: add support for Technologic Systems TS-5500 A-D converter

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]

Date:	Tue, 03 May 2011 11:55:34 -0400
From:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:	Guenter Roeck <guenter.roeck@...csson.com>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"lm-sensors@...sensors.org" <lm-sensors@...sensors.org>,
	"linux-serial@...r.kernel.org" <linux-serial@...r.kernel.org>,
	Jonas Fonseca <jonas.fonseca@...oirfairelinux.com>,
	"platform-driver-x86@...r.kernel.org" 
	<platform-driver-x86@...r.kernel.org>,
	linux-iio <linux-iio@...r.kernel.org>
Subject: Re: [lm-sensors] [RFC 5/5] hwmon: add support for Technologic Systems TS-5500 A-D converter

Hi Guenter,

Excerpts from Guenter Roeck's message of 2011-04-29 23:39:38 -0400:
> Hi Vivien,
> 
> The headline and file name are a bit misleading, since the driver is really
> for MAX197 on a TS-5500 board.
> 
> You should split the driver into two parts, a generic driver
> for the MAX197 and a platform driver (residing somewhere in arch/
> or possibly drivers/platform/) to instantiate it.
> 
> There should be a platform data include file, probably in
> include/linux/platform_data/.
> 
> .ioaddr in platform data should not be necessary. The driver's probe
> function should get the values using platform_get_resource().
> 
> Having said that, from reading the code it looks like the chip is not really
> used for hardware monitoring, but for generic ADC functionality. A quick look
> into the TS-5500 user manual confirms this. So this should not be a hwmon
> driver in the first place, but a generic ADC driver. Given the ADC conversion rate
> of the MAX197, the hwmon ABI is not optimal anyway. You should move this driver
> into the iio subsystem, probably to drivers/staging/iio/adc. Copying the iio
> mailing list for input.
> 
> Thanks,
> Guenter

I've took a closer look to the manual and that's right, in fact the
driver doesn't talk to the MAX197 directly. The board uses a CPLD to
abstract the interface to the MAX197. So all the MAX197 logic is hidden
by the CPLD.  Therefore, the driver files should probably not have
function and structure names with a "max197_" prefix. I'll make the code
a bit clearer. What do you think?

Ok for iio subsystem, several ADC drivers are in drivers/hwmon/ but
drivers/staging/iio/adc seems to be the good place now.

Regards,
Vivien.
--
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/