Re: [PATCH] drm/omap/dss: Use syscon_regmap_lookup_by_phandle_args

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/01/2025 15:23, Tomi Valkeinen wrote:
> Hi,
> 
> On 12/01/2025 15:46, 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.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>> ---
>>   drivers/gpu/drm/omapdrm/dss/dss.c | 14 ++++----------
>>   1 file changed, 4 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/dss.c b/drivers/gpu/drm/omapdrm/dss/dss.c
>> index 7b2df3185de4..692df747e2ae 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/dss.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/dss.c
>> @@ -1236,21 +1236,15 @@ static int dss_video_pll_probe(struct dss_device *dss)
>>   	if (!np)
>>   		return 0;
>>   
>> -	if (of_property_read_bool(np, "syscon-pll-ctrl")) {
>> -		dss->syscon_pll_ctrl = syscon_regmap_lookup_by_phandle(np,
>> -			"syscon-pll-ctrl");
>> +	if (of_property_present(np, "syscon-pll-ctrl")) {

I see now that I combined two changes I wanted to split. This should be
a separate commit.

>> +		dss->syscon_pll_ctrl =
>> +			syscon_regmap_lookup_by_phandle_args(np, "syscon-pll-ctrl",
>> +							     1, &dss->syscon_pll_ctrl_offset);
>>   		if (IS_ERR(dss->syscon_pll_ctrl)) {
>>   			dev_err(&pdev->dev,
>>   				"failed to get syscon-pll-ctrl regmap\n");
>>   			return PTR_ERR(dss->syscon_pll_ctrl);
>>   		}
>> -
>> -		if (of_property_read_u32_index(np, "syscon-pll-ctrl", 1,
>> -				&dss->syscon_pll_ctrl_offset)) {
>> -			dev_err(&pdev->dev,
>> -				"failed to get syscon-pll-ctrl offset\n");
>> -			return -EINVAL;
>> -		}
>>   	}
>>   
>>   	pll_regulator = devm_regulator_get(&pdev->dev, "vdda_video");
> 
> Looks reasonable to me. Tested on dra76 evm.
> 
> I can push this to drm-misc-next later if I see no other comments.
> 
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Let me send a v2 with your tag.

Best regards,
Krzysztof