Hi Rob, Krzysztof, Quoting Kieran Bingham (2023-10-11 10:51:08) > Quoting Rob Herring (2023-10-10 18:09:41) > > On Tue, Oct 10, 2023 at 01:51:22AM +0100, Kieran Bingham wrote: > > > Add the bindings for the supply references used on the IMX335. > > > > > > Signed-off-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx> > > > --- > > > .../bindings/media/i2c/sony,imx335.yaml | 16 ++++++++++++++++ > > > 1 file changed, 16 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > > index a167dcdb3a32..1863b5608a5c 100644 > > > --- a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > > @@ -32,6 +32,15 @@ properties: > > > description: Clock frequency from 6 to 27 MHz, 37.125MHz, 74.25MHz > > > maxItems: 1 > > > > > > + avdd-supply: > > > + description: Analog power supply (2.9V) > > > + > > > + ovdd-supply: > > > + description: Interface power supply (1.8V) > > > + > > > + dvdd-supply: > > > + description: Digital power supply (1.2V) > > > + > > > reset-gpios: > > > description: Reference to the GPIO connected to the XCLR pin, if any. > > > maxItems: 1 > > > @@ -60,6 +69,9 @@ required: > > > - compatible > > > - reg > > > - clocks > > > + - avdd-supply > > > + - ovdd-supply > > > + - dvdd-supply > > > > New required properties are an ABI break. That's fine only if you can > > explain no one is using this binding. > No one is using this /in-kernel-tree/. This could be because the original support for IMX335 was added with ACPI devices in mind, but even for device-tree, that's not surprising as cameras may often be described in overlays, unless embedded in specific products. I'm trying to revise this series for a v2. Could I get a decision from the DT maintainers on which direction I should take this please? Would you prefer supplies to be 'required' (if supplies should always be required) - or should I leave this as optional as the binding has previously been accepted? > I made these required due to a previous review comment on another > driver: > > - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@xxxxxxxxxx/ > > I hadn't thought about the ABI break though. > > So to clarify (for me): > - New bindings should always add -supply's as required. > - Adding -supply to existing bindings should be optional. > > I guess that leaves a mix of devices that either are required or may be > optional - but perhaps that can't be helped if the bindings have already > got in. > > The IMX335 driver was added in 45d19b5fb9ae ("media: i2c: Add imx335 > camera sensor driver"), and the bindings in 932741d451a5 ("media: > dt-bindings: media: Add bindings for imx335") by Martina, who looks to > be an Intel employee - so I suspect this is used through ACPI so far and > not device tree. > > Danielle, get_maintainer tells me you are looking after this device - > can you confirm this ? > > -- > Kieran > > > > > > > - port > > > > > > additionalProperties: false > > > @@ -79,6 +91,10 @@ examples: > > > assigned-clock-parents = <&imx335_clk_parent>; > > > assigned-clock-rates = <24000000>; > > > > > > + avdd-supply = <&camera_vdda_2v9>; > > > + ovdd-supply = <&camera_vddo_1v8>; > > > + dvdd-supply = <&camera_vddd_1v2>; > > > + > > > port { > > > imx335: endpoint { > > > remote-endpoint = <&cam>; > > > -- > > > 2.34.1 > > >
