| Message ID | 20210330142023.141-1-martinax.krasteva@linux.intel.com |
|---|---|
| Headers | show |
| Series | Camera Sensor Drivers | expand |
On Tue, 30 Mar 2021 15:20:18 +0100, Martina Krasteva wrote: > From: Martina Krasteva <martinax.krasteva@intel.com> > > - Add dt-bindings documentation for Sony imx335 sensor driver > - Add MAINTAINERS entry for Sony imx335 binding documentation > > Signed-off-by: Martina Krasteva <martinax.krasteva@intel.com> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > Acked-by: Paul J. Murphy <paul.j.murphy@intel.com> > --- > .../devicetree/bindings/media/i2c/sony,imx335.yaml | 90 ++++++++++++++++++++++ > MAINTAINERS | 8 ++ > 2 files changed, 98 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > Reviewed-by: Rob Herring <robh@kernel.org>
On Tue, Mar 30, 2021 at 03:20:18PM +0100, Martina Krasteva wrote: > From: Martina Krasteva <martinax.krasteva@intel.com> > > - Add dt-bindings documentation for Sony imx335 sensor driver > - Add MAINTAINERS entry for Sony imx335 binding documentation > > Signed-off-by: Martina Krasteva <martinax.krasteva@intel.com> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > Acked-by: Paul J. Murphy <paul.j.murphy@intel.com> > --- > .../devicetree/bindings/media/i2c/sony,imx335.yaml | 90 ++++++++++++++++++++++ > MAINTAINERS | 8 ++ > 2 files changed, 98 insertions(+) > create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > diff --git a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > new file mode 100644 > index 000000000000..0e286226ad9b > --- /dev/null > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > @@ -0,0 +1,90 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +# Copyright (C) 2021 Intel Corporation > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/media/i2c/sony,imx335.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Sony IMX335 Sensor > + > +maintainers: > + - Paul J. Murphy <paul.j.murphy@intel.com> > + - Daniele Alessandrelli <daniele.alessandrelli@intel.com> > + > +description: > + IMX335 sensor is a Sony CMOS active pixel digital image sensor with an active > + array size of 2592H x 1944V. It is programmable through I2C interface. The > + I2C client address is fixed to 0x1a as per sensor data sheet. Image data is > + sent through MIPI CSI-2. > + > +properties: > + compatible: > + const: sony,imx335 > + reg: > + 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 > + > + reset-gpios: > + description: Reference to the GPIO connected to the XCLR pin, if any. Missed one thing. You need 'maxItems: 1' to define how many gpios. > + > + port: > + additionalProperties: false > + $ref: /schemas/graph.yaml#/properties/port > + > + properties: > + endpoint: > + $ref: /schemas/media/video-interfaces.yaml# > + unevaluatedProperties: false > + > + properties: > + data-lanes: true > + link-frequencies: true > + > + required: > + - data-lanes > + - link-frequencies > + > + required: > + - endpoint > + > +required: > + - compatible > + - reg > + - clocks > + - port > + > +additionalProperties: false > + > +examples: > + - | > + i2c0 { > + #address-cells = <1>; > + #size-cells = <0>; > + > + camera@1a { > + compatible = "sony,imx335"; > + reg = <0x1a>; > + clocks = <&imx335_clk>; > + > + assigned-clocks = <&imx335_clk>; > + assigned-clock-parents = <&imx335_clk_parent>; > + assigned-clock-rates = <24000000>; > + > + port { > + imx335: endpoint { > + remote-endpoint = <&cam>; > + data-lanes = <1 2 3 4>; > + link-frequencies = /bits/ 64 <594000000>; > + }; > + }; > + }; > + }; > +... > diff --git a/MAINTAINERS b/MAINTAINERS > index cd988b258fe0..510e3c4a45b6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16875,6 +16875,14 @@ T: git git://linuxtv.org/media_tree.git > F: Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml > F: drivers/media/i2c/imx334.c > > +SONY IMX335 SENSOR DRIVER > +M: Paul J. Murphy <paul.j.murphy@intel.com> > +M: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > +L: linux-media@vger.kernel.org > +S: Maintained > +T: git git://linuxtv.org/media_tree.git > +F: Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > + > SONY IMX355 SENSOR DRIVER > M: Tianshu Qiu <tian.shu.qiu@intel.com> > L: linux-media@vger.kernel.org > -- > 2.11.0 >
Hi Rob, Thank you for the review > > On Tue, Mar 30, 2021 at 03:20:18PM +0100, Martina Krasteva wrote: > > From: Martina Krasteva <martinax.krasteva@intel.com> > > > > - Add dt-bindings documentation for Sony imx335 sensor driver > > - Add MAINTAINERS entry for Sony imx335 binding documentation > > > > Signed-off-by: Martina Krasteva <martinax.krasteva@intel.com> > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > Acked-by: Paul J. Murphy <paul.j.murphy@intel.com> > > --- > > .../devicetree/bindings/media/i2c/sony,imx335.yaml | 90 > ++++++++++++++++++++++ > > MAINTAINERS | 8 ++ > > 2 files changed, 98 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > new file mode 100644 > > index 000000000000..0e286226ad9b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > @@ -0,0 +1,90 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) # Copyright > > +(C) 2021 Intel Corporation %YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/media/i2c/sony,imx335.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Sony IMX335 Sensor > > + > > +maintainers: > > + - Paul J. Murphy <paul.j.murphy@intel.com> > > + - Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > + > > +description: > > + IMX335 sensor is a Sony CMOS active pixel digital image sensor with > > +an active > > + array size of 2592H x 1944V. It is programmable through I2C > > +interface. The > > + I2C client address is fixed to 0x1a as per sensor data sheet. Image > > +data is > > + sent through MIPI CSI-2. > > + > > +properties: > > + compatible: > > + const: sony,imx335 > > + reg: > > + 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 > > + > > + reset-gpios: > > + description: Reference to the GPIO connected to the XCLR pin, if any. > > Missed one thing. You need 'maxItems: 1' to define how many gpios. > Will be added in next version :) > > + > > + port: > > + additionalProperties: false > > + $ref: /schemas/graph.yaml#/properties/port > > + > > + properties: > > + endpoint: > > + $ref: /schemas/media/video-interfaces.yaml# > > + unevaluatedProperties: false > > + > > + properties: > > + data-lanes: true > > + link-frequencies: true > > + > > + required: > > + - data-lanes > > + - link-frequencies > > + > > + required: > > + - endpoint > > + > > +required: > > + - compatible > > + - reg > > + - clocks > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + i2c0 { > > + #address-cells = <1>; > > + #size-cells = <0>; > > + > > + camera@1a { > > + compatible = "sony,imx335"; > > + reg = <0x1a>; > > + clocks = <&imx335_clk>; > > + > > + assigned-clocks = <&imx335_clk>; > > + assigned-clock-parents = <&imx335_clk_parent>; > > + assigned-clock-rates = <24000000>; > > + > > + port { > > + imx335: endpoint { > > + remote-endpoint = <&cam>; > > + data-lanes = <1 2 3 4>; > > + link-frequencies = /bits/ 64 <594000000>; > > + }; > > + }; > > + }; > > + }; > > +... > > diff --git a/MAINTAINERS b/MAINTAINERS index > > cd988b258fe0..510e3c4a45b6 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16875,6 +16875,14 @@ T: git git://linuxtv.org/media_tree.git > > F: Documentation/devicetree/bindings/media/i2c/sony,imx334.yaml > > F: drivers/media/i2c/imx334.c > > > > +SONY IMX335 SENSOR DRIVER > > +M: Paul J. Murphy <paul.j.murphy@intel.com> > > +M: Daniele Alessandrelli <daniele.alessandrelli@intel.com> > > +L: linux-media@vger.kernel.org > > +S: Maintained > > +T: git git://linuxtv.org/media_tree.git > > +F: Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml > > + > > SONY IMX355 SENSOR DRIVER > > M: Tianshu Qiu <tian.shu.qiu@intel.com> > > L: linux-media@vger.kernel.org > > -- > > 2.11.0 > > Best Regards, Martina
Hi Martina, Thanks for the a of new drivers. Also my apologies for reviewing them so late. On Tue, Mar 30, 2021 at 03:20:19PM +0100, Martina Krasteva wrote: ... > +static int imx335_probe(struct i2c_client *client) > +{ > + struct imx335 *imx335; > + int ret; > + > + imx335 = devm_kzalloc(&client->dev, sizeof(*imx335), GFP_KERNEL); > + if (!imx335) > + return -ENOMEM; > + > + imx335->dev = &client->dev; > + > + /* Initialize subdev */ > + v4l2_i2c_subdev_init(&imx335->sd, client, &imx335_subdev_ops); > + > + ret = imx335_parse_hw_config(imx335); > + if (ret) { > + dev_err(imx335->dev, "HW configuration is not supported"); > + return ret; > + } > + > + mutex_init(&imx335->mutex); > + > + ret = imx335_power_on(imx335->dev); > + if (ret) { > + dev_err(imx335->dev, "failed to power-on the sensor"); > + goto error_mutex_destroy; > + } > + > + /* Check module identity */ > + ret = imx335_detect(imx335); > + if (ret) { > + dev_err(imx335->dev, "failed to find sensor: %d", ret); > + goto error_power_off; > + } > + > + /* Set default mode to max resolution */ > + imx335->cur_mode = &supported_mode; > + imx335->vblank = imx335->cur_mode->vblank; > + > + ret = imx335_init_controls(imx335); > + if (ret) { > + dev_err(imx335->dev, "failed to init controls: %d", ret); > + goto error_power_off; > + } > + > + /* Initialize subdev */ > + imx335->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > + imx335->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > + > + /* Initialize source pad */ > + imx335->pad.flags = MEDIA_PAD_FL_SOURCE; > + ret = media_entity_pads_init(&imx335->sd.entity, 1, &imx335->pad); > + if (ret) { > + dev_err(imx335->dev, "failed to init entity pads: %d", ret); > + goto error_handler_free; > + } > + > + ret = v4l2_async_register_subdev_sensor_common(&imx335->sd); > + if (ret < 0) { > + dev_err(imx335->dev, > + "failed to register async subdev: %d", ret); > + goto error_media_entity; > + } > + > + pm_runtime_set_active(imx335->dev); > + pm_runtime_enable(imx335->dev); > + pm_runtime_idle(imx335->dev); > + > + return 0; > + > +error_media_entity: > + media_entity_cleanup(&imx335->sd.entity); > +error_handler_free: > + v4l2_ctrl_handler_free(imx335->sd.ctrl_handler); > +error_power_off: > + imx335_power_off(imx335->dev); > +error_mutex_destroy: > + mutex_destroy(&imx335->mutex); > + > + return ret; > +} > + > +/** > + * imx335_remove() - I2C client device unbinding > + * @client: pointer to I2C client device > + * > + * Return: 0 if successful, error code otherwise. > + */ > +static int imx335_remove(struct i2c_client *client) > +{ > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > + struct imx335 *imx335 = to_imx335(sd); > + > + v4l2_async_unregister_subdev(sd); > + media_entity_cleanup(&sd->entity); > + v4l2_ctrl_handler_free(sd->ctrl_handler); > + > + pm_runtime_disable(&client->dev); > + pm_runtime_suspended(&client->dev); The sensor will be powered off here only if runtime PM is enabled. Could you use pm_runtime_status_suspended() to check whether the device is still powered on, as e.g. the CCS driver (drivers/media/i2c/ccs/ccs-core.c) does? I think I'll merge these when this and Rob's comments have been addressed. > + > + mutex_destroy(&imx335->mutex); > + > + return 0; > +} > + > +static const struct dev_pm_ops imx335_pm_ops = { > + SET_RUNTIME_PM_OPS(imx335_power_off, imx335_power_on, NULL) > +}; > + > +static const struct of_device_id imx335_of_match[] = { > + { .compatible = "sony,imx335" }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(of, imx335_of_match); > + > +static struct i2c_driver imx335_driver = { > + .probe_new = imx335_probe, > + .remove = imx335_remove, > + .driver = { > + .name = "imx335", > + .pm = &imx335_pm_ops, > + .of_match_table = imx335_of_match, > + }, > +}; > + > +module_i2c_driver(imx335_driver); > + > +MODULE_DESCRIPTION("Sony imx335 sensor driver"); > +MODULE_LICENSE("GPL");
Hi Sakari, Thank you for your review. I will address both your and Rob's comments. I have a question regarding the switch from pm_runtime_get_sync() to pm_runtime_resume_and_get() In my understanding get_sync() is fine to use in case the error handling is correct, but for convenience resume_and_get() is recommended. So should I do this change in my drivers as well? Best Regards, Martina > > Hi Martina, > > Thanks for the a of new drivers. Also my apologies for reviewing them so > late. > > On Tue, Mar 30, 2021 at 03:20:19PM +0100, Martina Krasteva wrote: > > ... > > +static int imx335_probe(struct i2c_client *client) > > +{ > > + struct imx335 *imx335; > > + int ret; > > + > > + imx335 = devm_kzalloc(&client->dev, sizeof(*imx335), GFP_KERNEL); > > + if (!imx335) > > + return -ENOMEM; > > + > > + imx335->dev = &client->dev; > > + > > + /* Initialize subdev */ > > + v4l2_i2c_subdev_init(&imx335->sd, client, &imx335_subdev_ops); > > + > > + ret = imx335_parse_hw_config(imx335); > > + if (ret) { > > + dev_err(imx335->dev, "HW configuration is not supported"); > > + return ret; > > + } > > + > > + mutex_init(&imx335->mutex); > > + > > + ret = imx335_power_on(imx335->dev); > > + if (ret) { > > + dev_err(imx335->dev, "failed to power-on the sensor"); > > + goto error_mutex_destroy; > > + } > > + > > + /* Check module identity */ > > + ret = imx335_detect(imx335); > > + if (ret) { > > + dev_err(imx335->dev, "failed to find sensor: %d", ret); > > + goto error_power_off; > > + } > > + > > + /* Set default mode to max resolution */ > > + imx335->cur_mode = &supported_mode; > > + imx335->vblank = imx335->cur_mode->vblank; > > + > > + ret = imx335_init_controls(imx335); > > + if (ret) { > > + dev_err(imx335->dev, "failed to init controls: %d", ret); > > + goto error_power_off; > > + } > > + > > + /* Initialize subdev */ > > + imx335->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE; > > + imx335->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR; > > + > > + /* Initialize source pad */ > > + imx335->pad.flags = MEDIA_PAD_FL_SOURCE; > > + ret = media_entity_pads_init(&imx335->sd.entity, 1, &imx335->pad); > > + if (ret) { > > + dev_err(imx335->dev, "failed to init entity pads: %d", ret); > > + goto error_handler_free; > > + } > > + > > + ret = v4l2_async_register_subdev_sensor_common(&imx335->sd); > > + if (ret < 0) { > > + dev_err(imx335->dev, > > + "failed to register async subdev: %d", ret); > > + goto error_media_entity; > > + } > > + > > + pm_runtime_set_active(imx335->dev); > > + pm_runtime_enable(imx335->dev); > > + pm_runtime_idle(imx335->dev); > > + > > + return 0; > > + > > +error_media_entity: > > + media_entity_cleanup(&imx335->sd.entity); > > +error_handler_free: > > + v4l2_ctrl_handler_free(imx335->sd.ctrl_handler); > > +error_power_off: > > + imx335_power_off(imx335->dev); > > +error_mutex_destroy: > > + mutex_destroy(&imx335->mutex); > > + > > + return ret; > > +} > > + > > +/** > > + * imx335_remove() - I2C client device unbinding > > + * @client: pointer to I2C client device > > + * > > + * Return: 0 if successful, error code otherwise. > > + */ > > +static int imx335_remove(struct i2c_client *client) > > +{ > > + struct v4l2_subdev *sd = i2c_get_clientdata(client); > > + struct imx335 *imx335 = to_imx335(sd); > > + > > + v4l2_async_unregister_subdev(sd); > > + media_entity_cleanup(&sd->entity); > > + v4l2_ctrl_handler_free(sd->ctrl_handler); > > + > > + pm_runtime_disable(&client->dev); > > + pm_runtime_suspended(&client->dev); > > The sensor will be powered off here only if runtime PM is enabled. > > Could you use pm_runtime_status_suspended() to check whether the device is > still powered on, as e.g. the CCS driver (drivers/media/i2c/ccs/ccs-core.c) > does? > > I think I'll merge these when this and Rob's comments have been addressed. > > > + > > + mutex_destroy(&imx335->mutex); > > + > > + return 0; > > +} > > + > > +static const struct dev_pm_ops imx335_pm_ops = { > > + SET_RUNTIME_PM_OPS(imx335_power_off, imx335_power_on, NULL) > > +}; > > + > > +static const struct of_device_id imx335_of_match[] = { > > + { .compatible = "sony,imx335" }, > > + { } > > +}; > > + > > +MODULE_DEVICE_TABLE(of, imx335_of_match); > > + > > +static struct i2c_driver imx335_driver = { > > + .probe_new = imx335_probe, > > + .remove = imx335_remove, > > + .driver = { > > + .name = "imx335", > > + .pm = &imx335_pm_ops, > > + .of_match_table = imx335_of_match, > > + }, > > +}; > > + > > +module_i2c_driver(imx335_driver); > > + > > +MODULE_DESCRIPTION("Sony imx335 sensor driver"); > > +MODULE_LICENSE("GPL"); > > -- > Kind regards, > > Sakari Ailus
From: Martina Krasteva <martinax.krasteva@intel.com> Patch series contains Sony IMX335, Sony IMX412 and OmniVision OV9282 camera sensor drivers and respective binding documentation. Martina Krasteva (6): dt-bindings: media: Add bindings for imx335 media: i2c: Add imx335 camera sensor driver dt-bindings: media: Add bindings for imx412 media: i2c: Add imx412 camera sensor driver dt-bindings: media: Add bindings for ov9282 media: i2c: Add ov9282 camera sensor driver .../devicetree/bindings/media/i2c/ovti,ov9282.yaml | 90 ++ .../devicetree/bindings/media/i2c/sony,imx335.yaml | 90 ++ .../devicetree/bindings/media/i2c/sony,imx412.yaml | 90 ++ MAINTAINERS | 27 + drivers/media/i2c/Kconfig | 42 + drivers/media/i2c/Makefile | 4 +- drivers/media/i2c/imx335.c | 1126 +++++++++++++++++ drivers/media/i2c/imx412.c | 1269 ++++++++++++++++++++ drivers/media/i2c/ov9282.c | 1134 +++++++++++++++++ 9 files changed, 3871 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx335.yaml create mode 100644 Documentation/devicetree/bindings/media/i2c/sony,imx412.yaml create mode 100644 drivers/media/i2c/imx335.c create mode 100644 drivers/media/i2c/imx412.c create mode 100644 drivers/media/i2c/ov9282.c base-commit: 9d49ed9ca93b8c564033c1d6808017bc9052b5db