[v2] ipa: rpi: Initialize enum controls with a list of values
diff mbox series

Message ID 20250616100900.1524567-1-barnabas.pocze@ideasonboard.com
State Accepted
Headers show
Series
  • [v2] ipa: rpi: Initialize enum controls with a list of values
Related show

Commit Message

Barnabás Pőcze June 16, 2025, 10:09 a.m. UTC 
This is how uvcvideo and rkisp1 do it. See ee918b370a08b
("ipa: rkisp1: agc: Initialize enum controls with a list of values")
for the motivation. In summary, having a list of values is used as a sign
that the control is an enum in multiple places (e.g. `cam`, `camshark`).

Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
---
changes in v2:
  * specify all supported values explicitly

v1: https://patchwork.libcamera.org/patch/23505/
---
 src/ipa/rpi/common/ipa_base.cpp | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

--
2.49.0

Comments

Naushir Patuck June 16, 2025, 11:24 a.m. UTC | #1 
Hi Barnabás,

Looks reasonable to me.

On Mon, 16 Jun 2025 at 11:09, Barnabás Pőcze
<barnabas.pocze@ideasonboard.com> wrote:
>
> This is how uvcvideo and rkisp1 do it. See ee918b370a08b
> ("ipa: rkisp1: agc: Initialize enum controls with a list of values")
> for the motivation. In summary, having a list of values is used as a sign
> that the control is an enum in multiple places (e.g. `cam`, `camshark`).
>
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>

> ---
> changes in v2:
>   * specify all supported values explicitly
>
> v1: https://patchwork.libcamera.org/patch/23505/
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 50e25f057..6565f5366 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -58,23 +58,24 @@ const ControlInfoMap::Map ipaControls{
>         /* \todo Move this to the Camera class */
>         { &controls::AeEnable, ControlInfo(false, true, true) },
>         { &controls::ExposureTimeMode,
> -         ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> -                     static_cast<int32_t>(controls::ExposureTimeModeManual),
> -                     static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
> +         ControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),
> +                         ControlValue(controls::ExposureTimeModeManual) } },
> +                     ControlValue(controls::ExposureTimeModeAuto)) },
>         { &controls::ExposureTime,
>           ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
>         { &controls::AnalogueGainMode,
> -         ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> -                     static_cast<int32_t>(controls::AnalogueGainModeManual),
> -                     static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
> +         ControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),
> +                         ControlValue(controls::AnalogueGainModeManual) } },
> +                     ControlValue(controls::AnalogueGainModeAuto)) },
>         { &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
>         { &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>         { &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>         { &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>         { &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> -       { &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
> -                                               static_cast<int>(controls::FlickerManual),
> -                                               static_cast<int>(controls::FlickerOff)) },
> +       { &controls::AeFlickerMode,
> +         ControlInfo({ { ControlValue(controls::FlickerOff),
> +                         ControlValue(controls::FlickerManual) } },
> +                     ControlValue(controls::FlickerOff)) },
>         { &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>         { &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>         { &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },
> --
> 2.49.0
Laurent Pinchart June 16, 2025, noon UTC | #2 
On Mon, Jun 16, 2025 at 12:09:00PM +0200, Barnabás Pőcze wrote:
> This is how uvcvideo and rkisp1 do it. See ee918b370a08b
> ("ipa: rkisp1: agc: Initialize enum controls with a list of values")
> for the motivation. In summary, having a list of values is used as a sign
> that the control is an enum in multiple places (e.g. `cam`, `camshark`).
> 
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> changes in v2:
>   * specify all supported values explicitly
> 
> v1: https://patchwork.libcamera.org/patch/23505/
> ---
>  src/ipa/rpi/common/ipa_base.cpp | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
> index 50e25f057..6565f5366 100644
> --- a/src/ipa/rpi/common/ipa_base.cpp
> +++ b/src/ipa/rpi/common/ipa_base.cpp
> @@ -58,23 +58,24 @@ const ControlInfoMap::Map ipaControls{
>  	/* \todo Move this to the Camera class */
>  	{ &controls::AeEnable, ControlInfo(false, true, true) },
>  	{ &controls::ExposureTimeMode,
> -	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
> -		      static_cast<int32_t>(controls::ExposureTimeModeManual),
> -		      static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
> +	  ControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),
> +			  ControlValue(controls::ExposureTimeModeManual) } },
> +		      ControlValue(controls::ExposureTimeModeAuto)) },
>  	{ &controls::ExposureTime,
>  	  ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
>  	{ &controls::AnalogueGainMode,
> -	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
> -		      static_cast<int32_t>(controls::AnalogueGainModeManual),
> -		      static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
> +	  ControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),
> +			  ControlValue(controls::AnalogueGainModeManual) } },
> +		      ControlValue(controls::AnalogueGainModeAuto)) },
>  	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
>  	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
>  	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
>  	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
>  	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
> -	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
> -						static_cast<int>(controls::FlickerManual),
> -						static_cast<int>(controls::FlickerOff)) },
> +	{ &controls::AeFlickerMode,
> +	  ControlInfo({ { ControlValue(controls::FlickerOff),
> +			  ControlValue(controls::FlickerManual) } },
> +		      ControlValue(controls::FlickerOff)) },
>  	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
>  	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
>  	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },

