[PATCH v4 0/7] OV9281 support

[PATCH v4 0/7] OV9281 support

[Alexander Stein]

Hi,

this is the 4th series adding support for OV9281 which is quite similar to OV9282.
This includes:
* a small cleanup (Patch 1)
* adding a new compatible (Patch 2 & 3)
* adding support for regulators (Patch 4 & 5)
* Fixing v4l2 subdev name depending on actual model name (Patch 6)
* Add regmap support (Patch 7)

Thanks for anyone doing review and giving a feedback.

Here are the changes in v4:
* Use platform data to set sensor name (Patch 6)
* Fixed some style issues (Patch 7)

Here are the changes in v3:
* Removed struct field documentation as well (Patch 1)
* Dropped v2 Patch 6 (wrong approach)
* Added new Patch 6 to set subdev name according to model parsed form compatible
* Added new Patch 7 adding regmap support
  This is a preparation to solve the defunct auto-increment using regmap's
  'use_single_read' (still WIP)

Here are the changes in v2:
* Added Krzysztof's a-b for Patch 2 & 4
* Added Daniele's a-b for Patch 1 & 3
* Removed additional error message in ov9282_power_off
* Renamed function from ov9282_configure_regulators to ov9282_get_regulators
* Cleaned-up reading ID registers

The regulator support is based on the driver from Raspberry Pi downstream kernel
[1], the ID register read fix as well. Please refer to [2] why this fix is
required. I can confirm this is necessary by checking with a Logic analyzer on
the i2c bus.

Best regards,
Alexander

[1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/media/i2c/ov9281.c
[2] https://github.com/raspberrypi/linux/commit/58deee7c917e1c3c5e37987c3a89ad19d791f58a


Alexander Stein (7):
  media: i2c: ov9282: remove unused and unset i2c_client member
  media: dt-bindings: media: Add compatible for ov9281
  media: i2c: ov9282: Add ov9281 compatible
  media: dt-bindings: media: ov9282: Add power supply properties
  media: i2c: ov9282: Add regulator support
  media: i2c: ov9282: Set v4l2 subdev name according to sensor model
  media: i2c: ov9282: Add regmap support

 .../bindings/media/i2c/ovti,ov9282.yaml       |  14 ++-
 drivers/media/i2c/ov9282.c                    | 108 ++++++++++++------
 2 files changed, 89 insertions(+), 33 deletions(-)

-- 
2.25.1

[PATCH v4 1/7] media: i2c: ov9282: remove unused and unset i2c_client member

[Alexander Stein]

This is not need anyway as the i2c_client is stored in v4l2_subdev.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
---
 drivers/media/i2c/ov9282.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 2e0b315801e5..8a252bf3b59f 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -104,7 +104,6 @@ struct ov9282_mode {
 /**
  * struct ov9282 - ov9282 sensor device structure
  * @dev: Pointer to generic device
- * @client: Pointer to i2c client
  * @sd: V4L2 sub-device
  * @pad: Media pad. Only one pad supported
  * @reset_gpio: Sensor reset gpio
@@ -123,7 +122,6 @@ struct ov9282_mode {
  */
 struct ov9282 {
 	struct device *dev;
-	struct i2c_client *client;
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
-- 
2.25.1

[PATCH v4 2/7] media: dt-bindings: media: Add compatible for ov9281

[Alexander Stein]

This is a slightly different hardware with identical software interface.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
index bf115ab9d926..285f8c85f253 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
@@ -16,10 +16,13 @@ description:
   sensor with an active array size of 1296H x 816V. It is programmable through
   I2C interface. The I2C client address is fixed to 0x60/0x70 as per sensor data
   sheet. Image data is sent through MIPI CSI-2.
+  OV9281 has a different lens chief ray angle.
 
 properties:
   compatible:
-    const: ovti,ov9282
+    enum:
+      - ovti,ov9281
+      - ovti,ov9282
   reg:
     description: I2C address
     maxItems: 1
-- 
2.25.1

[PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Alexander Stein]

According to product brief they are identical from software point of view.
Differences are a different chief ray angle (CRA) and the package.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
---
 drivers/media/i2c/ov9282.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 8a252bf3b59f..c8d83a29f9bb 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
 };
 
 static const struct of_device_id ov9282_of_match[] = {
+	{ .compatible = "ovti,ov9281" },
 	{ .compatible = "ovti,ov9282" },
 	{ }
 };
-- 
2.25.1

[PATCH v4 4/7] media: dt-bindings: media: ov9282: Add power supply properties

[Alexander Stein]

Add regulators for each power domain.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/media/i2c/ovti,ov9282.yaml       | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
index 285f8c85f253..9abfaabd373a 100644
--- a/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
+++ b/Documentation/devicetree/bindings/media/i2c/ovti,ov9282.yaml
@@ -39,6 +39,15 @@ properties:
     description: Reference to the GPIO connected to the XCLR pin, if any.
     maxItems: 1
 
+  avdd-supply:
+    description: Analog power supply
+
+  dovdd-supply:
+    description: Digital I/O power supply
+
+  dvdd-supply:
+    description: Digital core supply
+
   port:
     additionalProperties: false
     $ref: /schemas/graph.yaml#/$defs/port-base
-- 
2.25.1

[PATCH v4 5/7] media: i2c: ov9282: Add regulator support

[Alexander Stein]

Need in case the sensors is supplied by a switchable regulator.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/media/i2c/ov9282.c | 39 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index c8d83a29f9bb..352dbe21a902 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
@@ -55,6 +56,14 @@
 #define OV9282_REG_MIN		0x00
 #define OV9282_REG_MAX		0xfffff
 
+static const char * const ov9282_supply_names[] = {
+	"avdd",		/* Analog power */
+	"dovdd",	/* Digital I/O power */
+	"dvdd",		/* Digital core power */
+};
+
+#define OV9282_NUM_SUPPLIES ARRAY_SIZE(ov9282_supply_names)
+
 /**
  * struct ov9282_reg - ov9282 sensor register
  * @address: Register address
@@ -126,6 +135,7 @@ struct ov9282 {
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
 	struct clk *inclk;
+	struct regulator_bulk_data supplies[OV9282_NUM_SUPPLIES];
 	struct v4l2_ctrl_handler ctrl_handler;
 	struct v4l2_ctrl *link_freq_ctrl;
 	struct v4l2_ctrl *pclk_ctrl;
@@ -882,10 +892,19 @@ static int ov9282_power_on(struct device *dev)
 		goto error_reset;
 	}
 
+	ret = regulator_bulk_enable(ARRAY_SIZE(ov9282->supplies),
+				    ov9282->supplies);
+	if (ret) {
+		dev_err(dev, "Failed to enable regulators\n");
+		goto disable_clk;
+	}
+
 	usleep_range(400, 600);
 
 	return 0;
 
+disable_clk:
+	clk_disable_unprepare(ov9282->inclk);
 error_reset:
 	gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
 
@@ -903,6 +922,8 @@ static int ov9282_power_off(struct device *dev)
 	struct v4l2_subdev *sd = dev_get_drvdata(dev);
 	struct ov9282 *ov9282 = to_ov9282(sd);
 
+	regulator_bulk_disable(ARRAY_SIZE(ov9282->supplies), ov9282->supplies);
+
 	gpiod_set_value_cansleep(ov9282->reset_gpio, 0);
 
 	clk_disable_unprepare(ov9282->inclk);
@@ -995,6 +1016,18 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
 	return 0;
 }
 
+static int ov9282_get_regulators(struct ov9282 *ov9282)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(ov9282->supplies); i++)
+		ov9282->supplies[i].supply = ov9282_supply_names[i];
+
+	return devm_regulator_bulk_get(ov9282->dev,
+				       ARRAY_SIZE(ov9282->supplies),
+				       ov9282->supplies);
+}
+
 /**
  * ov9282_probe() - I2C client device binding
  * @client: pointer to i2c client device
@@ -1021,6 +1054,12 @@ static int ov9282_probe(struct i2c_client *client)
 		return ret;
 	}
 
+	ret = ov9282_get_regulators(ov9282);
+	if (ret) {
+		dev_err(&client->dev, "Failed to get power regulators\n");
+		return ret;
+	}
+
 	mutex_init(&ov9282->mutex);
 
 	ret = ov9282_power_on(ov9282->dev);
-- 
2.25.1

[PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model

[Alexander Stein]

To distinguish ov9281 & ov9282 the name has to be explicitly set.
Provide a fixed string using platform data.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v4:
* Replaced v4l2_i2c_subdev_set_name with device_get_match_data and added
  platform data containing the sensor name

 drivers/media/i2c/ov9282.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index 352dbe21a902..f79bdfa821e8 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1037,6 +1037,7 @@ static int ov9282_get_regulators(struct ov9282 *ov9282)
 static int ov9282_probe(struct i2c_client *client)
 {
 	struct ov9282 *ov9282;
+	const char *sensor_name;
 	int ret;
 
 	ov9282 = devm_kzalloc(&client->dev, sizeof(*ov9282), GFP_KERNEL);
@@ -1047,6 +1048,12 @@ static int ov9282_probe(struct i2c_client *client)
 
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
+	sensor_name = device_get_match_data(ov9282->dev);
+	if (!sensor_name) {
+		dev_err(ov9282->dev, "Sensor name is missing");
+		return ret;
+	}
+	v4l2_i2c_subdev_set_name(&ov9282->sd, client, sensor_name, NULL);
 
 	ret = ov9282_parse_hw_config(ov9282);
 	if (ret) {
@@ -1152,8 +1159,8 @@ static const struct dev_pm_ops ov9282_pm_ops = {
 };
 
 static const struct of_device_id ov9282_of_match[] = {
-	{ .compatible = "ovti,ov9281" },
-	{ .compatible = "ovti,ov9282" },
+	{ .compatible = "ovti,ov9281", .data = "ov9281" },
+	{ .compatible = "ovti,ov9282", .data = "ov9282" },
 	{ }
 };
 
-- 
2.25.1

[PATCH v4 7/7] media: i2c: ov9282: Add regmap support

[Alexander Stein]

Register values apparently are stored in MSB format, hence the manual
swap when reading/writing.
regmap's endianness swapping is not applied when reading with val_bits = 8.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in v4:
* Removed superfluous line break
* Shortened variable initialization

 drivers/media/i2c/ov9282.c | 57 +++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index f79bdfa821e8..e9e595df187b 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -10,6 +10,7 @@
 #include <linux/delay.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 
@@ -113,6 +114,7 @@ struct ov9282_mode {
 /**
  * struct ov9282 - ov9282 sensor device structure
  * @dev: Pointer to generic device
+ * @regmap: sensor's regmap
  * @sd: V4L2 sub-device
  * @pad: Media pad. Only one pad supported
  * @reset_gpio: Sensor reset gpio
@@ -131,6 +133,7 @@ struct ov9282_mode {
  */
 struct ov9282 {
 	struct device *dev;
+	struct regmap *regmap;
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 	struct gpio_desc *reset_gpio;
@@ -151,6 +154,11 @@ struct ov9282 {
 	bool streaming;
 };
 
+static const struct regmap_config ov9282_regmap_config = {
+	.reg_bits = 16,
+	.val_bits = 8,
+};
+
 static const s64 link_freq[] = {
 	OV9282_LINK_FREQ,
 };
@@ -297,35 +305,20 @@ static inline struct ov9282 *to_ov9282(struct v4l2_subdev *subdev)
  */
 static int ov9282_read_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 *val)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd);
-	struct i2c_msg msgs[2] = {0};
-	u8 addr_buf[2] = {0};
-	u8 data_buf[4] = {0};
+	u8 data[4] = {0};
 	int ret;
 
 	if (WARN_ON(len > 4))
 		return -EINVAL;
 
-	put_unaligned_be16(reg, addr_buf);
-
-	/* Write register address */
-	msgs[0].addr = client->addr;
-	msgs[0].flags = 0;
-	msgs[0].len = ARRAY_SIZE(addr_buf);
-	msgs[0].buf = addr_buf;
-
-	/* Read data from register */
-	msgs[1].addr = client->addr;
-	msgs[1].flags = I2C_M_RD;
-	msgs[1].len = len;
-	msgs[1].buf = &data_buf[4 - len];
-
-	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
-	if (ret != ARRAY_SIZE(msgs))
-		return -EIO;
-
-	*val = get_unaligned_be32(data_buf);
+	ret = regmap_raw_read(ov9282->regmap, reg, &data[4 - len], len);
+	if (ret < 0) {
+		dev_err(ov9282->dev, "read from 0x%04x failed: %d\n",
+			reg, ret);
+		return ret;
+	}
 
+	*val = get_unaligned_be32(data);
 	return 0;
 }
 
