[PATCH RESEND 0/3] iommu: omap: Simplify few things

[PATCH RESEND 0/3] iommu: omap: Simplify few things

[Krzysztof Kozlowski]

Resending after 1 month.

Link to v1: https://lore.kernel.org/r/20250111-syscon-phandle-args-iommu-v1-0-3767dee585a6@linaro.org

Few code simplifications without functional impact.  Not tested on
hardware.

Best regards,
Krzysztof

---
Krzysztof Kozlowski (3):
      iommu: omap: Drop redundant check if ti,syscon-mmuconfig exists
      iommu: omap: Simplify returning syscon PTR_ERR
      iommu: omap: Use syscon_regmap_lookup_by_phandle_args

 drivers/iommu/omap-iommu.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)
---
base-commit: c674aa7c289e51659e40dda0f954886ef7f80042
change-id: 20250111-syscon-phandle-args-iommu-2ea162b223a4

Best regards,
-- 
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

[PATCH RESEND 1/3] iommu: omap: Drop redundant check if ti,syscon-mmuconfig exists

[Krzysztof Kozlowski]

The syscon_regmap_lookup_by_phandle() will fail if property does not
exist, so doing of_property_read_bool() earlier is redundant.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iommu/omap-iommu.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 3c62337f43c67720a15b67e8b610da7886f6f39c..04a7deaaba25cb270eb6eeaf6a21030440f78a5e 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1128,11 +1128,6 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
 	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
 		return 0;
 
