[PATCH 1/3] media: imx274: don't declare events, they are not implemented

[PATCH 1/3] media: imx274: don't declare events, they are not implemented

[Luca Ceresoli]

The V4L2_SUBDEV_FL_HAS_EVENTS flag should not be set, event are just
not implemented.

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 95a0e7d9851a..78746c51071d 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1878,7 +1878,7 @@ static int imx274_probe(struct i2c_client *client,
 	imx274->client = client;
 	sd = &imx274->sd;
 	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
-	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
+	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
 	/* initialize subdev media pad */
 	imx274->pad.flags = MEDIA_PAD_FL_SOURCE;
-- 
2.17.1

[PATCH 2/3] media: imx274: declare the correct number of controls

[Luca Ceresoli]

v4l2_ctrl_handler_init() expects a hint of how many controls this
handler is expected to refer to. Since this number here is always 4,
let's pass exactly 4.

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 78746c51071d..40c717f13eb8 100644
--- a/drivers/media/i2c/imx274.c
+++ b/drivers/media/i2c/imx274.c
@@ -1904,7 +1904,7 @@ static int imx274_probe(struct i2c_client *client,
 	imx274_reset(imx274, 1);
 
 	/* initialize controls */
-	ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 2);
+	ret = v4l2_ctrl_handler_init(&imx274->ctrls.handler, 4);
 	if (ret < 0) {
 		dev_err(&client->dev,
 			"%s : ctrl handler init Failed\n", __func__);
-- 
2.17.1

[PATCH 3/3] media: imx274: select REGMAP_I2C

[Luca Ceresoli]

The imx274 driver uses regmap and the build will fail without it.

Fixes:

  drivers/media/i2c/imx274.c:142:21: error: variable ‘imx274_regmap_config’ has initializer but incomplete type
   static const struct regmap_config imx274_regmap_config = {
                       ^~~~~~~~~~~~~
  drivers/media/i2c/imx274.c:1869:19: error: implicit declaration of function ‘devm_regmap_init_i2c’ [-Werror=implicit-function-declaration]
    imx274->regmap = devm_regmap_init_i2c(client, &imx274_regmap_config);
                     ^~~~~~~~~~~~~~~~~~~~

and others.

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

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index 66bbbec487ec..005fc2bd0d05 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -622,6 +622,7 @@ config VIDEO_IMX274
 	tristate "Sony IMX274 sensor support"
 	depends on I2C && VIDEO_V4L2 && VIDEO_V4L2_SUBDEV_API
 	depends on MEDIA_CAMERA_SUPPORT
+	select REGMAP_I2C
 	---help---
 	  This is a V4L2 sensor driver for the Sony IMX274
 	  CMOS image sensor.
-- 
2.17.1

Re: [PATCH 1/3] media: imx274: don't declare events, they are not implemented

[Sakari Ailus]

Hi Luca,

On Tue, Nov 27, 2018 at 09:34:43AM +0100, Luca Ceresoli wrote:
> The V4L2_SUBDEV_FL_HAS_EVENTS flag should not be set, event are just
> not implemented.
> 
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>

The driver supports controls, and so control events can be subscribed and
received by the user. Therefore I don't see a reason to remove the flag.

> ---
>  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 95a0e7d9851a..78746c51071d 100644
> --- a/drivers/media/i2c/imx274.c
> +++ b/drivers/media/i2c/imx274.c
> @@ -1878,7 +1878,7 @@ static int imx274_probe(struct i2c_client *client,
>  	imx274->client = client;
>  	sd = &imx274->sd;
>  	v4l2_i2c_subdev_init(sd, client, &imx274_subdev_ops);
> -	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  
>  	/* initialize subdev media pad */
>  	imx274->pad.flags = MEDIA_PAD_FL_SOURCE;

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/3] media: imx274: don't declare events, they are not implemented

[Luca Ceresoli]

Hi Sakari,

On 27/11/18 09:38, Sakari Ailus wrote:
> Hi Luca,
> 
> On Tue, Nov 27, 2018 at 09:34:43AM +0100, Luca Ceresoli wrote:
>> The V4L2_SUBDEV_FL_HAS_EVENTS flag should not be set, event are just
>> not implemented.
>>
>> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> 
> The driver supports controls, and so control events can be subscribed and
> received by the user. Therefore I don't see a reason to remove the flag.

Thanks, good to know.

Would it be worth adding a note where V4L2_SUBDEV_FL_HAS_EVENTS is
#defined, to make this clear?

-- 
Luca

Re: [PATCH 1/3] media: imx274: don't declare events, they are not implemented

[Sakari Ailus]

On Tue, Nov 27, 2018 at 10:09:08AM +0100, Luca Ceresoli wrote:
> Hi Sakari,
> 
> On 27/11/18 09:38, Sakari Ailus wrote:
> > Hi Luca,
> > 
> > On Tue, Nov 27, 2018 at 09:34:43AM +0100, Luca Ceresoli wrote:
> >> The V4L2_SUBDEV_FL_HAS_EVENTS flag should not be set, event are just
> >> not implemented.
> >>
> >> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
> > 
> > The driver supports controls, and so control events can be subscribed and
> > received by the user. Therefore I don't see a reason to remove the flag.

I further missed the driver does not set the event (un)subscription
callbacks; the event support is actually not functional as a result. :-\

It's trivial to do that, see e.g. the imx319 driver.

> 
> Thanks, good to know.
> 
> Would it be worth adding a note where V4L2_SUBDEV_FL_HAS_EVENTS is
> #defined, to make this clear?

Could you send a patch? A few words should be enough, no need for a too
elaborate description I guess.

-- 
Regards,

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