[PATCH 1/6] Input: pm8xxx-vib: reorder header alphabetically

[PATCH 1/6] Input: pm8xxx-vib: reorder header alphabetically

[Damien Riegel]

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 drivers/input/misc/pm8xxx-vibrator.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 5113877153d7..580448170342 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -10,13 +10,13 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/module.h>
-#include <linux/kernel.h>
 #include <linux/errno.h>
-#include <linux/platform_device.h>
 #include <linux/input.h>
-#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/slab.h>
 
 #define VIB_DRV			0x4A
 
-- 
2.12.0

[PATCH 2/6] Input: pm8xxx-vib: sync device tree bindings doc with the driver

[Damien Riegel]

The driver uses a hardcoded value for the register, so the parameter set
in the device tree is not actually used.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
index 4ed467b1e402..86ce95fc6cf8 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
@@ -12,7 +12,8 @@ PROPERTIES
 - reg:
 	Usage: required
 	Value type: <prop-encoded-array>
-	Definition: address of vibration control register
+	Definition: address of vibration control register. This value is
+		    actually ignored and hardcoded in the driver to value 0x4a
 
 EXAMPLE
 
-- 
2.12.0

[PATCH 3/6] Input: pm8xxx-vib: parametrize the driver

[Damien Riegel]

In order to prepare this driver to support other vibrators of the same
kind, move some hardcoded values to a structure holding register
parameters (address, mask, shit of the control register).

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 drivers/input/misc/pm8xxx-vibrator.c | 49 ++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 16 deletions(-)

diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 580448170342..f1daae08a8c2 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -14,36 +14,47 @@
 #include <linux/input.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/slab.h>
 
-#define VIB_DRV			0x4A
-
-#define VIB_DRV_SEL_MASK	0xf8
-#define VIB_DRV_SEL_SHIFT	0x03
-#define VIB_DRV_EN_MANUAL_MASK	0xfc
-
 #define VIB_MAX_LEVEL_mV	(3100)
 #define VIB_MIN_LEVEL_mV	(1200)
 #define VIB_MAX_LEVELS		(VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
 
 #define MAX_FF_SPEED		0xff
 
