Re: [PATCH v5] hwmon/mc13xxx-adc: add support for the MC13892 PMIC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Sun, 12 Feb 2012 11:09:44 +0100
Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote:

> Based on a patch by David Jander that mostly did s/mc13783/mc13xxx/ .
> 
> Additionally use dev_get_drvdata instead of to_platform_device +
> platform_get_drvdata in mc13783_adc_read (spotted by Jean Delvare).
> 
> Cc: David Jander <david.jander@xxxxxxxxxxx>
> Acked-by: Jean Delvare <khali@xxxxxxxxxxxx>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>
> ---
> changes since v4:
>  - provide touchscreen inputs on mc13892, too
> changes since v3:
>  - drop _adc in Documentation/hwmon/mc13783-adc, too
>  - use dev_get_drvdata instead of to_platform_device +
>    platform_get_drvdata
> changes since v2:
>  - make code generating name attribute more robust and strip [-_]adc
>  - consistenly use MC13783_ADC_16CHANS / mc13783_attr_16chans
> ---
>  Documentation/hwmon/mc13783-adc |   50 +++++++++++++-----
>  drivers/hwmon/Kconfig           |    6 +-
>  drivers/hwmon/mc13783-adc.c     |  107 +++++++++++++++++++++++++++++---------
>  3 files changed, 121 insertions(+), 42 deletions(-)
> 
> diff --git a/Documentation/hwmon/mc13783-adc b/Documentation/hwmon/mc13783-adc
> index 044531a..356b10a 100644
> --- a/Documentation/hwmon/mc13783-adc
> +++ b/Documentation/hwmon/mc13783-adc
> @@ -3,8 +3,11 @@ Kernel driver mc13783-adc
>  
>  Supported chips:
>    * Freescale Atlas MC13783
> -    Prefix: 'mc13783_adc'
> +    Prefix: 'mc13783'
>      Datasheet: http://www.freescale.com/files/rf_if/doc/data_sheet/MC13783.pdf?fsrch=1
> +  * Freescale Atlas MC13892
> +    Prefix: 'mc13892'
> +    Datasheet: http://cache.freescale.com/files/analog/doc/data_sheet/MC13892.pdf?fsrch=1&sr=1
>  
>  Authors:
>      Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> @@ -13,20 +16,21 @@ Authors:
>  Description
>  -----------
>  
> -The Freescale MC13783 is a Power Management and Audio Circuit. Among
> -other things it contains a 10-bit A/D converter. The converter has 16
> -channels which can be used in different modes.
> -The A/D converter has a resolution of 2.25mV. Channels 0-4 have
> -a dedicated meaning with chip internal scaling applied. Channels 5-7
> -can be used as general purpose inputs or alternatively in a dedicated
> -mode. Channels 12-15 are occupied by the touchscreen if it's active.
> +The Freescale MC13783 and MC13892 are Power Management and Audio Circuits.
> +Among other things they contain a 10-bit A/D converter. The converter has 16
> +(MC13783) resp. 12 (MC13892) channels which can be used in different modes. The
> +A/D converter has a resolution of 2.25mV.
>  
> -Currently the driver only supports channels 2 and 5-15 with no alternative
> -modes for channels 5-7.
> +Some channels can be used as General Purpose inputs or in a dedicated mode with
> +a chip internal scaling applied .
>  
> -See this table for the meaning of the different channels and their chip
> -internal scaling:
> +Currently the driver only supports the Application Supply channel (BP / BPSNS),
> +the General Purpose inputs and touchscreen.
>  
> +See the following tables for the meaning of the different channels and their
> +chip internal scaling:
> +
> +MC13783:
>  Channel	Signal						Input Range	Scaling
>  -------------------------------------------------------------------------------
>  0	Battery Voltage (BATT)				2.50 - 4.65V	-2.40V
> @@ -34,7 +38,7 @@ Channel	Signal						Input Range	Scaling
>  2	Application Supply (BP)				2.50 - 4.65V	-2.40V
>  3	Charger Voltage (CHRGRAW)			0 - 10V /	/5
>  							0 - 20V		/10
> -4	Charger Current (CHRGISNSP-CHRGISNSN)		-0.25V - 0.25V	x4
> +4	Charger Current (CHRGISNSP-CHRGISNSN)		-0.25 - 0.25V	x4
>  5	General Purpose ADIN5 / Battery Pack Thermistor	0 - 2.30V	No
>  6	General Purpose ADIN6 / Backup Voltage (LICELL)	0 - 2.30V /	No /
>  							1.50 - 3.50V	-1.20V
> @@ -48,3 +52,23 @@ Channel	Signal						Input Range	Scaling
>  13	General Purpose TSX2 / Touchscreen X-plate 2	0 - 2.30V	No
>  14	General Purpose TSY1 / Touchscreen Y-plate 1	0 - 2.30V	No
>  15	General Purpose TSY2 / Touchscreen Y-plate 2	0 - 2.30V	No
> +
> +MC13892:
> +Channel	Signal						Input Range	Scaling
> +-------------------------------------------------------------------------------
> +0	Battery Voltage (BATT)				0 - 4.8V	/2
> +1	Battery Current (BATT - BATTISNSCC)		-60 - 60 mV	x20
> +2	Application Supply (BPSNS)			0 - 4.8V	/2
> +3	Charger Voltage (CHRGRAW)			0 - 12V /	/5
> +							0 - 20V		/10
> +4	Charger Current (CHRGISNS-BPSNS) /		-0.3 - 0.3V /	x4 /
> +	Touchscreen X-plate 1				0 - 2.4V	No
> +5	General Purpose ADIN5 /	Battery Pack Thermistor	0 - 2.4V	No
> +6	General Purpose ADIN6 / Backup Voltage (LICELL)	0 - 2.4V /	No
> +	Backup Voltage (LICELL)                        	0 - 3.6V	x2/3
> +7	General Purpose ADIN7 / UID / Die Temperature	0 - 2.4V /	No /
> +							0 - 4.8V	/2
> +12	Touchscreen X-plate 1				0 - 2.4V	No
> +13	Touchscreen X-plate 2				0 - 2.4V	No
> +14	Touchscreen Y-plate 1				0 - 2.4V	No
> +15	Touchscreen Y-plate 2				0 - 2.4V	No