-	if (!of_property_read_bool(np, "ti,syscon-mmuconfig")) {
-		dev_err(&pdev->dev, "ti,syscon-mmuconfig property is missing\n");
-		return -EINVAL;
-	}
-
 	obj->syscfg =
 		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
 	if (IS_ERR(obj->syscfg)) {

-- 
2.43.0

[PATCH RESEND 2/3] iommu: omap: Simplify returning syscon PTR_ERR

[Krzysztof Kozlowski]

No need to store PTR_ERR into temporary, local 'ret' variable.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iommu/omap-iommu.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 04a7deaaba25cb270eb6eeaf6a21030440f78a5e..bce27805805010ae473aa8dbd9e0cb903dd79bba 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1123,7 +1123,6 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
 					      struct omap_iommu *obj)
 {
 	struct device_node *np = pdev->dev.of_node;
-	int ret;
 
 	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
 		return 0;
@@ -1132,8 +1131,7 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
 		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
 	if (IS_ERR(obj->syscfg)) {
 		/* can fail with -EPROBE_DEFER */
-		ret = PTR_ERR(obj->syscfg);
-		return ret;
+		return PTR_ERR(obj->syscfg);
 	}
 
 	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,

-- 
2.43.0

[PATCH RESEND 3/3] iommu: omap: Use syscon_regmap_lookup_by_phandle_args

[Krzysztof Kozlowski]

Use syscon_regmap_lookup_by_phandle_args() which is a wrapper over
syscon_regmap_lookup_by_phandle() combined with getting the syscon
argument.  Except simpler code this annotates within one line that given
phandle has arguments, so grepping for code would be easier.

There is also no real benefit in printing errors on missing syscon
argument, because this is done just too late: runtime check on
static/build-time data.  Dtschema and Devicetree bindings offer the
static/build-time check for this already.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 drivers/iommu/omap-iommu.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bce27805805010ae473aa8dbd9e0cb903dd79bba..f12812d3f828ad382c7a84f6ecd604c8f35a6d10 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1127,19 +1127,13 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
 	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
 		return 0;
 
-	obj->syscfg =
-		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
+	obj->syscfg = syscon_regmap_lookup_by_phandle_args(np, "ti,syscon-mmuconfig",
+							   1, &obj->id);
 	if (IS_ERR(obj->syscfg)) {
 		/* can fail with -EPROBE_DEFER */
 		return PTR_ERR(obj->syscfg);
 	}
 
-	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
-				       &obj->id)) {
-		dev_err(&pdev->dev, "couldn't get the IOMMU instance id within subsystem\n");
-		return -EINVAL;
-	}
-
 	if (obj->id != 0 && obj->id != 1) {
 		dev_err(&pdev->dev, "invalid IOMMU instance id\n");
 		return -EINVAL;

-- 
2.43.0

Re: [PATCH RESEND 2/3] iommu: omap: Simplify returning syscon PTR_ERR

[Robin Murphy]

On 2025-02-12 8:19 pm, Krzysztof Kozlowski wrote:
> No need to store PTR_ERR into temporary, local 'ret' variable.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   drivers/iommu/omap-iommu.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 04a7deaaba25cb270eb6eeaf6a21030440f78a5e..bce27805805010ae473aa8dbd9e0cb903dd79bba 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1123,7 +1123,6 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
>   					      struct omap_iommu *obj)
>   {
>   	struct device_node *np = pdev->dev.of_node;
> -	int ret;
>   
>   	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
>   		return 0;
> @@ -1132,8 +1131,7 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
>   		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
>   	if (IS_ERR(obj->syscfg)) {
>   		/* can fail with -EPROBE_DEFER */

This comment is no longer correct or useful, since it's alluding to the 
check which you removed in the previous patch. I'd just clean up the 
whole lot in patch #1 as it's all closely related, and also turn this 
return into a dev_err_probe() to capture the spirit of the other errors 
being replaced, perhaps something like "No valid ti,syscon-mmuconfig 
available" - that way it adds some value for debugging probe deferral 
issues more than broken DTs.

Thanks,
Robin.

> -		ret = PTR_ERR(obj->syscfg);
> -		return ret;
> +		return PTR_ERR(obj->syscfg);
>   	}
>   
>   	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
> 

Re: [PATCH RESEND 3/3] iommu: omap: Use syscon_regmap_lookup_by_phandle_args

[Robin Murphy]

On 2025-02-12 8:19 pm, Krzysztof Kozlowski wrote:
> Use syscon_regmap_lookup_by_phandle_args() which is a wrapper over
> syscon_regmap_lookup_by_phandle() combined with getting the syscon
> argument.  Except simpler code this annotates within one line that given
> phandle has arguments, so grepping for code would be easier.
> 
> There is also no real benefit in printing errors on missing syscon
> argument, because this is done just too late: runtime check on
> static/build-time data.  Dtschema and Devicetree bindings offer the
> static/build-time check for this already.

With my suggestion for capturing a more useful general error in the 
other change, I agree with this one.

Reviewed-by: Robin Murphy <robin.murphy@arm.com>

> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>   drivers/iommu/omap-iommu.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index bce27805805010ae473aa8dbd9e0cb903dd79bba..f12812d3f828ad382c7a84f6ecd604c8f35a6d10 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -1127,19 +1127,13 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
>   	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
>   		return 0;
>   
> -	obj->syscfg =
> -		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
> +	obj->syscfg = syscon_regmap_lookup_by_phandle_args(np, "ti,syscon-mmuconfig",
> +							   1, &obj->id);
>   	if (IS_ERR(obj->syscfg)) {
>   		/* can fail with -EPROBE_DEFER */
>   		return PTR_ERR(obj->syscfg);
>   	}
>   
> -	if (of_property_read_u32_index(np, "ti,syscon-mmuconfig", 1,
> -				       &obj->id)) {
> -		dev_err(&pdev->dev, "couldn't get the IOMMU instance id within subsystem\n");
> -		return -EINVAL;
> -	}
> -
>   	if (obj->id != 0 && obj->id != 1) {
>   		dev_err(&pdev->dev, "invalid IOMMU instance id\n");
>   		return -EINVAL;
> 

Re: [PATCH RESEND 2/3] iommu: omap: Simplify returning syscon PTR_ERR

[Krzysztof Kozlowski]

On 13/02/2025 12:30, Robin Murphy wrote:
> On 2025-02-12 8:19 pm, Krzysztof Kozlowski wrote:
>> No need to store PTR_ERR into temporary, local 'ret' variable.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   drivers/iommu/omap-iommu.c | 4 +---
>>   1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>> index 04a7deaaba25cb270eb6eeaf6a21030440f78a5e..bce27805805010ae473aa8dbd9e0cb903dd79bba 100644
>> --- a/drivers/iommu/omap-iommu.c
>> +++ b/drivers/iommu/omap-iommu.c
>> @@ -1123,7 +1123,6 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
>>   					      struct omap_iommu *obj)
>>   {
>>   	struct device_node *np = pdev->dev.of_node;
>> -	int ret;
>>   
>>   	if (!of_device_is_compatible(np, "ti,dra7-dsp-iommu"))
>>   		return 0;
>> @@ -1132,8 +1131,7 @@ static int omap_iommu_dra7_get_dsp_system_cfg(struct platform_device *pdev,
>>   		syscon_regmap_lookup_by_phandle(np, "ti,syscon-mmuconfig");
>>   	if (IS_ERR(obj->syscfg)) {
>>   		/* can fail with -EPROBE_DEFER */
> 
> This comment is no longer correct or useful, since it's alluding to the 
> check which you removed in the previous patch. I'd just clean up the 


I wouldn't call it useful ever, but I assume someone wants to note for
themself how syscon lookup works...

> whole lot in patch #1 as it's all closely related, and also turn this 
> return into a dev_err_probe() to capture the spirit of the other errors 
> being replaced, perhaps something like "No valid ti,syscon-mmuconfig 
> available" - that way it adds some value for debugging probe deferral 
> issues more than broken DTs.

Ack


Best regards,
Krzysztof