| Message ID | 20231010005126.3425444-1-kieran.bingham@ideasonboard.com |
|---|---|
| Headers | show |
| Series | media: Sony IMX335 improvements | expand |
Hi Kieran, Thank you for the patch. On 10/10/23 6:21 AM, Kieran Bingham wrote: > The existing imx335_parse_hw_config function has two paths > that can be taken without reporting to the user the reason > for failing to accept the hardware configuration. > > Extend the error reporting paths to identify failures when > probing the device. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > drivers/media/i2c/imx335.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c > index 1a34b2a43718..753e5c39e0fa 100644 > --- a/drivers/media/i2c/imx335.c > +++ b/drivers/media/i2c/imx335.c > @@ -864,8 +864,10 @@ static int imx335_parse_hw_config(struct imx335 *imx335) > } > > ep = fwnode_graph_get_next_endpoint(fwnode, NULL); > - if (!ep) > + if (!ep) { > + dev_err(imx335->dev, "Failed to get next endpoint"); missing '\n' at the end. > return -ENXIO; > + } > > ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg); > fwnode_handle_put(ep); > @@ -890,6 +892,8 @@ static int imx335_parse_hw_config(struct imx335 *imx335) > if (bus_cfg.link_frequencies[i] == IMX335_LINK_FREQ) > goto done_endpoint_free; > > + dev_err(imx335->dev, "no compatible link frequencies found"); Ditto. Other than that, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > + > ret = -EINVAL; > > done_endpoint_free:
Hi Kieran On 10/10/23 6:21 AM, Kieran Bingham wrote: > Support reporting of the Sensor Native and Active pixel array areas > through the Selection API. > > The implementation reports a single target crop only for the mode that > is presently exposed by the driver. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> LGTM, Reviewed-by: Umang Jain <umang.jain@ideasonboard.com> > --- > drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c > index bf12b9b69fce..026777eb362e 100644 > --- a/drivers/media/i2c/imx335.c > +++ b/drivers/media/i2c/imx335.c > @@ -55,6 +55,14 @@ > #define IMX335_REG_MIN 0x00 > #define IMX335_REG_MAX 0xfffff > > +/* IMX335 native and active pixel array size. */ > +#define IMX335_NATIVE_WIDTH 2616U > +#define IMX335_NATIVE_HEIGHT 1964U > +#define IMX335_PIXEL_ARRAY_LEFT 12U > +#define IMX335_PIXEL_ARRAY_TOP 12U > +#define IMX335_PIXEL_ARRAY_WIDTH 2592U > +#define IMX335_PIXEL_ARRAY_HEIGHT 1944U > + > /** > * struct imx335_reg - imx335 sensor register > * @address: Register address > @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd, > return imx335_set_pad_format(sd, sd_state, &fmt); > } > > +/** > + * imx335_get_selection() - Selection API > + * @sd: pointer to imx335 V4L2 sub-device structure > + * @sd_state: V4L2 sub-device configuration > + * @sel: V4L2 selection info > + * > + * Return: 0 if successful, error code otherwise. > + */ > +static int imx335_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_selection *sel) > +{ > + switch (sel->target) { > + case V4L2_SEL_TGT_NATIVE_SIZE: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = IMX335_NATIVE_WIDTH; > + sel->r.height = IMX335_NATIVE_HEIGHT; > + > + return 0; > + > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = IMX335_PIXEL_ARRAY_TOP; > + sel->r.left = IMX335_PIXEL_ARRAY_LEFT; > + sel->r.width = IMX335_PIXEL_ARRAY_WIDTH; > + sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT; > + > + return 0; > + } > + > + return -EINVAL; > +} > + > /** > * imx335_start_streaming() - Start sensor stream > * @imx335: pointer to imx335 device > @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = { > .init_cfg = imx335_init_pad_cfg, > .enum_mbus_code = imx335_enum_mbus_code, > .enum_frame_size = imx335_enum_frame_size, > + .get_selection = imx335_get_selection, > .get_fmt = imx335_get_pad_format, > .set_fmt = imx335_set_pad_format, > };
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 Kieran, On Tue, Oct 10, 2023 at 01:51:24AM +0100, Kieran Bingham wrote: > Support reporting of the Sensor Native and Active pixel array areas > through the Selection API. > > The implementation reports a single target crop only for the mode that > is presently exposed by the driver. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Shouldn't you use the same callback for .set_selection? I guess this is somewhat grey area but doing so would be in line with how V4L2 API works in general. Cc Laurent. > --- > drivers/media/i2c/imx335.c | 44 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c > index bf12b9b69fce..026777eb362e 100644 > --- a/drivers/media/i2c/imx335.c > +++ b/drivers/media/i2c/imx335.c > @@ -55,6 +55,14 @@ > #define IMX335_REG_MIN 0x00 > #define IMX335_REG_MAX 0xfffff > > +/* IMX335 native and active pixel array size. */ > +#define IMX335_NATIVE_WIDTH 2616U > +#define IMX335_NATIVE_HEIGHT 1964U > +#define IMX335_PIXEL_ARRAY_LEFT 12U > +#define IMX335_PIXEL_ARRAY_TOP 12U > +#define IMX335_PIXEL_ARRAY_WIDTH 2592U > +#define IMX335_PIXEL_ARRAY_HEIGHT 1944U > + > /** > * struct imx335_reg - imx335 sensor register > * @address: Register address > @@ -651,6 +659,41 @@ static int imx335_init_pad_cfg(struct v4l2_subdev *sd, > return imx335_set_pad_format(sd, sd_state, &fmt); > } > > +/** > + * imx335_get_selection() - Selection API > + * @sd: pointer to imx335 V4L2 sub-device structure > + * @sd_state: V4L2 sub-device configuration > + * @sel: V4L2 selection info > + * > + * Return: 0 if successful, error code otherwise. > + */ > +static int imx335_get_selection(struct v4l2_subdev *sd, > + struct v4l2_subdev_state *sd_state, > + struct v4l2_subdev_selection *sel) > +{ > + switch (sel->target) { > + case V4L2_SEL_TGT_NATIVE_SIZE: > + sel->r.top = 0; > + sel->r.left = 0; > + sel->r.width = IMX335_NATIVE_WIDTH; > + sel->r.height = IMX335_NATIVE_HEIGHT; > + > + return 0; > + > + case V4L2_SEL_TGT_CROP: > + case V4L2_SEL_TGT_CROP_DEFAULT: > + case V4L2_SEL_TGT_CROP_BOUNDS: > + sel->r.top = IMX335_PIXEL_ARRAY_TOP; > + sel->r.left = IMX335_PIXEL_ARRAY_LEFT; > + sel->r.width = IMX335_PIXEL_ARRAY_WIDTH; > + sel->r.height = IMX335_PIXEL_ARRAY_HEIGHT; > + > + return 0; > + } > + > + return -EINVAL; > +} > + > /** > * imx335_start_streaming() - Start sensor stream > * @imx335: pointer to imx335 device > @@ -864,6 +907,7 @@ static const struct v4l2_subdev_pad_ops imx335_pad_ops = { > .init_cfg = imx335_init_pad_cfg, > .enum_mbus_code = imx335_enum_mbus_code, > .enum_frame_size = imx335_enum_frame_size, > + .get_selection = imx335_get_selection, > .get_fmt = imx335_get_pad_format, > .set_fmt = imx335_set_pad_format, > };
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
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. > - 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 >
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 > > >