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

From: Krzysztof Kozlowski
Date: Tue Jan 14 2025 - 09:44:05 EST


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@xxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxx>
Let me send a v2 with your tag.

Best regards,
Krzysztof