[v2,1/4] i2c: qup: add probe path for Centriq ACPI devices

Message ID 1522871100-3621-2-git-send-email-austinwc@codeaurora.org
State New
Headers show
Series
  • Add Fast Mode Plus and other fixes
Related show

Commit Message

Christ, Austin April 4, 2018, 7:44 p.m.
Add support for Qualcomm Centriq devices that are qup-v2 compatible but
do not support DMA, so nodma needs to be set.

Signed-off-by: Austin Christ <austinwc@codeaurora.org>
---
v1:
	- Initial
v2:
	- Remove whitespace error
---
 drivers/i2c/busses/i2c-qup.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

Comments

Sricharan R April 12, 2018, 4:28 a.m. | #1
Hi Austin,

On 4/5/2018 1:14 AM, Austin Christ wrote:
> Add support for Qualcomm Centriq devices that are qup-v2 compatible but
> do not support DMA, so nodma needs to be set.
> 
> Signed-off-by: Austin Christ <austinwc@codeaurora.org>
> ---
> v1:
> 	- Initial
> v2:
> 	- Remove whitespace error
> ---
>  drivers/i2c/busses/i2c-qup.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
> index 904dfec..80af973 100644
> --- a/drivers/i2c/busses/i2c-qup.c
> +++ b/drivers/i2c/busses/i2c-qup.c
> @@ -1648,6 +1648,14 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>  	clk_disable_unprepare(qup->pclk);
>  }
>  
> +#if IS_ENABLED(CONFIG_ACPI)
> +static const struct acpi_device_id qup_i2c_acpi_match[] = {
> +	{ "QCOM8010"},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_match);
> +#endif
> +
>  static int qup_i2c_probe(struct platform_device *pdev)
>  {
>  	static const int blk_sizes[] = {4, 16, 32};
> @@ -1675,6 +1683,12 @@ static int qup_i2c_probe(struct platform_device *pdev)
>  			DEFAULT_CLK_FREQ);
>  	}
>  
> +	if (acpi_match_device(qup_i2c_acpi_match, qup->dev)) {
> +		qup->adap.algo = &qup_i2c_algo_v2;
> +		is_qup_v1 = false;
> +		goto nodma;
> +	}
> +

 What happens without the above check. Does it not get resolved below,
 when qup_i2c_req_dma fails ?

Regards,
 Sricharan
Christ, Austin April 12, 2018, 3:27 p.m. | #2
Hey Sricharan,

On 4/11/2018 10:28 PM, Sricharan R wrote:
> Hi Austin,
> 
> On 4/5/2018 1:14 AM, Austin Christ wrote:
>> Add support for Qualcomm Centriq devices that are qup-v2 compatible but
>> do not support DMA, so nodma needs to be set.
>>
>> Signed-off-by: Austin Christ <austinwc@codeaurora.org>
>> ---
>> v1:
>> 	- Initial
>> v2:
>> 	- Remove whitespace error
>> ---
>>   drivers/i2c/busses/i2c-qup.c | 22 ++++++++++++++--------
>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>> index 904dfec..80af973 100644
>> --- a/drivers/i2c/busses/i2c-qup.c
>> +++ b/drivers/i2c/busses/i2c-qup.c
>> @@ -1648,6 +1648,14 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>>   	clk_disable_unprepare(qup->pclk);
>>   }
>>   
>> +#if IS_ENABLED(CONFIG_ACPI)
>> +static const struct acpi_device_id qup_i2c_acpi_match[] = {
>> +	{ "QCOM8010"},
>> +	{ },
>> +};
>> +MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_match);
>> +#endif
>> +
>>   static int qup_i2c_probe(struct platform_device *pdev)
>>   {
>>   	static const int blk_sizes[] = {4, 16, 32};
>> @@ -1675,6 +1683,12 @@ static int qup_i2c_probe(struct platform_device *pdev)
>>   			DEFAULT_CLK_FREQ);
>>   	}
>>   
>> +	if (acpi_match_device(qup_i2c_acpi_match, qup->dev)) {
>> +		qup->adap.algo = &qup_i2c_algo_v2;
>> +		is_qup_v1 = false;
>> +		goto nodma;
>> +	}
>> +
> 
>   What happens without the above check. Does it not get resolved below,
>   when qup_i2c_req_dma fails ?
> 
> Regards,
>   Sricharan
> 