Why two different tables?
Both ADC blocks are almost identical. IMHO it would be better to only
highlight the minor differences, instead of making two almost identical tables
that are hard to compare this way.
AFAICR the only differences between the two ADC's are as follows:

- ADC channel 7 has slightly different general-purpose functionality.
- ADC channels 8...11 are not externally accessible on 13892.
- Scaling factors on channels 0, 2 and 6 are actually offsets on the 13783 (Do
  scaling and offsets belong here or in lm_sensors?)
- In touchscreen mode, on the 13892 the 3rd sample is invalid and needs to be
  discarded, whereas on the 13783 it could be used to get one more sample to
  average.

That's about it, correct me if I'm wrong.

> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 0226040..9c020bd 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1355,10 +1355,10 @@ config SENSORS_APPLESMC
>  	  the awesome power of applesmc.
>  
>  config SENSORS_MC13783_ADC
> -        tristate "Freescale MC13783 ADC"
> -        depends on MFD_MC13783
> +        tristate "Freescale MC13783/MC13892 ADC"
> +        depends on MFD_MC13XXX
>          help
> -          Support for the A/D converter on MC13783 PMIC.
> +          Support for the A/D converter on MC13783 and MC13892 PMIC.
>  
>  if ACPI
>  
> diff --git a/drivers/hwmon/mc13783-adc.c b/drivers/hwmon/mc13783-adc.c
> index ef65ab5..10f18dd 100644
> --- a/drivers/hwmon/mc13783-adc.c
> +++ b/drivers/hwmon/mc13783-adc.c
> @@ -1,5 +1,5 @@
>  /*
> - * Driver for the Freescale Semiconductor MC13783 adc.
> + * Driver for the ADC on Freescale Semiconductor MC13783 and MC13892 PMICs.
>   *
>   * Copyright 2004-2007 Freescale Semiconductor, Inc. All Rights Reserved.
>   * Copyright (C) 2009 Sascha Hauer, Pengutronix
> @@ -18,7 +18,7 @@
>   * Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
>   */
>  
> -#include <linux/mfd/mc13783.h>
> +#include <linux/mfd/mc13xxx.h>
>  #include <linux/platform_device.h>
>  #include <linux/hwmon-sysfs.h>
>  #include <linux/kernel.h>
> @@ -28,24 +28,30 @@
>  #include <linux/init.h>
>  #include <linux/err.h>
>  
> -#define MC13783_ADC_NAME	"mc13783-adc"
> +#define DRIVER_NAME	"mc13783-adc"
> +
> +/* platform device id driver data */
> +#define MC13783_ADC_16CHANS	1
> +#define MC13783_ADC_BPDIV2	2
>  
>  struct mc13783_adc_priv {
>  	struct mc13xxx *mc13xxx;
>  	struct device *hwmon_dev;
> +	char name[10];
>  };
>  
>  static ssize_t mc13783_adc_show_name(struct device *dev, struct device_attribute
>  			      *devattr, char *buf)
>  {
> -	return sprintf(buf, "mc13783_adc\n");
> +	struct mc13783_adc_priv *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", priv->name);
>  }
>  
>  static int mc13783_adc_read(struct device *dev,
>  		struct device_attribute *devattr, unsigned int *val)
>  {
> -	struct platform_device *pdev = to_platform_device(dev);
> -	struct mc13783_adc_priv *priv = platform_get_drvdata(pdev);
> +	struct mc13783_adc_priv *priv = dev_get_drvdata(dev);
>  	struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr);
>  	unsigned int channel = attr->index;
>  	unsigned int sample[4];
> @@ -68,16 +74,21 @@ static ssize_t mc13783_adc_read_bp(struct device *dev,
>  		struct device_attribute *devattr, char *buf)
>  {
>  	unsigned val;
> +	struct platform_device *pdev = to_platform_device(dev);
> +	kernel_ulong_t driver_data = platform_get_device_id(pdev)->driver_data;
>  	int ret = mc13783_adc_read(dev, devattr, &val);
>  
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * BP (channel 2) reports with offset 2.4V to the actual value to fit
> -	 * the input range of the ADC.  unit = 2.25mV = 9/4 mV.
> -	 */
> -	val = DIV_ROUND_CLOSEST(val * 9, 4) + 2400;
> +	if (driver_data & MC13783_ADC_BPDIV2)
> +		val = DIV_ROUND_CLOSEST(val * 9, 2);
> +	else
> +		/*
> +		 * BP (channel 2) reports with offset 2.4V to the actual value
> +		 * to fit the input range of the ADC.  unit = 2.25mV = 9/4 mV.
> +		 */
> +		val = DIV_ROUND_CLOSEST(val * 9, 4) + 2400;
>  
>  	return sprintf(buf, "%u\n", val);
>  }
> @@ -114,12 +125,21 @@ static SENSOR_DEVICE_ATTR(in13_input, S_IRUGO, mc13783_adc_read_gp, NULL, 13);
>  static SENSOR_DEVICE_ATTR(in14_input, S_IRUGO, mc13783_adc_read_gp, NULL, 14);
>  static SENSOR_DEVICE_ATTR(in15_input, S_IRUGO, mc13783_adc_read_gp, NULL, 15);
>  
> -static struct attribute *mc13783_attr[] = {
> +static struct attribute *mc13783_attr_base[] = {
>  	&dev_attr_name.attr,
>  	&sensor_dev_attr_in2_input.dev_attr.attr,
>  	&sensor_dev_attr_in5_input.dev_attr.attr,
>  	&sensor_dev_attr_in6_input.dev_attr.attr,
>  	&sensor_dev_attr_in7_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group mc13783_group_base = {
> +	.attrs = mc13783_attr_base,
> +};
> +
> +/* these are only used if MC13783_ADC_16CHANS is provided in driver data */
> +static struct attribute *mc13783_attr_16chans[] = {
>  	&sensor_dev_attr_in8_input.dev_attr.attr,
>  	&sensor_dev_attr_in9_input.dev_attr.attr,
>  	&sensor_dev_attr_in10_input.dev_attr.attr,
> @@ -127,8 +147,8 @@ static struct attribute *mc13783_attr[] = {
>  	NULL
>  };
>  
> -static const struct attribute_group mc13783_group = {
> -	.attrs = mc13783_attr,
> +static const struct attribute_group mc13783_group_16chans = {
> +	.attrs = mc13783_attr_16chans,
>  };
>  
>  /* last four channels may be occupied by the touchscreen */
> @@ -156,24 +176,39 @@ static int __init mc13783_adc_probe(struct platform_device *pdev)
>  {
>  	struct mc13783_adc_priv *priv;
>  	int ret;
> +	const struct platform_device_id *id = platform_get_device_id(pdev);
> +	char *dash;
>  
>  	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>  	if (!priv)
>  		return -ENOMEM;
>  
>  	priv->mc13xxx = dev_get_drvdata(pdev->dev.parent);
> +	snprintf(priv->name, ARRAY_SIZE(priv->name), "%s", id->name);
> +	dash = strchr(priv->name, '-');
> +	if (dash)
> +		*dash = '\0';
>  
>  	platform_set_drvdata(pdev, priv);
>  
>  	/* Register sysfs hooks */
> -	ret = sysfs_create_group(&pdev->dev.kobj, &mc13783_group);
> +	ret = sysfs_create_group(&pdev->dev.kobj, &mc13783_group_base);
>  	if (ret)
> -		goto out_err_create1;
> +		goto out_err_create_base;
> +
> +	if (id->driver_data & MC13783_ADC_16CHANS) {
> +		ret = sysfs_create_group(&pdev->dev.kobj,
> +				&mc13783_group_16chans);
> +		if (ret)
> +			goto out_err_create_16chans;
> +
> +	}
>  
>  	if (!mc13783_adc_use_touchscreen(pdev)) {
> -		ret = sysfs_create_group(&pdev->dev.kobj, &mc13783_group_ts);
> +		ret = sysfs_create_group(&pdev->dev.kobj,
> +				&mc13783_group_ts);
>  		if (ret)
> -			goto out_err_create2;
> +			goto out_err_create_ts;
>  	}
>  
>  	priv->hwmon_dev = hwmon_device_register(&pdev->dev);
> @@ -184,17 +219,20 @@ static int __init mc13783_adc_probe(struct platform_device *pdev)
>  		goto out_err_register;
>  	}
>  
> -
>  	return 0;
>  
>  out_err_register:
>  
>  	if (!mc13783_adc_use_touchscreen(pdev))
>  		sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_ts);
> -out_err_create2:
> +out_err_create_ts:
>  
> -	sysfs_remove_group(&pdev->dev.kobj, &mc13783_group);
> -out_err_create1:
> +	if (id->driver_data & MC13783_ADC_16CHANS)
> +		sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_16chans);
> +out_err_create_16chans:
> +
> +	sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_base);
> +out_err_create_base:
>  
>  	platform_set_drvdata(pdev, NULL);
>  	kfree(priv);
> @@ -205,13 +243,17 @@ out_err_create1:
>  static int __devexit mc13783_adc_remove(struct platform_device *pdev)
>  {
>  	struct mc13783_adc_priv *priv = platform_get_drvdata(pdev);
> +	kernel_ulong_t driver_data = platform_get_device_id(pdev)->driver_data;
>  
>  	hwmon_device_unregister(priv->hwmon_dev);
>  
>  	if (!mc13783_adc_use_touchscreen(pdev))
>  		sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_ts);
>  
> -	sysfs_remove_group(&pdev->dev.kobj, &mc13783_group);
> +	if (driver_data & MC13783_ADC_16CHANS)
> +		sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_16chans);
> +
> +	sysfs_remove_group(&pdev->dev.kobj, &mc13783_group_base);
>  
>  	platform_set_drvdata(pdev, NULL);
>  	kfree(priv);
> @@ -219,12 +261,26 @@ static int __devexit mc13783_adc_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static const struct platform_device_id mc13783_adc_idtable[] = {
> +	{
> +		.name = "mc13783-adc",
> +		.driver_data = MC13783_ADC_16CHANS,
> +	}, {
> +		.name = "mc13892-adc",
> +		.driver_data = MC13783_ADC_BPDIV2,

I am quite clueless about the meaning of these defines here... are you using
two of the specific differences as a flag to decide which chip we are on?

Sorry if I'm late to chime in...

Best regards,

-- 
David Jander
Protonic Holland.

_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors



[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux