[PATCH 1/2] i2c: qup: add ACPI support

[PATCH 1/2] i2c: qup: add ACPI support

[Naveen Kaje]

Add support to get the device parameters from ACPI. Assume that
the clocks are managed by firmware.

Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 61 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 46 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 23eaabb..ef31b26 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -29,6 +29,7 @@
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/scatterlist.h>
+#include <linux/acpi.h>
 
 /* QUP Registers */
 #define QUP_CONFIG		0x000
@@ -132,6 +133,10 @@
 /* Max timeout in ms for 32k bytes */
 #define TOUT_MAX			300
 
+/* Default values. Use these if FW query fails */
+#define DEFAULT_CLK_FREQ 100000
+#define DEFAULT_SRC_CLK 20000000
+
 struct qup_i2c_block {
 	int	count;
 	int	pos;
@@ -1360,9 +1365,10 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	struct resource *res;
 	u32 io_mode, hw_ver, size;
 	int ret, fs_div, hs_div;
-	int src_clk_freq;
-	u32 clk_freq = 100000;
+	u32 src_clk_freq = 0;
+	u32 clk_freq = 0;
 	int blocks;
+	bool input_clk_ctrl = true;
 
 	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
 	if (!qup)
@@ -1372,7 +1378,14 @@ static int qup_i2c_probe(struct platform_device *pdev)
 	init_completion(&qup->xfer);
 	platform_set_drvdata(pdev, qup);
 
-	of_property_read_u32(node, "clock-frequency", &clk_freq);
+	ret = device_property_read_u32(qup->dev, "clock-frequency", &clk_freq);
+	if (ret) {
+		dev_warn(qup->dev, "using default clock-frequency values");
+		clk_freq = DEFAULT_CLK_FREQ;
+	}
+
+	if (ACPI_HANDLE(qup->dev) && !node)
+		input_clk_ctrl = false;
 
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-v1.1.1")) {
 		qup->adap.algo = &qup_i2c_algo;
@@ -1454,20 +1467,29 @@ nodma:
 		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);
-	}
+	if (input_clk_ctrl) {
+		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);
+		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);
+		}
+		qup_i2c_enable_clocks(qup);
+		src_clk_freq = clk_get_rate(qup->clk);
+	} else {
+		ret = device_property_read_u32(qup->dev,
+				"src-clock-hz", &src_clk_freq);
+		if (ret) {
+			dev_warn(qup->dev, "using default src-clock-hz");
+			src_clk_freq = DEFAULT_SRC_CLK;
+		}
 	}
 
