Devicetree
 help / color / mirror / Atom feed
* Re: [PATCH linux v5 2/2] drivers: hwmon: Support for ASPEED PWM/Fan tach
From: Guenter Roeck @ 2017-04-01 22:45 UTC (permalink / raw)
  To: Jaghathiswari Rankappagounder Natarajan,
	joel-U3u1mxZcP9KHXe+LvDLADg, jdelvare-IBi9RG/b67k,
	linux-hwmon-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	openbmc-uLR06cmDAlY/bJ5BZ2RsiQ, corbet-T1hC0tSOHrs,
	linux-doc-u79uwXL29TY76Z2rM5mHXA, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8, devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20170324181723.29379-3-jaghu-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>

On 03/24/2017 11:17 AM, Jaghathiswari Rankappagounder Natarajan wrote:
> The ASPEED AST2400/2500 PWM controller supports 8 PWM output ports.
> The ASPEED AST2400/2500 Fan tach controller supports 16 tachometer
> inputs.
> The device driver matches on the device tree node. The configuration
> values
> are read from the device tree and written to the respective registers.
> The driver provides a sysfs entries through which the user can
> configure the duty-cycle value (ranging from 0 to 100 percent) and read
> the
> fan tach rpm value.
>

Odd line breaks.

> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <jaghu-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> ---
>  v5:
> - Changed the driver to suit the changes in the device tree documentation
>
>  v4:
> - Modified this driver to suit the representation in the devicetree
>
>  v3:
> - Only sent out device tree documentation; did not send this driver
>
>  v2:
> - Used BIT()
> - Used regmap
> - Avoided division when raw data is 0
> - Removed empty lines between declaration
> - Removed macros; Used two attribute groups and used is_visible callback
> - Returned error when properties are undefined
> - Removed .owner field
> - Used PTR_ERR_OR_ZERO
> - Removed explicit of_node_put for child nodes
>
>  Documentation/hwmon/aspeed-pwm-tacho |  22 +
>  drivers/hwmon/Kconfig                |  13 +-
>  drivers/hwmon/Makefile               |   1 +
>  drivers/hwmon/aspeed-pwm-tacho.c     | 854 +++++++++++++++++++++++++++++++++++
>  4 files changed, 888 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/hwmon/aspeed-pwm-tacho
>  create mode 100644 drivers/hwmon/aspeed-pwm-tacho.c
>
> diff --git a/Documentation/hwmon/aspeed-pwm-tacho b/Documentation/hwmon/aspeed-pwm-tacho
> new file mode 100644
> index 000000000000..0e9ec6d5f900
> --- /dev/null
> +++ b/Documentation/hwmon/aspeed-pwm-tacho
> @@ -0,0 +1,22 @@
> +Kernel driver aspeed-pwm-tacho
> +==============================
> +
> +Supported chips:
> +	ASPEED AST2400/2500
> +
> +Authors:
> +	<jaghu-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
> +
> +Description:
> +------------
> +This driver implements support for ASPEED AST2400/2500 PWM and Fan Tacho
> +controller. The PWM controller supports upto 8 PWM outputs. The Fan tacho
> +controller supports upto 16 tachometer inputs.
> +
up to

> +The driver provides the following sensor accesses in sysfs:
> +
> +fanX_input	ro	provide current fan rotation value in RPM as reported
> +			by the fan to the device.
> +
> +pwmX		rw	get or set PWM fan control value. This is an integer
> +			value between 0(off) and 255(full speed).
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 45cef3d2c75c..487110f21827 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -341,6 +341,15 @@ config SENSORS_ASB100
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called asb100.
>
> +config SENSORS_ASPEED
> +	tristate "ASPEED AST2400/AST2500 PWM and Fan tach driver"
> +	help
> +	  This driver provides support for ASPEED AST2400/AST2500 PWM
> +	  and Fan Tacho controllers.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called aspeed_pwm_tacho.
> +
>  config SENSORS_ATXP1
>  	tristate "Attansic ATXP1 VID controller"
>  	depends on I2C
> @@ -952,7 +961,7 @@ config SENSORS_LM70
>  	help
>  	  If you say yes here you get support for the National Semiconductor
>  	  LM70, LM71, LM74 and Texas Instruments TMP121/TMP123 digital tempera-
> -	  ture sensor chips.
> +	  true sensor chips.

Unrelated. Please drop. Separate patch would be appreciated, of course.

>
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called lm70.
> @@ -1506,7 +1515,7 @@ config SENSORS_ADS7871
>
>  config SENSORS_AMC6821
>  	tristate "Texas Instruments AMC6821"
> -	depends on I2C
> +	depends on I2C

Unrelated. Please drop.

>  	help
>  	  If you say yes here you get support for the Texas Instruments
>  	  AMC6821 hardware monitoring chips.
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index aecf4ba17460..83025cc9bb45 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -46,6 +46,7 @@ obj-$(CONFIG_SENSORS_ADT7475)	+= adt7475.o
>  obj-$(CONFIG_SENSORS_APPLESMC)	+= applesmc.o
>  obj-$(CONFIG_SENSORS_ARM_SCPI)	+= scpi-hwmon.o
>  obj-$(CONFIG_SENSORS_ASC7621)	+= asc7621.o
> +obj-$(CONFIG_SENSORS_ASPEED)	+= aspeed-pwm-tacho.o
>  obj-$(CONFIG_SENSORS_ATXP1)	+= atxp1.o
>  obj-$(CONFIG_SENSORS_CORETEMP)	+= coretemp.o
>  obj-$(CONFIG_SENSORS_DA9052_ADC)+= da9052-hwmon.o
> diff --git a/drivers/hwmon/aspeed-pwm-tacho.c b/drivers/hwmon/aspeed-pwm-tacho.c
> new file mode 100644
> index 000000000000..28888cc34555
> --- /dev/null
> +++ b/drivers/hwmon/aspeed-pwm-tacho.c
> @@ -0,0 +1,854 @@
> +/*
> + * Copyright (c) 2016 Google, Inc
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 or later as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/delay.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/sysfs.h>
> +#include <linux/regmap.h>
> +
> +/* ASPEED PWM & FAN Tach Register Definition */
> +#define ASPEED_PTCR_CTRL		0x00
> +#define ASPEED_PTCR_CLK_CTRL		0x04
> +#define ASPEED_PTCR_DUTY0_CTRL		0x08
> +#define ASPEED_PTCR_DUTY1_CTRL		0x0c
> +#define ASPEED_PTCR_TYPEM_CTRL		0x10
> +#define ASPEED_PTCR_TYPEM_CTRL1		0x14
> +#define ASPEED_PTCR_TYPEN_CTRL		0x18
> +#define ASPEED_PTCR_TYPEN_CTRL1		0x1c
> +#define ASPEED_PTCR_TACH_SOURCE		0x20
> +#define ASPEED_PTCR_TRIGGER		0x28
> +#define ASPEED_PTCR_RESULT		0x2c
> +#define ASPEED_PTCR_INTR_CTRL		0x30
> +#define ASPEED_PTCR_INTR_STS		0x34
> +#define ASPEED_PTCR_TYPEM_LIMIT		0x38
> +#define ASPEED_PTCR_TYPEN_LIMIT		0x3C
> +#define ASPEED_PTCR_CTRL_EXT		0x40
> +#define ASPEED_PTCR_CLK_CTRL_EXT	0x44
> +#define ASPEED_PTCR_DUTY2_CTRL		0x48
> +#define ASPEED_PTCR_DUTY3_CTRL		0x4c
> +#define ASPEED_PTCR_TYPEO_CTRL		0x50
> +#define ASPEED_PTCR_TYPEO_CTRL1		0x54
> +#define ASPEED_PTCR_TACH_SOURCE_EXT	0x60
> +#define ASPEED_PTCR_TYPEO_LIMIT		0x78
> +
> +/* ASPEED_PTCR_CTRL : 0x00 - General Control Register */
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1	15
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2	6
> +#define ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK	(BIT(7) | BIT(15))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1	14
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2	5
> +#define ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK	(BIT(6) | BIT(14))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1	13
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2	4
> +#define ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK	(BIT(5) | BIT(13))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1	12
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2	3
> +#define ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK	(BIT(4) | BIT(12))
> +
> +#define	ASPEED_PTCR_CTRL_FAN_NUM_EN(x)	BIT(16 + (x))
> +
> +#define	ASPEED_PTCR_CTRL_PWMD_EN	BIT(11)
> +#define	ASPEED_PTCR_CTRL_PWMC_EN	BIT(10)
> +#define	ASPEED_PTCR_CTRL_PWMB_EN	BIT(9)
> +#define	ASPEED_PTCR_CTRL_PWMA_EN	BIT(8)
> +
> +#define	ASPEED_PTCR_CTRL_CLK_SRC	BIT(1)
> +#define	ASPEED_PTCR_CTRL_CLK_EN		BIT(0)
> +
> +/* ASPEED_PTCR_CLK_CTRL : 0x04 - Clock Control Register */
> +/* TYPE N */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_MASK		GENMASK(31, 16)
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT		24
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_H		20
> +#define ASPEED_PTCR_CLK_CTRL_TYPEN_L		16
> +/* TYPE M */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_MASK         GENMASK(15, 0)
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT		8
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_H		4
> +#define ASPEED_PTCR_CLK_CTRL_TYPEM_L		0
> +
> +/*
> + * ASPEED_PTCR_DUTY_CTRL/1/2/3 : 0x08/0x0C/0x48/0x4C - PWM-FAN duty control
> + * 0/1/2/3 register
> + */
> +#define DUTY_CTRL_PWM2_FALL_POINT	24
> +#define DUTY_CTRL_PWM2_RISE_POINT	16
> +#define DUTY_CTRL_PWM2_RISE_FALL_MASK	GENMASK(31, 16)
> +#define DUTY_CTRL_PWM1_FALL_POINT	8
> +#define DUTY_CTRL_PWM1_RISE_POINT	0
> +#define DUTY_CTRL_PWM1_RISE_FALL_MASK   GENMASK(15, 0)
> +
> +/* ASPEED_PTCR_TYPEM_CTRL : 0x10/0x18/0x50 - Type M/N/O Ctrl 0 Register */
> +#define TYPE_CTRL_FAN_MASK		(GENMASK(5, 1) | GENMASK(31, 16))
> +#define TYPE_CTRL_FAN1_MASK		GENMASK(31, 0)
> +#define TYPE_CTRL_FAN_PERIOD		16
> +#define TYPE_CTRL_FAN_MODE		4
> +#define TYPE_CTRL_FAN_DIVISION		1
> +#define TYPE_CTRL_FAN_TYPE_EN		1
> +
> +/* ASPEED_PTCR_TACH_SOURCE : 0x20/0x60 - Tach Source Register */
> +/* bit [0,1] at 0x20, bit [2] at 0x60 */
> +#define TACH_PWM_SOURCE_BIT01(x)	((x) * 2)
> +#define TACH_PWM_SOURCE_BIT2(x)		((x) * 2)
> +#define TACH_PWM_SOURCE_MASK_BIT01(x)	(0x3 << ((x) * 2))
> +#define TACH_PWM_SOURCE_MASK_BIT2(x)	BIT((x) * 2)
> +
> +/* ASPEED_PTCR_RESULT : 0x2c - Result Register */
> +#define RESULT_STATUS_MASK		BIT(31)
> +#define RESULT_VALUE_MASK		0xfffff
> +
> +/* ASPEED_PTCR_CTRL_EXT : 0x40 - General Control Extension #1 Register */
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1	15
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2	6
> +#define ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK	(BIT(7) | BIT(15))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1	14
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2	5
> +#define ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK	(BIT(6) | BIT(14))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1	13
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2	4
> +#define ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK	(BIT(5) | BIT(13))
> +
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1	12
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2	3
> +#define ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK	(BIT(4) | BIT(12))
> +
> +#define	ASPEED_PTCR_CTRL_PWMH_EN	BIT(11)
> +#define	ASPEED_PTCR_CTRL_PWMG_EN	BIT(10)
> +#define	ASPEED_PTCR_CTRL_PWMF_EN	BIT(9)
> +#define	ASPEED_PTCR_CTRL_PWME_EN	BIT(8)
> +
> +/* ASPEED_PTCR_CLK_EXT_CTRL : 0x44 - Clock Control Extension #1 Register */
> +/* TYPE O */
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_MASK         GENMASK(15, 0)
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT		8
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_H		4
> +#define ASPEED_PTCR_CLK_CTRL_TYPEO_L		0
> +
> +#define PWM_MAX 255
> +
> +#define M_PWM_DIV_H 0x00
> +#define M_PWM_DIV_L 0x05
> +#define M_PWM_PERIOD 0x5F
> +#define M_TACH_CLK_DIV 0x00
> +#define M_TACH_MODE 0x00
> +#define M_TACH_UNIT 0x1000
> +#define INIT_FAN_CTRL 0xFF
> +
> +struct aspeed_pwm_tacho_regmap {
> +	void __iomem *regs;
> +};
> +

Can't you just use the regs pointer directly ? Why a structure with just one
variable in it ?