+struct pm8xxx_regs {
+	unsigned int drv_addr;
+	unsigned int drv_mask;
+	unsigned int drv_shift;
+	unsigned int drv_en_manual_mask;
+};
+
+static struct pm8xxx_regs pm8058_regs = {
+	.drv_addr = 0x4A,
+	.drv_mask = 0xf8,
+	.drv_shift = 3,
+	.drv_en_manual_mask = 0xfc,
+};
+
 /**
  * struct pm8xxx_vib - structure to hold vibrator data
  * @vib_input_dev: input device supporting force feedback
  * @work: work structure to set the vibration parameters
  * @regmap: regmap for register read/write
+ * @regs: registers' info
  * @speed: speed of vibration set from userland
  * @active: state of vibrator
  * @level: level of vibration to set in the chip
- * @reg_vib_drv: VIB_DRV register value
+ * @reg_vib_drv: regs->drv_addr register value
  */
 struct pm8xxx_vib {
 	struct input_dev *vib_input_dev;
 	struct work_struct work;
 	struct regmap *regmap;
+	struct pm8xxx_regs *regs;
 	int speed;
 	int level;
 	bool active;
@@ -59,13 +70,14 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
 {
 	int rc;
 	unsigned int val = vib->reg_vib_drv;
+	struct pm8xxx_regs *regs = vib->regs;
 
 	if (on)
-		val |= ((vib->level << VIB_DRV_SEL_SHIFT) & VIB_DRV_SEL_MASK);
+		val |= ((vib->level << regs->drv_shift) & regs->drv_mask);
 	else
-		val &= ~VIB_DRV_SEL_MASK;
+		val &= ~(regs->drv_mask);
 
-	rc = regmap_write(vib->regmap, VIB_DRV, val);
+	rc = regmap_write(vib->regmap, regs->drv_addr, val);
 	if (rc < 0)
 		return rc;
 
@@ -80,10 +92,11 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
 static void pm8xxx_work_handler(struct work_struct *work)
 {
 	struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
+	struct pm8xxx_regs *regs = vib->regs;
 	int rc;
 	unsigned int val;
 
-	rc = regmap_read(vib->regmap, VIB_DRV, &val);
+	rc = regmap_read(vib->regmap, regs->drv_addr, &val);
 	if (rc < 0)
 		return;
 
@@ -147,6 +160,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
 	struct input_dev *input_dev;
 	int error;
 	unsigned int val;
+	struct pm8xxx_regs *regs;
 
 	vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
 	if (!vib)
@@ -163,16 +177,19 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
 	INIT_WORK(&vib->work, pm8xxx_work_handler);
 	vib->vib_input_dev = input_dev;
 
+	regs = (struct pm8xxx_regs *)of_device_get_match_data(&pdev->dev);
+
 	/* operate in manual mode */
-	error = regmap_read(vib->regmap, VIB_DRV, &val);
+	error = regmap_read(vib->regmap, regs->drv_addr, &val);
 	if (error < 0)
 		return error;
 
-	val &= ~VIB_DRV_EN_MANUAL_MASK;
-	error = regmap_write(vib->regmap, VIB_DRV, val);
+	val &= ~(regs->drv_en_manual_mask);
+	error = regmap_write(vib->regmap, regs->drv_addr, val);
 	if (error < 0)
 		return error;
 
+	vib->regs = regs;
 	vib->reg_vib_drv = val;
 
 	input_dev->name = "pm8xxx_vib_ffmemless";
@@ -212,8 +229,8 @@ static int __maybe_unused pm8xxx_vib_suspend(struct device *dev)
 static SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
 
 static const struct of_device_id pm8xxx_vib_id_table[] = {
-	{ .compatible = "qcom,pm8058-vib" },
-	{ .compatible = "qcom,pm8921-vib" },
+	{ .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
+	{ .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
-- 
2.12.0

[PATCH 4/6] Input: pm8xxx-vib: handle separate enable register

[Damien Riegel]

Some PMIC vibrator IPs use a separate enable register to turn the
vibrator on and off. To detect if a vibrator uses this feature, rely on
the enable_mask being non-zero.

If they use it, the device tree is queried to retrieve the base address
of the control and enable registers.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 drivers/input/misc/pm8xxx-vibrator.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index f1daae08a8c2..803bc75d5531 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -14,6 +14,7 @@
 #include <linux/input.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -26,6 +27,9 @@
 #define MAX_FF_SPEED		0xff
 
 struct pm8xxx_regs {
+	unsigned int enable_addr;
+	unsigned int enable_mask;
+
 	unsigned int drv_addr;
 	unsigned int drv_mask;
 	unsigned int drv_shift;
@@ -82,7 +86,14 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
 		return rc;
 
 	vib->reg_vib_drv = val;
-	return 0;
+
+	if (regs->enable_mask) {
+		unsigned int val = on ? regs->enable_mask : 0;
+		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
+					regs->enable_mask, val);
+	}
+
+	return rc;
 }
 
 /**
@@ -179,6 +190,22 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
 
 	regs = (struct pm8xxx_regs *)of_device_get_match_data(&pdev->dev);
 
+	/*
+	 * If enable_mask is not zero, that means we're using a vibrator that
+	 * requires multiple registers to be controlled, the value read in the
+	 * device tree is the base address of these registers.
+	 */
+	if (regs->enable_mask) {
+		u32 base;
+
+		error = of_property_read_u32(pdev->dev.of_node, "reg", &base);
+		if (error)
+			return error;
+
+		regs->drv_addr += base;
+		regs->enable_addr += base;
+	}
+
 	/* operate in manual mode */
 	error = regmap_read(vib->regmap, regs->drv_addr, &val);
 	if (error < 0)
-- 
2.12.0

[PATCH 5/6] Input: pm8xxx-vib: add pm8916-vib device tree bindings

[Damien Riegel]

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
index 86ce95fc6cf8..4f968f02cad1 100644
--- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
+++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
@@ -7,13 +7,16 @@ PROPERTIES
 	Value type: <string>
 	Definition: must be one of:
 		    "qcom,pm8058-vib"
+		    "qcom,pm8916-vib"
 		    "qcom,pm8921-vib"
 
 - reg:
 	Usage: required
 	Value type: <prop-encoded-array>
-	Definition: address of vibration control register. This value is
-		    actually ignored and hardcoded in the driver to value 0x4a
+	Definition: pm8058/pm8921: address of vibration control register. This
+				   value is actually ignored and hardcoded in
+				   the driver to value 0x4a
+		    pm8916: base address of the vibration module
 
 EXAMPLE
 
-- 
2.12.0

[PATCH 6/6] Input: pm8xxx-vib: add support for pm8916's vibrator

[Damien Riegel]

Add pm8xxx_regs for this PMIC and the device tree match table entry.

Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
---
 drivers/input/misc/Kconfig           |  2 +-
 drivers/input/misc/pm8xxx-vibrator.c | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
index 5b6c52210d20..79d0be9885c5 100644
--- a/drivers/input/misc/Kconfig
+++ b/drivers/input/misc/Kconfig
@@ -143,7 +143,7 @@ config INPUT_PM8941_PWRKEY
 
 config INPUT_PM8XXX_VIBRATOR
 	tristate "Qualcomm PM8XXX vibrator support"
-	depends on MFD_PM8XXX
+	depends on MFD_PM8XXX || MFD_SPMI_PMIC
 	select INPUT_FF_MEMLESS
 	help
 	  This option enables device driver support for the vibrator
diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
index 803bc75d5531..96d18a300e7a 100644
--- a/drivers/input/misc/pm8xxx-vibrator.c
+++ b/drivers/input/misc/pm8xxx-vibrator.c
@@ -43,6 +43,15 @@ static struct pm8xxx_regs pm8058_regs = {
 	.drv_en_manual_mask = 0xfc,
 };
 
+static struct pm8xxx_regs pm8916_regs = {
+	.enable_addr = 0x46,
+	.enable_mask = BIT(7),
+	.drv_addr = 0x41,
+	.drv_mask = 0x1F,
+	.drv_shift = 0,
+	.drv_en_manual_mask = 0,
+};
+
 /**
  * struct pm8xxx_vib - structure to hold vibrator data
  * @vib_input_dev: input device supporting force feedback
@@ -258,6 +267,7 @@ static SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
 static const struct of_device_id pm8xxx_vib_id_table[] = {
 	{ .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
 	{ .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
+	{ .compatible = "qcom,pm8916-vib", .data = &pm8916_regs },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
-- 
2.12.0

Re: [PATCH 2/6] Input: pm8xxx-vib: sync device tree bindings doc with the driver

[Dmitry Torokhov]

On Fri, Mar 31, 2017 at 12:15:34PM -0400, Damien Riegel wrote:
> The driver uses a hardcoded value for the register, so the parameter set
> in the device tree is not actually used.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>  Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> index 4ed467b1e402..86ce95fc6cf8 100644
> --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> @@ -12,7 +12,8 @@ PROPERTIES
>  - reg:
>  	Usage: required
>  	Value type: <prop-encoded-array>
> -	Definition: address of vibration control register
> +	Definition: address of vibration control register. This value is
> +		    actually ignored and hardcoded in the driver to value 0x4a

I do not think we need to commit that the value is hard coded, it is
implementation detail of current version of Linux driver, whereas DT
bindings should be independent of OS as much as reasonably possible.

Also, I think you can change the code to actually read and use it from
DT to support your other device use case.

Thanks.

-- 
Dmitry

Re: [PATCH 3/6] Input: pm8xxx-vib: parametrize the driver

[Dmitry Torokhov]

On Fri, Mar 31, 2017 at 12:15:35PM -0400, Damien Riegel wrote:
> In order to prepare this driver to support other vibrators of the same
> kind, move some hardcoded values to a structure holding register
> parameters (address, mask, shit of the control register).
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>  drivers/input/misc/pm8xxx-vibrator.c | 49 ++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index 580448170342..f1daae08a8c2 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -14,36 +14,47 @@
>  #include <linux/input.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
>  #include <linux/slab.h>
>  
> -#define VIB_DRV			0x4A
> -
> -#define VIB_DRV_SEL_MASK	0xf8
> -#define VIB_DRV_SEL_SHIFT	0x03
> -#define VIB_DRV_EN_MANUAL_MASK	0xfc
> -
>  #define VIB_MAX_LEVEL_mV	(3100)
>  #define VIB_MIN_LEVEL_mV	(1200)
>  #define VIB_MAX_LEVELS		(VIB_MAX_LEVEL_mV - VIB_MIN_LEVEL_mV)
>  
>  #define MAX_FF_SPEED		0xff
>  
> +struct pm8xxx_regs {
> +	unsigned int drv_addr;
> +	unsigned int drv_mask;
> +	unsigned int drv_shift;
> +	unsigned int drv_en_manual_mask;
> +};
> +
> +static struct pm8xxx_regs pm8058_regs = {
> +	.drv_addr = 0x4A,
> +	.drv_mask = 0xf8,
> +	.drv_shift = 3,
> +	.drv_en_manual_mask = 0xfc,
> +};
> +
>  /**
>   * struct pm8xxx_vib - structure to hold vibrator data
>   * @vib_input_dev: input device supporting force feedback
>   * @work: work structure to set the vibration parameters
>   * @regmap: regmap for register read/write
> + * @regs: registers' info
>   * @speed: speed of vibration set from userland
>   * @active: state of vibrator
>   * @level: level of vibration to set in the chip
> - * @reg_vib_drv: VIB_DRV register value
> + * @reg_vib_drv: regs->drv_addr register value
>   */
>  struct pm8xxx_vib {
>  	struct input_dev *vib_input_dev;
>  	struct work_struct work;
>  	struct regmap *regmap;
> +	struct pm8xxx_regs *regs;

const struct

>  	int speed;
>  	int level;
>  	bool active;
> @@ -59,13 +70,14 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>  {
>  	int rc;
>  	unsigned int val = vib->reg_vib_drv;
> +	struct pm8xxx_regs *regs = vib->regs;

const struct

>  
>  	if (on)
> -		val |= ((vib->level << VIB_DRV_SEL_SHIFT) & VIB_DRV_SEL_MASK);
> +		val |= ((vib->level << regs->drv_shift) & regs->drv_mask);
>  	else
> -		val &= ~VIB_DRV_SEL_MASK;
> +		val &= ~(regs->drv_mask);

No need for parenthesis.

>  
> -	rc = regmap_write(vib->regmap, VIB_DRV, val);
> +	rc = regmap_write(vib->regmap, regs->drv_addr, val);
>  	if (rc < 0)
>  		return rc;
>  
> @@ -80,10 +92,11 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>  static void pm8xxx_work_handler(struct work_struct *work)
>  {
>  	struct pm8xxx_vib *vib = container_of(work, struct pm8xxx_vib, work);
> +	struct pm8xxx_regs *regs = vib->regs;
>  	int rc;
>  	unsigned int val;
>  
> -	rc = regmap_read(vib->regmap, VIB_DRV, &val);
> +	rc = regmap_read(vib->regmap, regs->drv_addr, &val);
>  	if (rc < 0)
>  		return;
>  
> @@ -147,6 +160,7 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>  	struct input_dev *input_dev;
>  	int error;
>  	unsigned int val;
> +	struct pm8xxx_regs *regs;

const struct.

>  
>  	vib = devm_kzalloc(&pdev->dev, sizeof(*vib), GFP_KERNEL);
>  	if (!vib)
> @@ -163,16 +177,19 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>  	INIT_WORK(&vib->work, pm8xxx_work_handler);
>  	vib->vib_input_dev = input_dev;
>  
> +	regs = (struct pm8xxx_regs *)of_device_get_match_data(&pdev->dev);

With const reg pointer you would not need to cast.

> +
>  	/* operate in manual mode */
> -	error = regmap_read(vib->regmap, VIB_DRV, &val);
> +	error = regmap_read(vib->regmap, regs->drv_addr, &val);
>  	if (error < 0)
>  		return error;
>  
> -	val &= ~VIB_DRV_EN_MANUAL_MASK;
> -	error = regmap_write(vib->regmap, VIB_DRV, val);
> +	val &= ~(regs->drv_en_manual_mask);

Please drop parenthesis.

> +	error = regmap_write(vib->regmap, regs->drv_addr, val);
>  	if (error < 0)
>  		return error;
>  
> +	vib->regs = regs;
>  	vib->reg_vib_drv = val;
>  
>  	input_dev->name = "pm8xxx_vib_ffmemless";
> @@ -212,8 +229,8 @@ static int __maybe_unused pm8xxx_vib_suspend(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(pm8xxx_vib_pm_ops, pm8xxx_vib_suspend, NULL);
>  
>  static const struct of_device_id pm8xxx_vib_id_table[] = {
> -	{ .compatible = "qcom,pm8058-vib" },
> -	{ .compatible = "qcom,pm8921-vib" },
> +	{ .compatible = "qcom,pm8058-vib", .data = &pm8058_regs },
> +	{ .compatible = "qcom,pm8921-vib", .data = &pm8058_regs },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(of, pm8xxx_vib_id_table);
> -- 
> 2.12.0
> 

Thanks.


-- 
Dmitry

Re: [PATCH 4/6] Input: pm8xxx-vib: handle separate enable register

[Dmitry Torokhov]

On Fri, Mar 31, 2017 at 12:15:36PM -0400, Damien Riegel wrote:
> Some PMIC vibrator IPs use a separate enable register to turn the
> vibrator on and off. To detect if a vibrator uses this feature, rely on
> the enable_mask being non-zero.
> 
> If they use it, the device tree is queried to retrieve the base address
> of the control and enable registers.
> 
> Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> ---
>  drivers/input/misc/pm8xxx-vibrator.c | 29 ++++++++++++++++++++++++++++-
>  1 file changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/misc/pm8xxx-vibrator.c b/drivers/input/misc/pm8xxx-vibrator.c
> index f1daae08a8c2..803bc75d5531 100644
> --- a/drivers/input/misc/pm8xxx-vibrator.c
> +++ b/drivers/input/misc/pm8xxx-vibrator.c
> @@ -14,6 +14,7 @@
>  #include <linux/input.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/platform_device.h>
>  #include <linux/regmap.h>
> @@ -26,6 +27,9 @@
>  #define MAX_FF_SPEED		0xff
>  
>  struct pm8xxx_regs {
> +	unsigned int enable_addr;
> +	unsigned int enable_mask;
> +
>  	unsigned int drv_addr;
>  	unsigned int drv_mask;
>  	unsigned int drv_shift;
> @@ -82,7 +86,14 @@ static int pm8xxx_vib_set(struct pm8xxx_vib *vib, bool on)
>  		return rc;
>  
>  	vib->reg_vib_drv = val;
> -	return 0;
> +
> +	if (regs->enable_mask) {
> +		unsigned int val = on ? regs->enable_mask : 0;
> +		rc = regmap_update_bits(vib->regmap, regs->enable_addr,
> +					regs->enable_mask, val);
> +	}
> +
> +	return rc;
>  }
>  
>  /**
> @@ -179,6 +190,22 @@ static int pm8xxx_vib_probe(struct platform_device *pdev)
>  
>  	regs = (struct pm8xxx_regs *)of_device_get_match_data(&pdev->dev);
>  
> +	/*
> +	 * If enable_mask is not zero, that means we're using a vibrator that
> +	 * requires multiple registers to be controlled, the value read in the
> +	 * device tree is the base address of these registers.
> +	 */
> +	if (regs->enable_mask) {
> +		u32 base;
> +
> +		error = of_property_read_u32(pdev->dev.of_node, "reg", &base);
> +		if (error)
> +			return error;

Is it really configurable? Shouldn't it be in chip-spoecific instance of
regs to begin with?

> +
> +		regs->drv_addr += base;
> +		regs->enable_addr += base;

Regs should be const, you should not change it (what happens if there
are 2 devices?).

> +	}
> +
>  	/* operate in manual mode */
>  	error = regmap_read(vib->regmap, regs->drv_addr, &val);
>  	if (error < 0)
> -- 
> 2.12.0
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/6] Input: pm8xxx-vib: sync device tree bindings doc with the driver

[Damien Riegel]

On Sat, Apr 01, 2017 at 09:54:09AM -0700, Dmitry Torokhov wrote:
> On Fri, Mar 31, 2017 at 12:15:34PM -0400, Damien Riegel wrote:
> > The driver uses a hardcoded value for the register, so the parameter set
> > in the device tree is not actually used.
> > 
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > ---
> >  Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > index 4ed467b1e402..86ce95fc6cf8 100644
> > --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > @@ -12,7 +12,8 @@ PROPERTIES
> >  - reg:
> >  	Usage: required
> >  	Value type: <prop-encoded-array>
> > -	Definition: address of vibration control register
> > +	Definition: address of vibration control register. This value is
> > +		    actually ignored and hardcoded in the driver to value 0x4a
> 
> I do not think we need to commit that the value is hard coded, it is
> implementation detail of current version of Linux driver, whereas DT
> bindings should be independent of OS as much as reasonably possible.
> 
> Also, I think you can change the code to actually read and use it from
> DT to support your other device use case.

I was hesitant to do that because that might break stuff for people who
use a device tree with reg != 0x4a, but if you tell me that's okay I'll
send a v2 that reads device tree for all pm8xxx vibrators.

Thanks,
-- 
Damien

Re: [PATCH 2/6] Input: pm8xxx-vib: sync device tree bindings doc with the driver

[Dmitry Torokhov]

On Sat, Apr 01, 2017 at 01:51:27PM -0400, Damien Riegel wrote:
> On Sat, Apr 01, 2017 at 09:54:09AM -0700, Dmitry Torokhov wrote:
> > On Fri, Mar 31, 2017 at 12:15:34PM -0400, Damien Riegel wrote:
> > > The driver uses a hardcoded value for the register, so the parameter set
> > > in the device tree is not actually used.
> > > 
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > > ---
> > >  Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > index 4ed467b1e402..86ce95fc6cf8 100644
> > > --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > @@ -12,7 +12,8 @@ PROPERTIES
> > >  - reg:
> > >  	Usage: required
> > >  	Value type: <prop-encoded-array>
> > > -	Definition: address of vibration control register
> > > +	Definition: address of vibration control register. This value is
> > > +		    actually ignored and hardcoded in the driver to value 0x4a
> > 
> > I do not think we need to commit that the value is hard coded, it is
> > implementation detail of current version of Linux driver, whereas DT
> > bindings should be independent of OS as much as reasonably possible.
> > 
> > Also, I think you can change the code to actually read and use it from
> > DT to support your other device use case.
> 
> I was hesitant to do that because that might break stuff for people who
> use a device tree with reg != 0x4a, but if you tell me that's okay I'll
> send a v2 that reads device tree for all pm8xxx vibrators.

Actually, I was looking at the rest of the code and I now I wonder if we
should be doing this for any of the devices. The registers are
chip-specific and we get chip data from compatible string, so the driver
is fine to simply use static mappings. It is only if we start using a
common compatible string for different chips we would need to start
parsing DT data.

Thanks.

-- 
Dmitry

Re: [PATCH 2/6] Input: pm8xxx-vib: sync device tree bindings doc with the driver

[Damien Riegel]

On Sat, Apr 01, 2017 at 11:06:07AM -0700, Dmitry Torokhov wrote:
> On Sat, Apr 01, 2017 at 01:51:27PM -0400, Damien Riegel wrote:
> > On Sat, Apr 01, 2017 at 09:54:09AM -0700, Dmitry Torokhov wrote:
> > > On Fri, Mar 31, 2017 at 12:15:34PM -0400, Damien Riegel wrote:
> > > > The driver uses a hardcoded value for the register, so the parameter set
> > > > in the device tree is not actually used.
> > > > 
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > > index 4ed467b1e402..86ce95fc6cf8 100644
> > > > --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > > +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > > @@ -12,7 +12,8 @@ PROPERTIES
> > > >  - reg:
> > > >  	Usage: required
> > > >  	Value type: <prop-encoded-array>
> > > > -	Definition: address of vibration control register
> > > > +	Definition: address of vibration control register. This value is
> > > > +		    actually ignored and hardcoded in the driver to value 0x4a
> > > 
> > > I do not think we need to commit that the value is hard coded, it is
> > > implementation detail of current version of Linux driver, whereas DT
> > > bindings should be independent of OS as much as reasonably possible.
> > > 
> > > Also, I think you can change the code to actually read and use it from
> > > DT to support your other device use case.
> > 
> > I was hesitant to do that because that might break stuff for people who
> > use a device tree with reg != 0x4a, but if you tell me that's okay I'll
> > send a v2 that reads device tree for all pm8xxx vibrators.
> 
> Actually, I was looking at the rest of the code and I now I wonder if we
> should be doing this for any of the devices. The registers are
> chip-specific and we get chip data from compatible string, so the driver
> is fine to simply use static mappings. It is only if we start using a
> common compatible string for different chips we would need to start
> parsing DT data.

I agree that for now, it's chip-specific and storing the base address as
DT data instead of static data doesn't bring much. But on the other
hand, using DT data makes us consistent with the other PMIC IPs' drivers
and future-ready if they ever decide to reuse the same IP at a different
offset in another chip.

Anyway I'm fine with both ways, just let me know which one you prefer
and I'll do it that way.

Thank you,
-- 
Damien

Re: [PATCH 2/6] Input: pm8xxx-vib: sync device tree bindings doc with the driver

[Dmitry Torokhov]

On Sat, Apr 01, 2017 at 02:57:23PM -0400, Damien Riegel wrote:
> On Sat, Apr 01, 2017 at 11:06:07AM -0700, Dmitry Torokhov wrote:
> > On Sat, Apr 01, 2017 at 01:51:27PM -0400, Damien Riegel wrote:
> > > On Sat, Apr 01, 2017 at 09:54:09AM -0700, Dmitry Torokhov wrote:
> > > > On Fri, Mar 31, 2017 at 12:15:34PM -0400, Damien Riegel wrote:
> > > > > The driver uses a hardcoded value for the register, so the parameter set
> > > > > in the device tree is not actually used.
> > > > > 
> > > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > > Cc: Mark Rutland <mark.rutland@arm.com>
> > > > > Signed-off-by: Damien Riegel <damien.riegel@savoirfairelinux.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > > > index 4ed467b1e402..86ce95fc6cf8 100644
> > > > > --- a/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > > > +++ b/Documentation/devicetree/bindings/input/qcom,pm8xxx-vib.txt
> > > > > @@ -12,7 +12,8 @@ PROPERTIES
> > > > >  - reg:
> > > > >  	Usage: required
> > > > >  	Value type: <prop-encoded-array>
> > > > > -	Definition: address of vibration control register
> > > > > +	Definition: address of vibration control register. This value is
> > > > > +		    actually ignored and hardcoded in the driver to value 0x4a
> > > > 
> > > > I do not think we need to commit that the value is hard coded, it is
> > > > implementation detail of current version of Linux driver, whereas DT
> > > > bindings should be independent of OS as much as reasonably possible.
> > > > 
> > > > Also, I think you can change the code to actually read and use it from
> > > > DT to support your other device use case.
> > > 
> > > I was hesitant to do that because that might break stuff for people who
> > > use a device tree with reg != 0x4a, but if you tell me that's okay I'll
> > > send a v2 that reads device tree for all pm8xxx vibrators.
> > 
> > Actually, I was looking at the rest of the code and I now I wonder if we
> > should be doing this for any of the devices. The registers are
> > chip-specific and we get chip data from compatible string, so the driver
> > is fine to simply use static mappings. It is only if we start using a
> > common compatible string for different chips we would need to start
> > parsing DT data.
> 
> I agree that for now, it's chip-specific and storing the base address as
> DT data instead of static data doesn't bring much. But on the other
> hand, using DT data makes us consistent with the other PMIC IPs' drivers
> and future-ready if they ever decide to reuse the same IP at a different
> offset in another chip.
> 
> Anyway I'm fine with both ways, just let me know which one you prefer
> and I'll do it that way.

I think to keep with the current structure of the driver I'd prefer
using compatible property to fetch a structure describing chip's
registers instead of fetching it from device tree.

Thanks.

-- 
Dmitry