[PATCH 0/2] Add driver for Moortec PVT controller

[PATCH 0/2] Add driver for Moortec PVT controller

[Rahul Tanwar]

Patch 1 adds DT bindings schema in YAML format.
Patch 2 adds driver for MR75203 PVT controller.

Rahul Tanwar (2):
  Add DT bindings schema for PVT controller
  Add driver for Moortec MR75203 PVT controller

 .../devicetree/bindings/hwmon/moortec,mr75203.yaml |  70 +++
 drivers/hwmon/Kconfig                              |  10 +
 drivers/hwmon/Makefile                             |   1 +
 drivers/hwmon/mr75203.c                            | 617 +++++++++++++++++++++
 4 files changed, 698 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
 create mode 100644 drivers/hwmon/mr75203.c

-- 
2.11.0

[PATCH 1/2] Add DT bindings schema for PVT controller

[Rahul Tanwar]

PVT controller (MR75203) is used to configure & control
Moortec embedded analog IP which contains temprature sensor(TS),
voltage monitor(VM) & process detector(PD) modules.

Add DT bindings schema for PVT controller.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 .../devicetree/bindings/hwmon/moortec,mr75203.yaml | 70 ++++++++++++++++++++++
 1 file changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
new file mode 100644
index 000000000000..0d5a90117522
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
@@ -0,0 +1,70 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/hwmon/moortec,mr75203.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Moortec Semiconductor MR75203 PVT Controller bindings
+
+maintainers:
+  - Rahul Tanwar <rtanwar@maxlinear.com>
+
+properties:
+  compatible:
+    const: moortec,mr75203
+
+  reg:
+    items:
+      - description: PVT common registers
+      - description: PVT temprature sensor registers
+      - description: PVT process detector registers
+      - description: PVT voltage monitor registers
+
+  reg-names:
+    items:
+      - const: common
+      - const: ts
+      - const: pd
+      - const: vm
+
+  vm-map:
+    description:
+      PVT controller has 5 VM (voltage monitor) sensors.
+      vm-map defines CPU core to VM instance mapping. A
+      value of 0xff means that VM sensor is unused.
+    maxItems: 5
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  "#thermal-sensor-cells":
+    const: 1
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - vm-map
+  - clocks
+  - resets
+  - "#thermal-sensor-cells":
+
+additionalProperties: false
+
+examples:
+  - |
+    pvt: pvt@e0680000 {
+        compatible = "moortec,mr75203";
+        reg = <0xe0680000 0x80
+               0xe0680080 0x180
+               0xe0680200 0x200
+               0xe0680400 0xc00>;
+        reg-names = "common", "ts", "pd", "vm";
+        vm-map = [03 01 04 ff ff];
+        clocks = <&osc0>;
+        resets = <&rcu0 0x40 7>;
+        #thermal-sensor-cells = <1>;
+    };
-- 
2.11.0

Re: [PATCH 1/2] Add DT bindings schema for PVT controller

[Rob Herring]

On Wed, 09 Sep 2020 14:52:04 +0800, Rahul Tanwar wrote:
> PVT controller (MR75203) is used to configure & control
> Moortec embedded analog IP which contains temprature sensor(TS),
> voltage monitor(VM) & process detector(PD) modules.
> 
> Add DT bindings schema for PVT controller.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  .../devicetree/bindings/hwmon/moortec,mr75203.yaml | 70 ++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml: required:6: {'#thermal-sensor-cells': None} is not of type 'string'
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml: ignoring, error in schema: required: 6
warning: no schema found in file: ./Documentation/devicetree/bindings/hwmon/moortec,mr75203.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/hwmon/moortec,mr75203.example.dt.yaml: example-0: pvt@e0680000:reg:0: [3764912128, 128, 3764912256, 384, 3764912640, 512, 3764913152, 3072] is too long
	From schema: /usr/local/lib/python3.8/dist-packages/dtschema/schemas/reg.yaml


See https://patchwork.ozlabs.org/patch/1360350

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

[PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Rahul Tanwar]

PVT controller (MR75203) is used to configure & control
Moortec embedded analog IP which contains temprature
sensor(TS), voltage monitor(VM) & process detector(PD)
modules. Add driver to support MR75203 PVT controller.

Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
---
 drivers/hwmon/Kconfig   |  10 +
 drivers/hwmon/Makefile  |   1 +
 drivers/hwmon/mr75203.c | 617 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 628 insertions(+)
 create mode 100644 drivers/hwmon/mr75203.c

diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
index 8dc28b26916e..2defb46677b4 100644
--- a/drivers/hwmon/Kconfig
+++ b/drivers/hwmon/Kconfig
@@ -1112,6 +1112,16 @@ config SENSORS_MENF21BMC_HWMON
 	  This driver can also be built as a module. If so the module
 	  will be called menf21bmc_hwmon.
 
+config SENSORS_MR75203
+	tristate "Moortec Semiconductor MR75203 PVT Controller"
+	select REGMAP_MMIO
+	help
+	  If you say yes here you get support for Moortec MR75203
+	  PVT controller.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called mr75203.
+
 config SENSORS_ADCXX
 	tristate "National Semiconductor ADCxxxSxxx"
 	depends on SPI_MASTER
diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
index a8f4b35b136b..bb4bd92a5149 100644
--- a/drivers/hwmon/Makefile
+++ b/drivers/hwmon/Makefile
@@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
 obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
 obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
 obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
+obj-$(CONFIG_SENSORS_MR75203)	+= mr75203.o
 obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
 obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
 obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
