[PATCH RESEND v4 0/4] Initial support for ImgTec PWM DAC and PDM DAC

[PATCH RESEND v4 0/4] Initial support for ImgTec PWM DAC and PDM DAC

[naidu.tellapati]

From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

The Pistachio SOC from Imagination Technologies includes – 4x PDM blocks
and 1x “basic PWM” block with four output channels. These share 4 MFIO pins,
each PWM channel being muxed with each PDM block output (0-3 muxed with 0-3)
using the CR_PERIP_PWM_PDM_CONTROL register in the peripheral wrapper
register bank (offset 0x140). The PDM and PWM outputs are used to control
targets such as LCD backlight.

The PWM driver uses Linux PWM framework. Since the PDM does not fit into PWM
framework we have implemented it as a MISC driver.

The series is based on v3.18-rc5.

I am re-sending the series with gmail SMTP as we had some problems with
the mail server we used in the previous rounds.

Please review and provide your comments.

Changes from v3:

 * Addressed couple of comments given in v3.

Changes from v2:

 * Added depends on MIPS || COMPILE_TEST.

 * Corrected the logic while calculating time base steps.

 * Addressed few other minor comments given in v2.

Changes from v1:

 * Used regmap_update_bits at some places in the driver.

 * Placed some #defines next to their register.

 * Added depends on HAS_IOMEM in Kconfig.

 * Removed unnecessary paragraph from file header.

 * Defined register masks in a readable way.

 * Renamed clk-names with clock-names.

 * Added system interface clock in DT binding document.

 * Addressed few other minor review comments given in v1.

Naidu Tellapati (4):
  pwm: Imagination Technologies PWM DAC driver
  DT: pwm: Add binding document for IMG PWM DAC
  pdm: Imagination Technologies PDM DAC driver
  DT: pdm: Add binding document for IMG PDM DAC

 Documentation/devicetree/bindings/misc/img-pdm.txt |   54 ++
 Documentation/devicetree/bindings/pwm/img-pwm.txt  |   23 +
 drivers/misc/Kconfig                               |   12 +
 drivers/misc/Makefile                              |    1 +
 drivers/misc/img-pdm.c                             |  676 ++++++++++++++++++++
 drivers/pwm/Kconfig                                |   12 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-img.c                              |  270 ++++++++
 include/linux/img_pdm.h                            |   35 +
 9 files changed, 1084 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/misc/img-pdm.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/img-pwm.txt
 create mode 100644 drivers/misc/img-pdm.c
 create mode 100644 drivers/pwm/pwm-img.c
 create mode 100644 include/linux/img_pdm.h

[PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[naidu.tellapati]

From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

The Pistachio SOC from Imagination Technologies includes a Pulse Width
Modulation DAC which produces 1 to 4 digital bit-outputs which represent
digital waveforms. These PWM outputs are primarily in charge of controlling
backlight LED devices.

Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
---
 drivers/pwm/Kconfig   |   12 ++
 drivers/pwm/Makefile  |    1 +
 drivers/pwm/pwm-img.c |  270 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 283 insertions(+), 0 deletions(-)
 create mode 100644 drivers/pwm/pwm-img.c

diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
index ef2dd2e..6b4581a 100644
--- a/drivers/pwm/Kconfig
+++ b/drivers/pwm/Kconfig
@@ -110,6 +110,18 @@ config PWM_FSL_FTM
 	  To compile this driver as a module, choose M here: the module
 	  will be called pwm-fsl-ftm.
 
+config PWM_IMG
+	tristate "Imagination Technologies PWM driver"
+	depends on MFD_SYSCON
+	depends on HAS_IOMEM
+	depends on MIPS || COMPILE_TEST
+	help
+	  Generic PWM framework driver for Imagination Technologies
+	  PWM block which supports 4 channels.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called pwm-img.
+
 config PWM_IMX
 	tristate "i.MX PWM support"
 	depends on ARCH_MXC
diff --git a/drivers/pwm/Makefile b/drivers/pwm/Makefile
index c458606..29655fd 100644
--- a/drivers/pwm/Makefile
+++ b/drivers/pwm/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_PWM_BFIN)		+= pwm-bfin.o
 obj-$(CONFIG_PWM_CLPS711X)	+= pwm-clps711x.o
 obj-$(CONFIG_PWM_EP93XX)	+= pwm-ep93xx.o
 obj-$(CONFIG_PWM_FSL_FTM)	+= pwm-fsl-ftm.o
+obj-$(CONFIG_PWM_IMG)		+= pwm-img.o
 obj-$(CONFIG_PWM_IMX)		+= pwm-imx.o
 obj-$(CONFIG_PWM_JZ4740)	+= pwm-jz4740.o
 obj-$(CONFIG_PWM_LP3943)	+= pwm-lp3943.o
diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c
new file mode 100644
index 0000000..79baac2
--- /dev/null
+++ b/drivers/pwm/pwm-img.c
@@ -0,0 +1,270 @@
+/*
+ * Imagination Technologies Pulse Width Modulator driver
+ *
+ * Copyright (c) 2014, Imagination Technologies
+ *
+ * Based on drivers/pwm/pwm-tegra.c, Copyright (c) 2010, NVIDIA Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/pwm.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+/* PWM registers */
+#define CR_PWM_CTRL_CFG				0x0000
+#define CR_PWM_CTRL_CFG_NO_SUB_DIV		0
+#define CR_PWM_CTRL_CFG_SUB_DIV0		1
+#define CR_PWM_CTRL_CFG_SUB_DIV1		2
+#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1		3
+#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch)		((ch) * 2 + 4)
+#define CR_PWM_CTRL_CFG_DIV_MASK		0x3
+
+#define CR_PWM_CH_CFG(ch)			(0x4 + (ch) * 4)
+#define CR_PWM_CH_CFG_TMBASE_SHIFT		0
+#define CR_PWM_CH_CFG_DUTY_SHIFT		16
+
+#define CR_PERIP_PWM_PDM_CONTROL		0x0140
+#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK	0x1
+#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch)	((ch) * 4)
+
+#define IMG_NUM_PWM				4
+#define MAX_TMBASE_STEPS			65536
+
+struct img_pwm_chip {
+	struct device	*dev;
+	struct pwm_chip	chip;
+	struct clk	*pwm_clk;
+	struct clk	*sys_clk;
+	void __iomem	*base;
+	struct regmap	*periph_regs;
+};
+
+static inline struct img_pwm_chip *to_img_pwm_chip(struct pwm_chip *chip)
+{
+	return container_of(chip, struct img_pwm_chip, chip);
+}
+
+static inline void img_pwm_writel(struct img_pwm_chip *chip,
+				  unsigned int reg, unsigned long val)
+{
+	writel(val, chip->base + reg);
+}
+
+static inline unsigned int img_pwm_readl(struct img_pwm_chip *chip,
+					 unsigned int reg)
+{
+	return readl(chip->base + reg);
+}
+
+static int img_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
+			  int duty_ns, int period_ns)
+{
+	unsigned int div;
+	unsigned int duty_steps;
+	unsigned int tmbase_steps;
+	unsigned long val;
+	unsigned long mul;
+	unsigned long output_clk_hz;
+	unsigned long input_clk_hz;
+	struct img_pwm_chip *pwm_chip;
+
+	pwm_chip = to_img_pwm_chip(chip);
+
+	input_clk_hz = clk_get_rate(pwm_chip->pwm_clk);
+	output_clk_hz = DIV_ROUND_UP(NSEC_PER_SEC, period_ns);
+
+	mul = DIV_ROUND_UP(input_clk_hz, output_clk_hz);
+	if (mul > MAX_TMBASE_STEPS * 512) {
+		dev_err(chip->dev,
+			"failed to configure timebase steps/divider value.\n");
+		return -EINVAL;
+	}
+
+	if (mul <= MAX_TMBASE_STEPS) {
+		div = CR_PWM_CTRL_CFG_NO_SUB_DIV;
+		tmbase_steps = DIV_ROUND_UP(mul, 1);
+	} else if (mul <= MAX_TMBASE_STEPS * 8) {
+		div = CR_PWM_CTRL_CFG_SUB_DIV0;
+		tmbase_steps = DIV_ROUND_UP(mul, 8);
+	} else if (mul <= MAX_TMBASE_STEPS * 64) {
+		div = CR_PWM_CTRL_CFG_SUB_DIV1;
+		tmbase_steps = DIV_ROUND_UP(mul, 64);
+	} else if (mul <= MAX_TMBASE_STEPS * 512) {
+		div = CR_PWM_CTRL_CFG_SUB_DIV0_DIV1;
+		tmbase_steps = DIV_ROUND_UP(mul, 512);
+	}
+
+	duty_steps = DIV_ROUND_UP(tmbase_steps * duty_ns, period_ns);
+
+	val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
+	val &= ~(CR_PWM_CTRL_CFG_DIV_MASK <<
+					CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm));
+	val |= (div & CR_PWM_CTRL_CFG_DIV_MASK) <<
+					CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm);
+	img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
+
+	val = (duty_steps << CR_PWM_CH_CFG_DUTY_SHIFT) |
+				(tmbase_steps << CR_PWM_CH_CFG_TMBASE_SHIFT);
+	img_pwm_writel(pwm_chip, CR_PWM_CH_CFG(pwm->hwpwm), val);
+
+	return 0;
+}
+
+static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	unsigned int val;
+	struct img_pwm_chip *pwm_chip;
+
+	pwm_chip = to_img_pwm_chip(chip);
+
+	val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
+	val |= BIT(pwm->hwpwm);
+	img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
+
+	regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
+			   CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
+			   CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);
+
+	return 0;
+}
+
+static void img_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
+{
+	unsigned int val;
+	struct img_pwm_chip *pwm_chip;
+
+	pwm_chip = to_img_pwm_chip(chip);
+
+	val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
+	val &= ~BIT(pwm->hwpwm);
+	img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
+
+	regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
+			   CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
+			   CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 1);
+}
+
+static const struct pwm_ops img_pwm_ops = {
+	.config = img_pwm_config,
+	.enable = img_pwm_enable,
+	.disable = img_pwm_disable,
+	.owner = THIS_MODULE,
+};
+
+static int img_pwm_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct resource *res;
+	struct img_pwm_chip *pwm;
+
+	pwm = devm_kzalloc(&pdev->dev, sizeof(*pwm), GFP_KERNEL);
+	if (!pwm)
+		return -ENOMEM;
+
+	pwm->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pwm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(pwm->base))
+		return PTR_ERR(pwm->base);
+
+	pwm->periph_regs = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+							"img,cr-periph");
+	if (IS_ERR(pwm->periph_regs))
+		return PTR_ERR(pwm->periph_regs);
+
+	pwm->sys_clk = devm_clk_get(&pdev->dev, "sys");
+	if (IS_ERR(pwm->sys_clk)) {
+		dev_err(&pdev->dev, "failed to get system clock.\n");
+		return PTR_ERR(pwm->sys_clk);
+	}
+
+	pwm->pwm_clk = devm_clk_get(&pdev->dev, "pwm");
+	if (IS_ERR(pwm->pwm_clk)) {
+		dev_err(&pdev->dev, "failed to get pwm clock.\n");
+		return PTR_ERR(pwm->pwm_clk);
+	}
+
+	ret = clk_prepare_enable(pwm->sys_clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not prepare or enable sys clock.\n");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(pwm->pwm_clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not prepare or enable pwm clock.\n");
+		goto disable_sysclk;
+	}
+
+	pwm->chip.dev = &pdev->dev;
+	pwm->chip.ops = &img_pwm_ops;
+	pwm->chip.base = -1;
+	pwm->chip.npwm = IMG_NUM_PWM;
+
+	ret = pwmchip_add(&pwm->chip);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "pwmchip_add failed: %d\n", ret);
+		goto disable_pwmclk;
+	}
+
+	platform_set_drvdata(pdev, pwm);
+
+disable_pwmclk:
+	clk_disable_unprepare(pwm->pwm_clk);
+disable_sysclk:
+	clk_disable_unprepare(pwm->sys_clk);
+
+	return 0;
+}
+
+static int img_pwm_remove(struct platform_device *pdev)
+{
+	unsigned int i;
+	unsigned int val;
+
+	struct img_pwm_chip *pwm_chip = platform_get_drvdata(pdev);
+
+	for (i = 0; i < IMG_NUM_PWM; i++) {
+		val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
+		val &= ~BIT(i);
+		img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
+	}
+
+	clk_disable_unprepare(pwm_chip->pwm_clk);
+	clk_disable_unprepare(pwm_chip->sys_clk);
+
+	return pwmchip_remove(&pwm_chip->chip);
+}
+
+static const struct of_device_id img_pwm_of_match[] = {
+	{ .compatible = "img,pistachio-pwm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, img_pwm_of_match);
+
+static struct platform_driver img_pwm_driver = {
+	.driver = {
+		.name = "img-pwm",
+		.of_match_table = of_match_ptr(img_pwm_of_match),
+	},
+	.probe = img_pwm_probe,
+	.remove = img_pwm_remove,
+};
+module_platform_driver(img_pwm_driver);
+
+MODULE_AUTHOR("Sai Masarapu <Sai.Masarapu@imgtec.com>");
+MODULE_DESCRIPTION("Imagination Technologies PWM DAC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.0.4

[PATCH RESEND v4 2/4] DT: pwm: Add binding document for IMG PWM DAC

[naidu.tellapati]

From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

Add binding document for IMG Pulse Width Modulator (PWM) DAC present on the
Pistachio SOC. The PWM DAC has four channels.

Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
Reviewed-by: Andrew Bresticker <abrestic@chromium.org>
---
No changes from v3.
---
 Documentation/devicetree/bindings/pwm/img-pwm.txt |   23 +++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pwm/img-pwm.txt

diff --git a/Documentation/devicetree/bindings/pwm/img-pwm.txt b/Documentation/devicetree/bindings/pwm/img-pwm.txt
new file mode 100644
index 0000000..d3828dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/pwm/img-pwm.txt
@@ -0,0 +1,23 @@
+*Imagination Technologies PWM DAC driver
+
+Required properties:
+  - compatible: Should be "img,pistachio-pwm"
+  - reg: Should contain physical base address and length of pwm registers.
+  - clocks: Must contain an entry for each entry in clock-names.
+	See ../clock/clock-bindings.txt for details.
+  - clock-names: input clock names.
+	Required elements: "pwm" and "sys".
+  - #pwm-cells: Should be 2. See pwm.txt in this directory for the
+	description of the cells format.
+  - img,cr-periph: Must contain a phandle to the peripheral control
+	syscon node which contains PWM control registers.
+
+Example:
+	pwm: pwm@18101300 {
+		compatible = "img,pistachio-pwm";
+		reg = <0x18101300 0x100>;
+		clocks = <&pwm_clk>, <&system_clk>;
+		clock-names = "pwm", "sys";
+		#pwm-cells = <2>;
+		img,cr-periph = <&cr_periph>;
+	};
-- 
1.7.0.4

[PATCH RESEND v4 3/4] pdm: Imagination Technologies PDM DAC driver

[naidu.tellapati]

From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

The Pistachio SOC from Imagination Technologies includes a Pulse Density
Modulation DAC which produces a form of analogue output according to the
relative density of output pulses to the intended analogue signal amplitude.
Four PDM outputs are provided that can be used to control targets such as LCD
backlight.

Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Signed-off-by: Arul Ramasamy <Arul.Ramasamy@imgtec.com>
---
 drivers/misc/Kconfig    |   12 +
 drivers/misc/Makefile   |    1 +
 drivers/misc/img-pdm.c  |  676 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/img_pdm.h |   35 +++
 4 files changed, 724 insertions(+), 0 deletions(-)
 create mode 100644 drivers/misc/img-pdm.c
 create mode 100644 include/linux/img_pdm.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index bbeb451..5157fa9 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -515,6 +515,18 @@ config VEXPRESS_SYSCFG
 	  bus. System Configuration interface is one of the possible means
 	  of generating transactions on this bus.
 
+config IMG_PDM
+	tristate "Imagination Technologies PDM driver"
+	depends on MFD_SYSCON
+	depends on HAS_IOMEM
+	depends on MIPS || COMPILE_TEST
+	help
+	  PDM driver for Imagination Technologies PDM block which supports 4
+	  channels.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called img-pdm.
+
 source "drivers/misc/c2port/Kconfig"
 source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index 7d5c4cd..d8c7d55 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -56,3 +56,4 @@ obj-$(CONFIG_GENWQE)		+= genwqe/
 obj-$(CONFIG_ECHO)		+= echo/
 obj-$(CONFIG_VEXPRESS_SYSCFG)	+= vexpress-syscfg.o
 obj-$(CONFIG_CXL_BASE)		+= cxl/
+obj-$(CONFIG_IMG_PDM)		+= img-pdm.o
diff --git a/drivers/misc/img-pdm.c b/drivers/misc/img-pdm.c
new file mode 100644
index 0000000..091d77b
--- /dev/null
+++ b/drivers/misc/img-pdm.c
@@ -0,0 +1,676 @@
+/**
+ * Imagination Technologies Pulse Density Modulator driver
+ *
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ *
+ * 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.
+ */
+
+#include <linux/capability.h>
+#include <linux/clk.h>
+#include <linux/ctype.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/img_pdm.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/list.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/string.h>
+#include <linux/sysfs.h>
+
+/* Registers */
+#define CR_PERIP_PWM_PDM_CONTROL		0x0140
+#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK	0x1
+#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch)	((ch) * 4)
+#define CR_PERIP_PDM_CHANNEL_ENABLE		1
+#define CR_PERIP_PDM_CHANNEL_DISABLE		0
+
+#define CR_PERIP_PDM0_VAL			0x0144
+#define CR_PERIP_PDM_CH_ADDR_SHIFT(ch)		((ch) * 4)
+#define CR_PERIP_PDM_SRC_DATA_MASK		0xfff
+
+#define IMG_NUM_PDM				4
+#define PDM_CHANNEL_REQUESTED			1
+#define PDM_CHANNEL_ENABLED			2
+
+struct img_pdm_channel *pdm_channels;
+
+static DEFINE_MUTEX(pdm_lock);
+
+static struct img_pdm_channel *img_pdm_channel_request(unsigned int pdm_id);
+static int img_pdm_channel_free(struct img_pdm_channel *chan);
+
+static struct img_pdm_channel *of_img_pdm_channel_get(struct device_node *np)
+{
+	int err;
+	int index = 0;
+	struct of_phandle_args args;
+	struct img_pdm_channel *chan;
+
+	err = of_parse_phandle_with_args(np, "pdms", "#pdm-cells",
+					 index, &args);
+	if (err) {
+		pr_debug("%s(): can't parse \"pdms\" property\n", __func__);
+		return ERR_PTR(err);
+	}
+
+	if (args.args_count != 2) {
+		pr_debug("%s(): wrong #pwm-cells\n", __func__);
+		return ERR_PTR(-EINVAL);
+	}
+
+	chan = img_pdm_channel_request(args.args[0]);
+	if (chan)
+		img_pdm_channel_pulse_in_set(chan, args.args[1]);
+
+	return chan;
+}
+
+static void of_img_pdm_channel_put(struct device_node *np)
+{
+	struct img_pdm_channel *chan = NULL;
+	struct of_phandle_args args;
+	int index = 0;
+	int err;
+
+	err = of_parse_phandle_with_args(np, "pdms", "#pdm-cells",
+					 index, &args);
+	if (err) {
+		pr_debug("%s(): can't parse \"pdms\" property\n", __func__);
+		return;
+	}
+
+	if (args.args_count != 2) {
+		pr_debug("%s(): wrong #pwm-cells\n", __func__);
+		return;
+	}
+
+	if (args.args[0] < 0 || args.args[0] >= IMG_NUM_PDM || !pdm_channels)
+		return;
+
+	chan = &pdm_channels[args.args[0]];
+	img_pdm_channel_free(chan);
+}
+
+struct img_pdm_channel *img_pdm_channel_get(struct device *dev)
+{
+	/* look up via DT */
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		return of_img_pdm_channel_get(dev->of_node);
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(img_pdm_channel_get);
+
+void img_pdm_channel_put(struct device *dev)
+{
+	/* look up via DT */
+	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node)
+		of_img_pdm_channel_put(dev->of_node);
+}
+EXPORT_SYMBOL_GPL(img_pdm_channel_put);
+
+static struct img_pdm_channel *img_pdm_channel_request(unsigned int pdm_id)
+{
+	struct img_pdm_device *pdm_dev;
+	struct img_pdm_channel *chan = NULL;
+	unsigned int i;
+
+	mutex_lock(&pdm_lock);
+
+	if (pdm_id < 0 || pdm_id >= IMG_NUM_PDM || !pdm_channels)
+		return NULL;
+
+	pdm_dev = pdm_channels[0].pdm_dev;
+	if (!pdm_dev)
+		return NULL;
+
+	for (i = 0; i < IMG_NUM_PDM; i++) {
+		if (&pdm_channels[i] && (pdm_channels[i].pdm_id == pdm_id)) {
+			chan = &pdm_channels[i];
+			break;
+		}
+	}
+
+	if (!chan) {
+		mutex_unlock(&pdm_lock);
+		return NULL;
+	}
+
+	/* Check if channel is already requested */
+	if (test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev,
+			"pdm channel %d already requested\n", chan->pdm_id);
+		mutex_unlock(&pdm_lock);
+		return NULL;
+	}
+
+	set_bit(PDM_CHANNEL_REQUESTED, &chan->flags);
+	mutex_unlock(&pdm_lock);
+
+	return chan;
+}
+
+static int img_pdm_channel_free(struct img_pdm_channel *chan)
+{
+	int i;
+	struct img_pdm_device *pdm_dev;
+
+	mutex_lock(&pdm_lock);
+
+	if (!pdm_channels || !chan) {
+		mutex_unlock(&pdm_lock);
+		return -EINVAL;
+	}
+
+	pdm_dev = pdm_channels[0].pdm_dev;
+	if (!pdm_dev) {
+		mutex_unlock(&pdm_lock);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < IMG_NUM_PDM; i++) {
+		if (&pdm_channels[i] && (&pdm_channels[i] == chan))
+			break;
+	}
+
+	if (i == IMG_NUM_PDM) {
+		dev_err(&pdm_dev->pdev->dev,
+			"Invalid pdm channel address to free\n");
+		mutex_unlock(&pdm_lock);
+		return -EINVAL;
+	}
+
+	if (test_bit(PDM_CHANNEL_ENABLED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev,
+			"can't free the channel while it is enabled\n");
+		mutex_unlock(&pdm_lock);
+		return -EINVAL;
+	}
+
+	if (!test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev,
+			"trying to free channel which is not requested\n");
+		mutex_unlock(&pdm_lock);
+		return -EINVAL;
+	}
+
+	clear_bit(PDM_CHANNEL_REQUESTED, &chan->flags);
+
+	mutex_unlock(&pdm_lock);
+
+	return 0;
+}
+
+int img_pdm_channel_enable_get(struct img_pdm_channel *chan)
+{
+	int ret;
+	struct img_pdm_device *pdm_dev;
+
+	if (!chan)
+		return -EINVAL;
+
+	pdm_dev = chan->pdm_dev;
+
+	if (!test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev, "pdm chan is notrequested\n");
+		return -EINVAL;
+	}
+
+	if (test_bit(PDM_CHANNEL_ENABLED, &chan->flags))
+		ret = CR_PERIP_PDM_CHANNEL_ENABLE;
+	else
+		ret = CR_PERIP_PDM_CHANNEL_DISABLE;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(img_pdm_channel_enable_get);
+
+int img_pdm_channel_pulse_in_get(struct img_pdm_channel *chan)
+{
+	int val = 0;
+	struct img_pdm_device *pdm_dev;
+
+	if (!chan)
+		return -EINVAL;
+
+	pdm_dev = chan->pdm_dev;
+
+	if (!test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev, "pdm chan is notrequested\n");
+		return -EINVAL;
+	}
+
+	regmap_read(pdm_dev->periph_regs,
+		    CR_PERIP_PDM0_VAL +
+		    CR_PERIP_PDM_CH_ADDR_SHIFT(chan->pdm_id), &val);
+	val &= CR_PERIP_PDM_SRC_DATA_MASK;
+
+	return val;
+}
+EXPORT_SYMBOL_GPL(img_pdm_channel_pulse_in_get);
+
+int img_pdm_channel_enable_set(struct img_pdm_channel *chan, bool state)
+{
+	struct img_pdm_device *pdm_dev;
+
+	if (!chan)
+		return -EINVAL;
+
+	pdm_dev = chan->pdm_dev;
+
+	if (!test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev, "invalid pdm chan enable\n");
+		return -EINVAL;
+	}
+
+	if ((state == CR_PERIP_PDM_CHANNEL_ENABLE) &&
+		test_bit(PDM_CHANNEL_ENABLED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev, "pdm chan already enabled\n");
+		return -EBUSY;
+	} else if ((state == CR_PERIP_PDM_CHANNEL_DISABLE) &&
+		!test_bit(PDM_CHANNEL_ENABLED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev, "pdm chan not enabled\n");
+		return -EINVAL;
+	}
+
+	if (state == CR_PERIP_PDM_CHANNEL_ENABLE) {
+		regmap_update_bits(pdm_dev->periph_regs,
+				CR_PERIP_PWM_PDM_CONTROL,
+				CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
+				CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(chan->pdm_id),
+				1);
+		set_bit(PDM_CHANNEL_ENABLED, &chan->flags);
+	} else if (state == CR_PERIP_PDM_CHANNEL_DISABLE) {
+		regmap_update_bits(pdm_dev->periph_regs,
+				CR_PERIP_PWM_PDM_CONTROL,
+				CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
+				CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(chan->pdm_id),
+				0);
+		regmap_write(pdm_dev->periph_regs,
+			     CR_PERIP_PDM0_VAL +
+			     CR_PERIP_PDM_CH_ADDR_SHIFT(chan->pdm_id), 0);
+		clear_bit(PDM_CHANNEL_ENABLED, &chan->flags);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(img_pdm_channel_enable_set);
+
+int img_pdm_channel_pulse_in_set(struct img_pdm_channel *chan, unsigned int val)
+{
+	struct img_pdm_device *pdm_dev;
+
+	if (!chan)
+		return -EINVAL;
+
+	pdm_dev = chan->pdm_dev;
+
+	if (!test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+		dev_err(&pdm_dev->pdev->dev, "invalid pdm chan to configure\n");
+		return -EINVAL;
+	}
+
+	val = val & CR_PERIP_PDM_SRC_DATA_MASK;
+	regmap_write(pdm_dev->periph_regs,
+		     CR_PERIP_PDM0_VAL +
+		     CR_PERIP_PDM_CH_ADDR_SHIFT(chan->pdm_id), val);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(img_pdm_channel_pulse_in_set);
+
+static ssize_t img_pdm_enable_read(struct kobject *kobj,
+			struct kobj_attribute *attr, char *buf)
+{
+	int ret;
+	unsigned int val = 0;
+	unsigned int ch_num = 0;
+	unsigned char kobj_name[2];
+	struct platform_device *pdev;
+	struct img_pdm_device *pdm_dev;
+
+	pdev = to_platform_device(kobj_to_dev(kobj->parent));
+	pdm_dev = platform_get_drvdata(pdev);
+	kobj_name[0] = *(kobj->name+3);
+	kobj_name[1] = '\0';
+	ret = kstrtou32(kobj_name, 10, &ch_num);
+	if (ret) {
+		dev_err(&pdev->dev, "could not parse channel number string\n");
+		return ret;
+	}
+
+	val = img_pdm_channel_enable_get(&pdm_channels[ch_num]);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t img_pdm_pulse_in_read(struct kobject *kobj,
+			struct kobj_attribute *attr, char *buf)
+{
+	int ret;
+	unsigned int val;
+	unsigned int ch_num;
+	unsigned char kobj_name[2];
+	struct platform_device *pdev;
+	struct img_pdm_device *pdm_dev;
+
+	pdev = to_platform_device(kobj_to_dev(kobj->parent));
+	pdm_dev = platform_get_drvdata(pdev);
+	kobj_name[0] = *(kobj->name+3);
+	kobj_name[1] = '\0';
+	ret = kstrtou32(kobj_name, 10, &ch_num);
+	if (ret) {
+		dev_err(&pdev->dev, "could not parse channel number string\n");
+		return ret;
+	}
+
+	val = img_pdm_channel_pulse_in_get(&pdm_channels[ch_num]);
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t img_pdm_enable_write(struct kobject *kobj,
+					struct kobj_attribute *attr,
+						const char *buf, size_t size)
+{
+	int ret;
+	unsigned int enable;
+	unsigned int ch_num;
+	unsigned char kobj_name[2];
+	struct platform_device *pdev;
+	struct img_pdm_device *pdm_dev;
+
+	pdev = to_platform_device(kobj_to_dev(kobj->parent));
+	pdm_dev = platform_get_drvdata(pdev);
+	kobj_name[0] = *(kobj->name+3);
+	kobj_name[1] = '\0';
+	ret = kstrtou32(kobj_name, 10, &ch_num);
+	if (ret) {
+		dev_err(&pdev->dev, "could not parse channel number string\n");
+		return ret;
+	}
+
+	ret = kstrtou32(buf, 10, &enable);
+	if (ret) {
+		dev_err(&pdev->dev, "could not parse enable attr value\n");
+		return ret;
+	}
+
+	if (enable > CR_PERIP_PDM_CHANNEL_ENABLE) {
+		dev_err(&pdev->dev, "invalid enable attribute value\n");
+		return ret;
+	}
+
+	ret = img_pdm_channel_enable_set(&pdm_channels[ch_num],
+					 enable ? CR_PERIP_PDM_CHANNEL_ENABLE :
+					 CR_PERIP_PDM_CHANNEL_DISABLE);
+	if (ret < 0)
+		return ret;
+
+	return size;
+}
+
+static ssize_t img_pdm_pulse_in_write(struct kobject *kobj,
+		struct kobj_attribute *attr, const char *buf, size_t size)
+{
+	int ret;
+	unsigned int pulse_in;
+	unsigned int ch_num;
+	unsigned char kobj_name[2];
+	struct platform_device *pdev;
+	struct img_pdm_device *pdm_dev;
+
+	pdev = to_platform_device(kobj_to_dev(kobj->parent));
+	pdm_dev = platform_get_drvdata(pdev);
+	kobj_name[0] = *(kobj->name+3);
+	kobj_name[1] = '\0';
+	ret = kstrtou32(kobj_name, 10, &ch_num);
+	if (ret) {
+		dev_err(&pdev->dev, "could not parse channel number string\n");
+		return ret;
+	}
+
+	ret = kstrtouint(buf, 16, &pulse_in);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"could not parse pulse_in attr value\n");
+		return ret;
+	}
+
+	if (pulse_in > CR_PERIP_PDM_SRC_DATA_MASK) {
+		dev_err(&pdev->dev,
+			"invalid attr value for pulse_in string\n");
+		return -EINVAL;
+	}
+
+	ret = img_pdm_channel_pulse_in_set(&pdm_channels[ch_num], pulse_in);
+	if (ret < 0)
+		return ret;
+
+	return size;
+}
+
+#define PDM_ATTR(_name, _mode, _show, _store) \
+struct kobj_attribute pdm_attr_##_name = { \
+	.attr = {.name = __stringify(_name), .mode = _mode}, \
+	.show = _show, \
+	.store = _store, \
+}
+
+static PDM_ATTR(enable, S_IRUGO | S_IWUSR, img_pdm_enable_read,
+						img_pdm_enable_write);
+static PDM_ATTR(pulse_in, S_IRUGO | S_IWUSR, img_pdm_pulse_in_read,
+						img_pdm_pulse_in_write);
+
+static struct attribute *pdm_sysfs_attrs[] = {
+	&pdm_attr_enable.attr,
+	&pdm_attr_pulse_in.attr,
+	NULL,
+};
+
+static const struct attribute_group pdm_attr_group = {
+	.attrs = pdm_sysfs_attrs,
+};
+
+static ssize_t img_pdm_export(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	int ret;
+	unsigned int ch_num;
+	unsigned char buf1[5];
+	struct platform_device *pdev;
+	struct img_pdm_device *pdm_dev;
+
+	pdev = to_platform_device(dev);
+	pdm_dev = platform_get_drvdata(pdev);
+
+	ret = kstrtou32(buf, 10, &ch_num);
+	if (ret) {
+		dev_err(&pdev->dev, "could not parse channel number string\n");
+		return ret;
+	}
+
+	if (img_pdm_channel_request(ch_num) == NULL)
+		return -EINVAL;
+
+	memset(buf1, 0, sizeof(buf1));
+	sprintf(buf1, "pdm%d", ch_num);
+	pdm_dev->pdm_kobj[ch_num] = kobject_create_and_add(buf1,
+							&pdev->dev.kobj);
+	if (!pdm_dev->pdm_kobj[ch_num])
+		return -ENOMEM;
+
+	ret = sysfs_create_group(pdm_dev->pdm_kobj[ch_num], &pdm_attr_group);
+	if (ret) {
+		kobject_put(pdm_dev->pdm_kobj[ch_num]);
+		dev_err(&pdev->dev, "unable to register device attributes\n");
+		return ret;
+	}
+
+	return size;
+}
+
+static ssize_t img_pdm_unexport(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf, size_t size)
+{
+	int ret;
+	unsigned int ch_num;
+	struct img_pdm_channel *channel;
+	struct platform_device *pdev;
+	struct img_pdm_device *pdm_dev;
+
+	pdev = to_platform_device(dev);
+	pdm_dev = platform_get_drvdata(pdev);
+
+	ret = kstrtou32(buf, 10, &ch_num);
+	if (ret < 0)
+		return ret;
+
+	if (ch_num < 0 || ch_num >= IMG_NUM_PDM) {
+		dev_err(&pdev->dev, "Invalid channel number %d\n", ch_num);
+		return -EINVAL;
+	}
+
+	channel = &pdm_channels[ch_num];
+	if (img_pdm_channel_free(channel) < 0)
+		return -EINVAL;
+
+	if (pdm_dev->pdm_kobj[ch_num]) {
+		sysfs_remove_group(pdm_dev->pdm_kobj[ch_num], &pdm_attr_group);
+		kobject_put(pdm_dev->pdm_kobj[ch_num]);
+	}
+
+	return size;
+}
+
+/* The sysfs attributes. */
+static DEVICE_ATTR(export, S_IRUGO | S_IWUSR, NULL, img_pdm_export);
+static DEVICE_ATTR(unexport, S_IRUGO | S_IWUSR, NULL, img_pdm_unexport);
+
+static struct attribute *img_pdm_sysfs_attrs[] = {
+	&dev_attr_export.attr,
+	&dev_attr_unexport.attr,
+	NULL,
+};
+
+static const struct attribute_group img_pdm_attr_group = {
+	.attrs = img_pdm_sysfs_attrs,
+};
+
+static int img_pdm_probe(struct platform_device *pdev)
+{
+	int i, ret;
+	struct img_pdm_device *pdm_dev;
+
+	pdm_dev = devm_kzalloc(&pdev->dev,
+			sizeof(struct img_pdm_device), GFP_KERNEL);
+	if (!pdm_dev)
+		return -ENOMEM;
+
+	pdm_dev->pdm_kobj = devm_kzalloc(&pdev->dev,
+					 IMG_NUM_PDM *
+					 sizeof(struct img_pdm_device *),
+					 GFP_KERNEL);
+	if (!pdm_dev->pdm_kobj)
+		return -ENOMEM;
+
+	pdm_channels = devm_kzalloc(&pdev->dev,
+				    IMG_NUM_PDM *
+				    sizeof(struct img_pdm_channel), GFP_KERNEL);
+	if (!pdm_channels)
+		return -ENOMEM;
+
+	pdm_dev->periph_regs = syscon_regmap_lookup_by_phandle(
+					pdev->dev.of_node, "img,cr-periph");
+	if (IS_ERR(pdm_dev->periph_regs))
+		return PTR_ERR(pdm_dev->periph_regs);
+
+	pdm_dev->clk = devm_clk_get(&pdev->dev, "pdm");
+	if (IS_ERR(pdm_dev->clk)) {
+		dev_err(&pdev->dev, "failed to get pdm clock\n");
+		return PTR_ERR(pdm_dev->clk);
+	}
+
+	ret = clk_prepare_enable(pdm_dev->clk);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "could not prepare or enable pdm clock.\n");
+		return ret;
+	}
+
+	for (i = 0; i < IMG_NUM_PDM; i++) {
+		pdm_channels[i].pdm_id = i;
+		pdm_channels[i].pdm_dev = pdm_dev;
+	}
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &img_pdm_attr_group);
+	if (ret) {
+		dev_err(&pdev->dev, "unable to register device attributes\n");
+		clk_disable_unprepare(pdm_dev->clk);
+		return ret;
+	}
+
+	pdm_dev->pdev = pdev;
+	platform_set_drvdata(pdev, pdm_dev);
+
+	return 0;
+}
+
+static int img_pdm_remove(struct platform_device *pdev)
+{
+	int i;
+	struct img_pdm_channel *chan;
+	struct img_pdm_device *pdm_dev;
+
+	pdm_dev = platform_get_drvdata(pdev);
+
+	for (i = 0; i < IMG_NUM_PDM; i++) {
+		chan = &pdm_channels[i];
+		if (test_bit(PDM_CHANNEL_REQUESTED, &chan->flags)) {
+			img_pdm_channel_enable_set(chan,
+						CR_PERIP_PDM_CHANNEL_DISABLE);
+			img_pdm_channel_pulse_in_set(chan, 0);
+			if (pdm_dev->pdm_kobj[i]) {
+				sysfs_remove_group(pdm_dev->pdm_kobj[i],
+							&pdm_attr_group);
+				kobject_del(pdm_dev->pdm_kobj[i]);
+			}
+		}
+	}
+
+	clk_disable_unprepare(pdm_dev->clk);
+
+	return 0;
+}
+
+static const struct of_device_id img_pdm_of_match[] = {
+	{ .compatible = "img,pistachio-pdm", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, img_pdm_of_match);
+
+static struct platform_driver img_pdm_driver = {
+	.driver = {
+		.name = "imgtec-pdm",
+		.of_match_table = img_pdm_of_match,
+	},
+	.probe = img_pdm_probe,
+	.remove = img_pdm_remove,
+};
+module_platform_driver(img_pdm_driver);
+
+MODULE_AUTHOR("Arul Ramasamy <Arul.Ramasamy@imgtec.com>");
+MODULE_DESCRIPTION("Imagination Technologies PDM driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/img_pdm.h b/include/linux/img_pdm.h
new file mode 100644
index 0000000..0203951
--- /dev/null
+++ b/include/linux/img_pdm.h
@@ -0,0 +1,35 @@
+/**
+ * Imagination Technologies Pulse Density Modulator driver
+ *
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ *
+ * 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.
+ */
+
+#ifndef __IMG_PDM_H
+#define __IMG_PDM_H
+
+struct img_pdm_device {
+	struct clk *clk;
+	struct kobject **pdm_kobj;
+	struct regmap *periph_regs;
+	struct platform_device *pdev;
+};
+
+struct img_pdm_channel {
+	unsigned int pdm_id;
+	unsigned long flags;
+	struct img_pdm_device *pdm_dev;
+};
+
+void img_pdm_channel_put(struct device *dev);
+struct img_pdm_channel *img_pdm_channel_get(struct device *dev);
+int img_pdm_channel_enable_set(struct img_pdm_channel *chan, bool state);
+int img_pdm_channel_enable_get(struct img_pdm_channel *chan);
+int img_pdm_channel_pulse_in_get(struct img_pdm_channel *chan);
+int img_pdm_channel_pulse_in_set(struct img_pdm_channel *chan,
+				 unsigned int val);
+
+#endif /* ifndef _IMG_PDM_H */
-- 
1.7.0.4

[PATCH RESEND v4 4/4] DT: pdm: Add binding document for IMG PDM DAC

[naidu.tellapati]

From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

Add binding document for the Pulse Density Modulator (PDM) DAC present on the
Pistachio SOC.

Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
Signed-off-by: Arul Ramasamy <Arul.Ramasamy@imgtec.com>
---
 Documentation/devicetree/bindings/misc/img-pdm.txt |   54 ++++++++++++++++++++
 1 files changed, 54 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/misc/img-pdm.txt

diff --git a/Documentation/devicetree/bindings/misc/img-pdm.txt b/Documentation/devicetree/bindings/misc/img-pdm.txt
new file mode 100644
index 0000000..730d38a
--- /dev/null
+++ b/Documentation/devicetree/bindings/misc/img-pdm.txt
@@ -0,0 +1,54 @@
+*Imagination Technologies Pulse Density Modulator (PDM) DAC.
+
+Required properties:
+  - compatible: Must be "img,pistachio-pdm"
+  - clocks: Must contain an entry for each entry in clock-names.
+  - clock-names: Must include the following entry:
+	Required elements: "pdm"
+  - img,cr-periph: Must contain a phandle to the peripheral control
+	syscon node which contains PDM control registers.
+  - #pdm-cells: Must be 2.
+  - The first cell is the PDM channel number (valid values: 0, 1, 2, 3)
+  - The second cell is 12-bit pulse-in value
+
+Specifying PDM information for devices
+======================================
+
+1. PDM User nodes
+
+PDM properties should be named "pdms". The exact meaning of each pdms property
+is described above.
+
+	pdm-specifier : array of #pdm-cells specifying the given PDM
+						(controller specific)
+
+The following example could be used to describe a PDM-based backlight device:
+
+	pdm: pdm {
+		#pdm-cells = <2>;
+	};
+
+	[...]
+
+	bl: backlight {
+		pdms = <&pdm 2 0>;
+	};
+
+pdm-specifier typically encodes the chip-relative PDM channel number and the
+12-bit pulse-in value.
+
+2. PDM Controller nodes
+
+PDM controller nodes must specify the number of cells used for the specifier
+using the '#pdm-cells' property.
+
+An example PDM controller might look like this:
+
+Example:
+	pdm: pdm@18148000 {
+		compatible = "img,pistachio-pdm";
+		clocks = <&pdm_clk>;
+		clk-names = "pdm";
+		img,cr-periph = <&cr_periph>;
+		#pdm-cells = <2>;
+	};
-- 
1.7.0.4

Re: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Thierry Reding]

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

On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote:
> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> 
> The Pistachio SOC from Imagination Technologies includes a Pulse Width
> Modulation DAC which produces 1 to 4 digital bit-outputs which represent
> digital waveforms. These PWM outputs are primarily in charge of controlling
> backlight LED devices.
> 
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
> ---
>  drivers/pwm/Kconfig   |   12 ++
>  drivers/pwm/Makefile  |    1 +
>  drivers/pwm/pwm-img.c |  270 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 283 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/pwm/pwm-img.c
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index ef2dd2e..6b4581a 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -110,6 +110,18 @@ config PWM_FSL_FTM
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called pwm-fsl-ftm.
>  
> +config PWM_IMG

This sounds really generic to me. Basically this says that every PWM IP
developed by Imagination Technologies will be compatible with this one.
It's typical to name modules after <vendor>-<soc> to avoid this type of
ambiguity.

Is there any reason why this can't be called PWM_IMG_PISTACHIO?

> +	tristate "Imagination Technologies PWM driver"
> +	depends on MFD_SYSCON
> +	depends on HAS_IOMEM

I think you'll need at least COMMON_CLK here as well.

> diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c
[...]
> +/* PWM registers */
> +#define CR_PWM_CTRL_CFG				0x0000
> +#define CR_PWM_CTRL_CFG_NO_SUB_DIV		0
> +#define CR_PWM_CTRL_CFG_SUB_DIV0		1
> +#define CR_PWM_CTRL_CFG_SUB_DIV1		2
> +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1		3
> +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch)		((ch) * 2 + 4)
> +#define CR_PWM_CTRL_CFG_DIV_MASK		0x3
> +
> +#define CR_PWM_CH_CFG(ch)			(0x4 + (ch) * 4)
> +#define CR_PWM_CH_CFG_TMBASE_SHIFT		0
> +#define CR_PWM_CH_CFG_DUTY_SHIFT		16

