From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guenter Roeck Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module Date: Mon, 20 Sep 2010 07:38:34 -0700 Message-ID: <20100920143834.GA1500@ericsson.com> References: <1284632604-25303-1-git-send-email-j-keerthy@ti.com> <20100916151736.GC14165@ericsson.com> <20100920140905.GB1306@ericsson.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Return-path: Received: from imr3.ericy.com ([198.24.6.13]:56332 "EHLO imr3.ericy.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756442Ab0ITOjS (ORCPT ); Mon, 20 Sep 2010 10:39:18 -0400 Content-Disposition: inline In-Reply-To: <20100920140905.GB1306@ericsson.com> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: "J, KEERTHY" Cc: "linux-omap@vger.kernel.org" , "Krishnamoorthy, Balaji T" , "lm-sensors@lm-sensors.org" On Mon, Sep 20, 2010 at 10:09:05AM -0400, Guenter Roeck wrote: > Hi, > On Mon, Sep 20, 2010 at 06:34:24AM -0400, J, KEERTHY wrote: > > > > > > > -----Original Message----- > > > From: Guenter Roeck [mailto:guenter.roeck@ericsson.com] > > > Sent: Thursday, September 16, 2010 8:48 PM > > > To: J, KEERTHY > > > Cc: linux-omap@vger.kernel.org; lm-sensors@lm-sensors.org; Krishnamoorthy, > > > Balaji T > > > Subject: Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 > > > madc module > > > > [...] > > > > +EXPORT_SYMBOL(twl4030_madc_conversion); > > > > + > > > If this function is going to be called from external code, it should not > > > really be defined here. I would suggest to move it to a global location > > > such as > > > mfd instead, including all related functions. > > > > > > The existence of this function export indicates that another non-hwmon > > > driver depends on this one, which should not really be the case. Another > > > reason to have a separate common driver instead, and mfd might just be the > > > place for it. > > Few kernel modules need to perform ADC conversion to measure battery > > voltage, battery temperature, VBUS voltage via twl4030_madc_conversion. > > the_madc is needed as those drivers will not have this device pointer. > > > The point I was trying to make is that this function (as well as the ioctl) > should not be in this driver in the first place. hwmon is about providing > hwmon information, not about providing adc readings to another driver. > > Or, in other words, hwmon should be a consumer of information from other drivers, > not a producer of information to other drivers. > > There should be a higher level driver (presumably a mfd driver) to provide > adc readings to all consumers, ie to all callers of twl4030_madc_conversion(). > This driver can provide all data and information needed by more than one driver, > and would also be the logical place for the ioctl providing raw adc readings > to the user. > I just noticed that there already is a mfd core driver for tw4030. You might want to consider moving the functionality to read adc values into that driver. Guenter