diff mbox

[10/11] hwmon: Add Altera A10-SR power supply alarms

Message ID 1461339219-15255-11-git-send-email-tthayer@opensource.altera.com (mailing list archive)
State Changes Requested
Headers show

Commit Message

tthayer@opensource.altera.com April 22, 2016, 3:33 p.m. UTC
From: Thor Thayer <tthayer@opensource.altera.com>

This patch adds the power supply alarms of the hwmon framework
to the Arria10 System Resource chip.

Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
---
 drivers/hwmon/Kconfig              |    9 ++
 drivers/hwmon/Makefile             |    1 +
 drivers/hwmon/altera-a10sr-hwmon.c |  215 ++++++++++++++++++++++++++++++++++++
 3 files changed, 225 insertions(+)
 create mode 100644 drivers/hwmon/altera-a10sr-hwmon.c

Comments

Guenter Roeck April 22, 2016, 10:24 p.m. UTC | #1
On Fri, Apr 22, 2016 at 10:33:38AM -0500, tthayer@opensource.altera.com wrote:
> From: Thor Thayer <tthayer@opensource.altera.com>
> 
> This patch adds the power supply alarms of the hwmon framework
> to the Arria10 System Resource chip.
> 
> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
> ---

Please read and follow Documentation/hwmon/sysfs-interface 
as well as Documentation/hwmon/submitting-patches. At the very least,
you are expected to document your driver, and you are expected to
use standard attribute names.

>  drivers/hwmon/Kconfig              |    9 ++
>  drivers/hwmon/Makefile             |    1 +
>  drivers/hwmon/altera-a10sr-hwmon.c |  215 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 225 insertions(+)
>  create mode 100644 drivers/hwmon/altera-a10sr-hwmon.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index ff94007..af08846 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -248,6 +248,15 @@ config SENSORS_ADT7475
>  	  This driver can also be build as a module.  If so, the module
>  	  will be called adt7475.
>  
> +config SENSORS_ALTERA_A10SR
> +	bool "Altera Arria10 System Status"
> +	depends on MFD_ALTERA_A10SR
> +	help
> +	  If you say yes here you get support for the power ready status
> +	  for the Arria10's external power supplies on the Arria10 DevKit.
> +	  These values are read over the SPI bus from the Arria10 System
> +	  Resource chip.
> +
>  config SENSORS_ASC7621
>  	tristate "Andigilog aSC7621"
>  	depends on I2C
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 2ef5b7c..17d72a7 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -43,6 +43,7 @@ obj-$(CONFIG_SENSORS_ADT7411)	+= adt7411.o
>  obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
>  obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
>  obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
> +obj-$(CONFIG_SENSORS_ALTERA_A10SR) += altera-a10sr-hwmon.o
>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> diff --git a/drivers/hwmon/altera-a10sr-hwmon.c b/drivers/hwmon/altera-a10sr-hwmon.c
> new file mode 100644
> index 0000000..1eecc6b
> --- /dev/null
> +++ b/drivers/hwmon/altera-a10sr-hwmon.c
> @@ -0,0 +1,215 @@
> +/*
> + *  Copyright Altera Corporation (C) 2014-2016. All Rights Reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.
> + *
> + * HW Monitor driver for  Altera Arria10 MAX5 System Resource Chip
> + * Adapted from DA9052
> + */
> +
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/mfd/altera-a10sr.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

Please list include files in alphabetic order.

