Re: [lm-sensors] [RFC][PATCH 22/26] hwmon: Add JZ4740 ADC driver

[Lars-Peter Clausen <lars@metafoo.de>]

Jonathan Cameron wrote:
> On 06/05/10 20:08, Lars-Peter Clausen wrote:
>   
>> Hi
>>
>> Jonathan Cameron wrote:
>>     
>>> On 06/02/10 20:12, Lars-Peter Clausen wrote:
>>>   
>>>       
>>>> This patch adds support for the ADC module on JZ4740 SoCs.
>>>>
>>>> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> Cc: lm-sensors@lm-sensors.org
>>>> ---
>>>>  drivers/hwmon/Kconfig      |   11 ++
>>>>  drivers/hwmon/Makefile     |    1 +
>>>>  drivers/hwmon/jz4740-adc.c |  423 ++++++++++++++++++++++++++++++++++++++++++++
>>>>  include/linux/jz4740-adc.h |   25 +++
>>>>  4 files changed, 460 insertions(+), 0 deletions(-)
>>>>  create mode 100644 drivers/hwmon/jz4740-adc.c
>>>>  create mode 100644 include/linux/jz4740-adc.h
>>>>     
>>>>         
>>> Hi, I'm just wondering of one wants the majority of this driver to sit in hwmon?
>>>
>>> Looks to me like a fairly classic case for something that might be best implemented
>>> as an mfd with the hwmon, touchscreen and battery drivers separately hanging off that.
>>> You might well have someone who needs the battery driver to work, but doesn't care
>>> about hwmon and so doesn't want to build that bit in... 
>>>
>>> Just an immediate thought.  Perhaps this is the best way to do things...
>>>   
>>>       
>> I've thought about it before and rejected the idea at that time, because
>> I thought it will add more abstraction then actually needed.
>> But at that time the adc driver was not a hwmon driver yet and thus
>> didn't pull in the whole hwmon framework if you only wanted to use the
>> battery driver.
>> But the more I'm thinking about it now it might actually make sense to
>> move the common code to a MFD driver.
>>     
>>> Also after a quick look.  How is it used by the touchscreen driver?
>>> If not, please remove the reference from kconfig until it it is true.
>>>   
>>>       
>> There is no touchscreen driver yet. But if I'm going to remove the
>> reference I'm pretty sure that someone will come up and ask why it
>> actually is necessary to have a separate driver instead of putting all
>> the code into the battery driver.
>>     
> Fair enough.  Perhaps a comment for the patch rather than in Kconfig
> as it currently is.  People will enable it then go 'Why can't I now
> enable the touchscreen driver?'
>
>   
I guess that will work.
>>> Few other bits and bobs inline.
>>>   
>>>       
>>>> diff --git a/drivers/hwmon/jz4740-adc.c b/drivers/hwmon/jz4740-adc.c
>>>> new file mode 100644
>>>> index 0000000..635dfe9
>>>> --- /dev/null
>>>> +++ b/drivers/hwmon/jz4740-adc.c
>>>> @@ -0,0 +1,423 @@
>>>> + [...]
>>>> +static ssize_t jz4740_adc_read_adcin(struct device *dev,
>>>> +					struct device_attribute *dev_attr,
>>>> +					char *buf)
>>>> +{
>>>> +	struct jz4740_adc *adc = dev_get_drvdata(dev);
>>>> +	unsigned long t;
>>>> +	uint16_t val;
>>>> +
>>>> +	jz4740_adc_clk_enable(adc);
>>>> +
>>>>     
>>>>         
>>> Is there a possible race here?
>>>   
>>>       
>> Where exactly?
>>     
> I can't recall off the top of my head if sysfs attributes can having multiple
> simultaneous readers. If they can then thread two is just past the next line.
> Whilst the earlier thread has passed the t = wait.... line as the interrupt has
> fired.  The irq is then disabled by thread 1 whilst thread 2 enables the adc.
> Clearly the timeout will prevent any serious issues but the 2nd thread is going
> to falsely wait a second I think... ?
>   
Hm, right. I didn't thought of that. There can be multiple simultaneous
reads.
Actually there are multiple issues with concurrent reads from adcin pin,
I guess the whole function should be protected by a mutex.
And additionally the clock is not turned off in case of an error.

- Lars