-	qup_i2c_enable_clocks(qup);
-
 	/*
 	 * Bootloaders might leave a pending interrupt on certain QUP's,
 	 * so we reset the core before registering for interrupts.
@@ -1514,7 +1536,6 @@ nodma:
 	size = QUP_INPUT_FIFO_SIZE(io_mode);
 	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
 
-	src_clk_freq = clk_get_rate(qup->clk);
 	fs_div = ((src_clk_freq / clk_freq) / 2) - 3;
 	hs_div = 3;
 	qup->clk_ctl = (hs_div << 8) | (fs_div & 0xff);
@@ -1534,6 +1555,8 @@ nodma:
 	qup->adap.dev.parent = qup->dev;
 	qup->adap.dev.of_node = pdev->dev.of_node;
 	qup->is_last = true;
+	if (ACPI_HANDLE(qup->dev))
+		ACPI_COMPANION_SET(&qup->adap.dev, ACPI_COMPANION(qup->dev));
 
 	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup->adap.name));
 
@@ -1639,6 +1662,13 @@ static const struct of_device_id qup_i2c_dt_match[] = {
 };
 MODULE_DEVICE_TABLE(of, qup_i2c_dt_match);
 
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id qup_i2c_acpi_match[] = {
+	{ "QCOM8010"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids);
+#endif
 static struct platform_driver qup_i2c_driver = {
 	.probe  = qup_i2c_probe,
 	.remove = qup_i2c_remove,
@@ -1646,6 +1676,7 @@ static struct platform_driver qup_i2c_driver = {
 		.name = "i2c_qup",
 		.pm = &qup_i2c_qup_pm_ops,
 		.of_match_table = qup_i2c_dt_match,
+		.acpi_match_table = ACPI_PTR(qup_i2c_acpi_match),
 	},
 };
 
-- 
1.8.2.1

[PATCH 2/2] i2c: qup: support SMBus block read

[Naveen Kaje]

I2C QUP driver relies on SMBus emulation support from the framework.
To handle SMBus block reads, the driver should check I2C_M_RECV_LEN
flag and should read the first byte received as the message length.

The driver configures the QUP hardware to read one byte. Once the
message length is known from this byte, the QUP hardware is
configured to read the rest.

Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
---
 drivers/i2c/busses/i2c-qup.c | 95 ++++++++++++++++++++++++++++++--------------
 1 file changed, 66 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index ef31b26..2658b67 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -526,38 +526,49 @@ static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
 
 	int last = (qup->blk.pos == (qup->blk.count - 1)) && (qup->is_last);
 
-	if (qup->blk.pos == 0) {
-		tags[len++] = QUP_TAG_V2_START;
-		tags[len++] = addr & 0xff;
+	/* Handle SMBus block data read */
+	if ((msg->flags & I2C_M_RD) && (msg->flags & I2C_M_RECV_LEN) &&
+	    (msg->len > 1)) {
+		tags[len++] = QUP_TAG_V2_DATARD_STOP;
+		data_len = qup_i2c_get_data_len(qup);
+		tags[len++] = data_len - 1;
+	} else {
+		if (qup->blk.pos == 0) {
+			tags[len++] = QUP_TAG_V2_START;
+			tags[len++] = addr & 0xff;
 
-		if (msg->flags & I2C_M_TEN)
-			tags[len++] = addr >> 8;
-	}
+			if (msg->flags & I2C_M_TEN)
+				tags[len++] = addr >> 8;
+		}
 
-	/* Send _STOP commands for the last block */
-	if (last) {
-		if (msg->flags & I2C_M_RD)
-			tags[len++] = QUP_TAG_V2_DATARD_STOP;
-		else
-			tags[len++] = QUP_TAG_V2_DATAWR_STOP;
-	} else {
-		if (msg->flags & I2C_M_RD)
+		/* Send _STOP commands for the last block */
+		if ((msg->flags & I2C_M_RD)
+			&& (msg->flags & I2C_M_RECV_LEN)) {
 			tags[len++] = QUP_TAG_V2_DATARD;
-		else
-			tags[len++] = QUP_TAG_V2_DATAWR;
-	}
+		} else if (last) {
+			if (msg->flags & I2C_M_RD)
+				tags[len++] = QUP_TAG_V2_DATARD_STOP;
+			else
+				tags[len++] = QUP_TAG_V2_DATAWR_STOP;
+		} else {
+			if (msg->flags & I2C_M_RD)
+				tags[len++] = QUP_TAG_V2_DATARD;
+			else
+				tags[len++] = QUP_TAG_V2_DATAWR;
+		}
 
-	data_len = qup_i2c_get_data_len(qup);
+		data_len = qup_i2c_get_data_len(qup);
 
-	/* 0 implies 256 bytes */
-	if (data_len == QUP_READ_LIMIT)
-		tags[len++] = 0;
-	else
-		tags[len++] = data_len;
+		/* 0 implies 256 bytes */
+		if (data_len == QUP_READ_LIMIT)
+			tags[len++] = 0;
+		else
+			tags[len++] = data_len;
 
-	if ((msg->flags & I2C_M_RD) && last && is_dma) {
-		tags[len++] = QUP_BAM_INPUT_EOT;
-		tags[len++] = QUP_BAM_FLUSH_STOP;
+		if ((msg->flags & I2C_M_RD) && last && is_dma) {
+			tags[len++] = QUP_BAM_INPUT_EOT;
+			tags[len++] = QUP_BAM_FLUSH_STOP;
+		}
 	}
 
 	return len;