new file mode 100644
index 000000000000..8b616e53f27b
--- /dev/null
+++ b/drivers/hwmon/mr75203.c
@@ -0,0 +1,617 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 MaxLinear, Inc.
+ *
+ * This driver is for PVT controller (MR75203) which is used
+ * to configure & control Moortec embedded analog IP to enable
+ * multiple embedded temprature sensor(TS), voltage monitor(VM)
+ * & process detector(PD) modules.
+ */
+#include <linux/clk.h>
+#include <linux/hwmon.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+/* PVT Common register */
+#define PVT_IP_CONFIG	0x04
+#define TS_NUM_MSK	GENMASK(4, 0)
+#define TS_NUM_SFT	0
+#define PD_NUM_MSK	GENMASK(12, 8)
+#define PD_NUM_SFT	8
+#define VM_NUM_MSK	GENMASK(20, 16)
+#define VM_NUM_SFT	16
+#define CH_NUM_MSK	GENMASK(31, 24)
+#define CH_NUM_SFT	24
+
+/* Macro Common Register */
+#define CLK_SYNTH		0x00
+#define CLK_SYNTH_LO_SFT	0
+#define CLK_SYNTH_HI_SFT	8
+#define CLK_SYNTH_HOLD_SFT	16
+#define CLK_SYNTH_EN		BIT(24)
+
+#define SDIF_DISABLE	0x04
+
+#define SDIF_STAT	0x08
+#define SDIF_BUSY	BIT(0)
+#define SDIF_LOCK	BIT(1)
+
+#define SDIF_W		0x0c
+#define SDIF_PROG	BIT(31)
+#define SDIF_WRN_W	BIT(27)
+#define SDIF_WRN_R	0x00
+#define SDIF_ADDR_SFT	24
+
+#define SDIF_HALT	0x10
+#define SDIF_CTRL	0x14
+#define SDIF_SMPL_CTRL	0x20
+
+/* TS & PD Individual Macro Register */
+#define COM_REG_SIZE	0x40
+
+#define SDIF_DONE(n)	(COM_REG_SIZE + 0x14 + 0x40 * (n))
+#define SDIF_SMPL_DONE	BIT(0)
+
+#define SDIF_DATA(n)	(COM_REG_SIZE + 0x18 + 0x40 * (n))
+#define SAMPLE_DATA_MSK	GENMASK(15, 0)
+
+#define HILO_RESET(n)	(COM_REG_SIZE + 0x2c + 0x40 * (n))
+
+/* VM Individual Macro Register */
+#define VM_COM_REG_SIZE	0x200
+#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
+#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
+
+/* SDA Slave Register */
+#define IP_CTRL			0x00
+#define IP_RST_REL		BIT(1)
+#define IP_RUN_CONT		BIT(3)
+#define IP_AUTO			BIT(8)
+#define IP_VM_MODE		BIT(10)
+
+#define IP_CFG			0x01
+#define CFG0_MODE_2		BIT(0)
+#define CFG0_PARALLEL_OUT	0
+#define CFG0_12_BIT		0
+#define CFG1_VOL_MEAS_MODE	0
+#define CFG1_PARALLEL_OUT	0
+#define CFG1_14_BIT		0
+
+#define IP_DATA		0x03
+
+#define IP_POLL		0x04
+#define VM_CH_INIT	BIT(20)
+#define VM_CH_REQ	BIT(21)
+
+#define IP_TMR			0x05
+#define POWER_DELAY_CYCLE_256	0x80
+#define POWER_DELAY_CYCLE_64	0x40
+
+#define PVT_POLL_DELAY_US	20
+#define PVT_POLL_TIMEOUT	20000
+#define PVT_H_CONST		100000
+#define PVT_CAL5_CONST		2047
+#define PVT_G_CONST		40000
+#define PVT_CONV_BITS		10
+#define PVT_N_CONST		90
+#define PVT_R_CONST		245805
+
+struct pvt_device {
+	struct regmap		*c_map;
+	struct regmap		*t_map;
+	struct regmap		*p_map;
+	struct regmap		*v_map;
+	struct clk		*clk;
+	struct reset_control	*rst;
+	u32			t_num;
+	u32			p_num;
+	u32			v_num;
+	u32			ip_freq;
+	u8			*vm_idx;
+	struct mutex		lock; /* lock to protect register */
+};
+
+static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
+			      u32 attr, int channel)
+{
+	switch (type) {
+	case hwmon_temp:
+		if (attr == hwmon_temp_input)
+			return 0444;
+		else
+			return 0;
+	case hwmon_in:
+		if (attr == hwmon_in_input)
+			return 0444;
+		else
+			return 0;
+	default:
+		return 0;
+	}
+}
+
+static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct pvt_device *pvt = dev_get_drvdata(dev);
+	struct regmap *t_map = pvt->t_map;
+	u32 stat, nbs;
+	int ret;
+	u64 tmp;
+
+	if (channel >= pvt->t_num)
+		return -EINVAL;
+
+	switch (attr) {
+	case hwmon_temp_input:
+		mutex_lock(&pvt->lock);
+		ret = regmap_read_poll_timeout(t_map, SDIF_DONE(channel),
+					       stat, stat & SDIF_SMPL_DONE,
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret) {
+			mutex_unlock(&pvt->lock);
+			return ret;
+		}
+
+		regmap_read(t_map, SDIF_DATA(channel), &nbs);
+		nbs &= SAMPLE_DATA_MSK;
+		mutex_unlock(&pvt->lock);
+
+		/*
+		 * Convert the register value to
+		 * degrees centigrade temperature
+		 */
+		tmp = nbs * PVT_H_CONST;
+		do_div(tmp, PVT_CAL5_CONST);
+		*val = tmp - PVT_G_CONST - pvt->ip_freq;
+
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
+{
+	struct pvt_device *pvt = dev_get_drvdata(dev);
+	struct regmap *v_map = pvt->v_map;
+	u32 n, stat;
+	u8 vm_idx;
+	int ret;
+
+	if (channel >= pvt->v_num)
+		return -EINVAL;
+
+	vm_idx = pvt->vm_idx[channel];
+
+	switch (attr) {
+	case hwmon_in_input:
+		mutex_lock(&pvt->lock);
+		ret = regmap_read_poll_timeout(v_map, VM_SDIF_DONE(vm_idx),
+					       stat, stat & SDIF_SMPL_DONE,
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret) {
+			mutex_unlock(&pvt->lock);
+			return ret;
+		}
+
+		regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
+		n &= SAMPLE_DATA_MSK;
+		mutex_unlock(&pvt->lock);
+
+
+		/* Convert the N bitstream count into voltage */
+		*val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
+
+		return 0;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int pvt_read(struct device *dev, enum hwmon_sensor_types type,
+		    u32 attr, int channel, long *val)
+{
+	switch (type) {
+	case hwmon_temp:
+		return pvt_read_temp(dev, attr, channel, val);
+	case hwmon_in:
+		return pvt_read_in(dev, attr, channel, val);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static const u32 pvt_chip_config[] = {
+	HWMON_C_REGISTER_TZ,
+	0
+};
+
+static const struct hwmon_channel_info pvt_chip = {
+	.type = hwmon_chip,
+	.config = pvt_chip_config,
+};
+
+static struct hwmon_channel_info pvt_temp = {
+	.type = hwmon_temp,
+};
+
+static struct hwmon_channel_info pvt_in = {
+	.type = hwmon_in,
+};
+
+static const struct hwmon_ops pvt_hwmon_ops = {
+	.is_visible = pvt_is_visible,
+	.read = pvt_read,
+};
+
+static struct hwmon_chip_info pvt_chip_info = {
+	.ops = &pvt_hwmon_ops,
+};
+
+static int pvt_init(struct pvt_device *pvt)
+{
+	u16 sys_freq, key, middle, low = 4, high = 8;
+	struct regmap *t_map = pvt->t_map;
+	struct regmap *p_map = pvt->p_map;
+	struct regmap *v_map = pvt->v_map;
+	u32 t_num = pvt->t_num;
+	u32 p_num = pvt->p_num;
+	u32 v_num = pvt->v_num;
+	u32 clk_synth, val;
+	int ret;
+
+	sys_freq = clk_get_rate(pvt->clk) / 1000000;
+	while (high >= low) {
+		middle = DIV_ROUND_CLOSEST(low + high, 2);
+		key = DIV_ROUND_CLOSEST(sys_freq, middle);
+		if (key > 514) {
+			low = middle + 1;
+			continue;
+		} else if (key < 2) {
+			high = middle - 1;
+			continue;
+		}
+
+		break;
+	}
+
+	key = clamp_val(key, 2, 514) - 2;
+
+	clk_synth = ((key + 1) >> 1) << CLK_SYNTH_LO_SFT |
+		    (key >> 1) << CLK_SYNTH_HI_SFT |
+		    (key >> 1) << CLK_SYNTH_HOLD_SFT | CLK_SYNTH_EN;
+
+	pvt->ip_freq = sys_freq * 100 / (key + 2);
+
+	if (t_num) {
+		regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);
+		regmap_write(t_map, SDIF_HALT, 0x0);
+		regmap_write(t_map, CLK_SYNTH, clk_synth);
+		regmap_write(t_map, SDIF_DISABLE, 0);
+
+		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret)
+			return ret;
+
+		val = CFG0_MODE_2 | CFG0_PARALLEL_OUT | CFG0_12_BIT |
+		      IP_CFG << SDIF_ADDR_SFT | SDIF_WRN_W | SDIF_PROG;
+		regmap_write(t_map, SDIF_W, val);
+
+		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret)
+			return ret;
+
+		val = POWER_DELAY_CYCLE_256 | IP_TMR << SDIF_ADDR_SFT |
+			      SDIF_WRN_W | SDIF_PROG;
+		regmap_write(t_map, SDIF_W, val);
+
+		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret)
+			return ret;
+
+		val = IP_RST_REL | IP_RUN_CONT | IP_AUTO |
+		      IP_CTRL << SDIF_ADDR_SFT |
+		      SDIF_WRN_W | SDIF_PROG;
+		regmap_write(t_map, SDIF_W, val);
+	}
+
+	if (p_num) {
+		regmap_write(p_map, SDIF_HALT, 0x0);
+		regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
+		regmap_write(p_map, CLK_SYNTH, clk_synth);
+	}
+
+	if (v_num) {
+		regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
+		regmap_write(v_map, SDIF_HALT, 0x0);
+		regmap_write(v_map, CLK_SYNTH, clk_synth);
+		regmap_write(v_map, SDIF_DISABLE, 0);
+
+		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret)
+			return ret;
+
+		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
+		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
+		      SDIF_WRN_W | SDIF_PROG;
+		regmap_write(v_map, SDIF_W, val);
+
+		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret)
+			return ret;
+
+		val = POWER_DELAY_CYCLE_64 | IP_TMR << SDIF_ADDR_SFT |
+		      SDIF_WRN_W | SDIF_PROG;
+		regmap_write(v_map, SDIF_W, val);
+
+		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
+					       val, !(val & SDIF_BUSY),
+					       PVT_POLL_DELAY_US,
+					       PVT_POLL_TIMEOUT);
+		if (ret)
+			return ret;
+
+		val = IP_RST_REL | IP_RUN_CONT | IP_AUTO | IP_VM_MODE |
+		      IP_CTRL << SDIF_ADDR_SFT |
+		      SDIF_WRN_W | SDIF_PROG;
+		regmap_write(v_map, SDIF_W, val);
+	}
+
+	return 0;
+}
+
+static struct regmap_config pvt_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+};
+
+static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
+{
+	struct device *dev = &pdev->dev;
+	struct pvt_device *pvt = platform_get_drvdata(pdev);
+	struct regmap **reg_map;
+	void __iomem *io_base;
+	struct resource *res;
+
+	if (!strcmp(reg_name, "common"))
+		reg_map = &pvt->c_map;
+	else if (!strcmp(reg_name, "ts"))
+		reg_map = &pvt->t_map;
+	else if (!strcmp(reg_name, "pd"))
+		reg_map = &pvt->p_map;
+	else if (!strcmp(reg_name, "vm"))
+		reg_map = &pvt->v_map;
+	else
+		return -EINVAL;
+
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
+	io_base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(io_base))
+		return PTR_ERR(io_base);
+
+	pvt_regmap_config.name = reg_name;
+	*reg_map = devm_regmap_init_mmio(dev, io_base, &pvt_regmap_config);
+	if (IS_ERR(*reg_map)) {
+		dev_err(dev, "failed to init register map\n");
+		return PTR_ERR(*reg_map);
+	}
+
+	return 0;
+}
+
+static void pvt_clk_disable(void *data)
+{
+	struct pvt_device *pvt = data;
+
+	clk_disable_unprepare(pvt->clk);
+}
+
+static int pvt_clk_enable(struct device *dev, struct pvt_device *pvt)
+{
+	int ret;
+
+	ret = clk_prepare_enable(pvt->clk);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, pvt_clk_disable, pvt);
+}
+
+static void pvt_reset_control_assert(void *data)
+{
+	struct pvt_device *pvt = data;
+
+	reset_control_assert(pvt->rst);
+}
+
+static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt)
+{
+	int ret;
+
+	ret = reset_control_deassert(pvt->rst);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(dev, pvt_reset_control_assert, pvt);
+}
+
+static int mr75203_probe(struct platform_device *pdev)
+{
+	const struct hwmon_channel_info **pvt_info;
+	u32 ts_num, vm_num, pd_num, val, index, i;
+	struct device *dev = &pdev->dev;
+	u32 *temp_config, *in_config;
+	struct device *hwmon_dev;
+	struct pvt_device *pvt;
+	int ret;
+
+	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
+	if (!pvt)
+		return -ENOMEM;
+
+	ret = pvt_get_regmap(pdev, "common");
+	if (ret)
+		return ret;
+
+	pvt->rst = devm_reset_control_get(dev, NULL);
+	if (IS_ERR(pvt->rst))
+		return dev_err_probe(dev, PTR_ERR(pvt->rst),
+				     "failed to get reset control\n");
+
+	ret = pvt_reset_control_deassert(dev, pvt);
+	if (ret)
+		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
+
+	pvt->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(pvt->clk))
+		return dev_err_probe(dev, PTR_ERR(pvt->clk), "failed to get clock\n");
+
+	ret = pvt_clk_enable(dev, pvt);
+	if (ret) {
+		dev_err(dev, "failed to enable clock\n");
+		return ret;
+	}
+
+	regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
+	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
+	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
+	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
+	pvt->t_num = ts_num;
+	pvt->p_num = pd_num;
+	pvt->v_num = vm_num;
+	val = 0;
+	if (ts_num)
+		val++;
+	if (vm_num)
+		val++;
+	if (!val && !pd_num) {
+		dev_err(dev, "None of TS, VM or PD configured\n");
+		return -EINVAL;
+	}
+
+	pvt_info = devm_kcalloc(dev, val + 2, sizeof(*pvt_info), GFP_KERNEL);
+	if (!pvt_info)
+		return -ENOMEM;
+	pvt_info[0] = &pvt_chip;
+	index = 1;
+
+	if (ts_num) {
+		ret = pvt_get_regmap(pdev, "ts");
+		if (ret)
+			return ret;
+
+		temp_config = devm_kcalloc(dev, ts_num + 1,
+					   sizeof(*temp_config), GFP_KERNEL);
+		if (!temp_config)
+			return -ENOMEM;
+
+		for (i = 0; i < ts_num; i++)
+			temp_config[i] = HWMON_T_INPUT;
+
+		temp_config[ts_num] = 0;
+		pvt_temp.config = temp_config;
+
+		pvt_info[index++] = &pvt_temp;
+	}
+
+	if (pd_num) {
+		ret = pvt_get_regmap(pdev, "pd");
+		if (ret)
+			return ret;
+	}
+
+	if (vm_num) {
+		u32 num = vm_num;
+
+		ret = pvt_get_regmap(pdev, "vm");
+		if (ret)
+			return ret;
+
+		pvt->vm_idx = devm_kcalloc(dev, vm_num, sizeof(*pvt->vm_idx),
+					   GFP_KERNEL);
+		if (!pvt->vm_idx)
+			return -ENOMEM;
+
+		for (i = 0; i < vm_num; i++)
+			pvt->vm_idx[i] = i;
+
+		ret = of_property_read_u8_array(dev->of_node, "vm-map",
+						pvt->vm_idx, vm_num);
+
+		if (!ret)
+			for (i = 0; i < vm_num; i++)
+				if (pvt->vm_idx[i] >= vm_num ||
+				    pvt->vm_idx[i] == 0xff) {
+					num = i;
+					break;
+				}
+
+		in_config = devm_kcalloc(dev, num + 1,
+					 sizeof(*in_config), GFP_KERNEL);
+		if (!in_config)
+			return -ENOMEM;
+
+		for (i = 0; i < num; i++)
+			in_config[i] = HWMON_I_INPUT;
+
+		in_config[num] = 0;
+		pvt_in.config = in_config;
+
+		pvt_info[index++] = &pvt_in;
+	}
+
+	ret = pvt_init(pvt);
+	if (ret) {
+		dev_err(dev, "failed to init pvt: %d\n", ret);
+		return ret;
+	}
+
+	mutex_init(&pvt->lock);
+
+	pvt_info[index] = NULL;
+	pvt_chip_info.info = pvt_info;
+	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pvt",
+							 pvt,
+							 &pvt_chip_info,
+							 NULL);
+
+	platform_set_drvdata(pdev, pvt);
+	return PTR_ERR_OR_ZERO(hwmon_dev);
+}
+
+static const struct of_device_id moortec_pvt_of_match[] = {
+	{ .compatible = "moortec,mr75203" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, moortec_pvt_of_match);
+
+static struct platform_driver moortec_pvt_driver = {
+	.driver = {
+		.name = "moortec-pvt",
+		.of_match_table = moortec_pvt_of_match,
+	},
+	.probe = mr75203_probe,
+};
+module_platform_driver(moortec_pvt_driver);
-- 
2.11.0

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Andy Shevchenko]

