| Message ID | 20231010005126.3425444-2-kieran.bingham@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] media: dt-bindings: media: imx335: Add supply bindings | expand |
Hi Kieran, Thank you for the patch On 10/10/23 6:21 AM, Kieran Bingham wrote: > Add the bindings for the supply references used on the IMX335. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> LGTM, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > .../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 > - 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>;
On 23-10-10, Kieran Bingham wrote: > Add the bindings for the supply references used on the IMX335. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Reviewed-by: Marco Felsch <m.felsch@pengutronix.de> > --- > .../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 > - 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 > > >
Hi Kieran, 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@ideasonboard.com> > --- > .../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) I wonder what's the policy in this case --- some of the regulators are often hard-wired and the bindings didn't have them previously either (I wonder why, maybe they were all hard wired in the board??). Could they be optional? The driver will need to be able to do without these in any case. > + > 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 > - 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>;
Hi Sakari, Quoting Sakari Ailus (2023-10-10 07:06:26) > Hi Kieran, > > 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@ideasonboard.com> > > --- > > .../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) > > I wonder what's the policy in this case --- some of the regulators are > often hard-wired and the bindings didn't have them previously either (I > wonder why, maybe they were all hard wired in the board??). > > Could they be optional? The driver will need to be able to do without these > in any case. Indeed - many devices do not need to define how they are powered up. But Krzysztof stated that supplies should be required by the bindings on my recent posting for a VCM driver: - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/ So based on that I have made these 'required'. Even in my case here, with a camera module that is compatible with the Raspberry Pi camera connector - there isn't really 3 supplies. It's just a single gpio enable pin to bring this device up for me. Of course that's specific to the module not the sensor. > > + > > 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 > > - 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>; > > -- > Kind regards, > > Sakari Ailus
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@ideasonboard.com> > > --- > > .../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. I made these required due to a previous review comment on another driver: - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/ 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 > >
Hi Kieran, On Tue, Oct 10, 2023 at 02:25:09PM +0100, Kieran Bingham wrote: > Hi Sakari, > > Quoting Sakari Ailus (2023-10-10 07:06:26) > > Hi Kieran, > > > > 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@ideasonboard.com> > > > --- > > > .../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) > > > > I wonder what's the policy in this case --- some of the regulators are > > often hard-wired and the bindings didn't have them previously either (I > > wonder why, maybe they were all hard wired in the board??). > > > > Could they be optional? The driver will need to be able to do without these > > in any case. > > Indeed - many devices do not need to define how they are powered up. > > But Krzysztof stated that supplies should be required by the bindings on > my recent posting for a VCM driver: > > - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/ > > So based on that I have made these 'required'. I guess it's good to align bindings regarding this, in practice the driver will need to work without regulators (or with dummies), too. > > Even in my case here, with a camera module that is compatible with the > Raspberry Pi camera connector - there isn't really 3 supplies. It's just > a single gpio enable pin to bring this device up for me. Of course > that's specific to the module not the sensor. How do you declare that in DT? One of the regulators will be a GPIO one?
Quoting Sakari Ailus (2023-10-11 12:01:23) > Hi Kieran, > > On Tue, Oct 10, 2023 at 02:25:09PM +0100, Kieran Bingham wrote: > > Hi Sakari, > > > > Quoting Sakari Ailus (2023-10-10 07:06:26) > > > Hi Kieran, > > > > > > 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@ideasonboard.com> > > > > --- > > > > .../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) > > > > > > I wonder what's the policy in this case --- some of the regulators are > > > often hard-wired and the bindings didn't have them previously either (I > > > wonder why, maybe they were all hard wired in the board??). > > > > > > Could they be optional? The driver will need to be able to do without these > > > in any case. > > > > Indeed - many devices do not need to define how they are powered up. > > > > But Krzysztof stated that supplies should be required by the bindings on > > my recent posting for a VCM driver: > > > > - https://lore.kernel.org/all/6e163f4d-061d-3c20-4c2e-44c74d529f10@linaro.org/ > > > > So based on that I have made these 'required'. > > I guess it's good to align bindings regarding this, in practice the driver > will need to work without regulators (or with dummies), too. > > > > > Even in my case here, with a camera module that is compatible with the > > Raspberry Pi camera connector - there isn't really 3 supplies. It's just > > a single gpio enable pin to bring this device up for me. Of course > > that's specific to the module not the sensor. > > How do you declare that in DT? One of the regulators will be a GPIO one? I have the following as an imx335.dtsi which I include. It /should/ be an overlay / dtbo - but the current bootloader on the baord I have doesn't support applying overlays - so I just include it directly for now. ``` / { /* 24 MHz Crystal on the camera module */ imx335_inclk_1: imx335_inclk_24m { compatible = "fixed-clock"; #clock-cells = <0>; status = "okay"; clock-frequency = <24000000>; }; reg_imx335_1_3v3: regulator-imx335_1-vdd3v3 { compatible = "regulator-fixed"; pinctrl-names = "default"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; regulator-name = "IMX335_1_POWER_EN"; gpio = <&expander0 4 GPIO_ACTIVE_HIGH>; vin-supply = <®_csi2_3v3>; startup-delay-us = <300000>; enable-active-high; }; }; &i2c3 { imx335_0: sensor@1a { compatible = "sony,imx335"; reg = <0x1a>; clocks = <&imx335_inclk_1>; clock-names = "xclk"; rotation = <180>; orientation = <0>; status = "okay"; /* The IMX335 module uses *only* the 3v3 line */ avdd-supply = <®_imx335_1_3v3>; ovdd-supply = <®_imx335_1_3v3>; dvdd-supply = <®_imx335_1_3v3>; port { sensor_1_out: endpoint { remote-endpoint = <&mipi_csi_1_in>; clock-lanes = <0>; data-lanes = <1 2 3 4>; link-frequencies = /bits/ 64 <594000000>; }; }; }; }; &mipi_csi_1 { status = "okay"; ports { port@0 { mipi_csi_1_in: endpoint { remote-endpoint = <&sensor_1_out>; clock-lanes = <0>; data-lanes = <1 2 3 4>; }; }; }; }; ``` We could argue that the reg_imx335_1_3v3, should be 3 separate regulators each targetting vin-supply = <®_csi2_3v3>; But they are all wired up to the same enable pin, and I think they would then fail to probe if they all tried to control that gpio - while a regulator-fixed can be shared and handles this for us. The gpio at: ®_imx335_1_3v3 { gpio = <&expander0 4 GPIO_ACTIVE_HIGH>; }; connects to the enable line of all three regulators on the camera module. In fact - looking at the schematics of the camera module - they all power up at 'the same time'. There are no hardware delays introduced on this module, so that might answer the regulator-bulk question on the driver. -- Kieran > > -- > Regards, > > Sakari Ailus
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@ideasonboard.com> > > > --- > > > .../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@linaro.org/ > > 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 > > >
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 - 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>;
Add the bindings for the supply references used on the IMX335. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- .../bindings/media/i2c/sony,imx335.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)