Re: [PATCH v2 1/6] dt-bindings: media: qcom,sc8280xp-camss: Fix interrupt types

[Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>]

Hi Krzysztof.

On 10/8/24 14:15, Krzysztof Kozlowski wrote:
> On 08/10/2024 12:02, Vladimir Zapolskiy wrote:
>> Hi Bjorn,
>>
>> On 10/6/24 05:36, Bjorn Andersson wrote:
>>> On Mon, Sep 23, 2024 at 10:28:22AM GMT, Vladimir Zapolskiy wrote:
>>>> The expected type of all CAMSS interrupts is edge rising, fix it in
>>>> the documented example from CAMSS device tree bindings for sc8280xp.
>>>>
>>>
>>> Who/what expects them to be RISING?
>>
>> I've checked CAMSS device tree bindings in a number of downstream kernels,
>> all of them describe interrupt types as edge rising.
>>
>> $ grep -Hn IRQF_TRIGGER drivers/media/platform/qcom/camss/*
>> drivers/media/platform/qcom/camss/camss-csid.c:619:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>> drivers/media/platform/qcom/camss/camss-csiphy.c:605:			       IRQF_TRIGGER_RISING | IRQF_NO_AUTOEN,
>> drivers/media/platform/qcom/camss/camss-ispif.c:1164:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>> drivers/media/platform/qcom/camss/camss-ispif.c:1168:			       IRQF_TRIGGER_RISING, ispif->irq_name, ispif);
>> drivers/media/platform/qcom/camss/camss-vfe.c:1327:			       IRQF_TRIGGER_RISING, vfe->irq_name, vfe);
> 
> Downstream has a lot of bad code, so I am not sure how good argument
> this is.
> 
> I acked the patch because I assumed you *checked in hardware*.
> 
>>
>>   From runtime point of view it's more important to get re-probed camss
>> driver, see an absolutely similar and previously discussed case (in the
>> cover letter):
>>
>> https://lore.kernel.org/lkml/20220530080842.37024-4-manivannan.sadhasivam@linaro.org/
>>
>> Now in runtime I get this error, it's easy to check by unbinding/binding any
>> camss device:
>>
>> irq: type mismatch, failed to map hwirq-509 for interrupt-controller@17a00000!
>>
>> Basically camss devices can not be bound on the second time on the
>> number of platforms touched by this changeset.
> 
> This is solveable different way and I do not understand this rationale.
> The driver should not request trigger type but use what DTS is
> providing, unless of course only one valid trigger is possible.

Right at the moment the driver uses rising edge type of interrupts, and
it works properly.

> But so
> far you did not provide any arguments for this. Downstream crappy code?

Downstream code works, that's the argument to support the change.

> Nope. Existing driver? Same.

The existing driver works, that's the argument to support the change.

> Was anything here actually checked with datasheets/hardware?

The initially open question is unanswered, why sc8280xp CAMSS does
specify interrupts as level high type, was it actually checked with
datasheets/hardware, as you say it? It has never been tested by anyone
and anywhere, downstream or upstream wise, only rising edge interrupts
were tested, and they do work.

I don't have access to datasheets or hardware of sc8280xp powered board,
someone may either verify, if CAMSS level high type interrupts are
supported/working at all or not (obviously its current presence in dts is
insufficient), or check the SoC datasheet.

To sum up, the intention of this change:
1) fix the unpleasant runtime issue with no regressions (it's been tested),
2) align CAMSS device description in firmware with known to work well
IP hardware configuration.

--
Best wishes,
Vladimir