> +
> +#define ALTR_A10SR_1V0_BIT_POS          ALTR_A10SR_PG1_1V0_SHIFT
> +#define ALTR_A10SR_0V95_BIT_POS         ALTR_A10SR_PG1_0V95_SHIFT
> +#define ALTR_A10SR_0V9_BIT_POS          ALTR_A10SR_PG1_0V9_SHIFT
> +#define ALTR_A10SR_10V_BIT_POS          ALTR_A10SR_PG1_10V_SHIFT
> +#define ALTR_A10SR_5V0_BIT_POS          ALTR_A10SR_PG1_5V0_SHIFT
> +#define ALTR_A10SR_3V3_BIT_POS          ALTR_A10SR_PG1_3V3_SHIFT
> +#define ALTR_A10SR_2V5_BIT_POS          ALTR_A10SR_PG1_2V5_SHIFT
> +#define ALTR_A10SR_1V8_BIT_POS          ALTR_A10SR_PG1_1V8_SHIFT
> +#define ALTR_A10SR_OP_FLAG_BIT_POS      ALTR_A10SR_PG1_OP_FLAG_SHIFT
> +/* 2nd register needs an offset of 8 to get to 2nd register */
> +#define ALTR_A10SR_FBC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FBC2MP_SHIFT)
> +#define ALTR_A10SR_FAC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FAC2MP_SHIFT)
> +#define ALTR_A10SR_FMCBVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCBVADJ_SHIFT)
> +#define ALTR_A10SR_FMCAVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCAVADJ_SHIFT)
> +#define ALTR_A10SR_HL_VDDQ_BIT_POS      (8 + ALTR_A10SR_PG2_HL_VDDQ_SHIFT)
> +#define ALTR_A10SR_HL_VDD_BIT_POS       (8 + ALTR_A10SR_PG2_HL_VDD_SHIFT)
> +#define ALTR_A10SR_HL_HPS_BIT_POS       (8 + ALTR_A10SR_PG2_HL_HPS_SHIFT)
> +#define ALTR_A10SR_HPS_BIT_POS          (8 + ALTR_A10SR_PG2_HPS_SHIFT)
> +/* 3rd register needs an offset of 16 to get to 3rd register */
> +#define ALTR_A10SR_10V_FAIL_BIT_POS     (16 + ALTR_A10SR_PG3_10V_FAIL_SHIFT)
> +#define ALTR_A10SR_FAM2C_BIT_POS        (16 + ALTR_A10SR_PG3_FAM2C_SHIFT)
> +
> +/**
> + * struct altr_a10sr_hwmon - Altera Max5 HWMON device private data structure
> + * @device: hwmon class.
> + * @regmap: the regmap from the parent device.
> + */
> +struct altr_a10sr_hwmon {
> +	struct device		*class_device;
> +	struct regmap		*regmap;
> +};
> +
> +static ssize_t altr_a10sr_read_status(struct device *dev,
> +				      struct device_attribute *devattr,
> +				      char *buf)
> +{
> +	struct altr_a10sr_hwmon *hwmon = dev_get_drvdata(dev);
> +	int val, ret, index = to_sensor_dev_attr(devattr)->index;
> +	int mask = ALTR_A10SR_REG_BIT_MASK(index);
> +	unsigned char reg = ALTR_A10SR_PWR_GOOD1_REG +
> +			    ALTR_A10SR_REG_OFFSET(index);
> +
> +	ret = regmap_read(hwmon->regmap, reg, &val);

The pointer parameter to regmap_read() is unsigned int. I would suggest
to use the same variable type to avoid surprises.

> +	if (ret < 0)
> +		return ret;
> +
> +	return sprintf(buf, "%d\n", !!(~val & mask));

Are alarms all active low ?

> +}
> +
> +static ssize_t altr_a10sr_hwmon_show_name(struct device *dev,
> +					  struct device_attribute *devattr,
> +					  char *buf)
> +{
> +	return sprintf(buf, "altr_a10sr\n");
> +}
> +
> +/* First Power Good Register Bits */
> +static SENSOR_DEVICE_ATTR(1v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_1V0_BIT_POS);
> +static SENSOR_DEVICE_ATTR(0v95_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_0V95_BIT_POS);
> +static SENSOR_DEVICE_ATTR(0v9_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_0V9_BIT_POS);
> +static SENSOR_DEVICE_ATTR(5v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_5V0_BIT_POS);
> +static SENSOR_DEVICE_ATTR(3v3_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_3V3_BIT_POS);
> +static SENSOR_DEVICE_ATTR(2v5_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_2V5_BIT_POS);
> +static SENSOR_DEVICE_ATTR(1v8_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_1V8_BIT_POS);
> +static SENSOR_DEVICE_ATTR(opflag_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_OP_FLAG_BIT_POS);
> +/* Second Power Good Register Bits */
> +static SENSOR_DEVICE_ATTR(fbc2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_FBC2MP_BIT_POS);
> +static SENSOR_DEVICE_ATTR(fac2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_FAC2MP_BIT_POS);
> +static SENSOR_DEVICE_ATTR(fmcbvadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_FMCBVADJ_BIT_POS);
> +static SENSOR_DEVICE_ATTR(fmcavadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_FMCAVADJ_BIT_POS);
> +static SENSOR_DEVICE_ATTR(hl_vddq_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_HL_VDDQ_BIT_POS);
> +static SENSOR_DEVICE_ATTR(hl_vdd_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_HL_VDD_BIT_POS);
> +static SENSOR_DEVICE_ATTR(hlhps_vdd_alarm, S_IRUGO, altr_a10sr_read_status,
> +			  NULL, ALTR_A10SR_HL_HPS_BIT_POS);
> +static SENSOR_DEVICE_ATTR(hps_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_HPS_BIT_POS);
> +/* Third Power Good Register Bits */
> +static SENSOR_DEVICE_ATTR(10v_alarm, S_IRUGO, altr_a10sr_read_status,
> +			  NULL, ALTR_A10SR_10V_FAIL_BIT_POS);
> +static SENSOR_DEVICE_ATTR(fam2c_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
> +			  ALTR_A10SR_FAM2C_BIT_POS);
> +
> +static DEVICE_ATTR(name, S_IRUGO, altr_a10sr_hwmon_show_name, NULL);
> +
> +static struct attribute *altr_a10sr_attr[] = {
> +	&dev_attr_name.attr,
> +	/* First Power Good Register */
> +	&sensor_dev_attr_opflag_alarm.dev_attr.attr,
> +	&sensor_dev_attr_1v8_alarm.dev_attr.attr,
> +	&sensor_dev_attr_2v5_alarm.dev_attr.attr,
> +	&sensor_dev_attr_1v0_alarm.dev_attr.attr,
> +	&sensor_dev_attr_3v3_alarm.dev_attr.attr,
> +	&sensor_dev_attr_5v0_alarm.dev_attr.attr,
> +	&sensor_dev_attr_0v9_alarm.dev_attr.attr,
> +	&sensor_dev_attr_0v95_alarm.dev_attr.attr,
> +	/* Second Power Good Register */
> +	&sensor_dev_attr_hps_alarm.dev_attr.attr,
> +	&sensor_dev_attr_hlhps_vdd_alarm.dev_attr.attr,
> +	&sensor_dev_attr_hl_vdd_alarm.dev_attr.attr,
> +	&sensor_dev_attr_hl_vddq_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fmcavadj_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fmcbvadj_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fac2mp_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fbc2mp_alarm.dev_attr.attr,
> +	/* Third Power Good Register */
> +	&sensor_dev_attr_10v_alarm.dev_attr.attr,
> +	&sensor_dev_attr_fam2c_alarm.dev_attr.attr,

Ultimately, this doesn't really make much sense, even with standard attribute
names. The ABI assumes that _input attrributes exist. Not sure if that could
be solved by providing 'dummy' unreadable input attributes (permission 0);
that might be worth a try. If you want to attach names to the attributes,
you would need to provide label attributes. Specifically you would need

	inX_input (with permission 0) - not sure if that is even possible
	inX_alarm
	inX_label

> +	NULL
> +};
> +
> +static const struct attribute_group altr_a10sr_attr_group = {
> +	.attrs = altr_a10sr_attr
> +};
> +
> +static int altr_a10sr_hwmon_probe(struct platform_device *pdev)
> +{
> +	struct altr_a10sr_hwmon *hwmon;
> +	int ret;
> +	struct altr_a10sr *a10sr = dev_get_drvdata(pdev->dev.parent);
> +

What if someone creates a stand-alone devicetree property for this driver,
and there is no parent (or parent driver data) ?

> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
> +	if (!hwmon)
> +		return -ENOMEM;
> +
> +	hwmon->regmap = a10sr->regmap;
> +
> +	platform_set_drvdata(pdev, hwmon);
> +
> +	ret = sysfs_create_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
> +	if (ret)
> +		goto err_mem;
> +
> +	hwmon->class_device = hwmon_device_register(&pdev->dev);
> +	if (IS_ERR(hwmon->class_device)) {
> +		ret = PTR_ERR(hwmon->class_device);
> +		goto err_sysfs;
> +	}

Please use devm_hwmon_device_register_with_groups().

> +
> +	return 0;
> +
> +err_sysfs:
> +	sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
> +err_mem:
> +	return ret;
> +}
> +
> +static int altr_a10sr_hwmon_remove(struct platform_device *pdev)
> +{
> +	struct altr_a10sr_hwmon *hwmon = platform_get_drvdata(pdev);
> +
> +	hwmon_device_unregister(hwmon->class_device);
> +	sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id altr_a10sr_hwmon_of_match[] = {
> +	{ .compatible = "altr,a10sr-hwmon" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, altr_a10sr_hwmon_of_match);
> +
> +static struct platform_driver altr_a10sr_hwmon_driver = {
> +	.probe = altr_a10sr_hwmon_probe,
> +	.remove = altr_a10sr_hwmon_remove,
> +	.driver = {
> +		.name = "altr_a10sr_hwmon",
> +		.of_match_table = of_match_ptr(altr_a10sr_hwmon_of_match),
> +	},
> +};
> +
> +module_platform_driver(altr_a10sr_hwmon_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Thor Thayer <tthayer@opensource.altera.com>");
> +MODULE_DESCRIPTION("HW Monitor driver for Altera Arria10 System Resource Chip");
> -- 
> 1.7.9.5
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
tthayer@opensource.altera.com April 25, 2016, 2:41 p.m. UTC | #2
On 04/22/2016 05:24 PM, Guenter Roeck wrote:
> On Fri, Apr 22, 2016 at 10:33:38AM -0500, tthayer@opensource.altera.com wrote:
>> From: Thor Thayer <tthayer@opensource.altera.com>
>>
>> This patch adds the power supply alarms of the hwmon framework
>> to the Arria10 System Resource chip.
>>
>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>> ---
>
> Please read and follow Documentation/hwmon/sysfs-interface
> as well as Documentation/hwmon/submitting-patches. At the very least,
> you are expected to document your driver, and you are expected to
> use standard attribute names.
>

OK. I misunderstood. The standard attribute names would be 
power[1-*]_alarm and the binding documentation the a10sr-hwmon portion 
for MFD doesn't cover this.

>>   drivers/hwmon/Kconfig              |    9 ++
>>   drivers/hwmon/Makefile             |    1 +
>>   drivers/hwmon/altera-a10sr-hwmon.c |  215 ++++++++++++++++++++++++++++++++++++
>>   3 files changed, 225 insertions(+)
>>   create mode 100644 drivers/hwmon/altera-a10sr-hwmon.c
>>
>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>> index ff94007..af08846 100644
>> --- a/drivers/hwmon/Kconfig
>> +++ b/drivers/hwmon/Kconfig
>> @@ -248,6 +248,15 @@ config SENSORS_ADT7475
>>   	  This driver can also be build as a module.  If so, the module
>>   	  will be called adt7475.
>>
>> +config SENSORS_ALTERA_A10SR
>> +	bool "Altera Arria10 System Status"
>> +	depends on MFD_ALTERA_A10SR
>> +	help
>> +	  If you say yes here you get support for the power ready status
>> +	  for the Arria10's external power supplies on the Arria10 DevKit.
>> +	  These values are read over the SPI bus from the Arria10 System
>> +	  Resource chip.
>> +
>>   config SENSORS_ASC7621
>>   	tristate "Andigilog aSC7621"
>>   	depends on I2C
>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>> index 2ef5b7c..17d72a7 100644
>> --- a/drivers/hwmon/Makefile
>> +++ b/drivers/hwmon/Makefile
>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SENSORS_ADT7411)	+= adt7411.o
>>   obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
>>   obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
>>   obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
>> +obj-$(CONFIG_SENSORS_ALTERA_A10SR) += altera-a10sr-hwmon.o
>>   obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>>   obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>>   obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
>> diff --git a/drivers/hwmon/altera-a10sr-hwmon.c b/drivers/hwmon/altera-a10sr-hwmon.c
>> new file mode 100644
>> index 0000000..1eecc6b
>> --- /dev/null
>> +++ b/drivers/hwmon/altera-a10sr-hwmon.c
>> @@ -0,0 +1,215 @@
>> +/*
>> + *  Copyright Altera Corporation (C) 2014-2016. All Rights Reserved
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>> + *
>> + * HW Monitor driver for  Altera Arria10 MAX5 System Resource Chip
>> + * Adapted from DA9052
>> + */
>> +
>> +#include <linux/hwmon.h>
>> +#include <linux/hwmon-sysfs.h>
>> +#include <linux/mfd/altera-a10sr.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>
> Please list include files in alphabetic order.
>

OK. I thought this was in alphabetical order. I'll update this after 
figuring out where I was wrong.

>> +
>> +#define ALTR_A10SR_1V0_BIT_POS          ALTR_A10SR_PG1_1V0_SHIFT
>> +#define ALTR_A10SR_0V95_BIT_POS         ALTR_A10SR_PG1_0V95_SHIFT
>> +#define ALTR_A10SR_0V9_BIT_POS          ALTR_A10SR_PG1_0V9_SHIFT
>> +#define ALTR_A10SR_10V_BIT_POS          ALTR_A10SR_PG1_10V_SHIFT
>> +#define ALTR_A10SR_5V0_BIT_POS          ALTR_A10SR_PG1_5V0_SHIFT
>> +#define ALTR_A10SR_3V3_BIT_POS          ALTR_A10SR_PG1_3V3_SHIFT
>> +#define ALTR_A10SR_2V5_BIT_POS          ALTR_A10SR_PG1_2V5_SHIFT
>> +#define ALTR_A10SR_1V8_BIT_POS          ALTR_A10SR_PG1_1V8_SHIFT
>> +#define ALTR_A10SR_OP_FLAG_BIT_POS      ALTR_A10SR_PG1_OP_FLAG_SHIFT
>> +/* 2nd register needs an offset of 8 to get to 2nd register */
>> +#define ALTR_A10SR_FBC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FBC2MP_SHIFT)
>> +#define ALTR_A10SR_FAC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FAC2MP_SHIFT)
>> +#define ALTR_A10SR_FMCBVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCBVADJ_SHIFT)
>> +#define ALTR_A10SR_FMCAVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCAVADJ_SHIFT)
>> +#define ALTR_A10SR_HL_VDDQ_BIT_POS      (8 + ALTR_A10SR_PG2_HL_VDDQ_SHIFT)
>> +#define ALTR_A10SR_HL_VDD_BIT_POS       (8 + ALTR_A10SR_PG2_HL_VDD_SHIFT)
>> +#define ALTR_A10SR_HL_HPS_BIT_POS       (8 + ALTR_A10SR_PG2_HL_HPS_SHIFT)
>> +#define ALTR_A10SR_HPS_BIT_POS          (8 + ALTR_A10SR_PG2_HPS_SHIFT)
>> +/* 3rd register needs an offset of 16 to get to 3rd register */
>> +#define ALTR_A10SR_10V_FAIL_BIT_POS     (16 + ALTR_A10SR_PG3_10V_FAIL_SHIFT)
>> +#define ALTR_A10SR_FAM2C_BIT_POS        (16 + ALTR_A10SR_PG3_FAM2C_SHIFT)
>> +
>> +/**
>> + * struct altr_a10sr_hwmon - Altera Max5 HWMON device private data structure
>> + * @device: hwmon class.
>> + * @regmap: the regmap from the parent device.
>> + */
>> +struct altr_a10sr_hwmon {
>> +	struct device		*class_device;
>> +	struct regmap		*regmap;
>> +};
>> +
>> +static ssize_t altr_a10sr_read_status(struct device *dev,
>> +				      struct device_attribute *devattr,
>> +				      char *buf)
>> +{
>> +	struct altr_a10sr_hwmon *hwmon = dev_get_drvdata(dev);
>> +	int val, ret, index = to_sensor_dev_attr(devattr)->index;
>> +	int mask = ALTR_A10SR_REG_BIT_MASK(index);
>> +	unsigned char reg = ALTR_A10SR_PWR_GOOD1_REG +
>> +			    ALTR_A10SR_REG_OFFSET(index);
>> +
>> +	ret = regmap_read(hwmon->regmap, reg, &val);
>
> The pointer parameter to regmap_read() is unsigned int. I would suggest
> to use the same variable type to avoid surprises.
>
Understood. Thanks.

>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return sprintf(buf, "%d\n", !!(~val & mask));
>
> Are alarms all active low ?
>
Yes, They are actually Power Good signals (Good = high) so I'm inverting.

>> +}
>> +
>> +static ssize_t altr_a10sr_hwmon_show_name(struct device *dev,
>> +					  struct device_attribute *devattr,
>> +					  char *buf)
>> +{
>> +	return sprintf(buf, "altr_a10sr\n");
>> +}
>> +
>> +/* First Power Good Register Bits */
>> +static SENSOR_DEVICE_ATTR(1v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_1V0_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(0v95_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_0V95_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(0v9_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_0V9_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(5v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_5V0_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(3v3_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_3V3_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(2v5_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_2V5_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(1v8_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_1V8_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(opflag_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_OP_FLAG_BIT_POS);
>> +/* Second Power Good Register Bits */
>> +static SENSOR_DEVICE_ATTR(fbc2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_FBC2MP_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(fac2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_FAC2MP_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(fmcbvadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_FMCBVADJ_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(fmcavadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_FMCAVADJ_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(hl_vddq_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_HL_VDDQ_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(hl_vdd_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_HL_VDD_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(hlhps_vdd_alarm, S_IRUGO, altr_a10sr_read_status,
>> +			  NULL, ALTR_A10SR_HL_HPS_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(hps_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_HPS_BIT_POS);
>> +/* Third Power Good Register Bits */
>> +static SENSOR_DEVICE_ATTR(10v_alarm, S_IRUGO, altr_a10sr_read_status,
>> +			  NULL, ALTR_A10SR_10V_FAIL_BIT_POS);
>> +static SENSOR_DEVICE_ATTR(fam2c_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>> +			  ALTR_A10SR_FAM2C_BIT_POS);
>> +
>> +static DEVICE_ATTR(name, S_IRUGO, altr_a10sr_hwmon_show_name, NULL);
>> +
>> +static struct attribute *altr_a10sr_attr[] = {
>> +	&dev_attr_name.attr,
>> +	/* First Power Good Register */
>> +	&sensor_dev_attr_opflag_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_1v8_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_2v5_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_1v0_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_3v3_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_5v0_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_0v9_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_0v95_alarm.dev_attr.attr,
>> +	/* Second Power Good Register */
>> +	&sensor_dev_attr_hps_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_hlhps_vdd_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_hl_vdd_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_hl_vddq_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fmcavadj_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fmcbvadj_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fac2mp_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fbc2mp_alarm.dev_attr.attr,
>> +	/* Third Power Good Register */
>> +	&sensor_dev_attr_10v_alarm.dev_attr.attr,
>> +	&sensor_dev_attr_fam2c_alarm.dev_attr.attr,
>
> Ultimately, this doesn't really make much sense, even with standard attribute
> names. The ABI assumes that _input attrributes exist. Not sure if that could
> be solved by providing 'dummy' unreadable input attributes (permission 0);
> that might be worth a try. If you want to attach names to the attributes,
> you would need to provide label attributes. Specifically you would need
>
> 	inX_input (with permission 0) - not sure if that is even possible
> 	inX_alarm
> 	inX_label
>
Hmm. OK. I may need to rethink this.

>> +	NULL
>> +};
>> +
>> +static const struct attribute_group altr_a10sr_attr_group = {
>> +	.attrs = altr_a10sr_attr
>> +};
>> +
>> +static int altr_a10sr_hwmon_probe(struct platform_device *pdev)
>> +{
>> +	struct altr_a10sr_hwmon *hwmon;
>> +	int ret;
>> +	struct altr_a10sr *a10sr = dev_get_drvdata(pdev->dev.parent);
>> +
>
> What if someone creates a stand-alone devicetree property for this driver,
> and there is no parent (or parent driver data) ?
>
I agree if this was a commercially available part. However, this is a 
specific driver for a programmable logic device (PLD) that will always 
be part of an MFD parent. The PLD only supports the Altera Development Kit.

>> +	hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
>> +	if (!hwmon)
>> +		return -ENOMEM;
>> +
>> +	hwmon->regmap = a10sr->regmap;
>> +
>> +	platform_set_drvdata(pdev, hwmon);
>> +
>> +	ret = sysfs_create_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>> +	if (ret)
>> +		goto err_mem;
>> +
>> +	hwmon->class_device = hwmon_device_register(&pdev->dev);
>> +	if (IS_ERR(hwmon->class_device)) {
>> +		ret = PTR_ERR(hwmon->class_device);
>> +		goto err_sysfs;
>> +	}
>
> Please use devm_hwmon_device_register_with_groups().
>
Understood. Thanks.

Thanks for reviewing.

>> +
>> +	return 0;
>> +
>> +err_sysfs:
>> +	sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>> +err_mem:
>> +	return ret;
>> +}
>> +
>> +static int altr_a10sr_hwmon_remove(struct platform_device *pdev)
>> +{
>> +	struct altr_a10sr_hwmon *hwmon = platform_get_drvdata(pdev);
>> +
>> +	hwmon_device_unregister(hwmon->class_device);
>> +	sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id altr_a10sr_hwmon_of_match[] = {
>> +	{ .compatible = "altr,a10sr-hwmon" },
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(of, altr_a10sr_hwmon_of_match);
>> +
>> +static struct platform_driver altr_a10sr_hwmon_driver = {
>> +	.probe = altr_a10sr_hwmon_probe,
>> +	.remove = altr_a10sr_hwmon_remove,
>> +	.driver = {
>> +		.name = "altr_a10sr_hwmon",
>> +		.of_match_table = of_match_ptr(altr_a10sr_hwmon_of_match),
>> +	},
>> +};
>> +
>> +module_platform_driver(altr_a10sr_hwmon_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR("Thor Thayer <tthayer@opensource.altera.com>");
>> +MODULE_DESCRIPTION("HW Monitor driver for Altera Arria10 System Resource Chip");
>> --
>> 1.7.9.5
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck April 25, 2016, 2:58 p.m. UTC | #3
On 04/25/2016 07:41 AM, Thor Thayer wrote:
>
>
> On 04/22/2016 05:24 PM, Guenter Roeck wrote:
>> On Fri, Apr 22, 2016 at 10:33:38AM -0500, tthayer@opensource.altera.com wrote:
>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>
>>> This patch adds the power supply alarms of the hwmon framework
>>> to the Arria10 System Resource chip.
>>>
>>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>>> ---
>>
>> Please read and follow Documentation/hwmon/sysfs-interface
>> as well as Documentation/hwmon/submitting-patches. At the very least,
>> you are expected to document your driver, and you are expected to
>> use standard attribute names.
>>
>
> OK. I misunderstood. The standard attribute names would be power[1-*]_alarm and the binding documentation the a10sr-hwmon portion for MFD doesn't cover this.
>

power ? Why power ? The names suggest those are voltages.
Also not sure why those names would be in the bindings.

>>>   drivers/hwmon/Kconfig              |    9 ++
>>>   drivers/hwmon/Makefile             |    1 +
>>>   drivers/hwmon/altera-a10sr-hwmon.c |  215 ++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 225 insertions(+)
>>>   create mode 100644 drivers/hwmon/altera-a10sr-hwmon.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index ff94007..af08846 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -248,6 +248,15 @@ config SENSORS_ADT7475
>>>         This driver can also be build as a module.  If so, the module
>>>         will be called adt7475.
>>>
>>> +config SENSORS_ALTERA_A10SR
>>> +    bool "Altera Arria10 System Status"
>>> +    depends on MFD_ALTERA_A10SR
>>> +    help
>>> +      If you say yes here you get support for the power ready status
>>> +      for the Arria10's external power supplies on the Arria10 DevKit.
>>> +      These values are read over the SPI bus from the Arria10 System
>>> +      Resource chip.
>>> +
>>>   config SENSORS_ASC7621
>>>       tristate "Andigilog aSC7621"
>>>       depends on I2C
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index 2ef5b7c..17d72a7 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SENSORS_ADT7411)    += adt7411.o
>>>   obj-$(CONFIG_SENSORS_ADT7462)    += adt7462.o
>>>   obj-$(CONFIG_SENSORS_ADT7470)    += adt7470.o
>>>   obj-$(CONFIG_SENSORS_ADT7475)    += adt7475.o
>>> +obj-$(CONFIG_SENSORS_ALTERA_A10SR) += altera-a10sr-hwmon.o
>>>   obj-$(CONFIG_SENSORS_APPLESMC)    += applesmc.o
>>>   obj-$(CONFIG_SENSORS_ARM_SCPI)    += scpi-hwmon.o
>>>   obj-$(CONFIG_SENSORS_ASC7621)    += asc7621.o
>>> diff --git a/drivers/hwmon/altera-a10sr-hwmon.c b/drivers/hwmon/altera-a10sr-hwmon.c
>>> new file mode 100644
>>> index 0000000..1eecc6b
>>> --- /dev/null
>>> +++ b/drivers/hwmon/altera-a10sr-hwmon.c
>>> @@ -0,0 +1,215 @@
>>> +/*
>>> + *  Copyright Altera Corporation (C) 2014-2016. All Rights Reserved
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify it
>>> + * under the terms and conditions of the GNU General Public License,
>>> + * version 2, as published by the Free Software Foundation.
>>> + *
>>> + * This program is distributed in the hope it will be useful, but WITHOUT
>>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>>> + * more details.
>>> + *
>>> + * You should have received a copy of the GNU General Public License along with
>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>> + *
>>> + * HW Monitor driver for  Altera Arria10 MAX5 System Resource Chip
>>> + * Adapted from DA9052
>>> + */
>>> +
>>> +#include <linux/hwmon.h>
>>> +#include <linux/hwmon-sysfs.h>
>>> +#include <linux/mfd/altera-a10sr.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>
>> Please list include files in alphabetic order.
>>
>
> OK. I thought this was in alphabetical order. I'll update this after figuring out where I was wrong.
>

Sorry, PBKAC on my side.

Guenter

>>> +
>>> +#define ALTR_A10SR_1V0_BIT_POS          ALTR_A10SR_PG1_1V0_SHIFT
>>> +#define ALTR_A10SR_0V95_BIT_POS         ALTR_A10SR_PG1_0V95_SHIFT
>>> +#define ALTR_A10SR_0V9_BIT_POS          ALTR_A10SR_PG1_0V9_SHIFT
>>> +#define ALTR_A10SR_10V_BIT_POS          ALTR_A10SR_PG1_10V_SHIFT
>>> +#define ALTR_A10SR_5V0_BIT_POS          ALTR_A10SR_PG1_5V0_SHIFT
>>> +#define ALTR_A10SR_3V3_BIT_POS          ALTR_A10SR_PG1_3V3_SHIFT
>>> +#define ALTR_A10SR_2V5_BIT_POS          ALTR_A10SR_PG1_2V5_SHIFT
>>> +#define ALTR_A10SR_1V8_BIT_POS          ALTR_A10SR_PG1_1V8_SHIFT
>>> +#define ALTR_A10SR_OP_FLAG_BIT_POS      ALTR_A10SR_PG1_OP_FLAG_SHIFT
>>> +/* 2nd register needs an offset of 8 to get to 2nd register */
>>> +#define ALTR_A10SR_FBC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FBC2MP_SHIFT)
>>> +#define ALTR_A10SR_FAC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FAC2MP_SHIFT)
>>> +#define ALTR_A10SR_FMCBVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCBVADJ_SHIFT)
>>> +#define ALTR_A10SR_FMCAVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCAVADJ_SHIFT)
>>> +#define ALTR_A10SR_HL_VDDQ_BIT_POS      (8 + ALTR_A10SR_PG2_HL_VDDQ_SHIFT)
>>> +#define ALTR_A10SR_HL_VDD_BIT_POS       (8 + ALTR_A10SR_PG2_HL_VDD_SHIFT)
>>> +#define ALTR_A10SR_HL_HPS_BIT_POS       (8 + ALTR_A10SR_PG2_HL_HPS_SHIFT)
>>> +#define ALTR_A10SR_HPS_BIT_POS          (8 + ALTR_A10SR_PG2_HPS_SHIFT)
>>> +/* 3rd register needs an offset of 16 to get to 3rd register */
>>> +#define ALTR_A10SR_10V_FAIL_BIT_POS     (16 + ALTR_A10SR_PG3_10V_FAIL_SHIFT)
>>> +#define ALTR_A10SR_FAM2C_BIT_POS        (16 + ALTR_A10SR_PG3_FAM2C_SHIFT)
>>> +
>>> +/**
>>> + * struct altr_a10sr_hwmon - Altera Max5 HWMON device private data structure
>>> + * @device: hwmon class.
>>> + * @regmap: the regmap from the parent device.
>>> + */
>>> +struct altr_a10sr_hwmon {
>>> +    struct device        *class_device;
>>> +    struct regmap        *regmap;
>>> +};
>>> +
>>> +static ssize_t altr_a10sr_read_status(struct device *dev,
>>> +                      struct device_attribute *devattr,
>>> +                      char *buf)
>>> +{
>>> +    struct altr_a10sr_hwmon *hwmon = dev_get_drvdata(dev);
>>> +    int val, ret, index = to_sensor_dev_attr(devattr)->index;
>>> +    int mask = ALTR_A10SR_REG_BIT_MASK(index);
>>> +    unsigned char reg = ALTR_A10SR_PWR_GOOD1_REG +
>>> +                ALTR_A10SR_REG_OFFSET(index);
>>> +
>>> +    ret = regmap_read(hwmon->regmap, reg, &val);
>>
>> The pointer parameter to regmap_read() is unsigned int. I would suggest
>> to use the same variable type to avoid surprises.
>>
> Understood. Thanks.
>
>>> +    if (ret < 0)
>>> +        return ret;
>>> +
>>> +    return sprintf(buf, "%d\n", !!(~val & mask));
>>
>> Are alarms all active low ?
>>
> Yes, They are actually Power Good signals (Good = high) so I'm inverting.
>
>>> +}
>>> +
>>> +static ssize_t altr_a10sr_hwmon_show_name(struct device *dev,
>>> +                      struct device_attribute *devattr,
>>> +                      char *buf)
>>> +{
>>> +    return sprintf(buf, "altr_a10sr\n");
>>> +}
>>> +
>>> +/* First Power Good Register Bits */
>>> +static SENSOR_DEVICE_ATTR(1v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_1V0_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(0v95_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_0V95_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(0v9_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_0V9_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(5v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_5V0_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(3v3_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_3V3_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(2v5_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_2V5_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(1v8_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_1V8_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(opflag_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_OP_FLAG_BIT_POS);
>>> +/* Second Power Good Register Bits */
>>> +static SENSOR_DEVICE_ATTR(fbc2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_FBC2MP_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(fac2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_FAC2MP_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(fmcbvadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_FMCBVADJ_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(fmcavadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_FMCAVADJ_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(hl_vddq_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_HL_VDDQ_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(hl_vdd_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_HL_VDD_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(hlhps_vdd_alarm, S_IRUGO, altr_a10sr_read_status,
>>> +              NULL, ALTR_A10SR_HL_HPS_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(hps_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_HPS_BIT_POS);
>>> +/* Third Power Good Register Bits */
>>> +static SENSOR_DEVICE_ATTR(10v_alarm, S_IRUGO, altr_a10sr_read_status,
>>> +              NULL, ALTR_A10SR_10V_FAIL_BIT_POS);
>>> +static SENSOR_DEVICE_ATTR(fam2c_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
>>> +              ALTR_A10SR_FAM2C_BIT_POS);
>>> +
>>> +static DEVICE_ATTR(name, S_IRUGO, altr_a10sr_hwmon_show_name, NULL);
>>> +
>>> +static struct attribute *altr_a10sr_attr[] = {
>>> +    &dev_attr_name.attr,
>>> +    /* First Power Good Register */
>>> +    &sensor_dev_attr_opflag_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_1v8_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_2v5_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_1v0_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_3v3_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_5v0_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_0v9_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_0v95_alarm.dev_attr.attr,
>>> +    /* Second Power Good Register */
>>> +    &sensor_dev_attr_hps_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_hlhps_vdd_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_hl_vdd_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_hl_vddq_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_fmcavadj_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_fmcbvadj_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_fac2mp_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_fbc2mp_alarm.dev_attr.attr,
>>> +    /* Third Power Good Register */
>>> +    &sensor_dev_attr_10v_alarm.dev_attr.attr,
>>> +    &sensor_dev_attr_fam2c_alarm.dev_attr.attr,
>>
>> Ultimately, this doesn't really make much sense, even with standard attribute
>> names. The ABI assumes that _input attrributes exist. Not sure if that could
>> be solved by providing 'dummy' unreadable input attributes (permission 0);
>> that might be worth a try. If you want to attach names to the attributes,
>> you would need to provide label attributes. Specifically you would need
>>
>>     inX_input (with permission 0) - not sure if that is even possible
>>     inX_alarm
>>     inX_label
>>
> Hmm. OK. I may need to rethink this.
>
>>> +    NULL
>>> +};
>>> +
>>> +static const struct attribute_group altr_a10sr_attr_group = {
>>> +    .attrs = altr_a10sr_attr
>>> +};
>>> +
>>> +static int altr_a10sr_hwmon_probe(struct platform_device *pdev)
>>> +{
>>> +    struct altr_a10sr_hwmon *hwmon;
>>> +    int ret;
>>> +    struct altr_a10sr *a10sr = dev_get_drvdata(pdev->dev.parent);
>>> +
>>
>> What if someone creates a stand-alone devicetree property for this driver,
>> and there is no parent (or parent driver data) ?
>>
> I agree if this was a commercially available part. However, this is a specific driver for a programmable logic device (PLD) that will always be part of an MFD parent. The PLD only supports the Altera Development Kit.
>
>>> +    hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
>>> +    if (!hwmon)
>>> +        return -ENOMEM;
>>> +
>>> +    hwmon->regmap = a10sr->regmap;
>>> +
>>> +    platform_set_drvdata(pdev, hwmon);
>>> +
>>> +    ret = sysfs_create_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>>> +    if (ret)
>>> +        goto err_mem;
>>> +
>>> +    hwmon->class_device = hwmon_device_register(&pdev->dev);
>>> +    if (IS_ERR(hwmon->class_device)) {
>>> +        ret = PTR_ERR(hwmon->class_device);
>>> +        goto err_sysfs;
>>> +    }
>>
>> Please use devm_hwmon_device_register_with_groups().
>>
> Understood. Thanks.
>
> Thanks for reviewing.
>
>>> +
>>> +    return 0;
>>> +
>>> +err_sysfs:
>>> +    sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>>> +err_mem:
>>> +    return ret;
>>> +}
>>> +
>>> +static int altr_a10sr_hwmon_remove(struct platform_device *pdev)
>>> +{
>>> +    struct altr_a10sr_hwmon *hwmon = platform_get_drvdata(pdev);
>>> +
>>> +    hwmon_device_unregister(hwmon->class_device);
>>> +    sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id altr_a10sr_hwmon_of_match[] = {
>>> +    { .compatible = "altr,a10sr-hwmon" },
>>> +    { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, altr_a10sr_hwmon_of_match);
>>> +
>>> +static struct platform_driver altr_a10sr_hwmon_driver = {
>>> +    .probe = altr_a10sr_hwmon_probe,
>>> +    .remove = altr_a10sr_hwmon_remove,
>>> +    .driver = {
>>> +        .name = "altr_a10sr_hwmon",
>>> +        .of_match_table = of_match_ptr(altr_a10sr_hwmon_of_match),
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(altr_a10sr_hwmon_driver);
>>> +
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_AUTHOR("Thor Thayer <tthayer@opensource.altera.com>");
>>> +MODULE_DESCRIPTION("HW Monitor driver for Altera Arria10 System Resource Chip");
>>> --
>>> 1.7.9.5
>>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
tthayer@opensource.altera.com April 25, 2016, 3:18 p.m. UTC | #4
On 04/25/2016 09:58 AM, Guenter Roeck wrote:
> On 04/25/2016 07:41 AM, Thor Thayer wrote:
>>
>>
>> On 04/22/2016 05:24 PM, Guenter Roeck wrote:
>>> On Fri, Apr 22, 2016 at 10:33:38AM -0500,
>>> tthayer@opensource.altera.com wrote:
>>>> From: Thor Thayer <tthayer@opensource.altera.com>
>>>>
>>>> This patch adds the power supply alarms of the hwmon framework
>>>> to the Arria10 System Resource chip.
>>>>
>>>> Signed-off-by: Thor Thayer <tthayer@opensource.altera.com>
>>>> ---
>>>
>>> Please read and follow Documentation/hwmon/sysfs-interface
>>> as well as Documentation/hwmon/submitting-patches. At the very least,
>>> you are expected to document your driver, and you are expected to
>>> use standard attribute names.
>>>
>>
>> OK. I misunderstood. The standard attribute names would be
>> power[1-*]_alarm and the binding documentation the a10sr-hwmon portion
>> for MFD doesn't cover this.
>>
>
> power ? Why power ? The names suggest those are voltages.
> Also not sure why those names would be in the bindings.
>
Yes, you are correct. I should be using in[0-*]_lcrit_alarm.

I didn't phrase my reply clearly. It was my initial understanding that 
the binding documentation for the MFD would cover the documentation for 
the driver. I missed that I needed to add the documentation into 
Documentation/hwmon/altera-a10sr-hwmon.c but I will fix this oversight.

Thanks.

>>>>   drivers/hwmon/Kconfig              |    9 ++
>>>>   drivers/hwmon/Makefile             |    1 +
>>>>   drivers/hwmon/altera-a10sr-hwmon.c |  215
>>>> ++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 225 insertions(+)
>>>>   create mode 100644 drivers/hwmon/altera-a10sr-hwmon.c
>>>>
>>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>>> index ff94007..af08846 100644
>>>> --- a/drivers/hwmon/Kconfig
>>>> +++ b/drivers/hwmon/Kconfig
>>>> @@ -248,6 +248,15 @@ config SENSORS_ADT7475
>>>>         This driver can also be build as a module.  If so, the module
>>>>         will be called adt7475.
>>>>
>>>> +config SENSORS_ALTERA_A10SR
>>>> +    bool "Altera Arria10 System Status"
>>>> +    depends on MFD_ALTERA_A10SR
>>>> +    help
>>>> +      If you say yes here you get support for the power ready status
>>>> +      for the Arria10's external power supplies on the Arria10 DevKit.
>>>> +      These values are read over the SPI bus from the Arria10 System
>>>> +      Resource chip.
>>>> +
>>>>   config SENSORS_ASC7621
>>>>       tristate "Andigilog aSC7621"
>>>>       depends on I2C
>>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>>> index 2ef5b7c..17d72a7 100644
>>>> --- a/drivers/hwmon/Makefile
>>>> +++ b/drivers/hwmon/Makefile
>>>> @@ -43,6 +43,7 @@ obj-$(CONFIG_SENSORS_ADT7411)    += adt7411.o
>>>>   obj-$(CONFIG_SENSORS_ADT7462)    += adt7462.o
>>>>   obj-$(CONFIG_SENSORS_ADT7470)    += adt7470.o
>>>>   obj-$(CONFIG_SENSORS_ADT7475)    += adt7475.o
>>>> +obj-$(CONFIG_SENSORS_ALTERA_A10SR) += altera-a10sr-hwmon.o
>>>>   obj-$(CONFIG_SENSORS_APPLESMC)    += applesmc.o
>>>>   obj-$(CONFIG_SENSORS_ARM_SCPI)    += scpi-hwmon.o
>>>>   obj-$(CONFIG_SENSORS_ASC7621)    += asc7621.o
>>>> diff --git a/drivers/hwmon/altera-a10sr-hwmon.c
>>>> b/drivers/hwmon/altera-a10sr-hwmon.c
>>>> new file mode 100644
>>>> index 0000000..1eecc6b
>>>> --- /dev/null
>>>> +++ b/drivers/hwmon/altera-a10sr-hwmon.c
>>>> @@ -0,0 +1,215 @@
>>>> +/*
>>>> + *  Copyright Altera Corporation (C) 2014-2016. All Rights Reserved
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or
>>>> modify it
>>>> + * under the terms and conditions of the GNU General Public License,
>>>> + * version 2, as published by the Free Software Foundation.
>>>> + *
>>>> + * This program is distributed in the hope it will be useful, but
>>>> WITHOUT
>>>> + * ANY WARRANTY; without even the implied warranty of
>>>> MERCHANTABILITY or
>>>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public
>>>> License for
>>>> + * more details.
>>>> + *
>>>> + * You should have received a copy of the GNU General Public
>>>> License along with
>>>> + * this program.  If not, see <http://www.gnu.org/licenses/>.
>>>> + *
>>>> + * HW Monitor driver for  Altera Arria10 MAX5 System Resource Chip
>>>> + * Adapted from DA9052
>>>> + */
>>>> +
>>>> +#include <linux/hwmon.h>
>>>> +#include <linux/hwmon-sysfs.h>
>>>> +#include <linux/mfd/altera-a10sr.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>
>>> Please list include files in alphabetic order.
>>>
>>
>> OK. I thought this was in alphabetical order. I'll update this after
>> figuring out where I was wrong.
>>
>
> Sorry, PBKAC on my side.
>
> Guenter
>
>>>> +
>>>> +#define ALTR_A10SR_1V0_BIT_POS          ALTR_A10SR_PG1_1V0_SHIFT
>>>> +#define ALTR_A10SR_0V95_BIT_POS         ALTR_A10SR_PG1_0V95_SHIFT
>>>> +#define ALTR_A10SR_0V9_BIT_POS          ALTR_A10SR_PG1_0V9_SHIFT
>>>> +#define ALTR_A10SR_10V_BIT_POS          ALTR_A10SR_PG1_10V_SHIFT
>>>> +#define ALTR_A10SR_5V0_BIT_POS          ALTR_A10SR_PG1_5V0_SHIFT
>>>> +#define ALTR_A10SR_3V3_BIT_POS          ALTR_A10SR_PG1_3V3_SHIFT
>>>> +#define ALTR_A10SR_2V5_BIT_POS          ALTR_A10SR_PG1_2V5_SHIFT
>>>> +#define ALTR_A10SR_1V8_BIT_POS          ALTR_A10SR_PG1_1V8_SHIFT
>>>> +#define ALTR_A10SR_OP_FLAG_BIT_POS      ALTR_A10SR_PG1_OP_FLAG_SHIFT
>>>> +/* 2nd register needs an offset of 8 to get to 2nd register */
>>>> +#define ALTR_A10SR_FBC2MP_BIT_POS       (8 +
>>>> ALTR_A10SR_PG2_FBC2MP_SHIFT)
>>>> +#define ALTR_A10SR_FAC2MP_BIT_POS       (8 +
>>>> ALTR_A10SR_PG2_FAC2MP_SHIFT)
>>>> +#define ALTR_A10SR_FMCBVADJ_BIT_POS     (8 +
>>>> ALTR_A10SR_PG2_FMCBVADJ_SHIFT)
>>>> +#define ALTR_A10SR_FMCAVADJ_BIT_POS     (8 +
>>>> ALTR_A10SR_PG2_FMCAVADJ_SHIFT)
>>>> +#define ALTR_A10SR_HL_VDDQ_BIT_POS      (8 +
>>>> ALTR_A10SR_PG2_HL_VDDQ_SHIFT)
>>>> +#define ALTR_A10SR_HL_VDD_BIT_POS       (8 +
>>>> ALTR_A10SR_PG2_HL_VDD_SHIFT)
>>>> +#define ALTR_A10SR_HL_HPS_BIT_POS       (8 +
>>>> ALTR_A10SR_PG2_HL_HPS_SHIFT)
>>>> +#define ALTR_A10SR_HPS_BIT_POS          (8 + ALTR_A10SR_PG2_HPS_SHIFT)
>>>> +/* 3rd register needs an offset of 16 to get to 3rd register */
>>>> +#define ALTR_A10SR_10V_FAIL_BIT_POS     (16 +
>>>> ALTR_A10SR_PG3_10V_FAIL_SHIFT)
>>>> +#define ALTR_A10SR_FAM2C_BIT_POS        (16 +
>>>> ALTR_A10SR_PG3_FAM2C_SHIFT)
>>>> +
>>>> +/**
>>>> + * struct altr_a10sr_hwmon - Altera Max5 HWMON device private data
>>>> structure
>>>> + * @device: hwmon class.
>>>> + * @regmap: the regmap from the parent device.
>>>> + */
>>>> +struct altr_a10sr_hwmon {
>>>> +    struct device        *class_device;
>>>> +    struct regmap        *regmap;
>>>> +};
>>>> +
>>>> +static ssize_t altr_a10sr_read_status(struct device *dev,
>>>> +                      struct device_attribute *devattr,
>>>> +                      char *buf)
>>>> +{
>>>> +    struct altr_a10sr_hwmon *hwmon = dev_get_drvdata(dev);
>>>> +    int val, ret, index = to_sensor_dev_attr(devattr)->index;
>>>> +    int mask = ALTR_A10SR_REG_BIT_MASK(index);
>>>> +    unsigned char reg = ALTR_A10SR_PWR_GOOD1_REG +
>>>> +                ALTR_A10SR_REG_OFFSET(index);
>>>> +
>>>> +    ret = regmap_read(hwmon->regmap, reg, &val);
>>>
>>> The pointer parameter to regmap_read() is unsigned int. I would suggest
>>> to use the same variable type to avoid surprises.
>>>
>> Understood. Thanks.
>>
>>>> +    if (ret < 0)
>>>> +        return ret;
>>>> +
>>>> +    return sprintf(buf, "%d\n", !!(~val & mask));
>>>
>>> Are alarms all active low ?
>>>
>> Yes, They are actually Power Good signals (Good = high) so I'm inverting.
>>
>>>> +}
>>>> +
>>>> +static ssize_t altr_a10sr_hwmon_show_name(struct device *dev,
>>>> +                      struct device_attribute *devattr,
>>>> +                      char *buf)
>>>> +{
>>>> +    return sprintf(buf, "altr_a10sr\n");
>>>> +}
>>>> +
>>>> +/* First Power Good Register Bits */
>>>> +static SENSOR_DEVICE_ATTR(1v0_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_1V0_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(0v95_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_0V95_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(0v9_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_0V9_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(5v0_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_5V0_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(3v3_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_3V3_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(2v5_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_2V5_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(1v8_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_1V8_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(opflag_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_OP_FLAG_BIT_POS);
>>>> +/* Second Power Good Register Bits */
>>>> +static SENSOR_DEVICE_ATTR(fbc2mp_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_FBC2MP_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(fac2mp_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_FAC2MP_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(fmcbvadj_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_FMCBVADJ_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(fmcavadj_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_FMCAVADJ_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(hl_vddq_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_HL_VDDQ_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(hl_vdd_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_HL_VDD_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(hlhps_vdd_alarm, S_IRUGO,
>>>> altr_a10sr_read_status,
>>>> +              NULL, ALTR_A10SR_HL_HPS_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(hps_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_HPS_BIT_POS);
>>>> +/* Third Power Good Register Bits */
>>>> +static SENSOR_DEVICE_ATTR(10v_alarm, S_IRUGO, altr_a10sr_read_status,
>>>> +              NULL, ALTR_A10SR_10V_FAIL_BIT_POS);
>>>> +static SENSOR_DEVICE_ATTR(fam2c_alarm, S_IRUGO,
>>>> altr_a10sr_read_status, NULL,
>>>> +              ALTR_A10SR_FAM2C_BIT_POS);
>>>> +
>>>> +static DEVICE_ATTR(name, S_IRUGO, altr_a10sr_hwmon_show_name, NULL);
>>>> +
>>>> +static struct attribute *altr_a10sr_attr[] = {
>>>> +    &dev_attr_name.attr,
>>>> +    /* First Power Good Register */
>>>> +    &sensor_dev_attr_opflag_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_1v8_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_2v5_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_1v0_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_3v3_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_5v0_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_0v9_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_0v95_alarm.dev_attr.attr,
>>>> +    /* Second Power Good Register */
>>>> +    &sensor_dev_attr_hps_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_hlhps_vdd_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_hl_vdd_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_hl_vddq_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_fmcavadj_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_fmcbvadj_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_fac2mp_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_fbc2mp_alarm.dev_attr.attr,
>>>> +    /* Third Power Good Register */
>>>> +    &sensor_dev_attr_10v_alarm.dev_attr.attr,
>>>> +    &sensor_dev_attr_fam2c_alarm.dev_attr.attr,
>>>
>>> Ultimately, this doesn't really make much sense, even with standard
>>> attribute
>>> names. The ABI assumes that _input attrributes exist. Not sure if
>>> that could
>>> be solved by providing 'dummy' unreadable input attributes
>>> (permission 0);
>>> that might be worth a try. If you want to attach names to the
>>> attributes,
>>> you would need to provide label attributes. Specifically you would need
>>>
>>>     inX_input (with permission 0) - not sure if that is even possible
>>>     inX_alarm
>>>     inX_label
>>>
>> Hmm. OK. I may need to rethink this.
>>
>>>> +    NULL
>>>> +};
>>>> +
>>>> +static const struct attribute_group altr_a10sr_attr_group = {
>>>> +    .attrs = altr_a10sr_attr
>>>> +};
>>>> +
>>>> +static int altr_a10sr_hwmon_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct altr_a10sr_hwmon *hwmon;
>>>> +    int ret;
>>>> +    struct altr_a10sr *a10sr = dev_get_drvdata(pdev->dev.parent);
>>>> +
>>>
>>> What if someone creates a stand-alone devicetree property for this
>>> driver,
>>> and there is no parent (or parent driver data) ?
>>>
>> I agree if this was a commercially available part. However, this is a
>> specific driver for a programmable logic device (PLD) that will always
>> be part of an MFD parent. The PLD only supports the Altera Development
>> Kit.
>>
>>>> +    hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
>>>> +    if (!hwmon)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    hwmon->regmap = a10sr->regmap;
>>>> +
>>>> +    platform_set_drvdata(pdev, hwmon);
>>>> +
>>>> +    ret = sysfs_create_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>>>> +    if (ret)
>>>> +        goto err_mem;
>>>> +
>>>> +    hwmon->class_device = hwmon_device_register(&pdev->dev);
>>>> +    if (IS_ERR(hwmon->class_device)) {
>>>> +        ret = PTR_ERR(hwmon->class_device);
>>>> +        goto err_sysfs;
>>>> +    }
>>>
>>> Please use devm_hwmon_device_register_with_groups().
>>>
>> Understood. Thanks.
>>
>> Thanks for reviewing.
>>
>>>> +
>>>> +    return 0;
>>>> +
>>>> +err_sysfs:
>>>> +    sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>>>> +err_mem:
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static int altr_a10sr_hwmon_remove(struct platform_device *pdev)
>>>> +{
>>>> +    struct altr_a10sr_hwmon *hwmon = platform_get_drvdata(pdev);
>>>> +
>>>> +    hwmon_device_unregister(hwmon->class_device);
>>>> +    sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>> +static const struct of_device_id altr_a10sr_hwmon_of_match[] = {
>>>> +    { .compatible = "altr,a10sr-hwmon" },
>>>> +    { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(of, altr_a10sr_hwmon_of_match);
>>>> +
>>>> +static struct platform_driver altr_a10sr_hwmon_driver = {
>>>> +    .probe = altr_a10sr_hwmon_probe,
>>>> +    .remove = altr_a10sr_hwmon_remove,
>>>> +    .driver = {
>>>> +        .name = "altr_a10sr_hwmon",
>>>> +        .of_match_table = of_match_ptr(altr_a10sr_hwmon_of_match),
>>>> +    },
>>>> +};
>>>> +
>>>> +module_platform_driver(altr_a10sr_hwmon_driver);
>>>> +
>>>> +MODULE_LICENSE("GPL v2");
>>>> +MODULE_AUTHOR("Thor Thayer <tthayer@opensource.altera.com>");
>>>> +MODULE_DESCRIPTION("HW Monitor driver for Altera Arria10 System
>>>> Resource Chip");
>>>> --
>>>> 1.7.9.5
>>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index ff94007..af08846 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -248,6 +248,15 @@  config SENSORS_ADT7475
 	  This driver can also be build as a module.  If so, the module
 	  will be called adt7475.
 
+config SENSORS_ALTERA_A10SR
+	bool "Altera Arria10 System Status"
+	depends on MFD_ALTERA_A10SR
+	help
+	  If you say yes here you get support for the power ready status
+	  for the Arria10's external power supplies on the Arria10 DevKit.
+	  These values are read over the SPI bus from the Arria10 System
+	  Resource chip.
+
 config SENSORS_ASC7621
 	tristate "Andigilog aSC7621"
 	depends on I2C
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index 2ef5b7c..17d72a7 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -43,6 +43,7 @@  obj-$(CONFIG_SENSORS_ADT7411)	+= adt7411.o
 obj-$(CONFIG_SENSORS_ADT7462)	+= adt7462.o
 obj-$(CONFIG_SENSORS_ADT7470)	+= adt7470.o
 obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
+obj-$(CONFIG_SENSORS_ALTERA_A10SR) += altera-a10sr-hwmon.o
 obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
 obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
 obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
diff --git a/drivers/hwmon/altera-a10sr-hwmon.c b/drivers/hwmon/altera-a10sr-hwmon.c
new file mode 100644
index 0000000..1eecc6b
--- /dev/null
+++ b/drivers/hwmon/altera-a10sr-hwmon.c
@@ -0,0 +1,215 @@ 
+/*
+ *  Copyright Altera Corporation (C) 2014-2016. All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * HW Monitor driver for  Altera Arria10 MAX5 System Resource Chip
+ * Adapted from DA9052
+ */
+
+#include <linux/hwmon.h>
+#include <linux/hwmon-sysfs.h>
+#include <linux/mfd/altera-a10sr.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#define ALTR_A10SR_1V0_BIT_POS          ALTR_A10SR_PG1_1V0_SHIFT
+#define ALTR_A10SR_0V95_BIT_POS         ALTR_A10SR_PG1_0V95_SHIFT
+#define ALTR_A10SR_0V9_BIT_POS          ALTR_A10SR_PG1_0V9_SHIFT
+#define ALTR_A10SR_10V_BIT_POS          ALTR_A10SR_PG1_10V_SHIFT
+#define ALTR_A10SR_5V0_BIT_POS          ALTR_A10SR_PG1_5V0_SHIFT
+#define ALTR_A10SR_3V3_BIT_POS          ALTR_A10SR_PG1_3V3_SHIFT
+#define ALTR_A10SR_2V5_BIT_POS          ALTR_A10SR_PG1_2V5_SHIFT
+#define ALTR_A10SR_1V8_BIT_POS          ALTR_A10SR_PG1_1V8_SHIFT
+#define ALTR_A10SR_OP_FLAG_BIT_POS      ALTR_A10SR_PG1_OP_FLAG_SHIFT
+/* 2nd register needs an offset of 8 to get to 2nd register */
+#define ALTR_A10SR_FBC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FBC2MP_SHIFT)
+#define ALTR_A10SR_FAC2MP_BIT_POS       (8 + ALTR_A10SR_PG2_FAC2MP_SHIFT)
+#define ALTR_A10SR_FMCBVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCBVADJ_SHIFT)
+#define ALTR_A10SR_FMCAVADJ_BIT_POS     (8 + ALTR_A10SR_PG2_FMCAVADJ_SHIFT)
+#define ALTR_A10SR_HL_VDDQ_BIT_POS      (8 + ALTR_A10SR_PG2_HL_VDDQ_SHIFT)
+#define ALTR_A10SR_HL_VDD_BIT_POS       (8 + ALTR_A10SR_PG2_HL_VDD_SHIFT)
+#define ALTR_A10SR_HL_HPS_BIT_POS       (8 + ALTR_A10SR_PG2_HL_HPS_SHIFT)
+#define ALTR_A10SR_HPS_BIT_POS          (8 + ALTR_A10SR_PG2_HPS_SHIFT)
+/* 3rd register needs an offset of 16 to get to 3rd register */
+#define ALTR_A10SR_10V_FAIL_BIT_POS     (16 + ALTR_A10SR_PG3_10V_FAIL_SHIFT)
+#define ALTR_A10SR_FAM2C_BIT_POS        (16 + ALTR_A10SR_PG3_FAM2C_SHIFT)
+
+/**
+ * struct altr_a10sr_hwmon - Altera Max5 HWMON device private data structure
+ * @device: hwmon class.
+ * @regmap: the regmap from the parent device.
+ */
+struct altr_a10sr_hwmon {
+	struct device		*class_device;
+	struct regmap		*regmap;
+};
+
+static ssize_t altr_a10sr_read_status(struct device *dev,
+				      struct device_attribute *devattr,
+				      char *buf)
+{
+	struct altr_a10sr_hwmon *hwmon = dev_get_drvdata(dev);
+	int val, ret, index = to_sensor_dev_attr(devattr)->index;
+	int mask = ALTR_A10SR_REG_BIT_MASK(index);
+	unsigned char reg = ALTR_A10SR_PWR_GOOD1_REG +
+			    ALTR_A10SR_REG_OFFSET(index);
+
+	ret = regmap_read(hwmon->regmap, reg, &val);
+	if (ret < 0)
+		return ret;
+
+	return sprintf(buf, "%d\n", !!(~val & mask));
+}
+
+static ssize_t altr_a10sr_hwmon_show_name(struct device *dev,
+					  struct device_attribute *devattr,
+					  char *buf)
+{
+	return sprintf(buf, "altr_a10sr\n");
+}
+
+/* First Power Good Register Bits */
+static SENSOR_DEVICE_ATTR(1v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_1V0_BIT_POS);
+static SENSOR_DEVICE_ATTR(0v95_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_0V95_BIT_POS);
+static SENSOR_DEVICE_ATTR(0v9_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_0V9_BIT_POS);
+static SENSOR_DEVICE_ATTR(5v0_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_5V0_BIT_POS);
+static SENSOR_DEVICE_ATTR(3v3_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_3V3_BIT_POS);
+static SENSOR_DEVICE_ATTR(2v5_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_2V5_BIT_POS);
+static SENSOR_DEVICE_ATTR(1v8_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_1V8_BIT_POS);
+static SENSOR_DEVICE_ATTR(opflag_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_OP_FLAG_BIT_POS);
+/* Second Power Good Register Bits */
+static SENSOR_DEVICE_ATTR(fbc2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_FBC2MP_BIT_POS);
+static SENSOR_DEVICE_ATTR(fac2mp_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_FAC2MP_BIT_POS);
+static SENSOR_DEVICE_ATTR(fmcbvadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_FMCBVADJ_BIT_POS);
+static SENSOR_DEVICE_ATTR(fmcavadj_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_FMCAVADJ_BIT_POS);
+static SENSOR_DEVICE_ATTR(hl_vddq_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_HL_VDDQ_BIT_POS);
+static SENSOR_DEVICE_ATTR(hl_vdd_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_HL_VDD_BIT_POS);
+static SENSOR_DEVICE_ATTR(hlhps_vdd_alarm, S_IRUGO, altr_a10sr_read_status,
+			  NULL, ALTR_A10SR_HL_HPS_BIT_POS);
+static SENSOR_DEVICE_ATTR(hps_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_HPS_BIT_POS);
+/* Third Power Good Register Bits */
+static SENSOR_DEVICE_ATTR(10v_alarm, S_IRUGO, altr_a10sr_read_status,
+			  NULL, ALTR_A10SR_10V_FAIL_BIT_POS);
+static SENSOR_DEVICE_ATTR(fam2c_alarm, S_IRUGO, altr_a10sr_read_status, NULL,
+			  ALTR_A10SR_FAM2C_BIT_POS);
+
+static DEVICE_ATTR(name, S_IRUGO, altr_a10sr_hwmon_show_name, NULL);
+
+static struct attribute *altr_a10sr_attr[] = {
+	&dev_attr_name.attr,
+	/* First Power Good Register */
+	&sensor_dev_attr_opflag_alarm.dev_attr.attr,
+	&sensor_dev_attr_1v8_alarm.dev_attr.attr,
+	&sensor_dev_attr_2v5_alarm.dev_attr.attr,
+	&sensor_dev_attr_1v0_alarm.dev_attr.attr,
+	&sensor_dev_attr_3v3_alarm.dev_attr.attr,
+	&sensor_dev_attr_5v0_alarm.dev_attr.attr,
+	&sensor_dev_attr_0v9_alarm.dev_attr.attr,
+	&sensor_dev_attr_0v95_alarm.dev_attr.attr,
+	/* Second Power Good Register */
+	&sensor_dev_attr_hps_alarm.dev_attr.attr,
+	&sensor_dev_attr_hlhps_vdd_alarm.dev_attr.attr,
+	&sensor_dev_attr_hl_vdd_alarm.dev_attr.attr,
+	&sensor_dev_attr_hl_vddq_alarm.dev_attr.attr,
+	&sensor_dev_attr_fmcavadj_alarm.dev_attr.attr,
+	&sensor_dev_attr_fmcbvadj_alarm.dev_attr.attr,
+	&sensor_dev_attr_fac2mp_alarm.dev_attr.attr,
+	&sensor_dev_attr_fbc2mp_alarm.dev_attr.attr,
+	/* Third Power Good Register */
+	&sensor_dev_attr_10v_alarm.dev_attr.attr,
+	&sensor_dev_attr_fam2c_alarm.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group altr_a10sr_attr_group = {
+	.attrs = altr_a10sr_attr
+};
+
+static int altr_a10sr_hwmon_probe(struct platform_device *pdev)
+{
+	struct altr_a10sr_hwmon *hwmon;
+	int ret;
+	struct altr_a10sr *a10sr = dev_get_drvdata(pdev->dev.parent);
+
+	hwmon = devm_kzalloc(&pdev->dev, sizeof(*hwmon), GFP_KERNEL);
+	if (!hwmon)
+		return -ENOMEM;
+
+	hwmon->regmap = a10sr->regmap;
+
+	platform_set_drvdata(pdev, hwmon);
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
+	if (ret)
+		goto err_mem;
+
+	hwmon->class_device = hwmon_device_register(&pdev->dev);
+	if (IS_ERR(hwmon->class_device)) {
+		ret = PTR_ERR(hwmon->class_device);
+		goto err_sysfs;
+	}
+
+	return 0;
+
+err_sysfs:
+	sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
+err_mem:
+	return ret;
+}
+
+static int altr_a10sr_hwmon_remove(struct platform_device *pdev)
+{
+	struct altr_a10sr_hwmon *hwmon = platform_get_drvdata(pdev);
+
+	hwmon_device_unregister(hwmon->class_device);
+	sysfs_remove_group(&pdev->dev.kobj, &altr_a10sr_attr_group);
+
+	return 0;
+}
+
+static const struct of_device_id altr_a10sr_hwmon_of_match[] = {
+	{ .compatible = "altr,a10sr-hwmon" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, altr_a10sr_hwmon_of_match);
+
+static struct platform_driver altr_a10sr_hwmon_driver = {
+	.probe = altr_a10sr_hwmon_probe,
+	.remove = altr_a10sr_hwmon_remove,
+	.driver = {
+		.name = "altr_a10sr_hwmon",
+		.of_match_table = of_match_ptr(altr_a10sr_hwmon_of_match),
+	},
+};
+
+module_platform_driver(altr_a10sr_hwmon_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Thor Thayer <tthayer@opensource.altera.com>");
+MODULE_DESCRIPTION("HW Monitor driver for Altera Arria10 System Resource Chip");