Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
Hi Keerthy, On Sat, Jan 22, 2011 at 10:19:33PM +0530, J, KEERTHY wrote: > On Fri, Jan 7, 2011 at 5:42 PM, J, KEERTHY wrote: > > On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck > > wrote: > >> On Thu, 2011-01-06 at 15:21 -0500, Mark Brown wrote: > >>> On Thu, Jan 06, 2011 at 07:04:30AM -0800, Guenter Roeck wrote: > >>> > On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: > >>> > >>> > > Why? It's not like hwmon has an unreasonably large core or similar. > >>> > >>> > Because it creates an unnecessary dependency, and because it is not > >>> > hwmon's > >>> > responsibility to provide infrastructure for other subsystems or > >>> > drivers. > >>> > >>> hwmon isn't really doing anything, though. The *driver* is doing > >>> something but it doesn't really impact the core that much. Not that I'm > >>> particularly sold on putting the ADC core in here, but total NACK based > >>> on that alone seems rather harsh. > >> > >> Possibly. However, I had suggested the following earlier (to the 1st > >> version of the patch): > >> > >>> I commented on this a couple of times below - the driver mixes generic > >>> ADC reading functions with hwmon functionality. Generic ADC reading > >>> functionality should be moved into another driver, possibly to mfd. > >> > >> Obviously that was ignored. Maybe someone is willing to listen this time > >> around. > >> > > I am sorry for not responding on the generic ADC handling part. Since many > > other ADC drivers are part of hwmon i thought hwmon was the appropriate > > place. However I can surely split the generic ADC handling part in mfd and > > only hardware monitoring part in hwmon as suggested. > > > >> I won't let people break modularity just for convenience in a subsystem > >> I am responsible for. And forcing the hwmon subsystem, and with it a > >> specific hwmon driver, to exist just because the adc functionality it > >> provides is needed by some other (most likely unrelated) subsystem / > >> driver _does_ break modularity. Worse, it is completely unnecessary to > >> do so. Other twl4030 functionality was extracted into generic code. > >> twl-core.c, twl4030-codec.c, twl4030-irq.c, twl4030-power.c are all in > >> mfd. I fail to see the problem with mfd/twl4030-adc.c. > >> > >> Guenter > >> > >> > >> > > > > Hello Samuel, > > > > Is it ok to have the generic ADC functionality in mfd as a separate file? > > > > Regards, > > Keerthy > > > > Hello Samuel, > > We need your valuable inputs. Can generic ADC functionality can be in > mfd as a separate file? If it's really generic and doesn't mix hwmon stuff in the middle, then yes. Cheers, Samuel. -- Intel Open Source Technology Centre http://oss.intel.com/ -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Fri, Jan 7, 2011 at 5:42 PM, J, KEERTHY wrote: > On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck > wrote: >> On Thu, 2011-01-06 at 15:21 -0500, Mark Brown wrote: >>> On Thu, Jan 06, 2011 at 07:04:30AM -0800, Guenter Roeck wrote: >>> > On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: >>> >>> > > Why? It's not like hwmon has an unreasonably large core or similar. >>> >>> > Because it creates an unnecessary dependency, and because it is not >>> > hwmon's >>> > responsibility to provide infrastructure for other subsystems or drivers. >>> >>> hwmon isn't really doing anything, though. The *driver* is doing >>> something but it doesn't really impact the core that much. Not that I'm >>> particularly sold on putting the ADC core in here, but total NACK based >>> on that alone seems rather harsh. >> >> Possibly. However, I had suggested the following earlier (to the 1st >> version of the patch): >> >>> I commented on this a couple of times below - the driver mixes generic >>> ADC reading functions with hwmon functionality. Generic ADC reading >>> functionality should be moved into another driver, possibly to mfd. >> >> Obviously that was ignored. Maybe someone is willing to listen this time >> around. >> > I am sorry for not responding on the generic ADC handling part. Since many > other ADC drivers are part of hwmon i thought hwmon was the appropriate > place. However I can surely split the generic ADC handling part in mfd and > only hardware monitoring part in hwmon as suggested. > >> I won't let people break modularity just for convenience in a subsystem >> I am responsible for. And forcing the hwmon subsystem, and with it a >> specific hwmon driver, to exist just because the adc functionality it >> provides is needed by some other (most likely unrelated) subsystem / >> driver _does_ break modularity. Worse, it is completely unnecessary to >> do so. Other twl4030 functionality was extracted into generic code. >> twl-core.c, twl4030-codec.c, twl4030-irq.c, twl4030-power.c are all in >> mfd. I fail to see the problem with mfd/twl4030-adc.c. >> >> Guenter >> >> >> > > Hello Samuel, > > Is it ok to have the generic ADC functionality in mfd as a separate file? > > Regards, > Keerthy > Hello Samuel, We need your valuable inputs. Can generic ADC functionality can be in mfd as a separate file? Regards and Thanks, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Fri, Jan 07, 2011 at 07:12:13AM -0500, J, KEERTHY wrote: > On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck > wrote: > > On Thu, 2011-01-06 at 15:21 -0500, Mark Brown wrote: > >> On Thu, Jan 06, 2011 at 07:04:30AM -0800, Guenter Roeck wrote: > >> > On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: > >> > >> > > Why? It's not like hwmon has an unreasonably large core or similar. > >> > >> > Because it creates an unnecessary dependency, and because it is not > >> > hwmon's > >> > responsibility to provide infrastructure for other subsystems or drivers. > >> > >> hwmon isn't really doing anything, though. The *driver* is doing > >> something but it doesn't really impact the core that much. Not that I'm > >> particularly sold on putting the ADC core in here, but total NACK based > >> on that alone seems rather harsh. > > > > Possibly. However, I had suggested the following earlier (to the 1st > > version of the patch): > > > >> I commented on this a couple of times below - the driver mixes generic > >> ADC reading functions with hwmon functionality. Generic ADC reading > >> functionality should be moved into another driver, possibly to mfd. > > > > Obviously that was ignored. Maybe someone is willing to listen this time > > around. > > > I am sorry for not responding on the generic ADC handling part. Since many > other ADC drivers are part of hwmon i thought hwmon was the appropriate > place. However I can surely split the generic ADC handling part in mfd and > only hardware monitoring part in hwmon as suggested. > Other drivers don't _export_ that functionality. Key difference. Sure, the lis3 driver does, but that should not be in hwmon in the first place and is on the way out (if I ever get to do it), and max is just setting a bad example. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck wrote: > On Thu, 2011-01-06 at 15:21 -0500, Mark Brown wrote: >> On Thu, Jan 06, 2011 at 07:04:30AM -0800, Guenter Roeck wrote: >> > On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: >> >> > > Why? It's not like hwmon has an unreasonably large core or similar. >> >> > Because it creates an unnecessary dependency, and because it is not hwmon's >> > responsibility to provide infrastructure for other subsystems or drivers. >> >> hwmon isn't really doing anything, though. The *driver* is doing >> something but it doesn't really impact the core that much. Not that I'm >> particularly sold on putting the ADC core in here, but total NACK based >> on that alone seems rather harsh. > > Possibly. However, I had suggested the following earlier (to the 1st > version of the patch): > >> I commented on this a couple of times below - the driver mixes generic >> ADC reading functions with hwmon functionality. Generic ADC reading >> functionality should be moved into another driver, possibly to mfd. > > Obviously that was ignored. Maybe someone is willing to listen this time > around. > I am sorry for not responding on the generic ADC handling part. Since many other ADC drivers are part of hwmon i thought hwmon was the appropriate place. However I can surely split the generic ADC handling part in mfd and only hardware monitoring part in hwmon as suggested. > I won't let people break modularity just for convenience in a subsystem > I am responsible for. And forcing the hwmon subsystem, and with it a > specific hwmon driver, to exist just because the adc functionality it > provides is needed by some other (most likely unrelated) subsystem / > driver _does_ break modularity. Worse, it is completely unnecessary to > do so. Other twl4030 functionality was extracted into generic code. > twl-core.c, twl4030-codec.c, twl4030-irq.c, twl4030-power.c are all in > mfd. I fail to see the problem with mfd/twl4030-adc.c. > > Guenter > > > Hello Samuel, Is it ok to have the generic ADC functionality in mfd as a separate file? Regards, Keerthy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Fri, Jan 7, 2011 at 4:14 PM, Mark Brown
wrote:
> On Fri, Jan 07, 2011 at 02:55:45PM +0530, J, KEERTHY wrote:
>> On Thu, Jan 6, 2011 at 5:34 PM, Mark Brown
>> > On Thu, Jan 06, 2011 at 09:26:40AM +0530, Keerthy wrote:
>
>> >> +static ssize_t madc_read(struct device *dev,
>> >> + struct device_attribute *devattr, char *buf)
>> >> +{
>> >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>
> ...
>
>> >> + return sprintf(buf, "%ld\n", val);
>
>> > Does this return data in the appropriate units - milivolts for voltages?
>
>> This function returns the raw channel values read and not in terms of the
>> units.
>
> It needs to convert the values into real world units before they go to
> userspace - look at what applications like sensors do with the values.
>
Ok. Converting to the real world units will be added.
>> >> + case TWL4030_MADC_RT:
>> >> + default:
>> >> + break;
>
>> > This looks odd - the function won't actually do anything for non-SW
>> > conversions but won't return an error?
>
>> Ok. In the case of RT conversion request no software setting is required
>> and it is signal driven. So the function does nothing in case of RT.
>
> Again, the biggest problem is that the code isn't clear.
OK. I will add comments explaining the flow.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Fri, Jan 07, 2011 at 02:55:45PM +0530, J, KEERTHY wrote:
> On Thu, Jan 6, 2011 at 5:34 PM, Mark Brown
> > On Thu, Jan 06, 2011 at 09:26:40AM +0530, Keerthy wrote:
> >> +static ssize_t madc_read(struct device *dev,
> >> + struct device_attribute *devattr, char *buf)
> >> +{
> >> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
...
> >> + return sprintf(buf, "%ld\n", val);
> > Does this return data in the appropriate units - milivolts for voltages?
> This function returns the raw channel values read and not in terms of the
> units.
It needs to convert the values into real world units before they go to
userspace - look at what applications like sensors do with the values.
> >> + case TWL4030_MADC_RT:
> >> + default:
> >> + break;
> > This looks odd - the function won't actually do anything for non-SW
> > conversions but won't return an error?
> Ok. In the case of RT conversion request no software setting is required
> and it is signal driven. So the function does nothing in case of RT.
Again, the biggest problem is that the code isn't clear.
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Thu, Jan 6, 2011 at 5:34 PM, Mark Brown
wrote:
> On Thu, Jan 06, 2011 at 09:26:40AM +0530, Keerthy wrote:
>
>> ---
>> drivers/hwmon/Kconfig | 11 +
>> drivers/hwmon/Makefile | 1 +
>> drivers/hwmon/twl4030-madc.c | 794
>> ++
>> include/linux/i2c/twl4030-madc.h | 118 ++
>> 4 files changed, 924 insertions(+), 0 deletions(-)
>> create mode 100644 drivers/hwmon/twl4030-madc.c
>> create mode 100644 include/linux/i2c/twl4030-madc.h
>
> hwmon drivers are also expected to have a file under Documentation.
I will add a documentation file.
>
>> +struct twl4030_madc_data {
>> + struct device *hwmon_dev;
>> + struct mutex lock;/* mutex protecting this data structire */
>> + struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
>> + int imr;
>> + int isr;
>> +};
>
>> +static struct twl4030_madc_data *twl4030_madc;
>
> I'd expect this to be per driver instance rather than global (I know
> it's vanishingly unlikely that you'll get multiple twl4030s in a single
> system but it's nicer).
>
>> +/*
>> + * sysfs hook function
>> + */
>> +static ssize_t madc_read(struct device *dev,
>> + struct device_attribute *devattr, char *buf)
>> +{
>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>> + struct twl4030_madc_request req;
>> + long val;
>> +
>> + req.channels = (1 << attr->index);
>> + req.method = TWL4030_MADC_SW2;
>> + req.func_cb = NULL;
>> + val = twl4030_madc_conversion(&req);
>> + if (val >= 0)
>> + val = req.rbuf[attr->index];
>> + else
>> + return val;
>> + return sprintf(buf, "%ld\n", val);
>
> Does this return data in the appropriate units - milivolts for voltages?
This function returns the raw channel values read and not in terms of the
units.
>
>> +/*
>> + * Enables irq.
>> + * @madc - pointer to twl4030_madc_data struct
>> + * @id - irq number to be enabled
>> + * can take one of TWL4030_MADC_RT, TWL4030_MADC_SW1, TWL4030_MADC_SW2
>> + * corresponding to RT, SW1, SW2 conversion requests.
>> + * If the i2c read fails it returns an error else returns 0.
>> + */
>> +static int twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
>
> It'd be good to clarify that this is interrupt sources within this
> module rather than Linux interrupt numbers.
Ok.
>
>> + dev_dbg(madc->hwmon_dev,
>> + "Disable interrupt failed%d\n", i);
>> + }
>> +
>> + madc->requests[i].result_pending = 1;
>> + }
>> + mutex_lock(&madc->lock);
>> + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
>> +
>> + r = &madc->requests[i];
>
> In general through the driver your use of blank lines is really odd
> which doesn't help readability.
Ok. I will correct it.
>
>> + switch (conv_method) {
>> + case TWL4030_MADC_SW1:
>> + case TWL4030_MADC_SW2:
>> + ret = twl_i2c_write_u8(TWL4030_MODULE_MADC,
>> + TWL4030_MADC_SW_START, method->ctrl);
>> + if (ret) {
>> + dev_err(madc->hwmon_dev,
>> + "unable to write ctrl register 0x%X\n", method->ctrl);
>> + return ret;
>> + }
>> + break;
>> + case TWL4030_MADC_RT:
>> + default:
>> + break;
>
> This looks odd - the function won't actually do anything for non-SW
> conversions but won't return an error?
Ok. In the case of RT conversion request no software setting is required
and it is signal driven. So the function does nothing in case of RT.
>
>> +/* sysfs nodes to read individual channels from user side */
>> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, madc_read, NULL, 0);
>> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, madc_read, NULL, 1);
>> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, madc_read, NULL, 2);
>> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, madc_read, NULL, 3);
>> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, madc_read, NULL, 4);
>> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, madc_read, NULL, 5);
>> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, madc_read, NULL, 6);
>> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, madc_read, NULL, 7);
>> +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, madc_read, NULL, 8);
>> +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, madc_read, NULL, 9);
>> +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO, madc_read, NULL, 10);
>> +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO, madc_read, NULL, 11);
>> +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO, madc_read, NULL, 12);
>> +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, madc_read, NULL, 13);
>> +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, madc_read, NULL, 14);
>> +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, madc_read, NULL, 15);
>
> I suspect some of these are temperatures, some are voltages and that
> some are fixed to particular inputs? The
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Thu, 2011-01-06 at 15:21 -0500, Mark Brown wrote: > On Thu, Jan 06, 2011 at 07:04:30AM -0800, Guenter Roeck wrote: > > On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: > > > > Why? It's not like hwmon has an unreasonably large core or similar. > > > Because it creates an unnecessary dependency, and because it is not hwmon's > > responsibility to provide infrastructure for other subsystems or drivers. > > hwmon isn't really doing anything, though. The *driver* is doing > something but it doesn't really impact the core that much. Not that I'm > particularly sold on putting the ADC core in here, but total NACK based > on that alone seems rather harsh. Possibly. However, I had suggested the following earlier (to the 1st version of the patch): > I commented on this a couple of times below - the driver mixes generic > ADC reading functions with hwmon functionality. Generic ADC reading > functionality should be moved into another driver, possibly to mfd. Obviously that was ignored. Maybe someone is willing to listen this time around. I won't let people break modularity just for convenience in a subsystem I am responsible for. And forcing the hwmon subsystem, and with it a specific hwmon driver, to exist just because the adc functionality it provides is needed by some other (most likely unrelated) subsystem / driver _does_ break modularity. Worse, it is completely unnecessary to do so. Other twl4030 functionality was extracted into generic code. twl-core.c, twl4030-codec.c, twl4030-irq.c, twl4030-power.c are all in mfd. I fail to see the problem with mfd/twl4030-adc.c. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Thu, Jan 06, 2011 at 07:04:30AM -0800, Guenter Roeck wrote: > On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: > > Why? It's not like hwmon has an unreasonably large core or similar. > Because it creates an unnecessary dependency, and because it is not hwmon's > responsibility to provide infrastructure for other subsystems or drivers. hwmon isn't really doing anything, though. The *driver* is doing something but it doesn't really impact the core that much. Not that I'm particularly sold on putting the ADC core in here, but total NACK based on that alone seems rather harsh. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Thu, Jan 06, 2011 at 07:07:13AM -0500, Mark Brown wrote: > On Wed, Jan 05, 2011 at 09:33:28PM -0800, Guenter Roeck wrote: > > > [...] > > > +EXPORT_SYMBOL_GPL(twl4030_madc_conversion); > > [...] > > > +EXPORT_SYMBOL_GPL(twl4030_get_madc_conversion); > > > No symbol export from hwmon drivers. Other parts of the kernel > > should not depend on HWMON configuration. > > Why? It's not like hwmon has an unreasonably large core or similar. > Because it creates an unnecessary dependency, and because it is not hwmon's responsibility to provide infrastructure for other subsystems or drivers. > > I would suggest to check if drivers/staging/iio would be a better fit. > > That does have the problem that it's in staging and constantly churning, > though. When I've looked at it it seemed like awfully hard work to use > for devices like this. > > What I've done in some of my drivers is put the ADC core in the MFD core > (it's used by both hwmon and power supply function drivers, plus any > board specific stuff people do). Fine as well. I think I had suggested that earlier already. Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Wed, Jan 05, 2011 at 09:33:28PM -0800, Guenter Roeck wrote: > [...] > > +EXPORT_SYMBOL_GPL(twl4030_madc_conversion); > [...] > > +EXPORT_SYMBOL_GPL(twl4030_get_madc_conversion); > No symbol export from hwmon drivers. Other parts of the kernel > should not depend on HWMON configuration. Why? It's not like hwmon has an unreasonably large core or similar. > I would suggest to check if drivers/staging/iio would be a better fit. That does have the problem that it's in staging and constantly churning, though. When I've looked at it it seemed like awfully hard work to use for devices like this. What I've done in some of my drivers is put the ADC core in the MFD core (it's used by both hwmon and power supply function drivers, plus any board specific stuff people do). -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module
On Thu, Jan 06, 2011 at 09:26:40AM +0530, Keerthy wrote:
> ---
> drivers/hwmon/Kconfig| 11 +
> drivers/hwmon/Makefile |1 +
> drivers/hwmon/twl4030-madc.c | 794
> ++
> include/linux/i2c/twl4030-madc.h | 118 ++
> 4 files changed, 924 insertions(+), 0 deletions(-)
> create mode 100644 drivers/hwmon/twl4030-madc.c
> create mode 100644 include/linux/i2c/twl4030-madc.h
hwmon drivers are also expected to have a file under Documentation.
> +struct twl4030_madc_data {
> + struct device *hwmon_dev;
> + struct mutex lock;/* mutex protecting this data structire */
> + struct twl4030_madc_request requests[TWL4030_MADC_NUM_METHODS];
> + int imr;
> + int isr;
> +};
> +static struct twl4030_madc_data *twl4030_madc;
I'd expect this to be per driver instance rather than global (I know
it's vanishingly unlikely that you'll get multiple twl4030s in a single
system but it's nicer).
> +/*
> + * sysfs hook function
> + */
> +static ssize_t madc_read(struct device *dev,
> + struct device_attribute *devattr, char *buf)
> +{
> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
> + struct twl4030_madc_request req;
> + long val;
> +
> + req.channels = (1 << attr->index);
> + req.method = TWL4030_MADC_SW2;
> + req.func_cb = NULL;
> + val = twl4030_madc_conversion(&req);
> + if (val >= 0)
> + val = req.rbuf[attr->index];
> + else
> + return val;
> + return sprintf(buf, "%ld\n", val);
Does this return data in the appropriate units - milivolts for voltages?
> +/*
> + * Enables irq.
> + * @madc - pointer to twl4030_madc_data struct
> + * @id - irq number to be enabled
> + * can take one of TWL4030_MADC_RT, TWL4030_MADC_SW1, TWL4030_MADC_SW2
> + * corresponding to RT, SW1, SW2 conversion requests.
> + * If the i2c read fails it returns an error else returns 0.
> + */
> +static int twl4030_madc_enable_irq(struct twl4030_madc_data *madc, int id)
It'd be good to clarify that this is interrupt sources within this
module rather than Linux interrupt numbers.
> + dev_dbg(madc->hwmon_dev,
> + "Disable interrupt failed%d\n", i);
> + }
> +
> + madc->requests[i].result_pending = 1;
> + }
> + mutex_lock(&madc->lock);
> + for (i = 0; i < TWL4030_MADC_NUM_METHODS; i++) {
> +
> + r = &madc->requests[i];
In general through the driver your use of blank lines is really odd
which doesn't help readability.
> + switch (conv_method) {
> + case TWL4030_MADC_SW1:
> + case TWL4030_MADC_SW2:
> + ret = twl_i2c_write_u8(TWL4030_MODULE_MADC,
> + TWL4030_MADC_SW_START, method->ctrl);
> + if (ret) {
> + dev_err(madc->hwmon_dev,
> + "unable to write ctrl register 0x%X\n", method->ctrl);
> + return ret;
> + }
> + break;
> + case TWL4030_MADC_RT:
> + default:
> + break;
This looks odd - the function won't actually do anything for non-SW
conversions but won't return an error?
> +/* sysfs nodes to read individual channels from user side */
> +static SENSOR_DEVICE_ATTR(in0_input, S_IRUGO, madc_read, NULL, 0);
> +static SENSOR_DEVICE_ATTR(in1_input, S_IRUGO, madc_read, NULL, 1);
> +static SENSOR_DEVICE_ATTR(in2_input, S_IRUGO, madc_read, NULL, 2);
> +static SENSOR_DEVICE_ATTR(in3_input, S_IRUGO, madc_read, NULL, 3);
> +static SENSOR_DEVICE_ATTR(in4_input, S_IRUGO, madc_read, NULL, 4);
> +static SENSOR_DEVICE_ATTR(in5_input, S_IRUGO, madc_read, NULL, 5);
> +static SENSOR_DEVICE_ATTR(in6_input, S_IRUGO, madc_read, NULL, 6);
> +static SENSOR_DEVICE_ATTR(in7_input, S_IRUGO, madc_read, NULL, 7);
> +static SENSOR_DEVICE_ATTR(in8_input, S_IRUGO, madc_read, NULL, 8);
> +static SENSOR_DEVICE_ATTR(in9_input, S_IRUGO, madc_read, NULL, 9);
> +static SENSOR_DEVICE_ATTR(in10_input, S_IRUGO, madc_read, NULL, 10);
> +static SENSOR_DEVICE_ATTR(in11_input, S_IRUGO, madc_read, NULL, 11);
> +static SENSOR_DEVICE_ATTR(in12_input, S_IRUGO, madc_read, NULL, 12);
> +static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, madc_read, NULL, 13);
> +static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, madc_read, NULL, 14);
> +static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, madc_read, NULL, 15);
I suspect some of these are temperatures, some are voltages and that
some are fixed to particular inputs? The temperatures should be temp_
and if the inputs are from known sources it'd be good to label them.
> + madc->imr = (pdata->irq_line == 1) ?
> + TWL4030_MADC_IMR1 : TWL4030_MADC_IMR2;
> + madc->isr = (pdata->irq_line == 1) ?
> + TWL4030_MADC_ISR1 : TWL4030_MADC_ISR2;
This looks really odd - what's going on here? Comments might help.
> +
> +MODULE_DESCRIPTION("TWL4030 ADC driver");
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("J Keerthy")