> +struct aspeed_pwm_tacho_data {
> +	struct regmap *regmap;
> +	unsigned long clk_freq;
> +	bool pwm_present[8];
> +	bool fan_tach_present[16];
> +	u8 type_pwm_clock_unit[3];
> +	u8 type_pwm_clock_division_h[3];
> +	u8 type_pwm_clock_division_l[3];
> +	u8 type_fan_tach_clock_division[3];
> +	u16 type_fan_tach_unit[3];
> +	u8 pwm_port_type[8];
> +	u8 pwm_port_fan_ctrl[8];
> +	u8 fan_tach_ch_source[16];
> +	const struct attribute_group *groups[2];

groups must be NULL terminated, so you'll need three groups here since
two are used.

> +};
> +
> +enum type { TYPEM, TYPEN, TYPEO };
> +
> +struct type_params {
> +	u32 l_value;
> +	u32 h_value;
> +	u32 unit_value;
> +	u32 clk_ctrl_mask;
> +	u32 clk_ctrl_reg;
> +	u32 ctrl_reg;
> +	u32 ctrl_reg1;
> +};
> +
> +static const struct type_params type_params[] = {
> +	[TYPEM] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEM_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEM_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEM_UNIT,
> +		.clk_ctrl_mask = ASPEED_PTCR_CLK_CTRL_TYPEM_MASK,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
> +		.ctrl_reg = ASPEED_PTCR_TYPEM_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEM_CTRL1,
> +	},
> +	[TYPEN] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEN_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEN_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEN_UNIT,
> +		.clk_ctrl_mask = ASPEED_PTCR_CLK_CTRL_TYPEN_MASK,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL,
> +		.ctrl_reg = ASPEED_PTCR_TYPEN_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEN_CTRL1,
> +	},
> +	[TYPEO] = {
> +		.l_value = ASPEED_PTCR_CLK_CTRL_TYPEO_L,
> +		.h_value = ASPEED_PTCR_CLK_CTRL_TYPEO_H,
> +		.unit_value = ASPEED_PTCR_CLK_CTRL_TYPEO_UNIT,
> +		.clk_ctrl_mask = ASPEED_PTCR_CLK_CTRL_TYPEO_MASK,
> +		.clk_ctrl_reg = ASPEED_PTCR_CLK_CTRL_EXT,
> +		.ctrl_reg = ASPEED_PTCR_TYPEO_CTRL,
> +		.ctrl_reg1 = ASPEED_PTCR_TYPEO_CTRL1,
> +	}
> +};
> +
> +enum pwm_port { PWMA, PWMB, PWMC, PWMD, PWME, PWMF, PWMG, PWMH };
> +
> +struct pwm_port_params {
> +	u32 pwm_en;
> +	u32 ctrl_reg;
> +	u32 type_part1;
> +	u32 type_part2;
> +	u32 type_mask;
> +	u32 duty_ctrl_rise_point;
> +	u32 duty_ctrl_fall_point;
> +	u32 duty_ctrl_reg;
> +	u32 duty_ctrl_rise_fall_mask;
> +};
> +
> +static const struct pwm_port_params pwm_port_params[] = {
> +	[PWMA] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMA_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMA_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM1_RISE_FALL_MASK,
> +	},
> +	[PWMB] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMB_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMB_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY0_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM2_RISE_FALL_MASK,
> +	},
> +	[PWMC] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMC_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMC_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM1_RISE_FALL_MASK,
> +	},
> +	[PWMD] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMD_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMD_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY1_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM2_RISE_FALL_MASK,
> +	},
> +	[PWME] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWME_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWME_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWME_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM1_RISE_FALL_MASK,
> +	},
> +	[PWMF] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMF_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMF_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY2_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM2_RISE_FALL_MASK,
> +	},
> +	[PWMG] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMG_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMG_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM1_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM1_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM1_RISE_FALL_MASK,
> +	},
> +	[PWMH] = {
> +		.pwm_en = ASPEED_PTCR_CTRL_PWMH_EN,
> +		.ctrl_reg = ASPEED_PTCR_CTRL_EXT,
> +		.type_part1 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART1,
> +		.type_part2 = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_PART2,
> +		.type_mask = ASPEED_PTCR_CTRL_SET_PWMH_TYPE_MASK,
> +		.duty_ctrl_rise_point = DUTY_CTRL_PWM2_RISE_POINT,
> +		.duty_ctrl_fall_point = DUTY_CTRL_PWM2_FALL_POINT,
> +		.duty_ctrl_reg = ASPEED_PTCR_DUTY3_CTRL,
> +		.duty_ctrl_rise_fall_mask = DUTY_CTRL_PWM2_RISE_FALL_MASK,
> +	}
> +};
> +
> +static int regmap_aspeed_pwm_tacho_reg_write(void *context, unsigned int reg,
> +					     unsigned int val)
> +{
> +	struct aspeed_pwm_tacho_regmap *hregmap = context;
> +
> +	writel(val, hregmap->regs + reg);
> +	return 0;
> +}
> +
> +static int regmap_aspeed_pwm_tacho_reg_read(void *context, unsigned int reg,
> +					    unsigned int *val)
> +{
> +	struct aspeed_pwm_tacho_regmap *hregmap = context;
> +
> +	*val = readl(hregmap->regs + reg);
> +	return 0;
> +}
> +
> +static const struct regmap_config aspeed_pwm_tacho_regmap_config = {
> +	.reg_bits = 32,
> +	.val_bits = 32,
> +	.reg_stride = 4,
> +	.max_register = ASPEED_PTCR_TYPEO_LIMIT,
> +	.reg_write = regmap_aspeed_pwm_tacho_reg_write,
> +	.reg_read = regmap_aspeed_pwm_tacho_reg_read,
> +	.fast_io = true,
> +};
> +
> +static void aspeed_set_clock_enable(struct regmap *regmap, bool val)
> +{
> +	regmap_update_bits(regmap, ASPEED_PTCR_CTRL,
> +			   ASPEED_PTCR_CTRL_CLK_EN,
> +			   val ? ASPEED_PTCR_CTRL_CLK_EN : 0);
> +}
> +
> +static void aspeed_set_clock_source(struct regmap *regmap, int val)
> +{
> +	regmap_update_bits(regmap, ASPEED_PTCR_CTRL,
> +			   ASPEED_PTCR_CTRL_CLK_SRC,
> +			   val ? ASPEED_PTCR_CTRL_CLK_SRC : 0);
> +}
> +
> +static void aspeed_set_pwm_clock_values(struct regmap *regmap, u8 type,
> +					u8 div_high, u8 div_low, u8 unit)
> +{
> +	u32 reg_value = ((div_high << type_params[type].h_value) |
> +			 (div_low << type_params[type].l_value) |
> +			 (unit << type_params[type].unit_value));
> +
> +	regmap_update_bits(regmap, type_params[type].clk_ctrl_reg,
> +			   type_params[type].clk_ctrl_mask, reg_value);
> +}
> +
> +static void aspeed_set_pwm_port_enable(struct regmap *regmap, u8 pwm_port,
> +				       bool enable)
> +{
> +	regmap_update_bits(regmap, pwm_port_params[pwm_port].ctrl_reg,
> +			   pwm_port_params[pwm_port].pwm_en,
> +			   enable ? pwm_port_params[pwm_port].pwm_en : 0);
> +}
> +
> +static void aspeed_set_pwm_port_type(struct regmap *regmap,
> +				     u8 pwm_port, u8 type)
> +{
> +	u32 reg_value = (type & 0x1) << pwm_port_params[pwm_port].type_part1;
> +
> +	reg_value |= (type & 0x2) << pwm_port_params[pwm_port].type_part2;
> +
> +	regmap_update_bits(regmap, pwm_port_params[pwm_port].ctrl_reg,
> +			   pwm_port_params[pwm_port].type_mask, reg_value);
> +}
> +
> +static void aspeed_set_pwm_port_duty_rising_falling(struct regmap *regmap,
> +						    u8 pwm_port, u8 rising,
> +						    u8 falling)
> +{
> +	u32 reg_value = (rising <<
> +			 pwm_port_params[pwm_port].duty_ctrl_rise_point);
> +	reg_value |= (falling <<
> +		      pwm_port_params[pwm_port].duty_ctrl_fall_point);
> +
> +	regmap_update_bits(regmap, pwm_port_params[pwm_port].duty_ctrl_reg,
> +			   pwm_port_params[pwm_port].duty_ctrl_rise_fall_mask,
> +			   reg_value);
> +}
> +
> +static void aspeed_set_tacho_type_enable(struct regmap *regmap, u8 type,
> +					 bool enable)
> +{
> +	regmap_update_bits(regmap, type_params[type].ctrl_reg,
> +			   TYPE_CTRL_FAN_TYPE_EN,
> +			   enable ? TYPE_CTRL_FAN_TYPE_EN : 0);
> +}
> +
> +static void aspeed_set_tacho_type_values(struct regmap *regmap, u8 type,
> +					 u8 mode, u16 unit, u8 division)
> +{
> +	u32 reg_value = ((mode << TYPE_CTRL_FAN_MODE) |
> +			 (unit << TYPE_CTRL_FAN_PERIOD) |
> +			 (division << TYPE_CTRL_FAN_DIVISION));
> +
> +	regmap_update_bits(regmap, type_params[type].ctrl_reg,
> +			   TYPE_CTRL_FAN_MASK, reg_value);
> +	regmap_update_bits(regmap, type_params[type].ctrl_reg1,
> +			   TYPE_CTRL_FAN1_MASK, unit << 16);
> +}
> +
> +static void aspeed_set_fan_tach_ch_enable(struct regmap *regmap, u8 fan_tach_ch,
> +					  bool enable)
> +{
> +	regmap_update_bits(regmap, ASPEED_PTCR_CTRL,
> +			   ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch),
> +			   enable ?
> +			   ASPEED_PTCR_CTRL_FAN_NUM_EN(fan_tach_ch) : 0);
> +}
> +
> +static void aspeed_set_fan_tach_ch_source(struct regmap *regmap, u8 fan_tach_ch,
> +					  u8 fan_tach_ch_source)
> +{
> +	u32 reg_value1 = ((fan_tach_ch_source & 0x3) <<
> +			  TACH_PWM_SOURCE_BIT01(fan_tach_ch));
> +	u32 reg_value2 = (((fan_tach_ch_source & 0x4) >> 2) <<
> +			  TACH_PWM_SOURCE_BIT2(fan_tach_ch));
> +
> +	regmap_update_bits(regmap, ASPEED_PTCR_TACH_SOURCE,
> +			   TACH_PWM_SOURCE_MASK_BIT01(fan_tach_ch),
> +			   reg_value1);
> +
> +	regmap_update_bits(regmap, ASPEED_PTCR_TACH_SOURCE_EXT,
> +			   TACH_PWM_SOURCE_MASK_BIT2(fan_tach_ch),
> +			   reg_value2);
> +}
> +
> +static void aspeed_set_pwm_port_fan_ctrl(struct aspeed_pwm_tacho_data *priv,
> +					 u8 index, u8 fan_ctrl)
> +{
> +	u16 period, dc_time_on;
> +
> +	period = priv->type_pwm_clock_unit[priv->pwm_port_type[index]];
> +	period += 1;
> +	dc_time_on = (fan_ctrl * period) / PWM_MAX;
> +
> +	if (dc_time_on == 0) {
> +		aspeed_set_pwm_port_enable(priv->regmap, index, false);
> +	} else {
> +		if (dc_time_on == period)
> +			dc_time_on = 0;
> +
> +		aspeed_set_pwm_port_duty_rising_falling(priv->regmap, index, 0,
> +							dc_time_on);
> +		aspeed_set_pwm_port_enable(priv->regmap, index, true);
> +	}
> +}
> +
> +static u32 aspeed_get_fan_tach_ch_measure_period(struct aspeed_pwm_tacho_data
> +						 *priv, u8 type)
> +{
> +	u32 clk;
> +	u16 tacho_unit;
> +	u8 clk_unit, div_h, div_l, tacho_div;
> +
> +	clk = priv->clk_freq;
> +	clk_unit = priv->type_pwm_clock_unit[type];
> +	div_h = priv->type_pwm_clock_division_h[type];
> +	div_h = 0x1 << div_h;
> +	div_l = priv->type_pwm_clock_division_l[type];
> +	if (div_l == 0)
> +		div_l = 1;
> +	else
> +		div_l = div_l * 2;
> +
> +	tacho_unit = priv->type_fan_tach_unit[type];
> +	tacho_div = priv->type_fan_tach_clock_division[type];
> +
> +	tacho_div = 0x4 << (tacho_div * 2);
> +	return clk / (clk_unit * div_h * div_l * tacho_div * tacho_unit);
> +}
> +
> +static u32 aspeed_get_fan_tach_ch_rpm(struct aspeed_pwm_tacho_data *priv,
> +				      u8 fan_tach_ch)
> +{
> +	u32 raw_data, tach_div, clk_source, timeout = 0, sec, val;
> +	u8 fan_tach_ch_source, type;
> +
> +	regmap_write(priv->regmap, ASPEED_PTCR_TRIGGER, 0);
> +	regmap_write(priv->regmap, ASPEED_PTCR_TRIGGER, 0x1 << fan_tach_ch);
> +
> +	fan_tach_ch_source = priv->fan_tach_ch_source[fan_tach_ch];
> +	type = priv->pwm_port_type[fan_tach_ch_source];
> +
> +	sec = (1000 / aspeed_get_fan_tach_ch_measure_period(priv, type));
> +
> +	msleep(sec);
> +
> +	regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val);
> +	while (!(val & RESULT_STATUS_MASK)) {
> +		timeout++;
> +		if (timeout > 1)
> +			return 0;
> +		msleep(sec);

val never changes.

> +	}
> +
> +	regmap_read(priv->regmap, ASPEED_PTCR_RESULT, &val);
> +	raw_data = val & RESULT_VALUE_MASK;
> +	tach_div = priv->type_fan_tach_clock_division[type];
> +
> +	tach_div = 0x4 << (tach_div * 2);
> +	clk_source = priv->clk_freq;
> +
> +	if (raw_data == 0)
> +		return 0;
> +	else

else is unnecessary after return

> +		return (clk_source * 60) / (2 * raw_data * tach_div);
> +}
> +
> +static ssize_t set_pwm(struct device *dev, struct device_attribute *attr,
> +		       const char *buf, size_t count)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	int ret;
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +	long fan_ctrl;
> +
> +	ret = kstrtol(buf, 10, &fan_ctrl);
> +	if (ret != 0)
> +		return ret;
> +
> +	if (fan_ctrl < 0 || fan_ctrl > PWM_MAX)
> +		return -EINVAL;
> +
> +	if (priv->pwm_port_fan_ctrl[index] == fan_ctrl)
> +		return count;
> +
> +	priv->pwm_port_fan_ctrl[index] = fan_ctrl;
> +	aspeed_set_pwm_port_fan_ctrl(priv, index, fan_ctrl);
> +
> +	return count;
> +}
> +
> +static ssize_t show_pwm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%u\n", priv->pwm_port_fan_ctrl[index]);
> +}
> +
> +static ssize_t show_rpm(struct device *dev, struct device_attribute *attr,
> +			char *buf)
> +{
> +	struct sensor_device_attribute *sensor_attr = to_sensor_dev_attr(attr);
> +	int index = sensor_attr->index;
> +	u32 rpm;
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	rpm = aspeed_get_fan_tach_ch_rpm(priv, index);
> +
> +	return sprintf(buf, "%u\n", rpm);
> +}
> +
> +static umode_t pwm_is_visible(struct kobject *kobj,
> +			      struct attribute *a, int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	if (!priv->pwm_present[index])
> +		return 0;
> +	return a->mode;
> +}
> +
> +static umode_t fan_dev_is_visible(struct kobject *kobj,
> +				  struct attribute *a, int index)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct aspeed_pwm_tacho_data *priv = dev_get_drvdata(dev);
> +
> +	if (!priv->fan_tach_present[index])
> +		return 0;
> +	return a->mode;
> +}
> +
> +static SENSOR_DEVICE_ATTR(pwm0, 0644,
> +			show_pwm, set_pwm, 0);
> +static SENSOR_DEVICE_ATTR(pwm1, 0644,
> +			show_pwm, set_pwm, 1);
> +static SENSOR_DEVICE_ATTR(pwm2, 0644,
> +			show_pwm, set_pwm, 2);
> +static SENSOR_DEVICE_ATTR(pwm3, 0644,
> +			show_pwm, set_pwm, 3);
> +static SENSOR_DEVICE_ATTR(pwm4, 0644,
> +			show_pwm, set_pwm, 4);
> +static SENSOR_DEVICE_ATTR(pwm5, 0644,
> +			show_pwm, set_pwm, 5);
> +static SENSOR_DEVICE_ATTR(pwm6, 0644,
> +			show_pwm, set_pwm, 6);
> +static SENSOR_DEVICE_ATTR(pwm7, 0644,
> +			show_pwm, set_pwm, 7);

Unnecessary continuation lines.

> +
> +static struct attribute *pwm_dev_attrs[] = {
> +	&sensor_dev_attr_pwm0.dev_attr.attr,
> +	&sensor_dev_attr_pwm1.dev_attr.attr,
> +	&sensor_dev_attr_pwm2.dev_attr.attr,
> +	&sensor_dev_attr_pwm3.dev_attr.attr,
> +	&sensor_dev_attr_pwm4.dev_attr.attr,
> +	&sensor_dev_attr_pwm5.dev_attr.attr,
> +	&sensor_dev_attr_pwm6.dev_attr.attr,
> +	&sensor_dev_attr_pwm7.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group pwm_dev_group = {
> +	.attrs = pwm_dev_attrs,
> +	.is_visible = pwm_is_visible,
> +};
> +
> +static SENSOR_DEVICE_ATTR(fan0_input, 0444,
> +		show_rpm, NULL, 0);
> +static SENSOR_DEVICE_ATTR(fan1_input, 0444,
> +		show_rpm, NULL, 1);
> +static SENSOR_DEVICE_ATTR(fan2_input, 0444,
> +		show_rpm, NULL, 2);
> +static SENSOR_DEVICE_ATTR(fan3_input, 0444,
> +		show_rpm, NULL, 3);
> +static SENSOR_DEVICE_ATTR(fan4_input, 0444,
> +		show_rpm, NULL, 4);
> +static SENSOR_DEVICE_ATTR(fan5_input, 0444,
> +		show_rpm, NULL, 5);
> +static SENSOR_DEVICE_ATTR(fan6_input, 0444,
> +		show_rpm, NULL, 6);
> +static SENSOR_DEVICE_ATTR(fan7_input, 0444,
> +		show_rpm, NULL, 7);
> +static SENSOR_DEVICE_ATTR(fan8_input, 0444,
> +		show_rpm, NULL, 8);
> +static SENSOR_DEVICE_ATTR(fan9_input, 0444,
> +		show_rpm, NULL, 9);
> +static SENSOR_DEVICE_ATTR(fan10_input, 0444,
> +		show_rpm, NULL, 10);
> +static SENSOR_DEVICE_ATTR(fan11_input, 0444,
> +		show_rpm, NULL, 11);
> +static SENSOR_DEVICE_ATTR(fan12_input, 0444,
> +		show_rpm, NULL, 12);
> +static SENSOR_DEVICE_ATTR(fan13_input, 0444,
> +		show_rpm, NULL, 13);
> +static SENSOR_DEVICE_ATTR(fan14_input, 0444,
> +		show_rpm, NULL, 14);
> +static SENSOR_DEVICE_ATTR(fan15_input, 0444,
> +		show_rpm, NULL, 15);
> +
Unnecessary continuation lines.

> +static struct attribute *fan_dev_attrs[] = {
> +	&sensor_dev_attr_fan0_input.dev_attr.attr,
> +	&sensor_dev_attr_fan1_input.dev_attr.attr,
> +	&sensor_dev_attr_fan2_input.dev_attr.attr,
> +	&sensor_dev_attr_fan3_input.dev_attr.attr,
> +	&sensor_dev_attr_fan4_input.dev_attr.attr,
> +	&sensor_dev_attr_fan5_input.dev_attr.attr,
> +	&sensor_dev_attr_fan6_input.dev_attr.attr,
> +	&sensor_dev_attr_fan7_input.dev_attr.attr,
> +	&sensor_dev_attr_fan8_input.dev_attr.attr,
> +	&sensor_dev_attr_fan9_input.dev_attr.attr,
> +	&sensor_dev_attr_fan10_input.dev_attr.attr,
> +	&sensor_dev_attr_fan11_input.dev_attr.attr,
> +	&sensor_dev_attr_fan12_input.dev_attr.attr,
> +	&sensor_dev_attr_fan13_input.dev_attr.attr,
> +	&sensor_dev_attr_fan14_input.dev_attr.attr,
> +	&sensor_dev_attr_fan15_input.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group fan_dev_group = {
> +	.attrs = fan_dev_attrs,
> +	.is_visible = fan_dev_is_visible,
> +};
> +
> +/*
> + * The clock type is type M :
> + * The PWM frequency = 24MHz / (type M clock division L bit *
> + * type M clock division H bit * (type M PWM period bit + 1))
> + */
> +static void aspeed_create_type(struct aspeed_pwm_tacho_data *priv)
> +{
> +	priv->type_pwm_clock_division_h[TYPEM] = M_PWM_DIV_H;
> +	priv->type_pwm_clock_division_l[TYPEM] = M_PWM_DIV_L;
> +	priv->type_pwm_clock_unit[TYPEM] = M_PWM_PERIOD;
> +	aspeed_set_pwm_clock_values(priv->regmap, TYPEM, M_PWM_DIV_H,
> +				    M_PWM_DIV_L, M_PWM_PERIOD);
> +	aspeed_set_tacho_type_enable(priv->regmap, TYPEM, true);
> +	priv->type_fan_tach_clock_division[TYPEM] = M_TACH_CLK_DIV;
> +	priv->type_fan_tach_unit[TYPEM] = M_TACH_UNIT;
> +	aspeed_set_tacho_type_values(priv->regmap, TYPEM, M_TACH_MODE,
> +				     M_TACH_UNIT, M_TACH_CLK_DIV);
> +}
> +
> +static void aspeed_create_pwm_port(struct aspeed_pwm_tacho_data *priv,
> +				   u8 pwm_port)
> +{
> +	aspeed_set_pwm_port_enable(priv->regmap, pwm_port, true);
> +	priv->pwm_present[pwm_port] = true;
> +
> +	priv->pwm_port_type[pwm_port] = TYPEM;
> +	aspeed_set_pwm_port_type(priv->regmap, pwm_port, TYPEM);
> +
> +	priv->pwm_port_fan_ctrl[pwm_port] = INIT_FAN_CTRL;
> +	aspeed_set_pwm_port_fan_ctrl(priv, pwm_port, INIT_FAN_CTRL);
> +}
> +
> +static void aspeed_create_fan_tach_channel(struct aspeed_pwm_tacho_data *priv,
> +					   u8 *fan_tach_ch,
> +					   int count,
> +					   u8 pwm_source)
> +{
> +	u8 val, index;
> +
> +	for (val = 0; val < count; val++) {
> +		index = fan_tach_ch[val];
> +		aspeed_set_fan_tach_ch_enable(priv->regmap, index, true);
> +		priv->fan_tach_present[index] = true;
> +		priv->fan_tach_ch_source[index] = pwm_source;
> +		aspeed_set_fan_tach_ch_source(priv->regmap, index, pwm_source);
> +	}
> +}
> +
> +static int aspeed_create_fan(struct device *dev,
> +			     struct device_node *child,
> +			     struct aspeed_pwm_tacho_data *priv)
> +{
> +	u8 *fan_tach_ch;
> +	u32 pwm_port;
> +	int ret, count;
> +
> +	ret = of_property_read_u32(child, "reg", &pwm_port);
> +	if (ret)
> +		return ret;
> +	aspeed_create_pwm_port(priv, (u8)pwm_port);
> +
> +	count = of_property_count_u8_elems(child, "aspeed-fan-tach-ch");
> +	if (count < 1)
> +		return -EINVAL;
> +	fan_tach_ch = devm_kzalloc(dev, sizeof(*fan_tach_ch) * count,
> +				   GFP_KERNEL);
> +	if (!fan_tach_ch)
> +		return -ENOMEM;
> +	ret = of_property_read_u8_array(child, "aspeed-fan-tach-ch",
> +					fan_tach_ch, count);
> +	if (ret)
> +		return ret;
> +	aspeed_create_fan_tach_channel(priv, fan_tach_ch, count, pwm_port);
> +
> +	return 0;
> +}
> +
> +static int aspeed_pwm_tacho_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np, *child;
> +	struct aspeed_pwm_tacho_data *priv;
> +	struct aspeed_pwm_tacho_regmap *hregmap;
> +	struct resource *res;
> +	struct device *hwmon;
> +	struct clk *clk;
> +	int ret;
> +
> +	np = dev->of_node;
> +
> +	hregmap = devm_kzalloc(dev, sizeof(*hregmap), GFP_KERNEL);
> +	if (!hregmap)
> +		return -ENOMEM;
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENOENT;
> +	hregmap->regs = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(hregmap->regs))
> +		return PTR_ERR(hregmap->regs);
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	priv->regmap = devm_regmap_init(dev, NULL, hregmap,
> +			&aspeed_pwm_tacho_regmap_config);
> +	if (IS_ERR(priv->regmap))
> +		return PTR_ERR(priv->regmap);
> +	regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE, 0);
> +	regmap_write(priv->regmap, ASPEED_PTCR_TACH_SOURCE_EXT, 0);
> +
> +	clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(clk))
> +		return -ENODEV;
> +	priv->clk_freq = clk_get_rate(clk);
> +	aspeed_set_clock_enable(priv->regmap, true);
> +	aspeed_set_clock_source(priv->regmap, 0);
> +
> +	aspeed_create_type(priv);
> +
> +	for_each_child_of_node(np, child) {
> +		ret = aspeed_create_fan(dev, child, priv);
> +		of_node_put(child);
> +		if (ret)
> +			return ret;
> +	}
> +	of_node_put(np);
> +
> +	priv->groups[0] = &pwm_dev_group;
> +	priv->groups[1] = &fan_dev_group;
> +	hwmon = devm_hwmon_device_register_with_groups(dev,
> +						       "aspeed_pwm_tacho",
> +						       priv, priv->groups);
> +
> +	return PTR_ERR_OR_ZERO(hwmon);
> +}
> +
> +static const struct of_device_id of_pwm_tacho_match_table[] = {
> +	{ .compatible = "aspeed,ast2400-pwm-tacho", },
> +	{ .compatible = "aspeed,ast2500-pwm-tacho", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, of_pwm_tacho_match_table);
> +
> +static struct platform_driver aspeed_pwm_tacho_driver = {
> +	.probe		= aspeed_pwm_tacho_probe,
> +	.driver		= {
> +		.name	= "aspeed_pwm_tacho",
> +		.of_match_table = of_pwm_tacho_match_table,
> +	},
> +};
> +
> +module_platform_driver(aspeed_pwm_tacho_driver);
> +
> +MODULE_AUTHOR("Jaghathiswari Rankappagounder Natarajan <jaghu-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("ASPEED PWM and Fan Tacho device driver");
> +MODULE_LICENSE("GPL");
> --
> 2.12.1.578.ge9c3154ca4-goog
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH V4 1/2] dt-bindings: thermal: add support for Broadcom's Northstar thermal
From: Eduardo Valentin @ 2017-04-02  4:13 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Florian Fainelli, Jon Mason, Zhang Rui, Rob Herring, Mark Rutland,
	Stephen Warren, Lee Jones, Eric Anholt, Ray Jui, Scott Branden,
	bcm-kernel-feedback-list-dY08KVG/lbpWk0Htik3J/w,
	linux-pm-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-rpi-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	Rafał Miłecki
In-Reply-To: <6dd907e4-c213-3174-8613-99427e6ea0e9-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Hey,

On Sat, Apr 01, 2017 at 11:50:26PM +0200, Rafał Miłecki wrote:
> On 04/01/2017 09:51 PM, Eduardo Valentin wrote:
> >On Fri, Mar 31, 2017 at 10:11:23PM +0200, Rafał Miłecki wrote:
> >>From: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> >>
> >>This commit documents binding for thermal used in Northstar family SoCs.
> >>
> >>Signed-off-by: Rafał Miłecki <rafal-g1n6cQUeyibVItvQsEIGlw@public.gmane.org>
> >>---
> >>V3: Add thermal-zones to the example
> >>    Rob: Because of this update, I didn't include Acked-by I got for V2
> >>---
> >> .../devicetree/bindings/thermal/brcm,ns-thermal    | 26 ++++++++++++++++++++++
> >> 1 file changed, 26 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/thermal/brcm,ns-thermal
> >>
> >>diff --git a/Documentation/devicetree/bindings/thermal/brcm,ns-thermal b/Documentation/devicetree/bindings/thermal/brcm,ns-thermal
> >>new file mode 100644
> >>index 000000000000..c561c7349f17
> >>--- /dev/null
> >>+++ b/Documentation/devicetree/bindings/thermal/brcm,ns-thermal
> >>@@ -0,0 +1,26 @@
> >>+* Broadcom Northstar Thermal
> >>+
> >>+This binding describes thermal sensor that is part of Northstar's DMU (Device
> >>+Management Unit).
> >>+
> >>+Required properties:
> >>+- compatible : Must be "brcm,ns-thermal"
> >>+- reg : iomem address range of PVTMON registers
> >>+- #thermal-sensor-cells : Should be <0>
> >>+
> >>+Example:
> >>+
> >>+thermal: thermal@1800c2c0 {
> >>+	compatible = "brcm,ns-thermal";
> >>+	reg = <0x1800c2c0 0x10>;
> >>+	#thermal-sensor-cells = <0>;
> >>+};
> >>+
> >>+thermal-zones {
> >>+	cpu_thermal: cpu-thermal {
> >>+		polling-delay-passive = <0>;
> >>+		polling-delay = <1000>;
> >>+		coefficients = <(-556) 418000>;
> >>+		thermal-sensors = <&thermal>;
> >
> >You need to define trips and cooling devices here. Otherwise, makes
> >little sense to have this device in thermal subsystem. Here is an
> >example of minimal set:
> >https://git.kernel.org/pub/scm/linux/kernel/git/evalenti/linux-soc-thermal.git/commit/?h=linus&id=1e2ac9821de6a85d3e8358f238436708d1d46869
> >
> >The above has no passive action. It is just gonna shutdown the system if
> >temperature crosses a threshold.
> >
> >But, a typical cooling device would be CPU frequency throttling. Do you have
> >that up and running in your routers?
> 
> I don't have CPU freq throttling, so shutdown will be the only solution for
> critical temp right now.

OK. Is there any plans to get cpu cooling / DVFS working on your boards?

> 
> I know I should have at least a trip for critical temperature, but the problem
> is I don't know what value to use. There isn't any info about this in public

And that is enough justification to block this to go upstream. We need
the correct shutdown threshold value to put as example. If we don't,
users of this drivers will be pretty much guessing.

> datasheets. Broadcom's SDK doesn't mention it. Vendors share only the max
> environment temp, not the max CPU temp.

Silicon goes bad typically at 125C. But that also typically requires a
good extrapolation of hotspot(s). Some drivers prefer to play safer and
use lower numbers (100C).

> 
> So for now I only meant to provide user space access to reading current CPU
> temperature. I could do some stress tests and ask other users to do it as well.
> 

I see..


> Or maybe I could just put in Documentation some round value that makes more or
> less sense and then work on a proper content of real DTS files?
> 

The problem with the above is that I see very commonly examples being
copied into DTS. So, better to have a good binding
example/documentation. That is what they are for anyway.

> Unless we can get some hint from Broadcom people. Jon? Florian? Anyone?
> 

Yeah, that would be probably best.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v2 0/5] Add ARM Mali Midgard device tree bindings and gpu node for rk3288
From: Guillaume Tucker @ 2017-04-02  7:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Sjoerd Simons, Enric Balletbo i Serra, John Reitan, Wookey,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Guillaume Tucker

The ARM Mali Midgard GPU kernel driver is only available
out-of-tree and is not going to be merged in its current form.
However, it would be useful to have its device tree bindings
merged.  In particular, this would enable distributions to create
working driver packages (dkms...) without having to patch the
kernel.

The bindings for the earlier Mali Utgard GPU family have already
been merged, so this is essentially the same scenario but for
newer GPUs (Mali-T604 ~ Mali-T880).

This series of patches first imports the bindings from the latest
driver release with some clean-up then adds a gpu node for the
rk3288 SoC.  This was successfully tested on Radxa Rock2 Square,
Firefly, Veyron Minnie and Jerry boards board using Mali kernel
driver r16p0 and r12p0 user-space binary.

Changes since v1:
- enabled gpu on rk3288-veyron boards

Enric Balletbo i Serra (1):
  ARM: dts: rockchip: enable ARM Mali GPU on rk3288-veyron

Guillaume Tucker (4):
  dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU
  ARM: dts: rockchip: add ARM Mali GPU node for rk3288
  ARM: dts: rockchip: enable ARM Mali GPU on rk3288-rock2-som
  ARM: dts: rockchip: enable ARM Mali GPU on rk3288-firefly

 .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
 arch/arm/boot/dts/rk3288-firefly.dtsi              |  5 ++
 arch/arm/boot/dts/rk3288-rock2-som.dtsi            |  5 ++
 arch/arm/boot/dts/rk3288-veyron.dtsi               |  5 ++
 arch/arm/boot/dts/rk3288.dtsi                      | 23 ++++++++++
 5 files changed, 91 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt

--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* [PATCH v2 1/5] dt-bindings: gpu: add bindings for the ARM Mali Midgard GPU
From: Guillaume Tucker @ 2017-04-02  7:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Sjoerd Simons, Enric Balletbo i Serra, John Reitan, Wookey,
	devicetree, linux-rockchip, linux-arm-kernel, linux-kernel,
	Guillaume Tucker
In-Reply-To: <cover.1491118230.git.guillaume.tucker@collabora.com>

The ARM Mali Midgard GPU family is present in a number of SoCs
from many different vendors such as Samsung Exynos and Rockchip.

Import the device tree bindings documentation from the r16p0
release of the Mali Midgard GPU kernel driver:

  https://developer.arm.com/-/media/Files/downloads/mali-drivers/kernel/mali-midgard-gpu/TX011-SW-99002-r16p0-00rel0.tgz

The following optional bindings have been omitted in this initial
version as they are only used in very specific cases:

  * snoop_enable_smc
  * snoop_disable_smc
  * jm_config
  * power_model
  * system-coherency
  * ipa-model

The example has been simplified accordingly.

The compatible string definition has been limited to
"arm,mali-midgard" to avoid checkpatch.pl warnings and to match
what the driver actually expects (as of r16p0 out-of-tree).

CC: John Reitan <john.reitan@arm.com>
Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 .../devicetree/bindings/gpu/arm,mali-midgard.txt   | 53 ++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
new file mode 100644
index 000000000000..da8fc6d21bbf
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpu/arm,mali-midgard.txt
@@ -0,0 +1,53 @@
+#
+# (C) COPYRIGHT 2013-2016 ARM Limited.
+# Copyright (C) 2017 Collabora Ltd
+#
+# This program is free software and is provided to you under the terms of the
+# GNU General Public License version 2 as published by the Free Software
+# Foundation, and any use by you of this program is subject to the terms
+# of such GNU licence.
+#
+
+
+ARM Mali Midgard GPU
+====================
+
+Required properties:
+
+- compatible : Should be "arm,mali-midgard".
+- reg : Physical base address of the device and length of the register area.
+- interrupts : Contains the three IRQ lines required by Mali Midgard devices.
+- interrupt-names : Contains the names of IRQ resources in the order they were
+  provided in the interrupts property. Must contain: "JOB, "MMU", "GPU".
+
+Optional:
+
+- clocks : Phandle to clock for the Mali Midgard device.
+- clock-names : Shall be "clk_mali".
+- mali-supply : Phandle to regulator for the Mali device. Refer to
+  Documentation/devicetree/bindings/regulator/regulator.txt for details.
+- operating-points : Refer to Documentation/devicetree/bindings/power/opp.txt
+  for details.
+
+Example for a Mali-T602:
+
+gpu@0xfc010000 {
+	compatible = "arm,mali-midgard";
+	reg = <0xfc010000 0x4000>;
+	interrupts = <0 36 4>, <0 37 4>, <0 38 4>;
+	interrupt-names = "JOB", "MMU", "GPU";
+
+	clocks = <&pclk_mali>;
+	clock-names = "clk_mali";
+	mali-supply = <&vdd_mali>;
+	operating-points = <
+		/* KHz   uV */
+		533000 1250000,
+		450000 1150000,
+		400000 1125000,
+		350000 1075000,
+		266000 1025000,
+		160000  925000,
+		100000  912500,
+	>;
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 2/5] ARM: dts: rockchip: add ARM Mali GPU node for rk3288
From: Guillaume Tucker @ 2017-04-02  7:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: devicetree, Guillaume Tucker, Sjoerd Simons, Wookey, linux-kernel,
	linux-rockchip, John Reitan, Enric Balletbo i Serra,
	linux-arm-kernel
In-Reply-To: <cover.1491118230.git.guillaume.tucker@collabora.com>

Add Mali GPU device tree node for the rk3288 SoC, with devfreq
opp table.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/rk3288.dtsi | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index df8a0dbe9d91..213224ea309e 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -43,6 +43,7 @@
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/pinctrl/rockchip.h>
 #include <dt-bindings/clock/rk3288-cru.h>
+#include <dt-bindings/power/rk3288-power.h>
 #include <dt-bindings/thermal/thermal.h>
 #include <dt-bindings/power/rk3288-power.h>
 #include <dt-bindings/soc/rockchip,boot-mode.h>
@@ -227,6 +228,28 @@
 		ports = <&vopl_out>, <&vopb_out>;
 	};
 
+	gpu: mali@ffa30000 {
+		compatible = "arm,mali-midgard";
+		reg = <0xffa30000 0x10000>;
+		interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "JOB", "MMU", "GPU";
+		clocks = <&cru ACLK_GPU>;
+		clock-names = "clk_mali";
+		operating-points = <
+			/* KHz uV */
+			100000 950000
+			200000 950000
+			300000 1000000
+			400000 1100000
+			500000 1200000
+			600000 1250000
+		>;
+		power-domains = <&power RK3288_PD_GPU>;
+		status = "disabled";
+	};
+
 	sdmmc: dwmmc@ff0c0000 {
 		compatible = "rockchip,rk3288-dw-mshc";
 		max-frequency = <150000000>;
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 3/5] ARM: dts: rockchip: enable ARM Mali GPU on rk3288-rock2-som
From: Guillaume Tucker @ 2017-04-02  7:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Sjoerd Simons, Enric Balletbo i Serra, John Reitan, Wookey,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Guillaume Tucker
In-Reply-To: <cover.1491118230.git.guillaume.tucker-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>

Add reference to the Mali GPU device tree node on the
rk3288-rock2-som platform.  Tested on a Radxa Rock2 Square board.

Signed-off-by: Guillaume Tucker <guillaume.tucker-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 arch/arm/boot/dts/rk3288-rock2-som.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-rock2-som.dtsi b/arch/arm/boot/dts/rk3288-rock2-som.dtsi
index 1c0bbc9b928b..f694867fa46a 100644
--- a/arch/arm/boot/dts/rk3288-rock2-som.dtsi
+++ b/arch/arm/boot/dts/rk3288-rock2-som.dtsi
@@ -301,3 +301,8 @@
 &wdt {
 	status = "okay";
 };
+
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* [PATCH v2 4/5] ARM: dts: rockchip: enable ARM Mali GPU on rk3288-firefly
From: Guillaume Tucker @ 2017-04-02  7:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: devicetree, Guillaume Tucker, Sjoerd Simons, Wookey, linux-kernel,
	linux-rockchip, John Reitan, Enric Balletbo i Serra,
	linux-arm-kernel
In-Reply-To: <cover.1491118230.git.guillaume.tucker@collabora.com>

Add reference to the Mali GPU device tree node on rk3288-firefly.
Tested on Firefly board.

Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
---
 arch/arm/boot/dts/rk3288-firefly.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-firefly.dtsi b/arch/arm/boot/dts/rk3288-firefly.dtsi
index 10793ac18599..f520589493b4 100644
--- a/arch/arm/boot/dts/rk3288-firefly.dtsi
+++ b/arch/arm/boot/dts/rk3288-firefly.dtsi
@@ -594,3 +594,8 @@
 &wdt {
 	status = "okay";
 };
+
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
-- 
2.11.0

^ permalink raw reply related

* [PATCH v2 5/5] ARM: dts: rockchip: enable ARM Mali GPU on rk3288-veyron
From: Guillaume Tucker @ 2017-04-02  7:59 UTC (permalink / raw)
  To: Rob Herring, Mark Rutland
  Cc: Sjoerd Simons, Enric Balletbo i Serra, John Reitan, Wookey,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, Guillaume Tucker
In-Reply-To: <cover.1491118230.git.guillaume.tucker-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>

From: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>

Add reference to the Mali GPU device tree node on rk3288-veyron.
Tested on Minnie and Jerry boards.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
Signed-off-by: Guillaume Tucker <guillaume.tucker-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
---
 arch/arm/boot/dts/rk3288-veyron.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-veyron.dtsi b/arch/arm/boot/dts/rk3288-veyron.dtsi
index 5d1eb0a25827..9847d5c6db3b 100644
--- a/arch/arm/boot/dts/rk3288-veyron.dtsi
+++ b/arch/arm/boot/dts/rk3288-veyron.dtsi
@@ -447,6 +447,11 @@
 	status = "okay";
 };
 
+&gpu {
+	mali-supply = <&vdd_gpu>;
+	status = "okay";
+};
+
 &wdt {
 	status = "okay";
 };
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related

* Re: [PATCH v2 2/2] iio: adc: stm32: add dt option to set resolution
From: Jonathan Cameron @ 2017-04-02  8:54 UTC (permalink / raw)
  To: Fabrice Gasnier, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
	alexandre.torgue-qxv4g6HH51o, lars-Qo5EllUWu/uELgA04lAiVw,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A,
	benjamin.gaignard-qxv4g6HH51o,
	linus.walleij-QSEj5FYQhm4dnm+yROfE0A
