[PATCH v3 0/7] OV9281 support

[PATCH v3 0/7] OV9281 support

[Alexander Stein]

Hi,

this is the 3rd 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 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                    | 102 ++++++++++++------
 2 files changed, 84 insertions(+), 32 deletions(-)

-- 
2.25.1

[PATCH v3 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 v3 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 v3 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 v3 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 v3 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 v3 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.
i2c_client already has the name parsed from the compatible.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.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 352dbe21a902..dbc0a4cd060f 100644
--- a/drivers/media/i2c/ov9282.c
+++ b/drivers/media/i2c/ov9282.c
@@ -1047,6 +1047,7 @@ static int ov9282_probe(struct i2c_client *client)
 
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
+	v4l2_i2c_subdev_set_name(&ov9282->sd, client, client->name, NULL);
 
 	ret = ov9282_parse_hw_config(ov9282);
 	if (ret) {
-- 
2.25.1

[PATCH v3 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>
---
 drivers/media/i2c/ov9282.c | 59 +++++++++++++++++++-------------------
 1 file changed, 30 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
index dbc0a4cd060f..d8eb71245aae 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, 0, 0, 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,19 @@ 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, 0, 0, 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;
 }
@@ -1044,6 +1040,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 v3 7/7] media: i2c: ov9282: Add regmap support

[Sakari Ailus]

Hi Alexander,

Thanks for the update.

On Fri, Jul 22, 2022 at 03:19:47PM +0200, Alexander Stein wrote:
> 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>
> ---
>  drivers/media/i2c/ov9282.c | 59 +++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> index dbc0a4cd060f..d8eb71245aae 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, 0, 0, 0};

You'll just need to initialise one of the array entries. I.e. this should
be { 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,19 @@ 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, 0, 0, 0};

Ditto.

> +	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);

Fits on one line.

> +	}
>  
>  	return 0;
>  }
> @@ -1044,6 +1040,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);

-- 
Sakari Ailus

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

[Sakari Ailus]

Hi Alexander,

On Fri, Jul 22, 2022 at 03:19:46PM +0200, Alexander Stein wrote:
> To distinguish ov9281 & ov9282 the name has to be explicitly set.
> i2c_client already has the name parsed from the compatible.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.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 352dbe21a902..dbc0a4cd060f 100644
> --- a/drivers/media/i2c/ov9282.c
> +++ b/drivers/media/i2c/ov9282.c
> @@ -1047,6 +1047,7 @@ static int ov9282_probe(struct i2c_client *client)
>  
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
> +	v4l2_i2c_subdev_set_name(&ov9282->sd, client, client->name, NULL);
>  

Could you instead do this based on the compatible string in the driver,
using device_get_match_data()? The approach works on non-OF systems, too.

>  	ret = ov9282_parse_hw_config(ov9282);
>  	if (ret) {

-- 
Regards,

Sakari Ailus

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

[Alexander Stein]

Hi Sakari,

Am Freitag, 22. Juli 2022, 15:50:30 CEST schrieb Sakari Ailus:
> Hi Alexander,
> 
> On Fri, Jul 22, 2022 at 03:19:46PM +0200, Alexander Stein wrote:
> > To distinguish ov9281 & ov9282 the name has to be explicitly set.
> > i2c_client already has the name parsed from the compatible.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.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 352dbe21a902..dbc0a4cd060f 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -1047,6 +1047,7 @@ static int ov9282_probe(struct i2c_client *client)
> > 
> >  	/* Initialize subdev */
> >  	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
> > 
> > +	v4l2_i2c_subdev_set_name(&ov9282->sd, client, client->name, NULL);
> 
> Could you instead do this based on the compatible string in the driver,
> using device_get_match_data()? The approach works on non-OF systems, too.

I actually don't like doing the same as of_modalias_node() is doing.
Until non-OF support is added (if ever), I don't see any benefit in doing so 
right now.

Best regards,
Alexander

> >  	ret = ov9282_parse_hw_config(ov9282);
> >  	if (ret) {

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

[Sakari Ailus]

Hi Alexander,

On Mon, Jul 25, 2022 at 08:38:57AM +0200, Alexander Stein wrote:
> Hi Sakari,
> 
> Am Freitag, 22. Juli 2022, 15:50:30 CEST schrieb Sakari Ailus:
> > Hi Alexander,
> > 
> > On Fri, Jul 22, 2022 at 03:19:46PM +0200, Alexander Stein wrote:
> > > To distinguish ov9281 & ov9282 the name has to be explicitly set.
> > > i2c_client already has the name parsed from the compatible.
> > > 
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.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 352dbe21a902..dbc0a4cd060f 100644
> > > --- a/drivers/media/i2c/ov9282.c
> > > +++ b/drivers/media/i2c/ov9282.c
> > > @@ -1047,6 +1047,7 @@ static int ov9282_probe(struct i2c_client *client)
> > > 
> > >  	/* Initialize subdev */
> > >  	v4l2_i2c_subdev_init(&ov9282->sd, client, &ov9282_subdev_ops);
> > > 
> > > +	v4l2_i2c_subdev_set_name(&ov9282->sd, client, client->name, NULL);
> > 
> > Could you instead do this based on the compatible string in the driver,
> > using device_get_match_data()? The approach works on non-OF systems, too.
> 
> I actually don't like doing the same as of_modalias_node() is doing.
> Until non-OF support is added (if ever), I don't see any benefit in doing so 
> right now.

client->name will be wrong on un-OF; putting this string to device match
data is a better option. It'll be about the same number of lines, too.

-- 
Sakari Ailus