@@ -1065,9 +1076,18 @@ static int qup_i2c_read_fifo_v2(struct qup_i2c_dev *qup,
 				struct i2c_msg *msg)
 {
 	u32 val;
-	int idx, pos = 0, ret = 0, total;
+	int idx, pos = 0, ret = 0, total, msg_offset = 0, msg_pos;
 
+	/*
+	 * If the message length is already read in
+	 * the first byte of the buffer, account for
+	 * that by setting the offset
+	 */
+	if ((msg->flags & I2C_M_RECV_LEN) &&
+	    (msg->len > 1))
+		msg_offset = 1;
 	total = qup_i2c_get_data_len(qup);
+	total -= msg_offset;
 
 	/* 2 extra bytes for read tags */
 	while (pos < (total + 2)) {
@@ -1087,8 +1107,9 @@ static int qup_i2c_read_fifo_v2(struct qup_i2c_dev *qup,
 
 			if (pos >= (total + 2))
 				goto out;
-
-			msg->buf[qup->pos++] = val & 0xff;
+			msg_pos = qup->pos + msg_offset;
+			msg->buf[msg_pos] = val & 0xff;
+			qup->pos++;
 		}
 	}
 
@@ -1128,6 +1149,22 @@ static int qup_i2c_read_one_v2(struct qup_i2c_dev *qup, struct i2c_msg *msg)
 			goto err;
 
 		qup->blk.pos++;
+
+		/* Handle SMBus block read length */
+		if ((msg->flags & I2C_M_RECV_LEN) && (msg->len == 1)) {
+			if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) {
+				ret = -EPROTO;
+				goto err;
+			}
+			msg->len += msg->buf[0];
+			qup->pos = 0;
+			qup_i2c_set_read_mode_v2(qup, msg->len);
+			qup_i2c_issue_xfer_v2(qup, msg);
+			ret = qup_i2c_wait_for_complete(qup, msg);
+			if (ret)
+				goto err;
+			qup_i2c_set_blk_data(qup, msg);
+		}
 	} while (qup->blk.pos < qup->blk.count);
 
 err:
-- 
1.8.2.1

RE: [PATCH 1/2] i2c: qup: add ACPI support

[Sricharan]

+additional lists

