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

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

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. But so
far you did not provide any arguments for this. Downstream crappy code?
Nope. Existing driver? Same.

Was anything here actually checked with datasheets/hardware?

Best regards,
Krzysztof