[2/3] Samsung SoC: ready to use NTC value inside kernel

Commit Message

MyungJoo Ham June 30, 2011, 8:26 a.m. UTC

This patch allows kernel codes to use values from NTC LM-Sensor
driver, which allows SYSFS access only.

Signed-off-by: MyungJoo Ham <[e-mail]>
Signed-off-by: Kyungmin Park <[e-mail]>
---
 arch/arm/plat-samsung/dev-adc.c              |   62 ++++++++++++++++++++++++++
 arch/arm/plat-samsung/include/plat/adc-ntc.h |   19 ++++++++
 2 files changed, 81 insertions(+), 0 deletions(-)
 create mode 100644 arch/arm/plat-samsung/include/plat/adc-ntc.h

Comments

MyungJoo Ham July 1, 2011, 12:28 a.m. UTC | #1

On Thu, Jun 30, 2011 at 6:00 PM, Russell King - ARM Linux
<[e-mail]> wrote:
> On Thu, Jun 30, 2011 at 05:26:26PM +0900, MyungJoo Ham wrote:
>> diff --git a/arch/arm/plat-samsung/dev-adc.c b/arch/arm/plat-samsung/dev-adc.c
>> index 622972c..526097a 100644
>> --- a/arch/arm/plat-samsung/dev-adc.c
>> +++ b/arch/arm/plat-samsung/dev-adc.c
>> @@ -22,6 +22,8 @@
>>  #include <plat/devs.h>
>>  #include <plat/cpu.h>
>>
>> +#include "../../../fs/sysfs/sysfs.h"
>
> That is a big hint that you're doing something wrong.
>
[]
>
> This is wrong on just about every level.  It needs to be rewritten to
> avoid poking about in subsystem internal data structures, and you really
> should not be sprint-ing a value to only sscanf it later.
>
> Plus, container_of doesn't return a pointer-error code.
>
> You need to come up with a far better way of doing this altogether.
>

Um.. is there any clean and nice way to read values that are exported to sysfs?

Or if the values are only exported through sysfs (HWMON), is the only
clean way to read such values (w/o extending HWMON itself) to seperate
the HWMON device driver into two pieces: a platform driver that
provide values to somewhere sharable in kernel and a HWMON driver that
reads the values and exports them to sysfs?



Thanks.

- MyungJoo

Linus Walleij July 1, 2011, 3:15 p.m. UTC | #2

On Fri, Jul 1, 2011 at 2:28 AM, MyungJoo Ham <[e-mail]> wrote:

> Or if the values are only exported through sysfs (HWMON), is the only
> clean way to read such values (w/o extending HWMON itself) to seperate
> the HWMON device driver into two pieces: a platform driver that
> provide values to somewhere sharable in kernel and a HWMON driver that
> reads the values and exports them to sysfs?

Sounds like you could very well separate out the ADC part and have
functions for reading values off them? Compare
drivers/mfd/ab8500-gpadc.c for example, this will be used by a
yet to be submitted HWMON driver for doing ADC.

There is no clean nice ADC subsystem though, and *that* feels
like a big problem to drivers like this (and no I don't like the AB8500
GPADC living in MFD either) so if you'd like to create a ADC
subsystem just go ahead, there is something in drivers/staging/iio/adc
but that had another problem last time I looked: it was only intended
for userspace control, not in-kernel use.

Linus Walleij

Mark Brown July 1, 2011, 3:59 p.m. UTC | #3

On Fri, Jul 01, 2011 at 05:15:05PM +0200, Linus Walleij wrote:

> There is no clean nice ADC subsystem though, and *that* feels
> like a big problem to drivers like this (and no I don't like the AB8500
> GPADC living in MFD either) so if you'd like to create a ADC
> subsystem just go ahead, there is something in drivers/staging/iio/adc
> but that had another problem last time I looked: it was only intended
> for userspace control, not in-kernel use.

The other issue is that it's designed for high volume data and these
AUXADCs tend to be relatively slow.  That said I've had some discussions
with Jonathan about supporting these devices via IIO and he seems to be
coming round to the idea that we could integrate support.  Not sure how
fast IIO is progressing to mainline, though.

Arnd Bergmann July 1, 2011, 5:23 p.m. UTC | #4

On Friday 01 July 2011, Mark Brown wrote:
> On Fri, Jul 01, 2011 at 05:15:05PM +0200, Linus Walleij wrote:
> 
> > There is no clean nice ADC subsystem though, and that feels
> > like a big problem to drivers like this (and no I don't like the AB8500
> > GPADC living in MFD either) so if you'd like to create a ADC
> > subsystem just go ahead, there is something in drivers/staging/iio/adc
> > but that had another problem last time I looked: it was only intended
> > for userspace control, not in-kernel use.

Agreed.

> The other issue is that it's designed for high volume data and these
> AUXADCs tend to be relatively slow.  That said I've had some discussions
> with Jonathan about supporting these devices via IIO and he seems to be
> coming round to the idea that we could integrate support.  Not sure how
> fast IIO is progressing to mainline, though.

I think IIO is progressing well and I would rather base products on that
while it's in staging than having a random out-of-tree driver. It seems
to fit the purpose of this driver, though not the in-kernel uses that
Linus mentioned above.

