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

[Lars-Peter Clausen]

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@xxxxxxxxxx>
>> Cc: lm-sensors@xxxxxxxxxxxxxx
>> ---
>>  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.
> 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?
>> +	jz4740_adc_enable_irq(adc, JZ_ADC_IRQ_ADCIN);
>> +	jz4740_adc_enable_adc(adc, JZ_ADC_ENABLE_ADCIN);
>> +
>> +	t = wait_for_completion_interruptible_timeout(&adc->adc_completion,
>> +							HZ);
>> +
>> +	jz4740_adc_disable_irq(adc, JZ_ADC_IRQ_ADCIN);
>> +
>> +	if (t <= 0) {
>> +		jz4740_adc_disable_adc(adc, JZ_ADC_ENABLE_ADCIN);
>> +		return t ? t : -ETIMEDOUT;
>> +	}
>> +
>> +	val = readw(adc->base + JZ_REG_ADC_ADCIN);
>> +	jz4740_adc_clk_disable(adc);
>> +
>>     
> Does this device really use units of milivolts? (standard in hwmon).
> I couldn't confirm either way via quick googling.
>   
Hm, right, it does not. Interestingly the datasheet does not tell the
unit of the returned data, but I found a formula in the datasheet for a
similar SoC.
>> +	return sprintf(buf, "%d\n", val);
>> +}
>> +
>> + [...]
>>     
Thanks for reviewing the patch.

- Lars
--
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/