On Wed, Sep 09, 2020 at 02:52:05PM +0800, Rahul Tanwar wrote:
> PVT controller (MR75203) is used to configure & control
> Moortec embedded analog IP which contains temprature
> sensor(TS), voltage monitor(VM) & process detector(PD)
> modules. Add driver to support MR75203 PVT controller.

...

> +#include <linux/clk.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>


> +#include <linux/of.h>

I don't see anything special about OF here.
Perhaps
	mod_devicetable.h
	property.h
?

> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>

...

> +#define PVT_POLL_TIMEOUT	20000

Units?

...

> +	sys_freq = clk_get_rate(pvt->clk) / 1000000;

HZ_PER_MHZ ?

> +	while (high >= low) {
> +		middle = DIV_ROUND_CLOSEST(low + high, 2);

I'm wondering would it be better in the code like

	middle = (low + high + 1) / 2;

> +		key = DIV_ROUND_CLOSEST(sys_freq, middle);
> +		if (key > 514) {
> +			low = middle + 1;
> +			continue;
> +		} else if (key < 2) {
> +			high = middle - 1;
> +			continue;
> +		}
> +
> +		break;
> +	}
> +
> +	key = clamp_val(key, 2, 514) - 2;

I guess above deserves a comment with formulas.

...

> +		regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));

For non-constants better would be BIT(p_num) - 1.

...

> +		regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
> +		regmap_write(v_map, SDIF_HALT, 0x0);
> +		regmap_write(v_map, SDIF_DISABLE, 0);

In some you have 0, in some 0x0 over the file, can it be consistent?

...

> +static struct regmap_config pvt_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,

How do you use regmap's lock?

> +};

...

> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
> +{
> +	struct device *dev = &pdev->dev;

> +	struct pvt_device *pvt = platform_get_drvdata(pdev);

Can it be first line in definition block?

> +	struct regmap **reg_map;
> +	void __iomem *io_base;
> +	struct resource *res;
> +
> +	if (!strcmp(reg_name, "common"))
> +		reg_map = &pvt->c_map;
> +	else if (!strcmp(reg_name, "ts"))
> +		reg_map = &pvt->t_map;
> +	else if (!strcmp(reg_name, "pd"))
> +		reg_map = &pvt->p_map;
> +	else if (!strcmp(reg_name, "vm"))
> +		reg_map = &pvt->v_map;
> +	else
> +		return -EINVAL;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
> +	io_base = devm_ioremap_resource(dev, res);

	io_base = devm_platform_ioremap_resource_by_name(pdev, reg_name);

?

> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	pvt_regmap_config.name = reg_name;

Hmm... regmap API keeps it in devres. Why is there a copy?

> +	*reg_map = devm_regmap_init_mmio(dev, io_base, &pvt_regmap_config);
> +	if (IS_ERR(*reg_map)) {
> +		dev_err(dev, "failed to init register map\n");
> +		return PTR_ERR(*reg_map);
> +	}
> +
> +	return 0;
> +}

...

> +		for (i = 0; i < num; i++)
> +			in_config[i] = HWMON_I_INPUT;

memset32() ?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Tanwar, Rahul]

Hi Andy,

Thanks for the review & feedback.

On 9/9/2020 6:33 pm, Andy Shevchenko wrote:
> On Wed, Sep 09, 2020 at 02:52:05PM +0800, Rahul Tanwar wrote:
>> PVT controller (MR75203) is used to configure & control
>> Moortec embedded analog IP which contains temprature
>> sensor(TS), voltage monitor(VM) & process detector(PD)
>> modules. Add driver to support MR75203 PVT controller.
> ...
>
>> +#include <linux/clk.h>
>> +#include <linux/hwmon.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>
>> +#include <linux/of.h>
> I don't see anything special about OF here.
> Perhaps
> 	mod_devicetable.h
> 	property.h
> ?

of.h is needed because of of_property_read_u8_array(). I will add
mod_devicetable.h.
property.h seems not required at all.

>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
> ...
>
>> +#define PVT_POLL_TIMEOUT	20000
> Units?

Well noted.

> ...
>
>> +	sys_freq = clk_get_rate(pvt->clk) / 1000000;
> HZ_PER_MHZ ?

Well noted.

>> +	while (high >= low) {
>> +		middle = DIV_ROUND_CLOSEST(low + high, 2);
> I'm wondering would it be better in the code like
>
> 	middle = (low + high + 1) / 2;

Will update.

>> +		key = DIV_ROUND_CLOSEST(sys_freq, middle);
>> +		if (key > 514) {
>> +			low = middle + 1;
>> +			continue;
>> +		} else if (key < 2) {
>> +			high = middle - 1;
>> +			continue;
>> +		}
>> +
>> +		break;
>> +	}
>> +
>> +	key = clamp_val(key, 2, 514) - 2;
> I guess above deserves a comment with formulas.

Hmm..I will try to add some more info. Problem is that the datasheet doesn't
explain it clearly.

> ...
>
>> +		regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
> For non-constants better would be BIT(p_num) - 1.

Well noted.

> ...
>
>> +		regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
>> +		regmap_write(v_map, SDIF_HALT, 0x0);
>> +		regmap_write(v_map, SDIF_DISABLE, 0);
> In some you have 0, in some 0x0 over the file, can it be consistent?

Yes, missed that, will update.

> ...
>
>> +static struct regmap_config pvt_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
> How do you use regmap's lock?

We mutex lock whenever read temperature or voltage values from the registers.
All non-probe/non-init paths. We do not override regmap's internal lock.

>> +};
> ...
>
>> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct pvt_device *pvt = platform_get_drvdata(pdev);
> Can it be first line in definition block?

Well noted.

>> +	struct regmap **reg_map;
>> +	void __iomem *io_base;
>> +	struct resource *res;
>> +
>> +	if (!strcmp(reg_name, "common"))
>> +		reg_map = &pvt->c_map;
>> +	else if (!strcmp(reg_name, "ts"))
>> +		reg_map = &pvt->t_map;
>> +	else if (!strcmp(reg_name, "pd"))
>> +		reg_map = &pvt->p_map;
>> +	else if (!strcmp(reg_name, "vm"))
>> +		reg_map = &pvt->v_map;
>> +	else
>> +		return -EINVAL;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
>> +	io_base = devm_ioremap_resource(dev, res);
> 	io_base = devm_platform_ioremap_resource_by_name(pdev, reg_name);
>
> ?

Well noted.

>> +	if (IS_ERR(io_base))
>> +		return PTR_ERR(io_base);
>> +
>> +	pvt_regmap_config.name = reg_name;
> Hmm... regmap API keeps it in devres. Why is there a copy?

Just populating the name in regmap config because of multiple register
regions.. 

>> +	*reg_map = devm_regmap_init_mmio(dev, io_base, &pvt_regmap_config);
>> +	if (IS_ERR(*reg_map)) {
>> +		dev_err(dev, "failed to init register map\n");
>> +		return PTR_ERR(*reg_map);
>> +	}
>> +
>> +	return 0;
>> +}
> ...
>
>> +		for (i = 0; i < num; i++)
>> +			in_config[i] = HWMON_I_INPUT;
> memset32() ?
>

Well noted. Thanks.

Regards,
Rahul

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Andy Shevchenko]

On Thu, Sep 10, 2020 at 03:26:04PM +0800, Tanwar, Rahul wrote:
> On 9/9/2020 6:33 pm, Andy Shevchenko wrote:
> > On Wed, Sep 09, 2020 at 02:52:05PM +0800, Rahul Tanwar wrote:

...

> >> +#include <linux/of.h>
> > I don't see anything special about OF here.
> > Perhaps
> > 	mod_devicetable.h
> > 	property.h
> > ?
> 
> of.h is needed because of of_property_read_u8_array(). I will add
> mod_devicetable.h.
> property.h seems not required at all.

if you use device property API then it will be more generic.

...

> >> +static struct regmap_config pvt_regmap_config = {
> >> +	.reg_bits = 32,
> >> +	.reg_stride = 4,
> >> +	.val_bits = 32,
> > How do you use regmap's lock?
> 
> We mutex lock whenever read temperature or voltage values from the registers.
> All non-probe/non-init paths. We do not override regmap's internal lock.

Exactly and my point is why do we have too many locks?

> >> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Guenter Roeck]