In-Reply-To: <1490963559-32159-3-git-send-email-fabrice.gasnier-qxv4g6HH51o@public.gmane.org>

On 31/03/17 13:32, Fabrice Gasnier wrote:
> stm32 adc supports several resolution. Add 'assigned-resolution-bits'
> dt optional property to set it. Default to maximum resolution in case
> it isn't set.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
The code is fine.  Just waiting on review of the binding.

Give me a poke if I seem to have dropped this in a week or two.

Thanks,

Jonathan
> ---
> Changes in v2:
> - rename property from 'st,adc-res' to 'assigned-resolution-bits', so it
>   is more common.
> ---
>  drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 9b49a6ad..c28e7ff 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -60,6 +60,8 @@
>  #define STM32F4_EOC			BIT(1)
>  
>  /* STM32F4_ADC_CR1 - bit fields */
> +#define STM32F4_RES_SHIFT		24
> +#define STM32F4_RES_MASK		GENMASK(25, 24)
>  #define STM32F4_SCAN			BIT(8)
>  #define STM32F4_EOCIE			BIT(5)
>  
> @@ -141,6 +143,7 @@ struct stm32_adc_regs {
>   * @lock:		spinlock
>   * @bufi:		data buffer index
>   * @num_conv:		expected number of scan conversions
> + * @res:		data resolution (e.g. RES bitfield value)
>   * @trigger_polarity:	external trigger polarity (e.g. exten)
>   * @dma_chan:		dma channel
>   * @rx_buf:		dma rx buffer cpu address
> @@ -157,6 +160,7 @@ struct stm32_adc {
>  	spinlock_t		lock;		/* interrupt lock */
>  	unsigned int		bufi;
>  	unsigned int		num_conv;
> +	u32			res;
>  	u32			trigger_polarity;
>  	struct dma_chan		*dma_chan;
>  	u8			*rx_buf;
> @@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
>  	{ IIO_VOLTAGE, 15, "in15" },
>  };
>  
> +static const unsigned int stm32f4_adc_resolutions[] = {
> +	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
> +	12, 10, 8, 6,
> +};
> +
>  /**
>   * stm32f4_sq - describe regular sequence registers
>   * - L: sequence len (register & bit field)
> @@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>  	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
>  }
>  
> +static void stm32_adc_set_res(struct stm32_adc *adc)
> +{
> +	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
> +
> +	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
> +	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
> +}
> +
>  /**
>   * stm32_adc_start_conv() - Start conversions for regular channels.
>   * @adc: stm32 adc instance
> @@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>  	{},
>  };
>  
> +static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
> +{
> +	struct device_node *node = indio_dev->dev.of_node;
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	unsigned int i;
> +	u32 res;
> +
> +	if (of_property_read_u32(node, "assigned-resolution-bits", &res))
> +		res = stm32f4_adc_resolutions[0];
> +
> +	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
> +		if (res == stm32f4_adc_resolutions[i])
> +			break;
> +	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
> +		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
> +	adc->res = i;
> +
> +	return 0;
> +}
> +
>  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  				    struct iio_chan_spec *chan,
>  				    const struct stm32_adc_chan_spec *channel,
>  				    int scan_index)
>  {
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +
>  	chan->type = channel->type;
>  	chan->channel = channel->channel;
>  	chan->datasheet_name = channel->name;
> @@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>  	chan->scan_type.sign = 'u';
> -	chan->scan_type.realbits = 12;
> +	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
>  	chan->scan_type.storagebits = 16;
>  	chan->ext_info = stm32_adc_ext_info;
>  }
> @@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = stm32_adc_of_get_resolution(indio_dev);
> +	if (ret < 0)
> +		goto err_clk_disable;
> +	stm32_adc_set_res(adc);
> +
>  	ret = stm32_adc_chan_of_init(indio_dev);
>  	if (ret < 0)
>  		goto err_clk_disable;
> 

^ permalink raw reply

* Re: [PATCH v4 1/6] dt-bindings: iio: rockchip-saradc: add support for rk3328
From: Jonathan Cameron @ 2017-04-02  8:58 UTC (permalink / raw)
  To: Heiko Stuebner, cl-TNX95d0MmH7DzftRWevZcw
  Cc: robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	zhengxing-TNX95d0MmH7DzftRWevZcw, andy.yan-TNX95d0MmH7DzftRWevZcw,
	jay.xu-TNX95d0MmH7DzftRWevZcw,
	matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w,
	paweljarosz3691-Re5JQEeQqe8AvxtiuMwx3w,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA, wsa-z923LK4zBo2bacvFa/9K2g,
	linux-i2c-u79uwXL29TY76Z2rM5mHXA, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	wxt-TNX95d0MmH7DzftRWevZcw, david.wu-TNX95d0MmH7DzftRWevZcw,
	linux-iio-u79uwXL29TY76Z2rM5mHXA,
	shawn.lin-TNX95d0MmH7DzftRWevZcw,
	akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
	dianders-F7+t8E8rja9g9hUCZPvPmw,
	yamada.masahiro-uWyLwvC0a2jby3iVrkZq2A,
	catalin.marinas-5wv7dgnIgG8, will.deacon-5wv7dgnIgG8,
	afaerber-l3A5Bk7waGM, shawnguo-DgEjT+Ai2ygdnm+yROfE0A,
	khilman-rdvid1DuHRBWk0Htik3J/w, arnd-r2nGTMty4D4,
	fabio.estevam-3arQi8VN3Tc, zhangqing-TNX95d0MmH7DzftRWevZcw,
	kever.yang-TNX95d0MmH7DzftRWevZcw, tony.xie
In-Reply-To: <1940747.DLKBel529v@phil>

On 27/03/17 14:25, Heiko Stuebner wrote:
> Hi Jonathan,
> 
> Am Montag, 27. März 2017, 17:40:46 CEST schrieb cl-TNX95d0MmH7DzftRWevZcw@public.gmane.org:
>> From: Liang Chen <cl-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>>
>> The rk3328 saradc is the same as rk3399.
>>
>> Signed-off-by: Liang Chen <cl-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
>> Reviewed-by: Heiko Stuebner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
>> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> just for planning purposes, do you plan on picking this up or do you
> want me to queue it with the rest of the changes (Ack needed).

Applied to the togreg branch of iio.git.  Makes more sense that
way as we might get some other changes around that file and it
doesn't make any difference to the rest of the series.

Will probably start it's way upstream this afternoon if I get
through the rest of my backlog.

Thanks,

Jonathan
> 
> Heiko
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH 1/2] iio:imu:mpu6050 update i2c bindings to reflect i2c-gate and supported parts
From: Jonathan Cameron @ 2017-04-02  9:09 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mark.Rutland-5wv7dgnIgG8
In-Reply-To: <20170330230252.cxbdh3g744qwdtfh@rob-hp-laptop>

On 31/03/17 00:02, Rob Herring wrote:
> On Sun, Mar 26, 2017 at 12:10:59PM +0100, Jonathan Cameron wrote:
>> These bindings are somewhat lagging the state of the driver.  The i2c-gate
>> bindings are documented elsewhere, but it seems sensible to at least have
>> an example and cross reference in here.  SPI bindings will need to be the
>> subject of a future patch.
>>
>> Signed-off-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>>  .../devicetree/bindings/iio/imu/inv_mpu6050.txt    | 23 ++++++++++++++++++++--
>>  1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>> index a9fc11e43b45..e18e04f3c6f4 100644
>> --- a/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>> +++ b/Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt
>> @@ -3,14 +3,17 @@ InvenSense MPU-6050 Six-Axis (Gyro + Accelerometer) MEMS MotionTracking Device
>>  http://www.invensense.com/mems/gyro/mpu6050.html
>>  
>>  Required properties:
>> - - compatible : should be "invensense,mpu6050"
>> + - compatible : should be "invensense,mpu6050", "invensense,mpu6500",
>> +   "invensense,mpu9150" or "invensense,icm20608"
> 
> One per line please. With that,
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
Fixed and applied.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH 2/2] iio:imu:mpu6050 add explicit mpu9250 support
From: Jonathan Cameron @ 2017-04-02  9:12 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Mark.Rutland-5wv7dgnIgG8
In-Reply-To: <20170330231846.j45psrzast3pjc5b@rob-hp-laptop>

On 31/03/17 00:18, Rob Herring wrote:
> On Sun, Mar 26, 2017 at 12:11:00PM +0100, Jonathan Cameron wrote:
>> The mpu9250 is a SIP containing an mpu6500 and an ak8975.  If this was all
>> there was too it there would be no need for explicit handling in the driver.
>> Arguably the bindings would also only reflect the presence of an mpu6500 with
>> the ak8975 hanging off it, as the kernel doesn't care that they are in one
>> package.
>>
>> However, the WHOAMI value changes as well so best to add explicit support.
>>
>> Signed-off-by: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> ---
>>  Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt | 2 +-
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c                | 6 ++++++
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c                 | 5 +++++
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h                 | 2 ++
>>  drivers/iio/imu/inv_mpu6050/inv_mpu_spi.c                 | 1 +
>>  5 files changed, 15 insertions(+), 1 deletion(-)
> 
> Same one per line comment, but
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
Fixed up and applied.  Thanks Rob.

Jonathan

^ permalink raw reply

* Re: [PATCH v2] iio:adc: Driver for Linear Technology LTC2497 ADC
From: Jonathan Cameron @ 2017-04-02  9:30 UTC (permalink / raw)
  To: michael.hennerich-OyLXuOCK7orQT0dZR+AlfA,
	lars-Qo5EllUWu/uELgA04lAiVw, knaack.h-Mmb7MZpHnFY,
	pmeerw-jW+XmwGofnusTnJN9+BGXg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1490599433-6103-1-git-send-email-michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

On 27/03/17 08:23, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> This patch adds support for the Analog Devices / Linear Technology
> LTC2497 ADCs. The LTC2497 is a 16-channel (eight differential),
> 16-bit, high precision, delta-sigma ADC with an automatic, differential,
> input current cancellation front end and a 2-wire, I2C interface.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
Looks good to me.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Bindings are trivial so happy to take without pestering Rob or Mark.
(of course their comments are always welcome!)

Thanks,

Jonathan
> 
> ---
> 
> Changes since v1:
> 
>  - remove confusing kconfig help text
>  - use proper defines
>  - add descriptive comment
>  - fix removal order
>  - add poper commit mssage
> ---
>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/Kconfig                            |  10 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ltc2497.c                          | 269 +++++++++++++++++++++
>  5 files changed, 294 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>  create mode 100644 drivers/iio/adc/ltc2497.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> new file mode 100644
> index 0000000..c2829c19
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> @@ -0,0 +1,13 @@
> +* Linear Technology / Analog Devices LTC2497 ADC
> +
> +Required properties:
> + - compatible: Should be "lltc,ltc2497"
> + - reg: Should contain the ADC I2C address
> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Example:
> +	ltc2497: adc@76 {
> +		compatible = "lltc,ltc2497";
> +		reg = <0x76>;
> +		vref-supply = <&ltc2497_reg>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7d6f9a..173043c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
>  W:	http://ez.analog.com/community/linux-device-drivers
>  S:	Supported
>  F:	drivers/iio/*/ad*
> +F:	drivers/iio/adc/ltc2497*
>  X:	drivers/iio/*/adjd*
>  F:	drivers/staging/iio/*/ad*
>  F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6f..acc115b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -326,6 +326,16 @@ config LTC2485
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2485.
>  
> +config LTC2497
> +	tristate "Linear Technology LTC2497 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Linear Technology LTC2497
> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ltc2497.
> +
>  config MAX1027
>  	tristate "Maxim max1027 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe39..9d626b5 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
> +obj-$(CONFIG_LTC2497) += ltc2497.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> new file mode 100644
> index 0000000..7907534
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -0,0 +1,269 @@
> +/*
> + * ltc2497.c - Driver for Linear Technology LTC2497 ADC
> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/of.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define LTC2497_ENABLE			0xA0
> +#define LTC2497_SGL			(1 << 4)
> +#define LTC2497_DIFF			(0 << 4)
> +#define LTC2497_SIGN			(1 << 3)
> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> +#define LTC2497_CONVERSION_TIME_MS	150ULL
> +
> +struct ltc2497_st {
> +	struct i2c_client *client;
> +	struct regulator *ref;
> +	ktime_t	time_prev;
> +	u8 addr_prev;
> +};
> +
> +static int ltc2497_wait_conv(struct ltc2497_st *st)
> +{
> +	s64 time_elapsed;
> +
> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
> +
> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
> +		/* delay if conversion time not passed
> +		 * since last read or write
> +		 */
> +		msleep(LTC2497_CONVERSION_TIME_MS - time_elapsed);
> +		return 0;
> +	}
> +
> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
> +		/* We're in automatic mode -
> +		 * so the last reading is stil not outdated
> +		 */
> +		return 0;
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
> +{
> +	struct i2c_client *client = st->client;
> +	__be32 buf = 0;
> +	int ret;
> +
> +	ret = ltc2497_wait_conv(st);
> +	if (ret < 0 || st->addr_prev != address) {
> +		ret = i2c_smbus_write_byte(st->client,
> +					   LTC2497_ENABLE | address);
> +		if (ret < 0)
> +			return ret;
> +		st->addr_prev = address;
> +		msleep(LTC2497_CONVERSION_TIME_MS);
> +	}
> +	ret = i2c_master_recv(client, (char *)&buf, 3);
> +	if (ret < 0)  {
> +		dev_err(&client->dev, "i2c_master_recv failed\n");
> +		return ret;
> +	}
> +	st->time_prev = ktime_get();
> +
> +	/* convert and shift the result,
> +	 * and finally convert from offset binary to signed integer
> +	 */
> +	*val = (be32_to_cpu(buf) >> 14) - (1 << 17);
> +
> +	return ret;
> +}
> +
> +static int ltc2497_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		ret = ltc2497_read(st, chan->address, val);
> +		mutex_unlock(&indio_dev->mlock);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(st->ref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = 17;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#define LTC2497_CHAN(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan), \
> +	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +#define LTC2497_CHAN_DIFF(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
> +	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
> +	.address = (_addr | _chan), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.differential = 1, \
> +}
> +
> +static const struct iio_chan_spec ltc2497_channel[] = {
> +	LTC2497_CHAN(0, LTC2497_SGL),
> +	LTC2497_CHAN(1, LTC2497_SGL),
> +	LTC2497_CHAN(2, LTC2497_SGL),
> +	LTC2497_CHAN(3, LTC2497_SGL),
> +	LTC2497_CHAN(4, LTC2497_SGL),
> +	LTC2497_CHAN(5, LTC2497_SGL),
> +	LTC2497_CHAN(6, LTC2497_SGL),
> +	LTC2497_CHAN(7, LTC2497_SGL),
> +	LTC2497_CHAN(8, LTC2497_SGL),
> +	LTC2497_CHAN(9, LTC2497_SGL),
> +	LTC2497_CHAN(10, LTC2497_SGL),
> +	LTC2497_CHAN(11, LTC2497_SGL),
> +	LTC2497_CHAN(12, LTC2497_SGL),
> +	LTC2497_CHAN(13, LTC2497_SGL),
> +	LTC2497_CHAN(14, LTC2497_SGL),
> +	LTC2497_CHAN(15, LTC2497_SGL),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
> +};
> +
> +static const struct iio_info ltc2497_info = {
> +	.read_raw = ltc2497_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int ltc2497_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc2497_st *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_WRITE_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	st->client = client;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = id->name;
> +	indio_dev->info = &ltc2497_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2497_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
> +
> +	st->ref = devm_regulator_get(&client->dev, "vref");
> +	if (IS_ERR(st->ref))
> +		return PTR_ERR(st->ref);
> +
> +	ret = regulator_enable(st->ref);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	st->addr_prev = LTC2497_CONFIG_DEFAULT;
> +	st->time_prev = ktime_get();
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	return 0;
> +
> +err_regulator_disable:
> +	regulator_disable(st->ref);
> +
> +	return ret;
> +}
> +
> +static int ltc2497_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	regulator_disable(st->ref);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ltc2497_id[] = {
> +	{ "ltc2497", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2497_id);
> +
> +static const struct of_device_id ltc2497_of_match[] = {
> +	{ .compatible = "lltc,ltc2497", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ltc2497_of_match);
> +
> +static struct i2c_driver ltc2497_driver = {
> +	.driver = {
> +		.name = "ltc2497",
> +		.of_match_table = of_match_ptr(ltc2497_of_match),
> +	},
> +	.probe = ltc2497_probe,
> +	.remove = ltc2497_remove,
> +	.id_table = ltc2497_id,
> +};
> +module_i2c_driver(ltc2497_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v4 1/4] Documentation: dt-bindings: iio: Add max9611 ADC
From: Jonathan Cameron @ 2017-04-02  9:35 UTC (permalink / raw)
  To: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
	knaack.h, lars, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, linux-renesas-soc, devicetree
In-Reply-To: <1490627578-15938-2-git-send-email-jacopo+renesas@jmondi.org>

On 27/03/17 16:12, Jacopo Mondi wrote:
> Add device tree bindings documentation for Maxim max9611/max9612 current
> sense amplifier.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../devicetree/bindings/iio/adc/max9611.txt        | 26 ++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max9611.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max9611.txt b/Documentation/devicetree/bindings/iio/adc/max9611.txt
> new file mode 100644
> index 0000000..8430d3b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max9611.txt
> @@ -0,0 +1,26 @@
> +* Maxim max9611/max9612 current sense amplifier with 12-bits ADC interface
> +
> +Maxim max9611/max9612 is an high-side current sense amplifier with integrated
> +12-bits ADC communicating over I2c bus.
> +The device node for this driver shall be a child of a I2c controller.
> +
> +Required properties
> +  - compatible: Should be "maxim,max9611" or "maxim,max9612"
> +  - reg: The 7-bits long I2c address of the device
> +  - shunt-resistor-uohm: Value, in uOhm, of the current sense shunt resistor
Just did a quick grep for this see if I felt comfortable enough to
not wait for Rob / Mark to have a chance to look at it.
Grep gave me:
shunt-resistor-micro-ohms for the ina2xx.
Feels rather like we should match that.

Otherwise looks good to me.

Jonathan
> +
> +Example:
> +
> +&i2c4 {
> +	csa: adc@7c {
> +		compatible = "maxim,max9611";
> +		reg = <0x7c>;
> +
> +		shunt-resistor-uohm = <5000>;
> +	};
> +};
> +
> +This device node describes a current sense amplifier sitting on I2c4 bus
> +with address 0x7c (read address is 0xf9, write address is 0xf8).
> +A sense resistor of 0,005 Ohm is installed between RS+ and RS- current-sensing
> +inputs.
> 

^ permalink raw reply

* Re: [PATCH v4 2/4] iio: Documentation: Add max9611 sysfs documentation
From: Jonathan Cameron @ 2017-04-02  9:36 UTC (permalink / raw)
  To: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
	knaack.h, lars, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, linux-renesas-soc, devicetree
In-Reply-To: <1490627578-15938-3-git-send-email-jacopo+renesas@jmondi.org>

On 27/03/17 16:12, Jacopo Mondi wrote:
> Add documentation for max9611 driver.
> Document attributes describing value of shunt resistor installed between
> RS+ and RS- voltage sense inputs.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Good.  Will pick up once the binding is sorted.

Kick me if I don't!
> ---
>  Documentation/ABI/testing/sysfs-bus-iio-adc-max9611 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max9611
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max9611 b/Documentation/ABI/testing/sysfs-bus-iio-adc-max9611
> new file mode 100644
> index 0000000..5020d00
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max9611
> @@ -0,0 +1,17 @@
> +What:		/sys/bus/iio/devices/iio:deviceX/in_power_shunt_resistor
> +Date:		March 2017
> +KernelVersion:	4.12
> +Contact:	linux-iio@vger.kernel.org
> +Description: 	The value of the shunt resistor used to compute power drain on
> +                common input voltage pin (RS+). In micro Ohms.
> +
> +What:		/sys/bus/iio/devices/iio:deviceX/in_current_shunt_resistor
> +Date:		March 2017
> +KernelVersion:	4.12
> +Contact:	linux-iio@vger.kernel.org
> +Description: 	The value of the shunt resistor used to compute current flowing
> +                between RS+ and RS- voltage sense inputs. In micro Ohms.
> +
> +These attributes describe a single physical component, exposed as two distinct
> +attributes as it is used to calculate two different values: power load and
> +current flowing between RS+ and RS- inputs.
> 

^ permalink raw reply

* Re: [PATCH v4 3/4] iio: adc: Add Maxim max9611 ADC driver
From: Jonathan Cameron @ 2017-04-02  9:43 UTC (permalink / raw)
  To: Jacopo Mondi, geert-Td1EMuHUCqxL1ZNQvxDV9g,
	wsa+renesas-jBu1N2QxHDJrcw3mvpCnnVaTQe2KTcn/,
	magnus.damm-Re5JQEeQqe8AvxtiuMwx3w,
	laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw, knaack.h-Mmb7MZpHnFY,
	lars-Qo5EllUWu/uELgA04lAiVw, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	linux-renesas-soc-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1490627578-15938-4-git-send-email-jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>

On 27/03/17 16:12, Jacopo Mondi wrote:
> Add iio driver for Maxim max9611 and max9612 current-sense amplifiers
> with 12-bits ADC interface.
> 
> Datasheet publicly available at:
> https://datasheets.maximintegrated.com/en/ds/MAX9611-MAX9612.pdf
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>
>
couple of bits inline.  I'd failed to pick up on the shunt resistor units
before.  To be consistent with the rest of the ABI, those exposed via
sysfs ought to be in ohms.  It's also in ohms in the ina2xx driver.

Thanks,

Jonathan
> ---
> 
> Output of iio_info on Salvator-X board for max9611 chip installed on
> VDD_0.8 lines.
> The VDD_0.8 line powers the CPU cluster and on-board RAM.
> 
>         iio:device0: max9611
>                 5 channels found:
>                         voltage0:  (input)
>                         1 channel-specific attributes found:
>                                 attr 0: input value: 4.085000000
>                         voltage1:  (input)
>                         3 channel-specific attributes found:
>                                 attr 0: scale value: 14
>                                 attr 1: offset value: 1
>                                 attr 2: raw value: 59
>                         power:  (input)
>                         2 channel-specific attributes found:
>                                 attr 0: shunt_resistor value: 5000
>                                 attr 1: input value: 663.404000000
>                         current:  (input)
>                         2 channel-specific attributes found:
>                                 attr 0: shunt_resistor value: 5000
>                                 attr 1: input value: 817.000000000
>                         temp:  (input)
>                         2 channel-specific attributes found:
>                                 attr 0: scale value: 480.076812289
>                                 attr 1: raw value: 57
> 
> The collected information represent:
> 
> * voltage0 (current sense voltage) Vcsa
>   voltage drop between RS+ and RS- input = 4,085 mV
> * voltage1: (common input voltage) Vcim
>   voltage at RS+ input = (59 - 1) * 14 = 812 mV
> * current flowing on shunt resistor (Icsa)
>   = Vcsa / Rshunt = 817 mA
> * power load on the sensed line (Pload)
>   = Vcim * Icsa = 663 m
> * die temperature = (57 * 480.07) = 27360 milli Celsius
> ---
>  drivers/iio/adc/Kconfig   |  10 +
>  drivers/iio/adc/Makefile  |   1 +
>  drivers/iio/adc/max9611.c | 583 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 594 insertions(+)
>  create mode 100644 drivers/iio/adc/max9611.c
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7a..82f2e7b8 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -354,6 +354,16 @@ config MAX1363
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max1363.
>  
> +config	MAX9611
> +	tristate "Maxim max9611/max9612 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Maxim max9611/max9612 current sense
> +	  amplifier with 12-bits ADC interface.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max9611.
> +
>  config MCP320X
>  	tristate "Microchip Technology MCP3x01/02/04/08"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d001262..149f979 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -34,6 +34,7 @@ obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> +obj-$(CONFIG_MAX9611) += max9611.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
>  obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o
> diff --git a/drivers/iio/adc/max9611.c b/drivers/iio/adc/max9611.c
> new file mode 100644
> index 0000000..e5aaf73
> --- /dev/null
> +++ b/drivers/iio/adc/max9611.c
> @@ -0,0 +1,583 @@
> +/*
> + * iio/adc/max9611.c
> + *
> + * Maxim max9611/max9612 high side current sense amplifier with
> + * 12-bit ADC interface.
> + *
> + * Copyright (C) 2017 Jacopo Mondi
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +/*
> + * This driver supports input common-mode voltage, current-sense
> + * amplifier with programmable gains and die temperature reading from
> + * Maxim max9611/max9612.
> + *
> + * Op-amp, analog comparator, and watchdog functionalities are not
> + * supported by this driver.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +
> +#define DRIVER_NAME 			"max9611"
> +#define MAX9611_DEV_NAME		"max9611"
> +#define MAX9612_DEV_NAME		"max9612"
Really trivial, but what do these defines give us other than
making the code slightly harder to review?

Just put them inline where they are used.
> +
> +/* max9611 register addresses */
> +#define MAX9611_REG_CSA_DATA		0x00
> +#define MAX9611_REG_RS_DATA		0x02
> +#define MAX9611_REG_TEMP_DATA		0x08
> +#define MAX9611_REG_CTRL1		0x0a
> +#define MAX9611_REG_CTRL2		0x0b
> +
> +/* max9611 REG1 mux configuration options */
> +#define MAX9611_MUX_MASK		GENMASK(3,0)
> +#define MAX9611_MUX_SENSE_1x		0x00
> +#define MAX9611_MUX_SENSE_4x		0x01
> +#define MAX9611_MUX_SENSE_8x		0x02
> +#define MAX9611_INPUT_VOLT		0x03
> +#define MAX9611_MUX_TEMP		0x06
> +
> +/* max9611 voltage (both csa and input) helper macros */
> +#define MAX9611_VOLTAGE_SHIFT		0x04
> +#define MAX9611_VOLTAGE_RAW(_r)		((_r) >> MAX9611_VOLTAGE_SHIFT)
> +
> +/*
> + * max9611 current sense amplifier voltage output:
> + * LSB and offset values depends on selected gain (1x, 4x, 8x)
> + *
> + * GAIN		LSB (nV)	OFFSET (LSB steps)
> + * 1x		107500		1
> + * 4x		26880		1
> + * 8x		13440		3
> + *
> + * The complete formula to calculate current sense voltage is:
> + *     (((adc_read >> 4) - offset) / ((1 / LSB) * 10^-3)
> + */
> +#define MAX9611_CSA_1X_LSB_nV		107500
> +#define MAX9611_CSA_4X_LSB_nV		26880
> +#define MAX9611_CSA_8X_LSB_nV		13440
> +
> +#define MAX9611_CSA_1X_OFFS_RAW		1
> +#define MAX9611_CSA_4X_OFFS_RAW		1
> +#define MAX9611_CSA_8X_OFFS_RAW		3
> +
> +/*
> + * max9611 common input mode (CIM): LSB is 14mV, with 14mV offset at 25 C
> + *
> + * The complete formula to calculate input common voltage is:
> + *     (((adc_read >> 4) * 1000) - offset) / (1 / 14 * 1000)
> + */
> +#define MAX9611_CIM_LSB_mV		14
> +#define MAX9611_CIM_OFFSET_RAW		1
> +
> +/*
> + * max9611 temperature reading: LSB is 480 milli degrees Celsius
> + *
> + * The complete formula to calculate temperature is:
> + *     ((adc_read >> 7) * 1000) / (1 / 480 * 1000)
> + */
> +#define MAX9611_TEMP_MAX_POS		0x7f80
> +#define MAX9611_TEMP_MAX_NEG		0xff80
> +#define MAX9611_TEMP_MIN_NEG		0xd980
> +#define MAX9611_TEMP_MASK		GENMASK(7, 15)
> +#define MAX9611_TEMP_SHIFT		0x07
> +#define MAX9611_TEMP_RAW(_r)		((_r) >> MAX9611_TEMP_SHIFT)
> +#define MAX9611_TEMP_SCALE_NUM		1000000
> +#define MAX9611_TEMP_SCALE_DIV		2083
> +
> +struct max9611_dev {
> +	struct device *dev;
> +	struct i2c_client *i2c_client;
> +	struct mutex lock;
> +	unsigned int shunt_resistor_uohm;
> +};
> +
> +enum max9611_conf_ids {
> +	CONF_SENSE_1x,
> +	CONF_SENSE_4x,
> +	CONF_SENSE_8x,
> +	CONF_IN_VOLT,
> +	CONF_TEMP,
> +};
> +
> +/**
> + * max9611_mux_conf - associate ADC mux configuration with register address
> + *		      where data shall be read from
> + */
> +static const unsigned int max9611_mux_conf[][2] = {
> +	/* CONF_SENSE_1x */
> +	{ MAX9611_MUX_SENSE_1x, MAX9611_REG_CSA_DATA },
> +	/* CONF_SENSE_4x */
> +	{ MAX9611_MUX_SENSE_4x, MAX9611_REG_CSA_DATA },
> +	/* CONF_SENSE_8x */
> +	{ MAX9611_MUX_SENSE_8x, MAX9611_REG_CSA_DATA },
> +	/* CONF_IN_VOLT */
> +	{ MAX9611_INPUT_VOLT, MAX9611_REG_RS_DATA },
> +	/* CONF_TEMP */
> +	{ MAX9611_MUX_TEMP, MAX9611_REG_TEMP_DATA },
> +};
> +
> +enum max9611_csa_gain {
> +	CSA_GAIN_1x,
> +	CSA_GAIN_4x,
> +	CSA_GAIN_8x,
> +};
> +
> +enum max9611_csa_gain_params {
> +	CSA_GAIN_LSB_nV,
> +	CSA_GAIN_OFFS_RAW,
> +};
> +
> +/**
> + * max9611_csa_gain_conf - associate gain multiplier with LSB and
> + *			   offset values.
> + *
> + * Group together parameters associated with configurable gain
> + * on current sense amplifier path to ADC interface.
> + * Current sense read routine adjusts gain until it gets a meaningful
> + * value; use this structure to retrieve the correct LSB and offset values.
> + */
> +static const unsigned int max9611_gain_conf[][2] = {
> +	{ /* [0] CSA_GAIN_1x */
> +		MAX9611_CSA_1X_LSB_nV,
> +		MAX9611_CSA_1X_OFFS_RAW,
> +	},
> +	{ /* [1] CSA_GAIN_4x */
> +		MAX9611_CSA_4X_LSB_nV,
> +		MAX9611_CSA_4X_OFFS_RAW,
> +	},
> +	{ /* [2] CSA_GAIN_8x */
> +		MAX9611_CSA_8X_LSB_nV,
> +		MAX9611_CSA_8X_OFFS_RAW,
> +	},
> +};
> +
> +enum max9611_chan_addrs {
> +	MAX9611_CHAN_VOLTAGE_INPUT,
> +	MAX9611_CHAN_VOLTAGE_SENSE,
> +	MAX9611_CHAN_TEMPERATURE,
> +	MAX9611_CHAN_CURRENT_LOAD,
> +	MAX9611_CHAN_POWER_LOAD,
> +};
> +
> +static const struct iio_chan_spec max9611_channels[] = {
> +	{
> +	  .type			= IIO_TEMP,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW) |
> +				  BIT(IIO_CHAN_INFO_SCALE),
> +	  .address		= MAX9611_CHAN_TEMPERATURE,
> +	},
> +	{
> +	  .type			= IIO_VOLTAGE,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_PROCESSED),
> +	  .address		= MAX9611_CHAN_VOLTAGE_SENSE,
> +	  .indexed		= 1,
> +	  .channel		= 0,
> +	},
> +	{
> +	  .type			= IIO_VOLTAGE,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_RAW)   |
> +				  BIT(IIO_CHAN_INFO_SCALE) |
> +				  BIT(IIO_CHAN_INFO_OFFSET),
> +	  .address		= MAX9611_CHAN_VOLTAGE_INPUT,
> +	  .indexed		= 1,
> +	  .channel		= 1,
> +	},
> +	{
> +	  .type			= IIO_CURRENT,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_PROCESSED),
> +	  .address		= MAX9611_CHAN_CURRENT_LOAD,
> +	},
> +	{
> +	  .type			= IIO_POWER,
> +	  .info_mask_separate	= BIT(IIO_CHAN_INFO_PROCESSED),
> +	  .address		= MAX9611_CHAN_POWER_LOAD
> +	},
> +};
> +
> +/**
> + * max9611_read_single() - read a single value from ADC interface
> + *
> + * Data registers are 16 bit long, spread between two 8 bit registers
> + * with consecutive addresses.
> + * Configure ADC mux first, then read register at address "reg_addr".
> + * The smbus_read_word routine asks for 16 bits and the ADC is kind enough
> + * to return values from "reg_addr" and "reg_addr + 1" consecutively.
> + * Data are transmitted with big-endian ordering: MSB arrives first.
> + *
> + * @max9611: max9611 device
> + * @selector: index for mux and register configuration
> + * @raw_val: the value returned from ADC
> + */
> +static int max9611_read_single(struct max9611_dev *max9611,
> +			       enum max9611_conf_ids selector,
> +			       u16 *raw_val)
> +{
> +	int ret;
> +
> +	u8 mux_conf = max9611_mux_conf[selector][0] & MAX9611_MUX_MASK;
> +	u8 reg_addr = max9611_mux_conf[selector][1];
> +
> +	/*
> +	 * Keep mutex lock held during read-write to avoid mux register
> +	 * (CTRL1) re-configuration.
> +	 */
> +	mutex_lock(&max9611->lock);
> +	ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> +					MAX9611_REG_CTRL1, mux_conf);
> +	if (ret) {
> +		dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> +			MAX9611_REG_CTRL1, mux_conf);
> +		mutex_unlock(&max9611->lock);
> +		return ret;
> +	}
> +
> +	/*
> +	 * need a delay here to make register configuration
> +	 * stabilize. 1 msec at least, from empirical testing.
> +	 */
> +	usleep_range(1000, 2000);
> +
> +	ret = i2c_smbus_read_word_swapped(max9611->i2c_client, reg_addr);
> +	if (ret < 0) {
> +		dev_err(max9611->dev, "i2c read word from 0x%2x failed\n",
> +			reg_addr);
> +		mutex_unlock(&max9611->lock);
> +		return ret;
> +	}
> +
> +	*raw_val = ret;
> +	mutex_unlock(&max9611->lock);
> +
> +	return 0;
> +}
> +
> +/**
> + * max9611_read_csa_voltage() - read current sense amplifier output voltage
> + *
> + * Current sense amplifier output voltage is read through a configurable
> + * 1x, 4x or 8x gain.
> + * Start with plain 1x gain, and adjust gain control properly until a
> + * meaningful value is read from ADC output.
> + *
> + * @max9611: max9611 device
> + * @adc_raw: raw value read from ADC output
> + * @csa_gain: gain configuration option selector
> + */
> +static int max9611_read_csa_voltage(struct max9611_dev *max9611,
> +				    u16 *adc_raw,
> +				    enum max9611_csa_gain *csa_gain)
> +{
> +	enum max9611_conf_ids gain_selectors[] = {
> +		CONF_SENSE_1x,
> +		CONF_SENSE_4x,
> +		CONF_SENSE_8x
> +	};
> +	unsigned int i;
> +	int ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(gain_selectors); ++i) {
> +		ret = max9611_read_single(max9611, gain_selectors[i], adc_raw);
> +		if (ret)
> +			return ret;
> +
> +		if (*adc_raw > 0) {
> +			*csa_gain = gain_selectors[i];
> +			return 0;
> +		}
> +	}
> +
> +	return -EIO;
> +}
> +
> +static int max9611_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct max9611_dev *dev = iio_priv(indio_dev);
> +	enum max9611_csa_gain gain_selector;
> +	const unsigned int *csa_gain;
> +	u16 adc_data;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +
> +		switch (chan->address) {
> +		case MAX9611_CHAN_TEMPERATURE:
> +			ret = max9611_read_single(dev, CONF_TEMP,
> +						  &adc_data);
> +			if (ret)
> +				return -EINVAL;
> +
> +			*val = MAX9611_TEMP_RAW(adc_data);
> +			return IIO_VAL_INT;
> +
> +		case MAX9611_CHAN_VOLTAGE_INPUT:
> +			ret = max9611_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +			if (ret)
> +				return -EINVAL;
> +
> +			*val = MAX9611_VOLTAGE_RAW(adc_data);
> +			return IIO_VAL_INT;
> +		}
> +
> +		break;
> +
> +	case IIO_CHAN_INFO_OFFSET:
> +		/* MAX9611_CHAN_VOLTAGE_INPUT */
> +		*val = MAX9611_CIM_OFFSET_RAW;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +
> +		switch (chan->address) {
> +		case MAX9611_CHAN_TEMPERATURE:
> +			*val = MAX9611_TEMP_SCALE_NUM;
> +			*val2 = MAX9611_TEMP_SCALE_DIV;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX9611_CHAN_VOLTAGE_INPUT:
> +			*val = MAX9611_CIM_LSB_mV;
> +
> +			return IIO_VAL_INT;
> +		}
> +
> +		break;
> +
> +	case IIO_CHAN_INFO_PROCESSED:
> +
> +		switch (chan->address) {
> +		case MAX9611_CHAN_VOLTAGE_SENSE:
> +			/*
> +			 * processed (mV): (raw - offset) * LSB (nV) / 10^6
> +			 *
> +			 * Even if max9611 can output raw csa voltage readings,
> +			 * use a produced value as scale depends on gain.
> +			 */
> +			ret = max9611_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return -EINVAL;
> +
> +			csa_gain = max9611_gain_conf[gain_selector];
> +
> +			adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> +			*val = MAX9611_VOLTAGE_RAW(adc_data) *
> +			       csa_gain[CSA_GAIN_LSB_nV];
> +			*val2 = 1000000;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX9611_CHAN_CURRENT_LOAD:
> +			/* processed (mA): Vcsa (nV) / Rshunt (uOhm)  */
> +			ret = max9611_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return -EINVAL;
> +
> +			csa_gain = max9611_gain_conf[gain_selector];
> +
> +			adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> +			*val = MAX9611_VOLTAGE_RAW(adc_data) *
> +			       csa_gain[CSA_GAIN_LSB_nV];
> +			*val2 = dev->shunt_resistor_uohm;
> +
> +			return IIO_VAL_FRACTIONAL;
> +
> +		case MAX9611_CHAN_POWER_LOAD:
> +			/*
> +			 * processed (mW): Vin (mV) * Vcsa (uV) /
> +			 *		   Rshunt (uOhm)
> +			 */
> +			ret = max9611_read_single(dev, CONF_IN_VOLT,
> +						  &adc_data);
> +			if (ret)
> +				return -EINVAL;
> +
> +			adc_data -= MAX9611_CIM_OFFSET_RAW;
> +			*val = MAX9611_VOLTAGE_RAW(adc_data) *
> +			       MAX9611_CIM_LSB_mV;
> +
> +			ret = max9611_read_csa_voltage(dev, &adc_data,
> +						       &gain_selector);
> +			if (ret)
> +				return -EINVAL;
> +
> +			csa_gain = max9611_gain_conf[gain_selector];
> +
> +			/* divide by 10^3 here to avoid 32bit overflow */
> +			adc_data -= csa_gain[CSA_GAIN_OFFS_RAW];
> +			*val *= MAX9611_VOLTAGE_RAW(adc_data) *
> +				csa_gain[CSA_GAIN_LSB_nV] / 1000;
> +			*val2 = dev->shunt_resistor_uohm;
> +
> +			return IIO_VAL_FRACTIONAL;
> +		}
> +
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t max9611_shunt_resistor_show(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct max9611_dev *max9611 = iio_priv(dev_to_iio_dev(dev));
> +
> +	return sprintf(buf, "%u\n", max9611->shunt_resistor_uohm);
Oops didn't pick up on this before.  Should really be ohms with
appropriate fixed point arithmetic.

This would match with the use of ohms for DPOT devices that we already
have documented.
> +}
> +
> +static IIO_DEVICE_ATTR(in_power_shunt_resistor, 0444,
> +		       max9611_shunt_resistor_show, NULL, 0);
> +static IIO_DEVICE_ATTR(in_current_shunt_resistor, 0444,
> +		       max9611_shunt_resistor_show, NULL, 0);
> +
> +static struct attribute *max9611_attributes[] = {
> +	&iio_dev_attr_in_power_shunt_resistor.dev_attr.attr,
> +	&iio_dev_attr_in_current_shunt_resistor.dev_attr.attr,
> +	NULL,
> +};
> +
> +static const struct attribute_group max9611_attribute_group = {
> +	.attrs = max9611_attributes,
> +};
> +
> +static const struct iio_info indio_info = {
> +	.driver_module	= THIS_MODULE,
> +	.read_raw	= max9611_read_raw,
> +	.attrs		= &max9611_attribute_group,
> +};
> +
> +static int max9611_init(struct max9611_dev *max9611)
> +{
> +	struct i2c_client *client = max9611->i2c_client;
> +	u16 regval;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_WRITE_BYTE	|
> +				     I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> +		dev_err(max9611->dev,
> +			"I2c adapter does not support smbus write_byte or read_word functionalities: aborting probe.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* Make sure die temperature is in range to test communications. */
> +	ret = max9611_read_single(max9611, CONF_TEMP, &regval);
> +	if (ret)
> +		return ret;
> +
> +	regval = ret & MAX9611_TEMP_MASK;
> +
> +	if ((regval > MAX9611_TEMP_MAX_POS &&
> +	     regval < MAX9611_TEMP_MIN_NEG) ||
> +	     regval > MAX9611_TEMP_MAX_NEG) {
> +		dev_err(max9611->dev,
> +			"Invalid value received from ADC 0x%4x: aborting\n",
> +			regval);
> +		return -EIO;
> +	}
> +
> +	/* Mux shall be zeroed back before applying other configurations */
> +	ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> +					MAX9611_REG_CTRL1, 0);
> +	if (ret) {
> +		dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> +			MAX9611_REG_CTRL1, 0);
> +		return ret;
> +	}
> +
> +	ret = i2c_smbus_write_byte_data(max9611->i2c_client,
> +					MAX9611_REG_CTRL2, 0);
> +	if (ret) {
> +		dev_err(max9611->dev, "i2c write byte failed: 0x%2x - 0x%2x\n",
> +			MAX9611_REG_CTRL2, 0);
> +		return ret;
> +	}
> +	usleep_range(1000, 2000);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id max9611_of_table[] = {
> +	{.compatible = "maxim,max9611", .data = MAX9611_DEV_NAME},
> +	{.compatible = "maxim,max9612", .data = MAX9612_DEV_NAME},
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, max9611_of_table);
> +static int max9611_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	const char * const shunt_res_prop = "shunt-resistor-uohm";
> +	const struct device_node *of_node = client->dev.of_node;
> +	const struct of_device_id *of_id =
> +		of_match_device(max9611_of_table, &client->dev);
> +	struct max9611_dev *max9611;
> +	struct iio_dev *indio_dev;
> +	unsigned int of_shunt;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*max9611));
> +	if (IS_ERR(indio_dev))
> +		return PTR_ERR(indio_dev);
> +
> +	i2c_set_clientdata(client, indio_dev);
> +
> +	max9611			= iio_priv(indio_dev);
> +	max9611->dev		= &client->dev;
> +	max9611->i2c_client	= client;
> +	mutex_init(&max9611->lock);
> +
> +	ret = of_property_read_u32(of_node, shunt_res_prop, &of_shunt);
> +	if (ret) {
> +		dev_err(&client->dev,
> +			"Missing %s property for %s node\n",
> +			shunt_res_prop, of_node->full_name);
> +		return ret;
> +	}
> +	max9611->shunt_resistor_uohm = of_shunt;
> +
> +	ret = max9611_init(max9611);
> +	if (ret)
> +		return ret;
> +
> +	indio_dev->dev.parent	= &client->dev;
> +	indio_dev->dev.of_node	= client->dev.of_node;
> +	indio_dev->name		= of_id->data;
> +	indio_dev->modes	= INDIO_DIRECT_MODE;
> +	indio_dev->info		= &indio_info;
> +	indio_dev->channels	= max9611_channels;
> +	indio_dev->num_channels	= ARRAY_SIZE(max9611_channels);
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static struct i2c_driver max9611_driver = {
> +	.driver = {
> +		   .name = DRIVER_NAME,
> +		   .owner = THIS_MODULE,
> +		   .of_match_table = max9611_of_table,
> +	},
> +	.probe = max9611_probe,
> +};
> +module_i2c_driver(max9611_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas-AW8dsiIh9cEdnm+yROfE0A@public.gmane.org>");
> +MODULE_DESCRIPTION("Maxim max9611/12 current sense amplifier with 12bit ADC");
> +MODULE_LICENSE("GPL v2");
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2] iio:adc: Driver for Linear Technology LTC2497 ADC
From: Lars-Peter Clausen @ 2017-04-02  9:52 UTC (permalink / raw)
  To: Jonathan Cameron, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <3d54516e-cbd1-8a61-0d83-a91d7055f44c-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

