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

[Vivien Didelot]

Le Mon, 23 Jan 2012 16:41:38 -0800,
Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> a écrit :

> On Mon, Jan 23, 2012 at 05:35:51PM -0500, Vivien Didelot wrote:
> > On Mon, 23 Jan 2012 15:54:08 -0500,
> > Vivien Didelot <vivien.didelot@xxxxxxxxxxxxxxxxxxxx> wrote:
> > 
> > > On Sun, 22 Jan 2012 20:36:46 -0800,
> > > Guenter Roeck <guenter.roeck@xxxxxxxxxxxx> wrote: 
> > > > > > 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.
> > > > > 
> > > > > We should either split every driver into corresponding
> > > > > subdirectories, or put everything in a common platform
> > > > > directory. My first RFC patches set has every driver
> > > > > separated. As they are really specific to the platform,
> > > > > people seem to agree with grouping them, mainly because they
> > > > > won't be shared. I changed that in the following patches
> > > > > sets, and X86 maintainers seemed to be ok with that.
> > > > > 
> > > > > I'm ok with both solutions, but we should all agree on one.
> > > > > Maybe we should have other maintainers view on this?
> > > > > 
> > > > That is what I had asked for. I thought the whole point of
> > > > per-module directories was to have all drivers there. If that
> > > > is no longer true, fine with me; who am I to argue about
> > > > something like that. I'd just like to know.
> > > > 
> > > > > > 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.
> > > > > 
> > > > > You're right, it should be possible to create a generic max197
> > > > > driver and provide read/write functions through platform
> > > > > data. But we don't have a max197 right now... So it can stay
> > > > > as a compact TS-5500 ADC driver for the moment, and maybe we
> > > > > will split later. What do you think?
> > > > > 
> > > > I am lost. If you don't have a TS-5500 with max197, how do you
> > > > test the driver ?
> > > 
> > > Sorry, I wasn't clear. I meant the only max197 I have is the one
> > > behind the TS-5500 CPLD, I don't have any others to test
> > > independently.
> > > 
> > > > I had another look into the MAX197 and TS-5500 data sheets. In
> > > > my opinion, a generic MAX197 driver in drivers/hwmon combined
> > > > with a platform driver in the current location would be the way
> > > > to go. That driver would then also work for the other TS-5x00
> > > > systems. All you need is a single chip access function in the
> > > > platform code, since the chip is always accessed with a write
> > > > followed by a read.
> > > 
> > > I took a deeper look at the datasheets, and you're right, a MAX197
> > > driver seems to be a good choice. However, there are a number of
> > > differences between a direct usage of a MAX197 and the TS-5500
> > > mapped MAX197.
> > > 
> > > To start a conversion of a channel for a given range and
> > > polarity, it consists on both sides of a u8 outb() call on pins
> > > 7-14 (i.e. bits D7-D0). To be notified when the result is ready,
> > > we can either set an IRQ on INT pin (falling edge), or poll it.
> > > Then on the MAX197, you read the pins 7-14, set pin HBEN to 1, and
> > > read the same pins again to get the 4 remaining bits. On the
> > > TS-5500, only polling is available, and the 12 bits are mapped on
> > > 2 registers.
> > > 
> > > I propose to write a max197 driver with default read and write
> > > functions. A platform_data will be used to specify the base
> > > address (pins 7-14), and eventually a custom read function
> > > pointer, which will be used instead of the default one if it is
> > > different of NULL.
> > > 
> > > What do you think?
> > > 
> > > I will write a max197 driver with default read and write
> > > functions. A platform_data will be used to specify the base
> > > address (pins 7-14), and eventually a custom read function
> > > pointer, which will be used instead of the default, if it is not
> > > NULL.
> > > 
> > > What do you think? 
> > 
> Sounds like a plan to me.
> 
> > Sorry for the duplicate :)
> > 
> > BTW, I've added Jean Delvare and the lm-sensors mailing list in Cc,
> > in case they have an opinion on this.
> > 
> 
> Thanks,
> Guenter

Hi Guenter,

There's another disadvantage with the generic driver.
Usually, the MAX197 is memory mapped, but on the TS-5500, the CPLD is
port mapped, so even the write function would be different.
It sounds like the amount of reusable lines of code is not that
significant.

Do you think it is still worthwhile?

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