What's with the CR_ prefix here? What does it stand for? Can't you just
drop it?

> +#define CR_PERIP_PWM_PDM_CONTROL		0x0140
> +#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK	0x1
> +#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch)	((ch) * 4)
> +
> +#define IMG_NUM_PWM				4

I don't think you need this. See below for more details.

> +static inline void img_pwm_writel(struct img_pwm_chip *chip,
> +				  unsigned int reg, unsigned long val)
> +{
> +	writel(val, chip->base + reg);
> +}
> +
> +static inline unsigned int img_pwm_readl(struct img_pwm_chip *chip,
> +					 unsigned int reg)
> +{
> +	return readl(chip->base + reg);
> +}

readl() and writel() deal with u32 data, please use consistent types.

> +static int img_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
> +			  int duty_ns, int period_ns)
> +{
> +	unsigned int div;
> +	unsigned int duty_steps;
> +	unsigned int tmbase_steps;
> +	unsigned long val;
> +	unsigned long mul;
> +	unsigned long output_clk_hz;
> +	unsigned long input_clk_hz;

Many of these can be folded into single lines. And again, val is used to
store register contents and should be u32.

> +	struct img_pwm_chip *pwm_chip;
> +
> +	pwm_chip = to_img_pwm_chip(chip);
> +
> +	input_clk_hz = clk_get_rate(pwm_chip->pwm_clk);
> +	output_clk_hz = DIV_ROUND_UP(NSEC_PER_SEC, period_ns);
> +
> +	mul = DIV_ROUND_UP(input_clk_hz, output_clk_hz);
> +	if (mul > MAX_TMBASE_STEPS * 512) {
> +		dev_err(chip->dev,
> +			"failed to configure timebase steps/divider value.\n");
> +		return -EINVAL;
> +	}

I think it'd be more readable if this was the final else in the block of
if/else if below.

> +
> +	if (mul <= MAX_TMBASE_STEPS) {
> +		div = CR_PWM_CTRL_CFG_NO_SUB_DIV;
> +		tmbase_steps = DIV_ROUND_UP(mul, 1);
> +	} else if (mul <= MAX_TMBASE_STEPS * 8) {
> +		div = CR_PWM_CTRL_CFG_SUB_DIV0;
> +		tmbase_steps = DIV_ROUND_UP(mul, 8);
> +	} else if (mul <= MAX_TMBASE_STEPS * 64) {
> +		div = CR_PWM_CTRL_CFG_SUB_DIV1;
> +		tmbase_steps = DIV_ROUND_UP(mul, 64);
> +	} else if (mul <= MAX_TMBASE_STEPS * 512) {
> +		div = CR_PWM_CTRL_CFG_SUB_DIV0_DIV1;
> +		tmbase_steps = DIV_ROUND_UP(mul, 512);
> +	}
> +
> +	duty_steps = DIV_ROUND_UP(tmbase_steps * duty_ns, period_ns);
> +
> +	val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
> +	val &= ~(CR_PWM_CTRL_CFG_DIV_MASK <<
> +					CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm));
> +	val |= (div & CR_PWM_CTRL_CFG_DIV_MASK) <<
> +					CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm);

