On Tue, Oct 20, 2020 at 03:00:58PM +0300, Sakari Ailus wrote:

Hi Krzysztof,

On Tue, Oct 20, 2020 at 12:54:09PM +0200, Krzysztof Kozlowski wrote:

quoted

On Tue, 20 Oct 2020 at 12:38, Sakari Ailus [off-list ref] wrote:

quoted

Hi Krzysztof,

On Mon, Oct 19, 2020 at 07:02:44PM +0200, Krzysztof Kozlowski wrote:

quoted

Add bindings for the IMX258 camera sensor.  The bindings, just like the
driver, are quite limited, e.g. do not support regulator supplies.

Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>

---

Changes since v4:
1. Add clock-lanes,
2. Add Rob's review,
3. Add one more example and extend existing one,
4. Add common clock properties (assigned-*).

Using the assigned-* clock properties may be workable for this driver at
the moment. But using these properties does not guarantee the external
clock frequency intended to be used on the hardware.

It guarantees it. The clock frequency will be as expected (except if
someone misconfigures the DTS).

Is that guaranteed?

I'm not saying no to the approach, but if we change how camera sensor DT
bindings are defined, I'd prefer an informed decision is made on the
matter.

quoted

quoted

Using other
frequencies *is not* expected to work. That applies to this driver as well.

This is the binding which is HW description. According to HW datasheet
other frequencies from described range are accepted and expected to
work.

As per datasheet, yes, different external clock frequencies can be used.
But the link frequency is still not independent of the external clock
frequency.

The properties of the sensor's PLL tree determines what can be achieved
given a certain external clock frequency. So picking a wrong external clock
frequency quite possibly means that none of the designated link frequencies
are available, rendering the sensor inoperable.

The driver then controls the HW and knows exactly what is needed. If
link frequency (which has its own DT property) requires some clock
frequency, the driver will configure the clock to that value. The same
going other direction. Driver has the knowledge about both its input
clock and link frequency, therefore it can make the best decision.

If someone configures DT or the driver to wrong frequencies (making the
link frequencies not available), the solution is not to add more
properties. It would be a band aid. The solution is to configure it
correctly.

quoted

quoted

This, instead of the clock-frequency property, effectively removes the
ability to set the correct frequency from the driver, at least with current
set of the used APIs.

It seems you confuse DT bindings with some specific driver
implementation. Bindings do not describe the driver behavior but the
HW. The ability to set the correct frequency from the driver is not
removed. It was never part of the bindings and never should. It is
part of the driver.

quoted

I suppose you could add a function to set the assigned clock frequency and
keep it, just as clk_set_rate_exclusive does?

I did not reply to this comment, so let me know. Of course, one could
add such functions. It's not a job for DT bindings, though.

quoted

quoted

Cc the common clock framework list + maintainers.

The bindings have Rob review which is the DT maintainer. His
ack/review is needed for the bindings to be accepted. What more do you
need? Shall I point to submitting-bindings document?

I am really tired of discussing this. You raise some concerns about
driver behavior in the wrong context - in the patch for device tree
bindings. You use the arguments about the driver while we talk about
bindings. This is clearly not correct. I am all the time repeating
myself - the bindings describe the hardware, not the driver.

My concerns are not related to the current driver implementation nor the
driver patches in this set.

Yet you refer to driver related tasks (configuring provided clock) while
discussing the bindings.

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel