Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module

[Samuel Ortiz]

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 j-keer...@ti.com wrote:
  On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck
  guenter.ro...@ericsson.com 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

[J, KEERTHY]

On Fri, Jan 7, 2011 at 5:42 PM, J, KEERTHY j-keer...@ti.com wrote:
 On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck
 guenter.ro...@ericsson.com 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

[J, KEERTHY]

On Thu, Jan 6, 2011 at 5:34 PM, Mark Brown
broo...@opensource.wolfsonmicro.com 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 temperatures should be temp_
 and if the inputs are from known sources it'd be good to label them.

Yes some are voltages and some are fixed to particular inputs.
I will label them.

 +  

Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module

[Mark Brown]

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

[J, KEERTHY]

On Fri, Jan 7, 2011 at 4:14 PM, Mark Brown
broo...@opensource.wolfsonmicro.com 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

[J, KEERTHY]

On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck
guenter.ro...@ericsson.com 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

[Guenter Roeck]

On Fri, Jan 07, 2011 at 07:12:13AM -0500, J, KEERTHY wrote:
 On Fri, Jan 7, 2011 at 3:25 AM, Guenter Roeck
 guenter.ro...@ericsson.com 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

[Mark Brown]

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);

A MODULE_ALIAS to enable automatic loading of teh driver would also be
good.
--
To unsubscribe from this list: send the 

Re: [lm-sensors] [PATCH v3] hwmon: twl4030: Driver for twl4030 madc module

[Mark Brown]

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

[Guenter Roeck]

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

[Mark Brown]

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

[Guenter Roeck]

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