If you leave out the CR_ prefix these will actually fit on a single
line.

> +	img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
> +
> +	val = (duty_steps << CR_PWM_CH_CFG_DUTY_SHIFT) |
> +				(tmbase_steps << CR_PWM_CH_CFG_TMBASE_SHIFT);

I prefer subsequent lines to be aligned with the first, like so:

	val = (duty_steps ...) |
	      (tmbase_steps ...);

Also I'd leave out the _steps suffix, it doesn't really add information.

> +static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	unsigned int val;

Should be u32 as well. There are other occurrences like this in the
remainder of the driver, but I haven't commented on all of them
explicitly. Please fix them all up to be consistent.

> +	struct img_pwm_chip *pwm_chip;
> +
> +	pwm_chip = to_img_pwm_chip(chip);

The above can be a single line.

> +
> +	val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
> +	val |= BIT(pwm->hwpwm);
> +	img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
> +
> +	regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
> +			   CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
> +			   CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);

This smells like pinmux and should probably be a separate driver.

> +static void img_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
> +{
> +	unsigned int val;
> +	struct img_pwm_chip *pwm_chip;
> +
> +	pwm_chip = to_img_pwm_chip(chip);
> +
> +	val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
> +	val &= ~BIT(pwm->hwpwm);
> +	img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
> +
> +	regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
> +			   CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
> +			   CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 1);
> +}

