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

[PATCH v2 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 | 59 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 23eaabb..559947c 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,8 +1365,8 @@ 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;
 
 	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
@@ -1372,7 +1377,12 @@ 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 %d",
+			DEFAULT_CLK_FREQ);
+		clk_freq = DEFAULT_CLK_FREQ;
+	}
 
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-v1.1.1")) {
 		qup->adap.algo = &qup_i2c_algo;
@@ -1454,20 +1464,31 @@ 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 (ACPI_HANDLE(qup->dev)) {
+		ret = device_property_read_u32(qup->dev,
+				"src-clock-hz", &src_clk_freq);
+		if (ret) {
+			dev_warn(qup->dev, "using default src-clock-hz %d",
+				DEFAULT_SRC_CLK);
+			src_clk_freq = DEFAULT_SRC_CLK;
+		}
+		ACPI_COMPANION_SET(&qup->adap.dev, ACPI_COMPANION(qup->dev));
+	} else {
+		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);
 	}
 
-	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 +1535,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);
@@ -1639,6 +1659,14 @@ 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 +1674,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),
 	},
 };
 
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

[PATCH v2 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 | 72 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 559947c..2ca18d8 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -517,6 +517,39 @@ static int qup_i2c_get_data_len(struct qup_i2c_dev *qup)
 	return data_len;
 }
 
+static bool qup_i2c_check_msg_len(struct i2c_msg *msg)
+{
+	return ((msg->flags & I2C_M_RD) && (msg->flags & I2C_M_RECV_LEN));
+}
+
+static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev *qup,
+			struct i2c_msg *msg)
+{
+	int len = 0;
+	int data_len = qup_i2c_get_data_len(qup);
+
+	if (msg->len > 1) {
+		tags[len++] = QUP_TAG_V2_DATARD_STOP;
+		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;
+		}
+
+		tags[len++] = QUP_TAG_V2_DATARD;
+		/* 0 implies 256 bytes */
+		if (data_len == QUP_READ_LIMIT)
+			tags[len++] = 0;
+		else
+			tags[len++] = data_len;
+	}
+	return len;
+}
+
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
 			    struct i2c_msg *msg,  int is_dma)
 {
@@ -526,6 +559,10 @@ 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);
 
