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