@@ -340,16 +333,17 @@ static int ov9282_read_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 *val)
  */
 static int ov9282_write_reg(struct ov9282 *ov9282, u16 reg, u32 len, u32 val)
 {
-	struct i2c_client *client = v4l2_get_subdevdata(&ov9282->sd);
-	u8 buf[6] = {0};
+	u8 data[4] = {0};
+	int ret;
 
 	if (WARN_ON(len > 4))
 		return -EINVAL;
 
-	put_unaligned_be16(reg, buf);
-	put_unaligned_be32(val << (8 * (4 - len)), buf + 2);
-	if (i2c_master_send(client, buf, len + 2) != len + 2)
-		return -EIO;
+	put_unaligned_be32(val << (8 * (4 - len)), data);
+
+	ret = regmap_raw_write(ov9282->regmap, reg, data, len);
+	if (ret < 0)
+		dev_err(ov9282->dev, "write to 0x%04x failed: %d\n", reg, ret);
 
 	return 0;
 }
@@ -1045,6 +1039,11 @@ static int ov9282_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	ov9282->dev = &client->dev;
+	ov9282->regmap = devm_regmap_init_i2c(client, &ov9282_regmap_config);
+	if (IS_ERR(ov9282->regmap)) {
+		dev_err(ov9282->dev, "Unable to initialize I2C\n");
+		return -ENODEV;
+	}
 
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
-- 
2.25.1

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Krzysztof Kozlowski]

On 28/07/2022 15:02, Alexander Stein wrote:
> According to product brief they are identical from software point of view.
> Differences are a different chief ray angle (CRA) and the package.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> ---
>  drivers/media/i2c/ov9282.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index 8a252bf3b59f..c8d83a29f9bb 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>  };
>  
>  static const struct of_device_id ov9282_of_match[] = {
> +	{ .compatible = "ovti,ov9281" },

The devices seem entirely compatible, so why you add a new compatible
and not re-use existing?

The difference in lens does not explain this.


Best regards,
Krzysztof

Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model

[kernel test robot]

Hi Alexander,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on media-tree/master]
[also build test WARNING on linus/master v5.19-rc8 next-20220728]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/OV9281-support/20220728-210448
base:   git://linuxtv.org/media_tree.git master
config: arm-randconfig-r022-20220728 (https://download.01.org/0day-ci/archive/20220729/202207290518.1D7MVS65-lkp@intel.com/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 8dfaecc4c24494337933aff9d9166486ca0949f1)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/intel-lab-lkp/linux/commit/ee28006553d4d23f600b0076ef6066710519f156
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Alexander-Stein/OV9281-support/20220728-210448
        git checkout ee28006553d4d23f600b0076ef6066710519f156
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/i2c/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/media/i2c/ov9282.c:1054:10: warning: variable 'ret' is uninitialized when used here [-Wuninitialized]
                   return ret;
                          ^~~
   drivers/media/i2c/ov9282.c:1041:9: note: initialize the variable 'ret' to silence this warning
           int ret;
                  ^
                   = 0
   1 warning generated.


vim +/ret +1054 drivers/media/i2c/ov9282.c

  1030	
  1031	/**
  1032	 * ov9282_probe() - I2C client device binding
  1033	 * @client: pointer to i2c client device
  1034	 *
  1035	 * Return: 0 if successful, error code otherwise.
  1036	 */
  1037	static int ov9282_probe(struct i2c_client *client)
  1038	{
  1039		struct ov9282 *ov9282;
  1040		const char *sensor_name;
  1041		int ret;
  1042	
  1043		ov9282 = devm_kzalloc(&client->dev, sizeof(*ov9282), GFP_KERNEL);
  1044		if (!ov9282)
  1045			return -ENOMEM;
  1046	
  1047		ov9282->dev = &client->dev;
  1048	
  1049		/* Initialize subdev */
  1050		v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
  1051		sensor_name = device_get_match_data(ov9282->dev);
  1052		if (!sensor_name) {
  1053			dev_err(ov9282->dev, "Sensor name is missing");
> 1054			return ret;
  1055		}
  1056		v4l2_i2c_subdev_set_name(&ov9282->sd, client, sensor_name, NULL);
  1057	
  1058		ret = ov9282_parse_hw_config(ov9282);
  1059		if (ret) {
  1060			dev_err(ov9282->dev, "HW configuration is not supported");
  1061			return ret;
  1062		}
  1063	
  1064		ret = ov9282_get_regulators(ov9282);
  1065		if (ret) {
  1066			dev_err(&client->dev, "Failed to get power regulators\n");
  1067			return ret;
  1068		}
  1069	
  1070		mutex_init(&ov9282->mutex);
  1071	
  1072		ret = ov9282_power_on(ov9282->dev);
  1073		if (ret) {
  1074			dev_err(ov9282->dev, "failed to power-on the sensor");
  1075			goto error_mutex_destroy;
  1076		}
  1077	
  1078		/* Check module identity */
  1079		ret = ov9282_detect(ov9282);
  1080		if (ret) {
  1081			dev_err(ov9282->dev, "failed to find sensor: %d", ret);
  1082			goto error_power_off;
  1083		}
  1084	
  1085		/* Set default mode to max resolution */
  1086		ov9282->cur_mode = &supported_mode;
  1087		ov9282->vblank = ov9282->cur_mode->vblank;
  1088	
  1089		ret = ov9282_init_controls(ov9282);
  1090		if (ret) {
  1091			dev_err(ov9282->dev, "failed to init controls: %d", ret);
  1092			goto error_power_off;
  1093		}
  1094	
  1095		/* Initialize subdev */
  1096		ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
  1097		ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
  1098	
  1099		/* Initialize source pad */
  1100		ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
  1101		ret = media_entity_pads_init(&ov9282->sd.entity, 1, &ov9282->pad);
  1102		if (ret) {
  1103			dev_err(ov9282->dev, "failed to init entity pads: %d", ret);
  1104			goto error_handler_free;
  1105		}
  1106	
  1107		ret = v4l2_async_register_subdev_sensor(&ov9282->sd);
  1108		if (ret < 0) {
  1109			dev_err(ov9282->dev,
  1110				"failed to register async subdev: %d", ret);
  1111			goto error_media_entity;
  1112		}
  1113	
  1114		pm_runtime_set_active(ov9282->dev);
  1115		pm_runtime_enable(ov9282->dev);
  1116		pm_runtime_idle(ov9282->dev);
  1117	
  1118		return 0;
  1119	
  1120	error_media_entity:
  1121		media_entity_cleanup(&ov9282->sd.entity);
  1122	error_handler_free:
  1123		v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler);
  1124	error_power_off:
  1125		ov9282_power_off(ov9282->dev);
  1126	error_mutex_destroy:
  1127		mutex_destroy(&ov9282->mutex);
  1128	
  1129		return ret;
  1130	}
  1131	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Sakari Ailus]