Same comments as for img_pwm_enable().

> +static int img_pwm_probe(struct platform_device *pdev)
> +{
[...]
> +	pwm->chip.dev = &pdev->dev;
> +	pwm->chip.ops = &img_pwm_ops;
> +	pwm->chip.base = -1;
> +	pwm->chip.npwm = IMG_NUM_PWM;

You can directly substitute 4 here since it's the only place you need
it.

> +static int img_pwm_remove(struct platform_device *pdev)
> +{
> +	unsigned int i;
> +	unsigned int val;
> +
> +	struct img_pwm_chip *pwm_chip = platform_get_drvdata(pdev);

This should go at the very top of the variable declarations.

> +
> +	for (i = 0; i < IMG_NUM_PWM; i++) {

This would better be pwm_chip->chip.npwm.

Thierry

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

Re: [PATCH RESEND v4 2/4] DT: pwm: Add binding document for IMG PWM DAC

[Thierry Reding]

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

On Sat, Nov 22, 2014 at 07:23:30AM +0530, naidu.tellapati@gmail.com wrote:
> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> 
> Add binding document for IMG Pulse Width Modulator (PWM) DAC present on the
> Pistachio SOC. The PWM DAC has four channels.
> 
> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
> Reviewed-by: Andrew Bresticker <abrestic@chromium.org>
> ---
> No changes from v3.
> ---
>  Documentation/devicetree/bindings/pwm/img-pwm.txt |   23 +++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/pwm/img-pwm.txt
> 
> diff --git a/Documentation/devicetree/bindings/pwm/img-pwm.txt b/Documentation/devicetree/bindings/pwm/img-pwm.txt
> new file mode 100644
> index 0000000..d3828dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pwm/img-pwm.txt
> @@ -0,0 +1,23 @@
> +*Imagination Technologies PWM DAC driver
> +
> +Required properties:
> +  - compatible: Should be "img,pistachio-pwm"
> +  - reg: Should contain physical base address and length of pwm registers.
> +  - clocks: Must contain an entry for each entry in clock-names.
> +	See ../clock/clock-bindings.txt for details.
> +  - clock-names: input clock names.
> +	Required elements: "pwm" and "sys".

I think it'd be useful to explain at least what the "sys" clock is.
"pwm" I'd assume that it clocks the IP block, but what does "sys" do?

Thierry

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

RE: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Naidu Tellapati]

Hi Thierry,

Many thanks for the review.

> On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote:
>> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>>
>> The Pistachio SOC from Imagination Technologies includes a Pulse Width
>> Modulation DAC which produces 1 to 4 digital bit-outputs which represent
>> digital waveforms. These PWM outputs are primarily in charge of controlling
>> backlight LED devices.
>>
>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
>> ---
>>  drivers/pwm/Kconfig   |   12 ++
>>  drivers/pwm/Makefile  |    1 +
>>  drivers/pwm/pwm-img.c |  270 +++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 283 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/pwm/pwm-img.c
>>
>> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> index ef2dd2e..6b4581a 100644
>> --- a/drivers/pwm/Kconfig
>> +++ b/drivers/pwm/Kconfig
>> @@ -110,6 +110,18 @@ config PWM_FSL_FTM
>>         To compile this driver as a module, choose M here: the module
>>         will be called pwm-fsl-ftm.
>>
>> +config PWM_IMG

> This sounds really generic to me. Basically this says that every PWM IP
> developed by Imagination Technologies will be compatible with this one.
> It's typical to name modules after <vendor>-<soc> to avoid this type of
> ambiguity.

> Is there any reason why this can't be called PWM_IMG_PISTACHIO?

At this moment, this IP Block is available at least on one more SOC from Imagination Technologies.
It is possible that the IP will be available on more SOCs in future.

Shall we go ahead with PWM_IMG?

>> +     tristate "Imagination Technologies PWM driver"
>> +     depends on MFD_SYSCON
>> +     depends on HAS_IOMEM

> I think you'll need at least COMMON_CLK here as well.

OK. Will address in the next Patch set.

>> diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c
[...]
>> +/* PWM registers */
>> +#define CR_PWM_CTRL_CFG                              0x0000
>> +#define CR_PWM_CTRL_CFG_NO_SUB_DIV           0
>> +#define CR_PWM_CTRL_CFG_SUB_DIV0             1
>> +#define CR_PWM_CTRL_CFG_SUB_DIV1             2
>> +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1                3
>> +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch)                ((ch) * 2 + 4)
>> +#define CR_PWM_CTRL_CFG_DIV_MASK             0x3
>> +
>> +#define CR_PWM_CH_CFG(ch)                    (0x4 + (ch) * 4)
>> +#define CR_PWM_CH_CFG_TMBASE_SHIFT           0
>> +#define CR_PWM_CH_CFG_DUTY_SHIFT             16

> What's with the CR_ prefix here? What does it stand for? Can't you just
> drop it?

CR stands for Control Register. We have picked it from the datasheet and wanted to make the
register names compatible with the same in the datasheet.

Hope Andrew agrees with Thierry's comments here. Hope we can drop it. 

Andrew, any comments here, please.

>> +#define CR_PERIP_PWM_PDM_CONTROL             0x0140
>> +#define CR_PERIP_PWM_PDM_CONTROL_CH_MASK     0x1
>> +#define CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(ch)        ((ch) * 4)
>> +
>> +#define IMG_NUM_PWM                          4

> I don't think you need this. See below for more details.

OK. Will address in the next patch set.

>> +static inline void img_pwm_writel(struct img_pwm_chip *chip,
>> +                               unsigned int reg, unsigned long val)
>> +{
>> +     writel(val, chip->base + reg);
>> +}
>> +
>> +static inline unsigned int img_pwm_readl(struct img_pwm_chip *chip,
>> +                                      unsigned int reg)
>> +{
>> +     return readl(chip->base + reg);
>> +}

> readl() and writel() deal with u32 data, please use consistent types.

OK. Will address in the next patch set.

>> +static int img_pwm_config(struct pwm_chip *chip, struct pwm_device *pwm,
>> +                       int duty_ns, int period_ns)
>> +{
>> +     unsigned int div;
>> +     unsigned int duty_steps;
>> +     unsigned int tmbase_steps;
>> +     unsigned long val;
>> +     unsigned long mul;
>> +     unsigned long output_clk_hz;
>> +     unsigned long input_clk_hz;

> Many of these can be folded into single lines. And again, val is used to
> store register contents and should be u32.

Agree. Will address in the next patch set.

>> +     struct img_pwm_chip *pwm_chip;
>> +
>> +     pwm_chip = to_img_pwm_chip(chip);
>> +
>> +     input_clk_hz = clk_get_rate(pwm_chip->pwm_clk);
>> +     output_clk_hz = DIV_ROUND_UP(NSEC_PER_SEC, period_ns);
>> +
>> +     mul = DIV_ROUND_UP(input_clk_hz, output_clk_hz);
>> +     if (mul > MAX_TMBASE_STEPS * 512) {
>> +             dev_err(chip->dev,
>> +                     "failed to configure timebase steps/divider value.\n");
>> +             return -EINVAL;
>> +     }

> I think it'd be more readable if this was the final else in the block of
> if/else if below.

OK. Will address in the next patch set.

>> +
>> +     if (mul <= MAX_TMBASE_STEPS) {
>> +             div = CR_PWM_CTRL_CFG_NO_SUB_DIV;
>> +             tmbase_steps = DIV_ROUND_UP(mul, 1);
>> +     } else if (mul <= MAX_TMBASE_STEPS * 8) {
>> +             div = CR_PWM_CTRL_CFG_SUB_DIV0;
>> +             tmbase_steps = DIV_ROUND_UP(mul, 8);
>> +     } else if (mul <= MAX_TMBASE_STEPS * 64) {
>> +             div = CR_PWM_CTRL_CFG_SUB_DIV1;
>> +             tmbase_steps = DIV_ROUND_UP(mul, 64);
>> +     } else if (mul <= MAX_TMBASE_STEPS * 512) {
>> +             div = CR_PWM_CTRL_CFG_SUB_DIV0_DIV1;
>> +             tmbase_steps = DIV_ROUND_UP(mul, 512);
>> +     }
>> +
>> +     duty_steps = DIV_ROUND_UP(tmbase_steps * duty_ns, period_ns);
>> +
>> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>> +     val &= ~(CR_PWM_CTRL_CFG_DIV_MASK <<
>> +                                     CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm));
>> +     val |= (div & CR_PWM_CTRL_CFG_DIV_MASK) <<
>> +                                     CR_PWM_CTRL_CFG_DIV_SHIFT(pwm->hwpwm);

> If you leave out the CR_ prefix these will actually fit on a single
> line.

OK. Will address in the next patch set.

>> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>> +
>> +     val = (duty_steps << CR_PWM_CH_CFG_DUTY_SHIFT) |
>> +                             (tmbase_steps << CR_PWM_CH_CFG_TMBASE_SHIFT);

> I prefer subsequent lines to be aligned with the first, like so:

OK. Will address in the next patch set.

>        val = (duty_steps ...) |
>              (tmbase_steps ...);

> Also I'd leave out the _steps suffix, it doesn't really add information.

OK. Will address in the next patch set.

>> +static int img_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     unsigned int val;

> Should be u32 as well. There are other occurrences like this in the
> remainder of the driver, but I haven't commented on all of them
> explicitly. Please fix them all up to be consistent.

OK. Will fix the issue at all the places in the driver.

>> +     struct img_pwm_chip *pwm_chip;
>> +
>> +     pwm_chip = to_img_pwm_chip(chip);

> The above can be a single line.

OK. Will address in the next patch set.

>> +
>> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>> +     val |= BIT(pwm->hwpwm);
>> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>> +
>> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);