Yes currently qup_i2c_req_dma fails and then the nodma: path is chosen. 
However, this causes unexpected logging of "tx channel not available" 
for the Centriq platform. This seemed a more correct solution to those 
incorrect logging statements than lowering the dev_err to dev_info or 
dev_notify.
Sricharan R April 13, 2018, 6:22 a.m. | #3
Hi Austin,

On 4/12/2018 8:57 PM, Christ, Austin wrote:
> Hey Sricharan,
> 
> On 4/11/2018 10:28 PM, Sricharan R wrote:
>> Hi Austin,
>>
>> On 4/5/2018 1:14 AM, Austin Christ wrote:
>>> Add support for Qualcomm Centriq devices that are qup-v2 compatible but
>>> do not support DMA, so nodma needs to be set.
>>>
>>> Signed-off-by: Austin Christ <austinwc@codeaurora.org>
>>> ---
>>> v1:
>>>     - Initial
>>> v2:
>>>     - Remove whitespace error
>>> ---
>>>   drivers/i2c/busses/i2c-qup.c | 22 ++++++++++++++--------
>>>   1 file changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>>> index 904dfec..80af973 100644
>>> --- a/drivers/i2c/busses/i2c-qup.c
>>> +++ b/drivers/i2c/busses/i2c-qup.c
>>> @@ -1648,6 +1648,14 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>>>       clk_disable_unprepare(qup->pclk);
>>>   }
>>>   +#if IS_ENABLED(CONFIG_ACPI)
>>> +static const struct acpi_device_id qup_i2c_acpi_match[] = {
>>> +    { "QCOM8010"},
>>> +    { },
>>> +};
>>> +MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_match);
>>> +#endif
>>> +
>>>   static int qup_i2c_probe(struct platform_device *pdev)
>>>   {
>>>       static const int blk_sizes[] = {4, 16, 32};
>>> @@ -1675,6 +1683,12 @@ static int qup_i2c_probe(struct platform_device *pdev)
>>>               DEFAULT_CLK_FREQ);
>>>       }
>>>   +    if (acpi_match_device(qup_i2c_acpi_match, qup->dev)) {
>>> +        qup->adap.algo = &qup_i2c_algo_v2;
>>> +        is_qup_v1 = false;
>>> +        goto nodma;
>>> +    }
>>> +
>>
>>   What happens without the above check. Does it not get resolved below,
>>   when qup_i2c_req_dma fails ?
>>
>> Regards,
>>   Sricharan
>>
> 
> Yes currently qup_i2c_req_dma fails and then the nodma: path is chosen. However, this causes unexpected logging of "tx channel not available" for the Centriq platform. This seemed a more correct solution to those incorrect logging statements than lowering the dev_err to dev_info or dev_notify.

 hmm, then just push this  if (acpi_match_device(qup_i2c_acpi_match, qup->dev) down.

	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-v1.1.1")) {
		....
	} else {
               qup->adap.algo = &qup_i2c_algo_v2;
                is_qup_v1 = false;
		if (acpi_match_device(qup_i2c_acpi_match, qup->dev))
			goto nodma;
		else
	                ret = qup_i2c_req_dma(qup);

Regards,
 Sricharan
Christ, Austin April 13, 2018, 3:50 p.m. | #4
Hey Sricharan,

On 4/13/2018 12:22 AM, Sricharan R wrote:
> Hi Austin,
> 
> On 4/12/2018 8:57 PM, Christ, Austin wrote:
>> Hey Sricharan,
>>
>> On 4/11/2018 10:28 PM, Sricharan R wrote:
>>> Hi Austin,
>>>
>>> On 4/5/2018 1:14 AM, Austin Christ wrote:
>>>> Add support for Qualcomm Centriq devices that are qup-v2 compatible but
>>>> do not support DMA, so nodma needs to be set.
>>>>
>>>> Signed-off-by: Austin Christ <austinwc@codeaurora.org>
>>>> ---
>>>> v1:
>>>>      - Initial
>>>> v2:
>>>>      - Remove whitespace error
>>>> ---
>>>>    drivers/i2c/busses/i2c-qup.c | 22 ++++++++++++++--------
>>>>    1 file changed, 14 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
>>>> index 904dfec..80af973 100644
>>>> --- a/drivers/i2c/busses/i2c-qup.c
>>>> +++ b/drivers/i2c/busses/i2c-qup.c
>>>> @@ -1648,6 +1648,14 @@ static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
>>>>        clk_disable_unprepare(qup->pclk);
>>>>    }
>>>>    +#if IS_ENABLED(CONFIG_ACPI)
>>>> +static const struct acpi_device_id qup_i2c_acpi_match[] = {
>>>> +    { "QCOM8010"},
>>>> +    { },
>>>> +};
>>>> +MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_match);
>>>> +#endif
>>>> +
>>>>    static int qup_i2c_probe(struct platform_device *pdev)
>>>>    {
>>>>        static const int blk_sizes[] = {4, 16, 32};
>>>> @@ -1675,6 +1683,12 @@ static int qup_i2c_probe(struct platform_device *pdev)
>>>>                DEFAULT_CLK_FREQ);
>>>>        }
>>>>    +    if (acpi_match_device(qup_i2c_acpi_match, qup->dev)) {
>>>> +        qup->adap.algo = &qup_i2c_algo_v2;
>>>> +        is_qup_v1 = false;
>>>> +        goto nodma;
>>>> +    }
>>>> +
>>>
>>>    What happens without the above check. Does it not get resolved below,
>>>    when qup_i2c_req_dma fails ?
>>>
>>> Regards,
>>>    Sricharan
>>>
>>
>> Yes currently qup_i2c_req_dma fails and then the nodma: path is chosen. However, this causes unexpected logging of "tx channel not available" for the Centriq platform. This seemed a more correct solution to those incorrect logging statements than lowering the dev_err to dev_info or dev_notify.
> 
>   hmm, then just push this  if (acpi_match_device(qup_i2c_acpi_match, qup->dev) down.
> 
> 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-v1.1.1")) {
> 		....
> 	} else {
>                 qup->adap.algo = &qup_i2c_algo_v2;
>                  is_qup_v1 = false;
> 		if (acpi_match_device(qup_i2c_acpi_match, qup->dev))
> 			goto nodma;
> 		else
> 	                ret = qup_i2c_req_dma(qup);
> 
Ok I'll make this change.

> Regards,
>   Sricharan
>

Patch

diff --git a/drivers/i2c/busses/i2c-qup.c b/drivers/i2c/busses/i2c-qup.c
index 904dfec..80af973 100644
--- a/drivers/i2c/busses/i2c-qup.c
+++ b/drivers/i2c/busses/i2c-qup.c
@@ -1648,6 +1648,14 @@  static void qup_i2c_disable_clocks(struct qup_i2c_dev *qup)
 	clk_disable_unprepare(qup->pclk);
 }
 
+#if IS_ENABLED(CONFIG_ACPI)
+static const struct acpi_device_id qup_i2c_acpi_match[] = {
+	{ "QCOM8010"},
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, qup_i2c_acpi_match);
+#endif
+
 static int qup_i2c_probe(struct platform_device *pdev)
 {
 	static const int blk_sizes[] = {4, 16, 32};
@@ -1675,6 +1683,12 @@  static int qup_i2c_probe(struct platform_device *pdev)
 			DEFAULT_CLK_FREQ);
 	}
 
+	if (acpi_match_device(qup_i2c_acpi_match, qup->dev)) {
+		qup->adap.algo = &qup_i2c_algo_v2;
+		is_qup_v1 = false;
+		goto nodma;
+	}
+
 	if (of_device_is_compatible(pdev->dev.of_node, "qcom,i2c-qup-v1.1.1")) {
 		qup->adap.algo = &qup_i2c_algo;
 		qup->adap.quirks = &qup_i2c_quirks;
@@ -1959,14 +1973,6 @@  static int qup_i2c_resume(struct device *device)
 };
 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_match);
-#endif
-
 static struct platform_driver qup_i2c_driver = {
 	.probe  = qup_i2c_probe,
 	.remove = qup_i2c_remove,