On 04/02/2017 11:30 AM, Jonathan Cameron wrote:
> On 27/03/17 08:23, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
>> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>
>> This patch adds support for the Analog Devices / Linear Technology
>> LTC2497 ADCs. The LTC2497 is a 16-channel (eight differential),
>> 16-bit, high precision, delta-sigma ADC with an automatic, differential,
>> input current cancellation front end and a 2-wire, I2C interface.
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> Looks good to me.
> 
> Applied to the togreg branch of iio.git and pushed out as testing for
> the autobuilders to play with it.

There is a v3 of this patch.

^ permalink raw reply

* Re: [PATCH v4 2/4] iio: Documentation: Add max9611 sysfs documentation
From: Jonathan Cameron @ 2017-04-02  9:53 UTC (permalink / raw)
  To: Jacopo Mondi, geert, wsa+renesas, magnus.damm, laurent.pinchart,
	knaack.h, lars, pmeerw, robh+dt, mark.rutland
  Cc: linux-iio, linux-renesas-soc, devicetree
In-Reply-To: <6b249f47-2a79-e9a0-0599-284301319a9d@kernel.org>

On 02/04/17 10:36, Jonathan Cameron wrote:
> On 27/03/17 16:12, Jacopo Mondi wrote:
>> Add documentation for max9611 driver.
>> Document attributes describing value of shunt resistor installed between
>> RS+ and RS- voltage sense inputs.
>>
>> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Good.  Will pick up once the binding is sorted.
> 
> Kick me if I don't!
Oops. Missed the units being micro ohms.  They want to be ohms to be consistent
within the ABI.

