Qualcomm Universal Peripheral (QUP) I2C controller

From: "Ivan T. Ivanov" <[email protected]>

The Qualcomm Universal Peripherial (QUP) wraps I2C mini-core and provide input and output FIFO's for it. I2C controller can operate as master with supported bus speeds of 100Kbps and 400Kbps.

Signed-off-by Ivan T. Ivanov <[email protected]>

[bjorn: reformulated part of binding description and cleaned up example]

Signed-off-by Bjorn Andersson <[email protected]>

---
 .../devicetree/bindings/i2c/qcom,i2c-qup.txt       | 41 ++++++++++++++++++++++
 1 file changed, 41 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +0 -0

new file mode 100644
index 0000000..a99711b

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +10 -0

+ - clock-names: Should be "core" for the core clock and "iface" for the
+                AHB clock.
+
+ - #address-cells: Should be <1> Address cells for i2c device address
+ - #size-cells: Should be <0> as i2c addresses have no size component
+
+Optional properties:
+ - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
+                    default is 100kHz if omitted.
+

Thanks Bjorn,

I have prepared second version, but never send it out :-). One thing suggested by Mark was missed in this version.

Quoted 2 lines

From: "Ivan T. Ivanov" <[email protected]>

Quoted 1 line

+ - clocks: Should contain the core clock and the AHB clock.

+ - clocks: a list of phandle + clock-specifier pairs for each entry in
+ clock-names

Quoted 10 lines

+ - clock-names: Should be "core" for the core clock and "iface" for the

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +9 -0

+                AHB clock.
+
+ - #address-cells: Should be <1> Address cells for i2c device address
+ - #size-cells: Should be <0> as i2c addresses have no size component
+
+Optional properties:
+ - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
+                    default is 100kHz if omitted.
+

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt line 28

Quoted 10 lines

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +10 -0

+Child nodes should conform to i2c bus binding.
+
+Example:
+
+ i2c2: i2c at f9924000 {
+ 	compatible = "qcom,i2c-qup";
+ 	reg = <0xf9924000 0x1000>;
+ 	interrupts = <0 96 0>;
+
+ 	clocks = <&gcc_blsp1_qup2_i2c_apps_clk>, <&gcc_blsp1_ahb_clk>;

In the light of the latest patches from Stephen, this could be

Quoted 5 lines · Ivan T. Ivanov

Thanks Bjorn,

I have prepared second version, but never send it out :-). One thing suggested by Mark was missed in this version.

Yeah, Mattew told me you we're assigned to other things and asked me to send out an update as I had gotten it to work on our boards.

I did modify the wording of most of these to match how it is written in the other Qualcomm definitions.

@Mark, would you rather have me change this to your suggested wording?

Quoted 5 lines · Bjorn Andersson

Quoted 2 lines

From: "Ivan T. Ivanov" <[email protected]>

[snip]

Quoted 5 lines

Quoted 1 line

+ - clocks: Should contain the core clock and the AHB clock.

+ - clocks: a list of phandle + clock-specifier pairs for each entry in
+ clock-names

This is in line with how it's written in other drivers, so if the DT maintainers doesn't disagree I would like to keep phandles out of the description. This specific line is a verbatime copy of the msm_serial documentation (same block, different mode)...

Quoted 23 lines

Quoted 10 lines

+ - clock-names: Should be "core" for the core clock and "iface" for the

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +9 -0

+                AHB clock.
+
+ - #address-cells: Should be <1> Address cells for i2c device address
+ - #size-cells: Should be <0> as i2c addresses have no size component
+
+Optional properties:
+ - clock-frequency: Should specify the desired i2c bus clock frequency in Hz,
+                    default is 100kHz if omitted.
+

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt line 28

Quoted 10 lines

Documentation/devicetree/bindings/i2c/qcom,i2c-qup.txt new file +10 -0

+Child nodes should conform to i2c bus binding.
+
+Example:
+
+ i2c2: i2c at f9924000 {
+ 	compatible = "qcom,i2c-qup";
+ 	reg = <0xf9924000 0x1000>;
+ 	interrupts = <0 96 0>;
+
+ 	clocks = <&gcc_blsp1_qup2_i2c_apps_clk>, <&gcc_blsp1_ahb_clk>;

In the light of the latest patches from Stephen, this could be

Review

Quoted 1 line

(quoted diff) +1 -0

+ 	clocks = <&gcc GCC_BLSP1_QUP2_I2C_APPS_CLK>, <&gcc GCC_BLSP1_AHB_CLK>;

Yes, that's exactly what I have in my dts. However as this is just an example I didn't feel it was worth tainting the documentation with all those capital letters ;)
So unless DT maintainers disagree I would like to just keep it as an example.

