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

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

[Austin Christ]

From: Naveen Kaje <nkaje@codeaurora.org>

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>
Signed-off-by: Austin Christ <austinwc@codeaurora.org>
Reviewed-by: sricharan@codeaurora.org
---
 drivers/i2c/busses/i2c-qup.c | 59 ++++++++++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 16 deletions(-)

Changes:
- v4:
 - remove warning for fall back to default clock frequency
- v3:
 - clean up unused variable
- v2:
 - clean up redundant checks and variables

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index dddd4da..0f23d58 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;
@@ -1354,14 +1359,13 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
 static int qup_i2c_probe(struct platform_device *pdev)
 {
 	static const int blk_sizes[] = {4, 16, 32};
-	struct device_node *node = pdev->dev.of_node;
 	struct qup_i2c_dev *qup;
 	unsigned long one_bit_t;
 	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 = DEFAULT_CLK_FREQ;
 	int blocks;
 
 	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
@@ -1372,7 +1376,11 @@ 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_notice(qup->dev, "using default clock-frequency %d",
+			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 +1462,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 +1533,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 +1657,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 +1672,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 v4 2/2] i2c: qup: support SMBus block read

[Austin Christ]

From: Naveen Kaje <nkaje@codeaurora.org>

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>
Signed-off-by: Austin Christ <austinwc@codeaurora.org>
Reviewed-by: sricharan@codeaurora.org
---
 drivers/i2c/busses/i2c-qup.c | 68 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 65 insertions(+), 3 deletions(-)

Changes:
-v4:
 - correct error code
- v3:
 - clean up redundant checks
 - use constant instead of variable for smbus length field
- v2:
 - rework the smbus block read and break into separate function

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 0f23d58..003a695 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -517,6 +517,33 @@ 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;
+
+	if (msg->len > 1) {
+		tags[len++] = QUP_TAG_V2_DATARD_STOP;
+		tags[len++] = qup_i2c_get_data_len(qup) - 1;
+	} else {
+		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;
+		/* Read 1 byte indicating the length of the SMBus message */
+		tags[len++] = 1;
+	}
+	return len;
+}
+
 static int qup_i2c_set_tags(u8 *tags, struct qup_i2c_dev *qup,
 			    struct i2c_msg *msg,  int is_dma)
 {
@@ -526,6 +553,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 +1096,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 +1126,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 +1167,24 @@ 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);
+			ret = qup_i2c_issue_xfer_v2(qup, msg);
+			if (ret)
+				goto err;
+			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 +1267,11 @@ static int qup_i2c_xfer(struct i2c_adapter *adap,
 			goto out;
 		}
 
+		if (qup_i2c_check_msg_len(&msgs[idx])) {
+			ret = -EINVAL;
+			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 v4 1/2] i2c: qup: add ACPI support

[Arnd Bergmann]

On Wednesday, June 8, 2016 12:19:44 PM CEST Austin Christ wrote:
> +               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;
> +               }
> 

Where is this property documented?

	Arnd

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

[kbuild test robot]

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

Hi,

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v4.7-rc2 next-20160609]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Austin-Christ/i2c-qup-add-ACPI-support/20160609-022325
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux i2c/for-next
config: arm64-allmodconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   In file included from drivers/i2c/busses/i2c-qup.c:27:0:
>> drivers/i2c/busses/i2c-qup.c:1665:27: error: 'qup_i2c_acpi_ids' undeclared here (not in a function)
    MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids);
                              ^
   include/linux/module.h:223:21: note: in definition of macro 'MODULE_DEVICE_TABLE'
    extern const typeof(name) __mod_##type##__##name##_device_table  \
                        ^
   include/linux/module.h:223:27: error: '__mod_acpi__qup_i2c_acpi_ids_device_table' aliased to undefined symbol 'qup_i2c_acpi_ids'
    extern const typeof(name) __mod_##type##__##name##_device_table  \
                              ^
   drivers/i2c/busses/i2c-qup.c:1665:1: note: in expansion of macro 'MODULE_DEVICE_TABLE'
    MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids);
    ^

vim +/qup_i2c_acpi_ids +1665 drivers/i2c/busses/i2c-qup.c

  1659	
  1660	#if IS_ENABLED(CONFIG_ACPI)
  1661	static const struct acpi_device_id qup_i2c_acpi_match[] = {
  1662		{ "QCOM8010"},
  1663		{ },
  1664	};