> Add support to get the device parameters from ACPI. Assume that the clocks
> are managed by firmware.
> 
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> ---
<snip>
> +	bool input_clk_ctrl = true;
> 
>  	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
>  	if (!qup)
> @@ -1372,7 +1378,14 @@ static int qup_i2c_probe(struct platform_device
> *pdev)
>  	init_completion(&qup->xfer);
>  	platform_set_drvdata(pdev, qup);
> 
> -	of_property_read_u32(node, "clock-frequency", &clk_freq);
> +	ret = device_property_read_u32(qup->dev, "clock-frequency",
> &clk_freq);
> +	if (ret) {
> +		dev_warn(qup->dev, "using default clock-frequency values");
> +		clk_freq = DEFAULT_CLK_FREQ;
> +	}
> +
> +	if (ACPI_HANDLE(qup->dev) && !node)
> +		input_clk_ctrl = false;
> 
>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-
> v1.1.1")) {
>  		qup->adap.algo = &qup_i2c_algo;
> @@ -1454,20 +1467,29 @@ nodma:
>  		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);
> -	}
> +	if (input_clk_ctrl) {
     The above ACPI_HANDLE check can be directly used here and avoid the
variable.

> +		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);
> +		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);
> +		}
> +		qup_i2c_enable_clocks(qup);
> +		src_clk_freq = clk_get_rate(qup->clk);
> +	} else {
> +		ret = device_property_read_u32(qup->dev,
> +				"src-clock-hz", &src_clk_freq);
> +		if (ret) {
> +			dev_warn(qup->dev, "using default src-clock-hz");
> +			src_clk_freq = DEFAULT_SRC_CLK;
> +		}
>  	}
> 
> -	qup_i2c_enable_clocks(qup);
> -
>  	/*
>  	 * Bootloaders might leave a pending interrupt on certain QUP's,
>  	 * so we reset the core before registering for interrupts.
> @@ -1514,7 +1536,6 @@ nodma:
>  	size = QUP_INPUT_FIFO_SIZE(io_mode);
>  	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
> 
> -	src_clk_freq = clk_get_rate(qup->clk);
>  	fs_div = ((src_clk_freq / clk_freq) / 2) - 3;
>  	hs_div = 3;
>  	qup->clk_ctl = (hs_div << 8) | (fs_div & 0xff); @@ -1534,6 +1555,8
> @@ nodma:
>  	qup->adap.dev.parent = qup->dev;
>  	qup->adap.dev.of_node = pdev->dev.of_node;
>  	qup->is_last = true;
> +	if (ACPI_HANDLE(qup->dev))
> +		ACPI_COMPANION_SET(&qup->adap.dev,
> ACPI_COMPANION(qup->dev));
    Can we move this inside the above check.
> 
>  	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup-
> >adap.name));
> 
> @@ -1639,6 +1662,13 @@ static const struct of_device_id
> qup_i2c_dt_match[] = {  };  MODULE_DEVICE_TABLE(of, qup_i2c_dt_match);
> 
> +#if IS_ENABLED(CONFIG_ACPI)
> +static const struct acpi_device_id qup_i2c_acpi_match[] = {
> +	{ "QCOM8010"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids); #endif
>  static struct platform_driver qup_i2c_driver = {
>  	.probe  = qup_i2c_probe,
>  	.remove = qup_i2c_remove,
> @@ -1646,6 +1676,7 @@ static struct platform_driver qup_i2c_driver = {
>  		.name = "i2c_qup",
>  		.pm = &qup_i2c_qup_pm_ops,
>  		.of_match_table = qup_i2c_dt_match,
> +		.acpi_match_table = ACPI_PTR(qup_i2c_acpi_match),
 Should this also be in #if IS_ENABLED(CONFIG_ACPI) check ?

Regards,
 Sricharan

RE: [PATCH 2/2] i2c: qup: support SMBus block read

[Sricharan]

+additional lists

> I2C QUP driver relies on SMBus emulation support from the framework.
> To handle SMBus block reads, the driver should check I2C_M_RECV_LEN flag
> and should read the first byte received as the message length.
> 
> The driver configures the QUP hardware to read one byte. Once the message
> length is known from this byte, the QUP hardware is configured to read the
> rest.
> 
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> ---
>  drivers/i2c/busses/i2c-qup.c | 95 ++++++++++++++++++++++++++++++-------
> -------
>  1 file changed, 66 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index
> ef31b26..2658b67 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -526,38 +526,49 @@ static int qup_i2c_set_tags(u8 *tags, struct
> qup_i2c_dev *qup,
> 
>  	int last = (qup->blk.pos == (qup->blk.count - 1)) && (qup->is_last);
> 
> -	if (qup->blk.pos == 0) {
> -		tags[len++] = QUP_TAG_V2_START;
> -		tags[len++] = addr & 0xff;
> +	/* Handle SMBus block data read */
> +	if ((msg->flags & I2C_M_RD) && (msg->flags & I2C_M_RECV_LEN)
> &&
> +	    (msg->len > 1)) {
> +		tags[len++] = QUP_TAG_V2_DATARD_STOP;
> +		data_len = qup_i2c_get_data_len(qup);
> +		tags[len++] = data_len - 1;
> +	} else {
> +		if (qup->blk.pos == 0) {
> +			tags[len++] = QUP_TAG_V2_START;
> +			tags[len++] = addr & 0xff;
> 
> -		if (msg->flags & I2C_M_TEN)
> -			tags[len++] = addr >> 8;
> -	}
> +			if (msg->flags & I2C_M_TEN)
> +				tags[len++] = addr >> 8;
> +		}
> 
> -	/* Send _STOP commands for the last block */
> -	if (last) {
> -		if (msg->flags & I2C_M_RD)
> -			tags[len++] = QUP_TAG_V2_DATARD_STOP;
> -		else
> -			tags[len++] = QUP_TAG_V2_DATAWR_STOP;
> -	} else {
> -		if (msg->flags & I2C_M_RD)
> +		/* Send _STOP commands for the last block */
> +		if ((msg->flags & I2C_M_RD)
> +			&& (msg->flags & I2C_M_RECV_LEN)) {
>  			tags[len++] = QUP_TAG_V2_DATARD;
> -		else
> -			tags[len++] = QUP_TAG_V2_DATAWR;
> -	}
> +		} else if (last) {
> +			if (msg->flags & I2C_M_RD)
> +				tags[len++] = QUP_TAG_V2_DATARD_STOP;
> +			else
> +				tags[len++] = QUP_TAG_V2_DATAWR_STOP;
> +		} else {
> +			if (msg->flags & I2C_M_RD)
> +				tags[len++] = QUP_TAG_V2_DATARD;
> +			else
> +				tags[len++] = QUP_TAG_V2_DATAWR;
> +		}
> 
> -	data_len = qup_i2c_get_data_len(qup);
> +		data_len = qup_i2c_get_data_len(qup);
> 
> -	/* 0 implies 256 bytes */
> -	if (data_len == QUP_READ_LIMIT)
> -		tags[len++] = 0;
> -	else
> -		tags[len++] = data_len;
> +		/* 0 implies 256 bytes */
> +		if (data_len == QUP_READ_LIMIT)
> +			tags[len++] = 0;
> +		else
> +			tags[len++] = data_len;
> 
> -	if ((msg->flags & I2C_M_RD) && last && is_dma) {
> -		tags[len++] = QUP_BAM_INPUT_EOT;
> -		tags[len++] = QUP_BAM_FLUSH_STOP;
> +		if ((msg->flags & I2C_M_RD) && last && is_dma) {
> +			tags[len++] = QUP_BAM_INPUT_EOT;
> +			tags[len++] = QUP_BAM_FLUSH_STOP;
> +		}
>  	}
> 
   Will it look simpler if we add a qup_i2c_set_tags_smb instead and add an
   Comment ? Here it is not clear how this is different from a normal read.