> This smells like pinmux and should probably be a separate driver.

Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
and call the pin muxing driver APIs from here.

Please correct me if my understanding is wrong? 

>> +static void img_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
>> +{
>> +     unsigned int val;
>> +     struct img_pwm_chip *pwm_chip;
>> +
>> +     pwm_chip = to_img_pwm_chip(chip);
>> +
>> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>> +     val &= ~BIT(pwm->hwpwm);
>> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>> +
>> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 1);
>> +}

> Same comments as for img_pwm_enable().

OK.

>> +static int img_pwm_probe(struct platform_device *pdev)
>> +{
[...]
>> +     pwm->chip.dev = &pdev->dev;
>> +     pwm->chip.ops = &img_pwm_ops;
>> +     pwm->chip.base = -1;
>> +     pwm->chip.npwm = IMG_NUM_PWM;

> You can directly substitute 4 here since it's the only place you need
> it.

OK. Will address in the next patch set.

>> +static int img_pwm_remove(struct platform_device *pdev)
>> +{
>> +     unsigned int i;
>> +     unsigned int val;
>> +
>> +     struct img_pwm_chip *pwm_chip = platform_get_drvdata(pdev);

> This should go at the very top of the variable declarations.

OK. Will address in the next patch set.

>> +
>> +     for (i = 0; i < IMG_NUM_PWM; i++) {

> This would better be pwm_chip->chip.npwm.

OK. Will address in the next patch set.

> Thierry

Thanks and regards,
Naidu.

RE: [PATCH RESEND v4 2/4] DT: pwm: Add binding document for IMG PWM DAC

[Naidu Tellapati]

Hi Thierry,

Many thanks for your review comments.

> On Sat, Nov 22, 2014 at 07:23:30AM +0530, naidu.tellapati@gmail.com wrote:
>> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>>
>> Add binding document for IMG Pulse Width Modulator (PWM) DAC present on the
>> Pistachio SOC. The PWM DAC has four channels.
>>
>> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
>> Reviewed-by: Andrew Bresticker <abrestic@chromium.org>
>> ---
>> No changes from v3.
>> ---
>>  Documentation/devicetree/bindings/pwm/img-pwm.txt |   23 +++++++++++++++++++++
>>  1 files changed, 23 insertions(+), 0 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/pwm/img-pwm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/pwm/img-pwm.txt b/Documentation/devicetree/bindings/pwm/img-pwm.txt
>> new file mode 100644
>> index 0000000..d3828dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/pwm/img-pwm.txt
>> @@ -0,0 +1,23 @@
>> +*Imagination Technologies PWM DAC driver
>> +
>> +Required properties:
>> +  - compatible: Should be "img,pistachio-pwm"
>> +  - reg: Should contain physical base address and length of pwm registers.
>> +  - clocks: Must contain an entry for each entry in clock-names.
>> +     See ../clock/clock-bindings.txt for details.
>> +  - clock-names: input clock names.
>> +     Required elements: "pwm" and "sys".

> I think it'd be useful to explain at least what the "sys" clock is.
> "pwm" I'd assume that it clocks the IP block, but what does "sys" do?

OK. Will add the description in the next patch set.

The sys clock enables access to Internal configuration registers of PWM block.


> Thierry

Regards,
Naidu.

Re: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Thierry Reding]

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

On Mon, Nov 24, 2014 at 11:22:31AM +0000, Naidu Tellapati wrote:
> Hi Thierry,
> 
> Many thanks for the review.
> 
> > On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote:
> >> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> >>
> >> The Pistachio SOC from Imagination Technologies includes a Pulse Width
> >> Modulation DAC which produces 1 to 4 digital bit-outputs which represent
> >> digital waveforms. These PWM outputs are primarily in charge of controlling
> >> backlight LED devices.
> >>
> >> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
> >> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
> >> ---
> >>  drivers/pwm/Kconfig   |   12 ++
> >>  drivers/pwm/Makefile  |    1 +
> >>  drivers/pwm/pwm-img.c |  270 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 283 insertions(+), 0 deletions(-)
> >>  create mode 100644 drivers/pwm/pwm-img.c
> >>
> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> >> index ef2dd2e..6b4581a 100644
> >> --- a/drivers/pwm/Kconfig
> >> +++ b/drivers/pwm/Kconfig
> >> @@ -110,6 +110,18 @@ config PWM_FSL_FTM
> >>         To compile this driver as a module, choose M here: the module
> >>         will be called pwm-fsl-ftm.
> >>
> >> +config PWM_IMG
> 
> > This sounds really generic to me. Basically this says that every PWM IP
> > developed by Imagination Technologies will be compatible with this one.
> > It's typical to name modules after <vendor>-<soc> to avoid this type of
> > ambiguity.
> 
> > Is there any reason why this can't be called PWM_IMG_PISTACHIO?
> 
> At this moment, this IP Block is available at least on one more SOC from Imagination Technologies.
> It is possible that the IP will be available on more SOCs in future.
> 
> Shall we go ahead with PWM_IMG?

Alright, if ever there's a different PWM IP block from Imagination
Technologies, the driver for that can have a more specific name.

> >> +
> >> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
> >> +     val |= BIT(pwm->hwpwm);
> >> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
> >> +
> >> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
> >> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
> >> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);
> 
> > This smells like pinmux and should probably be a separate driver.
> 
> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
> and call the pin muxing driver APIs from here.
> 
> Please correct me if my understanding is wrong?

I don't think you need to call the pinmux API from here. Rather I'll
assume that the muxing is fixed on a given board, so this configuration
would be part of the static board-level pinmux so it will automatically
be applied at boot time.

Thierry

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

RE: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Naidu Tellapati]

