On Tue, 2011-05-03 at 11:55 -0400, Vivien Didelot wrote:

Hi Guenter,

Excerpts from Guenter Roeck's message of 2011-04-29 23:39:38 -0400:

quoted

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?

The original driver for the chip on http://mcrapet.free.fr/ has a
platform dependent and a platform independent part. Other than coding
style issues, that approach seems to be cleaner to me.

Thanks,
Guenter