On 9/8/20 11:52 PM, Rahul Tanwar wrote:
> PVT controller (MR75203) is used to configure & control
> Moortec embedded analog IP which contains temprature
> sensor(TS), voltage monitor(VM) & process detector(PD)
> modules. Add driver to support MR75203 PVT controller.
> 
The subject needs to include a hint that this is a hardware monitoring
driver, and the above should be "Add hardware monitoring driver to ...".

> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
>  drivers/hwmon/Kconfig   |  10 +
>  drivers/hwmon/Makefile  |   1 +
>  drivers/hwmon/mr75203.c | 617 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 628 insertions(+)
>  create mode 100644 drivers/hwmon/mr75203.c
> 
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 8dc28b26916e..2defb46677b4 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -1112,6 +1112,16 @@ config SENSORS_MENF21BMC_HWMON
>  	  This driver can also be built as a module. If so the module
>  	  will be called menf21bmc_hwmon.
>  
> +config SENSORS_MR75203
> +	tristate "Moortec Semiconductor MR75203 PVT Controller"
> +	select REGMAP_MMIO
> +	help
> +	  If you say yes here you get support for Moortec MR75203
> +	  PVT controller.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called mr75203.
> +
>  config SENSORS_ADCXX
>  	tristate "National Semiconductor ADCxxxSxxx"
>  	depends on SPI_MASTER
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index a8f4b35b136b..bb4bd92a5149 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
>  obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
>  obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
>  obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
> +obj-$(CONFIG_SENSORS_MR75203)	+= mr75203.o
>  obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
>  obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>  obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
> new file mode 100644
> index 000000000000..8b616e53f27b
> --- /dev/null
> +++ b/drivers/hwmon/mr75203.c
> @@ -0,0 +1,617 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 MaxLinear, Inc.
> + *
> + * This driver is for PVT controller (MR75203) which is used
> + * to configure & control Moortec embedded analog IP to enable
> + * multiple embedded temprature sensor(TS), voltage monitor(VM)
> + * & process detector(PD) modules.
> + */
> +#include <linux/clk.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
> +
> +/* PVT Common register */
> +#define PVT_IP_CONFIG	0x04
> +#define TS_NUM_MSK	GENMASK(4, 0)
> +#define TS_NUM_SFT	0
> +#define PD_NUM_MSK	GENMASK(12, 8)
> +#define PD_NUM_SFT	8
> +#define VM_NUM_MSK	GENMASK(20, 16)
> +#define VM_NUM_SFT	16
> +#define CH_NUM_MSK	GENMASK(31, 24)
> +#define CH_NUM_SFT	24
> +
> +/* Macro Common Register */
> +#define CLK_SYNTH		0x00
> +#define CLK_SYNTH_LO_SFT	0
> +#define CLK_SYNTH_HI_SFT	8
> +#define CLK_SYNTH_HOLD_SFT	16
> +#define CLK_SYNTH_EN		BIT(24)
> +
> +#define SDIF_DISABLE	0x04
> +
> +#define SDIF_STAT	0x08
> +#define SDIF_BUSY	BIT(0)
> +#define SDIF_LOCK	BIT(1)
> +
> +#define SDIF_W		0x0c
> +#define SDIF_PROG	BIT(31)
> +#define SDIF_WRN_W	BIT(27)
> +#define SDIF_WRN_R	0x00
> +#define SDIF_ADDR_SFT	24
> +
> +#define SDIF_HALT	0x10
> +#define SDIF_CTRL	0x14
> +#define SDIF_SMPL_CTRL	0x20
> +
> +/* TS & PD Individual Macro Register */
> +#define COM_REG_SIZE	0x40
> +
> +#define SDIF_DONE(n)	(COM_REG_SIZE + 0x14 + 0x40 * (n))
> +#define SDIF_SMPL_DONE	BIT(0)
> +
> +#define SDIF_DATA(n)	(COM_REG_SIZE + 0x18 + 0x40 * (n))
> +#define SAMPLE_DATA_MSK	GENMASK(15, 0)
> +
> +#define HILO_RESET(n)	(COM_REG_SIZE + 0x2c + 0x40 * (n))
> +
> +/* VM Individual Macro Register */
> +#define VM_COM_REG_SIZE	0x200
> +#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
> +#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
> +
> +/* SDA Slave Register */
> +#define IP_CTRL			0x00
> +#define IP_RST_REL		BIT(1)
> +#define IP_RUN_CONT		BIT(3)
> +#define IP_AUTO			BIT(8)
> +#define IP_VM_MODE		BIT(10)
> +
> +#define IP_CFG			0x01
> +#define CFG0_MODE_2		BIT(0)
> +#define CFG0_PARALLEL_OUT	0
> +#define CFG0_12_BIT		0
> +#define CFG1_VOL_MEAS_MODE	0
> +#define CFG1_PARALLEL_OUT	0
> +#define CFG1_14_BIT		0
> +
> +#define IP_DATA		0x03
> +
> +#define IP_POLL		0x04
> +#define VM_CH_INIT	BIT(20)
> +#define VM_CH_REQ	BIT(21)
> +
> +#define IP_TMR			0x05
> +#define POWER_DELAY_CYCLE_256	0x80
> +#define POWER_DELAY_CYCLE_64	0x40
> +
> +#define PVT_POLL_DELAY_US	20
> +#define PVT_POLL_TIMEOUT	20000
> +#define PVT_H_CONST		100000
> +#define PVT_CAL5_CONST		2047
> +#define PVT_G_CONST		40000
> +#define PVT_CONV_BITS		10
> +#define PVT_N_CONST		90
> +#define PVT_R_CONST		245805
> +
> +struct pvt_device {
> +	struct regmap		*c_map;
> +	struct regmap		*t_map;
> +	struct regmap		*p_map;
> +	struct regmap		*v_map;
> +	struct clk		*clk;
> +	struct reset_control	*rst;
> +	u32			t_num;
> +	u32			p_num;
> +	u32			v_num;
> +	u32			ip_freq;
> +	u8			*vm_idx;
> +	struct mutex		lock; /* lock to protect register */
> +};
> +
> +static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
> +			      u32 attr, int channel)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		if (attr == hwmon_temp_input)
> +			return 0444;
> +		else
> +			return 0;
> +	case hwmon_in:
> +		if (attr == hwmon_in_input)
> +			return 0444;
> +		else
> +			return 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
> +static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct pvt_device *pvt = dev_get_drvdata(dev);
> +	struct regmap *t_map = pvt->t_map;
> +	u32 stat, nbs;
> +	int ret;
> +	u64 tmp;
> +
> +	if (channel >= pvt->t_num)
> +		return -EINVAL;
> +
> +	switch (attr) {
> +	case hwmon_temp_input:
> +		mutex_lock(&pvt->lock);
> +		ret = regmap_read_poll_timeout(t_map, SDIF_DONE(channel),
> +					       stat, stat & SDIF_SMPL_DONE,
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret) {
> +			mutex_unlock(&pvt->lock);
> +			return ret;
> +		}
> +
> +		regmap_read(t_map, SDIF_DATA(channel), &nbs);
> +		nbs &= SAMPLE_DATA_MSK;
> +		mutex_unlock(&pvt->lock);
> +
> +		/*
> +		 * Convert the register value to
> +		 * degrees centigrade temperature
> +		 */
> +		tmp = nbs * PVT_H_CONST;
> +		do_div(tmp, PVT_CAL5_CONST);
> +		*val = tmp - PVT_G_CONST - pvt->ip_freq;
> +
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
> +{
> +	struct pvt_device *pvt = dev_get_drvdata(dev);
> +	struct regmap *v_map = pvt->v_map;
> +	u32 n, stat;
> +	u8 vm_idx;
> +	int ret;
> +
> +	if (channel >= pvt->v_num)
> +		return -EINVAL;
> +
> +	vm_idx = pvt->vm_idx[channel];
> +
> +	switch (attr) {
> +	case hwmon_in_input:
> +		mutex_lock(&pvt->lock);
> +		ret = regmap_read_poll_timeout(v_map, VM_SDIF_DONE(vm_idx),
> +					       stat, stat & SDIF_SMPL_DONE,
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret) {
> +			mutex_unlock(&pvt->lock);
> +			return ret;
> +		}
> +
> +		regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
> +		n &= SAMPLE_DATA_MSK;
> +		mutex_unlock(&pvt->lock);
> +
> +
> +		/* Convert the N bitstream count into voltage */
> +		*val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
> +
> +		return 0;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static int pvt_read(struct device *dev, enum hwmon_sensor_types type,
> +		    u32 attr, int channel, long *val)
> +{
> +	switch (type) {
> +	case hwmon_temp:
> +		return pvt_read_temp(dev, attr, channel, val);
> +	case hwmon_in:
> +		return pvt_read_in(dev, attr, channel, val);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static const u32 pvt_chip_config[] = {
> +	HWMON_C_REGISTER_TZ,
> +	0
> +};
> +
> +static const struct hwmon_channel_info pvt_chip = {
> +	.type = hwmon_chip,
> +	.config = pvt_chip_config,
> +};
> +
> +static struct hwmon_channel_info pvt_temp = {
> +	.type = hwmon_temp,
> +};
> +
> +static struct hwmon_channel_info pvt_in = {
> +	.type = hwmon_in,
> +};
> +
> +static const struct hwmon_ops pvt_hwmon_ops = {
> +	.is_visible = pvt_is_visible,
> +	.read = pvt_read,
> +};
> +
> +static struct hwmon_chip_info pvt_chip_info = {
> +	.ops = &pvt_hwmon_ops,
> +};
> +
> +static int pvt_init(struct pvt_device *pvt)
> +{
> +	u16 sys_freq, key, middle, low = 4, high = 8;
> +	struct regmap *t_map = pvt->t_map;
> +	struct regmap *p_map = pvt->p_map;
> +	struct regmap *v_map = pvt->v_map;
> +	u32 t_num = pvt->t_num;
> +	u32 p_num = pvt->p_num;
> +	u32 v_num = pvt->v_num;
> +	u32 clk_synth, val;
> +	int ret;
> +
> +	sys_freq = clk_get_rate(pvt->clk) / 1000000;
> +	while (high >= low) {
> +		middle = DIV_ROUND_CLOSEST(low + high, 2);
> +		key = DIV_ROUND_CLOSEST(sys_freq, middle);
> +		if (key > 514) {
> +			low = middle + 1;
> +			continue;
> +		} else if (key < 2) {
> +			high = middle - 1;
> +			continue;
> +		}
> +
> +		break;
> +	}
> +
> +	key = clamp_val(key, 2, 514) - 2;
> +
> +	clk_synth = ((key + 1) >> 1) << CLK_SYNTH_LO_SFT |
> +		    (key >> 1) << CLK_SYNTH_HI_SFT |
> +		    (key >> 1) << CLK_SYNTH_HOLD_SFT | CLK_SYNTH_EN;
> +
> +	pvt->ip_freq = sys_freq * 100 / (key + 2);
> +
> +	if (t_num) {
> +		regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);
> +		regmap_write(t_map, SDIF_HALT, 0x0);
> +		regmap_write(t_map, CLK_SYNTH, clk_synth);
> +		regmap_write(t_map, SDIF_DISABLE, 0);
> +
> +		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		val = CFG0_MODE_2 | CFG0_PARALLEL_OUT | CFG0_12_BIT |
> +		      IP_CFG << SDIF_ADDR_SFT | SDIF_WRN_W | SDIF_PROG;
> +		regmap_write(t_map, SDIF_W, val);
> +
> +		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		val = POWER_DELAY_CYCLE_256 | IP_TMR << SDIF_ADDR_SFT |
> +			      SDIF_WRN_W | SDIF_PROG;
> +		regmap_write(t_map, SDIF_W, val);
> +
> +		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		val = IP_RST_REL | IP_RUN_CONT | IP_AUTO |
> +		      IP_CTRL << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;
> +		regmap_write(t_map, SDIF_W, val);
> +	}
> +
> +	if (p_num) {
> +		regmap_write(p_map, SDIF_HALT, 0x0);
> +		regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
> +		regmap_write(p_map, CLK_SYNTH, clk_synth);
> +	}
> +
> +	if (v_num) {
> +		regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
> +		regmap_write(v_map, SDIF_HALT, 0x0);
> +		regmap_write(v_map, CLK_SYNTH, clk_synth);
> +		regmap_write(v_map, SDIF_DISABLE, 0);
> +
> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
> +		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;
> +		regmap_write(v_map, SDIF_W, val);
> +
> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		val = POWER_DELAY_CYCLE_64 | IP_TMR << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;
> +		regmap_write(v_map, SDIF_W, val);
> +
> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
> +					       val, !(val & SDIF_BUSY),
> +					       PVT_POLL_DELAY_US,
> +					       PVT_POLL_TIMEOUT);
> +		if (ret)
> +			return ret;
> +
> +		val = IP_RST_REL | IP_RUN_CONT | IP_AUTO | IP_VM_MODE |
> +		      IP_CTRL << SDIF_ADDR_SFT |
> +		      SDIF_WRN_W | SDIF_PROG;
> +		regmap_write(v_map, SDIF_W, val);
> +	}
> +
> +	return 0;
> +}
> +
> +static struct regmap_config pvt_regmap_config = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +};
> +
> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct pvt_device *pvt = platform_get_drvdata(pdev);

I am quite at loss how this is supposed to work. Platform driver
data is not initialized with a pointer to struct pvt_device at this point.
How does this code not crash ? What am I missing ?

> +	struct regmap **reg_map;
> +	void __iomem *io_base;
> +	struct resource *res;
> +
> +	if (!strcmp(reg_name, "common"))
> +		reg_map = &pvt->c_map;
> +	else if (!strcmp(reg_name, "ts"))
> +		reg_map = &pvt->t_map;
> +	else if (!strcmp(reg_name, "pd"))
> +		reg_map = &pvt->p_map;
> +	else if (!strcmp(reg_name, "vm"))
> +		reg_map = &pvt->v_map;
> +	else
> +		return -EINVAL;
> +
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
> +	io_base = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(io_base))
> +		return PTR_ERR(io_base);
> +
> +	pvt_regmap_config.name = reg_name;
> +	*reg_map = devm_regmap_init_mmio(dev, io_base, &pvt_regmap_config);
> +	if (IS_ERR(*reg_map)) {
> +		dev_err(dev, "failed to init register map\n");
> +		return PTR_ERR(*reg_map);
> +	}
> +
> +	return 0;
> +}
> +
> +static void pvt_clk_disable(void *data)
> +{
> +	struct pvt_device *pvt = data;
> +
> +	clk_disable_unprepare(pvt->clk);
> +}
> +
> +static int pvt_clk_enable(struct device *dev, struct pvt_device *pvt)
> +{
> +	int ret;
> +
> +	ret = clk_prepare_enable(pvt->clk);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, pvt_clk_disable, pvt);
> +}
> +
> +static void pvt_reset_control_assert(void *data)
> +{
> +	struct pvt_device *pvt = data;
> +
> +	reset_control_assert(pvt->rst);
> +}
> +
> +static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt)
> +{
> +	int ret;
> +
> +	ret = reset_control_deassert(pvt->rst);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(dev, pvt_reset_control_assert, pvt);
> +}
> +
> +static int mr75203_probe(struct platform_device *pdev)
> +{
> +	const struct hwmon_channel_info **pvt_info;
> +	u32 ts_num, vm_num, pd_num, val, index, i;
> +	struct device *dev = &pdev->dev;
> +	u32 *temp_config, *in_config;
> +	struct device *hwmon_dev;
> +	struct pvt_device *pvt;
> +	int ret;
> +
> +	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
> +	if (!pvt)
> +		return -ENOMEM;
> +
> +	ret = pvt_get_regmap(pdev, "common");
> +	if (ret)
> +		return ret;
> +
> +	pvt->rst = devm_reset_control_get(dev, NULL);
> +	if (IS_ERR(pvt->rst))
> +		return dev_err_probe(dev, PTR_ERR(pvt->rst),
> +				     "failed to get reset control\n");
> +
> +	ret = pvt_reset_control_deassert(dev, pvt);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
> +
> +	pvt->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(pvt->clk))
> +		return dev_err_probe(dev, PTR_ERR(pvt->clk), "failed to get clock\n");
> +
> +	ret = pvt_clk_enable(dev, pvt);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clock\n");
> +		return ret;
> +	}
> +
> +	regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
> +	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
> +	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
> +	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
> +	pvt->t_num = ts_num;
> +	pvt->p_num = pd_num;
> +	pvt->v_num = vm_num;
> +	val = 0;
> +	if (ts_num)
> +		val++;
> +	if (vm_num)
> +		val++;
> +	if (!val && !pd_num) {
> +		dev_err(dev, "None of TS, VM or PD configured\n");
> +		return -EINVAL;
> +	}