We let various odd units go in right back at the beginning to try and remain
compatible with hwmon.  Turned out to be a bad idea.  Also the ina2xx uses
ohms and you should stay consistent with that.

Thanks,

Jonathan
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio-adc-max9611 | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-adc-max9611
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-adc-max9611 b/Documentation/ABI/testing/sysfs-bus-iio-adc-max9611
>> new file mode 100644
>> index 0000000..5020d00
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-adc-max9611
>> @@ -0,0 +1,17 @@
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_power_shunt_resistor
>> +Date:		March 2017
>> +KernelVersion:	4.12
>> +Contact:	linux-iio@vger.kernel.org
>> +Description: 	The value of the shunt resistor used to compute power drain on
>> +                common input voltage pin (RS+). In micro Ohms.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_current_shunt_resistor
>> +Date:		March 2017
>> +KernelVersion:	4.12
>> +Contact:	linux-iio@vger.kernel.org
>> +Description: 	The value of the shunt resistor used to compute current flowing
>> +                between RS+ and RS- voltage sense inputs. In micro Ohms.
>> +
>> +These attributes describe a single physical component, exposed as two distinct
>> +attributes as it is used to calculate two different values: power load and
>> +current flowing between RS+ and RS- inputs.
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

^ permalink raw reply

* Re: [PATCH v2] iio:adc: Driver for Linear Technology LTC2497 ADC
From: Jonathan Cameron @ 2017-04-02  9:54 UTC (permalink / raw)
  To: Lars-Peter Clausen, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <f0229cde-15b6-d45b-6d85-3c90082d0226-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>

On 02/04/17 10:52, Lars-Peter Clausen wrote:
> On 04/02/2017 11:30 AM, Jonathan Cameron wrote:
>> On 27/03/17 08:23, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
>>> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>>>
>>> This patch adds support for the Analog Devices / Linear Technology
>>> LTC2497 ADCs. The LTC2497 is a 16-channel (eight differential),
>>> 16-bit, high precision, delta-sigma ADC with an automatic, differential,
>>> input current cancellation front end and a 2-wire, I2C interface.
>>>
>>> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
>> Looks good to me.
>>
>> Applied to the togreg branch of iio.git and pushed out as testing for
>> the autobuilders to play with it.
> 
> There is a v3 of this patch.
> 
unapplied.  Thanks Lars!

^ permalink raw reply

* Re: [PATCH v3] iio:adc: Driver for Linear Technology LTC2497 ADC
From: Jonathan Cameron @ 2017-04-02  9:58 UTC (permalink / raw)
  To: michael.hennerich-OyLXuOCK7orQT0dZR+AlfA,
	lars-Qo5EllUWu/uELgA04lAiVw, knaack.h-Mmb7MZpHnFY,
	pmeerw-jW+XmwGofnusTnJN9+BGXg, robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	mark.rutland-5wv7dgnIgG8
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1490780578-2468-1-git-send-email-michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>

On 29/03/17 10:42, michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org wrote:
> From: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
> 
> This patch adds support for the Analog Devices / Linear Technology
> LTC2497 ADCs. The LTC2497 is a 16-channel (eight differential),
> 16-bit, high precision, delta-sigma ADC with an automatic, differential,
> input current cancellation front end and a 2-wire, I2C interface.
> 
> Signed-off-by: Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>
oops. Picked up the wrong version (thanks Lars). Dropped v2 and 
now applied v3.

Sorry about that

