| Message ID | 20241012-b4-linux-next-202041004-i2c-media-yaml-fixes-v1-1-a2bb12a1796d@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: i2c: Cleanup assigned-clocks and endpoint: properties: unevaluatedProperties: false | expand |
On 14/10/2024 10:29, Bryan O'Donoghue wrote: > On 14/10/2024 08:43, Krzysztof Kozlowski wrote: >>> - - assigned-clocks >>> - - assigned-clock-rates >> That's not extraneous, but has a meaning that without assigned-clocks >> this device or driver will not operate. >> >> File should rather stay as is. > > Hmm, I've obviously missed a trick here. > > I'll check it out. My response was probably not complete: this still might be extraneous, because maybe the driver/device do not care. But in general requiring assigned-clocks could have a meaning. Best regards, Krzysztof
On 14/10/2024 09:44, Krzysztof Kozlowski wrote: > On 14/10/2024 10:29, Bryan O'Donoghue wrote: >> On 14/10/2024 08:43, Krzysztof Kozlowski wrote: >>>> - - assigned-clocks >>>> - - assigned-clock-rates >>> That's not extraneous, but has a meaning that without assigned-clocks >>> this device or driver will not operate. >>> >>> File should rather stay as is. >> >> Hmm, I've obviously missed a trick here. >> >> I'll check it out. > > My response was probably not complete: this still might be extraneous, > because maybe the driver/device do not care. But in general requiring > assigned-clocks could have a meaning. No I see what you mean Even though assigned-clock* is a property of the SoC this driver.. drivers/media/i2c/hi846.c mclk_freq = clk_get_rate(hi846->clock); if (mclk_freq != 25000000) dev_warn(&client->dev, "External clock freq should be 25000000, not %u.\n", mclk_freq); doesn't support setting the clock. So it actually is a requirement, yes. --- bod
On Mon, Oct 14, 2024 at 09:43:07AM +0200, Krzysztof Kozlowski wrote: > On Sat, Oct 12, 2024 at 04:02:50PM +0100, Bryan O'Donoghue wrote: > > Remove extraneous assigned-clock* from media/i2c/* schemas, retain in the > > relevant examples. > > > > Link: https://lore.kernel.org/linux-media/j7kgz2lyxnler5qwd7yiazdq6fmsv77kyozdrxf33h54ydakjz@uqjhwhoyv6re > > Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > --- > > Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml | 8 -------- > > Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml | 8 -------- > > Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml | 8 -------- > > Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml | 4 ---- > > Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ---- > > Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 4 ---- > > Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml | 4 ---- > > Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 4 ---- > > 8 files changed, 44 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml > > index 60f19e1152b33128cf3baa15b8c70a874ca6d52e..d18ead8f7fc43bfacc291aed85b5ca9166c46edb 100644 > > --- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml > > +++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml > > @@ -28,12 +28,6 @@ properties: > > items: > > - description: Reference to the mclk clock. > > > > - assigned-clocks: > > - maxItems: 1 > > - > > - assigned-clock-rates: > > - maxItems: 1 > > - > > reset-gpios: > > description: Reference to the GPIO connected to the RESETB pin. Active low. > > maxItems: 1 > > @@ -82,8 +76,6 @@ required: > > - compatible > > - reg > > - clocks > > - - assigned-clocks > > - - assigned-clock-rates > > That's not extraneous, but has a meaning that without assigned-clocks > this device or driver will not operate. How so ? Even if we assume that the device requires a specific clock frequency (which is often not the case for camera sensors, the limitation usually comes from drivers, so the constraint shouldn't be expressed in the bindings in that case), there is no overall requirement to assign a clock rate as in many cases the clock will come from a fixed-frequency oscillator. This seems to be a constraint that is outside of the scope of DT bindings. It is similar to regulators, where the regulator consumer doesn't have a way to express supported voltages in its DT bindings. > File should rather stay as is. > > > - vddio-supply > > - vdda-supply > > - vddd-supply [snip]
On Tue, Oct 15, 2024 at 02:29:22PM +0300, Laurent Pinchart wrote: > On Tue, Oct 15, 2024 at 08:13:19AM +0200, Krzysztof Kozlowski wrote: > > On 14/10/2024 22:34, Laurent Pinchart wrote: > > > On Mon, Oct 14, 2024 at 09:43:07AM +0200, Krzysztof Kozlowski wrote: > > >> On Sat, Oct 12, 2024 at 04:02:50PM +0100, Bryan O'Donoghue wrote: > > >>> Remove extraneous assigned-clock* from media/i2c/* schemas, retain in the > > >>> relevant examples. > > >>> > > >>> Link: https://lore.kernel.org/linux-media/j7kgz2lyxnler5qwd7yiazdq6fmsv77kyozdrxf33h54ydakjz@uqjhwhoyv6re > > >>> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> > > >>> --- > > >>> Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml | 8 -------- > > >>> Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml | 8 -------- > > >>> Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml | 8 -------- > > >>> Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml | 4 ---- > > >>> Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ---- > > >>> Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 4 ---- > > >>> Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml | 4 ---- > > >>> Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 4 ---- > > >>> 8 files changed, 44 deletions(-) > > >>> > > >>> diff --git a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml > > >>> index 60f19e1152b33128cf3baa15b8c70a874ca6d52e..d18ead8f7fc43bfacc291aed85b5ca9166c46edb 100644 > > >>> --- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml > > >>> +++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml > > >>> @@ -28,12 +28,6 @@ properties: > > >>> items: > > >>> - description: Reference to the mclk clock. > > >>> > > >>> - assigned-clocks: > > >>> - maxItems: 1 > > >>> - > > >>> - assigned-clock-rates: > > >>> - maxItems: 1 > > >>> - > > >>> reset-gpios: > > >>> description: Reference to the GPIO connected to the RESETB pin. Active low. > > >>> maxItems: 1 > > >>> @@ -82,8 +76,6 @@ required: > > >>> - compatible > > >>> - reg > > >>> - clocks > > >>> - - assigned-clocks > > >>> - - assigned-clock-rates > > >> > > >> That's not extraneous, but has a meaning that without assigned-clocks > > >> this device or driver will not operate. > > > > > > How so ? Even if we assume that the device requires a specific clock > > > frequency (which is often not the case for camera sensors, the > > > limitation usually comes from drivers, so the constraint shouldn't be > > > expressed in the bindings in that case), there is no overall requirement > > > to assign a clock rate as in many cases the clock will come from a > > > fixed-frequency oscillator. This seems to be a constraint that is > > > outside of the scope of DT bindings. It is similar to regulators, where > > > the regulator consumer doesn't have a way to express supported voltages > > > in its DT bindings. > > > > This property does not say it comes from a fixed-frequency oscillator, > > so I do not understand why you think it is unreasonable constraint. I > > have no clue what the author wanted to say here, so I just explained > > that there is a meaning behind requiring such properties. If you claim > > device or implementations do not have such requirement, after > > investigating each case, feel free to drop this. I think I also stated > > this already in other reply. > > For camera sensor drivers I'm pretty sure we can drop those properties > when they're marked as required, as there's no intrinsic characteristics > of camera sensors that would require assigned-clock*. I tend to agree, and would take it one step further to say that applies everywhere. Whatever clock setup needed is outside the scope of a binding. The simplest case is all input clocks are fixed. The next simplest case is firmware did all clock setup needed for a specific board and the boot time default works. Rob
On 15/10/2024 18:46, Rob Herring wrote: >>>> >>>> How so ? Even if we assume that the device requires a specific clock >>>> frequency (which is often not the case for camera sensors, the >>>> limitation usually comes from drivers, so the constraint shouldn't be >>>> expressed in the bindings in that case), there is no overall requirement >>>> to assign a clock rate as in many cases the clock will come from a >>>> fixed-frequency oscillator. This seems to be a constraint that is >>>> outside of the scope of DT bindings. It is similar to regulators, where >>>> the regulator consumer doesn't have a way to express supported voltages >>>> in its DT bindings. >>> >>> This property does not say it comes from a fixed-frequency oscillator, >>> so I do not understand why you think it is unreasonable constraint. I >>> have no clue what the author wanted to say here, so I just explained >>> that there is a meaning behind requiring such properties. If you claim >>> device or implementations do not have such requirement, after >>> investigating each case, feel free to drop this. I think I also stated >>> this already in other reply. >> >> For camera sensor drivers I'm pretty sure we can drop those properties >> when they're marked as required, as there's no intrinsic characteristics >> of camera sensors that would require assigned-clock*. > > I tend to agree, and would take it one step further to say that applies > everywhere. Whatever clock setup needed is outside the scope of a > binding. The simplest case is all input clocks are fixed. The next > simplest case is firmware did all clock setup needed for a specific > board and the boot time default works. Ack. Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
diff --git a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml index 60f19e1152b33128cf3baa15b8c70a874ca6d52e..d18ead8f7fc43bfacc291aed85b5ca9166c46edb 100644 --- a/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml +++ b/Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml @@ -28,12 +28,6 @@ properties: items: - description: Reference to the mclk clock. - assigned-clocks: - maxItems: 1 - - assigned-clock-rates: - maxItems: 1 - reset-gpios: description: Reference to the GPIO connected to the RESETB pin. Active low. maxItems: 1 @@ -82,8 +76,6 @@ required: - compatible - reg - clocks - - assigned-clocks - - assigned-clock-rates - vddio-supply - vdda-supply - vddd-supply diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml index 1f497679168c8395a94b3867beb49b251ef621fc..622243cae03caa5d14aa312df40ef5907e190d2c 100644 --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml @@ -20,12 +20,6 @@ properties: items: - description: XVCLK Clock - assigned-clocks: - maxItems: 1 - - assigned-clock-rates: - maxItems: 1 - dvdd-supply: description: Digital Domain Power Supply @@ -68,8 +62,6 @@ required: - compatible - reg - clocks - - assigned-clocks - - assigned-clock-rates - dvdd-supply - dovdd-supply - port diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml index 8a70e23ba6abed67d8b61c33bd7a261089bddda2..382d7de7a89bcea11be384a2a3800512994f9346 100644 --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml @@ -20,12 +20,6 @@ properties: items: - description: EXTCLK Clock - assigned-clocks: - maxItems: 1 - - assigned-clock-rates: - maxItems: 1 - dvdd-supply: description: Digital Domain Power Supply @@ -68,8 +62,6 @@ required: - compatible - reg - clocks - - assigned-clocks - - assigned-clock-rates - dvdd-supply - avdd-supply - dovdd-supply diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml index 79a7658f6d0547e4d6fb2267e5757eedf49fd416..38325cf318f7bd2cd60a4c7bbb6a65b54b855e26 100644 --- a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml +++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml @@ -27,10 +27,6 @@ properties: description: I2C address maxItems: 1 - assigned-clocks: true - assigned-clock-parents: true - assigned-clock-rates: true - clocks: description: Clock frequency from 6 to 27MHz maxItems: 1 diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml index c978abc0cdb35cfe2b85069946cf1be435a58cb8..f0f9726a2add89492b8c56e17ed607841baa3a0d 100644 --- a/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml @@ -24,10 +24,6 @@ properties: - sony,imx258 - sony,imx258-pdaf - assigned-clocks: true - assigned-clock-parents: true - assigned-clock-rates: true - clocks: description: Clock frequency from 6 to 27 MHz. diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml index bce57b22f7b63bd73f08d8831d9bb04858ef03e0..872b8288948b2bba743f2365a55165181df156ae 100644 --- a/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml @@ -24,10 +24,6 @@ properties: description: I2C address maxItems: 1 - assigned-clocks: true - assigned-clock-parents: true - assigned-clock-rates: true - clocks: description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz maxItems: 1 diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml index 77bf3a4ee89db3b5d16149470c0380ef8f1aeac1..38bd1c7304a59bb5fea90954c1e4e626a7c86f2f 100644 --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml @@ -24,10 +24,6 @@ properties: description: I2C address maxItems: 1 - assigned-clocks: true - assigned-clock-parents: true - assigned-clock-rates: true - clocks: description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz maxItems: 1 diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml index d9b7815650fdb890418fc96c828acc9147dfb6e8..ece1e17fe34553671e61c965eb1833c5eb08131b 100644 --- a/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml @@ -26,10 +26,6 @@ properties: description: I2C address maxItems: 1 - assigned-clocks: true - assigned-clock-parents: true - assigned-clock-rates: true - clocks: description: Clock frequency 6MHz, 12MHz, 18MHz, 24MHz or 27MHz maxItems: 1
Remove extraneous assigned-clock* from media/i2c/* schemas, retain in the relevant examples. Link: https://lore.kernel.org/linux-media/j7kgz2lyxnler5qwd7yiazdq6fmsv77kyozdrxf33h54ydakjz@uqjhwhoyv6re Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org> --- Documentation/devicetree/bindings/media/i2c/hynix,hi846.yaml | 8 -------- Documentation/devicetree/bindings/media/i2c/ovti,ov5648.yaml | 8 -------- Documentation/devicetree/bindings/media/i2c/ovti,ov8865.yaml | 8 -------- Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml | 4 ---- Documentation/devicetree/bindings/media/i2c/sony,imx258.yaml | 4 ---- Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml | 4 ---- Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml | 4 ---- Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml | 4 ---- 8 files changed, 44 deletions(-)