From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757556Ab2AXVY1 (ORCPT ); Tue, 24 Jan 2012 16:24:27 -0500 Received: from imr4.ericy.com ([198.24.6.9]:48493 "EHLO imr4.ericy.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757531Ab2AXVY0 (ORCPT ); Tue, 24 Jan 2012 16:24:26 -0500 Message-ID: <1327440010.2649.204.camel@groeck-laptop> Subject: Re: [PATCH] x86/platform: (TS-5500) revised ADC driver From: Guenter Roeck Reply-To: guenter.roeck@ericsson.com To: Vivien Didelot CC: "khali@linux-fr.org" , "x86@kernel.org" , Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , "linux-kernel@vger.kernel.org" , "lm-sensors@lm-sensors.org" Date: Tue, 24 Jan 2012 13:20:10 -0800 In-Reply-To: <20120124151616.5723934b@v0nbox> References: <1326930731-21215-5-git-send-email-vivien.didelot@savoirfairelinux.com> <1327103002-5834-1-git-send-email-vivien.didelot@savoirfairelinux.com> <20120121044631.GA4143@ericsson.com> <20120122203619.22fabe79@lenov0n> <20120123043645.GA14660@ericsson.com> <20120123155408.7d52e4da@v0nbox> <20120123173551.309af2eb@v0nbox> <20120124004138.GA20363@ericsson.com> <20120124151616.5723934b@v0nbox> Organization: Ericsson Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.2.1- Content-Transfer-Encoding: 8bit MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2012-01-24 at 15:16 -0500, Vivien Didelot wrote: > Le Mon, 23 Jan 2012 16:41:38 -0800, > Guenter Roeck 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 wrote: > > > > > > > On Sun, 22 Jan 2012 20:36:46 -0800, > > > > Guenter Roeck 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? Points to a platform specific access function, as I had suggested earlier. The access function is just a small part of the driver; the entire hwmon/sysfs code is generic. Yes, I think it is worthwhile. Guenter