<snip..>

> +
> +		/* Handle SMBus block read length */
> +		if ((msg->flags & I2C_M_RECV_LEN) && (msg->len == 1)) {
> +			if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) {
> +				ret = -EPROTO;
> +				goto err;
> +			}
> +			msg->len += msg->buf[0];
> +			qup->pos = 0;
> +			qup_i2c_set_read_mode_v2(qup, msg->len);
> +			qup_i2c_issue_xfer_v2(qup, msg);
> +			ret = qup_i2c_wait_for_complete(qup, msg);
> +			if (ret)
> +				goto err;
> +			qup_i2c_set_blk_data(qup, msg);
> +		}
>  	} while (qup->blk.pos < qup->blk.count);
       
         When v2 mode is not supported this should return an error,
         in qup_i2c_xfer for msg->flags & I2C_M_RECV_LEN

Regards,
 Sricharan

Re: [PATCH 1/2] i2c: qup: add ACPI support

[Naveen Kaje]

On 5/10/2016 12:05 AM, Sricharan wrote:
> +additional lists
>
>> Add support to get the device parameters from ACPI. Assume that the clocks
>> are managed by firmware.
>>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>> ---
> <snip>
>> +	bool input_clk_ctrl = true;
>>
>>   	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
>>   	if (!qup)
>> @@ -1372,7 +1378,14 @@ static int qup_i2c_probe(struct platform_device
>> *pdev)
>>   	init_completion(&qup->xfer);
>>   	platform_set_drvdata(pdev, qup);
>>
>> -	of_property_read_u32(node, "clock-frequency", &clk_freq);
>> +	ret = device_property_read_u32(qup->dev, "clock-frequency",
>> &clk_freq);
>> +	if (ret) {
>> +		dev_warn(qup->dev, "using default clock-frequency values");
>> +		clk_freq = DEFAULT_CLK_FREQ;
>> +	}
>> +
>> +	if (ACPI_HANDLE(qup->dev) && !node)
>> +		input_clk_ctrl = false;
>>
>>   	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-
>> v1.1.1")) {
>>   		qup->adap.algo = &qup_i2c_algo;
>> @@ -1454,20 +1467,29 @@ nodma:
>>   		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);
>> -	}
>> +	if (input_clk_ctrl) {
>       The above ACPI_HANDLE check can be directly used here and avoid the
> variable.
Thanks for reviewing. Will do in V2.
>
>> +		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);
>> +		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);
>> +		}
>> +		qup_i2c_enable_clocks(qup);
>> +		src_clk_freq = clk_get_rate(qup->clk);
>> +	} else {
>> +		ret = device_property_read_u32(qup->dev,
>> +				"src-clock-hz", &src_clk_freq);
>> +		if (ret) {
>> +			dev_warn(qup->dev, "using default src-clock-hz");
>> +			src_clk_freq = DEFAULT_SRC_CLK;
>> +		}
>>   	}
>>
>> -	qup_i2c_enable_clocks(qup);
>> -
>>   	/*
>>   	 * Bootloaders might leave a pending interrupt on certain QUP's,
>>   	 * so we reset the core before registering for interrupts.
>> @@ -1514,7 +1536,6 @@ nodma:
>>   	size = QUP_INPUT_FIFO_SIZE(io_mode);
>>   	qup->in_fifo_sz = qup->in_blk_sz * (2 << size);
>>
>> -	src_clk_freq = clk_get_rate(qup->clk);
>>   	fs_div = ((src_clk_freq / clk_freq) / 2) - 3;
>>   	hs_div = 3;
>>   	qup->clk_ctl = (hs_div << 8) | (fs_div & 0xff); @@ -1534,6 +1555,8
>> @@ nodma:
>>   	qup->adap.dev.parent = qup->dev;
>>   	qup->adap.dev.of_node = pdev->dev.of_node;
>>   	qup->is_last = true;
>> +	if (ACPI_HANDLE(qup->dev))
>> +		ACPI_COMPANION_SET(&qup->adap.dev,
>> ACPI_COMPANION(qup->dev));
>      Can we move this inside the above check.
Will do in V2.
>>   	strlcpy(qup->adap.name, "QUP I2C adapter", sizeof(qup-
>>> adap.name));
>> @@ -1639,6 +1662,13 @@ static const struct of_device_id
>> qup_i2c_dt_match[] = {  };  MODULE_DEVICE_TABLE(of, qup_i2c_dt_match);
>>
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static const struct acpi_device_id qup_i2c_acpi_match[] = {
>> +	{ "QCOM8010"},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids); #endif
>>   static struct platform_driver qup_i2c_driver = {
>>   	.probe  = qup_i2c_probe,
>>   	.remove = qup_i2c_remove,
>> @@ -1646,6 +1676,7 @@ static struct platform_driver qup_i2c_driver = {
>>   		.name = "i2c_qup",
>>   		.pm = &qup_i2c_qup_pm_ops,
>>   		.of_match_table = qup_i2c_dt_match,
>> +		.acpi_match_table = ACPI_PTR(qup_i2c_acpi_match),
>   Should this also be in #if IS_ENABLED(CONFIG_ACPI) check ?
No, ACPI_PTR (defined in include/linux/acpi.h) takes care of setting the 
pointer value based on the availability of CONFIG_ACPI.
>
> Regards,
>   Sricharan
>

Re: [PATCH 2/2] i2c: qup: support SMBus block read

[Naveen Kaje]

On 5/10/2016 1:13 AM, Sricharan wrote:
> +additional lists
>
>> I2C QUP driver relies on SMBus emulation support from the framework.
>> To handle SMBus block reads, the driver should check I2C_M_RECV_LEN flag
>> and should read the first byte received as the message length.
>>
>> The driver configures the QUP hardware to read one byte. Once the message
>> length is known from this byte, the QUP hardware is configured to read the
>> rest.
>>
>> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
>> ---
>>   drivers/i2c/busses/i2c-qup.c | 95 ++++++++++++++++++++++++++++++-------
>> -------
>>   1 file changed, 66 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index
>> ef31b26..2658b67 100644
>> --- a/drivers/i2c/busses/i2c-qup.c
>> +++ b/drivers/i2c/busses/i2c-qup.c
>> @@ -526,38 +526,49 @@ static int qup_i2c_set_tags(u8 *tags, struct
>> qup_i2c_dev *qup,
>>
>>   	int last = (qup->blk.pos == (qup->blk.count - 1)) && (qup->is_last);
>>
>> -	if (qup->blk.pos == 0) {
>> -		tags[len++] = QUP_TAG_V2_START;
>> -		tags[len++] = addr & 0xff;
>> +	/* Handle SMBus block data read */
>> +	if ((msg->flags & I2C_M_RD) && (msg->flags & I2C_M_RECV_LEN)
>> &&
>> +	    (msg->len > 1)) {
>> +		tags[len++] = QUP_TAG_V2_DATARD_STOP;
>> +		data_len = qup_i2c_get_data_len(qup);
>> +		tags[len++] = data_len - 1;
>> +	} else {
>> +		if (qup->blk.pos == 0) {
>> +			tags[len++] = QUP_TAG_V2_START;
>> +			tags[len++] = addr & 0xff;
>>
>> -		if (msg->flags & I2C_M_TEN)
>> -			tags[len++] = addr >> 8;
>> -	}
>> +			if (msg->flags & I2C_M_TEN)
>> +				tags[len++] = addr >> 8;
>> +		}
>>
>> -	/* Send _STOP commands for the last block */
>> -	if (last) {
>> -		if (msg->flags & I2C_M_RD)
>> -			tags[len++] = QUP_TAG_V2_DATARD_STOP;
>> -		else
>> -			tags[len++] = QUP_TAG_V2_DATAWR_STOP;
>> -	} else {
>> -		if (msg->flags & I2C_M_RD)
>> +		/* Send _STOP commands for the last block */
>> +		if ((msg->flags & I2C_M_RD)
>> +			&& (msg->flags & I2C_M_RECV_LEN)) {
>>   			tags[len++] = QUP_TAG_V2_DATARD;
>> -		else
>> -			tags[len++] = QUP_TAG_V2_DATAWR;
>> -	}
>> +		} else if (last) {
>> +			if (msg->flags & I2C_M_RD)
>> +				tags[len++] = QUP_TAG_V2_DATARD_STOP;
>> +			else
>> +				tags[len++] = QUP_TAG_V2_DATAWR_STOP;
>> +		} else {
>> +			if (msg->flags & I2C_M_RD)
>> +				tags[len++] = QUP_TAG_V2_DATARD;
>> +			else
>> +				tags[len++] = QUP_TAG_V2_DATAWR;
>> +		}
>>
>> -	data_len = qup_i2c_get_data_len(qup);
>> +		data_len = qup_i2c_get_data_len(qup);
>>
>> -	/* 0 implies 256 bytes */
>> -	if (data_len == QUP_READ_LIMIT)
>> -		tags[len++] = 0;
>> -	else
>> -		tags[len++] = data_len;
>> +		/* 0 implies 256 bytes */
>> +		if (data_len == QUP_READ_LIMIT)
>> +			tags[len++] = 0;
>> +		else
>> +			tags[len++] = data_len;
>>
>> -	if ((msg->flags & I2C_M_RD) && last && is_dma) {
>> -		tags[len++] = QUP_BAM_INPUT_EOT;
>> -		tags[len++] = QUP_BAM_FLUSH_STOP;
>> +		if ((msg->flags & I2C_M_RD) && last && is_dma) {
>> +			tags[len++] = QUP_BAM_INPUT_EOT;
>> +			tags[len++] = QUP_BAM_FLUSH_STOP;
>> +		}
>>   	}
>>
>     Will it look simpler if we add a qup_i2c_set_tags_smb instead and add an
>     Comment ? Here it is not clear how this is different from a normal read.
Will rework this and send V2.
> <snip..>
>
>> +
>> +		/* Handle SMBus block read length */
>> +		if ((msg->flags & I2C_M_RECV_LEN) && (msg->len == 1)) {
>> +			if (msg->buf[0] > I2C_SMBUS_BLOCK_MAX) {
>> +				ret = -EPROTO;
>> +				goto err;
>> +			}
>> +			msg->len += msg->buf[0];
>> +			qup->pos = 0;
>> +			qup_i2c_set_read_mode_v2(qup, msg->len);
>> +			qup_i2c_issue_xfer_v2(qup, msg);
>> +			ret = qup_i2c_wait_for_complete(qup, msg);
>> +			if (ret)
>> +				goto err;
>> +			qup_i2c_set_blk_data(qup, msg);
>> +		}
>>   	} while (qup->blk.pos < qup->blk.count);
>         
>           When v2 mode is not supported this should return an error,
>           in qup_i2c_xfer for msg->flags & I2C_M_RECV_LEN
Will add a check in qup_i2c_xfer in the next version of the patch.
>
> Regards,
>   Sricharan
>