If ts_num and vm_num are both 0, the driver doesn't create any attributes and
would just sit there. What would be the point of that ?

> +
> +	pvt_info = devm_kcalloc(dev, val + 2, sizeof(*pvt_info), GFP_KERNEL);
> +	if (!pvt_info)
> +		return -ENOMEM;
> +	pvt_info[0] = &pvt_chip;
> +	index = 1;
> +
> +	if (ts_num) {
> +		ret = pvt_get_regmap(pdev, "ts");
> +		if (ret)
> +			return ret;
> +
> +		temp_config = devm_kcalloc(dev, ts_num + 1,
> +					   sizeof(*temp_config), GFP_KERNEL);
> +		if (!temp_config)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < ts_num; i++)
> +			temp_config[i] = HWMON_T_INPUT;
> +
> +		temp_config[ts_num] = 0;
> +		pvt_temp.config = temp_config;
> +
> +		pvt_info[index++] = &pvt_temp;
> +	}
> +
> +	if (pd_num) {
> +		ret = pvt_get_regmap(pdev, "pd");
> +		if (ret)
> +			return ret;
> +	}
> +
> +	if (vm_num) {
> +		u32 num = vm_num;
> +
> +		ret = pvt_get_regmap(pdev, "vm");
> +		if (ret)
> +			return ret;
> +
> +		pvt->vm_idx = devm_kcalloc(dev, vm_num, sizeof(*pvt->vm_idx),
> +					   GFP_KERNEL);
> +		if (!pvt->vm_idx)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < vm_num; i++)
> +			pvt->vm_idx[i] = i;
> +
> +		ret = of_property_read_u8_array(dev->of_node, "vm-map",
> +						pvt->vm_idx, vm_num);
> +
> +		if (!ret)
> +			for (i = 0; i < vm_num; i++)
> +				if (pvt->vm_idx[i] >= vm_num ||
> +				    pvt->vm_idx[i] == 0xff) {
> +					num = i;
> +					break;
> +				}
> +
> +		in_config = devm_kcalloc(dev, num + 1,
> +					 sizeof(*in_config), GFP_KERNEL);
> +		if (!in_config)
> +			return -ENOMEM;
> +
> +		for (i = 0; i < num; i++)
> +			in_config[i] = HWMON_I_INPUT;
> +
> +		in_config[num] = 0;
> +		pvt_in.config = in_config;
> +
> +		pvt_info[index++] = &pvt_in;
> +	}
> +
> +	ret = pvt_init(pvt);
> +	if (ret) {
> +		dev_err(dev, "failed to init pvt: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_init(&pvt->lock);
> +
> +	pvt_info[index] = NULL;

Unnecessary.

> +	pvt_chip_info.info = pvt_info;
> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pvt",
> +							 pvt,
> +							 &pvt_chip_info,
> +							 NULL);
> +
> +	platform_set_drvdata(pdev, pvt);

Again, I fail to understand how this is supposed to work.

> +	return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static const struct of_device_id moortec_pvt_of_match[] = {
> +	{ .compatible = "moortec,mr75203" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, moortec_pvt_of_match);
> +
> +static struct platform_driver moortec_pvt_driver = {
> +	.driver = {
> +		.name = "moortec-pvt",
> +		.of_match_table = moortec_pvt_of_match,
> +	},
> +	.probe = mr75203_probe,
> +};
> +module_platform_driver(moortec_pvt_driver);
> 

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Guenter Roeck]

On 9/10/20 12:27 AM, Tanwar, Rahul wrote:
> 
> Hi Guenter,
> 
> Thanks for the review & feedback.
> 
> On 9/9/2020 11:05 pm, Guenter Roeck wrote:
>> On 9/8/20 11:52 PM, Rahul Tanwar wrote:
>>> PVT controller (MR75203) is used to configure & control
>>> Moortec embedded analog IP which contains temprature
>>> sensor(TS), voltage monitor(VM) & process detector(PD)
>>> modules. Add driver to support MR75203 PVT controller.
>>>
>> The subject needs to include a hint that this is a hardware monitoring
>> driver, and the above should be "Add hardware monitoring driver to ...".
>>
>>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>>> ---
>>>  drivers/hwmon/Kconfig   |  10 +
>>>  drivers/hwmon/Makefile  |   1 +
>>>  drivers/hwmon/mr75203.c | 617 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 628 insertions(+)
>>>  create mode 100644 drivers/hwmon/mr75203.c
>>>
>>> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
>>> index 8dc28b26916e..2defb46677b4 100644
>>> --- a/drivers/hwmon/Kconfig
>>> +++ b/drivers/hwmon/Kconfig
>>> @@ -1112,6 +1112,16 @@ config SENSORS_MENF21BMC_HWMON
>>>  	  This driver can also be built as a module. If so the module
>>>  	  will be called menf21bmc_hwmon.
>>>  
>>> +config SENSORS_MR75203
>>> +	tristate "Moortec Semiconductor MR75203 PVT Controller"
>>> +	select REGMAP_MMIO
>>> +	help
>>> +	  If you say yes here you get support for Moortec MR75203
>>> +	  PVT controller.
>>> +
>>> +	  This driver can also be built as a module. If so, the module
>>> +	  will be called mr75203.
>>> +
>>>  config SENSORS_ADCXX
>>>  	tristate "National Semiconductor ADCxxxSxxx"
>>>  	depends on SPI_MASTER
>>> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
>>> index a8f4b35b136b..bb4bd92a5149 100644
>>> --- a/drivers/hwmon/Makefile
>>> +++ b/drivers/hwmon/Makefile
>>> @@ -142,6 +142,7 @@ obj-$(CONFIG_SENSORS_MCP3021)	+= mcp3021.o
>>>  obj-$(CONFIG_SENSORS_TC654)	+= tc654.o
>>>  obj-$(CONFIG_SENSORS_MLXREG_FAN) += mlxreg-fan.o
>>>  obj-$(CONFIG_SENSORS_MENF21BMC_HWMON) += menf21bmc_hwmon.o
>>> +obj-$(CONFIG_SENSORS_MR75203)	+= mr75203.o
>>>  obj-$(CONFIG_SENSORS_NCT6683)	+= nct6683.o
>>>  obj-$(CONFIG_SENSORS_NCT6775)	+= nct6775.o
>>>  obj-$(CONFIG_SENSORS_NCT7802)	+= nct7802.o
>>> diff --git a/drivers/hwmon/mr75203.c b/drivers/hwmon/mr75203.c
>>> new file mode 100644
>>> index 000000000000..8b616e53f27b
>>> --- /dev/null
>>> +++ b/drivers/hwmon/mr75203.c
>>> @@ -0,0 +1,617 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2020 MaxLinear, Inc.
>>> + *
>>> + * This driver is for PVT controller (MR75203) which is used
>>> + * to configure & control Moortec embedded analog IP to enable
>>> + * multiple embedded temprature sensor(TS), voltage monitor(VM)
>>> + * & process detector(PD) modules.
>>> + */
>>> +#include <linux/clk.h>
>>> +#include <linux/hwmon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/of.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/reset.h>
>>> +
>>> +/* PVT Common register */
>>> +#define PVT_IP_CONFIG	0x04
>>> +#define TS_NUM_MSK	GENMASK(4, 0)
>>> +#define TS_NUM_SFT	0
>>> +#define PD_NUM_MSK	GENMASK(12, 8)
>>> +#define PD_NUM_SFT	8
>>> +#define VM_NUM_MSK	GENMASK(20, 16)
>>> +#define VM_NUM_SFT	16
>>> +#define CH_NUM_MSK	GENMASK(31, 24)
>>> +#define CH_NUM_SFT	24
>>> +
>>> +/* Macro Common Register */
>>> +#define CLK_SYNTH		0x00
>>> +#define CLK_SYNTH_LO_SFT	0
>>> +#define CLK_SYNTH_HI_SFT	8
>>> +#define CLK_SYNTH_HOLD_SFT	16
>>> +#define CLK_SYNTH_EN		BIT(24)
>>> +
>>> +#define SDIF_DISABLE	0x04
>>> +
>>> +#define SDIF_STAT	0x08
>>> +#define SDIF_BUSY	BIT(0)
>>> +#define SDIF_LOCK	BIT(1)
>>> +
>>> +#define SDIF_W		0x0c
>>> +#define SDIF_PROG	BIT(31)
>>> +#define SDIF_WRN_W	BIT(27)
>>> +#define SDIF_WRN_R	0x00
>>> +#define SDIF_ADDR_SFT	24
>>> +
>>> +#define SDIF_HALT	0x10
>>> +#define SDIF_CTRL	0x14
>>> +#define SDIF_SMPL_CTRL	0x20
>>> +
>>> +/* TS & PD Individual Macro Register */
>>> +#define COM_REG_SIZE	0x40
>>> +
>>> +#define SDIF_DONE(n)	(COM_REG_SIZE + 0x14 + 0x40 * (n))
>>> +#define SDIF_SMPL_DONE	BIT(0)
>>> +
>>> +#define SDIF_DATA(n)	(COM_REG_SIZE + 0x18 + 0x40 * (n))
>>> +#define SAMPLE_DATA_MSK	GENMASK(15, 0)
>>> +
>>> +#define HILO_RESET(n)	(COM_REG_SIZE + 0x2c + 0x40 * (n))
>>> +
>>> +/* VM Individual Macro Register */
>>> +#define VM_COM_REG_SIZE	0x200
>>> +#define VM_SDIF_DONE(n)	(VM_COM_REG_SIZE + 0x34 + 0x200 * (n))
>>> +#define VM_SDIF_DATA(n)	(VM_COM_REG_SIZE + 0x40 + 0x200 * (n))
>>> +
>>> +/* SDA Slave Register */
>>> +#define IP_CTRL			0x00
>>> +#define IP_RST_REL		BIT(1)
>>> +#define IP_RUN_CONT		BIT(3)
>>> +#define IP_AUTO			BIT(8)
>>> +#define IP_VM_MODE		BIT(10)
>>> +
>>> +#define IP_CFG			0x01
>>> +#define CFG0_MODE_2		BIT(0)
>>> +#define CFG0_PARALLEL_OUT	0
>>> +#define CFG0_12_BIT		0
>>> +#define CFG1_VOL_MEAS_MODE	0
>>> +#define CFG1_PARALLEL_OUT	0
>>> +#define CFG1_14_BIT		0
>>> +
>>> +#define IP_DATA		0x03
>>> +
>>> +#define IP_POLL		0x04
>>> +#define VM_CH_INIT	BIT(20)
>>> +#define VM_CH_REQ	BIT(21)
>>> +
>>> +#define IP_TMR			0x05
>>> +#define POWER_DELAY_CYCLE_256	0x80
>>> +#define POWER_DELAY_CYCLE_64	0x40
>>> +
>>> +#define PVT_POLL_DELAY_US	20
>>> +#define PVT_POLL_TIMEOUT	20000
>>> +#define PVT_H_CONST		100000
>>> +#define PVT_CAL5_CONST		2047
>>> +#define PVT_G_CONST		40000
>>> +#define PVT_CONV_BITS		10
>>> +#define PVT_N_CONST		90
>>> +#define PVT_R_CONST		245805
>>> +
>>> +struct pvt_device {
>>> +	struct regmap		*c_map;
>>> +	struct regmap		*t_map;
>>> +	struct regmap		*p_map;
>>> +	struct regmap		*v_map;
>>> +	struct clk		*clk;
>>> +	struct reset_control	*rst;
>>> +	u32			t_num;
>>> +	u32			p_num;
>>> +	u32			v_num;
>>> +	u32			ip_freq;
>>> +	u8			*vm_idx;
>>> +	struct mutex		lock; /* lock to protect register */
>>> +};
>>> +
>>> +static umode_t pvt_is_visible(const void *data, enum hwmon_sensor_types type,
>>> +			      u32 attr, int channel)
>>> +{
>>> +	switch (type) {
>>> +	case hwmon_temp:
>>> +		if (attr == hwmon_temp_input)
>>> +			return 0444;
>>> +		else
>>> +			return 0;
>>> +	case hwmon_in:
>>> +		if (attr == hwmon_in_input)
>>> +			return 0444;
>>> +		else
>>> +			return 0;
>>> +	default:
>>> +		return 0;
>>> +	}
>>> +}
>>> +
>>> +static int pvt_read_temp(struct device *dev, u32 attr, int channel, long *val)
>>> +{
>>> +	struct pvt_device *pvt = dev_get_drvdata(dev);
>>> +	struct regmap *t_map = pvt->t_map;
>>> +	u32 stat, nbs;
>>> +	int ret;
>>> +	u64 tmp;
>>> +
>>> +	if (channel >= pvt->t_num)
>>> +		return -EINVAL;
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_temp_input:
>>> +		mutex_lock(&pvt->lock);
>>> +		ret = regmap_read_poll_timeout(t_map, SDIF_DONE(channel),
>>> +					       stat, stat & SDIF_SMPL_DONE,
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret) {
>>> +			mutex_unlock(&pvt->lock);
>>> +			return ret;
>>> +		}
>>> +
>>> +		regmap_read(t_map, SDIF_DATA(channel), &nbs);
>>> +		nbs &= SAMPLE_DATA_MSK;
>>> +		mutex_unlock(&pvt->lock);
>>> +
>>> +		/*
>>> +		 * Convert the register value to
>>> +		 * degrees centigrade temperature
>>> +		 */
>>> +		tmp = nbs * PVT_H_CONST;
>>> +		do_div(tmp, PVT_CAL5_CONST);
>>> +		*val = tmp - PVT_G_CONST - pvt->ip_freq;
>>> +
>>> +		return 0;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +}
>>> +
>>> +static int pvt_read_in(struct device *dev, u32 attr, int channel, long *val)
>>> +{
>>> +	struct pvt_device *pvt = dev_get_drvdata(dev);
>>> +	struct regmap *v_map = pvt->v_map;
>>> +	u32 n, stat;
>>> +	u8 vm_idx;
>>> +	int ret;
>>> +
>>> +	if (channel >= pvt->v_num)
>>> +		return -EINVAL;
>>> +
>>> +	vm_idx = pvt->vm_idx[channel];
>>> +
>>> +	switch (attr) {
>>> +	case hwmon_in_input:
>>> +		mutex_lock(&pvt->lock);
>>> +		ret = regmap_read_poll_timeout(v_map, VM_SDIF_DONE(vm_idx),
>>> +					       stat, stat & SDIF_SMPL_DONE,
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret) {
>>> +			mutex_unlock(&pvt->lock);
>>> +			return ret;
>>> +		}
>>> +
>>> +		regmap_read(v_map, VM_SDIF_DATA(vm_idx), &n);
>>> +		n &= SAMPLE_DATA_MSK;
>>> +		mutex_unlock(&pvt->lock);
>>> +
>>> +
>>> +		/* Convert the N bitstream count into voltage */
>>> +		*val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
>>> +
>>> +		return 0;
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +}
>>> +
>>> +static int pvt_read(struct device *dev, enum hwmon_sensor_types type,
>>> +		    u32 attr, int channel, long *val)
>>> +{
>>> +	switch (type) {
>>> +	case hwmon_temp:
>>> +		return pvt_read_temp(dev, attr, channel, val);
>>> +	case hwmon_in:
>>> +		return pvt_read_in(dev, attr, channel, val);
>>> +	default:
>>> +		return -EOPNOTSUPP;
>>> +	}
>>> +}
>>> +
>>> +static const u32 pvt_chip_config[] = {
>>> +	HWMON_C_REGISTER_TZ,
>>> +	0
>>> +};
>>> +
>>> +static const struct hwmon_channel_info pvt_chip = {
>>> +	.type = hwmon_chip,
>>> +	.config = pvt_chip_config,
>>> +};
>>> +
>>> +static struct hwmon_channel_info pvt_temp = {
>>> +	.type = hwmon_temp,
>>> +};
>>> +
>>> +static struct hwmon_channel_info pvt_in = {
>>> +	.type = hwmon_in,
>>> +};
>>> +
>>> +static const struct hwmon_ops pvt_hwmon_ops = {
>>> +	.is_visible = pvt_is_visible,
>>> +	.read = pvt_read,
>>> +};
>>> +
>>> +static struct hwmon_chip_info pvt_chip_info = {
>>> +	.ops = &pvt_hwmon_ops,
>>> +};
>>> +
>>> +static int pvt_init(struct pvt_device *pvt)
>>> +{
>>> +	u16 sys_freq, key, middle, low = 4, high = 8;
>>> +	struct regmap *t_map = pvt->t_map;
>>> +	struct regmap *p_map = pvt->p_map;
>>> +	struct regmap *v_map = pvt->v_map;
>>> +	u32 t_num = pvt->t_num;
>>> +	u32 p_num = pvt->p_num;
>>> +	u32 v_num = pvt->v_num;
>>> +	u32 clk_synth, val;
>>> +	int ret;
>>> +
>>> +	sys_freq = clk_get_rate(pvt->clk) / 1000000;
>>> +	while (high >= low) {
>>> +		middle = DIV_ROUND_CLOSEST(low + high, 2);
>>> +		key = DIV_ROUND_CLOSEST(sys_freq, middle);
>>> +		if (key > 514) {
>>> +			low = middle + 1;
>>> +			continue;
>>> +		} else if (key < 2) {
>>> +			high = middle - 1;
>>> +			continue;
>>> +		}
>>> +
>>> +		break;
>>> +	}
>>> +
>>> +	key = clamp_val(key, 2, 514) - 2;
>>> +
>>> +	clk_synth = ((key + 1) >> 1) << CLK_SYNTH_LO_SFT |
>>> +		    (key >> 1) << CLK_SYNTH_HI_SFT |
>>> +		    (key >> 1) << CLK_SYNTH_HOLD_SFT | CLK_SYNTH_EN;
>>> +
>>> +	pvt->ip_freq = sys_freq * 100 / (key + 2);
>>> +
>>> +	if (t_num) {
>>> +		regmap_write(t_map, SDIF_SMPL_CTRL, 0x0);
>>> +		regmap_write(t_map, SDIF_HALT, 0x0);
>>> +		regmap_write(t_map, CLK_SYNTH, clk_synth);
>>> +		regmap_write(t_map, SDIF_DISABLE, 0);
>>> +
>>> +		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
>>> +					       val, !(val & SDIF_BUSY),
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val = CFG0_MODE_2 | CFG0_PARALLEL_OUT | CFG0_12_BIT |
>>> +		      IP_CFG << SDIF_ADDR_SFT | SDIF_WRN_W | SDIF_PROG;
>>> +		regmap_write(t_map, SDIF_W, val);
>>> +
>>> +		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
>>> +					       val, !(val & SDIF_BUSY),
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val = POWER_DELAY_CYCLE_256 | IP_TMR << SDIF_ADDR_SFT |
>>> +			      SDIF_WRN_W | SDIF_PROG;
>>> +		regmap_write(t_map, SDIF_W, val);
>>> +
>>> +		ret = regmap_read_poll_timeout(t_map, SDIF_STAT,
>>> +					       val, !(val & SDIF_BUSY),
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val = IP_RST_REL | IP_RUN_CONT | IP_AUTO |
>>> +		      IP_CTRL << SDIF_ADDR_SFT |
>>> +		      SDIF_WRN_W | SDIF_PROG;
>>> +		regmap_write(t_map, SDIF_W, val);
>>> +	}
>>> +
>>> +	if (p_num) {
>>> +		regmap_write(p_map, SDIF_HALT, 0x0);
>>> +		regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
>>> +		regmap_write(p_map, CLK_SYNTH, clk_synth);
>>> +	}
>>> +
>>> +	if (v_num) {
>>> +		regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
>>> +		regmap_write(v_map, SDIF_HALT, 0x0);
>>> +		regmap_write(v_map, CLK_SYNTH, clk_synth);
>>> +		regmap_write(v_map, SDIF_DISABLE, 0);
>>> +
>>> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
>>> +					       val, !(val & SDIF_BUSY),
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val = CFG1_VOL_MEAS_MODE | CFG1_PARALLEL_OUT |
>>> +		      CFG1_14_BIT | IP_CFG << SDIF_ADDR_SFT |
>>> +		      SDIF_WRN_W | SDIF_PROG;
>>> +		regmap_write(v_map, SDIF_W, val);
>>> +
>>> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
>>> +					       val, !(val & SDIF_BUSY),
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val = POWER_DELAY_CYCLE_64 | IP_TMR << SDIF_ADDR_SFT |
>>> +		      SDIF_WRN_W | SDIF_PROG;
>>> +		regmap_write(v_map, SDIF_W, val);
>>> +
>>> +		ret = regmap_read_poll_timeout(v_map, SDIF_STAT,
>>> +					       val, !(val & SDIF_BUSY),
>>> +					       PVT_POLL_DELAY_US,
>>> +					       PVT_POLL_TIMEOUT);
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		val = IP_RST_REL | IP_RUN_CONT | IP_AUTO | IP_VM_MODE |
>>> +		      IP_CTRL << SDIF_ADDR_SFT |
>>> +		      SDIF_WRN_W | SDIF_PROG;
>>> +		regmap_write(v_map, SDIF_W, val);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct regmap_config pvt_regmap_config = {
>>> +	.reg_bits = 32,
>>> +	.reg_stride = 4,
>>> +	.val_bits = 32,
>>> +};
>>> +
>>> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
>>> +{
>>> +	struct device *dev = &pdev->dev;
>>> +	struct pvt_device *pvt = platform_get_drvdata(pdev);
>> I am quite at loss how this is supposed to work. Platform driver
>> data is not initialized with a pointer to struct pvt_device at this point.
>> How does this code not crash ? What am I missing ?
> 
> Big mistake on my part. Last minute change based on internal review feedback
> about moving platform_set_drvdata() at the end of probe. I will fix it in v2.
> Thanks.
> 
>>> +	struct regmap **reg_map;
>>> +	void __iomem *io_base;
>>> +	struct resource *res;
>>> +
>>> +	if (!strcmp(reg_name, "common"))
>>> +		reg_map = &pvt->c_map;
>>> +	else if (!strcmp(reg_name, "ts"))
>>> +		reg_map = &pvt->t_map;
>>> +	else if (!strcmp(reg_name, "pd"))
>>> +		reg_map = &pvt->p_map;
>>> +	else if (!strcmp(reg_name, "vm"))
>>> +		reg_map = &pvt->v_map;
>>> +	else
>>> +		return -EINVAL;
>>> +
>>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
>>> +	io_base = devm_ioremap_resource(dev, res);
>>> +	if (IS_ERR(io_base))
>>> +		return PTR_ERR(io_base);
>>> +
>>> +	pvt_regmap_config.name = reg_name;
>>> +	*reg_map = devm_regmap_init_mmio(dev, io_base, &pvt_regmap_config);
>>> +	if (IS_ERR(*reg_map)) {
>>> +		dev_err(dev, "failed to init register map\n");
>>> +		return PTR_ERR(*reg_map);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static void pvt_clk_disable(void *data)
>>> +{
>>> +	struct pvt_device *pvt = data;
>>> +
>>> +	clk_disable_unprepare(pvt->clk);
>>> +}
>>> +
>>> +static int pvt_clk_enable(struct device *dev, struct pvt_device *pvt)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = clk_prepare_enable(pvt->clk);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return devm_add_action_or_reset(dev, pvt_clk_disable, pvt);
>>> +}
>>> +
>>> +static void pvt_reset_control_assert(void *data)
>>> +{
>>> +	struct pvt_device *pvt = data;
>>> +
>>> +	reset_control_assert(pvt->rst);
>>> +}
>>> +
>>> +static int pvt_reset_control_deassert(struct device *dev, struct pvt_device *pvt)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = reset_control_deassert(pvt->rst);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return devm_add_action_or_reset(dev, pvt_reset_control_assert, pvt);
>>> +}
>>> +
>>> +static int mr75203_probe(struct platform_device *pdev)
>>> +{
>>> +	const struct hwmon_channel_info **pvt_info;
>>> +	u32 ts_num, vm_num, pd_num, val, index, i;
>>> +	struct device *dev = &pdev->dev;
>>> +	u32 *temp_config, *in_config;
>>> +	struct device *hwmon_dev;
>>> +	struct pvt_device *pvt;
>>> +	int ret;
>>> +
>>> +	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
>>> +	if (!pvt)
>>> +		return -ENOMEM;
>>> +
>>> +	ret = pvt_get_regmap(pdev, "common");
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	pvt->rst = devm_reset_control_get(dev, NULL);
>>> +	if (IS_ERR(pvt->rst))
>>> +		return dev_err_probe(dev, PTR_ERR(pvt->rst),
>>> +				     "failed to get reset control\n");
>>> +
>>> +	ret = pvt_reset_control_deassert(dev, pvt);
>>> +	if (ret)
>>> +		return dev_err_probe(dev, ret, "cannot deassert reset control\n");
>>> +
>>> +	pvt->clk = devm_clk_get(dev, NULL);
>>> +	if (IS_ERR(pvt->clk))
>>> +		return dev_err_probe(dev, PTR_ERR(pvt->clk), "failed to get clock\n");
>>> +
>>> +	ret = pvt_clk_enable(dev, pvt);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to enable clock\n");
>>> +		return ret;
>>> +	}
>>> +
>>> +	regmap_read(pvt->c_map, PVT_IP_CONFIG, &val);
>>> +	ts_num = (val & TS_NUM_MSK) >> TS_NUM_SFT;
>>> +	pd_num = (val & PD_NUM_MSK) >> PD_NUM_SFT;
>>> +	vm_num = (val & VM_NUM_MSK) >> VM_NUM_SFT;
>>> +	pvt->t_num = ts_num;
>>> +	pvt->p_num = pd_num;
>>> +	pvt->v_num = vm_num;
>>> +	val = 0;
>>> +	if (ts_num)
>>> +		val++;
>>> +	if (vm_num)
>>> +		val++;
>>> +	if (!val && !pd_num) {
>>> +		dev_err(dev, "None of TS, VM or PD configured\n");
>>> +		return -EINVAL;
>>> +	}
>> If ts_num and vm_num are both 0, the driver doesn't create any attributes and
>> would just sit there. What would be the point of that ?
> 
> ts_num and vm_num are read from a read only IP configuration register which
> is guaranteed to be non zero if the IP exists. So yes this check seems
> redundant. Are you suggesting to remove this check ?     
> 

No. From what you say, the check should be
	if (!val) {
		return -ENODEV;
	}

EINVAL it is not really appropriate - the user did nothing wrong.

Guenter

>>> +
>>> +	pvt_info = devm_kcalloc(dev, val + 2, sizeof(*pvt_info), GFP_KERNEL);
>>> +	if (!pvt_info)
>>> +		return -ENOMEM;
>>> +	pvt_info[0] = &pvt_chip;
>>> +	index = 1;
>>> +
>>> +	if (ts_num) {
>>> +		ret = pvt_get_regmap(pdev, "ts");
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		temp_config = devm_kcalloc(dev, ts_num + 1,
>>> +					   sizeof(*temp_config), GFP_KERNEL);
>>> +		if (!temp_config)
>>> +			return -ENOMEM;
>>> +
>>> +		for (i = 0; i < ts_num; i++)
>>> +			temp_config[i] = HWMON_T_INPUT;
>>> +
>>> +		temp_config[ts_num] = 0;
>>> +		pvt_temp.config = temp_config;
>>> +
>>> +		pvt_info[index++] = &pvt_temp;
>>> +	}
>>> +
>>> +	if (pd_num) {
>>> +		ret = pvt_get_regmap(pdev, "pd");
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	if (vm_num) {
>>> +		u32 num = vm_num;
>>> +
>>> +		ret = pvt_get_regmap(pdev, "vm");
>>> +		if (ret)
>>> +			return ret;
>>> +
>>> +		pvt->vm_idx = devm_kcalloc(dev, vm_num, sizeof(*pvt->vm_idx),
>>> +					   GFP_KERNEL);
>>> +		if (!pvt->vm_idx)
>>> +			return -ENOMEM;
>>> +
>>> +		for (i = 0; i < vm_num; i++)
>>> +			pvt->vm_idx[i] = i;
>>> +
>>> +		ret = of_property_read_u8_array(dev->of_node, "vm-map",
>>> +						pvt->vm_idx, vm_num);
>>> +
>>> +		if (!ret)
>>> +			for (i = 0; i < vm_num; i++)
>>> +				if (pvt->vm_idx[i] >= vm_num ||
>>> +				    pvt->vm_idx[i] == 0xff) {
>>> +					num = i;
>>> +					break;
>>> +				}
>>> +
>>> +		in_config = devm_kcalloc(dev, num + 1,
>>> +					 sizeof(*in_config), GFP_KERNEL);
>>> +		if (!in_config)
>>> +			return -ENOMEM;
>>> +
>>> +		for (i = 0; i < num; i++)
>>> +			in_config[i] = HWMON_I_INPUT;
>>> +
>>> +		in_config[num] = 0;
>>> +		pvt_in.config = in_config;
>>> +
>>> +		pvt_info[index++] = &pvt_in;
>>> +	}
>>> +
>>> +	ret = pvt_init(pvt);
>>> +	if (ret) {
>>> +		dev_err(dev, "failed to init pvt: %d\n", ret);
>>> +		return ret;
>>> +	}
>>> +
>>> +	mutex_init(&pvt->lock);
>>> +
>>> +	pvt_info[index] = NULL;
>> Unnecessary.
> 
> Well noted.
> 
>>> +	pvt_chip_info.info = pvt_info;
>>> +	hwmon_dev = devm_hwmon_device_register_with_info(dev, "pvt",
>>> +							 pvt,
>>> +							 &pvt_chip_info,
>>> +							 NULL);
>>> +
>>> +	platform_set_drvdata(pdev, pvt);
>> Again, I fail to understand how this is supposed to work.
> 
> Yup, this was a mistake. This is supposed to be on the top of probe just after
> pvt_device allocation. Will fix it in v2.
> 

Seems to be pretty useless, though. It is only used from the probe function,
to avoid passing an extra parameter to pvt_get_regmap(). Why not just pass
this parameter ?

Guenter

>>> +	return PTR_ERR_OR_ZERO(hwmon_dev);
>>> +}
>>> +
>>> +static const struct of_device_id moortec_pvt_of_match[] = {
>>> +	{ .compatible = "moortec,mr75203" },
>>> +	{ }
>>> +};
>>> +MODULE_DEVICE_TABLE(of, moortec_pvt_of_match);
>>> +
>>> +static struct platform_driver moortec_pvt_driver = {
>>> +	.driver = {
>>> +		.name = "moortec-pvt",
>>> +		.of_match_table = moortec_pvt_of_match,
>>> +	},
>>> +	.probe = mr75203_probe,
>>> +};
>>> +module_platform_driver(moortec_pvt_driver);
>>>
> 
> Regards,
> Rahul
> 

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Andy Shevchenko]

On Thu, Sep 10, 2020 at 03:27:11PM +0800, Tanwar, Rahul wrote:
> On 9/9/2020 11:05 pm, Guenter Roeck wrote:
> > On 9/8/20 11:52 PM, Rahul Tanwar wrote:

...

> >> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
> >> +{
> >> +	struct device *dev = &pdev->dev;
> >> +	struct pvt_device *pvt = platform_get_drvdata(pdev);
> > I am quite at loss how this is supposed to work. Platform driver
> > data is not initialized with a pointer to struct pvt_device at this point.
> > How does this code not crash ? What am I missing ?
> 
> Big mistake on my part. Last minute change based on internal review feedback
> about moving platform_set_drvdata() at the end of probe. I will fix it in v2.
> Thanks.

Since IIRC it was me who suggested this I should say that reviewer can make
mistakes, on the other hand contributor should have had known their code to
refuse certain changes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Tanwar, Rahul]

On 10/9/2020 5:46 pm, Andy Shevchenko wrote:
> On Thu, Sep 10, 2020 at 03:27:11PM +0800, Tanwar, Rahul wrote:
>> On 9/9/2020 11:05 pm, Guenter Roeck wrote:
>>> On 9/8/20 11:52 PM, Rahul Tanwar wrote:
> ...
>
>>>> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
>>>> +{
>>>> +	struct device *dev = &pdev->dev;
>>>> +	struct pvt_device *pvt = platform_get_drvdata(pdev);
>>> I am quite at loss how this is supposed to work. Platform driver
>>> data is not initialized with a pointer to struct pvt_device at this point.
>>> How does this code not crash ? What am I missing ?
>> Big mistake on my part. Last minute change based on internal review feedback
>> about moving platform_set_drvdata() at the end of probe. I will fix it in v2.
>> Thanks.
> Since IIRC it was me who suggested this I should say that reviewer can make
> mistakes, on the other hand contributor should have had known their code to
> refuse certain changes.
>

Totally agree. Overlooked in a hasty change..

Regards,
Rahul

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Philipp Zabel]

On Wed, 2020-09-09 at 14:52 +0800, Rahul Tanwar wrote:
> PVT controller (MR75203) is used to configure & control
> Moortec embedded analog IP which contains temprature
> sensor(TS), voltage monitor(VM) & process detector(PD)
> modules. Add driver to support MR75203 PVT controller.
> 
> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
> ---
[...]
> +static int mr75203_probe(struct platform_device *pdev)
> +{
> +	const struct hwmon_channel_info **pvt_info;
> +	u32 ts_num, vm_num, pd_num, val, index, i;
> +	struct device *dev = &pdev->dev;
> +	u32 *temp_config, *in_config;
> +	struct device *hwmon_dev;
> +	struct pvt_device *pvt;
> +	int ret;
> +
> +	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
> +	if (!pvt)
> +		return -ENOMEM;
> +
> +	ret = pvt_get_regmap(pdev, "common");
> +	if (ret)
> +		return ret;
> +
> +	pvt->rst = devm_reset_control_get(dev, NULL);

Please use devm_reset_control_get_exclusive().

regards
Philipp

Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

[Tanwar, Rahul]

On 10/9/2020 6:35 pm, Philipp Zabel wrote:
> On Wed, 2020-09-09 at 14:52 +0800, Rahul Tanwar wrote:
>> PVT controller (MR75203) is used to configure & control
>> Moortec embedded analog IP which contains temprature
>> sensor(TS), voltage monitor(VM) & process detector(PD)
>> modules. Add driver to support MR75203 PVT controller.
>>
>> Signed-off-by: Rahul Tanwar <rahul.tanwar@linux.intel.com>
>> ---
> [...]
>> +static int mr75203_probe(struct platform_device *pdev)
>> +{
>> +	const struct hwmon_channel_info **pvt_info;
>> +	u32 ts_num, vm_num, pd_num, val, index, i;
>> +	struct device *dev = &pdev->dev;
>> +	u32 *temp_config, *in_config;
>> +	struct device *hwmon_dev;
>> +	struct pvt_device *pvt;
>> +	int ret;
>> +
>> +	pvt = devm_kzalloc(dev, sizeof(*pvt), GFP_KERNEL);
>> +	if (!pvt)
>> +		return -ENOMEM;
>> +
>> +	ret = pvt_get_regmap(pdev, "common");
>> +	if (ret)
>> +		return ret;
>> +
>> +	pvt->rst = devm_reset_control_get(dev, NULL);
> Please use devm_reset_control_get_exclusive().

Well noted. Missed that, thanks.

Regards,
Rahul