> 1665	MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_ids);
  1666	#endif
  1667	
  1668	static struct platform_driver qup_i2c_driver = {

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 50194 bytes --]

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

[Wolfram Sang]

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

On Wed, Jun 08, 2016 at 12:19:44PM -0600, Austin Christ wrote:
> From: Naveen Kaje <nkaje@codeaurora.org>
> 
> Add support to get the device parameters from ACPI. Assume
> that the clocks are managed by firmware.

Adding Mika to CC: Can you have a look at the ACPI binding? Looks ok to
me...

> 
> Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> Signed-off-by: Austin Christ <austinwc@codeaurora.org>
> Reviewed-by: sricharan@codeaurora.org

Please add a name before the email address

> ---
>  drivers/i2c/busses/i2c-qup.c | 59 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 16 deletions(-)
> 
> Changes:
> - v4:
>  - remove warning for fall back to default clock frequency
> - v3:
>  - clean up unused variable
> - v2:
>  - clean up redundant checks and variables
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index dddd4da..0f23d58 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>

Keep includes sorted.

>  
>  /* 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;
> @@ -1354,14 +1359,13 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>  static int qup_i2c_probe(struct platform_device *pdev)
>  {
>  	static const int blk_sizes[] = {4, 16, 32};
> -	struct device_node *node = pdev->dev.of_node;
>  	struct qup_i2c_dev *qup;
>  	unsigned long one_bit_t;
>  	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 = DEFAULT_CLK_FREQ;
>  	int blocks;
>  
>  	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
> @@ -1372,7 +1376,11 @@ 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_notice(qup->dev, "using default clock-frequency %d",
> +			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 +1462,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 +1533,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 +1657,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 +1672,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.
> 

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

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

[Wolfram Sang]

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

On Wed, Jun 08, 2016 at 12:19:45PM -0600, Austin Christ wrote:
> From: Naveen Kaje <nkaje@codeaurora.org>
> 
> 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>
> Signed-off-by: Austin Christ <austinwc@codeaurora.org>
> Reviewed-by: sricharan@codeaurora.org

Missing name here, too. Looks good otherwise.


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

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

[Mika Westerberg]

On Sat, Jun 18, 2016 at 04:10:34PM +0200, Wolfram Sang wrote:
> On Wed, Jun 08, 2016 at 12:19:44PM -0600, Austin Christ wrote:
> > From: Naveen Kaje <nkaje@codeaurora.org>
> > 
> > Add support to get the device parameters from ACPI. Assume
> > that the clocks are managed by firmware.
> 
> Adding Mika to CC: Can you have a look at the ACPI binding? Looks ok to
> me...
> 
> > 
> > Signed-off-by: Naveen Kaje <nkaje@codeaurora.org>
> > Signed-off-by: Austin Christ <austinwc@codeaurora.org>
> > Reviewed-by: sricharan@codeaurora.org
> 
> Please add a name before the email address
> 
> > ---
> >  drivers/i2c/busses/i2c-qup.c | 59 ++++++++++++++++++++++++++++++++------------
> >  1 file changed, 43 insertions(+), 16 deletions(-)
> > 
> > Changes:
> > - v4:
> >  - remove warning for fall back to default clock frequency
> > - v3:
> >  - clean up unused variable
> > - v2:
> >  - clean up redundant checks and variables
> > 
> > diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> > index dddd4da..0f23d58 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>
> 
> Keep includes sorted.
> 
> >  
> >  /* 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;
> > @@ -1354,14 +1359,13 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
> >  static int qup_i2c_probe(struct platform_device *pdev)
> >  {
> >  	static const int blk_sizes[] = {4, 16, 32};
> > -	struct device_node *node = pdev->dev.of_node;
> >  	struct qup_i2c_dev *qup;
> >  	unsigned long one_bit_t;
> >  	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 = DEFAULT_CLK_FREQ;
> >  	int blocks;
> >  
> >  	qup = devm_kzalloc(&pdev->dev, sizeof(*qup), GFP_KERNEL);
> > @@ -1372,7 +1376,11 @@ 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_notice(qup->dev, "using default clock-frequency %d",
> > +			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 +1462,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)) {

Use has_acpi_companion() if you need to.

> > +		ret = device_property_read_u32(qup->dev,
> > +				"src-clock-hz", &src_clk_freq);

Alternatively you can make qup_i2c_acpi_probe() which creates clock for
you based on the ACPI ID of the device. Then you do not need to have
these custom properties just because ACPI does not have native support
for clocks.

Ideally if one uses device_property_* API it should not need to
distinguish between DT/ACPI/whatnot.

> > +		if (ret) {
> > +			dev_warn(qup->dev, "using default src-clock-hz %d",
> > +				DEFAULT_SRC_CLK);

Why this is warning?

> > +			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 +1533,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 +1657,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 +1672,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.
> > 

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

[Timur Tabi]

Mika Westerberg wrote:
> Use has_acpi_companion() if you need to.

Is has_acpi_companion() the preferred alternative to ACPI_HANDLE()?  We 
frequently need to write code that does something different on ACPI vs 
DT, and there doesn't appear to be much consistency on how that's handled.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

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

[Mika Westerberg]

On Mon, Jun 20, 2016 at 10:00:46AM -0500, Timur Tabi wrote:
> Mika Westerberg wrote:
> > Use has_acpi_companion() if you need to.
> 
> Is has_acpi_companion() the preferred alternative to ACPI_HANDLE()?  We
> frequently need to write code that does something different on ACPI vs DT,
> and there doesn't appear to be much consistency on how that's handled.

Yes, that's the preferred one.

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

[Christopher Covington]

Hi Arnd,

On 06/08/2016 05:02 PM, Arnd Bergmann wrote:
> On Wednesday, June 8, 2016 12:19:44 PM CEST Austin Christ wrote:
>> +               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;
>> +               }
>>
> 
> Where is this property documented?

We plan on submitting documentation via dsd@acpica.org to
https://github.com/ahs3/dsd once it is operational. As I understand it
the project is brand new. It may take several months to begin accepting
submissions. In the mean time, we could potentially include
documentation in a reply to this thread, the cover of the next series, a
wiki page on codeaurora.org, a file in Documentation (perhaps to be
replaced by ACPICA style imports of the OS-neutral DSD project or a git
submodule) or potentially other means. Please let us know what you think
is sufficient.

Thanks,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Arnd Bergmann]

On Thursday, June 30, 2016 7:35:14 AM CEST Christopher Covington wrote:
> Hi Arnd,
> 
> On 06/08/2016 05:02 PM, Arnd Bergmann wrote:
> > On Wednesday, June 8, 2016 12:19:44 PM CEST Austin Christ wrote:
> >> +               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;
> >> +               }
> >>
> > 
> > Where is this property documented?
> 
> We plan on submitting documentation via dsd@acpica.org to
> https://github.com/ahs3/dsd once it is operational. As I understand it
> the project is brand new. It may take several months to begin accepting
> submissions. In the mean time, we could potentially include
> documentation in a reply to this thread, the cover of the next series, a
> wiki page on codeaurora.org, a file in Documentation (perhaps to be
> replaced by ACPICA style imports of the OS-neutral DSD project or a git
> submodule) or potentially other means. Please let us know what you think
> is sufficient.

As you are reusing part of the DT binding, it seems appropriate to put
the documentation for this into the binding documentation in the kernel.

Not sure what the devicetree maintainers think about that though.

	Arnd

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

[Christopher Covington]

Hi Arnd,

On 06/30/2016 07:52 AM, Arnd Bergmann wrote:
> On Thursday, June 30, 2016 7:35:14 AM CEST Christopher Covington wrote:
>> Hi Arnd,
>>
>> On 06/08/2016 05:02 PM, Arnd Bergmann wrote:
>>> On Wednesday, June 8, 2016 12:19:44 PM CEST Austin Christ wrote:
>>>> +               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;
>>>> +               }
>>>>
>>>
>>> Where is this property documented?
>>
>> We plan on submitting documentation via dsd@acpica.org to
>> https://github.com/ahs3/dsd once it is operational. As I understand it
>> the project is brand new. It may take several months to begin accepting
>> submissions. In the mean time, we could potentially include
>> documentation in a reply to this thread, the cover of the next series, a
>> wiki page on codeaurora.org, a file in Documentation (perhaps to be
>> replaced by ACPICA style imports of the OS-neutral DSD project or a git
>> submodule) or potentially other means. Please let us know what you think
>> is sufficient.
> 
> As you are reusing part of the DT binding, it seems appropriate to put
> the documentation for this into the binding documentation in the kernel.

My understanding is that in the device tree case, the input/source clock
frequency is assumed to be run-time managed through Global Clock
Controller (GCC) code and the common clock framework.

drivers/i2c/busses/i2c-qup.c:1549

src_clk_freq = clk_get_rate(qup->clk);

So a given I2C device tree entry points to the GCC device tree entry,
but there isn't an explicit, fixed input/source clock frequency value.

arch/arm64/boot/dts/qcom/msm8916.dtsi:310

blsp_i2c2: i2c@78b6000 {
        compatible = "qcom,i2c-qup-v2.2.1";
        reg = <0x78b6000 0x1000>;
        interrupts = <GIC_SPI 96 0>;
        clocks = <&gcc GCC_BLSP1_AHB_CLK>,
                <&gcc GCC_BLSP1_QUP2_I2C_APPS_CLK>;
        clock-names = "iface", "core";
        pinctrl-names = "default", "sleep";
        pinctrl-0 = <&i2c2_default>;
        pinctrl-1 = <&i2c2_sleep>;
        #address-cells = <1>;
        #size-cells = <0>;
        status = "disabled";
};

On the other hand, when ACPI is in use, the driver assumes a fixed
input/source clock frequency value, which it tries to look up as a
device property.

(I'm out of my depth here, so somebody please correct me if I've
described this wrong.)

Thanks,
Cov

-- 
Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Arnd Bergmann]

On Thursday, June 30, 2016 2:50:44 PM CEST Christopher Covington wrote:
> Hi Arnd,
> 
> On 06/30/2016 07:52 AM, Arnd Bergmann wrote:
> > On Thursday, June 30, 2016 7:35:14 AM CEST Christopher Covington wrote:
> >> Hi Arnd,
> >>
> >> On 06/08/2016 05:02 PM, Arnd Bergmann wrote:
> >>> On Wednesday, June 8, 2016 12:19:44 PM CEST Austin Christ wrote:
> >>>> +               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;
> >>>> +               }
> >>>>
> >>>
> >>> Where is this property documented?
> >>
> >> We plan on submitting documentation via dsd@acpica.org to
> >> https://github.com/ahs3/dsd once it is operational. As I understand it
> >> the project is brand new. It may take several months to begin accepting
> >> submissions. In the mean time, we could potentially include
> >> documentation in a reply to this thread, the cover of the next series, a
> >> wiki page on codeaurora.org, a file in Documentation (perhaps to be
> >> replaced by ACPICA style imports of the OS-neutral DSD project or a git
> >> submodule) or potentially other means. Please let us know what you think
> >> is sufficient.
> > 
> > As you are reusing part of the DT binding, it seems appropriate to put
> > the documentation for this into the binding documentation in the kernel.
> 
> My understanding is that in the device tree case, the input/source clock
> frequency is assumed to be run-time managed through Global Clock
> Controller (GCC) code and the common clock framework.
> 
> drivers/i2c/busses/i2c-qup.c:1549
> 
> src_clk_freq = clk_get_rate(qup->clk);
> 
> So a given I2C device tree entry points to the GCC device tree entry,
> but there isn't an explicit, fixed input/source clock frequency value.

Correct.

> arch/arm64/boot/dts/qcom/msm8916.dtsi:310
> 
> blsp_i2c2: i2c@78b6000 {
>         compatible = "qcom,i2c-qup-v2.2.1";
>         reg = <0x78b6000 0x1000>;
>         interrupts = <GIC_SPI 96 0>;
>         clocks = <&gcc GCC_BLSP1_AHB_CLK>,
>                 <&gcc GCC_BLSP1_QUP2_I2C_APPS_CLK>;
>         clock-names = "iface", "core";
>         pinctrl-names = "default", "sleep";
>         pinctrl-0 = <&i2c2_default>;
>         pinctrl-1 = <&i2c2_sleep>;
>         #address-cells = <1>;
>         #size-cells = <0>;
>         status = "disabled";
> };
> 
> On the other hand, when ACPI is in use, the driver assumes a fixed
> input/source clock frequency value, which it tries to look up as a
> device property.

Also correct.
 
> (I'm out of my depth here, so somebody please correct me if I've
> described this wrong.)

What I'm saying was that the binding file could just document both
cases as alternatives. We prefer to specify the clocks directly
when using a dtb, but for the driver one of the two is ok, and
the ACPI documentation will already have to refer to the DT binding
for the other properties, so I think it makes sense to describe
both the "clocks" and "src-clock-hz" properties in the same document
as optional properties and clarify that at least one of the two
is requried.

Note that we have traditionally used "clock-frequency" as the
property name for the input clock frequency in DT bindings that
predate the generic clock handling (e.g. 8250 uarts on IBM Power
servers), so we could use the same here.

	Arnd