* [PATCH v4 0/7] OV9281 support
@ 2022-07-28 13:02 Alexander Stein
2022-07-28 13:02 ` [PATCH v4 1/7] media: i2c: ov9282: remove unused and unset i2c_client member Alexander Stein
` (6 more replies)
0 siblings, 7 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v4 1/7] media: i2c: ov9282: remove unused and unset i2c_client member
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
2022-07-28 13:02 ` [PATCH v4 2/7] media: dt-bindings: media: Add compatible for ov9281 Alexander Stein
` (5 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 2/7] media: dt-bindings: media: Add compatible for ov9281
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
2022-07-28 13:02 ` [PATCH v4 1/7] media: i2c: ov9282: remove unused and unset i2c_client member Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
2022-07-28 13:02 ` [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible Alexander Stein
` (4 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus,
Krzysztof Kozlowski
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
2022-07-28 13:02 ` [PATCH v4 1/7] media: i2c: ov9282: remove unused and unset i2c_client member Alexander Stein
2022-07-28 13:02 ` [PATCH v4 2/7] media: dt-bindings: media: Add compatible for ov9281 Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
2022-07-28 13:13 ` Krzysztof Kozlowski
[not found] ` <166821050429.550668.2828222448343135143@Monstersaurus>
2022-07-28 13:02 ` [PATCH v4 4/7] media: dt-bindings: media: ov9282: Add power supply properties Alexander Stein
` (3 subsequent siblings)
6 siblings, 2 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 4/7] media: dt-bindings: media: ov9282: Add power supply properties
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
` (2 preceding siblings ...)
2022-07-28 13:02 ` [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
2022-07-28 13:02 ` [PATCH v4 5/7] media: i2c: ov9282: Add regulator support Alexander Stein
` (2 subsequent siblings)
6 siblings, 0 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus,
Krzysztof Kozlowski
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 5/7] media: i2c: ov9282: Add regulator support
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
` (3 preceding siblings ...)
2022-07-28 13:02 ` [PATCH v4 4/7] media: dt-bindings: media: ov9282: Add power supply properties Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
2022-07-28 13:02 ` [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model Alexander Stein
2022-07-28 13:02 ` [PATCH v4 7/7] media: i2c: ov9282: Add regmap support Alexander Stein
6 siblings, 0 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
` (4 preceding siblings ...)
2022-07-28 13:02 ` [PATCH v4 5/7] media: i2c: ov9282: Add regulator support Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
2022-07-28 21:10 ` kernel test robot
2022-07-28 13:02 ` [PATCH v4 7/7] media: i2c: ov9282: Add regmap support Alexander Stein
6 siblings, 1 reply; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v4 7/7] media: i2c: ov9282: Add regmap support
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
` (5 preceding siblings ...)
2022-07-28 13:02 ` [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model Alexander Stein
@ 2022-07-28 13:02 ` Alexander Stein
6 siblings, 0 replies; 23+ messages in thread
From: Alexander Stein @ 2022-07-28 13:02 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski
Cc: Alexander Stein, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-07-28 13:02 ` [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible Alexander Stein
@ 2022-07-28 13:13 ` Krzysztof Kozlowski
2022-07-29 7:07 ` Sakari Ailus
[not found] ` <166821050429.550668.2828222448343135143@Monstersaurus>
1 sibling, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-28 13:13 UTC (permalink / raw)
To: Alexander Stein, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski
Cc: linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model
2022-07-28 13:02 ` [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model Alexander Stein
@ 2022-07-28 21:10 ` kernel test robot
2022-07-29 8:23 ` Alexander Stein
0 siblings, 1 reply; 23+ messages in thread
From: kernel test robot @ 2022-07-28 21:10 UTC (permalink / raw)
To: Alexander Stein, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski
Cc: llvm, kbuild-all, linux-media, Alexander Stein, devicetree, Sakari Ailus
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-07-28 13:13 ` Krzysztof Kozlowski
@ 2022-07-29 7:07 ` Sakari Ailus
2022-07-29 8:18 ` Laurent Pinchart
0 siblings, 1 reply; 23+ messages in thread
From: Sakari Ailus @ 2022-07-29 7:07 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Alexander Stein, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-07-29 7:07 ` Sakari Ailus
@ 2022-07-29 8:18 ` Laurent Pinchart
2022-08-01 18:07 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Laurent Pinchart @ 2022-07-29 8:18 UTC (permalink / raw)
To: Sakari Ailus
Cc: Krzysztof Kozlowski, Alexander Stein, Paul J . Murphy,
Daniele Alessandrelli, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, linux-media, devicetree, Dave Stevenson,
Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model
2022-07-28 21:10 ` kernel test robot
@ 2022-07-29 8:23 ` Alexander Stein
2022-08-01 12:16 ` Sakari Ailus
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Stein @ 2022-07-29 8:23 UTC (permalink / raw)
To: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski, kernel test robot
Cc: llvm, kbuild-all, linux-media, devicetree, Sakari Ailus
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model
2022-07-29 8:23 ` Alexander Stein
@ 2022-08-01 12:16 ` Sakari Ailus
0 siblings, 0 replies; 23+ messages in thread
From: Sakari Ailus @ 2022-08-01 12:16 UTC (permalink / raw)
To: Alexander Stein
Cc: Paul J . Murphy, Daniele Alessandrelli, Mauro Carvalho Chehab,
Rob Herring, Krzysztof Kozlowski, kernel test robot, llvm,
kbuild-all, linux-media, devicetree
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-07-29 8:18 ` Laurent Pinchart
@ 2022-08-01 18:07 ` Krzysztof Kozlowski
2022-08-01 18:08 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-01 18:07 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: Alexander Stein, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Dave Stevenson, Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-01 18:07 ` Krzysztof Kozlowski
@ 2022-08-01 18:08 ` Krzysztof Kozlowski
2022-08-02 8:23 ` Sakari Ailus
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-01 18:08 UTC (permalink / raw)
To: Laurent Pinchart, Sakari Ailus
Cc: Alexander Stein, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Dave Stevenson, Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-01 18:08 ` Krzysztof Kozlowski
@ 2022-08-02 8:23 ` Sakari Ailus
2022-08-02 8:30 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Sakari Ailus @ 2022-08-02 8:23 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Laurent Pinchart, Alexander Stein, Paul J . Murphy,
Daniele Alessandrelli, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, linux-media, devicetree, Dave Stevenson,
Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-02 8:23 ` Sakari Ailus
@ 2022-08-02 8:30 ` Krzysztof Kozlowski
2022-08-15 11:19 ` Alexander Stein
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-02 8:30 UTC (permalink / raw)
To: Sakari Ailus
Cc: Laurent Pinchart, Alexander Stein, Paul J . Murphy,
Daniele Alessandrelli, Mauro Carvalho Chehab, Rob Herring,
Krzysztof Kozlowski, linux-media, devicetree, Dave Stevenson,
Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-02 8:30 ` Krzysztof Kozlowski
@ 2022-08-15 11:19 ` Alexander Stein
2022-08-16 7:16 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Stein @ 2022-08-15 11:19 UTC (permalink / raw)
To: Sakari Ailus, Krzysztof Kozlowski
Cc: Laurent Pinchart, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Dave Stevenson, Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-15 11:19 ` Alexander Stein
@ 2022-08-16 7:16 ` Krzysztof Kozlowski
2022-08-16 7:21 ` Alexander Stein
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-16 7:16 UTC (permalink / raw)
To: Alexander Stein, Sakari Ailus
Cc: Laurent Pinchart, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Dave Stevenson, Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-16 7:16 ` Krzysztof Kozlowski
@ 2022-08-16 7:21 ` Alexander Stein
2022-08-16 7:35 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Alexander Stein @ 2022-08-16 7:21 UTC (permalink / raw)
To: Sakari Ailus, Krzysztof Kozlowski
Cc: Laurent Pinchart, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Dave Stevenson, Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
2022-08-16 7:21 ` Alexander Stein
@ 2022-08-16 7:35 ` Krzysztof Kozlowski
0 siblings, 0 replies; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-08-16 7:35 UTC (permalink / raw)
To: Alexander Stein, Sakari Ailus
Cc: Laurent Pinchart, Paul J . Murphy, Daniele Alessandrelli,
Mauro Carvalho Chehab, Rob Herring, Krzysztof Kozlowski,
linux-media, devicetree, Dave Stevenson, Naushir Patuck
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible
[not found] ` <166821050429.550668.2828222448343135143@Monstersaurus>
@ 2022-11-24 9:45 ` Alexander Stein
0 siblings, 0 replies; 23+ messages in thread
From: Alexander Stein @ 2022-11-24 9:45 UTC (permalink / raw)
To: Kieran Bingham
Cc: Daniele Alessandrelli, Krzysztof Kozlowski,
Mauro Carvalho Chehab, Paul J. Murphy, Rob Herring, linux-media,
devicetree, Sakari Ailus
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
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2022-11-24 9:45 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-28 13:02 [PATCH v4 0/7] OV9281 support Alexander Stein
2022-07-28 13:02 ` [PATCH v4 1/7] media: i2c: ov9282: remove unused and unset i2c_client member Alexander Stein
2022-07-28 13:02 ` [PATCH v4 2/7] media: dt-bindings: media: Add compatible for ov9281 Alexander Stein
2022-07-28 13:02 ` [PATCH v4 3/7] media: i2c: ov9282: Add ov9281 compatible Alexander Stein
2022-07-28 13:13 ` Krzysztof Kozlowski
2022-07-29 7:07 ` Sakari Ailus
2022-07-29 8:18 ` Laurent Pinchart
2022-08-01 18:07 ` Krzysztof Kozlowski
2022-08-01 18:08 ` Krzysztof Kozlowski
2022-08-02 8:23 ` Sakari Ailus
2022-08-02 8:30 ` Krzysztof Kozlowski
2022-08-15 11:19 ` Alexander Stein
2022-08-16 7:16 ` Krzysztof Kozlowski
2022-08-16 7:21 ` Alexander Stein
2022-08-16 7:35 ` Krzysztof Kozlowski
[not found] ` <166821050429.550668.2828222448343135143@Monstersaurus>
2022-11-24 9:45 ` Alexander Stein
2022-07-28 13:02 ` [PATCH v4 4/7] media: dt-bindings: media: ov9282: Add power supply properties Alexander Stein
2022-07-28 13:02 ` [PATCH v4 5/7] media: i2c: ov9282: Add regulator support Alexander Stein
2022-07-28 13:02 ` [PATCH v4 6/7] media: i2c: ov9282: Set v4l2 subdev name according to sensor model Alexander Stein
2022-07-28 21:10 ` kernel test robot
2022-07-29 8:23 ` Alexander Stein
2022-08-01 12:16 ` Sakari Ailus
2022-07-28 13:02 ` [PATCH v4 7/7] media: i2c: ov9282: Add regmap support Alexander Stein
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).