linux-kernel - Re: [PATCH v4 07/10] media: i2c: ov9282: add led

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ns3aybw7nvxrcowprdwbwcn6zpuybkbfmtv44emy567dryszih@yp4hgpgtbvnt>
Date: Wed, 28 May 2025 14:15:31 +0200
From: Richard Leitner <richard.leitner@...ux.dev>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: Dave Stevenson <dave.stevenson@...pberrypi.com>, 
	Mauro Carvalho Chehab <mchehab@...nel.org>, Lee Jones <lee@...nel.org>, Pavel Machek <pavel@...nel.org>, 
	Laurent Pinchart <laurent.pinchart@...asonboard.com>, Hans Verkuil <hverkuil@...all.nl>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org
Subject: Re: [PATCH v4 07/10] media: i2c: ov9282: add led_mode v4l2 control

Hi Sakari,

On Mon, May 26, 2025 at 08:41:08PM +0000, Sakari Ailus wrote:
> Hi Richard,
> 
> On Wed, May 07, 2025 at 09:51:36AM +0200, Richard Leitner wrote:
> > Add V4L2_CID_FLASH_LED_MODE support using the "strobe output enable"
> > feature of the sensor. This implements following modes:
> 
> The flash LED mode control is, well, setting the flash LED mode. There's no
> LED on the sensor so I think I'd add a new control for this.
> 
> I'd call it V4L2_FLASH_LED_STROBE_ENABLE, and make it a boolean control.
> 
> (My apologies for not giving a better review for this set earlier on.)

No problem. I'm open for discussions all the time ;-)

I'm basically fine with renaming this, but is there any benefit we get
by introducing such a new control?

IMHO V4L2_CID_FLASH_LED_MODE and V4L2_FLASH_LED_STROBE_ENABLE sound
pretty similar and I'm not sure this is "worth" a new v4l2 control.

But of course, you guys are the domain expert, so please feel free to
"overrule" my gut feeling. ;-)

> 
> How does this sensor make use the information? E.g. what's the latching
> point this setting in relation to a given frame?

I'm not sure if I understand you correctly, but the strobe pulse
"starting time" is configurable using registers on the sensor. To keep
this patchset small I've decided to not include this
"strobe_frame_shift" setting (which may be positive or negative) here.

Nonetheless I'm planning to send another series adding more features of
the sensors as soon as this got merged.

IMHO we would need another new v4l2 control for this then (something
like V4L2_FLASH_LED_STROBE_SHIFT).

Does that answer your question?

> 
> > 
> >  - V4L2_FLASH_LED_MODE_NONE, which disables the strobe output
> >  - V4L2_FLASH_LED_MODE_FLASH, which enables the strobe output
> > 
> > All values are based on the OV9281 datasheet v1.53 (january 2019) and
> > tested using an ov9281 VisionComponents module.
> > 
> > Signed-off-by: Richard Leitner <richard.leitner@...ux.dev>
> > ---
> >  drivers/media/i2c/ov9282.c | 29 ++++++++++++++++++++++++++++-
> >  1 file changed, 28 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/i2c/ov9282.c b/drivers/media/i2c/ov9282.c
> > index f42e0d439753e74d14e3a3592029e48f49234927..b6de96997426f7225a061bfdc841aa062e8d0891 100644
> > --- a/drivers/media/i2c/ov9282.c
> > +++ b/drivers/media/i2c/ov9282.c
> > @@ -670,6 +670,23 @@ static int ov9282_set_ctrl_vflip(struct ov9282 *ov9282, int value)
> >  				current_val);
> >  }
> >  
> > +static int ov9282_set_ctrl_flash_led_mode(struct ov9282 *ov9282, int mode)
> > +{
> > +	u32 current_val;
> > +	int ret = ov9282_read_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > +				  &current_val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (mode == V4L2_FLASH_LED_MODE_FLASH)
> > +		current_val |= OV9282_OUTPUT_ENABLE6_STROBE;
> > +	else
> > +		current_val &= ~OV9282_OUTPUT_ENABLE6_STROBE;
> > +
> > +	return ov9282_write_reg(ov9282, OV9282_REG_OUTPUT_ENABLE6, 1,
> > +				current_val);
> > +}
> > +
> >  /**
> >   * ov9282_set_ctrl() - Set subdevice control
> >   * @ctrl: pointer to v4l2_ctrl structure
> > @@ -736,6 +753,9 @@ static int ov9282_set_ctrl(struct v4l2_ctrl *ctrl)
> >  		ret = ov9282_write_reg(ov9282, OV9282_REG_TIMING_HTS, 2,
> >  				       (ctrl->val + ov9282->cur_mode->width) >> 1);
> >  		break;
> > +	case V4L2_CID_FLASH_LED_MODE:
> > +		ret = ov9282_set_ctrl_flash_led_mode(ov9282, ctrl->val);
> > +		break;
> >  	default:
> >  		dev_err(ov9282->dev, "Invalid control %d", ctrl->id);
> >  		ret = -EINVAL;
> > @@ -1326,7 +1346,7 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  	u32 lpfr;
> >  	int ret;
> >  
> > -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 10);
> > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1391,6 +1411,13 @@ static int ov9282_init_controls(struct ov9282 *ov9282)
> >  						OV9282_TIMING_HTS_MAX - mode->width,
> >  						1, hblank_min);
> >  
> > +	/* Flash/Strobe controls */
> > +	v4l2_ctrl_new_std_menu(ctrl_hdlr, &ov9282_ctrl_ops,
> > +			       V4L2_CID_FLASH_LED_MODE,
> > +			       V4L2_FLASH_LED_MODE_TORCH,
> > +			       (1 << V4L2_FLASH_LED_MODE_TORCH),
> > +			       V4L2_FLASH_LED_MODE_NONE);
> > +
> >  	ret = v4l2_fwnode_device_parse(ov9282->dev, &props);
> >  	if (!ret) {
> >  		/* Failure sets ctrl_hdlr->error, which we check afterwards anyway */
> > 
> 
> -- 
> Regards,
> 
> Sakari Ailus