[PATCH 0/7] media: imx274: miscellaneous improvements

[PATCH 0/7] media: imx274: miscellaneous improvements

[Luca Ceresoli]

Hi,

here's a series of small improvements to the imx274 sensor
driver.

The patches are mostly unrelated to each other. Patch 3 is a fix to
make the subdev name unique. Patches 2 and 6 are small
optimizations. The remaining patches have no functional effect.

Luca


Luca Ceresoli (7):
  media: imx274: rename IMX274_DEFAULT_MODE to IMX274_DEFAULT_BINNING
  media: imx274: rearrange sensor startup register tables
  media: imx274: don't hard-code the subdev name to DRIVER_NAME
  media: imx274: rename frmfmt and format to "mode"
  media: imx274: fix error in function docs
  media: imx274: add helper to read multibyte registers
  media: imx274: switch to SPDX license identifier

 drivers/media/i2c/imx274.c | 165 ++++++++++++++++---------------------
 1 file changed, 72 insertions(+), 93 deletions(-)

-- 
2.17.1

[PATCH 1/7] media: imx274: rename IMX274_DEFAULT_MODE to IMX274_DEFAULT_BINNING

[Luca Ceresoli]

The "mode" has been renamed to "binning" in commit 39dd23dc9d4c
("media: imx274: add cropping support via SELECTION API"), but this
define has not been updated.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index f8c70f1a34fe..4f629e4e53fd 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -76,7 +76,7 @@
  */
 #define IMX274_MIN_EXPOSURE_TIME		(4 * 260 / 72)
 
-#define IMX274_DEFAULT_MODE			IMX274_BINNING_OFF
+#define IMX274_DEFAULT_BINNING			IMX274_BINNING_OFF
 #define IMX274_MAX_WIDTH			(3840)
 #define IMX274_MAX_HEIGHT			(2160)
 #define IMX274_MAX_FRAME_RATE			(120)