Hi James,

> On Mon, Nov 24, 2014 at 11:22:31AM +0000, Naidu Tellapati wrote:
>> Hi Thierry,
>>
>> Many thanks for the review.
>>
>> > On Sat, Nov 22, 2014 at 07:23:29AM +0530, naidu.tellapati@gmail.com wrote:
>> >> From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>> >>
>> >> The Pistachio SOC from Imagination Technologies includes a Pulse Width
>> >> Modulation DAC which produces 1 to 4 digital bit-outputs which represent
>> >> digital waveforms. These PWM outputs are primarily in charge of controlling
>> >> backlight LED devices.
>> >>
>> >> Signed-off-by: Naidu Tellapati <Naidu.Tellapati@imgtec.com>
>> >> Signed-off-by: Sai Masarapu <Sai.Masarapu@imgtec.com>
>> >> ---
>>> >>  drivers/pwm/Kconfig   |   12 ++
>> >>  drivers/pwm/Makefile  |    1 +
>> >>  drivers/pwm/pwm-img.c |  270 +++++++++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 283 insertions(+), 0 deletions(-)
>> >>  create mode 100644 drivers/pwm/pwm-img.c
>> >>
>> >> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
>> >> index ef2dd2e..6b4581a 100644
>> >> --- a/drivers/pwm/Kconfig
>> >> +++ b/drivers/pwm/Kconfig
>> >> @@ -110,6 +110,18 @@ config PWM_FSL_FTM
>> >>         To compile this driver as a module, choose M here: the module
>> >>         will be called pwm-fsl-ftm.
>> >>
>> >> +config PWM_IMG
>>
>> > This sounds really generic to me. Basically this says that every PWM IP
>> > developed by Imagination Technologies will be compatible with this one.
>> > It's typical to name modules after <vendor>-<soc> to avoid this type of
>> > ambiguity.
>>
>> > Is there any reason why this can't be called PWM_IMG_PISTACHIO?
>>
>> At this moment, this IP Block is available at least on one more SOC from Imagination Technologies.
>> It is possible that the IP will be available on more SOCs in future.
>>
>> Shall we go ahead with PWM_IMG?

> Alright, if ever there's a different PWM IP block from Imagination
> Technologies, the driver for that can have a more specific name.

>> >> +
>> >> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>> >> +     val |= BIT(pwm->hwpwm);
>> >> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>> >> +
>> >> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
>> >> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
>> >> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);
>>
>> > This smells like pinmux and should probably be a separate driver.
>>
>> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
>> and call the pin muxing driver APIs from here.
>>
>> Please correct me if my understanding is wrong?

> I don't think you need to call the pinmux API from here. Rather I'll
> assume that the muxing is fixed on a given board, so this configuration
> would be part of the static board-level pinmux so it will automatically
> be applied at boot time.

Could you please suggest us on the output pin muxing between PWM & PDM.

As of now we are configuring the mux control register from the PWM & PDM drivers.

Regards,
Naidu.

Re: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Andrew Bresticker]