Hi Krzysztof,

On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> On 28/07/2022 15:02, Alexander Stein wrote:
> > According to product brief they are identical from software point of view.
> > Differences are a different chief ray angle (CRA) and the package.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > ---
> >  drivers/media/i2c/ov9282.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 8a252bf3b59f..c8d83a29f9bb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> >  };
> >  
> >  static const struct of_device_id ov9282_of_match[] = {
> > +	{ .compatible = "ovti,ov9281" },
> 
> The devices seem entirely compatible, so why you add a new compatible
> and not re-use existing?
> 
> The difference in lens does not explain this.

It is typically necessary to know what kind of related hardware can be
found in the system, beyond just the device's register interface. Apart
from USB cameras, less integrated cameras require low-level software
control in which specific device properties are important. In this case it
could be the lens shading table, among other things.

	https://www.ovt.com/sensor/ov9282/

Therefore I think adding a specific compatible string for this one is
justified.

Also cc Laurent.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Laurent Pinchart]

Hi Sakari,

(Adding Dave and Naush to the CC list)

On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> > On 28/07/2022 15:02, Alexander Stein wrote:
> > > According to product brief they are identical from software point of view.
> > > Differences are a different chief ray angle (CRA) and the package.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> > > ---
> > >  drivers/media/i2c/ov9282.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > > index 8a252bf3b59f..c8d83a29f9bb 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> > >  };
> > >  
> > >  static const struct of_device_id ov9282_of_match[] = {
> > > +	{ .compatible = "ovti,ov9281" },
> > 
> > The devices seem entirely compatible, so why you add a new compatible
> > and not re-use existing?
> > 
> > The difference in lens does not explain this.
> 
> It is typically necessary to know what kind of related hardware can be
> found in the system, beyond just the device's register interface. Apart
> from USB cameras, less integrated cameras require low-level software
> control in which specific device properties are important. In this case it
> could be the lens shading table, among other things.
> 
> 	https://www.ovt.com/sensor/ov9282/
> 
> Therefore I think adding a specific compatible string for this one is
> justified.
> 
> Also cc Laurent.

Interesting coincidence, we've talked about this topic (as part of a
broader discussion) no later than yesterday.

I agree with Sakari in that userspace needs to know the exact model of
the camera sensor. I don't see a good alternative to providing that
information through the platform firmware, so the device tree in this
case. The question is how it should be provided (the question of how it
should then be exposed to userspace is also important, but out of scope
in this discussion).

The compatible string is meant to indicate a device's compatibility with
"something", and that something is often considered from the point of
view of software support, and in particular to pick an appropriate
kernel driver and tune its behaviour for the device. Here, one could
argue that the exact model is also needed to ensure proper software
support, but in userspace this time, not in the kernel. I think using a
dedicated compatible string would be reasonable. An alternative would be
to use another DT property, which should then be standardized. I'm not
sure it's worth it.

Broadening the discussion, we also need to know detailed information
about the camera lens (I'm talking about the lens itself here, not the
lens controller IC that controls the motor that moves the focus lens).
The lens isn't described in the device tree with a dedicated device tree
node today, and I don't think it should (I'd have a hard time coming up
with a naming scheme for lenses that we could use in compatible strings,
and the lens-related data that a system requires can possibly vary based
not only on the lens itself but on the ISP that the camera sensor is
used with). Typical useful data are the lens movement range, the
hyperfocal distance, but also the lens shading tables. (Part of) that
information is sometimes stored in non-volatile memory in the camera
module (OTP in the camera sensor itself, or a separate EEPROM), but
that's not always the case. We have considered the possibility of
storing the information in the device tree, but I doubt that would be
accepted. We can store the information in userspace in configuration
files, but we will still need to device tree to provide lens
identification information to select the correct configuration file. I
don't know how that should be done.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model

[Alexander Stein]

