| Message ID | 1309422387-11546-3-git-send-email-myungjoo.ham@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 30, 2011 at 6:00 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> 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
On Fri, Jul 1, 2011 at 2:28 AM, MyungJoo Ham <myungjoo.ham@samsung.com> 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
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.
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
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.
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 */