Jonathan
> 
> ---
> 
> Changes since v1:
> 
>  - remove confusing kconfig help text
>  - use proper defines
>  - add descriptive comment
>  - fix removal order
>  - add poper commit mssage
> 
> Changes since v2:
> 
>  - Sort includes
>  - Use BIT() defines
>  - Avoid transfer buffers on the stack
>  - use msleep_interruptible()
>  - Fix documentation s/Should/Must/g
> ---
>  .../devicetree/bindings/iio/adc/ltc2497.txt        |  13 +
>  MAINTAINERS                                        |   1 +
>  drivers/iio/adc/Kconfig                            |  10 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/ltc2497.c                          | 279 +++++++++++++++++++++
>  5 files changed, 304 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/ltc2497.txt
>  create mode 100644 drivers/iio/adc/ltc2497.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ltc2497.txt b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> new file mode 100644
> index 0000000..a237ed9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/ltc2497.txt
> @@ -0,0 +1,13 @@
> +* Linear Technology / Analog Devices LTC2497 ADC
> +
> +Required properties:
> + - compatible: Must be "lltc,ltc2497"
> + - reg: Must contain the ADC I2C address
> + - vref-supply: The regulator supply for ADC reference voltage
> +
> +Example:
> +	ltc2497: adc@76 {
> +		compatible = "lltc,ltc2497";
> +		reg = <0x76>;
> +		vref-supply = <&ltc2497_reg>;
> +	};
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a7d6f9a..173043c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -813,6 +813,7 @@ W:	http://wiki.analog.com/
>  W:	http://ez.analog.com/community/linux-device-drivers
>  S:	Supported
>  F:	drivers/iio/*/ad*
> +F:	drivers/iio/adc/ltc2497*
>  X:	drivers/iio/*/adjd*
>  F:	drivers/staging/iio/*/ad*
>  F:	drivers/staging/iio/trigger/iio-trig-bfin-timer.c
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2268a6f..acc115b 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -326,6 +326,16 @@ config LTC2485
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called ltc2485.
>  
> +config LTC2497
> +	tristate "Linear Technology LTC2497 ADC driver"
> +	depends on I2C
> +	help
> +	  Say yes here to build support for Linear Technology LTC2497
> +	  16-Bit 8-/16-Channel Delta Sigma ADC.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called ltc2497.
> +
>  config MAX1027
>  	tristate "Maxim max1027 ADC driver"
>  	depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 73dbe39..9d626b5 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_LP8788_ADC) += lp8788_adc.o
>  obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LPC32XX_ADC) += lpc32xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
> +obj-$(CONFIG_LTC2497) += ltc2497.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
>  obj-$(CONFIG_MAX1363) += max1363.o
> diff --git a/drivers/iio/adc/ltc2497.c b/drivers/iio/adc/ltc2497.c
> new file mode 100644
> index 0000000..2691b10
> --- /dev/null
> +++ b/drivers/iio/adc/ltc2497.c
> @@ -0,0 +1,279 @@
> +/*
> + * ltc2497.c - Driver for Analog Devices/Linear Technology LTC2497 ADC
> + *
> + * Copyright (C) 2017 Analog Devices Inc.
> + *
> + * Licensed under the GPL-2.
> + *
> + * Datasheet: http://cds.linear.com/docs/en/datasheet/2497fd.pdf
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define LTC2497_ENABLE			0xA0
> +#define LTC2497_SGL			BIT(4)
> +#define LTC2497_DIFF			0
> +#define LTC2497_SIGN			BIT(3)
> +#define LTC2497_CONFIG_DEFAULT		LTC2497_ENABLE
> +#define LTC2497_CONVERSION_TIME_MS	150ULL
> +
> +struct ltc2497_st {
> +	struct i2c_client *client;
> +	struct regulator *ref;
> +	ktime_t	time_prev;
> +	u8 addr_prev;
> +	/*
> +	 * DMA (thus cache coherency maintenance) requires the
> +	 * transfer buffers to live in their own cache lines.
> +	 */
> +	__be32 buf ____cacheline_aligned;
> +};
> +
> +static int ltc2497_wait_conv(struct ltc2497_st *st)
> +{
> +	s64 time_elapsed;
> +
> +	time_elapsed = ktime_ms_delta(ktime_get(), st->time_prev);
> +
> +	if (time_elapsed < LTC2497_CONVERSION_TIME_MS) {
> +		/* delay if conversion time not passed
> +		 * since last read or write
> +		 */
> +		if (msleep_interruptible(
> +		    LTC2497_CONVERSION_TIME_MS - time_elapsed))
> +			return -ERESTARTSYS;
> +
> +		return 0;
> +	}
> +
> +	if (time_elapsed - LTC2497_CONVERSION_TIME_MS <= 0) {
> +		/* We're in automatic mode -
> +		 * so the last reading is stil not outdated
> +		 */
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int ltc2497_read(struct ltc2497_st *st, u8 address, int *val)
> +{
> +	struct i2c_client *client = st->client;
> +	int ret;
> +
> +	ret = ltc2497_wait_conv(st);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret || st->addr_prev != address) {
> +		ret = i2c_smbus_write_byte(st->client,
> +					   LTC2497_ENABLE | address);
> +		if (ret < 0)
> +			return ret;
> +		st->addr_prev = address;
> +		if (msleep_interruptible(LTC2497_CONVERSION_TIME_MS))
> +			return -ERESTARTSYS;
> +	}
> +	ret = i2c_master_recv(client, (char *)&st->buf, 3);
> +	if (ret < 0)  {
> +		dev_err(&client->dev, "i2c_master_recv failed\n");
> +		return ret;
> +	}
> +	st->time_prev = ktime_get();
> +
> +	/* convert and shift the result,
> +	 * and finally convert from offset binary to signed integer
> +	 */
> +	*val = (be32_to_cpu(st->buf) >> 14) - (1 << 17);
> +
> +	return ret;
> +}
> +
> +static int ltc2497_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +		ret = ltc2497_read(st, chan->address, val);
> +		mutex_unlock(&indio_dev->mlock);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(st->ref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = 17;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +#define LTC2497_CHAN(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan), \
> +	.address = (_addr | (_chan / 2) | ((_chan & 1) ? LTC2497_SIGN : 0)), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +}
> +
> +#define LTC2497_CHAN_DIFF(_chan, _addr) { \
> +	.type = IIO_VOLTAGE, \
> +	.indexed = 1, \
> +	.channel = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 1 : 0), \
> +	.channel2 = (_chan) * 2 + ((_addr) & LTC2497_SIGN ? 0 : 1),\
> +	.address = (_addr | _chan), \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
> +	.differential = 1, \
> +}
> +
> +static const struct iio_chan_spec ltc2497_channel[] = {
> +	LTC2497_CHAN(0, LTC2497_SGL),
> +	LTC2497_CHAN(1, LTC2497_SGL),
> +	LTC2497_CHAN(2, LTC2497_SGL),
> +	LTC2497_CHAN(3, LTC2497_SGL),
> +	LTC2497_CHAN(4, LTC2497_SGL),
> +	LTC2497_CHAN(5, LTC2497_SGL),
> +	LTC2497_CHAN(6, LTC2497_SGL),
> +	LTC2497_CHAN(7, LTC2497_SGL),
> +	LTC2497_CHAN(8, LTC2497_SGL),
> +	LTC2497_CHAN(9, LTC2497_SGL),
> +	LTC2497_CHAN(10, LTC2497_SGL),
> +	LTC2497_CHAN(11, LTC2497_SGL),
> +	LTC2497_CHAN(12, LTC2497_SGL),
> +	LTC2497_CHAN(13, LTC2497_SGL),
> +	LTC2497_CHAN(14, LTC2497_SGL),
> +	LTC2497_CHAN(15, LTC2497_SGL),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF),
> +	LTC2497_CHAN_DIFF(0, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(1, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(2, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(3, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(4, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(5, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(6, LTC2497_DIFF | LTC2497_SIGN),
> +	LTC2497_CHAN_DIFF(7, LTC2497_DIFF | LTC2497_SIGN),
> +};
> +
> +static const struct iio_info ltc2497_info = {
> +	.read_raw = ltc2497_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int ltc2497_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct iio_dev *indio_dev;
> +	struct ltc2497_st *st;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_WRITE_BYTE))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	st->client = client;
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->name = id->name;
> +	indio_dev->info = &ltc2497_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ltc2497_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(ltc2497_channel);
> +
> +	st->ref = devm_regulator_get(&client->dev, "vref");
> +	if (IS_ERR(st->ref))
> +		return PTR_ERR(st->ref);
> +
> +	ret = regulator_enable(st->ref);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = i2c_smbus_write_byte(st->client, LTC2497_CONFIG_DEFAULT);
> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	st->addr_prev = LTC2497_CONFIG_DEFAULT;
> +	st->time_prev = ktime_get();
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		goto err_regulator_disable;
> +
> +	return 0;
> +
> +err_regulator_disable:
> +	regulator_disable(st->ref);
> +
> +	return ret;
> +}
> +
> +static int ltc2497_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ltc2497_st *st = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	regulator_disable(st->ref);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id ltc2497_id[] = {
> +	{ "ltc2497", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ltc2497_id);
> +
> +static const struct of_device_id ltc2497_of_match[] = {
> +	{ .compatible = "lltc,ltc2497", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ltc2497_of_match);
> +
> +static struct i2c_driver ltc2497_driver = {
> +	.driver = {
> +		.name = "ltc2497",
> +		.of_match_table = of_match_ptr(ltc2497_of_match),
> +	},
> +	.probe = ltc2497_probe,
> +	.remove = ltc2497_remove,
> +	.id_table = ltc2497_id,
> +};
> +module_i2c_driver(ltc2497_driver);
> +
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich-OyLXuOCK7orQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Linear Technology LTC2497 ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

^ permalink raw reply

* [PATCH] powerpc/sequoia: fix NAND partitions not to overlap
From: Pavel Machek @ 2017-04-02 10:05 UTC (permalink / raw)
  To: sr-ynQEQJNshbs, jwboyer-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A, mark.rutland-5wv7dgnIgG8,
	benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r,
	paulus-eUNUBHrolfbYtjvyW6yDsg, mpe-Gsx/Oe8HsFggBc27wqDAHg,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA

[-- Attachment #1: Type: text/plain, Size: 639 bytes --]

Fix overlapping NAND partitions.

Signed-off-by: Pavel Machek <pavel-+ZI9xUNit7I@public.gmane.org>

diff --git a/arch/powerpc/boot/dts/sequoia.dts b/arch/powerpc/boot/dts/sequoia.dts
index b1d3292..e41b88a 100644
--- a/arch/powerpc/boot/dts/sequoia.dts
+++ b/arch/powerpc/boot/dts/sequoia.dts
@@ -229,7 +229,7 @@
 						};
 						partition@84000 {
 							label = "user";
-							reg = <0x00000000 0x01f7c000>;
+							reg = <0x00084000 0x01f7c000>;
 						};
 					};
 				};



-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

^ permalink raw reply related

* Re: [PATCH v2] iio: adc: add max1117/max1118/max1119 ADC driver
From: Jonathan Cameron @ 2017-04-02 10:23 UTC (permalink / raw)
  To: Akinobu Mita, linux-iio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
  Cc: Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald-Stadler,
	Rob Herring, Mark Rutland, Mark Brown
In-Reply-To: <1490718888-4760-1-git-send-email-akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

On 28/03/17 17:34, Akinobu Mita wrote:
> This adds max1117/max1118/max1119 8-bit, dual-channel ADC driver.
> 
> This new driver uses the zero length spi_transfers with the cs_change
> flag set and/or the non-zero delay_usecs.
> 
> 1. The zero length transfer with the spi_transfer.cs_change set is
> required in order to select CH1.  The chip select line must be brought
> high and low again without transfer.
> 
> 2. The zero length transfer with the spi_transfer.delay_usecs > 0 is
> required for waiting the conversion to be complete.  The conversion
> begins with the falling edge of the chip select.  During the conversion
> process, SCLK is ignored.
> 
> These two usages are unusual.  But the spi controller drivers that use
> a default implementation of transfer_one_message() are likely to work.
> (I've tested this adc driver with spi-omap2-mcspi and spi-xilinx)
> 
> On the other hand, some spi controller drivers that have their own
> transfer_one_message() may not work.  But at least for the zero length
> transfer with delay_usecs > 0, I'm proposing a new testcase for the
> spi-loopback-test that can test whether the delay_usecs setting has
> taken effect.

Thanks for the detailed description.  As you might imagine I'll
be looking for an Ack from Mark Brown on this one - or if the discussion
is proceeding elsewhere, please post a link.

Otherwise driver looks great to me.

Give me a poke in a week or two if I seem to have forgotten it.

Jonathan
> 
> Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>
> Cc: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
> Cc: Peter Meerwald-Stadler <pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org>
> Cc: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Cc: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> ---
> * v2
> - move max1118_remove() after max1118_probe() for usual code ordering
> - make the probe() fail for max1118 if vref-supply isn't acquired
> - add acked-by line
> 
>  .../devicetree/bindings/iio/adc/max1118.txt        |  21 ++
>  drivers/iio/adc/Kconfig                            |  12 +
>  drivers/iio/adc/Makefile                           |   1 +
>  drivers/iio/adc/max1118.c                          | 307 +++++++++++++++++++++
>  4 files changed, 341 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/adc/max1118.txt
>  create mode 100644 drivers/iio/adc/max1118.c
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/max1118.txt b/Documentation/devicetree/bindings/iio/adc/max1118.txt
> new file mode 100644
> index 0000000..cf33d0b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/max1118.txt
> @@ -0,0 +1,21 @@
> +* MAX1117/MAX1118/MAX1119 8-bit, dual-channel ADCs
> +
> +Required properties:
> + - compatible: Should be one of
> +	* "maxim,max1117"
> +	* "maxim,max1118"
> +	* "maxim,max1119"
> + - reg: spi chip select number for the device
> + - (max1118 only) vref-supply: The regulator supply for ADC reference voltage
> +
> +Recommended properties:
> + - spi-max-frequency: Definition as per
> +		Documentation/devicetree/bindings/spi/spi-bus.txt
> +
> +Example:
> +adc@0 {
> +	compatible = "maxim,max1118";
> +	reg = <0>;
> +	vref-supply = <&vdd_supply>;
> +	spi-max-frequency = <1000000>;
> +};
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index dedae7a..66262d1 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -335,6 +335,18 @@ config MAX11100
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called max11100.
>  
> +config MAX1118
> +	tristate "Maxim max1117/max1118/max1119 ADCs driver"
> +	depends on SPI
> +	select IIO_BUFFER
> +	select IIO_TRIGGERED_BUFFER
> +	help
> +	  Say yes here to build support for Maxim max1117/max1118/max1119
> +	  8-bit, dual-channel ADCs.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called max1118.
> +
>  config MAX1363
>  	tristate "Maxim max1363 ADC driver"
>  	depends on I2C
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index d001262..5ef3470 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -33,6 +33,7 @@ obj-$(CONFIG_LPC18XX_ADC) += lpc18xx_adc.o
>  obj-$(CONFIG_LTC2485) += ltc2485.o
>  obj-$(CONFIG_MAX1027) += max1027.o
>  obj-$(CONFIG_MAX11100) += max11100.o
> +obj-$(CONFIG_MAX1118) += max1118.o
>  obj-$(CONFIG_MAX1363) += max1363.o
>  obj-$(CONFIG_MCP320X) += mcp320x.o
>  obj-$(CONFIG_MCP3422) += mcp3422.o
> diff --git a/drivers/iio/adc/max1118.c b/drivers/iio/adc/max1118.c
> new file mode 100644
> index 0000000..2e9648a
> --- /dev/null
> +++ b/drivers/iio/adc/max1118.c
> @@ -0,0 +1,307 @@
> +/*
> + * MAX1117/MAX1118/MAX1119 8-bit, dual-channel ADCs driver
> + *
> + * Copyright (c) 2017 Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License.  See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * Datasheet: https://datasheets.maximintegrated.com/en/ds/MAX1117-MAX1119.pdf
> + *
> + * SPI interface connections
> + *
> + * SPI                MAXIM
> + * Master  Direction  MAX1117/8/9
> + * ------  ---------  -----------
> + * nCS        -->     CNVST
> + * SCK        -->     SCLK
> + * MISO       <--     DOUT
> + * ------  ---------  -----------
> + */
> +
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/regulator/consumer.h>
> +
> +enum max1118_id {
> +	max1117,
> +	max1118,
> +	max1119,
> +};
> +
> +struct max1118 {
> +	struct spi_device *spi;
> +	struct mutex lock;
> +	struct regulator *reg;
> +
> +	u8 data ____cacheline_aligned;
> +};
> +
> +#define MAX1118_CHANNEL(ch)						\
> +	{								\
> +		.type = IIO_VOLTAGE,					\
> +		.indexed = 1,						\
> +		.channel = (ch),					\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +		.scan_index = ch,					\
> +		.scan_type = {						\
> +			.sign = 'u',					\
> +			.realbits = 8,					\
> +			.storagebits = 8,				\
> +		},							\
> +	}
> +
> +static const struct iio_chan_spec max1118_channels[] = {
> +	MAX1118_CHANNEL(0),
> +	MAX1118_CHANNEL(1),
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
> +};
> +
> +static int max1118_read(struct spi_device *spi, int channel)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct max1118 *adc = iio_priv(indio_dev);
> +	struct spi_transfer xfers[] = {
> +		/*
> +		 * To select CH1 for conversion, CNVST pin must be brought high
> +		 * and low for a second time.
> +		 */
> +		{
> +			.len = 0,
> +			.delay_usecs = 1,	/* > CNVST Low Time 100 ns */
> +			.cs_change = 1,
> +		},
> +		/*
> +		 * The acquisition interval begins with the falling edge of
> +		 * CNVST.  The total acquisition and conversion process takes
> +		 * <7.5us.
> +		 */
> +		{
> +			.len = 0,
> +			.delay_usecs = 8,
> +		},
> +		{
> +			.rx_buf = &adc->data,
> +			.len = 1,
> +		},
> +	};
> +	int ret;
> +
> +	if (channel == 0)
> +		ret = spi_sync_transfer(spi, xfers + 1, 2);
> +	else
> +		ret = spi_sync_transfer(spi, xfers, 3);
> +
> +	if (ret)
> +		return ret;
> +
> +	return adc->data;
> +}
> +
> +static int max1118_get_vref_mV(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct max1118 *adc = iio_priv(indio_dev);
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int vref_uV;
> +
> +	switch (id->driver_data) {
> +	case max1117:
> +		return 2048;
> +	case max1119:
> +		return 4096;
> +	case max1118:
> +		vref_uV = regulator_get_voltage(adc->reg);
> +		if (vref_uV < 0)
> +			return vref_uV;
> +		return vref_uV / 1000;
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +static int max1118_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val, int *val2, long mask)
> +{
> +	struct max1118 *adc = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&adc->lock);
> +		*val = max1118_read(adc->spi, chan->channel);
> +		mutex_unlock(&adc->lock);
> +		if (*val < 0)
> +			return *val;
> +
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = max1118_get_vref_mV(adc->spi);
> +		if (*val < 0)
> +			return *val;
> +		*val2 = 8;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static const struct iio_info max1118_info = {
> +	.read_raw = max1118_read_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static irqreturn_t max1118_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct max1118 *adc = iio_priv(indio_dev);
> +	u8 data[16] = { }; /* 2x 8-bit ADC data + padding + 8 bytes timestamp */
> +	int scan_index;
> +	int i = 0;
> +
> +	mutex_lock(&adc->lock);
> +
> +	for_each_set_bit(scan_index, indio_dev->active_scan_mask,
> +			indio_dev->masklength) {
> +		const struct iio_chan_spec *scan_chan =
> +				&indio_dev->channels[scan_index];
> +		int ret = max1118_read(adc->spi, scan_chan->channel);
> +
> +		if (ret < 0) {
> +			dev_warn(&adc->spi->dev,
> +				"failed to get conversion data\n");
> +			goto out;
> +		}
> +
> +		data[i] = ret;
> +		i++;
> +	}
> +	iio_push_to_buffers_with_timestamp(indio_dev, data,
> +					   iio_get_time_ns(indio_dev));
> +out:
> +	mutex_unlock(&adc->lock);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int max1118_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct max1118 *adc;
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*adc));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	adc = iio_priv(indio_dev);
> +	adc->spi = spi;
> +	mutex_init(&adc->lock);
> +
> +	if (id->driver_data == max1118) {
> +		adc->reg = devm_regulator_get(&spi->dev, "vref");
> +		if (IS_ERR(adc->reg)) {
> +			dev_err(&spi->dev, "failed to get vref regulator\n");
> +			return PTR_ERR(adc->reg);
> +		}
> +		ret = regulator_enable(adc->reg);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &max1118_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = max1118_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max1118_channels);
> +
> +	/*
> +	 * To reinitiate a conversion on CH0, it is necessary to allow for a
> +	 * conversion to be complete and all of the data to be read out.  Once
> +	 * a conversion has been completed, the MAX1117/MAX1118/MAX1119 will go
> +	 * into AutoShutdown mode until the next conversion is initiated.
> +	 */
> +	max1118_read(spi, 0);
> +
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL,
> +					max1118_trigger_handler, NULL);
> +	if (ret)
> +		goto err_reg_disable;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err_buffer_cleanup;
> +
> +	return 0;
> +
> +err_buffer_cleanup:
> +	iio_triggered_buffer_cleanup(indio_dev);
> +err_reg_disable:
> +	if (id->driver_data == max1118)
> +		regulator_disable(adc->reg);
> +
> +	return ret;
> +}
> +
> +static int max1118_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct max1118 *adc = iio_priv(indio_dev);
> +	const struct spi_device_id *id = spi_get_device_id(spi);
> +
> +	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
> +	if (id->driver_data == max1118)
> +		return regulator_disable(adc->reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id max1118_id[] = {
> +	{ "max1117", max1117 },
> +	{ "max1118", max1118 },
> +	{ "max1119", max1119 },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, max1118_id);
> +
> +#ifdef CONFIG_OF
> +
> +static const struct of_device_id max1118_dt_ids[] = {
> +	{ .compatible = "maxim,max1117" },
> +	{ .compatible = "maxim,max1118" },
> +	{ .compatible = "maxim,max1119" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, max1118_dt_ids);
> +
> +#endif
> +
> +static struct spi_driver max1118_spi_driver = {
> +	.driver = {
> +		.name = "max1118",
> +		.of_match_table = of_match_ptr(max1118_dt_ids),
> +	},
> +	.probe = max1118_probe,
> +	.remove = max1118_remove,
> +	.id_table = max1118_id,
> +};
> +module_spi_driver(max1118_spi_driver);
> +
> +MODULE_AUTHOR("Akinobu Mita <akinobu.mita-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>");
> +MODULE_DESCRIPTION("MAXIM MAX1117/MAX1118/MAX1119 ADCs driver");
> +MODULE_LICENSE("GPL v2");
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCH v2 2/5] ARM: dts: rockchip: add ARM Mali GPU node for rk3288
From: Enric Balletbo i Serra @ 2017-04-02 10:41 UTC (permalink / raw)
  To: Guillaume Tucker, Rob Herring, Mark Rutland
  Cc: Sjoerd Simons, John Reitan, Wookey, devicetree, linux-rockchip,
	linux-arm-kernel, linux-kernel
In-Reply-To: <46d14d3c969a35e800b1c06da0ddb652986e6827.1491118230.git.guillaume.tucker@collabora.com>



On 02/04/17 09:59, Guillaume Tucker wrote:
> Add Mali GPU device tree node for the rk3288 SoC, with devfreq
> opp table.
> 
> Signed-off-by: Guillaume Tucker <guillaume.tucker@collabora.com>
> ---
>  arch/arm/boot/dts/rk3288.dtsi | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
> index df8a0dbe9d91..213224ea309e 100644
> --- a/arch/arm/boot/dts/rk3288.dtsi
> +++ b/arch/arm/boot/dts/rk3288.dtsi
> @@ -43,6 +43,7 @@
>  #include <dt-bindings/interrupt-controller/arm-gic.h>
>  #include <dt-bindings/pinctrl/rockchip.h>
>  #include <dt-bindings/clock/rk3288-cru.h>
> +#include <dt-bindings/power/rk3288-power.h>
>  #include <dt-bindings/thermal/thermal.h>
>  #include <dt-bindings/power/rk3288-power.h>
>  #include <dt-bindings/soc/rockchip,boot-mode.h>
> @@ -227,6 +228,28 @@
>  		ports = <&vopl_out>, <&vopb_out>;
>  	};
>  
> +	gpu: mali@ffa30000 {
> +		compatible = "arm,mali-midgard";
> +		reg = <0xffa30000 0x10000>;
> +		interrupts = <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "JOB", "MMU", "GPU";
> +		clocks = <&cru ACLK_GPU>;
> +		clock-names = "clk_mali";
> +		operating-points = <
> +			/* KHz uV */
> +			100000 950000
> +			200000 950000
> +			300000 1000000
> +			400000 1100000
> +			500000 1200000
> +			600000 1250000
> +		>;
> +		power-domains = <&power RK3288_PD_GPU>;
> +		status = "disabled";
> +	};
> +
>  	sdmmc: dwmmc@ff0c0000 {
>  		compatible = "rockchip,rk3288-dw-mshc";
>  		max-frequency = <150000000>;
> 

Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

^ permalink raw reply

* Re: [RFC PATCH 2/3] iio: adc: sun4i-gpadc-iio: add support for H3 thermal sensor
From: Jonathan Cameron @ 2017-04-02 10:51 UTC (permalink / raw)
  To: Quentin Schulz, Icenowy Zheng, Lee Jones, Rob Herring,
	Maxime Ripard, Chen-Yu Tsai, Zhang Rui
  Cc: devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-iio-u79uwXL29TY76Z2rM5mHXA, linux-pm-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <813dc1d8-89b4-9c0e-c1cc-e76efde2be1f-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>

On 29/03/17 07:54, Quentin Schulz wrote:
> Hi,
> 
> On 28/03/2017 19:30, Icenowy Zheng wrote:
>> This adds support for the Allwinner H3 thermal sensor.
>>
>> Allwinner H3 has a thermal sensor like the one in A33, but have its
>> registers nearly all re-arranged, sample clock moved to CCU and a pair
>> of bus clock and reset added. It's also the base of newer SoCs' thermal
>> sensors.
>>
>> An option is added to gpadc_data struct, to indicate whether this device
>> is a new-generation Allwinner thermal sensor.
>>
>> The thermal sensors on A64 and H5 is like the one on H3, but with of
>> course different formula factors.
>>
>> Signed-off-by: Icenowy Zheng <icenowy-h8G6r0blFSE@public.gmane.org>
>> ---
>>  drivers/iio/adc/sun4i-gpadc-iio.c | 130 ++++++++++++++++++++++++++++++++------
>>  include/linux/mfd/sun4i-gpadc.h   |  33 +++++++++-
>>  2 files changed, 141 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/iio/adc/sun4i-gpadc-iio.c b/drivers/iio/adc/sun4i-gpadc-iio.c
>> index 74705aa37982..7512b1cae877 100644
>> --- a/drivers/iio/adc/sun4i-gpadc-iio.c
>> +++ b/drivers/iio/adc/sun4i-gpadc-iio.c
>> @@ -22,6 +22,7 @@
>>   * shutdown for not being used.
>>   */
>>  
>> +#include <linux/clk.h>
>>  #include <linux/completion.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/io.h>
>> @@ -31,6 +32,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/regmap.h>
>> +#include <linux/reset.h>
>>  #include <linux/thermal.h>
>>  #include <linux/delay.h>
>>  
>> @@ -56,6 +58,7 @@ struct gpadc_data {
>>  	unsigned int	tp_adc_select;
>>  	unsigned int	(*adc_chan_select)(unsigned int chan);
>>  	unsigned int	adc_chan_mask;
>> +	bool		gen2_ths;
>>  };
>>  
> 
> Instead of a boolean, give the TEMP_DATA register address.
> 
>>  static const struct gpadc_data sun4i_gpadc_data = {
>> @@ -88,7 +91,20 @@ static const struct gpadc_data sun6i_gpadc_data = {
>>  static const struct gpadc_data sun8i_a33_gpadc_data = {
>>  	.temp_offset = -1662,
>>  	.temp_scale = 162,
>> -	.tp_mode_en = SUN8I_GPADC_CTRL1_CHOP_TEMP_EN,
>> +	.tp_mode_en = SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN,
>> +};
> 
> Separate patch for this?
Indeed, ideal to rework original first (really easy to review)
then add support for new part.
> 
>> +
>> +static const struct gpadc_data sun8i_h3_gpadc_data = {
>> +	/*
>> +	 * The original formula on the datasheet seems to be wrong.
>> +	 * These factors are calculated based on the formula in the BSP
>> +	 * kernel, which is originally Tem = 217 - (T / 8.253), in which Tem
>> +	 * is the temperature in Celsius degree and T is the raw value
>> +	 * from the sensor.
>> +	 */
>> +	.temp_offset = -1791,
>> +	.temp_scale = -121,
>> +	.gen2_ths = true,
>>  };
>>  
>>  struct sun4i_gpadc_iio {
>> @@ -103,6 +119,9 @@ struct sun4i_gpadc_iio {
>>  	atomic_t			ignore_temp_data_irq;
>>  	const struct gpadc_data		*data;
>>  	bool				no_irq;
>> +	struct clk			*ths_bus_clk;
>> +	struct clk			*ths_clk;
>> +	struct reset_control		*reset;
>>  	/* prevents concurrent reads of temperature and ADC */
>>  	struct mutex			mutex;
>>  };
>> @@ -274,7 +293,11 @@ static int sun4i_gpadc_temp_read(struct iio_dev *indio_dev, int *val)
>>  	if (info->no_irq) {
>>  		pm_runtime_get_sync(indio_dev->dev.parent);
>>  
>> -		regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>> +		if (info->data->gen2_ths)
>> +			regmap_read(info->regmap, SUN8I_H3_GPADC_TEMP_DATA,
>> +				    val);
>> +		else
>> +			regmap_read(info->regmap, SUN4I_GPADC_TEMP_DATA, val);
>>  
> 
> Instead of gen2_ths, use the TEMP_DATA register address.
Agreed. Put a set of register addresses into the gpadc_data structure.
Both more flexible and easier to read.
> 
>>  		pm_runtime_mark_last_busy(indio_dev->dev.parent);
>>  		pm_runtime_put_autosuspend(indio_dev->dev.parent);
>> @@ -386,10 +409,15 @@ static int sun4i_gpadc_runtime_suspend(struct device *dev)
>>  {
>>  	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
>>  
>> -	/* Disable the ADC on IP */
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
>> -	/* Disable temperature sensor on IP */
>> -	regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
>> +	if (info->data->gen2_ths) {
>> +		/* Disable temperature sensor */
>> +		regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2, 0);
>> +	} else {
>> +		/* Disable the ADC on IP */
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL1, 0);
>> +		/* Disable temperature sensor on IP */
>> +		regmap_write(info->regmap, SUN4I_GPADC_TPR, 0);
>> +	}
>>  
> 
> Either use another register address or add a suspend function to struct
> gpadc_data which will be different for each version of the IP.
> 
>>  	return 0;
>>  }
>> @@ -398,19 +426,36 @@ static int sun4i_gpadc_runtime_resume(struct device *dev)
>>  {
>>  	struct sun4i_gpadc_iio *info = iio_priv(dev_get_drvdata(dev));
>>  
>> -	/* clkin = 6MHz */
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
>> -		     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
>> -		     SUN4I_GPADC_CTRL0_FS_DIV(7) |
>> -		     SUN4I_GPADC_CTRL0_T_ACQ(63));
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL1, info->data->tp_mode_en);
>> -	regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
>> -		     SUN4I_GPADC_CTRL3_FILTER_EN |
>> -		     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
>> -	/* period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin; ~0.6s */
>> -	regmap_write(info->regmap, SUN4I_GPADC_TPR,
>> -		     SUN4I_GPADC_TPR_TEMP_ENABLE |
>> -		     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
>> +	if (info->data->gen2_ths) {
>> +		regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL2,
>> +			     SUN8I_H3_GPADC_CTRL2_TEMP_SENSE_EN |
>> +			     SUN8I_H3_GPADC_CTRL2_T_ACQ1(31));
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
>> +			     SUN4I_GPADC_CTRL0_T_ACQ(31));
>> +		regmap_write(info->regmap, SUN8I_H3_GPADC_CTRL3,
>> +			     SUN4I_GPADC_CTRL3_FILTER_EN |
>> +			     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
>> +		regmap_write(info->regmap, SUN8I_H3_GPADC_INTC,
>> +			     SUN8I_H3_GPADC_INTC_TEMP_PERIOD(800));
>> +	} else {
>> +		/* clkin = 6MHz */
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL0,
>> +			     SUN4I_GPADC_CTRL0_ADC_CLK_DIVIDER(2) |
>> +			     SUN4I_GPADC_CTRL0_FS_DIV(7) |
>> +			     SUN4I_GPADC_CTRL0_T_ACQ(63));
Looks like you'll need something like a function pointer in your
_data structure as well to cover cases where they are totally
different like here.  or you could use regmap fields if they map
reasonably well... (doesn't look like they do though!)
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL1,
>> +			     info->data->tp_mode_en);
>> +		regmap_write(info->regmap, SUN4I_GPADC_CTRL3,
>> +			     SUN4I_GPADC_CTRL3_FILTER_EN |
>> +			     SUN4I_GPADC_CTRL3_FILTER_TYPE(1));
>> +		/*
>> +		 * period = SUN4I_GPADC_TPR_TEMP_PERIOD * 256 * 16 / clkin;
>> +		 * ~0.6s
>> +		 */
>> +		regmap_write(info->regmap, SUN4I_GPADC_TPR,
>> +			     SUN4I_GPADC_TPR_TEMP_ENABLE |
>> +			     SUN4I_GPADC_TPR_TEMP_PERIOD(800));
>> +	}
>>  
> 
> Same here as suspend function?
> 
>>  	return 0;
>>  }
>> @@ -494,6 +539,10 @@ static const struct of_device_id sun4i_gpadc_of_id[] = {
>>  		.compatible = "allwinner,sun8i-a33-ths",
>>  		.data = &sun8i_a33_gpadc_data,
>>  	},
>> +	{
>> +		.compatible = "allwinner,sun8i-h3-ths",
>> +		.data = &sun8i_h3_gpadc_data,
>> +	},
>>  	{ /* sentinel */ }
>>  };
>>  
>> @@ -529,6 +578,43 @@ static int sun4i_gpadc_probe_dt(struct platform_device *pdev,
>>  		return ret;
>>  	}
>>  
>> +	if (info->data->gen2_ths) {
>> +		info->reset = devm_reset_control_get(&pdev->dev, NULL);
>> +		if (IS_ERR(info->reset)) {
>> +			ret = PTR_ERR(info->reset);
>> +			return ret;
>> +		}
>> +
>> +		ret = reset_control_deassert(info->reset);
>> +		if (ret)
>> +			return ret;
>> +
>> +		info->ths_bus_clk = devm_clk_get(&pdev->dev, "bus");
>> +		if (IS_ERR(info->ths_bus_clk)) {
>> +			ret = PTR_ERR(info->ths_bus_clk);
>> +			return ret;
>> +		}
>> +
>> +		ret = clk_prepare_enable(info->ths_bus_clk);
>> +		if (ret)
>> +			return ret;
>> +
>> +		info->ths_clk = devm_clk_get(&pdev->dev, "ths");
>> +		if (IS_ERR(info->ths_clk)) {
>> +			ret = PTR_ERR(info->ths_clk);
>> +			return ret;
>> +		}
>> +
>> +		/* Running at 6MHz */
>> +		ret = clk_set_rate(info->ths_clk, 6000000);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = clk_prepare_enable(info->ths_clk);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>>  	if (!IS_ENABLED(CONFIG_THERMAL_OF))
>>  		return 0;
>>  
>> @@ -691,6 +777,12 @@ static int sun4i_gpadc_remove(struct platform_device *pdev)
>>  	if (!info->no_irq && IS_ENABLED(CONFIG_THERMAL_OF))
>>  		iio_map_array_unregister(indio_dev);
>>  
>> +	if (info->data->gen2_ths) {
>> +		clk_disable_unprepare(info->ths_clk);
>> +		clk_disable_unprepare(info->ths_bus_clk);
>> +		reset_control_deassert(info->reset);
>> +	}
>> +
> 
> I'm not really fond of using this boolean as I don't see it being
> possibly reused for any other SoCs that has a GPADC or THS.
> 
> Here, you could make use of a list/array of clk which then can be reused
> for other SoCs just by changing the list. Add a default rate to the
> gpadc_data structure and you're go to go.
> 
>>  	return 0;
>>  }
>>  
>> diff --git a/include/linux/mfd/sun4i-gpadc.h b/include/linux/mfd/sun4i-gpadc.h
>> index 139872c2e0fe..f794a2988a93 100644
>> --- a/include/linux/mfd/sun4i-gpadc.h
>> +++ b/include/linux/mfd/sun4i-gpadc.h
>> @@ -38,9 +38,12 @@
>>  #define SUN6I_GPADC_CTRL1_ADC_CHAN_SELECT(x)		(GENMASK(3, 0) & BIT(x))
>>  #define SUN6I_GPADC_CTRL1_ADC_CHAN_MASK			GENMASK(3, 0)
>>  
>> -/* TP_CTRL1 bits for sun8i SoCs */
>> -#define SUN8I_GPADC_CTRL1_CHOP_TEMP_EN			BIT(8)
>> -#define SUN8I_GPADC_CTRL1_GPADC_CALI_EN			BIT(7)
>> +/* TP_CTRL1 bits for sun8i A23/A33 SoCs */
>> +#define SUN8I_A23_GPADC_CTRL1_CHOP_TEMP_EN		BIT(8)
>> +#define SUN8I_A23_GPADC_CTRL1_GPADC_CALI_EN		BIT(7)
>> +
> 
> Different patch for these?
Looks to me like you want a precursor that reworks the driver to
be ready to support the new part, followed by a patch that actually
introduces the new part.