Am Donnerstag, 28. Juli 2022, 23:10:07 CEST schrieb kernel test robot:
> Hi Alexander,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on media-tree/master]
> [also build test WARNING on linus/master v5.19-rc8 next-20220728]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:   
> https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/OV9281-suppo
> rt/20220728-210448 base:   git://linuxtv.org/media_tree.git master
> config: arm-randconfig-r022-20220728
> (https://download.01.org/0day-ci/archive/20220729/202207290518.1D7MVS65-lkp
> @intel.com/config) compiler: clang version 15.0.0
> (https://github.com/llvm/llvm-project
> 8dfaecc4c24494337933aff9d9166486ca0949f1) reproduce (this is a W=1 build):
>         wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross chmod +x ~/bin/make.cross
>         # install arm cross compiling tool for clang build
>         # apt-get install binutils-arm-linux-gnueabi
>         #
> https://github.com/intel-lab-lkp/linux/commit/ee28006553d4d23f600b0076ef606
> 6710519f156 git remote add linux-review
> https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review
> Alexander-Stein/OV9281-support/20220728-210448 git checkout
> ee28006553d4d23f600b0076ef6066710519f156
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/i2c/
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> >> drivers/media/i2c/ov9282.c:1054:10: warning: variable 'ret' is
> >> uninitialized when used here [-Wuninitialized]
>                    return ret;
>                           ^~~
>    drivers/media/i2c/ov9282.c:1041:9: note: initialize the variable 'ret' to
> silence this warning int ret;
>                   ^
>                    = 0
>    1 warning generated.
> 
> 
> vim +/ret +1054 drivers/media/i2c/ov9282.c
> 
>   1030
>   1031	/**
>   1032	 * ov9282_probe() - I2C client device binding
>   1033	 * @client: pointer to i2c client device
>   1034	 *
>   1035	 * Return: 0 if successful, error code otherwise.
>   1036	 */
>   1037	static int ov9282_probe(struct i2c_client *client)
>   1038	{
>   1039		struct ov9282 *ov9282;
>   1040		const char *sensor_name;
>   1041		int ret;
>   1042
>   1043		ov9282 = devm_kzalloc(&client->dev, sizeof(*ov9282), 
GFP_KERNEL);
>   1044		if (!ov9282)
>   1045			return -ENOMEM;
>   1046
>   1047		ov9282->dev = &client->dev;
>   1048
>   1049		/* Initialize subdev */
>   1050		v4l2_i2c_subdev_init(&ov9282->sd, client, 
&ov9282_subdev_ops);
>   1051		sensor_name = device_get_match_data(ov9282->dev);
>   1052		if (!sensor_name) {
>   1053			dev_err(ov9282->dev, "Sensor name is 
missing");
> 
> > 1054			return ret;
> 
>   1055		}
>   1056		v4l2_i2c_subdev_set_name(&ov9282->sd, client, 
sensor_name, NULL);
>   1057
>   1058		ret = ov9282_parse_hw_config(ov9282);
>   1059		if (ret) {
>   1060			dev_err(ov9282->dev, "HW configuration is not 
supported");
>   1061			return ret;
>   1062		}
>   1063
>   1064		ret = ov9282_get_regulators(ov9282);
>   1065		if (ret) {
>   1066			dev_err(&client->dev, "Failed to get power 
regulators\n");
>   1067			return ret;
>   1068		}
>   1069
>   1070		mutex_init(&ov9282->mutex);
>   1071
>   1072		ret = ov9282_power_on(ov9282->dev);
>   1073		if (ret) {
>   1074			dev_err(ov9282->dev, "failed to power-on the 
sensor");
>   1075			goto error_mutex_destroy;
>   1076		}
>   1077
>   1078		/* Check module identity */
>   1079		ret = ov9282_detect(ov9282);
>   1080		if (ret) {
>   1081			dev_err(ov9282->dev, "failed to find sensor: 
%d", ret);
>   1082			goto error_power_off;
>   1083		}
>   1084
>   1085		/* Set default mode to max resolution */
>   1086		ov9282->cur_mode = &supported_mode;
>   1087		ov9282->vblank = ov9282->cur_mode->vblank;
>   1088
>   1089		ret = ov9282_init_controls(ov9282);
>   1090		if (ret) {
>   1091			dev_err(ov9282->dev, "failed to init 
controls: %d", ret);
>   1092			goto error_power_off;
>   1093		}
>   1094
>   1095		/* Initialize subdev */
>   1096		ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>   1097		ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>   1098
>   1099		/* Initialize source pad */
>   1100		ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
>   1101		ret = media_entity_pads_init(&ov9282->sd.entity, 1, 
&ov9282->pad);
>   1102		if (ret) {
>   1103			dev_err(ov9282->dev, "failed to init entity 
pads: %d", ret);
>   1104			goto error_handler_free;
>   1105		}
>   1106
>   1107		ret = v4l2_async_register_subdev_sensor(&ov9282->sd);
>   1108		if (ret < 0) {
>   1109			dev_err(ov9282->dev,
>   1110				"failed to register async subdev: 
%d", ret);
>   1111			goto error_media_entity;
>   1112		}
>   1113
>   1114		pm_runtime_set_active(ov9282->dev);
>   1115		pm_runtime_enable(ov9282->dev);
>   1116		pm_runtime_idle(ov9282->dev);
>   1117
>   1118		return 0;
>   1119
>   1120	error_media_entity:
>   1121		media_entity_cleanup(&ov9282->sd.entity);
>   1122	error_handler_free:
>   1123		v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler);
>   1124	error_power_off:
>   1125		ov9282_power_off(ov9282->dev);
>   1126	error_mutex_destroy:
>   1127		mutex_destroy(&ov9282->mutex);
>   1128
>   1129		return ret;
>   1130	}
>   1131

Meh, I'll come up with a fixed once discussion about the additional compatible 
has settled. This will also include the missing member documentation in patch 
5

Best regards,
Alexander

Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model

[Alexander Stein]

[-- Attachment #1: Type: text/plain, Size: 6030 bytes --]

Am Donnerstag, 28. Juli 2022, 23:10:07 CEST schrieb kernel test robot:
> Hi Alexander,
> 
> Thank you for the patch! Perhaps something to improve:
> 
> [auto build test WARNING on media-tree/master]
> [also build test WARNING on linus/master v5.19-rc8 next-20220728]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:   
> https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/OV9281-suppo
> rt/20220728-210448 base:   git://linuxtv.org/media_tree.git master
> config: arm-randconfig-r022-20220728
> (https://download.01.org/0day-ci/archive/20220729/202207290518.1D7MVS65-lkp
> @intel.com/config) compiler: clang version 15.0.0
> (https://github.com/llvm/llvm-project
> 8dfaecc4c24494337933aff9d9166486ca0949f1) reproduce (this is a W=1 build):
>         wget
> https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> ~/bin/make.cross chmod +x ~/bin/make.cross
>         # install arm cross compiling tool for clang build
>         # apt-get install binutils-arm-linux-gnueabi
>         #
> https://github.com/intel-lab-lkp/linux/commit/ee28006553d4d23f600b0076ef606
> 6710519f156 git remote add linux-review
> https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review
> Alexander-Stein/OV9281-support/20220728-210448 git checkout
> ee28006553d4d23f600b0076ef6066710519f156
>         # save the config file
>         mkdir build_dir && cp config build_dir/.config
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/i2c/
> 
> If you fix the issue, kindly add following tag where applicable
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All warnings (new ones prefixed by >>):
> >> drivers/media/i2c/ov9282.c:1054:10: warning: variable 'ret' is
> >> uninitialized when used here [-Wuninitialized]
>                    return ret;
>                           ^~~
>    drivers/media/i2c/ov9282.c:1041:9: note: initialize the variable 'ret' to
> silence this warning int ret;
>                   ^
>                    = 0
>    1 warning generated.
> 
> 
> vim +/ret +1054 drivers/media/i2c/ov9282.c
> 
>   1030
>   1031	/**
>   1032	 * ov9282_probe() - I2C client device binding
>   1033	 * @client: pointer to i2c client device
>   1034	 *
>   1035	 * Return: 0 if successful, error code otherwise.
>   1036	 */
>   1037	static int ov9282_probe(struct i2c_client *client)
>   1038	{
>   1039		struct ov9282 *ov9282;
>   1040		const char *sensor_name;
>   1041		int ret;
>   1042
>   1043		ov9282 = devm_kzalloc(&client->dev, sizeof(*ov9282), 
GFP_KERNEL);
>   1044		if (!ov9282)
>   1045			return -ENOMEM;
>   1046
>   1047		ov9282->dev = &client->dev;
>   1048
>   1049		/* Initialize subdev */
>   1050		v4l2_i2c_subdev_init(&ov9282->sd, client, 
&ov9282_subdev_ops);
>   1051		sensor_name = device_get_match_data(ov9282->dev);
>   1052		if (!sensor_name) {
>   1053			dev_err(ov9282->dev, "Sensor name is 
missing");
> 
> > 1054			return ret;
> 
>   1055		}
>   1056		v4l2_i2c_subdev_set_name(&ov9282->sd, client, 
sensor_name, NULL);
>   1057
>   1058		ret = ov9282_parse_hw_config(ov9282);
>   1059		if (ret) {
>   1060			dev_err(ov9282->dev, "HW configuration is not 
supported");
>   1061			return ret;
>   1062		}
>   1063
>   1064		ret = ov9282_get_regulators(ov9282);
>   1065		if (ret) {
>   1066			dev_err(&client->dev, "Failed to get power 
regulators\n");
>   1067			return ret;
>   1068		}
>   1069
>   1070		mutex_init(&ov9282->mutex);
>   1071
>   1072		ret = ov9282_power_on(ov9282->dev);
>   1073		if (ret) {
>   1074			dev_err(ov9282->dev, "failed to power-on the 
sensor");
>   1075			goto error_mutex_destroy;
>   1076		}
>   1077
>   1078		/* Check module identity */
>   1079		ret = ov9282_detect(ov9282);
>   1080		if (ret) {
>   1081			dev_err(ov9282->dev, "failed to find sensor: 
%d", ret);
>   1082			goto error_power_off;
>   1083		}
>   1084
>   1085		/* Set default mode to max resolution */
>   1086		ov9282->cur_mode = &supported_mode;
>   1087		ov9282->vblank = ov9282->cur_mode->vblank;
>   1088
>   1089		ret = ov9282_init_controls(ov9282);
>   1090		if (ret) {
>   1091			dev_err(ov9282->dev, "failed to init 
controls: %d", ret);
>   1092			goto error_power_off;
>   1093		}
>   1094
>   1095		/* Initialize subdev */
>   1096		ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>   1097		ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
>   1098
>   1099		/* Initialize source pad */
>   1100		ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
>   1101		ret = media_entity_pads_init(&ov9282->sd.entity, 1, 
&ov9282->pad);
>   1102		if (ret) {
>   1103			dev_err(ov9282->dev, "failed to init entity 
pads: %d", ret);
>   1104			goto error_handler_free;
>   1105		}
>   1106
>   1107		ret = v4l2_async_register_subdev_sensor(&ov9282->sd);
>   1108		if (ret < 0) {
>   1109			dev_err(ov9282->dev,
>   1110				"failed to register async subdev: 
%d", ret);
>   1111			goto error_media_entity;
>   1112		}
>   1113
>   1114		pm_runtime_set_active(ov9282->dev);
>   1115		pm_runtime_enable(ov9282->dev);
>   1116		pm_runtime_idle(ov9282->dev);
>   1117
>   1118		return 0;
>   1119
>   1120	error_media_entity:
>   1121		media_entity_cleanup(&ov9282->sd.entity);
>   1122	error_handler_free:
>   1123		v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler);
>   1124	error_power_off:
>   1125		ov9282_power_off(ov9282->dev);
>   1126	error_mutex_destroy:
>   1127		mutex_destroy(&ov9282->mutex);
>   1128
>   1129		return ret;
>   1130	}
>   1131

Meh, I'll come up with a fixed once discussion about the additional compatible 
has settled. This will also include the missing member documentation in patch 
5

Best regards,
Alexander

Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model

[Sakari Ailus]

Hi Alexander,

On Fri, Jul 29, 2022 at 10:23:48AM +0200, Alexander Stein wrote:
> Am Donnerstag, 28. Juli 2022, 23:10:07 CEST schrieb kernel test robot:
> > Hi Alexander,
> > 
> > Thank you for the patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on media-tree/master]
> > [also build test WARNING on linus/master v5.19-rc8 next-20220728]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:   
> > https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/OV9281-suppo
> > rt/20220728-210448 base:   git://linuxtv.org/media_tree.git master
> > config: arm-randconfig-r022-20220728
> > (https://download.01.org/0day-ci/archive/20220729/202207290518.1D7MVS65-lkp
> > @intel.com/config) compiler: clang version 15.0.0
> > (https://github.com/llvm/llvm-project
> > 8dfaecc4c24494337933aff9d9166486ca0949f1) reproduce (this is a W=1 build):
> >         wget
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> > ~/bin/make.cross chmod +x ~/bin/make.cross
> >         # install arm cross compiling tool for clang build
> >         # apt-get install binutils-arm-linux-gnueabi
> >         #
> > https://github.com/intel-lab-lkp/linux/commit/ee28006553d4d23f600b0076ef606
> > 6710519f156 git remote add linux-review
> > https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review
> > Alexander-Stein/OV9281-support/20220728-210448 git checkout
> > ee28006553d4d23f600b0076ef6066710519f156
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> > O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/i2c/
> > 
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>):
> > >> drivers/media/i2c/ov9282.c:1054:10: warning: variable 'ret' is
> > >> uninitialized when used here [-Wuninitialized]
> >                    return ret;
> >                           ^~~
> >    drivers/media/i2c/ov9282.c:1041:9: note: initialize the variable 'ret' to
> > silence this warning int ret;
> >                   ^
> >                    = 0
> >    1 warning generated.
> > 
> > 
> > vim +/ret +1054 drivers/media/i2c/ov9282.c
> > 
> >   1030
> >   1031	/**
> >   1032	 * ov9282_probe() - I2C client device binding
> >   1033	 * @client: pointer to i2c client device
> >   1034	 *
> >   1035	 * Return: 0 if successful, error code otherwise.
> >   1036	 */
> >   1037	static int ov9282_probe(struct i2c_client *client)
> >   1038	{
> >   1039		struct ov9282 *ov9282;
> >   1040		const char *sensor_name;
> >   1041		int ret;
> >   1042
> >   1043		ov9282 = devm_kzalloc(&client->dev, sizeof(*ov9282), 
> GFP_KERNEL);
> >   1044		if (!ov9282)
> >   1045			return -ENOMEM;
> >   1046
> >   1047		ov9282->dev = &client->dev;
> >   1048
> >   1049		/* Initialize subdev */
> >   1050		v4l2_i2c_subdev_init(&ov9282->sd, client, 
> &ov9282_subdev_ops);
> >   1051		sensor_name = device_get_match_data(ov9282->dev);
> >   1052		if (!sensor_name) {
> >   1053			dev_err(ov9282->dev, "Sensor name is 
> missing");
> > 
> > > 1054			return ret;
> > 
> >   1055		}
> >   1056		v4l2_i2c_subdev_set_name(&ov9282->sd, client, 
> sensor_name, NULL);
> >   1057
> >   1058		ret = ov9282_parse_hw_config(ov9282);
> >   1059		if (ret) {
> >   1060			dev_err(ov9282->dev, "HW configuration is not 
> supported");
> >   1061			return ret;
> >   1062		}
> >   1063
> >   1064		ret = ov9282_get_regulators(ov9282);
> >   1065		if (ret) {
> >   1066			dev_err(&client->dev, "Failed to get power 
> regulators\n");
> >   1067			return ret;
> >   1068		}
> >   1069
> >   1070		mutex_init(&ov9282->mutex);
> >   1071
> >   1072		ret = ov9282_power_on(ov9282->dev);
> >   1073		if (ret) {
> >   1074			dev_err(ov9282->dev, "failed to power-on the 
> sensor");
> >   1075			goto error_mutex_destroy;
> >   1076		}
> >   1077
> >   1078		/* Check module identity */
> >   1079		ret = ov9282_detect(ov9282);
> >   1080		if (ret) {
> >   1081			dev_err(ov9282->dev, "failed to find sensor: 
> %d", ret);
> >   1082			goto error_power_off;
> >   1083		}
> >   1084
> >   1085		/* Set default mode to max resolution */
> >   1086		ov9282->cur_mode = &supported_mode;
> >   1087		ov9282->vblank = ov9282->cur_mode->vblank;
> >   1088
> >   1089		ret = ov9282_init_controls(ov9282);
> >   1090		if (ret) {
> >   1091			dev_err(ov9282->dev, "failed to init 
> controls: %d", ret);
> >   1092			goto error_power_off;
> >   1093		}
> >   1094
> >   1095		/* Initialize subdev */
> >   1096		ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >   1097		ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> >   1098
> >   1099		/* Initialize source pad */
> >   1100		ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
> >   1101		ret = media_entity_pads_init(&ov9282->sd.entity, 1, 
> &ov9282->pad);
> >   1102		if (ret) {
> >   1103			dev_err(ov9282->dev, "failed to init entity 
> pads: %d", ret);
> >   1104			goto error_handler_free;
> >   1105		}
> >   1106
> >   1107		ret = v4l2_async_register_subdev_sensor(&ov9282->sd);
> >   1108		if (ret < 0) {
> >   1109			dev_err(ov9282->dev,
> >   1110				"failed to register async subdev: 
> %d", ret);
> >   1111			goto error_media_entity;
> >   1112		}
> >   1113
> >   1114		pm_runtime_set_active(ov9282->dev);
> >   1115		pm_runtime_enable(ov9282->dev);
> >   1116		pm_runtime_idle(ov9282->dev);
> >   1117
> >   1118		return 0;
> >   1119
> >   1120	error_media_entity:
> >   1121		media_entity_cleanup(&ov9282->sd.entity);
> >   1122	error_handler_free:
> >   1123		v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler);
> >   1124	error_power_off:
> >   1125		ov9282_power_off(ov9282->dev);
> >   1126	error_mutex_destroy:
> >   1127		mutex_destroy(&ov9282->mutex);
> >   1128
> >   1129		return ret;
> >   1130	}
> >   1131
> 
> Meh, I'll come up with a fixed once discussion about the additional compatible 
> has settled. This will also include the missing member documentation in patch 
> 5

I think you could simply pass device_get_match_data() return value as the
sensor name. The driver is in direct control of the string so I don't think
you need error handling here.

-- 
Sakari Ailus

Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model

[Sakari Ailus]

[-- Attachment #1: Type: text/plain, Size: 6629 bytes --]

Hi Alexander,

On Fri, Jul 29, 2022 at 10:23:48AM +0200, Alexander Stein wrote:
> Am Donnerstag, 28. Juli 2022, 23:10:07 CEST schrieb kernel test robot:
> > Hi Alexander,
> > 
> > Thank you for the patch! Perhaps something to improve:
> > 
> > [auto build test WARNING on media-tree/master]
> > [also build test WARNING on linus/master v5.19-rc8 next-20220728]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch#_base_tree_information]
> > 
> > url:   
> > https://github.com/intel-lab-lkp/linux/commits/Alexander-Stein/OV9281-suppo
> > rt/20220728-210448 base:   git://linuxtv.org/media_tree.git master
> > config: arm-randconfig-r022-20220728
> > (https://download.01.org/0day-ci/archive/20220729/202207290518.1D7MVS65-lkp
> > @intel.com/config) compiler: clang version 15.0.0
> > (https://github.com/llvm/llvm-project
> > 8dfaecc4c24494337933aff9d9166486ca0949f1) reproduce (this is a W=1 build):
> >         wget
> > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O
> > ~/bin/make.cross chmod +x ~/bin/make.cross
> >         # install arm cross compiling tool for clang build
> >         # apt-get install binutils-arm-linux-gnueabi
> >         #
> > https://github.com/intel-lab-lkp/linux/commit/ee28006553d4d23f600b0076ef606
> > 6710519f156 git remote add linux-review
> > https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review
> > Alexander-Stein/OV9281-support/20220728-210448 git checkout
> > ee28006553d4d23f600b0076ef6066710519f156
> >         # save the config file
> >         mkdir build_dir && cp config build_dir/.config
> >         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1
> > O=build_dir ARCH=arm SHELL=/bin/bash drivers/media/i2c/
> > 
> > If you fix the issue, kindly add following tag where applicable
> > Reported-by: kernel test robot <lkp@intel.com>
> > 
> > All warnings (new ones prefixed by >>):
> > >> drivers/media/i2c/ov9282.c:1054:10: warning: variable 'ret' is
> > >> uninitialized when used here [-Wuninitialized]
> >                    return ret;
> >                           ^~~
> >    drivers/media/i2c/ov9282.c:1041:9: note: initialize the variable 'ret' to
> > silence this warning int ret;
> >                   ^
> >                    = 0
> >    1 warning generated.
> > 
> > 
> > vim +/ret +1054 drivers/media/i2c/ov9282.c
> > 
> >   1030
> >   1031	/**
> >   1032	 * ov9282_probe() - I2C client device binding
> >   1033	 * @client: pointer to i2c client device
> >   1034	 *
> >   1035	 * Return: 0 if successful, error code otherwise.
> >   1036	 */
> >   1037	static int ov9282_probe(struct i2c_client *client)
> >   1038	{
> >   1039		struct ov9282 *ov9282;
> >   1040		const char *sensor_name;
> >   1041		int ret;
> >   1042
> >   1043		ov9282 = devm_kzalloc(&client->dev, sizeof(*ov9282), 
> GFP_KERNEL);
> >   1044		if (!ov9282)
> >   1045			return -ENOMEM;
> >   1046
> >   1047		ov9282->dev = &client->dev;
> >   1048
> >   1049		/* Initialize subdev */
> >   1050		v4l2_i2c_subdev_init(&ov9282->sd, client, 
> &ov9282_subdev_ops);
> >   1051		sensor_name = device_get_match_data(ov9282->dev);
> >   1052		if (!sensor_name) {
> >   1053			dev_err(ov9282->dev, "Sensor name is 
> missing");
> > 
> > > 1054			return ret;
> > 
> >   1055		}
> >   1056		v4l2_i2c_subdev_set_name(&ov9282->sd, client, 
> sensor_name, NULL);
> >   1057
> >   1058		ret = ov9282_parse_hw_config(ov9282);
> >   1059		if (ret) {
> >   1060			dev_err(ov9282->dev, "HW configuration is not 
> supported");
> >   1061			return ret;
> >   1062		}
> >   1063
> >   1064		ret = ov9282_get_regulators(ov9282);
> >   1065		if (ret) {
> >   1066			dev_err(&client->dev, "Failed to get power 
> regulators\n");
> >   1067			return ret;
> >   1068		}
> >   1069
> >   1070		mutex_init(&ov9282->mutex);
> >   1071
> >   1072		ret = ov9282_power_on(ov9282->dev);
> >   1073		if (ret) {
> >   1074			dev_err(ov9282->dev, "failed to power-on the 
> sensor");
> >   1075			goto error_mutex_destroy;
> >   1076		}
> >   1077
> >   1078		/* Check module identity */
> >   1079		ret = ov9282_detect(ov9282);
> >   1080		if (ret) {
> >   1081			dev_err(ov9282->dev, "failed to find sensor: 
> %d", ret);
> >   1082			goto error_power_off;
> >   1083		}
> >   1084
> >   1085		/* Set default mode to max resolution */
> >   1086		ov9282->cur_mode = &supported_mode;
> >   1087		ov9282->vblank = ov9282->cur_mode->vblank;
> >   1088
> >   1089		ret = ov9282_init_controls(ov9282);
> >   1090		if (ret) {
> >   1091			dev_err(ov9282->dev, "failed to init 
> controls: %d", ret);
> >   1092			goto error_power_off;
> >   1093		}
> >   1094
> >   1095		/* Initialize subdev */
> >   1096		ov9282->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> >   1097		ov9282->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
> >   1098
> >   1099		/* Initialize source pad */
> >   1100		ov9282->pad.flags = MEDIA_PAD_FL_SOURCE;
> >   1101		ret = media_entity_pads_init(&ov9282->sd.entity, 1, 
> &ov9282->pad);
> >   1102		if (ret) {
> >   1103			dev_err(ov9282->dev, "failed to init entity 
> pads: %d", ret);
> >   1104			goto error_handler_free;
> >   1105		}
> >   1106
> >   1107		ret = v4l2_async_register_subdev_sensor(&ov9282->sd);
> >   1108		if (ret < 0) {
> >   1109			dev_err(ov9282->dev,
> >   1110				"failed to register async subdev: 
> %d", ret);
> >   1111			goto error_media_entity;
> >   1112		}
> >   1113
> >   1114		pm_runtime_set_active(ov9282->dev);
> >   1115		pm_runtime_enable(ov9282->dev);
> >   1116		pm_runtime_idle(ov9282->dev);
> >   1117
> >   1118		return 0;
> >   1119
> >   1120	error_media_entity:
> >   1121		media_entity_cleanup(&ov9282->sd.entity);
> >   1122	error_handler_free:
> >   1123		v4l2_ctrl_handler_free(ov9282->sd.ctrl_handler);
> >   1124	error_power_off:
> >   1125		ov9282_power_off(ov9282->dev);
> >   1126	error_mutex_destroy:
> >   1127		mutex_destroy(&ov9282->mutex);
> >   1128
> >   1129		return ret;
> >   1130	}
> >   1131
> 
> Meh, I'll come up with a fixed once discussion about the additional compatible 
> has settled. This will also include the missing member documentation in patch 
> 5

I think you could simply pass device_get_match_data() return value as the
sensor name. The driver is in direct control of the string so I don't think
you need error handling here.

-- 
Sakari Ailus

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Krzysztof Kozlowski]

On 29/07/2022 10:18, Laurent Pinchart wrote:
> Hi Sakari,
> 
> (Adding Dave and Naush to the CC list)
> 
> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>> According to product brief they are identical from software point of view.
>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>
>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>> ---
>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>  1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>> --- a/drivers/media/i2c/ov9282.c
>>>> +++ b/drivers/media/i2c/ov9282.c
>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>>>>  };
>>>>  
>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>> +	{ .compatible = "ovti,ov9281" },
>>>
>>> The devices seem entirely compatible, so why you add a new compatible
>>> and not re-use existing?
>>>
>>> The difference in lens does not explain this.
>>
>> It is typically necessary to know what kind of related hardware can be
>> found in the system, beyond just the device's register interface. Apart
>> from USB cameras, less integrated cameras require low-level software
>> control in which specific device properties are important. In this case it
>> could be the lens shading table, among other things.
>>
>> 	https://www.ovt.com/sensor/ov9282/
>>
>> Therefore I think adding a specific compatible string for this one is
>> justified.

Specific compatible in binding is a requirement. No one discussed this.
However not in the driver. None of the arguments above justify adding
such binding, unless user-space depends on matching compatible, but not
real compatible?

>>
>> Also cc Laurent.
> 
> Interesting coincidence, we've talked about this topic (as part of a
> broader discussion) no later than yesterday.
> 
> I agree with Sakari in that userspace needs to know the exact model of
> the camera sensor. I don't see a good alternative to providing that
> information through the platform firmware, so the device tree in this
> case. The question is how it should be provided (the question of how it
> should then be exposed to userspace is also important, but out of scope
> in this discussion).
> 
> The compatible string is meant to indicate a device's compatibility with
> "something", and that something is often considered from the point of
> view of software support, and in particular to pick an appropriate
> kernel driver and tune its behaviour for the device. Here, one could
> argue that the exact model is also needed to ensure proper software
> support, but in userspace this time, not in the kernel. I think using a
> dedicated compatible string would be reasonable. An alternative would be
> to use another DT property, which should then be standardized. I'm not
> sure it's worth it.
> 
> Broadening the discussion, we also need to know detailed information
> about the camera lens (I'm talking about the lens itself here, not the
> lens controller IC that controls the motor that moves the focus lens).
> The lens isn't described in the device tree with a dedicated device tree
> node today, and I don't think it should (I'd have a hard time coming up
> with a naming scheme for lenses that we could use in compatible strings,
> and the lens-related data that a system requires can possibly vary based
> not only on the lens itself but on the ISP that the camera sensor is
> used with). Typical useful data are the lens movement range, the
> hyperfocal distance, but also the lens shading tables. (Part of) that
> information is sometimes stored in non-volatile memory in the camera
> module (OTP in the camera sensor itself, or a separate EEPROM), but
> that's not always the case. We have considered the possibility of
> storing the information in the device tree, but I doubt that would be
> accepted. We can store the information in userspace in configuration
> files, but we will still need to device tree to provide lens
> identification information to select the correct configuration file. I
> don't know how that should be done.

It seems both you and Sakari suggested not to have specific compatible.
Such idea (not to have specific compatible) was not proposed by me.
Quite contrary - specific compatible is a requirement. However device
driver does no need it. Just use fallback for the driver.

Best regards,
Krzysztof

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Krzysztof Kozlowski]

On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> On 29/07/2022 10:18, Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> (Adding Dave and Naush to the CC list)
>>
>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>>> According to product brief they are identical from software point of view.
>>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>>
>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>>> ---
>>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>>> --- a/drivers/media/i2c/ov9282.c
>>>>> +++ b/drivers/media/i2c/ov9282.c
>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>>>>>  };
>>>>>  
>>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>>> +	{ .compatible = "ovti,ov9281" },
>>>>
>>>> The devices seem entirely compatible, so why you add a new compatible
>>>> and not re-use existing?
>>>>
>>>> The difference in lens does not explain this.
>>>
>>> It is typically necessary to know what kind of related hardware can be
>>> found in the system, beyond just the device's register interface. Apart
>>> from USB cameras, less integrated cameras require low-level software
>>> control in which specific device properties are important. In this case it
>>> could be the lens shading table, among other things.
>>>
>>> 	https://www.ovt.com/sensor/ov9282/
>>>
>>> Therefore I think adding a specific compatible string for this one is
>>> justified.
> 
> Specific compatible in binding is a requirement. No one discussed this.
> However not in the driver. None of the arguments above justify adding
> such binding, unless user-space depends on matching compatible, but not
> real compatible?

Eh, now I used vague words. This should be instead:

"However not in the driver. None of the arguments above justify adding
such compatible to driver, unless user-space depends on matching
compatible, but not real compatible?"

> 
>>>
>>> Also cc Laurent.
>>
>> Interesting coincidence, we've talked about this topic (as part of a
>> broader discussion) no later than yesterday.
>>
>> I agree with Sakari in that userspace needs to know the exact model of
>> the camera sensor. I don't see a good alternative to providing that
>> information through the platform firmware, so the device tree in this
>> case. The question is how it should be provided (the question of how it
>> should then be exposed to userspace is also important, but out of scope
>> in this discussion).
>>
>> The compatible string is meant to indicate a device's compatibility with
>> "something", and that something is often considered from the point of
>> view of software support, and in particular to pick an appropriate
>> kernel driver and tune its behaviour for the device. Here, one could
>> argue that the exact model is also needed to ensure proper software
>> support, but in userspace this time, not in the kernel. I think using a
>> dedicated compatible string would be reasonable. An alternative would be
>> to use another DT property, which should then be standardized. I'm not
>> sure it's worth it.
>>
>> Broadening the discussion, we also need to know detailed information
>> about the camera lens (I'm talking about the lens itself here, not the
>> lens controller IC that controls the motor that moves the focus lens).
>> The lens isn't described in the device tree with a dedicated device tree
>> node today, and I don't think it should (I'd have a hard time coming up
>> with a naming scheme for lenses that we could use in compatible strings,
>> and the lens-related data that a system requires can possibly vary based
>> not only on the lens itself but on the ISP that the camera sensor is
>> used with). Typical useful data are the lens movement range, the
>> hyperfocal distance, but also the lens shading tables. (Part of) that
>> information is sometimes stored in non-volatile memory in the camera
>> module (OTP in the camera sensor itself, or a separate EEPROM), but
>> that's not always the case. We have considered the possibility of
>> storing the information in the device tree, but I doubt that would be
>> accepted. We can store the information in userspace in configuration
>> files, but we will still need to device tree to provide lens
>> identification information to select the correct configuration file. I
>> don't know how that should be done.
> 
> It seems both you and Sakari suggested not to have specific compatible.
> Such idea (not to have specific compatible) was not proposed by me.
> Quite contrary - specific compatible is a requirement. However device
> driver does no need it. Just use fallback for the driver.


Best regards,
Krzysztof

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Sakari Ailus]

On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> > On 29/07/2022 10:18, Laurent Pinchart wrote:
> >> Hi Sakari,
> >>
> >> (Adding Dave and Naush to the CC list)
> >>
> >> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> >>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 28/07/2022 15:02, Alexander Stein wrote:
> >>>>> According to product brief they are identical from software point of view.
> >>>>> Differences are a different chief ray angle (CRA) and the package.
> >>>>>
> >>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >>>>> ---
> >>>>>  drivers/media/i2c/ov9282.c | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> >>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
> >>>>> --- a/drivers/media/i2c/ov9282.c
> >>>>> +++ b/drivers/media/i2c/ov9282.c
> >>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> >>>>>  };
> >>>>>  
> >>>>>  static const struct of_device_id ov9282_of_match[] = {
> >>>>> +	{ .compatible = "ovti,ov9281" },
> >>>>
> >>>> The devices seem entirely compatible, so why you add a new compatible
> >>>> and not re-use existing?
> >>>>
> >>>> The difference in lens does not explain this.
> >>>
> >>> It is typically necessary to know what kind of related hardware can be
> >>> found in the system, beyond just the device's register interface. Apart
> >>> from USB cameras, less integrated cameras require low-level software
> >>> control in which specific device properties are important. In this case it
> >>> could be the lens shading table, among other things.
> >>>
> >>> 	https://www.ovt.com/sensor/ov9282/
> >>>
> >>> Therefore I think adding a specific compatible string for this one is
> >>> justified.
> > 
> > Specific compatible in binding is a requirement. No one discussed this.
> > However not in the driver. None of the arguments above justify adding
> > such binding, unless user-space depends on matching compatible, but not
> > real compatible?
> 
> Eh, now I used vague words. This should be instead:
> 
> "However not in the driver. None of the arguments above justify adding
> such compatible to driver, unless user-space depends on matching
> compatible, but not real compatible?"

If I understand you right, you'd put the more specific model name as well
as the more generic one to the compatible property and let the driver match
against the more generic one?

But in this case neither of these models is more generic than the other.

-- 
Sakari Ailus

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Krzysztof Kozlowski]

On 02/08/2022 10:23, Sakari Ailus wrote:
> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
>>> On 29/07/2022 10:18, Laurent Pinchart wrote:
>>>> Hi Sakari,
>>>>
>>>> (Adding Dave and Naush to the CC list)
>>>>
>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>>>>> According to product brief they are identical from software point of view.
>>>>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>>>>
>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>>>>> ---
>>>>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>
>>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>>>>> --- a/drivers/media/i2c/ov9282.c
>>>>>>> +++ b/drivers/media/i2c/ov9282.c
>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
>>>>>>>  };
>>>>>>>  
>>>>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>>>>> +	{ .compatible = "ovti,ov9281" },
>>>>>>
>>>>>> The devices seem entirely compatible, so why you add a new compatible
>>>>>> and not re-use existing?
>>>>>>
>>>>>> The difference in lens does not explain this.
>>>>>
>>>>> It is typically necessary to know what kind of related hardware can be
>>>>> found in the system, beyond just the device's register interface. Apart
>>>>> from USB cameras, less integrated cameras require low-level software
>>>>> control in which specific device properties are important. In this case it
>>>>> could be the lens shading table, among other things.
>>>>>
>>>>> 	https://www.ovt.com/sensor/ov9282/
>>>>>
>>>>> Therefore I think adding a specific compatible string for this one is
>>>>> justified.
>>>
>>> Specific compatible in binding is a requirement. No one discussed this.
>>> However not in the driver. None of the arguments above justify adding
>>> such binding, unless user-space depends on matching compatible, but not
>>> real compatible?
>>
>> Eh, now I used vague words. This should be instead:
>>
>> "However not in the driver. None of the arguments above justify adding
>> such compatible to driver, unless user-space depends on matching
>> compatible, but not real compatible?"
> 
> If I understand you right, you'd put the more specific model name as well
> as the more generic one to the compatible property and let the driver match
> against the more generic one?

Yes.

> 
> But in this case neither of these models is more generic than the other.

It's not a problem. Also the spec explains it similar way:
"They
 allow a device to express its compatibility with a family of similar
devices, potentially allowing a single
 device driver to match against several devices."

Of course the numbers would suggest that ov9281 should be the family (as
lower number usually means designed earlier), but it is a matter of
convention which here can be skipped. The point is that ov9281 and
ov9282 are compatible between each other, therefore they belong to
single family.

Best regards,
Krzysztof

Re: Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Alexander Stein]

Hello,

Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski:
> On 02/08/2022 10:23, Sakari Ailus wrote:
> > On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> >> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> >>> On 29/07/2022 10:18, Laurent Pinchart wrote:
> >>>> Hi Sakari,
> >>>> 
> >>>> (Adding Dave and Naush to the CC list)
> >>>> 
> >>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> >>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> >>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
> >>>>>>> According to product brief they are identical from software point of
> >>>>>>> view.
> >>>>>>> Differences are a different chief ray angle (CRA) and the package.
> >>>>>>> 
> >>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >>>>>>> ---
> >>>>>>> 
> >>>>>>>  drivers/media/i2c/ov9282.c | 1 +
> >>>>>>>  1 file changed, 1 insertion(+)
> >>>>>>> 
> >>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> >>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
> >>>>>>> --- a/drivers/media/i2c/ov9282.c
> >>>>>>> +++ b/drivers/media/i2c/ov9282.c
> >>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops =
> >>>>>>> {
> >>>>>>> 
> >>>>>>>  };
> >>>>>>>  
> >>>>>>>  static const struct of_device_id ov9282_of_match[] = {
> >>>>>>> 
> >>>>>>> +	{ .compatible = "ovti,ov9281" },
> >>>>>> 
> >>>>>> The devices seem entirely compatible, so why you add a new compatible
> >>>>>> and not re-use existing?
> >>>>>> 
> >>>>>> The difference in lens does not explain this.
> >>>>> 
> >>>>> It is typically necessary to know what kind of related hardware can be
> >>>>> found in the system, beyond just the device's register interface.
> >>>>> Apart
> >>>>> from USB cameras, less integrated cameras require low-level software
> >>>>> control in which specific device properties are important. In this
> >>>>> case it
> >>>>> could be the lens shading table, among other things.
> >>>>> 
> >>>>> 	https://www.ovt.com/sensor/ov9282/
> >>>>> 
> >>>>> Therefore I think adding a specific compatible string for this one is
> >>>>> justified.
> >>> 
> >>> Specific compatible in binding is a requirement. No one discussed this.
> >>> However not in the driver. None of the arguments above justify adding
> >>> such binding, unless user-space depends on matching compatible, but not
> >>> real compatible?
> >> 
> >> Eh, now I used vague words. This should be instead:
> >> 
> >> "However not in the driver. None of the arguments above justify adding
> >> such compatible to driver, unless user-space depends on matching
> >> compatible, but not real compatible?"
> > 
> > If I understand you right, you'd put the more specific model name as well
> > as the more generic one to the compatible property and let the driver
> > match
> > against the more generic one?
> 
> Yes.
> 
> > But in this case neither of these models is more generic than the other.
> 
> It's not a problem. Also the spec explains it similar way:
> "They
>  allow a device to express its compatibility with a family of similar
> devices, potentially allowing a single
>  device driver to match against several devices."
> 
> Of course the numbers would suggest that ov9281 should be the family (as
> lower number usually means designed earlier), but it is a matter of
> convention which here can be skipped. The point is that ov9281 and
> ov9282 are compatible between each other, therefore they belong to
> single family.
> 
> Best regards,
> Krzysztof

So what is the conclusion of this?
If using the "family" name there is no way for userspace to see the actual 
device name rather than the driver name. This might be confusing, especially 
of both ov9281 and ov9282 are attached to the same platform. The only 
difference would be the i2c-bus-address.
You can also go for ov928x but this is not a real improvement.

Best regards,
Alexander

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Krzysztof Kozlowski]

On 15/08/2022 14:19, Alexander Stein wrote:
> Hello,
> 
> Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski:
>> On 02/08/2022 10:23, Sakari Ailus wrote:
>>> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
>>>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
>>>>> On 29/07/2022 10:18, Laurent Pinchart wrote:
>>>>>> Hi Sakari,
>>>>>>
>>>>>> (Adding Dave and Naush to the CC list)
>>>>>>
>>>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
>>>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>>>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
>>>>>>>>> According to product brief they are identical from software point of
>>>>>>>>> view.
>>>>>>>>> Differences are a different chief ray angle (CRA) and the package.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
>>>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>>  drivers/media/i2c/ov9282.c | 1 +
>>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
>>>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
>>>>>>>>> --- a/drivers/media/i2c/ov9282.c
>>>>>>>>> +++ b/drivers/media/i2c/ov9282.c
>>>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops =
>>>>>>>>> {
>>>>>>>>>
>>>>>>>>>  };
>>>>>>>>>  
>>>>>>>>>  static const struct of_device_id ov9282_of_match[] = {
>>>>>>>>>
>>>>>>>>> +	{ .compatible = "ovti,ov9281" },
>>>>>>>>
>>>>>>>> The devices seem entirely compatible, so why you add a new compatible
>>>>>>>> and not re-use existing?
>>>>>>>>
>>>>>>>> The difference in lens does not explain this.
>>>>>>>
>>>>>>> It is typically necessary to know what kind of related hardware can be
>>>>>>> found in the system, beyond just the device's register interface.
>>>>>>> Apart
>>>>>>> from USB cameras, less integrated cameras require low-level software
>>>>>>> control in which specific device properties are important. In this
>>>>>>> case it
>>>>>>> could be the lens shading table, among other things.
>>>>>>>
>>>>>>> 	https://www.ovt.com/sensor/ov9282/
>>>>>>>
>>>>>>> Therefore I think adding a specific compatible string for this one is
>>>>>>> justified.
>>>>>
>>>>> Specific compatible in binding is a requirement. No one discussed this.
>>>>> However not in the driver. None of the arguments above justify adding
>>>>> such binding, unless user-space depends on matching compatible, but not
>>>>> real compatible?
>>>>
>>>> Eh, now I used vague words. This should be instead:
>>>>
>>>> "However not in the driver. None of the arguments above justify adding
>>>> such compatible to driver, unless user-space depends on matching
>>>> compatible, but not real compatible?"
>>>
>>> If I understand you right, you'd put the more specific model name as well
>>> as the more generic one to the compatible property and let the driver
>>> match
>>> against the more generic one?
>>
>> Yes.
>>
>>> But in this case neither of these models is more generic than the other.
>>
>> It's not a problem. Also the spec explains it similar way:
>> "They
>>  allow a device to express its compatibility with a family of similar
>> devices, potentially allowing a single
>>  device driver to match against several devices."
>>
>> Of course the numbers would suggest that ov9281 should be the family (as
>> lower number usually means designed earlier), but it is a matter of
>> convention which here can be skipped. The point is that ov9281 and
>> ov9282 are compatible between each other, therefore they belong to
>> single family.
>>
>> Best regards,
>> Krzysztof
> 
> So what is the conclusion of this?
> If using the "family" name there is no way for userspace to see the actual 
> device name rather than the driver name. This might be confusing, especially 
> of both ov9281 and ov9282 are attached to the same platform. The only 
> difference would be the i2c-bus-address.
> You can also go for ov928x but this is not a real improvement.

I still don't understand. Why user-space cannot see this? I really
cannot find any trouble... Your 3/7 patch does nothing special here for
user-space...

Best regards,
Krzysztof

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Alexander Stein]

Am Dienstag, 16. August 2022, 09:16:44 CEST schrieb Krzysztof Kozlowski:
> On 15/08/2022 14:19, Alexander Stein wrote:
> > Hello,
> > 
> > Am Dienstag, 2. August 2022, 10:30:40 CEST schrieb Krzysztof Kozlowski:
> >> On 02/08/2022 10:23, Sakari Ailus wrote:
> >>> On Mon, Aug 01, 2022 at 08:08:58PM +0200, Krzysztof Kozlowski wrote:
> >>>> On 01/08/2022 20:07, Krzysztof Kozlowski wrote:
> >>>>> On 29/07/2022 10:18, Laurent Pinchart wrote:
> >>>>>> Hi Sakari,
> >>>>>> 
> >>>>>> (Adding Dave and Naush to the CC list)
> >>>>>> 
> >>>>>> On Fri, Jul 29, 2022 at 10:07:36AM +0300, Sakari Ailus wrote:
> >>>>>>> On Thu, Jul 28, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> >>>>>>>> On 28/07/2022 15:02, Alexander Stein wrote:
> >>>>>>>>> According to product brief they are identical from software point
> >>>>>>>>> of
> >>>>>>>>> view.
> >>>>>>>>> Differences are a different chief ray angle (CRA) and the package.
> >>>>>>>>> 
> >>>>>>>>> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> >>>>>>>>> Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> >>>>>>>>> ---
> >>>>>>>>> 
> >>>>>>>>>  drivers/media/i2c/ov9282.c | 1 +
> >>>>>>>>>  1 file changed, 1 insertion(+)
> >>>>>>>>> 
> >>>>>>>>> diff --git a/drivers/media/i2c/ov9282.c
> >>>>>>>>> b/drivers/media/i2c/ov9282.c
> >>>>>>>>> index 8a252bf3b59f..c8d83a29f9bb 100644
> >>>>>>>>> --- a/drivers/media/i2c/ov9282.c
> >>>>>>>>> +++ b/drivers/media/i2c/ov9282.c
> >>>>>>>>> @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops
> >>>>>>>>> =
> >>>>>>>>> {
> >>>>>>>>> 
> >>>>>>>>>  };
> >>>>>>>>>  
> >>>>>>>>>  static const struct of_device_id ov9282_of_match[] = {
> >>>>>>>>> 
> >>>>>>>>> +	{ .compatible = "ovti,ov9281" },
> >>>>>>>> 
> >>>>>>>> The devices seem entirely compatible, so why you add a new
> >>>>>>>> compatible
> >>>>>>>> and not re-use existing?
> >>>>>>>> 
> >>>>>>>> The difference in lens does not explain this.
> >>>>>>> 
> >>>>>>> It is typically necessary to know what kind of related hardware can
> >>>>>>> be
> >>>>>>> found in the system, beyond just the device's register interface.
> >>>>>>> Apart
> >>>>>>> from USB cameras, less integrated cameras require low-level software
> >>>>>>> control in which specific device properties are important. In this
> >>>>>>> case it
> >>>>>>> could be the lens shading table, among other things.
> >>>>>>> 
> >>>>>>> 	https://www.ovt.com/sensor/ov9282/
> >>>>>>> 
> >>>>>>> Therefore I think adding a specific compatible string for this one
> >>>>>>> is
> >>>>>>> justified.
> >>>>> 
> >>>>> Specific compatible in binding is a requirement. No one discussed
> >>>>> this.
> >>>>> However not in the driver. None of the arguments above justify adding
> >>>>> such binding, unless user-space depends on matching compatible, but
> >>>>> not
> >>>>> real compatible?
> >>>> 
> >>>> Eh, now I used vague words. This should be instead:
> >>>> 
> >>>> "However not in the driver. None of the arguments above justify adding
> >>>> such compatible to driver, unless user-space depends on matching
> >>>> compatible, but not real compatible?"
> >>> 
> >>> If I understand you right, you'd put the more specific model name as
> >>> well
> >>> as the more generic one to the compatible property and let the driver
> >>> match
> >>> against the more generic one?
> >> 
> >> Yes.
> >> 
> >>> But in this case neither of these models is more generic than the other.
> >> 
> >> It's not a problem. Also the spec explains it similar way:
> >> "They
> >> 
> >>  allow a device to express its compatibility with a family of similar
> >> 
> >> devices, potentially allowing a single
> >> 
> >>  device driver to match against several devices."
> >> 
> >> Of course the numbers would suggest that ov9281 should be the family (as
> >> lower number usually means designed earlier), but it is a matter of
> >> convention which here can be skipped. The point is that ov9281 and
> >> ov9282 are compatible between each other, therefore they belong to
> >> single family.
> >> 
> >> Best regards,
> >> Krzysztof
> > 
> > So what is the conclusion of this?
> > If using the "family" name there is no way for userspace to see the actual
> > device name rather than the driver name. This might be confusing,
> > especially of both ov9281 and ov9282 are attached to the same platform.
> > The only difference would be the i2c-bus-address.
> > You can also go for ov928x but this is not a real improvement.
> 
> I still don't understand. Why user-space cannot see this? I really
> cannot find any trouble... Your 3/7 patch does nothing special here for
> user-space...

3/7 itself does nothing for userspace, but 6/7 does, which relies on separate 
compatibles in the driver.

Best regards,
Alexander

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Krzysztof Kozlowski]

On 16/08/2022 10:21, Alexander Stein wrote:
>>>
>>> So what is the conclusion of this?
>>> If using the "family" name there is no way for userspace to see the actual
>>> device name rather than the driver name. This might be confusing,
>>> especially of both ov9281 and ov9282 are attached to the same platform.
>>> The only difference would be the i2c-bus-address.
>>> You can also go for ov928x but this is not a real improvement.
>>
>> I still don't understand. Why user-space cannot see this? I really
>> cannot find any trouble... Your 3/7 patch does nothing special here for
>> user-space...
> 
> 3/7 itself does nothing for userspace, but 6/7 does, which relies on separate 
> compatibles in the driver.

Ah, that's so confusing... First adding new incomplete device support in
patch 3 and then in patch 6 fixing it. 6 and 3 should be squashed. They
really do not make any sense being separate and this just brings this
unnecessary confusion.

I would argue that the binding still should have devices compatible (in
one family), but now it is a bit less important.

Best regards,
Krzysztof

Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible

[Alexander Stein]

Hello Kieran,

Am Samstag, 12. November 2022, 00:48:24 CET schrieb Kieran Bingham:
> Hi All,
> 
> Quoting Alexander Stein (2022-07-28 14:02:33)
> 
> > According to product brief they are identical from software point of view.
> > Differences are a different chief ray angle (CRA) and the package.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > Acked-by: Daniele Alessandrelli <daniele.alessandrelli@intel.com>
> 
> Throwing my hat in the ring on this thread as I see it has been hanging
> around for a while and my attention was sent here from [0]
> 
> [0]
> https://lists.libcamera.org/pipermail/libcamera-devel/2022-November/035495.
> html

I postponed working on this change for a while, because there are (at least) 
two series from Dave pending which conflict a bit with this series.

> > ---
> > 
> >  drivers/media/i2c/ov9282.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index 8a252bf3b59f..c8d83a29f9bb 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1113,6 +1113,7 @@ static const struct dev_pm_ops ov9282_pm_ops = {
> > 
> >  };
> >  
> >  static const struct of_device_id ov9282_of_match[] = {
> > 
> > +       { .compatible = "ovti,ov9281" },
> 
> I believe from my existing understanding of how we would support
> existing sensors even with very similar parts is that a direct
> compatible lets the DT express this.
> 
> If there were a common name that we could apply, we could have a generic
> name here too, but I don't see anything specifically generic, and I
> haven't yet seen a clear pattern in the namings schemes from omnivision
> so ov928x wouldn't be appropriate as I couldn't be sure that an
> unrelated ov9289 wouldn't exist with very different properties ... so ..
> 
> >         { .compatible = "ovti,ov9282" },
> 
> Either squashed with the later 6/7 that adds the name or not: (I think
> it's fine either separated or squashed)
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
> It does in turn bring questions into how we handle both the ov9281 and
> ov9282 together in libcamera, but I don't think that's an issue to solve
> here. Expressing the two separately to userspace also allows libcamera
> to make a distiction between the CRA should it need to.

Krzysztof is in favor of squashing so I'll respin this accordingly, removing 
the other changes from the ov9281 support series.

Best regards,
Alexander