Regards,
Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

From: "Ivan T. Ivanov" <[email protected]>

This bus driver supports the QUP i2c hardware controller in the Qualcomm MSM SOCs. The Qualcomm Universal Peripheral Engine (QUP) is a general purpose data path engine with input/output FIFOs and an embedded i2c
mini-core. The driver supports FIFO mode (for low bandwidth applications)
and block mode (interrupt generated for each block-size data transfer). The driver currently does not support DMA transfers.

Shamelessly based on codeaurora version of the driver.

Signed-off-by Ivan T. Ivanov <[email protected]>

[bjorn: updated to reflect i2c framework changes
splited up qup_i2c_enable() in enable/disable don't overwrite ret value on error in xfer functions initilize core for each transfer
remove explicit pinctrl selection
use existing clock instead of setting new core clock]

Signed-off-by Bjorn Andersson <[email protected]>

---

drivers/i2c/busses/i2c-qup.c new file +9 -0

+
+static int qup_i2c_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct qup_i2c_dev *qup;
+	struct resource *res;
+	u32 val, io_mode, hw_ver, size;
+	int ret, fs_div, hs_div;
+	int src_clk_freq;

drivers/i2c/busses/i2c-qup.c new file +97 -0

+
+	qup->dev = &pdev->dev;
+
+	qup->clk_freq = 100000;
+	if (!of_property_read_u32(node, "clock-frequency", &val))
+		qup->clk_freq = val;
+
+	/* We support frequencies up to FAST Mode(400KHz) */
+	if (qup->clk_freq <= 0 || qup->clk_freq > 400000) {
+		dev_err(qup->dev, "clock frequency not supported %d\n",
+			qup->clk_freq);
+		return -EIO;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	qup->base = devm_ioremap_resource(qup->dev, res);
+	if (IS_ERR(qup->base))
+		return PTR_ERR(qup->base);
+
+	qup->irq = platform_get_irq(pdev, 0);
+	if (qup->irq < 0) {
+		dev_err(qup->dev, "No IRQ defined\n");
+		return qup->irq;
+	}
+
+	qup->clk = devm_clk_get(qup->dev, "core");
+	if (IS_ERR(qup->clk)) {
+		dev_err(qup->dev, "Could not get core clock\n");
+		return PTR_ERR(qup->clk);
+	}
+
+	qup->pclk = devm_clk_get(qup->dev, "iface");
+	if (IS_ERR(qup->pclk)) {
+		dev_err(qup->dev, "Could not get iface clock\n");
+		return PTR_ERR(qup->pclk);
+	}
+
+	init_completion(&qup->xfer);
+
+	qup_i2c_enable_clocks(qup);
+
+	/*
+	 * Bootloaders might leave a pending interrupt on certain QUP's,
+	 * so we reset the core before registering for interrupts.
+	 */
+	writel(1, qup->base + QUP_SW_RESET);
+	ret = qup_i2c_poll_state(qup, 0, true);
+	if (ret)
+		goto fail;
+
+	ret = devm_request_irq(qup->dev, qup->irq, qup_i2c_interrupt,
+			       IRQF_TRIGGER_HIGH, "i2c_qup", qup);
+	if (ret) {
+		dev_err(qup->dev, "Request %d IRQ failed\n", qup->irq);
+		goto fail;
+	}
+	disable_irq(qup->irq);
+
+	hw_ver = readl(qup->base + QUP_HW_VERSION);
+	dev_dbg(qup->dev, "%d Revision %x\n", pdev->id, hw_ver);
+
+	src_clk_freq = clk_get_rate(qup->clk);
+	fs_div = ((src_clk_freq / qup->clk_freq) / 2) - 3;
+	hs_div = 3;
+	qup->clk_ctl = (hs_div << 8) | (fs_div & 0xff);
+	qup->one_bit_t = (USEC_PER_SEC / qup->clk_freq) + 1;
+
+	io_mode = readl(qup->base + QUP_IO_MODE);
+
+	size = QUP_OUTPUT_BLOCK_SIZE(io_mode);
+	if (size)
+		qup->out_blk_sz = size * 16;
+	else
+		qup->out_blk_sz = 16;
+
+	size = QUP_INPUT_BLOCK_SIZE(io_mode);
+	if (size)
+		qup->in_blk_sz = size * 16;
+	else
+		qup->in_blk_sz = 16;
+
+	qup->xfer_time = msecs_to_jiffies(qup->out_fifo_sz);
+
+	/*
+	 * The block/fifo size w.r.t. 'actual data' is 1/2 due to 'tag'
+	 * associated with each byte written/received
+	 */
+	qup->out_blk_sz /= 2;
+	qup->in_blk_sz /= 2;
+
+	size = QUP_OUTPUT_FIFO_SIZE(io_mode);
+	qup->out_fifo_sz = qup->out_blk_sz * (2 << size);
+
+	size = QUP_INPUT_FIFO_SIZE(io_mode);
+	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
+
+	/*

drivers/i2c/busses/i2c-qup.c new file +10 -0

+
+	i2c_set_adapdata(&qup->adap, qup);
+	qup->adap.algo = &qup_i2c_algo;
+	qup->adap.nr = pdev->id;
+	qup->adap.dev.parent = qup->dev;
+	qup->adap.dev.of_node = pdev->dev.of_node;
+	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
+
+	ret = i2c_add_numbered_adapter(&qup->adap);
+	if (!ret) {

+/*
+ * QUP driver for Qualcomm MSM platforms
+ *
+ */

drivers/i2c/busses/i2c-qup.c new file +13 -0

+	 * equal of fifo size or block size (in block mode)
+	 */
+	int			chunk_sz;
+	struct completion	xfer;
+};
+
+static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
+{
+	struct qup_i2c_dev *qup = dev;
+	u32 bus_err;
+	u32 qup_err;
+	u32 opflags;
+

drivers/i2c/busses/i2c-qup.c new file +15 -0

+		return IRQ_HANDLED;
+
+	if ((opflags & QUP_MX_INPUT_DONE) || (opflags & QUP_IN_SVC_FLAG))
+		writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+
+done:
+	qup->qup_err = qup_err;
+	qup->bus_err = bus_err;
+	complete(&qup->xfer);
+	return IRQ_HANDLED;
+}
+
+static int
+qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid)
+{

drivers/i2c/busses/i2c-qup.c new file +4 -0

+		if (state & QUP_STATE_VALID) {
+			if (only_valid)
+				return 0;
+			if ((req_state & QUP_I2C_MAST_GEN)

drivers/i2c/busses/i2c-qup.c new file +4 -0

+
+	return -ETIMEDOUT;
+}
+

drivers/i2c/busses/i2c-qup.c new file +16 -0

+static void qup_i2c_issue_write(struct qup_i2c_dev *qup, struct i2c_msg *msg)
+{
+	u32 addr = msg->addr << 1;
+	u32 val, qup_tag;
+	int idx, entries;
+
+	if (qup->pos == 0) {
+		val = QUP_OUT_START | addr;
+	} else {
+		/*
+		 * Avoid setup time issue by adding 1 NOP when number of bytes
+		 * are more than FIFO/BLOCK size. setup time issue can't appear
+		 * otherwise since next byte to be written will always be ready
+		 */
+		val = (QUP_OUT_NOP | 1);
+	}

drivers/i2c/busses/i2c-qup.c new file +7 -0

+
+		if (idx & 1) {
+			val |= (qup_tag | msg->buf[qup->pos]) << QUP_MSW_SHIFT;
+			writel(val, qup->base + QUP_OUT_FIFO_BASE);
+		} else {
+			val = qup_tag | msg->buf[qup->pos];
+		}

drivers/i2c/busses/i2c-qup.c new file +36 -0

+static int qup_i2c_pm_suspend_runtime(struct device *device)
+{
+	struct qup_i2c_dev *qup = dev_get_drvdata(device);
+
+	dev_dbg(device, "pm_runtime: suspending...\n");
+	qup_i2c_disable_clocks(qup);
+	return 0;
+}
+
+static int qup_i2c_pm_resume_runtime(struct device *device)
+{
+	struct qup_i2c_dev *qup = dev_get_drvdata(device);
+
+	dev_dbg(device, "pm_runtime: resuming...\n");
+	qup_i2c_enable_clocks(qup);
+	return 0;
+}
+
+static int qup_i2c_suspend(struct device *device)
+{
+	dev_dbg(device, "system suspend");
+	qup_i2c_pm_suspend_runtime(device);
+	return 0;
+}
+
+static int qup_i2c_resume(struct device *device)
+{
+	dev_dbg(device, "system resume");
+	qup_i2c_pm_resume_runtime(device);
+	pm_runtime_mark_last_busy(device);
+	pm_request_autosuspend(device);
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops qup_i2c_qup_pm_ops = {

Quoted 4 lines · Bjorn Andersson

+/*
+ * QUP driver for Qualcomm MSM platforms
+ *
+ */

This comment seems redundant, we know what file we're looking at.

drivers/i2c/busses/i2c-qup.c line 116

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+struct qup_i2c_dev {
+	struct device		*dev;
+	void __iomem		*base;
+	struct pinctrl		*pctrl;

This is unused.

drivers/i2c/busses/i2c-qup.c line 122

Quoted 6 lines

drivers/i2c/busses/i2c-qup.c new file +6 -0

+	int			irq;
+	struct clk		*clk;
+	struct clk		*pclk;
+	struct i2c_adapter	adap;
+
+	int			clk_freq;

This is only ever used in probe, so why do we need to store it away?

drivers/i2c/busses/i2c-qup.c line 133

Quoted 11 lines

drivers/i2c/busses/i2c-qup.c new file +11 -0

+	int			clk_ctl;
+	int			one_bit_t;
+	int			out_fifo_sz;
+	int			in_fifo_sz;
+	int			out_blk_sz;
+	int			in_blk_sz;
+	unsigned long		xfer_time;
+	unsigned long		wait_idle;
+
+	struct i2c_msg		*msg;
+	/* Current possion in user message buffer */

s/possion/position/?

drivers/i2c/busses/i2c-qup.c line 142

Quoted 9 lines

drivers/i2c/busses/i2c-qup.c new file +9 -0

+	int			pos;
+	/* Keep number of bytes left to be transmitted */
+	int			cnt;
+	/* I2C protocol errors */
+	u32			bus_err;
+	/* QUP core errors */
+	u32			qup_err;
+	/*
+	 * maximum bytes that could be send (per iterration). could be

s/iterration/iteration/?

Quoted 13 lines

drivers/i2c/busses/i2c-qup.c new file +13 -0

+	 * equal of fifo size or block size (in block mode)
+	 */
+	int			chunk_sz;
+	struct completion	xfer;
+};
+
+static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
+{
+	struct qup_i2c_dev *qup = dev;
+	u32 bus_err;
+	u32 qup_err;
+	u32 opflags;
+

[...]

drivers/i2c/busses/i2c-qup.c line 185

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+	if (opflags & QUP_OUT_SVC_FLAG)
+		writel(QUP_OUT_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+
+	if (!(qup->msg->flags == I2C_M_RD))

Should this be?

	if (!(qup->msg->flags & I2C_M_RD))

Otherwise it should be

	if (qup->msg->flags != I2C_M_RD)

Quoted 12 lines

drivers/i2c/busses/i2c-qup.c new file +12 -0

+		return IRQ_HANDLED;
+
+	if ((opflags & QUP_MX_INPUT_DONE) || (opflags & QUP_IN_SVC_FLAG))
+		writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+
+done:
+	qup->qup_err = qup_err;
+	qup->bus_err = bus_err;
+	complete(&qup->xfer);
+	return IRQ_HANDLED;
+}
+

Quoted 4 lines · Bjorn Andersson

+/*
+ * QUP driver for Qualcomm MSM platforms
+ *
+ */

This comment seems redundant, we know what file we're looking at.

drivers/i2c/busses/i2c-qup.c line 116

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+struct qup_i2c_dev {
+	struct device		*dev;
+	void __iomem		*base;
+	struct pinctrl		*pctrl;

This is unused.

drivers/i2c/busses/i2c-qup.c line 122

Quoted 6 lines

drivers/i2c/busses/i2c-qup.c new file +6 -0

+	int			irq;
+	struct clk		*clk;
+	struct clk		*pclk;
+	struct i2c_adapter	adap;
+
+	int			clk_freq;

This is only ever used in probe, so why do we need to store it away?

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+	int			clk_ctl;
+	int			one_bit_t;
+	int			out_fifo_sz;
+	int			in_fifo_sz;
+	int			out_blk_sz;

drivers/i2c/busses/i2c-qup.c line 133

Quoted 6 lines

drivers/i2c/busses/i2c-qup.c new file +6 -0

+	int			in_blk_sz;
+	unsigned long		xfer_time;
+	unsigned long		wait_idle;
+
+	struct i2c_msg		*msg;
+	/* Current possion in user message buffer */

s/possion/position/?

drivers/i2c/busses/i2c-qup.c line 142

Quoted 9 lines

drivers/i2c/busses/i2c-qup.c new file +9 -0

+	int			pos;
+	/* Keep number of bytes left to be transmitted */
+	int			cnt;
+	/* I2C protocol errors */
+	u32			bus_err;
+	/* QUP core errors */
+	u32			qup_err;
+	/*
+	 * maximum bytes that could be send (per iterration). could be

s/iterration/iteration/?

Quoted 13 lines

drivers/i2c/busses/i2c-qup.c new file +13 -0

+	 * equal of fifo size or block size (in block mode)
+	 */
+	int			chunk_sz;
+	struct completion	xfer;
+};
+
+static irqreturn_t qup_i2c_interrupt(int irq, void *dev)
+{
+	struct qup_i2c_dev *qup = dev;
+	u32 bus_err;
+	u32 qup_err;
+	u32 opflags;
+

[...]

drivers/i2c/busses/i2c-qup.c line 185

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+	if (opflags & QUP_OUT_SVC_FLAG)
+		writel(QUP_OUT_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+
+	if (!(qup->msg->flags == I2C_M_RD))

Should this be?

	if (!(qup->msg->flags & I2C_M_RD))

Otherwise it should be

	if (qup->msg->flags != I2C_M_RD)

Quoted 20 lines

drivers/i2c/busses/i2c-qup.c new file +20 -0

+		return IRQ_HANDLED;
+
+	if ((opflags & QUP_MX_INPUT_DONE) || (opflags & QUP_IN_SVC_FLAG))
+		writel(QUP_IN_SVC_FLAG, qup->base + QUP_OPERATIONAL);
+
+done:
+	qup->qup_err = qup_err;
+	qup->bus_err = bus_err;
+	complete(&qup->xfer);
+	return IRQ_HANDLED;
+}
+
+static int
+qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid)
+{
+	int retries = 0;
+	u32 state;
+
+	do {
+		state = readl(qup->base + QUP_STATE);

drivers/i2c/busses/i2c-qup.c line 210

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+		/*
+		 * If only valid bit needs to be checked, requested state is
+		 * 'don't care'
+		 */

It looks like req_state == 0 means only_valid == true. Can we drop the only_valid argument to this function?

Quoted 8 lines

drivers/i2c/busses/i2c-qup.c new file +8 -0

+		if (state & QUP_STATE_VALID) {
+			if (only_valid)
+				return 0;
+			if ((req_state & QUP_I2C_MAST_GEN)
+			    && (state & QUP_I2C_MAST_GEN))
+				return 0;
+			if ((state & QUP_STATE_MASK) == req_state)
+				return 0;

drivers/i2c/busses/i2c-qup.c line 224

Quoted 6 lines

drivers/i2c/busses/i2c-qup.c new file +6 -0

+		}
+
+		if (retries++ == 1000)
+			udelay(100);
+
+	} while (retries != 2000);

Please makes #defines for 1000 and 2000.

Quoted 4 lines

drivers/i2c/busses/i2c-qup.c new file +4 -0

+
+	return -ETIMEDOUT;
+}
+

[...]

Quoted 20 lines

drivers/i2c/busses/i2c-qup.c new file +20 -0

+static void qup_i2c_issue_write(struct qup_i2c_dev *qup, struct i2c_msg *msg)
+{
+	u32 addr = msg->addr << 1;
+	u32 val, qup_tag;
+	int idx, entries;
+
+	if (qup->pos == 0) {
+		val = QUP_OUT_START | addr;
+	} else {
+		/*
+		 * Avoid setup time issue by adding 1 NOP when number of bytes
+		 * are more than FIFO/BLOCK size. setup time issue can't appear
+		 * otherwise since next byte to be written will always be ready
+		 */
+		val = (QUP_OUT_NOP | 1);
+	}
+
+	entries = qup->cnt + 1;
+
+	if (entries > qup->chunk_sz)

drivers/i2c/busses/i2c-qup.c line 430

Quoted 6 lines

drivers/i2c/busses/i2c-qup.c new file +6 -0

+		entries = qup->chunk_sz;
+
+	qup_tag = QUP_OUT_DATA;
+
+	/* Reserve one entry for STOP */
+	for (idx = 1; idx < (entries - 1); idx++, qup->pos++) {

Unnecessary () here around entries.

Quoted 7 lines

drivers/i2c/busses/i2c-qup.c new file +7 -0

+
+		if (idx & 1) {
+			val |= (qup_tag | msg->buf[qup->pos]) << QUP_MSW_SHIFT;
+			writel(val, qup->base + QUP_OUT_FIFO_BASE);
+		} else {
+			val = qup_tag | msg->buf[qup->pos];
+		}

[...]

drivers/i2c/busses/i2c-qup.c line 832

Quoted 2 lines

drivers/i2c/busses/i2c-qup.c new file +2 -0

+
+#ifdef CONFIG_PM

This ifdef is probably wrong considering that you can disable CONFIG_PM_RUNTIME without disabling CONFIG_PM and then these runtime PM functions would be unused.

Quoted 40 lines

drivers/i2c/busses/i2c-qup.c new file +40 -0

+static int qup_i2c_pm_suspend_runtime(struct device *device)
+{
+	struct qup_i2c_dev *qup = dev_get_drvdata(device);
+
+	dev_dbg(device, "pm_runtime: suspending...\n");
+	qup_i2c_disable_clocks(qup);
+	return 0;
+}
+
+static int qup_i2c_pm_resume_runtime(struct device *device)
+{
+	struct qup_i2c_dev *qup = dev_get_drvdata(device);
+
+	dev_dbg(device, "pm_runtime: resuming...\n");
+	qup_i2c_enable_clocks(qup);
+	return 0;
+}
+
+static int qup_i2c_suspend(struct device *device)
+{
+	dev_dbg(device, "system suspend");
+	qup_i2c_pm_suspend_runtime(device);
+	return 0;
+}
+
+static int qup_i2c_resume(struct device *device)
+{
+	dev_dbg(device, "system resume");
+	qup_i2c_pm_resume_runtime(device);
+	pm_runtime_mark_last_busy(device);
+	pm_request_autosuspend(device);
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops qup_i2c_qup_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(
+		qup_i2c_suspend,
+		qup_i2c_resume)
+	SET_RUNTIME_PM_OPS(

drivers/i2c/busses/i2c-qup.c line 878

Quoted 6 lines

drivers/i2c/busses/i2c-qup.c new file +6 -0

+		qup_i2c_pm_suspend_runtime,
+		qup_i2c_pm_resume_runtime,
+		NULL)
+};
+
+static struct of_device_id qup_i2c_dt_match[] = {

const?

drivers/i2c/busses/i2c-qup.c line 881

Quoted 3 lines

drivers/i2c/busses/i2c-qup.c new file +3 -0

+	{.compatible = "qcom,i2c-qup"},
+	{}
+};

MODULE_DEVICE_TABLE(of, qup_i2c_dt_match)?

Quoted 10 lines · Stephen Boyd

Quoted 9 lines · Bjorn Andersson

drivers/i2c/busses/i2c-qup.c new file +9 -0

+
+static int
+qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid)
+{
+	int retries = 0;
+	u32 state;
+
+	do {
+		state = readl(qup->base + QUP_STATE);

Quoted 9 lines

drivers/i2c/busses/i2c-qup.c line 210

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+		/*
+		 * If only valid bit needs to be checked, requested state is
+		 * 'don't care'
+		 */

It looks like req_state == 0 means only_valid == true. Can we drop the only_valid argument to this function?

In all cases but the reset in the beginning of qup_i2c_xfer, so it seems that it has to stay.

Quoted 22 lines · Bjorn Andersson

Quoted 10 lines · Stephen Boyd

Quoted 9 lines · Bjorn Andersson

drivers/i2c/busses/i2c-qup.c new file +9 -0

+
+static int
+qup_i2c_poll_state(struct qup_i2c_dev *qup, u32 req_state, bool only_valid)
+{
+	int retries = 0;
+	u32 state;
+
+	do {
+		state = readl(qup->base + QUP_STATE);

Quoted 9 lines

drivers/i2c/busses/i2c-qup.c line 210

Quoted 5 lines

drivers/i2c/busses/i2c-qup.c new file +5 -0

+
+		/*
+		 * If only valid bit needs to be checked, requested state is
+		 * 'don't care'
+		 */

It looks like req_state == 0 means only_valid == true. Can we drop the only_valid argument to this function?

Quoted 2 lines

In all cases but the reset in the beginning of qup_i2c_xfer, so it seems that it has to stay.

Oh that's because QUP_RESET_STATE is equal to 0? It looks like bits 0 and 1 are the state field and bit 2 is a flag indicating that bits 0 and 1 are valid. Why not OR in the QUP_STATE_VALID flag into the macros that are passed to this function? Then the logic would simply be looping looking for a match of the req_state (which is really a mask now).