+	/* Handle tags for SMBus block read */
+	if (qup_i2c_check_msg_len(msg))
+		return qup_i2c_set_tags_smb(addr, tags, qup, msg);
+
 	if (qup->blk.pos == 0) {
 		tags[len++] = QUP_TAG_V2_START;
 		tags[len++] = addr & 0xff;
@@ -1065,9 +1102,17 @@ 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;
 
+	/*
+	 * If the message length is already read in
+	 * the first byte of the buffer, account for
+	 * that by setting the offset
+	 */
+	if (qup_i2c_check_msg_len(msg) && (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 +1132,8 @@ 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->buf[qup->pos+msg_offset] = val & 0xff;
+			qup->pos++;
 		}
 	}
 
@@ -1128,6 +1173,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 (qup_i2c_check_msg_len(msg) && (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:
@@ -1210,6 +1271,11 @@ static int qup_i2c_xfer(struct i2c_adapter *adap,
 			goto out;
 		}
 
+		if (qup_i2c_check_msg_len(&msgs[idx])) {
+			ret = -EOPNOTSUPP;
+			goto out;
+		}
+
 		if (msgs[idx].flags & I2C_M_RD)
 			ret = qup_i2c_read_one(qup, &msgs[idx]);
 		else
-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

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

[Sricharan]

Hi,

> 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 | 59 +++++++++++++++++++++++++++++++++---
> --------
>  1 file changed, 44 insertions(+), 15 deletions(-)
> 
<.snip>

> +/* 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,8 +1365,8 @@ 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;
> 
>  	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL); @@ -
> 1372,7 +1377,12 @@ 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 %d",
> +			DEFAULT_CLK_FREQ);
> +		clk_freq = DEFAULT_CLK_FREQ;
> +	}
> 
>  	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-
> v1.1.1")) {
>  		qup->adap.algo = &qup_i2c_algo;
> @@ -1454,20 +1464,31 @@ 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 (ACPI_HANDLE(qup->dev)) {
> +		ret = device_property_read_u32(qup->dev,
> +				"src-clock-hz", &src_clk_freq);
> +		if (ret) {
> +			dev_warn(qup->dev, "using default src-clock-hz %d",
> +				DEFAULT_SRC_CLK);
> +			src_clk_freq = DEFAULT_SRC_CLK;
> +		}
> +		ACPI_COMPANION_SET(&qup->adap.dev,
> ACPI_COMPANION(qup->dev));
> +	} else {
> +		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);
>  	}
> 
> -	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 +1535,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); @@ -1639,6 +1659,14
> @@ 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 +1674,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),
>  	},
>  };
> 
 Reviewed-by: sricharan@codeaurora.org

Regards,
 Sricharan

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

[Sricharan]

Hi,

<snip..>

> +static bool qup_i2c_check_msg_len(struct i2c_msg *msg) {
> +	return ((msg->flags & I2C_M_RD) && (msg->flags &
> I2C_M_RECV_LEN)); }
> +
> +static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev
> *qup,
> +			struct i2c_msg *msg)
> +{
> +	int len = 0;
> +	int data_len = qup_i2c_get_data_len(qup);
> +
> +	if (msg->len > 1) {
> +		tags[len++] = QUP_TAG_V2_DATARD_STOP;
> +		tags[len++] = data_len - 1;
> +	} else {
    So we hit this else part for msg->len = 1 and this case blk.pos = 0
    will always be true right ?

> +		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;
> +		}
> +
> +		tags[len++] = QUP_TAG_V2_DATARD;
> +		/* 0 implies 256 bytes */
> +		if (data_len == QUP_READ_LIMIT)
> +			tags[len++] = 0;
> +		else
> +			tags[len++] = data_len;
> +	}
       Even data_len will always be '1' right ?
> +	return len;
> +}
> +
>  static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
>  			    struct i2c_msg *msg,  int is_dma)  { @@ -526,6
> +559,10 @@ 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);
> 
> +	/* Handle tags for SMBus block read */
> +	if (qup_i2c_check_msg_len(msg))
> +		return qup_i2c_set_tags_smb(addr, tags, qup, msg);
> +
>  	if (qup->blk.pos == 0) {
>  		tags[len++] = QUP_TAG_V2_START;
>  		tags[len++] = addr & 0xff;
> @@ -1065,9 +1102,17 @@ 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;
> 
> +	/*
> +	 * If the message length is already read in
> +	 * the first byte of the buffer, account for
> +	 * that by setting the offset
> +	 */
> +	if (qup_i2c_check_msg_len(msg) && (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 +1132,8 @@ 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->buf[qup->pos+msg_offset] = val & 0xff;
	A space around '+' ?

> +			qup->pos++;
>  		}
>  	}
> 
> @@ -1128,6 +1173,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 (qup_i2c_check_msg_len(msg) && (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;
		Is the issue_xfer_v2 needed inside this here ? 

> +			qup_i2c_set_blk_data(qup, msg);
> +		}
>  	} while (qup->blk.pos < qup->blk.count);
   Regards,
     Sricharan

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

[Naveen Kaje]

Hi Sricharan,

On 5/18/2016 12:04 AM, Sricharan wrote:
> Hi,
>
>> 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 | 59 +++++++++++++++++++++++++++++++++---
>> --------
>>   1 file changed, 44 insertions(+), 15 deletions(-)
>>
> <.snip>
>
>> +/* 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,8 +1365,8 @@ 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;
>>
>>   	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL); @@ -
>> 1372,7 +1377,12 @@ 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 %d",
>> +			DEFAULT_CLK_FREQ);
>> +		clk_freq = DEFAULT_CLK_FREQ;
>> +	}
>>
>>   	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-
>> v1.1.1")) {
>>   		qup->adap.algo = &qup_i2c_algo;
>> @@ -1454,20 +1464,31 @@ 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 (ACPI_HANDLE(qup->dev)) {
>> +		ret = device_property_read_u32(qup->dev,
>> +				"src-clock-hz", &src_clk_freq);
>> +		if (ret) {
>> +			dev_warn(qup->dev, "using default src-clock-hz %d",
>> +				DEFAULT_SRC_CLK);
>> +			src_clk_freq = DEFAULT_SRC_CLK;
>> +		}
>> +		ACPI_COMPANION_SET(&qup->adap.dev,
>> ACPI_COMPANION(qup->dev));
>> +	} else {
>> +		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);
>>   	}
>>
>> -	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 +1535,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); @@ -1639,6 +1659,14
>> @@ 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 +1674,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),
>>   	},
>>   };
>>
>   Reviewed-by: sricharan@codeaurora.org

Thanks for reviewing. I spotted an unused variable, will send a V3 fixing the same.

> Regards,
>   Sricharan
>

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

[Naveen Kaje]

Hi Sricharan,


On 5/18/2016 1:06 AM, Sricharan wrote:
> Hi,
>
> <snip..>
>
>> +static bool qup_i2c_check_msg_len(struct i2c_msg *msg) {
>> +	return ((msg->flags & I2C_M_RD) && (msg->flags &
>> I2C_M_RECV_LEN)); }
>> +
>> +static int qup_i2c_set_tags_smb(u16 addr, u8 *tags, struct qup_i2c_dev
>> *qup,
>> +			struct i2c_msg *msg)
>> +{
>> +	int len = 0;
>> +	int data_len = qup_i2c_get_data_len(qup);
>> +
>> +	if (msg->len > 1) {
>> +		tags[len++] = QUP_TAG_V2_DATARD_STOP;
>> +		tags[len++] = data_len - 1;
>> +	} else {
>      So we hit this else part for msg->len = 1 and this case blk.pos = 0
>      will always be true right ?
Yes, will fix that in V3.
>
>> +		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;
>> +		}
>> +
>> +		tags[len++] = QUP_TAG_V2_DATARD;
>> +		/* 0 implies 256 bytes */
>> +		if (data_len == QUP_READ_LIMIT)
>> +			tags[len++] = 0;
>> +		else
>> +			tags[len++] = data_len;
>> +	}
>         Even data_len will always be '1' right ?
Yes, but here preferably we use a variable than a number without a context.
>> +	return len;
>> +}
>> +
>>   static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
>>   			    struct i2c_msg *msg,  int is_dma)  { @@ -526,6
>> +559,10 @@ 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);
>>
>> +	/* Handle tags for SMBus block read */
>> +	if (qup_i2c_check_msg_len(msg))
>> +		return qup_i2c_set_tags_smb(addr, tags, qup, msg);
>> +
>>   	if (qup->blk.pos == 0) {
>>   		tags[len++] = QUP_TAG_V2_START;
>>   		tags[len++] = addr & 0xff;
>> @@ -1065,9 +1102,17 @@ 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;
>>
>> +	/*
>> +	 * If the message length is already read in
>> +	 * the first byte of the buffer, account for
>> +	 * that by setting the offset
>> +	 */
>> +	if (qup_i2c_check_msg_len(msg) && (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 +1132,8 @@ 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->buf[qup->pos+msg_offset] = val & 0xff;
> 	A space around '+' ?
Thanks, will fix that.
>
>> +			qup->pos++;
>>   		}
>>   	}
>>
>> @@ -1128,6 +1173,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 (qup_i2c_check_msg_len(msg) && (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;
> 		Is the issue_xfer_v2 needed inside this here ?
No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data 
that is indicated by the length we read earlier.
>
>> +			qup_i2c_set_blk_data(qup, msg);
>> +		}
>>   	} while (qup->blk.pos < qup->blk.count);
>     Regards,
>       Sricharan

Thanks,
Naveen
>

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

[Timur Tabi]

Naveen Kaje wrote:
>>>
>>> +        tags[len++] = QUP_TAG_V2_DATARD;
>>> +        /* 0 implies 256 bytes */
>>> +        if (data_len == QUP_READ_LIMIT)
>>> +            tags[len++] = 0;
>>> +        else
>>> +            tags[len++] = data_len;
>>> +    }
>>         Even data_len will always be '1' right ?
> Yes, but here preferably we use a variable than a number without a context.