We have a bunch of things that seem to me like they should live together:
gpio, pinmux, pwm and adc. There are ongoing discussions about pinmux
and pwm subsystems right now, so it might be the right time to discuss
where we are going with these in general.

With regard to IIO, that could live on a higher level and just provide
a user interface on top of the generic in-kernel abstraction for adc.

	Arnd

Mark Brown July 1, 2011, 8:10 p.m. UTC | #5

On Fri, Jul 01, 2011 at 07:23:13PM +0200, Arnd Bergmann wrote:
> On Friday 01 July 2011, Mark Brown wrote:

> > The other issue is that it's designed for high volume data and these
> > AUXADCs tend to be relatively slow.  That said I've had some discussions
> > with Jonathan about supporting these devices via IIO and he seems to be
> > coming round to the idea that we could integrate support.  Not sure how
> > fast IIO is progressing to mainline, though.

> I think IIO is progressing well and I would rather base products on that
> while it's in staging than having a random out-of-tree driver. It seems

It's certainly progressing well - looks really nice right now.

> to fit the purpose of this driver, though not the in-kernel uses that
> Linus mentioned above.

The in kernel uses are the main usage of this sort of ADC in most
systems - you get a general purpose ADC used for (usually) voltage and
temperature monitoring with a few auxiliary inputs available for random
system specific things but generally not actually used so often (jack
detection stuff is the main use case).  The power supply and hwmon
subsystems are the main users.

> With regard to IIO, that could live on a higher level and just provide
> a user interface on top of the generic in-kernel abstraction for adc.

Yeah, that's roughly the option that Jonathan and myself had been
discussing last time this came up - have the convertor level deliver
events to a pluggable API layer with the IIO userspace be just one of
those plugins.

Patch

diff --git a/arch/arm/plat-samsung/dev-adc.c b/arch/arm/plat-samsung/dev-adc.c
index 622972c..526097a 100644
--- a/arch/arm/plat-samsung/dev-adc.c
+++ b/arch/arm/plat-samsung/dev-adc.c
@@ -22,6 +22,8 @@ 
 #include <plat/devs.h>
 #include <plat/cpu.h>
 
+#include "../../../fs/sysfs/sysfs.h"
+
 static struct resource s3c_adc_resource[] = {
 	[0] = {
 		.start = SAMSUNG_PA_ADC,
@@ -101,3 +103,63 @@  struct platform_device s3c_device_adc_ntc_thermistor = {
 		.platform_data = &ntc_adc_pdata,
 	},
 };
+
+static struct device_attribute *ntc_attr;
+
+static int init_s3c_adc_ntc_read(void)
+{
+	struct kobject *ntc;
+	struct sysfs_dirent *ntc_d;
+
+	ntc = &s3c_device_adc_ntc_thermistor.dev.kobj;
+	ntc_d = sysfs_get_dirent(ntc->sd, get_ktype(ntc)->namespace(ntc),
+				 "temp1_input");
+	if (!ntc_d || sysfs_type(ntc_d) != SYSFS_KOBJ_ATTR) {
+		dev_err(&s3c_device_adc_ntc_thermistor.dev,
+			"Cannot initialize thermistor dirent info.\n");
+		if (ntc_d)
+			sysfs_put(ntc_d);
+		return -ENODEV;
+	}
+	ntc_attr = container_of(ntc_d->s_attr.attr, struct device_attribute,
+				attr);
+
+	sysfs_put(ntc_d);
+	if (IS_ERR(ntc_attr)) {
+		dev_err(&s3c_device_adc_ntc_thermistor.dev,
+			"Cannot access NTC thermistor.\n");
+		return PTR_ERR(ntc_attr);
+	}
+
+	return 0;
+}
+
+/* A helper function to read values from NTC, in 1/1000 Centigrade */
+int read_s3c_adc_ntc(int *mC)
+{
+	char buf[32];
+	int ret;
+
+	/* init should be done after ADC and NTC are probed */
+	if (ntc_attr == NULL) {
+		ret = init_s3c_adc_ntc_read();
+		if (ret) {
+			if (ntc_attr == NULL)
+				ntc_attr = ERR_PTR(ret);
+			return ret;
+		}
+	}
+
+	if (IS_ERR(ntc_attr))
+		return -ENODEV;
+
+	if (!ntc_attr->show)
+		return -EINVAL;
+
+	ret = ntc_attr->show(&s3c_device_adc_ntc_thermistor.dev, ntc_attr, buf);
+	if (ret < 0)
+		return ret;
+	sscanf(buf, "%d", mC);
+
+	return 0;
+}
diff --git a/arch/arm/plat-samsung/include/plat/adc-ntc.h b/arch/arm/plat-samsung/include/plat/adc-ntc.h
new file mode 100644
index 0000000..3d74118
--- /dev/null
+++ b/arch/arm/plat-samsung/include/plat/adc-ntc.h
@@ -0,0 +1,19 @@ 
+/* linux/arch/arm/plat-samsung/include/plat/adc-ntc.h
+ *
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * NTC Thermistor attached to Samsung ADC Controller driver information
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#ifndef __PLAT_ADC_NTC_H
+#define __PLAT_ADC_NTC_H __FILE__
+
+extern void s3c_adc_ntc_init(int port);
+extern int read_s3c_adc_ntc(int *mC);
+
+#endif /* __PLAT_ADC_NTC_H */