>>> diff --git a/drivers/pwm/pwm-img.c b/drivers/pwm/pwm-img.c
> [...]
>>> +/* PWM registers */
>>> +#define CR_PWM_CTRL_CFG                              0x0000
>>> +#define CR_PWM_CTRL_CFG_NO_SUB_DIV           0
>>> +#define CR_PWM_CTRL_CFG_SUB_DIV0             1
>>> +#define CR_PWM_CTRL_CFG_SUB_DIV1             2
>>> +#define CR_PWM_CTRL_CFG_SUB_DIV0_DIV1                3
>>> +#define CR_PWM_CTRL_CFG_DIV_SHIFT(ch)                ((ch) * 2 + 4)
>>> +#define CR_PWM_CTRL_CFG_DIV_MASK             0x3
>>> +
>>> +#define CR_PWM_CH_CFG(ch)                    (0x4 + (ch) * 4)
>>> +#define CR_PWM_CH_CFG_TMBASE_SHIFT           0
>>> +#define CR_PWM_CH_CFG_DUTY_SHIFT             16
>
>> What's with the CR_ prefix here? What does it stand for? Can't you just
>> drop it?
>
> CR stands for Control Register. We have picked it from the datasheet and wanted to make the
> register names compatible with the same in the datasheet.
>
> Hope Andrew agrees with Thierry's comments here. Hope we can drop it.
>
> Andrew, any comments here, please.

I didn't think the CR_ prefix was totally necessary, but it matches
what's in the TRM, so I didn't have an issue with it.  I'm fine with
dropping it.

Re: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Andrew Bresticker]

>> >> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>> >> +     val |= BIT(pwm->hwpwm);
>> >> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>> >> +
>> >> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
>> >> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
>> >> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);
>>
>> > This smells like pinmux and should probably be a separate driver.
>>
>> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
>> and call the pin muxing driver APIs from here.
>>
>> Please correct me if my understanding is wrong?
>
> I don't think you need to call the pinmux API from here. Rather I'll
> assume that the muxing is fixed on a given board, so this configuration
> would be part of the static board-level pinmux so it will automatically
> be applied at boot time.

The issue here is that this register does not belong to the
pinmux/pinctrl block on this SoC and instead is part of the
"peripheral control" block which primarily provides system interface
gate clocks and resets (for which I was planning to write a clock
provider driver), but also has a bunch of control registers for
various IP blocks thrown together (like this one) which we're using
syscon to access.  I'm not sure it makes sense to create a pinmux
driver for these 4 bits, but I'll defer to others on that...

Another option would be to have the main pinctrl driver for Pistachio
control these bits, but that doesn't seem quite right since these bits
are completely separate from the pinctrl block.

Re: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[James Hartley]

On 24 Nov 2014, at 18:19, Andrew Bresticker <abrestic@chromium.org> wrote:

>>>>> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>>>>> +     val |= BIT(pwm->hwpwm);
>>>>> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>>>>> +
>>>>> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
>>>>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
>>>>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);
>>> 
>>>> This smells like pinmux and should probably be a separate driver.
>>> 
>>> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
>>> and call the pin muxing driver APIs from here.
>>> 
>>> Please correct me if my understanding is wrong?
>> 
>> I don't think you need to call the pinmux API from here. Rather I'll
>> assume that the muxing is fixed on a given board, so this configuration
>> would be part of the static board-level pinmux so it will automatically
>> be applied at boot time.
> 
> The issue here is that this register does not belong to the
> pinmux/pinctrl block on this SoC and instead is part of the
> "peripheral control" block which primarily provides system interface
> gate clocks and resets (for which I was planning to write a clock
> provider driver), but also has a bunch of control registers for
> various IP blocks thrown together (like this one) which we're using
> syscon to access.  I'm not sure it makes sense to create a pinmux
> driver for these 4 bits, but I'll defer to others on that...
> 
> Another option would be to have the main pinctrl driver for Pistachio
> control these bits, but that doesn't seem quite right since these bits
> are completely separate from the pinctrl block.

I would think the pinctrl driver should deal with this - yes it's a separate set of registers, but the config is expected to be static and can be represented in the DT.

James. 

RE: [PATCH RESEND v4 1/4] pwm: Imagination Technologies PWM DAC driver

[Naidu Tellapati]

Hi James, Andrew, Thierry and all,


> On 24 Nov 2014, at 18:19, Andrew Bresticker <abrestic@chromium.org> wrote:

>>>>>> +     val = img_pwm_readl(pwm_chip, CR_PWM_CTRL_CFG);
>>>>>> +     val |= BIT(pwm->hwpwm);
>>>>>> +     img_pwm_writel(pwm_chip, CR_PWM_CTRL_CFG, val);
>>>>>> +
>>>>>> +     regmap_update_bits(pwm_chip->periph_regs, CR_PERIP_PWM_PDM_CONTROL,
>>>>>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_MASK <<
>>>>>> +                        CR_PERIP_PWM_PDM_CONTROL_CH_SHIFT(pwm->hwpwm), 0);
>>>>
>>>>> This smells like pinmux and should probably be a separate driver.
>>>>
>>>> Hope you are suggesting us to have a separate driver for pin muxing (might already have one for Pistachio)
>>>> and call the pin muxing driver APIs from here.
>>>>
>>>> Please correct me if my understanding is wrong?
>>>
>>> I don't think you need to call the pinmux API from here. Rather I'll
>>> assume that the muxing is fixed on a given board, so this configuration
>>> would be part of the static board-level pinmux so it will automatically
>>> be applied at boot time.
>>
>> The issue here is that this register does not belong to the
>> pinmux/pinctrl block on this SoC and instead is part of the
>> "peripheral control" block which primarily provides system interface
>> gate clocks and resets (for which I was planning to write a clock
>> provider driver), but also has a bunch of control registers for
>> various IP blocks thrown together (like this one) which we're using
>> syscon to access.  I'm not sure it makes sense to create a pinmux
>> driver for these 4 bits, but I'll defer to others on that...
>>
>> Another option would be to have the main pinctrl driver for Pistachio
>> control these bits, but that doesn't seem quite right since these bits
>> are completely separate from the pinctrl block.

> I would think the pinctrl driver should deal with this - yes it's a separate set of registers,
> but the config is expected to be static and can be represented in the DT.

> James.

I have discussed with the hardware team one more time today and discovered that
the value we configure to CR_PERIP_PWM_PDM_CONTROL not only configures PWM/PDM
output mux, but also enables control signal for PDM blocks.

As an example, if the PDM user wants to enable PDM block 2, he needs to set bit 8 to 1 in the
register CR_PERIP_PWM_PDM_CONTROL. Please also note that setting bit 8  to 1 not only
configures the output PWM/PDM mux, but also enables control signal for PDM block 2.
He needs access to this register from the PDM driver at least to configure (enable/disable)
PDM control signal.

And also because of the reasons explained by Andrew above, I think it is better to use syscon
(instead of handling it in Pinctrl driver) to access this register both in PWM and PDM drivers.

Please let us know comments and suggestions.

Thanks and regards,
Naidu.

[PATCH RESEND v4 0/4] Initial support for ImgTec PWM DAC and PDM DAC

[Naidu.Tellapati]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2629 bytes --]

From: Naidu Tellapati <Naidu.Tellapati@imgtec.com>

The Pistachio SOC from Imagination Technologies includes ��������� 4x PDM blocks
and 1x ���������basic PWM��������� block with four output channels. These share 4 MFIO pins,
each PWM channel being muxed with each PDM block output (0-3 muxed with 0-3)
using the CR_PERIP_PWM_PDM_CONTROL register in the peripheral wrapper
register bank (offset 0x140). The PDM and PWM outputs are used to control
targets such as LCD backlight.

The PWM driver uses Linux PWM framework. Since the PDM does not fit into PWM
framework we have implemented it as a MISC driver.

The series is based on v3.18-rc5.

I am re-sending the series with gmail SMTP as we had some problems with
the mail server we used in the previous rounds.

Please review and provide your comments.

Changes from v3:

 * Addressed couple of comments given in v3.

Changes from v2:

 * Added depends on MIPS || COMPILE_TEST.

 * Corrected the logic while calculating time base steps.

 * Addressed few other minor comments given in v2.

Changes from v1:

 * Used regmap_update_bits at some places in the driver.

 * Placed some #defines next to their register.

 * Added depends on HAS_IOMEM in Kconfig.

 * Removed unnecessary paragraph from file header.

 * Defined register masks in a readable way.

 * Renamed clk-names with clock-names.

 * Added system interface clock in DT binding document.

 * Addressed few other minor review comments given in v1.

Naidu Tellapati (4):
  pwm: Imagination Technologies PWM DAC driver
  DT: pwm: Add binding document for IMG PWM DAC
  pdm: Imagination Technologies PDM DAC driver
  DT: pdm: Add binding document for IMG PDM DAC

 Documentation/devicetree/bindings/misc/img-pdm.txt |   54 ++
 Documentation/devicetree/bindings/pwm/img-pwm.txt  |   23 +
 drivers/misc/Kconfig                               |   12 +
 drivers/misc/Makefile                              |    1 +
 drivers/misc/img-pdm.c                             |  676 ++++++++++++++++++++
 drivers/pwm/Kconfig                                |   12 +
 drivers/pwm/Makefile                               |    1 +
 drivers/pwm/pwm-img.c                              |  270 ++++++++
 include/linux/img_pdm.h                            |   35 +
 9 files changed, 1084 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/misc/img-pdm.txt
 create mode 100644 Documentation/devicetree/bindings/pwm/img-pwm.txt
 create mode 100644 drivers/misc/img-pdm.c
 create mode 100644 drivers/pwm/pwm-img.c
 create mode 100644 include/linux/img_pdm.h