Actually, I would say the opposite.  I would rather see a constant with 
a comment explaining it, than a variable that we know will always 
contain only one number.

-- 
Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum, a Linux Foundation collaborative project.

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

[Naveen Kaje]

Hi Timur, Sricharan,

On 5/19/2016 2:21 PM, Timur Tabi wrote:
> Naveen Kaje wrote:
>>>>
>>>> +        tags[len++] = QUP_TAG_V2_DATARD;
>>>> +        /* 0 implies 256 bytes */
>>>> +        if (data_len == QUP_READ_LIMIT)
>>>> +            tags[len++] = 0;
>>>> +        else
>>>> +            tags[len++] = data_len;
>>>> +    }
>>>         Even data_len will always be '1' right ?
>> Yes, but here preferably we use a variable than a number without a 
>> context.
>
> Actually, I would say the opposite.  I would rather see a constant 
> with a comment explaining it, than a variable that we know will always 
> contain only one number.
>
Ok, got it. Will be fixed in V3.
Thanks,
Naveen

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

[Sricharan]

Hi,

<snip..>
>>> @@ -1128,6 +1173,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 (qup_i2c_check_msg_len(msg) && (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;
>> 		Is the issue_xfer_v2 needed inside this here ?
>No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data
>that is indicated by the length we read earlier.

So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop above this
 check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data inside this
 check, will not be enough ?

Regards,
 Sricharan

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

[Christ, Austin]

On 5/20/2016 2:31 AM, Sricharan wrote:
> Hi,
>
> <snip..>
>>>> @@ -1128,6 +1173,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 (qup_i2c_check_msg_len(msg) && (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;
>>> 		Is the issue_xfer_v2 needed inside this here ?
>> No, qup_i2c_issue_xfer_v2 is needed so that we read rest of the data
>> that is indicated by the length we read earlier.
> So qup_i2c_issue_xfer_v2 writes the tags and there is one already in the loop above this
>   check. So if you just do qup_i2c_set_read_mode_v2 and qup_i2c_set_blk_data inside this
>   check, will not be enough ?
>
> Regards,
>   Sricharan
>
In testing, removing the call to qup_i2c_issue_xfer_v2() within the 
conditional statement for SMBus length causes failures. That transfer 
request is needed to read that block of data.

Thanks,
Austin