Patch
diff mbox series 

diff --git a/src/ipa/rpi/common/ipa_base.cpp b/src/ipa/rpi/common/ipa_base.cpp
index 50e25f057..6565f5366 100644
--- a/src/ipa/rpi/common/ipa_base.cpp
+++ b/src/ipa/rpi/common/ipa_base.cpp
@@ -58,23 +58,24 @@  const ControlInfoMap::Map ipaControls{
 	/* \todo Move this to the Camera class */
 	{ &controls::AeEnable, ControlInfo(false, true, true) },
 	{ &controls::ExposureTimeMode,
-	  ControlInfo(static_cast<int32_t>(controls::ExposureTimeModeAuto),
-		      static_cast<int32_t>(controls::ExposureTimeModeManual),
-		      static_cast<int32_t>(controls::ExposureTimeModeAuto)) },
+	  ControlInfo({ { ControlValue(controls::ExposureTimeModeAuto),
+			  ControlValue(controls::ExposureTimeModeManual) } },
+		      ControlValue(controls::ExposureTimeModeAuto)) },
 	{ &controls::ExposureTime,
 	  ControlInfo(1, 66666, static_cast<int32_t>(defaultExposureTime.get<std::micro>())) },
 	{ &controls::AnalogueGainMode,
-	  ControlInfo(static_cast<int32_t>(controls::AnalogueGainModeAuto),
-		      static_cast<int32_t>(controls::AnalogueGainModeManual),
-		      static_cast<int32_t>(controls::AnalogueGainModeAuto)) },
+	  ControlInfo({ { ControlValue(controls::AnalogueGainModeAuto),
+			  ControlValue(controls::AnalogueGainModeManual) } },
+		      ControlValue(controls::AnalogueGainModeAuto)) },
 	{ &controls::AnalogueGain, ControlInfo(1.0f, 16.0f, 1.0f) },
 	{ &controls::AeMeteringMode, ControlInfo(controls::AeMeteringModeValues) },
 	{ &controls::AeConstraintMode, ControlInfo(controls::AeConstraintModeValues) },
 	{ &controls::AeExposureMode, ControlInfo(controls::AeExposureModeValues) },
 	{ &controls::ExposureValue, ControlInfo(-8.0f, 8.0f, 0.0f) },
-	{ &controls::AeFlickerMode, ControlInfo(static_cast<int>(controls::FlickerOff),
-						static_cast<int>(controls::FlickerManual),
-						static_cast<int>(controls::FlickerOff)) },
+	{ &controls::AeFlickerMode,
+	  ControlInfo({ { ControlValue(controls::FlickerOff),
+			  ControlValue(controls::FlickerManual) } },
+		      ControlValue(controls::FlickerOff)) },
 	{ &controls::AeFlickerPeriod, ControlInfo(100, 1000000) },
 	{ &controls::Brightness, ControlInfo(-1.0f, 1.0f, 0.0f) },
 	{ &controls::Contrast, ControlInfo(0.0f, 32.0f, 1.0f) },