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@xxxxxxxxxx/
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
