Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Krzysztof Kozlowski]

On 06/03/2023 13:17, Konrad Dybcio wrote:
>> That's still nothing to fix - msm8998 was added here only for
>> compatibility reasons for bindings. It wasn't ever tested on MSM8998 and
>> also probably never written in a way that it should work for MSM8998.
> Don't you think making something like
> 
> compatible = "qcom,msm8998-bwmon"
> 
> not actually compatible with MSM8998 is a bit wrong?

I would argue that's the binding problem, but I get your point. Driver
just incorrectly stated that it could work with msm8998.

> 
>  It
>> could work, but that was not the intention. The driver is supposed to
>> work on sdm845 and according to your description - there is nothing
>> wrong with that.
>>
>>>

(...)

>>>
>>>>
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
>>>>> +		/* Map the global base, if separate */
>>>>> +		base = devm_platform_ioremap_resource(pdev, 1);
>>>>
>>>> Wouldn't this now print errors for sdm845, thus introduce dmesg regression?
>>> I explicitly stated this in the cover letter and asked for opinions.
>>>
>>
>> Sorry, long time ago I stopped reading cover letters, maybe except it's
>> top few sentences. Just too many of them and too much of text usually
>> useless. Commits should describe everything as they go to the Git and
>> they should justify their own existence.
>>
>> Anyway, above dmesg error regression is a no. The devices turn out not
>> to be compatible with each other so just adjust the bindings and match
>> each driver by proper compatible string.
> So what am I supposed to do here? Add "qcom,msm8998-actual-msm8998-bwmon"?

No, make msm8998 binding working for msm8998 and add entry for sdm845.


> Otherwise, changing msm8998 data would break 845-8550, unless I added all
> of them to a separate match table.

Exactly.

>  Or I can add some boilerplate C code
> that would not throw a warning, or perhaps try and introduce
> devm_platform_ioremap_resource_optional..
> 
> I guess the last option sounds the best.

Best regards,
Krzysztof