Jonathan
> 
> Thanks,
> Quentin
> 

^ permalink raw reply

* Re: [PATCH 1/4] dt-bindings: iio: stm32-dac: Add support for STM32 DAC
From: Jonathan Cameron @ 2017-04-02 11:16 UTC (permalink / raw)
  To: Fabrice Gasnier, linux-I+IVW8TIWO2tmTQ+vhA3Yw,
	robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
	linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-kernel-u79uwXL29TY76Z2rM5mHXA
  Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA, mark.rutland-5wv7dgnIgG8,
	mcoquelin.stm32-Re5JQEeQqe8AvxtiuMwx3w,
	alexandre.torgue-qxv4g6HH51o, lars-Qo5EllUWu/uELgA04lAiVw,
	knaack.h-Mmb7MZpHnFY, pmeerw-jW+XmwGofnusTnJN9+BGXg,
	benjamin.gaignard-QSEj5FYQhm4dnm+yROfE0A,
	benjamin.gaignard-qxv4g6HH51o,
	linus.walleij-QSEj5FYQhm4dnm+yROfE0A, amelie.delaunay-qxv4g6HH51o
In-Reply-To: <1490960707-22422-2-git-send-email-fabrice.gasnier-qxv4g6HH51o@public.gmane.org>

On 31/03/17 12:45, Fabrice Gasnier wrote:
> Document STMicroelectronics STM32 DAC (digital-to-analog converter).
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/dac/st,stm32-dac.txt   | 56 ++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> new file mode 100644
> index 0000000..1981983
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/st,stm32-dac.txt
> @@ -0,0 +1,56 @@
> +STMicroelectronics STM32 DAC
> +
> +The STM32 DAC is a 12-bit voltage output digital-to-analog converter. The DAC
> +can be configured in 8- or 12-bit mode. In 12-bit mode, the data could be
> +left- or right-aligned.
Whilst possibly true, do we care about the alignment?  That'll all get wrapped
up in the driver.

> It has two output channels, each with its own converter.
> +It has built-in noise and triangle waveform generator and supports external
> +triggers for conversions. The DAC's output buffer allows a high drive output
> +current.
Ah.. This is going to be fun :) More unusual hardware to find in an SoC.
> +
> +Contents of a stm32 dac root node:
> +-----------------------------------
> +Required properties:
> +- compatible: Must be "st,stm32h7-dac-core".
> +- reg: Offset and length of the device's register set.
> +- clocks: Must contain an entry for pclk (which feeds the peripheral bus
> +  interface)
> +- clock-names: Must be "pclk".
> +- vref-supply: Phandle to the vref+ input analog reference supply.
> +
> +Optional properties:
> +- resets: Must contain the phandle to the reset controller.
> +- A pinctrl state named "default" for each DAC channel may be defined to set
> +  DAC_OUTx pin in mode of operation for analog output on external pin.
> +
> +Contents of a stm32 dac child node:
> +-----------------------------------
> +DAC core node should contain at least one subnode, representing a
> +DAC instance/channel available on the machine.
> +
> +Required properties:
> +- compatible: Must be "st,stm32-dac".
> +- #io-channel-cells = <1>: See the IIO bindings section "IIO consumers" in
> +  Documentation/devicetree/bindings/iio/iio-bindings.txt
> +- st,dac-channel: Must be either 1 or 2, to define channel in use (e.g.
> +  single channels: 1 or 2)
> +
> +Example:
> +	dac: dac@40007400 {
> +		compatible = "st,stm32h7-dac-core";
> +		reg = <0x40007400 0x400>;
> +		clocks = <&clk>;
> +		clock-names = "pclk";
> +		vref-supply = <&reg_vref>;
> +
> +		dac1: dac@1 {
> +			compatible = "st,stm32-dac";
> +			#io-channels-cells = <1>;
> +			st,dac-channel = <1>;
> +		};
> +
> +		dac2: dac@2 {
> +			compatible = "st,stm32-dac";
> +			#io-channels-cells = <1>;
> +			st,dac-channel = <2>;
> +		};
> +	};
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox