Re: [lm-sensors] [PATCH 1/2] hwmon: twl4030: Driver for twl4030 madc module

[Keerthy <a0393675@ti.com>]

Hello Sameo,

twl4030-madc  driver patch can be found here:
http://www.mail-archive.com/linux-omap@vger.kernel.org/msg34947.html

Based on the received inputs.
Can the twl4030-madc driver or part of the driver reside under mfd?

Regards,
Keerthy





On Monday 20 September 2010 08:08 PM, Guenter Roeck wrote:
> 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
>