@@ -1871,7 +1871,7 @@ static int imx274_probe(struct i2c_client *client,
 	mutex_init(&imx274->lock);
 
 	/* initialize format */
-	imx274->mode = &imx274_formats[IMX274_DEFAULT_MODE];
+	imx274->mode = &imx274_formats[IMX274_DEFAULT_BINNING];
 	imx274->crop.width = IMX274_MAX_WIDTH;
 	imx274->crop.height = IMX274_MAX_HEIGHT;
 	imx274->format.width = imx274->crop.width / imx274->mode->bin_ratio;
-- 
2.17.1

[PATCH 2/7] media: imx274: rearrange sensor startup register tables

[Luca Ceresoli]

Rearrange the imx274_start_<N> register tables to better match the
datasheet and slightly simplify code:

 - collapes tables 1 and 2, they are applied one after each other and
   together they implement the fixed part 1 of the startup procedure
   in the datasheet
 - while there, cleanup comments
 - rename tables 3 and 4 -> 2 and 3, coherently with the datasheet

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 36 +++++++++++++-----------------------
 1 file changed, 13 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 4f629e4e53fd..9b524de08470 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -349,20 +349,14 @@ static const struct reg_8 imx274_mode5_1280x720_raw10[] = {
  */
 static const struct reg_8 imx274_start_1[] = {
 	{IMX274_STANDBY_REG, 0x12},
-	{IMX274_TABLE_END, 0x00}
-};
 
-/*
- * imx274 second step register configuration for
- * starting stream
- */
-static const struct reg_8 imx274_start_2[] = {
-	{0x3120, 0xF0}, /* clock settings */
-	{0x3121, 0x00}, /* clock settings */
-	{0x3122, 0x02}, /* clock settings */
-	{0x3129, 0x9C}, /* clock settings */
-	{0x312A, 0x02}, /* clock settings */
-	{0x312D, 0x02}, /* clock settings */
+	/* PLRD: clock settings */
+	{0x3120, 0xF0},
+	{0x3121, 0x00},
+	{0x3122, 0x02},
+	{0x3129, 0x9C},
+	{0x312A, 0x02},
+	{0x312D, 0x02},
 
 	{0x310B, 0x00},
 
@@ -407,20 +401,20 @@ static const struct reg_8 imx274_start_2[] = {
 };
 
 /*
- * imx274 third step register configuration for
+ * imx274 second step register configuration for
  * starting stream
  */
-static const struct reg_8 imx274_start_3[] = {
+static const struct reg_8 imx274_start_2[] = {
 	{IMX274_STANDBY_REG, 0x00},
 	{0x303E, 0x02}, /* SYS_MODE = 2 */
 	{IMX274_TABLE_END, 0x00}
 };
 
 /*
- * imx274 forth step register configuration for
+ * imx274 third step register configuration for
  * starting stream
  */
-static const struct reg_8 imx274_start_4[] = {
+static const struct reg_8 imx274_start_3[] = {
 	{0x30F4, 0x00},
 	{0x3018, 0xA2}, /* XHS VHS OUTUPT */
 	{IMX274_TABLE_END, 0x00}
@@ -708,10 +702,6 @@ static int imx274_mode_regs(struct stimx274 *priv)
 	if (err)
 		return err;
 
-	err = imx274_write_table(priv, imx274_start_2);
-	if (err)
-		return err;
-
 	err = imx274_write_table(priv, priv->mode->init_regs);
 
 	return err;
@@ -733,7 +723,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 	 * give it 1 extra ms for margin
 	 */
 	msleep_range(11);
-	err = imx274_write_table(priv, imx274_start_3);
+	err = imx274_write_table(priv, imx274_start_2);
 	if (err)
 		return err;
 
@@ -743,7 +733,7 @@ static int imx274_start_stream(struct stimx274 *priv)
 	 * give it 1 extra ms for margin
 	 */
 	msleep_range(8);
-	err = imx274_write_table(priv, imx274_start_4);
+	err = imx274_write_table(priv, imx274_start_3);
 	if (err)
 		return err;
 
-- 
2.17.1

[PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Luca Ceresoli]

Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
it non-unique and less informative.

Let the driver use the default name from i2c, e.g. "IMX274 2-001a".

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 9b524de08470..570706695ca7 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
 	imx274->client = client;
 	sd = &imx274->sd;
 	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
-	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
 
 	/* initialize subdev media pad */
-- 
2.17.1

[PATCH 4/7] media: imx274: rename frmfmt and format to "mode"

[Luca Ceresoli]

A mix of "mode", "format" and "frmfmt" is used to refer to the sensor
readout mode. Use the term "mode" for all of them. Now "format" is
only used in the V4L2 meaning.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 570706695ca7..6572d5728791 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -178,7 +178,7 @@ enum imx274_binning {
  * @nocpiop: Number of clocks per internal offset period (see "Integration Time
  *           in Each Readout Drive Mode (CSI-2)" in the datasheet)
  */
-struct imx274_frmfmt {
+struct imx274_mode {
 	const struct reg_8 *init_regs;
 	unsigned int bin_ratio;
 	int min_frame_len;
@@ -453,7 +453,7 @@ static const struct reg_8 imx274_tp_regs[] = {
 };
 
 /* nocpiop happens to be the same number for the implemented modes */
-static const struct imx274_frmfmt imx274_formats[] = {
+static const struct imx274_mode imx274_modes[] = {
 	{
 		/* mode 1, 4K */
 		.bin_ratio = 1,
@@ -526,7 +526,7 @@ struct stimx274 {
 	struct regmap *regmap;
 	struct gpio_desc *reset_gpio;
 	struct mutex lock; /* mutex lock for operations */
-	const struct imx274_frmfmt *mode;
+	const struct imx274_mode *mode;
 };
 
 #define IMX274_ROUND(dim, step, flags)			\
@@ -871,7 +871,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
 	const struct v4l2_rect *cur_crop;
 	struct v4l2_mbus_framefmt *tgt_fmt;
 	unsigned int i;
-	const struct imx274_frmfmt *best_mode = &imx274_formats[0];
+	const struct imx274_mode *best_mode = &imx274_modes[0];
 	int best_goodness = INT_MIN;
 
 	if (which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -882,8 +882,8 @@ static int __imx274_change_compose(struct stimx274 *imx274,
 		tgt_fmt = &imx274->format;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(imx274_formats); i++) {
-		unsigned int ratio = imx274_formats[i].bin_ratio;
+	for (i = 0; i < ARRAY_SIZE(imx274_modes); i++) {
+		unsigned int ratio = imx274_modes[i].bin_ratio;
 
 		int goodness = imx274_binning_goodness(
 			imx274,
@@ -893,7 +893,7 @@ static int __imx274_change_compose(struct stimx274 *imx274,
 
 		if (goodness >= best_goodness) {
 			best_goodness = goodness;
-			best_mode = &imx274_formats[i];
+			best_mode = &imx274_modes[i];
 		}
 	}
 
@@ -1313,7 +1313,7 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 
 	dev_dbg(&imx274->client->dev, "%s : %s, mode index = %td\n", __func__,
 		on ? "Stream Start" : "Stream Stop",
-		imx274->mode - &imx274_formats[0]);
+		imx274->mode - &imx274_modes[0]);
 
 	mutex_lock(&imx274->lock);
 
@@ -1861,7 +1861,7 @@ static int imx274_probe(struct i2c_client *client,
 	mutex_init(&imx274->lock);
 
 	/* initialize format */
-	imx274->mode = &imx274_formats[IMX274_DEFAULT_BINNING];
+	imx274->mode = &imx274_modes[IMX274_DEFAULT_BINNING];
 	imx274->crop.width = IMX274_MAX_WIDTH;
 	imx274->crop.height = IMX274_MAX_HEIGHT;
 	imx274->format.width = imx274->crop.width / imx274->mode->bin_ratio;
-- 
2.17.1

[PATCH 5/7] media: imx274: fix error in function docs

[Luca Ceresoli]

This parameter holds the number of bytes, not bits.

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 6572d5728791..07bc41f537c5 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -668,7 +668,7 @@ static inline int imx274_write_reg(struct stimx274 *priv, u16 addr, u8 val)
  * @addr: Address of the LSB register.  Other registers must be
  *        consecutive, least-to-most significant.
  * @val: Value to be written to the register (cpu endianness)
- * @nbytes: Number of bits to write (range: [1..3])
+ * @nbytes: Number of bytes to write (range: [1..3])
  */
 static int imx274_write_mbreg(struct stimx274 *priv, u16 addr, u32 val,
 			      size_t nbytes)
-- 
2.17.1

[PATCH 6/7] media: imx274: add helper to read multibyte registers

[Luca Ceresoli]

Currently 2-bytes and 3-bytes registers are read one byte at a time,
doing the needed shift & mask each time.

Replace all of this code by a unique helper function that calls
regmap_bulk_read(), which has two advantages:
 - reads all the bytes in a unique I2C transaction
 - simplifies code to read multibyte registers

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 93 +++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 46 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 07bc41f537c5..783277068b45 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -659,6 +659,41 @@ static inline int imx274_write_reg(struct stimx274 *priv, u16 addr, u8 val)
 	return err;
 }
 
+/**
+ * Read a multibyte register.
+ *
+ * Uses a bulk read where possible.
+ *
+ * @priv: Pointer to device structure
+ * @addr: Address of the LSB register.  Other registers must be
+ *        consecutive, least-to-most significant.
+ * @val: Pointer to store the register value (cpu endianness)
+ * @nbytes: Number of bytes to read (range: [1..3]).
+ *          Other bytes are zet to 0.
+ *
+ * Return: 0 on success, errors otherwise
+ */
+static int imx274_read_mbreg(struct stimx274 *priv, u16 addr, u32 *val,
+			     size_t nbytes)
+{
+	__le32 val_le = 0;
+	int err;
+
+	err = regmap_bulk_read(priv->regmap, addr, &val_le, nbytes);
+	if (err) {
+		dev_err(&priv->client->dev,
+			"%s : i2c bulk read failed, %x (%zu bytes)\n",
+			__func__, addr, nbytes);
+	} else {
+		*val = le32_to_cpu(val_le);
+		dev_dbg(&priv->client->dev,
+			"%s : addr 0x%x, val=0x%x (%zu bytes)\n",
+			__func__, addr, *val, nbytes);
+	}
+
+	return err;
+}
+
 /**
  * Write a multibyte register.
  *
@@ -1377,37 +1412,17 @@ static int imx274_s_stream(struct v4l2_subdev *sd, int on)
 static int imx274_get_frame_length(struct stimx274 *priv, u32 *val)
 {
 	int err;
-	u16 svr;
+	u32 svr;
 	u32 vmax;
-	u8 reg_val[3];
 
-	/* svr */
-	err = imx274_read_reg(priv, IMX274_SVR_REG_LSB, &reg_val[0]);
+	err = imx274_read_mbreg(priv, IMX274_SVR_REG_LSB, &svr, 2);
 	if (err)
 		goto fail;
 
-	err = imx274_read_reg(priv, IMX274_SVR_REG_MSB, &reg_val[1]);
+	err = imx274_read_mbreg(priv, IMX274_VMAX_REG_3, &vmax, 3);
 	if (err)
 		goto fail;
 
-	svr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
-
-	/* vmax */
-	err = imx274_read_reg(priv, IMX274_VMAX_REG_3, &reg_val[0]);
-	if (err)
-		goto fail;
-
-	err = imx274_read_reg(priv, IMX274_VMAX_REG_2, &reg_val[1]);
-	if (err)
-		goto fail;
-
-	err = imx274_read_reg(priv, IMX274_VMAX_REG_1, &reg_val[2]);
-	if (err)
-		goto fail;
-
-	vmax = ((reg_val[2] & IMX274_MASK_LSB_3_BITS) << IMX274_SHIFT_16_BITS)
-		+ (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
-
 	*val = vmax * (svr + 1);
 
 	return 0;
@@ -1588,8 +1603,7 @@ static int imx274_set_coarse_time(struct stimx274 *priv, u32 *val)
 static int imx274_set_exposure(struct stimx274 *priv, int val)
 {
 	int err;
-	u16 hmax;
-	u8 reg_val[2];
+	u32 hmax;
 	u32 coarse_time; /* exposure time in unit of line (HMAX)*/
 
 	dev_dbg(&priv->client->dev,
@@ -1597,14 +1611,10 @@ static int imx274_set_exposure(struct stimx274 *priv, int val)
 
 	/* step 1: convert input exposure_time (val) into number of 1[HMAX] */
 
-	/* obtain HMAX value */
-	err = imx274_read_reg(priv, IMX274_HMAX_REG_LSB, &reg_val[0]);
-	if (err)
-		goto fail;
-	err = imx274_read_reg(priv, IMX274_HMAX_REG_MSB, &reg_val[1]);
+	err = imx274_read_mbreg(priv, IMX274_HMAX_REG_LSB, &hmax, 2);
 	if (err)
 		goto fail;
-	hmax = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
+
 	if (hmax == 0) {
 		err = -EINVAL;
 		goto fail;
@@ -1739,9 +1749,8 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 {
 	int err;
 	u32 frame_length, req_frame_rate;
-	u16 svr;
-	u16 hmax;
-	u8 reg_val[2];
+	u32 svr;
+	u32 hmax;
 
 	dev_dbg(&priv->client->dev, "%s: input frame interval = %d / %d",
 		__func__, frame_interval.numerator,
@@ -1769,25 +1778,17 @@ static int imx274_set_frame_interval(struct stimx274 *priv,
 	 * frame_length (i.e. VMAX) = (frame_interval) x 72M /(SVR+1) / HMAX
 	 */
 
-	/* SVR */
-	err = imx274_read_reg(priv, IMX274_SVR_REG_LSB, &reg_val[0]);
+	err = imx274_read_mbreg(priv, IMX274_SVR_REG_LSB, &svr, 2);
 	if (err)
 		goto fail;
-	err = imx274_read_reg(priv, IMX274_SVR_REG_MSB, &reg_val[1]);
-	if (err)
-		goto fail;
-	svr = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
+
 	dev_dbg(&priv->client->dev,
 		"%s : register SVR = %d\n", __func__, svr);
 
-	/* HMAX */
-	err = imx274_read_reg(priv, IMX274_HMAX_REG_LSB, &reg_val[0]);
+	err = imx274_read_mbreg(priv, IMX274_HMAX_REG_LSB, &hmax, 2);
 	if (err)
 		goto fail;
-	err = imx274_read_reg(priv, IMX274_HMAX_REG_MSB, &reg_val[1]);
-	if (err)
-		goto fail;
-	hmax = (reg_val[1] << IMX274_SHIFT_8_BITS) + reg_val[0];
+
 	dev_dbg(&priv->client->dev,
 		"%s : register HMAX = %d\n", __func__, hmax);
 
-- 
2.17.1

[PATCH 7/7] media: imx274: switch to SPDX license identifier

[Luca Ceresoli]

Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
---
 drivers/media/i2c/imx274.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
index 783277068b45..11c69281692e 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0
 /*
  * imx274.c - IMX274 CMOS Image Sensor driver
  *
@@ -6,18 +7,6 @@
  * Leon Luo <leonl@leopardimaging.com>
  * Edwin Zou <edwinz@leopardimaging.com>
  * Luca Ceresoli <luca@lucaceresoli.net>
- *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms and conditions of the GNU General Public License,
- * version 2, as published by the Free Software Foundation.
- *
- * This program is distributed in the hope it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
- * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
- * more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #include <linux/clk.h>
-- 
2.17.1

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Sakari Ailus]

Hi Luca,

On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
> it non-unique and less informative.
> 
> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> ---
>  drivers/media/i2c/imx274.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> index 9b524de08470..570706695ca7 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
>  	imx274->client = client;
>  	sd = &imx274->sd;
>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>  
>  	/* initialize subdev media pad */

This ends up changing the entity as well as the sub-device name which may
well break applications. On the other hand, you currently can't have more
than one of these devices on a media device complex due to the name being
specific to a driver, not the device.

An option avoiding that would be to let the user choose by e.g. through a
Kconfig option would avoid having to address that, but I really hate adding
such options.

I wonder what others think. If anyone ever needs to add another on a board
so that it ends up being the part of the same media device complex
(likely), then changing the name now rather than later would be the least
pain. In this case I'd be leaning (slightly) towards accepting the patch
and hoping there wouldn't be any fallout... I don't see any board (DT)
containing imx274, at least not in the upstream kernel.

Cc Hans and Laurent.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Luca Ceresoli]

Hi Sakari,

On 25/08/2018 16:49, Sakari Ailus wrote:
> Hi Luca,
> 
> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
>> it non-unique and less informative.
>>
>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>> ---
>>  drivers/media/i2c/imx274.c | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index 9b524de08470..570706695ca7 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
>>  	imx274->client = client;
>>  	sd = &imx274->sd;
>>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
>> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
>>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>>  
>>  	/* initialize subdev media pad */
> 
> This ends up changing the entity as well as the sub-device name which may
> well break applications.

Right, unfortunately.

> On the other hand, you currently can't have more
> than one of these devices on a media device complex due to the name being
> specific to a driver, not the device.
>
> An option avoiding that would be to let the user choose by e.g. through a
> Kconfig option would avoid having to address that, but I really hate adding
> such options.

I agree adding a Kconfig option just for this would be very annoying.
However I think the issue affects a few other drivers (sr030pc30.c and
s5c73m3-core.c apparently), thus maybe one option could serve them all.

> I wonder what others think. If anyone ever needs to add another on a board
> so that it ends up being the part of the same media device complex
> (likely), then changing the name now rather than later would be the least
> pain. In this case I'd be leaning (slightly) towards accepting the patch
> and hoping there wouldn't be any fallout... I don't see any board (DT)
> containing imx274, at least not in the upstream kernel.

I'll be OK with either decision. Should we keep it as is, then I think a
comment before that line would be appropriate to clarify it's not
correct but it is kept for backward userspace compatibility. This would
help avoid new driver writers doing the same mistake, and prevent other
people to send another patch like mine.

Regards,
-- 
Luca

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Philippe De Muyter]

Hi Sakari and Luca,
On Sun, Aug 26, 2018 at 10:41:13PM +0200, Luca Ceresoli wrote:
> Hi Sakari,
> 
> On 25/08/2018 16:49, Sakari Ailus wrote:
> > Hi Luca,
> > 
> > On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
> >> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
> >> it non-unique and less informative.
> >>
> >> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
> >>
...
> > 
> > This ends up changing the entity as well as the sub-device name which may
> > well break applications.
> 
> Right, unfortunately.
> 
> > On the other hand, you currently can't have more
> > than one of these devices on a media device complex due to the name being
> > specific to a driver, not the device.
> >
> > An option avoiding that would be to let the user choose by e.g. through a
> > Kconfig option would avoid having to address that, but I really hate adding
> > such options.
> 
> I agree adding a Kconfig option just for this would be very annoying.
> However I think the issue affects a few other drivers (sr030pc30.c and
> s5c73m3-core.c apparently), thus maybe one option could serve them all.
> 
> > I wonder what others think. If anyone ever needs to add another on a board
> > so that it ends up being the part of the same media device complex
> > (likely), then changing the name now rather than later would be the least
> > pain. In this case I'd be leaning (slightly) towards accepting the patch
> > and hoping there wouldn't be any fallout... I don't see any board (DT)
> > containing imx274, at least not in the upstream kernel.
> 
> I'll be OK with either decision. Should we keep it as is, then I think a
> comment before that line would be appropriate to clarify it's not
> correct but it is kept for backward userspace compatibility. This would
> help avoid new driver writers doing the same mistake, and prevent other
> people to send another patch like mine.

Would it be acceptable to accept Luca's patch but add a dev_info message
indicating the old and the new name, so that at least if the user notices
a problem he'll find an informative message helping him to fix his config ?
This dev_info message could even be standardized to be usable for other
drivers with only the names changed.

Philippe
-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Hans Verkuil]

On 26/08/18 22:41, Luca Ceresoli wrote:
> Hi Sakari,
> 
> On 25/08/2018 16:49, Sakari Ailus wrote:
>> Hi Luca,
>>
>> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
>>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
>>> it non-unique and less informative.
>>>
>>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
>>>
>>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
>>> ---
>>>  drivers/media/i2c/imx274.c | 1 -
>>>  1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>>> index 9b524de08470..570706695ca7 100644
>>> --- a/drivers/media/i2c/imx274.c
>>> +++ b/drivers/media/i2c/imx274.c
>>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
>>>  	imx274->client = client;
>>>  	sd = &imx274->sd;
>>>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
>>> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
>>>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
>>>  
>>>  	/* initialize subdev media pad */
>>
>> This ends up changing the entity as well as the sub-device name which may
>> well break applications.
> 
> Right, unfortunately.
> 
>> On the other hand, you currently can't have more
>> than one of these devices on a media device complex due to the name being
>> specific to a driver, not the device.
>>
>> An option avoiding that would be to let the user choose by e.g. through a
>> Kconfig option would avoid having to address that, but I really hate adding
>> such options.
> 
> I agree adding a Kconfig option just for this would be very annoying.
> However I think the issue affects a few other drivers (sr030pc30.c and
> s5c73m3-core.c apparently), thus maybe one option could serve them all.
> 
>> I wonder what others think. If anyone ever needs to add another on a board
>> so that it ends up being the part of the same media device complex
>> (likely), then changing the name now rather than later would be the least
>> pain. In this case I'd be leaning (slightly) towards accepting the patch
>> and hoping there wouldn't be any fallout... I don't see any board (DT)
>> containing imx274, at least not in the upstream kernel.
> 
> I'll be OK with either decision. Should we keep it as is, then I think a
> comment before that line would be appropriate to clarify it's not
> correct but it is kept for backward userspace compatibility. This would
> help avoid new driver writers doing the same mistake, and prevent other
> people to send another patch like mine.

In this end, this is a driver bug. I would just fix this, but add a comment
that states the old name and why it was changed. No need for a dev_info
IMHO.

It would be nice if you can check if the same mistake is made in other drivers,
and update those as well. It's easier if this is all done at the same time.

Regards,

	Hans

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Philippe De Muyter]

Hi Hans, Sakari and Luca

On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote:
> On 26/08/18 22:41, Luca Ceresoli wrote:
> > Hi Sakari,
> > 
> > On 25/08/2018 16:49, Sakari Ailus wrote:
> >> Hi Luca,
> >>
> >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
> >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
> >>> it non-unique and less informative.
> >>>
> >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
> >>>
> >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> >>> ---
> >>>  drivers/media/i2c/imx274.c | 1 -
> >>>  1 file changed, 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> >>> index 9b524de08470..570706695ca7 100644
> >>> --- a/drivers/media/i2c/imx274.c
> >>> +++ b/drivers/media/i2c/imx274.c
> >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
> >>>  	imx274->client = client;
> >>>  	sd = &imx274->sd;
> >>>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
> >>> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> >>>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> >>>  
> >>>  	/* initialize subdev media pad */
> >>
> >> This ends up changing the entity as well as the sub-device name which may
> >> well break applications.
> > 
> > Right, unfortunately.
> > 
> >> On the other hand, you currently can't have more
> >> than one of these devices on a media device complex due to the name being
> >> specific to a driver, not the device.
> >>
> >> An option avoiding that would be to let the user choose by e.g. through a
> >> Kconfig option would avoid having to address that, but I really hate adding
> >> such options.
> > 
> > I agree adding a Kconfig option just for this would be very annoying.
> > However I think the issue affects a few other drivers (sr030pc30.c and
> > s5c73m3-core.c apparently), thus maybe one option could serve them all.
> > 
> >> I wonder what others think. If anyone ever needs to add another on a board
> >> so that it ends up being the part of the same media device complex
> >> (likely), then changing the name now rather than later would be the least
> >> pain. In this case I'd be leaning (slightly) towards accepting the patch
> >> and hoping there wouldn't be any fallout... I don't see any board (DT)
> >> containing imx274, at least not in the upstream kernel.
> > 
> > I'll be OK with either decision. Should we keep it as is, then I think a
> > comment before that line would be appropriate to clarify it's not
> > correct but it is kept for backward userspace compatibility. This would
> > help avoid new driver writers doing the same mistake, and prevent other
> > people to send another patch like mine.
> 
> In this end, this is a driver bug. I would just fix this, but add a comment
> that states the old name and why it was changed. No need for a dev_info
> IMHO.
> 
> It would be nice if you can check if the same mistake is made in other drivers,
> and update those as well. It's easier if this is all done at the same time.
> 

Then we should probably also apply the following patch I submitted :

"media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
	https://patchwork.kernel.org/patch/10553035/

and perhaps

"media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
	https://patchwork.kernel.org/patch/10553037/

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Sakari Ailus]

Hi Philippe,

On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote:
> Hi Hans, Sakari and Luca
> 
> On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote:
> > On 26/08/18 22:41, Luca Ceresoli wrote:
> > > Hi Sakari,
> > > 
> > > On 25/08/2018 16:49, Sakari Ailus wrote:
> > >> Hi Luca,
> > >>
> > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
> > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
> > >>> it non-unique and less informative.
> > >>>
> > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
> > >>>
> > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> > >>> ---
> > >>>  drivers/media/i2c/imx274.c | 1 -
> > >>>  1 file changed, 1 deletion(-)
> > >>>
> > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > >>> index 9b524de08470..570706695ca7 100644
> > >>> --- a/drivers/media/i2c/imx274.c
> > >>> +++ b/drivers/media/i2c/imx274.c
> > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
> > >>>  	imx274->client = client;
> > >>>  	sd = &imx274->sd;
> > >>>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
> > >>> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > >>>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > >>>  
> > >>>  	/* initialize subdev media pad */
> > >>
> > >> This ends up changing the entity as well as the sub-device name which may
> > >> well break applications.
> > > 
> > > Right, unfortunately.
> > > 
> > >> On the other hand, you currently can't have more
> > >> than one of these devices on a media device complex due to the name being
> > >> specific to a driver, not the device.
> > >>
> > >> An option avoiding that would be to let the user choose by e.g. through a
> > >> Kconfig option would avoid having to address that, but I really hate adding
> > >> such options.
> > > 
> > > I agree adding a Kconfig option just for this would be very annoying.
> > > However I think the issue affects a few other drivers (sr030pc30.c and
> > > s5c73m3-core.c apparently), thus maybe one option could serve them all.
> > > 
> > >> I wonder what others think. If anyone ever needs to add another on a board
> > >> so that it ends up being the part of the same media device complex
> > >> (likely), then changing the name now rather than later would be the least
> > >> pain. In this case I'd be leaning (slightly) towards accepting the patch
> > >> and hoping there wouldn't be any fallout... I don't see any board (DT)
> > >> containing imx274, at least not in the upstream kernel.
> > > 
> > > I'll be OK with either decision. Should we keep it as is, then I think a
> > > comment before that line would be appropriate to clarify it's not
> > > correct but it is kept for backward userspace compatibility. This would
> > > help avoid new driver writers doing the same mistake, and prevent other
> > > people to send another patch like mine.
> > 
> > In this end, this is a driver bug. I would just fix this, but add a comment
> > that states the old name and why it was changed. No need for a dev_info
> > IMHO.
> > 
> > It would be nice if you can check if the same mistake is made in other drivers,
> > and update those as well. It's easier if this is all done at the same time.
> > 
> 
> Then we should probably also apply the following patch I submitted :
> 
> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
> 	https://patchwork.kernel.org/patch/10553035/
> 
> and perhaps
> 
> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
> 	https://patchwork.kernel.org/patch/10553037/

The problem with this patch is that the existing naming scheme is very
similar while the new one offers no tangible benefits apart from being in
line with the rest of the kernel. That's still not a benefit for uAPI:
changing the name is certain to break user space applications.

I posted another patch on which I'm somewhat uncertain applying it would be
only a good thing:

<URL:https://patchwork.linuxtv.org/patch/51791/>

But it would fix an actual bug.

I maintain that the information (device name as it's seen by the rest of
the system) would be good to have in device properties; Hans posted a
patchset on that some time ago.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Philippe De Muyter]

Hi Sakari,

On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote:
> Hi Philippe,
> 
> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote:
> > Hi Hans, Sakari and Luca
> > 
> > On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote:
> > > On 26/08/18 22:41, Luca Ceresoli wrote:
> > > > Hi Sakari,
> > > > 
> > > > On 25/08/2018 16:49, Sakari Ailus wrote:
> > > >> Hi Luca,
> > > >>
> > > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
> > > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
> > > >>> it non-unique and less informative.
> > > >>>
> > > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
> > > >>>
> > > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > >>> ---
> > > >>>  drivers/media/i2c/imx274.c | 1 -
> > > >>>  1 file changed, 1 deletion(-)
> > > >>>
> > > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > > >>> index 9b524de08470..570706695ca7 100644
> > > >>> --- a/drivers/media/i2c/imx274.c
> > > >>> +++ b/drivers/media/i2c/imx274.c
> > > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
> > > >>>  	imx274->client = client;
> > > >>>  	sd = &imx274->sd;
> > > >>>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
> > > >>> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > > >>>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > > >>>  
> > > >>>  	/* initialize subdev media pad */
> > > >>
> > > >> This ends up changing the entity as well as the sub-device name which may
> > > >> well break applications.
> > > > 
> > > > Right, unfortunately.
> > > > 
> > > >> On the other hand, you currently can't have more
> > > >> than one of these devices on a media device complex due to the name being
> > > >> specific to a driver, not the device.
> > > >>
> > > >> An option avoiding that would be to let the user choose by e.g. through a
> > > >> Kconfig option would avoid having to address that, but I really hate adding
> > > >> such options.
> > > > 
> > > > I agree adding a Kconfig option just for this would be very annoying.
> > > > However I think the issue affects a few other drivers (sr030pc30.c and
> > > > s5c73m3-core.c apparently), thus maybe one option could serve them all.
> > > > 
> > > >> I wonder what others think. If anyone ever needs to add another on a board
> > > >> so that it ends up being the part of the same media device complex
> > > >> (likely), then changing the name now rather than later would be the least
> > > >> pain. In this case I'd be leaning (slightly) towards accepting the patch
> > > >> and hoping there wouldn't be any fallout... I don't see any board (DT)
> > > >> containing imx274, at least not in the upstream kernel.
> > > > 
> > > > I'll be OK with either decision. Should we keep it as is, then I think a
> > > > comment before that line would be appropriate to clarify it's not
> > > > correct but it is kept for backward userspace compatibility. This would
> > > > help avoid new driver writers doing the same mistake, and prevent other
> > > > people to send another patch like mine.
> > > 
> > > In this end, this is a driver bug. I would just fix this, but add a comment
> > > that states the old name and why it was changed. No need for a dev_info
> > > IMHO.
> > > 
> > > It would be nice if you can check if the same mistake is made in other drivers,
> > > and update those as well. It's easier if this is all done at the same time.
> > > 
> > 
> > Then we should probably also apply the following patch I submitted :
> > 
> > "media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
> > 	https://patchwork.kernel.org/patch/10553035/
> > 
> > and perhaps
> > 
> > "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
> > 	https://patchwork.kernel.org/patch/10553037/
> 
> The problem with this patch is that the existing naming scheme is very
> similar while the new one offers no tangible benefits apart from being in
> line with the rest of the kernel. That's still not a benefit for uAPI:
> changing the name is certain to break user space applications.

I agree with you on the patch for v4l2_i2c_subdev_init (I wrote 'perhaps'),
but you don't say anything on the one about v4l2_spi_subdev_init :), which
fixes an actual bug.  I have 2 identical SPI-controlled sensors on the
same board, and without my patch they get the same subdev name.  Of course,
I could fix that in the sensor driver itself, but that's not what we want,
or do we ?

Best Regards

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Sakari Ailus]

On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote:
> Hi Sakari,
> 
> On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote:
> > Hi Philippe,
> > 
> > On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote:
> > > Hi Hans, Sakari and Luca
> > > 
> > > On Tue, Aug 28, 2018 at 11:22:28AM +0200, Hans Verkuil wrote:
> > > > On 26/08/18 22:41, Luca Ceresoli wrote:
> > > > > Hi Sakari,
> > > > > 
> > > > > On 25/08/2018 16:49, Sakari Ailus wrote:
> > > > >> Hi Luca,
> > > > >>
> > > > >> On Fri, Aug 24, 2018 at 06:35:21PM +0200, Luca Ceresoli wrote:
> > > > >>> Forcibly setting the subdev name to DRIVER_NAME (i.e. "IMX274") makes
> > > > >>> it non-unique and less informative.
> > > > >>>
> > > > >>> Let the driver use the default name from i2c, e.g. "IMX274 2-001a".
> > > > >>>
> > > > >>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> > > > >>> ---
> > > > >>>  drivers/media/i2c/imx274.c | 1 -
> > > > >>>  1 file changed, 1 deletion(-)
> > > > >>>
> > > > >>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
> > > > >>> index 9b524de08470..570706695ca7 100644
> > > > >>> --- a/drivers/media/i2c/imx274.c
> > > > >>> +++ b/drivers/media/i2c/imx274.c
> > > > >>> @@ -1885,7 +1885,6 @@ static int imx274_probe(struct i2c_client *client,
> > > > >>>  	imx274->client = client;
> > > > >>>  	sd = &imx274->sd;
> > > > >>>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
> > > > >>> -	strlcpy(sd->name, DRIVER_NAME, sizeof(sd->name));
> > > > >>>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> > > > >>>  
> > > > >>>  	/* initialize subdev media pad */
> > > > >>
> > > > >> This ends up changing the entity as well as the sub-device name which may
> > > > >> well break applications.
> > > > > 
> > > > > Right, unfortunately.
> > > > > 
> > > > >> On the other hand, you currently can't have more
> > > > >> than one of these devices on a media device complex due to the name being
> > > > >> specific to a driver, not the device.
> > > > >>
> > > > >> An option avoiding that would be to let the user choose by e.g. through a
> > > > >> Kconfig option would avoid having to address that, but I really hate adding
> > > > >> such options.
> > > > > 
> > > > > I agree adding a Kconfig option just for this would be very annoying.
> > > > > However I think the issue affects a few other drivers (sr030pc30.c and
> > > > > s5c73m3-core.c apparently), thus maybe one option could serve them all.
> > > > > 
> > > > >> I wonder what others think. If anyone ever needs to add another on a board
> > > > >> so that it ends up being the part of the same media device complex
> > > > >> (likely), then changing the name now rather than later would be the least
> > > > >> pain. In this case I'd be leaning (slightly) towards accepting the patch
> > > > >> and hoping there wouldn't be any fallout... I don't see any board (DT)
> > > > >> containing imx274, at least not in the upstream kernel.
> > > > > 
> > > > > I'll be OK with either decision. Should we keep it as is, then I think a
> > > > > comment before that line would be appropriate to clarify it's not
> > > > > correct but it is kept for backward userspace compatibility. This would
> > > > > help avoid new driver writers doing the same mistake, and prevent other
> > > > > people to send another patch like mine.
> > > > 
> > > > In this end, this is a driver bug. I would just fix this, but add a comment
> > > > that states the old name and why it was changed. No need for a dev_info
> > > > IMHO.
> > > > 
> > > > It would be nice if you can check if the same mistake is made in other drivers,
> > > > and update those as well. It's easier if this is all done at the same time.
> > > > 
> > > 
> > > Then we should probably also apply the following patch I submitted :
> > > 
> > > "media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
> > > 	https://patchwork.kernel.org/patch/10553035/
> > > 
> > > and perhaps
> > > 
> > > "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
> > > 	https://patchwork.kernel.org/patch/10553037/
> > 
> > The problem with this patch is that the existing naming scheme is very
> > similar while the new one offers no tangible benefits apart from being in
> > line with the rest of the kernel. That's still not a benefit for uAPI:
> > changing the name is certain to break user space applications.
> 
> I agree with you on the patch for v4l2_i2c_subdev_init (I wrote 'perhaps'),
> but you don't say anything on the one about v4l2_spi_subdev_init :), which
> fixes an actual bug.  I have 2 identical SPI-controlled sensors on the
> same board, and without my patch they get the same subdev name.  Of course,
> I could fix that in the sensor driver itself, but that's not what we want,
> or do we ?

Good point. I missed the naming of the SPI devices ignored any bus
information there. I'm rather inclined towards taking the SPI patch. Hans,
Mauro, Laurent; any opinion on that?

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Laurent Pinchart]

Hello,

On Wednesday, 29 August 2018 14:38:43 EEST Sakari Ailus wrote:
> On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote:
> > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote:
> >> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote:

[snip]

> >>> Then we should probably also apply the following patch I submitted :
> >>> 
> >>> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
> >>> 
> >>> 	https://patchwork.kernel.org/patch/10553035/
> >>> 
> >>> and perhaps
> >>> 
> >>> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
> >>> 
> >>> 	https://patchwork.kernel.org/patch/10553037/
> >> 
> >> The problem with this patch is that the existing naming scheme is very
> >> similar while the new one offers no tangible benefits apart from being
> >> in line with the rest of the kernel. That's still not a benefit for uAPI:
> >> changing the name is certain to break user space applications.
> > 
> > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote
> > 'perhaps'), but you don't say anything on the one about
> > v4l2_spi_subdev_init :), which fixes an actual bug.  I have 2 identical
> > SPI-controlled sensors on the same board, and without my patch they get
> > the same subdev name.  Of course, I could fix that in the sensor driver
> > itself, but that's not what we want, or do we ?
> 
> Good point. I missed the naming of the SPI devices ignored any bus
> information there. I'm rather inclined towards taking the SPI patch. Hans,
> Mauro, Laurent; any opinion on that?

I agree that the SPI patch makes sense, I think we should take it.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Philippe De Muyter]

Hi.

On Thu, Aug 30, 2018 at 12:23:23AM +0300, Laurent Pinchart wrote:
> Hello,
> 
> On Wednesday, 29 August 2018 14:38:43 EEST Sakari Ailus wrote:
> > On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote:
> > > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote:
> > >> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote:
> 
> [snip]
> 
> > >>> Then we should probably also apply the following patch I submitted :
> > >>> 
> > >>> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
> > >>> 
> > >>> 	https://patchwork.kernel.org/patch/10553035/
> > >>> 
> > >>> and perhaps
> > >>> 
> > >>> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
> > >>> 
> > >>> 	https://patchwork.kernel.org/patch/10553037/
> > >> 
> > >> The problem with this patch is that the existing naming scheme is very
> > >> similar while the new one offers no tangible benefits apart from being
> > >> in line with the rest of the kernel. That's still not a benefit for uAPI:
> > >> changing the name is certain to break user space applications.
> > > 
> > > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote
> > > 'perhaps'), but you don't say anything on the one about
> > > v4l2_spi_subdev_init :), which fixes an actual bug.  I have 2 identical
> > > SPI-controlled sensors on the same board, and without my patch they get
> > > the same subdev name.  Of course, I could fix that in the sensor driver
> > > itself, but that's not what we want, or do we ?
> > 
> > Good point. I missed the naming of the SPI devices ignored any bus
> > information there. I'm rather inclined towards taking the SPI patch. Hans,
> > Mauro, Laurent; any opinion on that?
> 
> I agree that the SPI patch makes sense, I think we should take it.

Do I need to resend https://patchwork.kernel.org/patch/10553035/ "media:
v4l2-common: v4l2_spi_subdev_init : generate unique name", adding Sakari's
and Laurent's Acked-by ? or will that patch be taken automagically ?

Philippe

-- 
Philippe De Muyter +32 2 6101532 Macq SA rue de l'Aeronef 2 B-1140 Bruxelles

Re: [PATCH 3/7] media: imx274: don't hard-code the subdev name to DRIVER_NAME

[Sakari Ailus]

On Thu, Aug 30, 2018 at 08:58:13AM +0200, Philippe De Muyter wrote:
> Hi.
> 
> On Thu, Aug 30, 2018 at 12:23:23AM +0300, Laurent Pinchart wrote:
> > Hello,
> > 
> > On Wednesday, 29 August 2018 14:38:43 EEST Sakari Ailus wrote:
> > > On Wed, Aug 29, 2018 at 01:29:36PM +0200, Philippe De Muyter wrote:
> > > > On Wed, Aug 29, 2018 at 02:07:21PM +0300, Sakari Ailus wrote:
> > > >> On Tue, Aug 28, 2018 at 06:02:55PM +0200, Philippe De Muyter wrote:
> > 
> > [snip]
> > 
> > > >>> Then we should probably also apply the following patch I submitted :
> > > >>> 
> > > >>> "media: v4l2-common: v4l2_spi_subdev_init : generate unique name"
> > > >>> 
> > > >>> 	https://patchwork.kernel.org/patch/10553035/
> > > >>> 
> > > >>> and perhaps
> > > >>> 
> > > >>> "media: v4l2-common: simplify v4l2_i2c_subdev_init name generation"
> > > >>> 
> > > >>> 	https://patchwork.kernel.org/patch/10553037/
> > > >> 
> > > >> The problem with this patch is that the existing naming scheme is very
> > > >> similar while the new one offers no tangible benefits apart from being
> > > >> in line with the rest of the kernel. That's still not a benefit for uAPI:
> > > >> changing the name is certain to break user space applications.
> > > > 
> > > > I agree with you on the patch for v4l2_i2c_subdev_init (I wrote
> > > > 'perhaps'), but you don't say anything on the one about
> > > > v4l2_spi_subdev_init :), which fixes an actual bug.  I have 2 identical
> > > > SPI-controlled sensors on the same board, and without my patch they get
> > > > the same subdev name.  Of course, I could fix that in the sensor driver
> > > > itself, but that's not what we want, or do we ?
> > > 
> > > Good point. I missed the naming of the SPI devices ignored any bus
> > > information there. I'm rather inclined towards taking the SPI patch. Hans,
> > > Mauro, Laurent; any opinion on that?
> > 
> > I agree that the SPI patch makes sense, I think we should take it.
> 
> Do I need to resend https://patchwork.kernel.org/patch/10553035/ "media:
> v4l2-common: v4l2_spi_subdev_init : generate unique name", adding Sakari's
> and Laurent's Acked-by ? or will that patch be taken automagically ?

No need to.

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi