linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Proposal for a v4l2_ctrl_new_std_compound() function
@ 2019-09-12 13:35 Hans Verkuil
  2019-09-13 16:08 ` kbuild test robot
  2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado
  0 siblings, 2 replies; 7+ messages in thread
From: Hans Verkuil @ 2019-09-12 13:35 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado; +Cc: Linux Media Mailing List

Hi Ricardo,

As per our irc discussion, here is a proposal how I think it can be done.
The core problem is that for compound types we want to provide a default
value that can be used in std_init_compound() without having to provide
our own type ops. The new v4l2_ctrl_new_std_compound would pass a const
pointer to the struct with the default value, and std_init_compound
should use that if p_def != NULL.

I think this is beneficial for the various codec compound types as well.

Implementing this in v4l2-ctrls.c is left as an exercise for the reader
(i.e., you!).

Regards,

	Hans

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 570ff4b0205a..7fdbf3abe49b 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -254,6 +254,7 @@ struct v4l2_ctrl {
 		s32 val;
 	} cur;

+	const union v4l2_ctrl_ptr p_def;
 	union v4l2_ctrl_ptr p_new;
 	union v4l2_ctrl_ptr p_cur;
 };
@@ -667,6 +668,27 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 					 u32 id, u8 max, u8 def,
 					 const s64 *qmenu_int);

+/**
+ * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
+ *	compound control.
+ *
+ * @hdl:	The control handler.
+ * @ops:	The control ops.
+ * @id:		The control ID.
+ * @min:	The control's minimum value.
+ * @max:	The control's maximum value.
+ * @step:	The control's step value
+ * @p_def:	The control's default value.
+ *
+ * If the &v4l2_ctrl struct could not be allocated, or the control
+ * ID is not known, then NULL is returned and @hdl->error is set to the
+ * appropriate error code (if it wasn't set already).
+ */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+					     const struct v4l2_ctrl_ops *ops,
+					     u32 id, s64 min, s64 max, u64 step,
+					     const union v4l2_ctrl_ptr p_def);
+
 /**
  * typedef v4l2_ctrl_filter - Typedef to define the filter function to be
  *	used when adding a control handler.

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: Proposal for a v4l2_ctrl_new_std_compound() function
  2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil
@ 2019-09-13 16:08 ` kbuild test robot
  2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado
  1 sibling, 0 replies; 7+ messages in thread
From: kbuild test robot @ 2019-09-13 16:08 UTC (permalink / raw)
  To: Hans Verkuil
  Cc: kbuild-all, Ricardo Ribalda Delgado, Linux Media Mailing List

[-- Attachment #1: Type: text/plain, Size: 32905 bytes --]

Hi Hans,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[cannot apply to v5.3-rc8 next-20190904]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-Verkuil/Proposal-for-a-v4l2_ctrl_new_std_compound-function/20190913-222917
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   Warning: The Sphinx 'sphinx_rtd_theme' HTML theme was not found. Make sure you have the theme installed to produce pretty HTML output. Falling back to the default theme.
   WARNING: dot(1) not found, for better output quality install graphviz from http://www.graphviz.org
   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   include/linux/w1.h:272: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
   include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:142: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:823: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2822: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:378: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch'
   drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:379: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
   drivers/gpu/drm/amd/amdgpu/amdgpu_xgmi.c:1: warning: no structured comments found
   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:1: warning: no structured comments found
   drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: 'pp_dpm_sclk pp_dpm_mclk pp_dpm_pcie' not found
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:131: warning: Incorrect use of kernel-doc format:          * @atomic_obj
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:237: warning: Incorrect use of kernel-doc format:          * gpu_info FW provided soc bounding box struct or 0 if not
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'backlight_caps' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:242: warning: Function parameter or member 'soc_bounding_box' not described in 'amdgpu_display_manager'
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'dm_pflip_high_irq' not found
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'register_hpd_handlers' not found
   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: 'dm_crtc_high_irq' not found
   include/linux/i2c.h:337: warning: Function parameter or member 'init_irq' not described in 'i2c_client'
   include/linux/regulator/machine.h:196: warning: Function parameter or member 'max_uV_step' not described in 'regulation_constraints'
   include/linux/regulator/driver.h:223: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   include/linux/spi/spi.h:190: warning: Function parameter or member 'driver_override' not described in 'spi_device'
   drivers/usb/typec/bus.c:1: warning: 'typec_altmode_unregister_driver' not found
   drivers/usb/typec/bus.c:1: warning: 'typec_altmode_register_driver' not found
   drivers/usb/typec/class.c:1: warning: 'typec_altmode_register_notifier' not found
   drivers/usb/typec/class.c:1: warning: 'typec_altmode_unregister_notifier' not found
   fs/direct-io.c:258: warning: Excess function parameter 'offset' description in 'dio_complete'
   fs/libfs.c:496: warning: Excess function parameter 'available' description in 'simple_write_end'
   fs/posix_acl.c:647: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:647: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:647: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
>> include/media/v4l2-ctrls.h:260: warning: Function parameter or member 'p_def' not described in 'v4l2_ctrl'
   mm/util.c:1: warning: 'get_user_pages_fast' not found
   mm/slab.c:4215: warning: Function parameter or member 'objp' not described in '__ksize'
   include/linux/input/sparse-keymap.h:43: warning: Function parameter or member 'sw' not described in 'key_entry'
   include/linux/skbuff.h:893: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'list' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:893: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:233: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:233: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:515: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
   include/net/sock.h:515: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
   include/net/sock.h:2439: warning: Function parameter or member 'tcp_rx_skb_cache_key' not described in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'sk' description in 'DECLARE_STATIC_KEY_FALSE'
   include/net/sock.h:2439: warning: Excess function parameter 'skb' description in 'DECLARE_STATIC_KEY_FALSE'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
   include/linux/netdevice.h:2040: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   drivers/net/phy/phylink.c:595: warning: Function parameter or member 'config' not described in 'phylink_create'
   drivers/net/phy/phylink.c:595: warning: Excess function parameter 'ndev' description in 'phylink_create'
   lib/genalloc.c:1: warning: 'gen_pool_add_virt' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc' not found
   lib/genalloc.c:1: warning: 'gen_pool_free' not found
   lib/genalloc.c:1: warning: 'gen_pool_alloc_algo' not found
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
   include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
   include/drm/drm_modeset_helper_vtables.h:1053: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
   include/drm/drm_modeset_helper_vtables.h:1053: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
   include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found
   drivers/gpu/drm/mcde/mcde_drv.c:1: warning: 'ST-Ericsson MCDE DRM Driver' not found
   include/net/cfg80211.h:1092: warning: Function parameter or member 'txpwr' not described in 'station_parameters'
   include/net/mac80211.h:4043: warning: Function parameter or member 'sta_set_txpwr' not described in 'ieee80211_ops'
   include/net/mac80211.h:2006: warning: Function parameter or member 'txpwr' not described in 'ieee80211_sta'
   Documentation/admin-guide/xfs.rst:257: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/index.rst:94: WARNING: toctree contains reference to nonexisting document 'virtual/index'
   Documentation/crypto/crypto_engine.rst:2: WARNING: Explicit markup ends without a blank line; unexpected unindent.
   Documentation/kbuild/makefiles.rst:1142: WARNING: Inline emphasis start-string without end-string.
   Documentation/kbuild/makefiles.rst:1152: WARNING: Inline emphasis start-string without end-string.
   Documentation/kbuild/makefiles.rst:1154: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1110: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1110: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string.
   Documentation/security/keys/core.rst:1108: WARNING: Inline emphasis start-string without end-string.
   Documentation/trace/kprobetrace.rst:99: WARNING: Explicit markup ends without a blank line; unexpected unindent.
   include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
   drivers/firewire/core-transaction.c:606: WARNING: Inline strong start-string without end-string.
   drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1964: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:245: WARNING: Unexpected indentation.
   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c:248: WARNING: Block quote ends without a blank line; unexpected unindent.
   drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c:271: WARNING: Unexpected indentation.

vim +260 include/media/v4l2-ctrls.h

8ac7a9493a4380 Hans Verkuil          2012-09-07  126  
8c2721d57a4bac Mauro Carvalho Chehab 2015-08-22  127  /**
8c2721d57a4bac Mauro Carvalho Chehab 2015-08-22  128   * struct v4l2_ctrl - The control structure.
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  129   *
0996517cf8eade Hans Verkuil          2010-08-01  130   * @node:	The list node.
77068d36d8b9e9 Hans Verkuil          2011-06-13  131   * @ev_subs:	The list of control event subscriptions.
0996517cf8eade Hans Verkuil          2010-08-01  132   * @handler:	The handler that owns the control.
0996517cf8eade Hans Verkuil          2010-08-01  133   * @cluster:	Point to start of cluster array.
0996517cf8eade Hans Verkuil          2010-08-01  134   * @ncontrols:	Number of controls in cluster array.
0996517cf8eade Hans Verkuil          2010-08-01  135   * @done:	Internal flag: set for each processed control.
2a863793beaa0f Hans Verkuil          2011-01-11  136   * @is_new:	Set when the user specified a new value for this control. It
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  137   *		is also set when called from v4l2_ctrl_handler_setup(). Drivers
2a863793beaa0f Hans Verkuil          2011-01-11  138   *		should never set this flag.
9ea1b7a4b66fdd Hans Verkuil          2014-01-17  139   * @has_changed: Set when the current value differs from the new value. Drivers
9ea1b7a4b66fdd Hans Verkuil          2014-01-17  140   *		should never use this flag.
0996517cf8eade Hans Verkuil          2010-08-01  141   * @is_private: If set, then this control is private to its handler and it
0996517cf8eade Hans Verkuil          2010-08-01  142   *		will not be added to any other handlers. Drivers can set
0996517cf8eade Hans Verkuil          2010-08-01  143   *		this flag.
72d877cac07c8d Hans Verkuil          2011-06-10  144   * @is_auto:   If set, then this control selects whether the other cluster
72d877cac07c8d Hans Verkuil          2011-06-10  145   *		members are in 'automatic' mode or 'manual' mode. This is
72d877cac07c8d Hans Verkuil          2011-06-10  146   *		used for autogain/gain type clusters. Drivers should never
72d877cac07c8d Hans Verkuil          2011-06-10  147   *		set this flag directly.
d9a2547150245f Hans Verkuil          2014-06-12  148   * @is_int:    If set, then this control has a simple integer value (i.e. it
d9a2547150245f Hans Verkuil          2014-06-12  149   *		uses ctrl->val).
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  150   * @is_string: If set, then this control has type %V4L2_CTRL_TYPE_STRING.
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  151   * @is_ptr:	If set, then this control is an array and/or has type >=
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  152   *		%V4L2_CTRL_COMPOUND_TYPES
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  153   *		and/or has type %V4L2_CTRL_TYPE_STRING. In other words, &struct
d9a2547150245f Hans Verkuil          2014-06-12  154   *		v4l2_ext_control uses field p to point to the data.
998e7659150760 Hans Verkuil          2014-06-10  155   * @is_array: If set, then this control contains an N-dimensional array.
5626b8c75bc13a Hans Verkuil          2011-08-26  156   * @has_volatiles: If set, then one or more members of the cluster are volatile.
5626b8c75bc13a Hans Verkuil          2011-08-26  157   *		Drivers should never touch this flag.
8ac7a9493a4380 Hans Verkuil          2012-09-07  158   * @call_notify: If set, then call the handler's notify function whenever the
8ac7a9493a4380 Hans Verkuil          2012-09-07  159   *		control's value changes.
72d877cac07c8d Hans Verkuil          2011-06-10  160   * @manual_mode_value: If the is_auto flag is set, then this is the value
72d877cac07c8d Hans Verkuil          2011-06-10  161   *		of the auto control that determines if that control is in
72d877cac07c8d Hans Verkuil          2011-06-10  162   *		manual mode. So if the value of the auto control equals this
72d877cac07c8d Hans Verkuil          2011-06-10  163   *		value, then the whole cluster is in manual mode. Drivers should
72d877cac07c8d Hans Verkuil          2011-06-10  164   *		never set this flag directly.
0996517cf8eade Hans Verkuil          2010-08-01  165   * @ops:	The control ops.
0176077a813933 Hans Verkuil          2014-04-27  166   * @type_ops:	The control type ops.
0996517cf8eade Hans Verkuil          2010-08-01  167   * @id:	The control ID.
0996517cf8eade Hans Verkuil          2010-08-01  168   * @name:	The control name.
0996517cf8eade Hans Verkuil          2010-08-01  169   * @type:	The control type.
0996517cf8eade Hans Verkuil          2010-08-01  170   * @minimum:	The control's minimum value.
0996517cf8eade Hans Verkuil          2010-08-01  171   * @maximum:	The control's maximum value.
0996517cf8eade Hans Verkuil          2010-08-01  172   * @default_value: The control's default value.
0996517cf8eade Hans Verkuil          2010-08-01  173   * @step:	The control's step value for non-menu controls.
20d88eef66a869 Hans Verkuil          2014-06-12  174   * @elems:	The number of elements in the N-dimensional array.
d9a2547150245f Hans Verkuil          2014-06-12  175   * @elem_size:	The size in bytes of the control.
20d88eef66a869 Hans Verkuil          2014-06-12  176   * @dims:	The size of each dimension.
20d88eef66a869 Hans Verkuil          2014-06-12  177   * @nr_of_dims:The number of dimensions in @dims.
0996517cf8eade Hans Verkuil          2010-08-01  178   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
0996517cf8eade Hans Verkuil          2010-08-01  179   *		easy to skip menu items that are not valid. If bit X is set,
0996517cf8eade Hans Verkuil          2010-08-01  180   *		then menu item X is skipped. Of course, this only works for
0996517cf8eade Hans Verkuil          2010-08-01  181   *		menus with <= 32 menu items. There are no menus that come
0996517cf8eade Hans Verkuil          2010-08-01  182   *		close to that number, so this is OK. Should we ever need more,
0996517cf8eade Hans Verkuil          2010-08-01  183   *		then this will have to be extended to a u64 or a bit array.
0996517cf8eade Hans Verkuil          2010-08-01  184   * @qmenu:	A const char * array for all menu items. Array entries that are
0996517cf8eade Hans Verkuil          2010-08-01  185   *		empty strings ("") correspond to non-existing menu items (this
0996517cf8eade Hans Verkuil          2010-08-01  186   *		is in addition to the menu_skip_mask above). The last entry
0996517cf8eade Hans Verkuil          2010-08-01  187   *		must be NULL.
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  188   *		Used only if the @type is %V4L2_CTRL_TYPE_MENU.
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  189   * @qmenu_int:	A 64-bit integer array for with integer menu items.
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  190   *		The size of array must be equal to the menu size, e. g.:
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  191   *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  192   *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
0996517cf8eade Hans Verkuil          2010-08-01  193   * @flags:	The control's flags.
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  194   * @cur:	Structure to store the current value.
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  195   * @cur.val:	The control's current value, if the @type is represented via
20139f1857c1a1 Mauro Carvalho Chehab 2017-09-27  196   *		a u32 integer (see &enum v4l2_ctrl_type).
0996517cf8eade Hans Verkuil          2010-08-01  197   * @val:	The control's new s32 value.
0996517cf8eade Hans Verkuil          2010-08-01  198   * @priv:	The control's private pointer. For use by the driver. It is
0996517cf8eade Hans Verkuil          2010-08-01  199   *		untouched by the control framework. Note that this pointer is
0996517cf8eade Hans Verkuil          2010-08-01  200   *		not freed when the control is deleted. Should this be needed
0996517cf8eade Hans Verkuil          2010-08-01  201   *		then a new internal bitfield can be added to tell the framework
0996517cf8eade Hans Verkuil          2010-08-01  202   *		to free this pointer.
bf7b70482704bb Baruch Siach          2018-07-05  203   * @p_cur:	The control's current value represented via a union which
7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22  204   *		provides a standard way of accessing control types
7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22  205   *		through a pointer.
bf7b70482704bb Baruch Siach          2018-07-05  206   * @p_new:	The control's new value represented via a union which provides
7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22  207   *		a standard way of accessing control types
7dc879190f55f4 Mauro Carvalho Chehab 2015-08-22  208   *		through a pointer.
0996517cf8eade Hans Verkuil          2010-08-01  209   */
0996517cf8eade Hans Verkuil          2010-08-01  210  struct v4l2_ctrl {
0996517cf8eade Hans Verkuil          2010-08-01  211  	/* Administrative fields */
0996517cf8eade Hans Verkuil          2010-08-01  212  	struct list_head node;
77068d36d8b9e9 Hans Verkuil          2011-06-13  213  	struct list_head ev_subs;
0996517cf8eade Hans Verkuil          2010-08-01  214  	struct v4l2_ctrl_handler *handler;
0996517cf8eade Hans Verkuil          2010-08-01  215  	struct v4l2_ctrl **cluster;
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  216  	unsigned int ncontrols;
8ec4bee7c10e55 Mauro Carvalho Chehab 2016-07-22  217  
0996517cf8eade Hans Verkuil          2010-08-01  218  	unsigned int done:1;
0996517cf8eade Hans Verkuil          2010-08-01  219  
2a863793beaa0f Hans Verkuil          2011-01-11  220  	unsigned int is_new:1;
9ea1b7a4b66fdd Hans Verkuil          2014-01-17  221  	unsigned int has_changed:1;
0996517cf8eade Hans Verkuil          2010-08-01  222  	unsigned int is_private:1;
72d877cac07c8d Hans Verkuil          2011-06-10  223  	unsigned int is_auto:1;
d9a2547150245f Hans Verkuil          2014-06-12  224  	unsigned int is_int:1;
d9a2547150245f Hans Verkuil          2014-06-12  225  	unsigned int is_string:1;
d9a2547150245f Hans Verkuil          2014-06-12  226  	unsigned int is_ptr:1;
998e7659150760 Hans Verkuil          2014-06-10  227  	unsigned int is_array:1;
5626b8c75bc13a Hans Verkuil          2011-08-26  228  	unsigned int has_volatiles:1;
8ac7a9493a4380 Hans Verkuil          2012-09-07  229  	unsigned int call_notify:1;
82a7c049449ec5 Hans Verkuil          2011-06-28  230  	unsigned int manual_mode_value:8;
0996517cf8eade Hans Verkuil          2010-08-01  231  
0996517cf8eade Hans Verkuil          2010-08-01  232  	const struct v4l2_ctrl_ops *ops;
0176077a813933 Hans Verkuil          2014-04-27  233  	const struct v4l2_ctrl_type_ops *type_ops;
0996517cf8eade Hans Verkuil          2010-08-01  234  	u32 id;
0996517cf8eade Hans Verkuil          2010-08-01  235  	const char *name;
0996517cf8eade Hans Verkuil          2010-08-01  236  	enum v4l2_ctrl_type type;
0ba2aeb6dab809 Hans Verkuil          2014-04-16  237  	s64 minimum, maximum, default_value;
20d88eef66a869 Hans Verkuil          2014-06-12  238  	u32 elems;
d9a2547150245f Hans Verkuil          2014-06-12  239  	u32 elem_size;
20d88eef66a869 Hans Verkuil          2014-06-12  240  	u32 dims[V4L2_CTRL_MAX_DIMS];
20d88eef66a869 Hans Verkuil          2014-06-12  241  	u32 nr_of_dims;
0996517cf8eade Hans Verkuil          2010-08-01  242  	union {
0ba2aeb6dab809 Hans Verkuil          2014-04-16  243  		u64 step;
0ba2aeb6dab809 Hans Verkuil          2014-04-16  244  		u64 menu_skip_mask;
0996517cf8eade Hans Verkuil          2010-08-01  245  	};
ce580fe5190dec Sakari Ailus          2011-08-04  246  	union {
513521eaee4375 Hans Verkuil          2010-12-29  247  		const char * const *qmenu;
ce580fe5190dec Sakari Ailus          2011-08-04  248  		const s64 *qmenu_int;
ce580fe5190dec Sakari Ailus          2011-08-04  249  	};
0996517cf8eade Hans Verkuil          2010-08-01  250  	unsigned long flags;
d9a2547150245f Hans Verkuil          2014-06-12  251  	void *priv;
0996517cf8eade Hans Verkuil          2010-08-01  252  	s32 val;
2a9ec3731137f9 Hans Verkuil          2014-04-27  253  	struct {
0996517cf8eade Hans Verkuil          2010-08-01  254  		s32 val;
d9a2547150245f Hans Verkuil          2014-06-12  255  	} cur;
0176077a813933 Hans Verkuil          2014-04-27  256  
4d9433f96f76aa Hans Verkuil          2019-09-12  257  	const union v4l2_ctrl_ptr p_def;
0176077a813933 Hans Verkuil          2014-04-27  258  	union v4l2_ctrl_ptr p_new;
0176077a813933 Hans Verkuil          2014-04-27  259  	union v4l2_ctrl_ptr p_cur;
0996517cf8eade Hans Verkuil          2010-08-01 @260  };
0996517cf8eade Hans Verkuil          2010-08-01  261  

:::::: The code at line 260 was first introduced by commit
:::::: 0996517cf8eaded69b8502c8f5abeb8cec62b6d4 V4L/DVB: v4l2: Add new control handling framework

:::::: TO: Hans Verkuil <hverkuil@xs4all.nl>
:::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7282 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound()
  2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil
  2019-09-13 16:08 ` kbuild test robot
@ 2019-09-17 16:19 ` Ricardo Ribalda Delgado
  2019-09-17 16:21   ` Ricardo Ribalda Delgado
  1 sibling, 1 reply; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-09-17 16:19 UTC (permalink / raw)
  To: hverkuil, linux-media; +Cc: Ricardo Ribalda Delgado

Implement v4l2_ctrl_new_std_compound. This is just a discussion patch,
do not merge as is, and be gentle with the author ;P.

Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 drivers/media/i2c/imx214.c           | 13 +++--
 drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++-----------
 include/media/v4l2-ctrls.h           | 25 +++++++++
 3 files changed, 81 insertions(+), 36 deletions(-)

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 625617d4c81a..e18fed9f31f1 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client)
 		.width = 1120,
 		.height = 1120,
 	};
+	union v4l2_ctrl_ptr p_def = {
+		.p_area = &unit_size,
+	};
 	int ret;
 
 	ret = imx214_parse_fwnode(dev);
@@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client)
 					     V4L2_CID_EXPOSURE,
 					     0, 3184, 1, 0x0c70);
 
-	imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
-					       &imx214_ctrl_ops,
-					       V4L2_CID_UNIT_CELL_SIZE,
-					       &unit_size);
-
+	imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
+						       NULL,
+						       V4L2_CID_UNIT_CELL_SIZE,
+						       0, 0x7ffffff, 1, 0,
+						       p_def);
 	ret = imx214->ctrls.error;
 	if (ret) {
 		dev_err(&client->dev, "%s control init failed (%d)\n",
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 3d2fa1868982..04813ba2262b 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 		p_mpeg2_slice_params->picture.picture_coding_type =
 					V4L2_MPEG2_PICTURE_CODING_TYPE_I;
 		break;
+	default:
+		if (ctrl->has_p_def)
+			memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+		break;
 	}
 }
 
@@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			s64 min, s64 max, u64 step, s64 def,
 			const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
 			u32 flags, const char * const *qmenu,
-			const s64 *qmenu_int, void *priv)
+			const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+			void *priv)
 {
 	struct v4l2_ctrl *ctrl;
 	unsigned sz_extra;
@@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		 is_array)
 		sz_extra += 2 * tot_ctrl_size;
 
+	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+		sz_extra += elem_size;
+
 	ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
 	if (ctrl == NULL) {
 		handler_set_err(hdl, -ENOMEM);
@@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		ctrl->p_new.p = &ctrl->val;
 		ctrl->p_cur.p = &ctrl->cur.val;
 	}
+
+	if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+		ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+		memcpy(ctrl->p_def.p, p_def.p, elem_size);
+		ctrl->has_p_def = true;
+	}
+
 	for (idx = 0; idx < elems; idx++) {
 		ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
 		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
 	s64 max = cfg->max;
 	u64 step = cfg->step;
 	s64 def = cfg->def;
+	const union v4l2_ctrl_ptr p_def = {};
 
 	if (name == NULL)
 		v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
@@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
 			type, min, max,
 			is_menu ? cfg->menu_skip_mask : step, def,
 			cfg->dims, cfg->elem_size,
-			flags, qmenu, qmenu_int, priv);
+			flags, qmenu, qmenu_int, p_def, priv);
 	if (ctrl)
 		ctrl->is_private = cfg->is_private;
 	return ctrl;
@@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
 	const char *name;
 	enum v4l2_ctrl_type type;
 	u32 flags;
+	const union v4l2_ctrl_ptr p_def = {};
 
 	v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
 	if (type == V4L2_CTRL_TYPE_MENU ||
@@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
 			     min, max, step, def, NULL, 0,
-			     flags, NULL, NULL, NULL);
+			     flags, NULL, NULL, p_def, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std);
 
@@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
 	s64 def = _def;
 	u64 step;
 	u32 flags;
+	const union v4l2_ctrl_ptr p_def = {};
 
 	v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
 
@@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
 			     0, max, mask, def, NULL, 0,
-			     flags, qmenu, qmenu_int, NULL);
+			     flags, qmenu, qmenu_int, p_def, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
 
@@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 	s64 min;
 	s64 max = _max;
 	s64 def = _def;
+	const union v4l2_ctrl_ptr p_def = {};
 
 	/* v4l2_ctrl_new_std_menu_items() should only be called for
 	 * standard controls without a standard menu.
@@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
 			     0, max, mask, def, NULL, 0,
-			     flags, qmenu, NULL, NULL);
+			     flags, qmenu, NULL, p_def, NULL);
 
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+			const struct v4l2_ctrl_ops *ops,
+			u32 id, s64 min, s64 max, u64 step, s64 def,
+			const union v4l2_ctrl_ptr p_def)
+{
+	const char *name;
+	enum v4l2_ctrl_type type;
+	u32 flags;
+
+	v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
+	if (type == V4L2_CTRL_TYPE_MENU ||
+	    type == V4L2_CTRL_TYPE_INTEGER_MENU) {
+		handler_set_err(hdl, -EINVAL);
+		return NULL;
+	}
+	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
+			     min, max, step, def, NULL, 0,
+			     flags, NULL, NULL, p_def, NULL);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
+
 /* Helper function for standard integer menu controls */
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
@@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 	s64 max = _max;
 	s64 def = _def;
 	u32 flags;
+	const union v4l2_ctrl_ptr p_def = {};
 
 	v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
 	if (type != V4L2_CTRL_TYPE_INTEGER_MENU) {
@@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 	}
 	return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
 			     0, max, 0, def, NULL, 0,
-			     flags, NULL, qmenu_int, NULL);
+			     flags, NULL, qmenu_int, p_def, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
@@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
 	return try_or_set_cluster(fh, master, true, ch_flags);
 }
 
-/* Helper function for area controls */
-struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
-				     const struct v4l2_ctrl_ops *ops,
-				     u32 id, const struct v4l2_area *area)
-{
-	static struct v4l2_ctrl_config ctrl = {};
-	struct v4l2_ctrl *c;
-	int ret;
-
-	ctrl.id = id;
-	c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
-	if (!c)
-		return NULL;
-
-	memcpy(c->p_new.p_area, area, sizeof(*area));
-	ret = set_ctrl(NULL, c, 0);
-	if (ret){
-		hdl->error = ret;
-		return NULL;
-	}
-
-	return c;
-}
-EXPORT_SYMBOL(v4l2_ctrl_new_area);
-
 /* Helper function for VIDIOC_S_CTRL compatibility */
 static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 			 struct v4l2_ext_control *c)
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index b5b42777a756..8dc7e9827056 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
  *		manual mode. So if the value of the auto control equals this
  *		value, then the whole cluster is in manual mode. Drivers should
  *		never set this flag directly.
+ * @has_p_def:  If set, the p_def field points to the default value of the control.
  * @ops:	The control ops.
  * @type_ops:	The control type ops.
  * @id:	The control ID.
@@ -230,6 +231,7 @@ struct v4l2_ctrl {
 	unsigned int has_volatiles:1;
 	unsigned int call_notify:1;
 	unsigned int manual_mode_value:8;
+	unsigned int has_p_def:1;
 
 	const struct v4l2_ctrl_ops *ops;
 	const struct v4l2_ctrl_type_ops *type_ops;
@@ -256,6 +258,7 @@ struct v4l2_ctrl {
 		s32 val;
 	} cur;
 
+	union v4l2_ctrl_ptr p_def;
 	union v4l2_ctrl_ptr p_new;
 	union v4l2_ctrl_ptr p_cur;
 };
@@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 					       u32 id, u8 max,
 					       u64 mask, u8 def,
 					       const char * const *qmenu);
+/**
+ * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
+ *      compound control.
+ *
+ * @hdl:	The control handler.
+ * @ops:	The control ops.
+ * @id:		The control ID.
+ * @min:	The control's minimum value.
+ * @max:	The control's maximum value.
+ * @step:	The control's step value
+ * @def:	The control's default value.
+ * @p_def:	The control's p_def value.
+ *
+ * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to
+ * the @p_def field.
+ *
+ */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+				    const struct v4l2_ctrl_ops *ops,
+				    u32 id, s64 min, s64 max, u64 step,
+				    s64 def, const union v4l2_ctrl_ptr p_def);
+
 
 /**
  * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
-- 
2.23.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound()
  2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado
@ 2019-09-17 16:21   ` Ricardo Ribalda Delgado
  2019-09-20 10:06     ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-09-17 16:21 UTC (permalink / raw)
  To: Hans Verkuil, linux-media

Hi Hans

Is this something close to what you were having in mind? Right now it
sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d
but I will make it as the beginning of my patchset if you think that I
am on the right track.

Thanks!

On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado
<ribalda@kernel.org> wrote:
>
> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch,
> do not merge as is, and be gentle with the author ;P.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
>  drivers/media/i2c/imx214.c           | 13 +++--
>  drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++-----------
>  include/media/v4l2-ctrls.h           | 25 +++++++++
>  3 files changed, 81 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 625617d4c81a..e18fed9f31f1 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client)
>                 .width = 1120,
>                 .height = 1120,
>         };
> +       union v4l2_ctrl_ptr p_def = {
> +               .p_area = &unit_size,
> +       };
>         int ret;
>
>         ret = imx214_parse_fwnode(dev);
> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client)
>                                              V4L2_CID_EXPOSURE,
>                                              0, 3184, 1, 0x0c70);
>
> -       imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
> -                                              &imx214_ctrl_ops,
> -                                              V4L2_CID_UNIT_CELL_SIZE,
> -                                              &unit_size);
> -
> +       imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> +                                                      NULL,
> +                                                      V4L2_CID_UNIT_CELL_SIZE,
> +                                                      0, 0x7ffffff, 1, 0,
> +                                                      p_def);
>         ret = imx214->ctrls.error;
>         if (ret) {
>                 dev_err(&client->dev, "%s control init failed (%d)\n",
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 3d2fa1868982..04813ba2262b 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>                 p_mpeg2_slice_params->picture.picture_coding_type =
>                                         V4L2_MPEG2_PICTURE_CODING_TYPE_I;
>                 break;
> +       default:
> +               if (ctrl->has_p_def)
> +                       memcpy(p, ctrl->p_def.p, ctrl->elem_size);
> +               break;
>         }
>  }
>
> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>                         s64 min, s64 max, u64 step, s64 def,
>                         const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
>                         u32 flags, const char * const *qmenu,
> -                       const s64 *qmenu_int, void *priv)
> +                       const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
> +                       void *priv)
>  {
>         struct v4l2_ctrl *ctrl;
>         unsigned sz_extra;
> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>                  is_array)
>                 sz_extra += 2 * tot_ctrl_size;
>
> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
> +               sz_extra += elem_size;
> +
>         ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
>         if (ctrl == NULL) {
>                 handler_set_err(hdl, -ENOMEM);
> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>                 ctrl->p_new.p = &ctrl->val;
>                 ctrl->p_cur.p = &ctrl->cur.val;
>         }
> +
> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
> +               ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
> +               memcpy(ctrl->p_def.p, p_def.p, elem_size);
> +               ctrl->has_p_def = true;
> +       }
> +
>         for (idx = 0; idx < elems; idx++) {
>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>         s64 max = cfg->max;
>         u64 step = cfg->step;
>         s64 def = cfg->def;
> +       const union v4l2_ctrl_ptr p_def = {};
>
>         if (name == NULL)
>                 v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>                         type, min, max,
>                         is_menu ? cfg->menu_skip_mask : step, def,
>                         cfg->dims, cfg->elem_size,
> -                       flags, qmenu, qmenu_int, priv);
> +                       flags, qmenu, qmenu_int, p_def, priv);
>         if (ctrl)
>                 ctrl->is_private = cfg->is_private;
>         return ctrl;
> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>         const char *name;
>         enum v4l2_ctrl_type type;
>         u32 flags;
> +       const union v4l2_ctrl_ptr p_def = {};
>
>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>         if (type == V4L2_CTRL_TYPE_MENU ||
> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>                              min, max, step, def, NULL, 0,
> -                            flags, NULL, NULL, NULL);
> +                            flags, NULL, NULL, p_def, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
>
> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>         s64 def = _def;
>         u64 step;
>         u32 flags;
> +       const union v4l2_ctrl_ptr p_def = {};
>
>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>
> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>                              0, max, mask, def, NULL, 0,
> -                            flags, qmenu, qmenu_int, NULL);
> +                            flags, qmenu, qmenu_int, p_def, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
>
> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>         s64 min;
>         s64 max = _max;
>         s64 def = _def;
> +       const union v4l2_ctrl_ptr p_def = {};
>
>         /* v4l2_ctrl_new_std_menu_items() should only be called for
>          * standard controls without a standard menu.
> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>                              0, max, mask, def, NULL, 0,
> -                            flags, qmenu, NULL, NULL);
> +                            flags, qmenu, NULL, p_def, NULL);
>
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
>
> +/* Helper function for standard compound controls */
> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> +                       const struct v4l2_ctrl_ops *ops,
> +                       u32 id, s64 min, s64 max, u64 step, s64 def,
> +                       const union v4l2_ctrl_ptr p_def)
> +{
> +       const char *name;
> +       enum v4l2_ctrl_type type;
> +       u32 flags;
> +
> +       v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> +       if (type == V4L2_CTRL_TYPE_MENU ||
> +           type == V4L2_CTRL_TYPE_INTEGER_MENU) {
> +               handler_set_err(hdl, -EINVAL);
> +               return NULL;
> +       }
> +       return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> +                            min, max, step, def, NULL, 0,
> +                            flags, NULL, NULL, p_def, NULL);
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> +
>  /* Helper function for standard integer menu controls */
>  struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>                         const struct v4l2_ctrl_ops *ops,
> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>         s64 max = _max;
>         s64 def = _def;
>         u32 flags;
> +       const union v4l2_ctrl_ptr p_def = {};
>
>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>         if (type != V4L2_CTRL_TYPE_INTEGER_MENU) {
> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>         }
>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>                              0, max, 0, def, NULL, 0,
> -                            flags, NULL, qmenu_int, NULL);
> +                            flags, NULL, qmenu_int, p_def, NULL);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
>
> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>         return try_or_set_cluster(fh, master, true, ch_flags);
>  }
>
> -/* Helper function for area controls */
> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
> -                                    const struct v4l2_ctrl_ops *ops,
> -                                    u32 id, const struct v4l2_area *area)
> -{
> -       static struct v4l2_ctrl_config ctrl = {};
> -       struct v4l2_ctrl *c;
> -       int ret;
> -
> -       ctrl.id = id;
> -       c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
> -       if (!c)
> -               return NULL;
> -
> -       memcpy(c->p_new.p_area, area, sizeof(*area));
> -       ret = set_ctrl(NULL, c, 0);
> -       if (ret){
> -               hdl->error = ret;
> -               return NULL;
> -       }
> -
> -       return c;
> -}
> -EXPORT_SYMBOL(v4l2_ctrl_new_area);
> -
>  /* Helper function for VIDIOC_S_CTRL compatibility */
>  static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>                          struct v4l2_ext_control *c)
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index b5b42777a756..8dc7e9827056 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>   *             manual mode. So if the value of the auto control equals this
>   *             value, then the whole cluster is in manual mode. Drivers should
>   *             never set this flag directly.
> + * @has_p_def:  If set, the p_def field points to the default value of the control.
>   * @ops:       The control ops.
>   * @type_ops:  The control type ops.
>   * @id:        The control ID.
> @@ -230,6 +231,7 @@ struct v4l2_ctrl {
>         unsigned int has_volatiles:1;
>         unsigned int call_notify:1;
>         unsigned int manual_mode_value:8;
> +       unsigned int has_p_def:1;
>
>         const struct v4l2_ctrl_ops *ops;
>         const struct v4l2_ctrl_type_ops *type_ops;
> @@ -256,6 +258,7 @@ struct v4l2_ctrl {
>                 s32 val;
>         } cur;
>
> +       union v4l2_ctrl_ptr p_def;
>         union v4l2_ctrl_ptr p_new;
>         union v4l2_ctrl_ptr p_cur;
>  };
> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>                                                u32 id, u8 max,
>                                                u64 mask, u8 def,
>                                                const char * const *qmenu);
> +/**
> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
> + *      compound control.
> + *
> + * @hdl:       The control handler.
> + * @ops:       The control ops.
> + * @id:                The control ID.
> + * @min:       The control's minimum value.
> + * @max:       The control's maximum value.
> + * @step:      The control's step value
> + * @def:       The control's default value.
> + * @p_def:     The control's p_def value.
> + *
> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to
> + * the @p_def field.
> + *
> + */
> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> +                                   const struct v4l2_ctrl_ops *ops,
> +                                   u32 id, s64 min, s64 max, u64 step,
> +                                   s64 def, const union v4l2_ctrl_ptr p_def);
> +
>
>  /**
>   * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
> --
> 2.23.0
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound()
  2019-09-17 16:21   ` Ricardo Ribalda Delgado
@ 2019-09-20 10:06     ` Hans Verkuil
  2019-09-20 10:33       ` Ricardo Ribalda Delgado
  0 siblings, 1 reply; 7+ messages in thread
From: Hans Verkuil @ 2019-09-20 10:06 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado, linux-media

On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote:
> Hi Hans
> 
> Is this something close to what you were having in mind? Right now it
> sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d
> but I will make it as the beginning of my patchset if you think that I
> am on the right track.
> 
> Thanks!
> 
> On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado
> <ribalda@kernel.org> wrote:
>>
>> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch,
>> do not merge as is, and be gentle with the author ;P.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
>> ---
>>  drivers/media/i2c/imx214.c           | 13 +++--
>>  drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++-----------
>>  include/media/v4l2-ctrls.h           | 25 +++++++++
>>  3 files changed, 81 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
>> index 625617d4c81a..e18fed9f31f1 100644
>> --- a/drivers/media/i2c/imx214.c
>> +++ b/drivers/media/i2c/imx214.c
>> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client)
>>                 .width = 1120,
>>                 .height = 1120,
>>         };
>> +       union v4l2_ctrl_ptr p_def = {
>> +               .p_area = &unit_size,
>> +       };
>>         int ret;
>>
>>         ret = imx214_parse_fwnode(dev);
>> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client)
>>                                              V4L2_CID_EXPOSURE,
>>                                              0, 3184, 1, 0x0c70);
>>
>> -       imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
>> -                                              &imx214_ctrl_ops,
>> -                                              V4L2_CID_UNIT_CELL_SIZE,
>> -                                              &unit_size);
>> -
>> +       imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
>> +                                                      NULL,
>> +                                                      V4L2_CID_UNIT_CELL_SIZE,
>> +                                                      0, 0x7ffffff, 1, 0,
>> +                                                      p_def);
>>         ret = imx214->ctrls.error;
>>         if (ret) {
>>                 dev_err(&client->dev, "%s control init failed (%d)\n",
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 3d2fa1868982..04813ba2262b 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>                 p_mpeg2_slice_params->picture.picture_coding_type =
>>                                         V4L2_MPEG2_PICTURE_CODING_TYPE_I;
>>                 break;
>> +       default:
>> +               if (ctrl->has_p_def)
>> +                       memcpy(p, ctrl->p_def.p, ctrl->elem_size);
>> +               break;

It makes more sense to do this at the beginning of this function:

	if (ctrl->p_def.p)
		memcpy(p, ctrl->p_def.p, ctrl->elem_size);
	else
		memset(p, 0, ctrl->elem_size);

and then enter the switch.

This avoids calling memset followed by a memcpy.

>>         }
>>  }
>>
>> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>                         s64 min, s64 max, u64 step, s64 def,
>>                         const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
>>                         u32 flags, const char * const *qmenu,
>> -                       const s64 *qmenu_int, void *priv)
>> +                       const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
>> +                       void *priv)
>>  {
>>         struct v4l2_ctrl *ctrl;
>>         unsigned sz_extra;
>> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>                  is_array)
>>                 sz_extra += 2 * tot_ctrl_size;
>>
>> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
>> +               sz_extra += elem_size;
>> +
>>         ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
>>         if (ctrl == NULL) {
>>                 handler_set_err(hdl, -ENOMEM);
>> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>                 ctrl->p_new.p = &ctrl->val;
>>                 ctrl->p_cur.p = &ctrl->cur.val;
>>         }
>> +
>> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
>> +               ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
>> +               memcpy(ctrl->p_def.p, p_def.p, elem_size);
>> +               ctrl->has_p_def = true;

Is this needed? ctrl->p_def.p would be NULL if there is no p_def.

>> +       }
>> +
>>         for (idx = 0; idx < elems; idx++) {
>>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
>>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
>> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>         s64 max = cfg->max;
>>         u64 step = cfg->step;
>>         s64 def = cfg->def;
>> +       const union v4l2_ctrl_ptr p_def = {};

I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;'
at the start of the source and just use that.

>>
>>         if (name == NULL)
>>                 v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
>> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>                         type, min, max,
>>                         is_menu ? cfg->menu_skip_mask : step, def,
>>                         cfg->dims, cfg->elem_size,
>> -                       flags, qmenu, qmenu_int, priv);
>> +                       flags, qmenu, qmenu_int, p_def, priv);
>>         if (ctrl)
>>                 ctrl->is_private = cfg->is_private;
>>         return ctrl;
>> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>>         const char *name;
>>         enum v4l2_ctrl_type type;
>>         u32 flags;
>> +       const union v4l2_ctrl_ptr p_def = {};
>>
>>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>         if (type == V4L2_CTRL_TYPE_MENU ||
>> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>>         }
>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>                              min, max, step, def, NULL, 0,
>> -                            flags, NULL, NULL, NULL);
>> +                            flags, NULL, NULL, p_def, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
>>
>> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>>         s64 def = _def;
>>         u64 step;
>>         u32 flags;
>> +       const union v4l2_ctrl_ptr p_def = {};
>>
>>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>
>> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>>         }
>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>                              0, max, mask, def, NULL, 0,
>> -                            flags, qmenu, qmenu_int, NULL);
>> +                            flags, qmenu, qmenu_int, p_def, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
>>
>> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>         s64 min;
>>         s64 max = _max;
>>         s64 def = _def;
>> +       const union v4l2_ctrl_ptr p_def = {};
>>
>>         /* v4l2_ctrl_new_std_menu_items() should only be called for
>>          * standard controls without a standard menu.
>> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>         }
>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>                              0, max, mask, def, NULL, 0,
>> -                            flags, qmenu, NULL, NULL);
>> +                            flags, qmenu, NULL, p_def, NULL);
>>
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
>>
>> +/* Helper function for standard compound controls */
>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>> +                       const struct v4l2_ctrl_ops *ops,
>> +                       u32 id, s64 min, s64 max, u64 step, s64 def,

The def arg makes no sense, since that's superseded by p_def.

>> +                       const union v4l2_ctrl_ptr p_def)
>> +{
>> +       const char *name;
>> +       enum v4l2_ctrl_type type;
>> +       u32 flags;

Add:

	s64 def = min;

It will be discarded anyway.

>> +
>> +       v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>> +       if (type == V4L2_CTRL_TYPE_MENU ||
>> +           type == V4L2_CTRL_TYPE_INTEGER_MENU) {
>> +               handler_set_err(hdl, -EINVAL);
>> +               return NULL;
>> +       }

This makes no sense. It should just check that the type is a compound type and
return an error if it isn't.

>> +       return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>> +                            min, max, step, def, NULL, 0,
>> +                            flags, NULL, NULL, p_def, NULL);
>> +}
>> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
>> +
>>  /* Helper function for standard integer menu controls */
>>  struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>                         const struct v4l2_ctrl_ops *ops,
>> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>         s64 max = _max;
>>         s64 def = _def;
>>         u32 flags;
>> +       const union v4l2_ctrl_ptr p_def = {};
>>
>>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>         if (type != V4L2_CTRL_TYPE_INTEGER_MENU) {
>> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>         }
>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>                              0, max, 0, def, NULL, 0,
>> -                            flags, NULL, qmenu_int, NULL);
>> +                            flags, NULL, qmenu_int, p_def, NULL);
>>  }
>>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
>>
>> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>>         return try_or_set_cluster(fh, master, true, ch_flags);
>>  }
>>
>> -/* Helper function for area controls */
>> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
>> -                                    const struct v4l2_ctrl_ops *ops,
>> -                                    u32 id, const struct v4l2_area *area)
>> -{
>> -       static struct v4l2_ctrl_config ctrl = {};
>> -       struct v4l2_ctrl *c;
>> -       int ret;
>> -
>> -       ctrl.id = id;
>> -       c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
>> -       if (!c)
>> -               return NULL;
>> -
>> -       memcpy(c->p_new.p_area, area, sizeof(*area));
>> -       ret = set_ctrl(NULL, c, 0);
>> -       if (ret){
>> -               hdl->error = ret;
>> -               return NULL;
>> -       }
>> -
>> -       return c;
>> -}
>> -EXPORT_SYMBOL(v4l2_ctrl_new_area);
>> -
>>  /* Helper function for VIDIOC_S_CTRL compatibility */
>>  static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>>                          struct v4l2_ext_control *c)
>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>> index b5b42777a756..8dc7e9827056 100644
>> --- a/include/media/v4l2-ctrls.h
>> +++ b/include/media/v4l2-ctrls.h
>> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>   *             manual mode. So if the value of the auto control equals this
>>   *             value, then the whole cluster is in manual mode. Drivers should
>>   *             never set this flag directly.
>> + * @has_p_def:  If set, the p_def field points to the default value of the control.
>>   * @ops:       The control ops.
>>   * @type_ops:  The control type ops.
>>   * @id:        The control ID.
>> @@ -230,6 +231,7 @@ struct v4l2_ctrl {
>>         unsigned int has_volatiles:1;
>>         unsigned int call_notify:1;
>>         unsigned int manual_mode_value:8;
>> +       unsigned int has_p_def:1;
>>
>>         const struct v4l2_ctrl_ops *ops;
>>         const struct v4l2_ctrl_type_ops *type_ops;
>> @@ -256,6 +258,7 @@ struct v4l2_ctrl {
>>                 s32 val;
>>         } cur;
>>
>> +       union v4l2_ctrl_ptr p_def;
>>         union v4l2_ctrl_ptr p_new;
>>         union v4l2_ctrl_ptr p_cur;
>>  };
>> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>                                                u32 id, u8 max,
>>                                                u64 mask, u8 def,
>>                                                const char * const *qmenu);
>> +/**
>> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
>> + *      compound control.
>> + *
>> + * @hdl:       The control handler.
>> + * @ops:       The control ops.
>> + * @id:                The control ID.
>> + * @min:       The control's minimum value.
>> + * @max:       The control's maximum value.
>> + * @step:      The control's step value
>> + * @def:       The control's default value.
>> + * @p_def:     The control's p_def value.
>> + *
>> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to
>> + * the @p_def field.
>> + *
>> + */
>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>> +                                   const struct v4l2_ctrl_ops *ops,
>> +                                   u32 id, s64 min, s64 max, u64 step,
>> +                                   s64 def, const union v4l2_ctrl_ptr p_def);
>> +
>>
>>  /**
>>   * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
>> --
>> 2.23.0
>>

So other than these fairly minor points, this is indeed what I was looking for.

Regards,

	Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound()
  2019-09-20 10:06     ` Hans Verkuil
@ 2019-09-20 10:33       ` Ricardo Ribalda Delgado
  2019-09-20 10:38         ` Hans Verkuil
  0 siblings, 1 reply; 7+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-09-20 10:33 UTC (permalink / raw)
  To: Hans Verkuil; +Cc: linux-media

Hello Hans

Thanks for your review!  I will implement the changes and resend, just
one comment.


On Fri, Sep 20, 2019 at 12:07 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote:
> > Hi Hans
> >
> > Is this something close to what you were having in mind? Right now it
> > sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d
> > but I will make it as the beginning of my patchset if you think that I
> > am on the right track.
> >
> > Thanks!
> >
> > On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado
> > <ribalda@kernel.org> wrote:
> >>
> >> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch,
> >> do not merge as is, and be gentle with the author ;P.
> >>
> >> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> >> ---
> >>  drivers/media/i2c/imx214.c           | 13 +++--
> >>  drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++-----------
> >>  include/media/v4l2-ctrls.h           | 25 +++++++++
> >>  3 files changed, 81 insertions(+), 36 deletions(-)
> >>
> >> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> >> index 625617d4c81a..e18fed9f31f1 100644
> >> --- a/drivers/media/i2c/imx214.c
> >> +++ b/drivers/media/i2c/imx214.c
> >> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client)
> >>                 .width = 1120,
> >>                 .height = 1120,
> >>         };
> >> +       union v4l2_ctrl_ptr p_def = {
> >> +               .p_area = &unit_size,
> >> +       };
> >>         int ret;
> >>
> >>         ret = imx214_parse_fwnode(dev);
> >> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client)
> >>                                              V4L2_CID_EXPOSURE,
> >>                                              0, 3184, 1, 0x0c70);
> >>
> >> -       imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
> >> -                                              &imx214_ctrl_ops,
> >> -                                              V4L2_CID_UNIT_CELL_SIZE,
> >> -                                              &unit_size);
> >> -
> >> +       imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> >> +                                                      NULL,
> >> +                                                      V4L2_CID_UNIT_CELL_SIZE,
> >> +                                                      0, 0x7ffffff, 1, 0,
> >> +                                                      p_def);
> >>         ret = imx214->ctrls.error;
> >>         if (ret) {
> >>                 dev_err(&client->dev, "%s control init failed (%d)\n",
> >> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> index 3d2fa1868982..04813ba2262b 100644
> >> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> >> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> >> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> >>                 p_mpeg2_slice_params->picture.picture_coding_type =
> >>                                         V4L2_MPEG2_PICTURE_CODING_TYPE_I;
> >>                 break;
> >> +       default:
> >> +               if (ctrl->has_p_def)
> >> +                       memcpy(p, ctrl->p_def.p, ctrl->elem_size);
> >> +               break;
>
> It makes more sense to do this at the beginning of this function:
>
>         if (ctrl->p_def.p)
>                 memcpy(p, ctrl->p_def.p, ctrl->elem_size);
>         else
>                 memset(p, 0, ctrl->elem_size);
>
> and then enter the switch.
>
> This avoids calling memset followed by a memcpy.
>
> >>         }
> >>  }
> >>
> >> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>                         s64 min, s64 max, u64 step, s64 def,
> >>                         const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
> >>                         u32 flags, const char * const *qmenu,
> >> -                       const s64 *qmenu_int, void *priv)
> >> +                       const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
> >> +                       void *priv)
> >>  {
> >>         struct v4l2_ctrl *ctrl;
> >>         unsigned sz_extra;
> >> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>                  is_array)
> >>                 sz_extra += 2 * tot_ctrl_size;
> >>
> >> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
> >> +               sz_extra += elem_size;
> >> +
> >>         ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
> >>         if (ctrl == NULL) {
> >>                 handler_set_err(hdl, -ENOMEM);
> >> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> >>                 ctrl->p_new.p = &ctrl->val;
> >>                 ctrl->p_cur.p = &ctrl->cur.val;
> >>         }
> >> +
> >> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
> >> +               ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
> >> +               memcpy(ctrl->p_def.p, p_def.p, elem_size);
> >> +               ctrl->has_p_def = true;
>
> Is this needed? ctrl->p_def.p would be NULL if there is no p_def.
>
> >> +       }
> >> +
> >>         for (idx = 0; idx < elems; idx++) {
> >>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
> >>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> >> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
> >>         s64 max = cfg->max;
> >>         u64 step = cfg->step;
> >>         s64 def = cfg->def;
> >> +       const union v4l2_ctrl_ptr p_def = {};
>
> I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;'
> at the start of the source and just use that.
>
> >>
> >>         if (name == NULL)
> >>                 v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
> >> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
> >>                         type, min, max,
> >>                         is_menu ? cfg->menu_skip_mask : step, def,
> >>                         cfg->dims, cfg->elem_size,
> >> -                       flags, qmenu, qmenu_int, priv);
> >> +                       flags, qmenu, qmenu_int, p_def, priv);
> >>         if (ctrl)
> >>                 ctrl->is_private = cfg->is_private;
> >>         return ctrl;
> >> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
> >>         const char *name;
> >>         enum v4l2_ctrl_type type;
> >>         u32 flags;
> >> +       const union v4l2_ctrl_ptr p_def = {};
> >>
> >>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> >>         if (type == V4L2_CTRL_TYPE_MENU ||
> >> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
> >>         }
> >>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> >>                              min, max, step, def, NULL, 0,
> >> -                            flags, NULL, NULL, NULL);
> >> +                            flags, NULL, NULL, p_def, NULL);
> >>  }
> >>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
> >>
> >> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
> >>         s64 def = _def;
> >>         u64 step;
> >>         u32 flags;
> >> +       const union v4l2_ctrl_ptr p_def = {};
> >>
> >>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> >>
> >> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
> >>         }
> >>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> >>                              0, max, mask, def, NULL, 0,
> >> -                            flags, qmenu, qmenu_int, NULL);
> >> +                            flags, qmenu, qmenu_int, p_def, NULL);
> >>  }
> >>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
> >>
> >> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> >>         s64 min;
> >>         s64 max = _max;
> >>         s64 def = _def;
> >> +       const union v4l2_ctrl_ptr p_def = {};
> >>
> >>         /* v4l2_ctrl_new_std_menu_items() should only be called for
> >>          * standard controls without a standard menu.
> >> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> >>         }
> >>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> >>                              0, max, mask, def, NULL, 0,
> >> -                            flags, qmenu, NULL, NULL);
> >> +                            flags, qmenu, NULL, p_def, NULL);
> >>
> >>  }
> >>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
> >>
> >> +/* Helper function for standard compound controls */
> >> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> >> +                       const struct v4l2_ctrl_ops *ops,
> >> +                       u32 id, s64 min, s64 max, u64 step, s64 def,
>
> The def arg makes no sense, since that's superseded by p_def.
>

If def does not make sense, then for the same reason min, max and step
should not make any sense (because they are not a pointer but a
integer).
What about removing them completely from the function prototype and
add them later if we change our mind, it is a internal API.

> >> +                       const union v4l2_ctrl_ptr p_def)
> >> +{
> >> +       const char *name;
> >> +       enum v4l2_ctrl_type type;
> >> +       u32 flags;
>
> Add:
>
>         s64 def = min;
>
> It will be discarded anyway.
>
> >> +
> >> +       v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> >> +       if (type == V4L2_CTRL_TYPE_MENU ||
> >> +           type == V4L2_CTRL_TYPE_INTEGER_MENU) {
> >> +               handler_set_err(hdl, -EINVAL);
> >> +               return NULL;
> >> +       }
>
> This makes no sense. It should just check that the type is a compound type and
> return an error if it isn't.
>
> >> +       return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> >> +                            min, max, step, def, NULL, 0,
> >> +                            flags, NULL, NULL, p_def, NULL);
> >> +}
> >> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> >> +
> >>  /* Helper function for standard integer menu controls */
> >>  struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
> >>                         const struct v4l2_ctrl_ops *ops,
> >> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
> >>         s64 max = _max;
> >>         s64 def = _def;
> >>         u32 flags;
> >> +       const union v4l2_ctrl_ptr p_def = {};
> >>
> >>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> >>         if (type != V4L2_CTRL_TYPE_INTEGER_MENU) {
> >> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
> >>         }
> >>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> >>                              0, max, 0, def, NULL, 0,
> >> -                            flags, NULL, qmenu_int, NULL);
> >> +                            flags, NULL, qmenu_int, p_def, NULL);
> >>  }
> >>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
> >>
> >> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
> >>         return try_or_set_cluster(fh, master, true, ch_flags);
> >>  }
> >>
> >> -/* Helper function for area controls */
> >> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
> >> -                                    const struct v4l2_ctrl_ops *ops,
> >> -                                    u32 id, const struct v4l2_area *area)
> >> -{
> >> -       static struct v4l2_ctrl_config ctrl = {};
> >> -       struct v4l2_ctrl *c;
> >> -       int ret;
> >> -
> >> -       ctrl.id = id;
> >> -       c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
> >> -       if (!c)
> >> -               return NULL;
> >> -
> >> -       memcpy(c->p_new.p_area, area, sizeof(*area));
> >> -       ret = set_ctrl(NULL, c, 0);
> >> -       if (ret){
> >> -               hdl->error = ret;
> >> -               return NULL;
> >> -       }
> >> -
> >> -       return c;
> >> -}
> >> -EXPORT_SYMBOL(v4l2_ctrl_new_area);
> >> -
> >>  /* Helper function for VIDIOC_S_CTRL compatibility */
> >>  static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> >>                          struct v4l2_ext_control *c)
> >> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> >> index b5b42777a756..8dc7e9827056 100644
> >> --- a/include/media/v4l2-ctrls.h
> >> +++ b/include/media/v4l2-ctrls.h
> >> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> >>   *             manual mode. So if the value of the auto control equals this
> >>   *             value, then the whole cluster is in manual mode. Drivers should
> >>   *             never set this flag directly.
> >> + * @has_p_def:  If set, the p_def field points to the default value of the control.
> >>   * @ops:       The control ops.
> >>   * @type_ops:  The control type ops.
> >>   * @id:        The control ID.
> >> @@ -230,6 +231,7 @@ struct v4l2_ctrl {
> >>         unsigned int has_volatiles:1;
> >>         unsigned int call_notify:1;
> >>         unsigned int manual_mode_value:8;
> >> +       unsigned int has_p_def:1;
> >>
> >>         const struct v4l2_ctrl_ops *ops;
> >>         const struct v4l2_ctrl_type_ops *type_ops;
> >> @@ -256,6 +258,7 @@ struct v4l2_ctrl {
> >>                 s32 val;
> >>         } cur;
> >>
> >> +       union v4l2_ctrl_ptr p_def;
> >>         union v4l2_ctrl_ptr p_new;
> >>         union v4l2_ctrl_ptr p_cur;
> >>  };
> >> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> >>                                                u32 id, u8 max,
> >>                                                u64 mask, u8 def,
> >>                                                const char * const *qmenu);
> >> +/**
> >> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
> >> + *      compound control.
> >> + *
> >> + * @hdl:       The control handler.
> >> + * @ops:       The control ops.
> >> + * @id:                The control ID.
> >> + * @min:       The control's minimum value.
> >> + * @max:       The control's maximum value.
> >> + * @step:      The control's step value
> >> + * @def:       The control's default value.
> >> + * @p_def:     The control's p_def value.
> >> + *
> >> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to
> >> + * the @p_def field.
> >> + *
> >> + */
> >> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> >> +                                   const struct v4l2_ctrl_ops *ops,
> >> +                                   u32 id, s64 min, s64 max, u64 step,
> >> +                                   s64 def, const union v4l2_ctrl_ptr p_def);
> >> +
> >>
> >>  /**
> >>   * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
> >> --
> >> 2.23.0
> >>
>
> So other than these fairly minor points, this is indeed what I was looking for.
>
> Regards,
>
>         Hans

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound()
  2019-09-20 10:33       ` Ricardo Ribalda Delgado
@ 2019-09-20 10:38         ` Hans Verkuil
  0 siblings, 0 replies; 7+ messages in thread
From: Hans Verkuil @ 2019-09-20 10:38 UTC (permalink / raw)
  To: Ricardo Ribalda Delgado; +Cc: linux-media

On 9/20/19 12:33 PM, Ricardo Ribalda Delgado wrote:
> Hello Hans
> 
> Thanks for your review!  I will implement the changes and resend, just
> one comment.
> 
> 
> On Fri, Sep 20, 2019 at 12:07 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>> On 9/17/19 6:21 PM, Ricardo Ribalda Delgado wrote:
>>> Hi Hans
>>>
>>> Is this something close to what you were having in mind? Right now it
>>> sits on https://github.com/ribalda/linux/commit/de21dbc2f57b58b22f5d73bc314dd8e59dff5c7d
>>> but I will make it as the beginning of my patchset if you think that I
>>> am on the right track.
>>>
>>> Thanks!
>>>
>>> On Tue, Sep 17, 2019 at 6:19 PM Ricardo Ribalda Delgado
>>> <ribalda@kernel.org> wrote:
>>>>
>>>> Implement v4l2_ctrl_new_std_compound. This is just a discussion patch,
>>>> do not merge as is, and be gentle with the author ;P.
>>>>
>>>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
>>>> ---
>>>>  drivers/media/i2c/imx214.c           | 13 +++--
>>>>  drivers/media/v4l2-core/v4l2-ctrls.c | 79 +++++++++++++++++-----------
>>>>  include/media/v4l2-ctrls.h           | 25 +++++++++
>>>>  3 files changed, 81 insertions(+), 36 deletions(-)
>>>>
>>>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
>>>> index 625617d4c81a..e18fed9f31f1 100644
>>>> --- a/drivers/media/i2c/imx214.c
>>>> +++ b/drivers/media/i2c/imx214.c
>>>> @@ -953,6 +953,9 @@ static int imx214_probe(struct i2c_client *client)
>>>>                 .width = 1120,
>>>>                 .height = 1120,
>>>>         };
>>>> +       union v4l2_ctrl_ptr p_def = {
>>>> +               .p_area = &unit_size,
>>>> +       };
>>>>         int ret;
>>>>
>>>>         ret = imx214_parse_fwnode(dev);
>>>> @@ -1034,11 +1037,11 @@ static int imx214_probe(struct i2c_client *client)
>>>>                                              V4L2_CID_EXPOSURE,
>>>>                                              0, 3184, 1, 0x0c70);
>>>>
>>>> -       imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
>>>> -                                              &imx214_ctrl_ops,
>>>> -                                              V4L2_CID_UNIT_CELL_SIZE,
>>>> -                                              &unit_size);
>>>> -
>>>> +       imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
>>>> +                                                      NULL,
>>>> +                                                      V4L2_CID_UNIT_CELL_SIZE,
>>>> +                                                      0, 0x7ffffff, 1, 0,
>>>> +                                                      p_def);
>>>>         ret = imx214->ctrls.error;
>>>>         if (ret) {
>>>>                 dev_err(&client->dev, "%s control init failed (%d)\n",
>>>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> index 3d2fa1868982..04813ba2262b 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>>>> @@ -1555,6 +1555,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
>>>>                 p_mpeg2_slice_params->picture.picture_coding_type =
>>>>                                         V4L2_MPEG2_PICTURE_CODING_TYPE_I;
>>>>                 break;
>>>> +       default:
>>>> +               if (ctrl->has_p_def)
>>>> +                       memcpy(p, ctrl->p_def.p, ctrl->elem_size);
>>>> +               break;
>>
>> It makes more sense to do this at the beginning of this function:
>>
>>         if (ctrl->p_def.p)
>>                 memcpy(p, ctrl->p_def.p, ctrl->elem_size);
>>         else
>>                 memset(p, 0, ctrl->elem_size);
>>
>> and then enter the switch.
>>
>> This avoids calling memset followed by a memcpy.
>>
>>>>         }
>>>>  }
>>>>
>>>> @@ -2369,7 +2373,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>>                         s64 min, s64 max, u64 step, s64 def,
>>>>                         const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
>>>>                         u32 flags, const char * const *qmenu,
>>>> -                       const s64 *qmenu_int, void *priv)
>>>> +                       const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
>>>> +                       void *priv)
>>>>  {
>>>>         struct v4l2_ctrl *ctrl;
>>>>         unsigned sz_extra;
>>>> @@ -2478,6 +2483,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>>                  is_array)
>>>>                 sz_extra += 2 * tot_ctrl_size;
>>>>
>>>> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
>>>> +               sz_extra += elem_size;
>>>> +
>>>>         ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
>>>>         if (ctrl == NULL) {
>>>>                 handler_set_err(hdl, -ENOMEM);
>>>> @@ -2521,6 +2529,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>>>                 ctrl->p_new.p = &ctrl->val;
>>>>                 ctrl->p_cur.p = &ctrl->cur.val;
>>>>         }
>>>> +
>>>> +       if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
>>>> +               ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
>>>> +               memcpy(ctrl->p_def.p, p_def.p, elem_size);
>>>> +               ctrl->has_p_def = true;
>>
>> Is this needed? ctrl->p_def.p would be NULL if there is no p_def.
>>
>>>> +       }
>>>> +
>>>>         for (idx = 0; idx < elems; idx++) {
>>>>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
>>>>                 ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
>>>> @@ -2550,6 +2565,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>>>         s64 max = cfg->max;
>>>>         u64 step = cfg->step;
>>>>         s64 def = cfg->def;
>>>> +       const union v4l2_ctrl_ptr p_def = {};
>>
>> I think it is cleaner to have a 'static const union v4l2_ctrl_ptr ptr_null;'
>> at the start of the source and just use that.
>>
>>>>
>>>>         if (name == NULL)
>>>>                 v4l2_ctrl_fill(cfg->id, &name, &type, &min, &max, &step,
>>>> @@ -2572,7 +2588,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
>>>>                         type, min, max,
>>>>                         is_menu ? cfg->menu_skip_mask : step, def,
>>>>                         cfg->dims, cfg->elem_size,
>>>> -                       flags, qmenu, qmenu_int, priv);
>>>> +                       flags, qmenu, qmenu_int, p_def, priv);
>>>>         if (ctrl)
>>>>                 ctrl->is_private = cfg->is_private;
>>>>         return ctrl;
>>>> @@ -2587,6 +2603,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>>>>         const char *name;
>>>>         enum v4l2_ctrl_type type;
>>>>         u32 flags;
>>>> +       const union v4l2_ctrl_ptr p_def = {};
>>>>
>>>>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>>>         if (type == V4L2_CTRL_TYPE_MENU ||
>>>> @@ -2597,7 +2614,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
>>>>         }
>>>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>>>                              min, max, step, def, NULL, 0,
>>>> -                            flags, NULL, NULL, NULL);
>>>> +                            flags, NULL, NULL, p_def, NULL);
>>>>  }
>>>>  EXPORT_SYMBOL(v4l2_ctrl_new_std);
>>>>
>>>> @@ -2616,6 +2633,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>>>>         s64 def = _def;
>>>>         u64 step;
>>>>         u32 flags;
>>>> +       const union v4l2_ctrl_ptr p_def = {};
>>>>
>>>>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>>>
>>>> @@ -2630,7 +2648,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
>>>>         }
>>>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>>>                              0, max, mask, def, NULL, 0,
>>>> -                            flags, qmenu, qmenu_int, NULL);
>>>> +                            flags, qmenu, qmenu_int, p_def, NULL);
>>>>  }
>>>>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
>>>>
>>>> @@ -2646,6 +2664,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>>>         s64 min;
>>>>         s64 max = _max;
>>>>         s64 def = _def;
>>>> +       const union v4l2_ctrl_ptr p_def = {};
>>>>
>>>>         /* v4l2_ctrl_new_std_menu_items() should only be called for
>>>>          * standard controls without a standard menu.
>>>> @@ -2662,11 +2681,33 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>>>         }
>>>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>>>                              0, max, mask, def, NULL, 0,
>>>> -                            flags, qmenu, NULL, NULL);
>>>> +                            flags, qmenu, NULL, p_def, NULL);
>>>>
>>>>  }
>>>>  EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
>>>>
>>>> +/* Helper function for standard compound controls */
>>>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>>>> +                       const struct v4l2_ctrl_ops *ops,
>>>> +                       u32 id, s64 min, s64 max, u64 step, s64 def,
>>
>> The def arg makes no sense, since that's superseded by p_def.
>>
> 
> If def does not make sense, then for the same reason min, max and step
> should not make any sense (because they are not a pointer but a
> integer).
> What about removing them completely from the function prototype and
> add them later if we change our mind, it is a internal API.

I've been going back and forth about whether or not min/max/step is useful
for e.g. this AREA control as it would still help in validating a range
for the width and height. But I think you are right, and it is better to
just drop this and mark min/max/step/def as N/A in the documentation for
these compound controls.

We DO need basic validation for AREA (i.e. the width/height shall be > 0),
but that's easy enough.

Regards,

	Hans

> 
>>>> +                       const union v4l2_ctrl_ptr p_def)
>>>> +{
>>>> +       const char *name;
>>>> +       enum v4l2_ctrl_type type;
>>>> +       u32 flags;
>>
>> Add:
>>
>>         s64 def = min;
>>
>> It will be discarded anyway.
>>
>>>> +
>>>> +       v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>>> +       if (type == V4L2_CTRL_TYPE_MENU ||
>>>> +           type == V4L2_CTRL_TYPE_INTEGER_MENU) {
>>>> +               handler_set_err(hdl, -EINVAL);
>>>> +               return NULL;
>>>> +       }
>>
>> This makes no sense. It should just check that the type is a compound type and
>> return an error if it isn't.
>>
>>>> +       return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>>> +                            min, max, step, def, NULL, 0,
>>>> +                            flags, NULL, NULL, p_def, NULL);
>>>> +}
>>>> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
>>>> +
>>>>  /* Helper function for standard integer menu controls */
>>>>  struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>>>                         const struct v4l2_ctrl_ops *ops,
>>>> @@ -2679,6 +2720,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>>>         s64 max = _max;
>>>>         s64 def = _def;
>>>>         u32 flags;
>>>> +       const union v4l2_ctrl_ptr p_def = {};
>>>>
>>>>         v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
>>>>         if (type != V4L2_CTRL_TYPE_INTEGER_MENU) {
>>>> @@ -2687,7 +2729,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
>>>>         }
>>>>         return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
>>>>                              0, max, 0, def, NULL, 0,
>>>> -                            flags, NULL, qmenu_int, NULL);
>>>> +                            flags, NULL, qmenu_int, p_def, NULL);
>>>>  }
>>>>  EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
>>>>
>>>> @@ -4030,31 +4072,6 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>>>>         return try_or_set_cluster(fh, master, true, ch_flags);
>>>>  }
>>>>
>>>> -/* Helper function for area controls */
>>>> -struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
>>>> -                                    const struct v4l2_ctrl_ops *ops,
>>>> -                                    u32 id, const struct v4l2_area *area)
>>>> -{
>>>> -       static struct v4l2_ctrl_config ctrl = {};
>>>> -       struct v4l2_ctrl *c;
>>>> -       int ret;
>>>> -
>>>> -       ctrl.id = id;
>>>> -       c = v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
>>>> -       if (!c)
>>>> -               return NULL;
>>>> -
>>>> -       memcpy(c->p_new.p_area, area, sizeof(*area));
>>>> -       ret = set_ctrl(NULL, c, 0);
>>>> -       if (ret){
>>>> -               hdl->error = ret;
>>>> -               return NULL;
>>>> -       }
>>>> -
>>>> -       return c;
>>>> -}
>>>> -EXPORT_SYMBOL(v4l2_ctrl_new_area);
>>>> -
>>>>  /* Helper function for VIDIOC_S_CTRL compatibility */
>>>>  static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>>>>                          struct v4l2_ext_control *c)
>>>> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
>>>> index b5b42777a756..8dc7e9827056 100644
>>>> --- a/include/media/v4l2-ctrls.h
>>>> +++ b/include/media/v4l2-ctrls.h
>>>> @@ -164,6 +164,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>>>>   *             manual mode. So if the value of the auto control equals this
>>>>   *             value, then the whole cluster is in manual mode. Drivers should
>>>>   *             never set this flag directly.
>>>> + * @has_p_def:  If set, the p_def field points to the default value of the control.
>>>>   * @ops:       The control ops.
>>>>   * @type_ops:  The control type ops.
>>>>   * @id:        The control ID.
>>>> @@ -230,6 +231,7 @@ struct v4l2_ctrl {
>>>>         unsigned int has_volatiles:1;
>>>>         unsigned int call_notify:1;
>>>>         unsigned int manual_mode_value:8;
>>>> +       unsigned int has_p_def:1;
>>>>
>>>>         const struct v4l2_ctrl_ops *ops;
>>>>         const struct v4l2_ctrl_type_ops *type_ops;
>>>> @@ -256,6 +258,7 @@ struct v4l2_ctrl {
>>>>                 s32 val;
>>>>         } cur;
>>>>
>>>> +       union v4l2_ctrl_ptr p_def;
>>>>         union v4l2_ctrl_ptr p_new;
>>>>         union v4l2_ctrl_ptr p_cur;
>>>>  };
>>>> @@ -647,6 +650,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
>>>>                                                u32 id, u8 max,
>>>>                                                u64 mask, u8 def,
>>>>                                                const char * const *qmenu);
>>>> +/**
>>>> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
>>>> + *      compound control.
>>>> + *
>>>> + * @hdl:       The control handler.
>>>> + * @ops:       The control ops.
>>>> + * @id:                The control ID.
>>>> + * @min:       The control's minimum value.
>>>> + * @max:       The control's maximum value.
>>>> + * @step:      The control's step value
>>>> + * @def:       The control's default value.
>>>> + * @p_def:     The control's p_def value.
>>>> + *
>>>> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks to
>>>> + * the @p_def field.
>>>> + *
>>>> + */
>>>> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
>>>> +                                   const struct v4l2_ctrl_ops *ops,
>>>> +                                   u32 id, s64 min, s64 max, u64 step,
>>>> +                                   s64 def, const union v4l2_ctrl_ptr p_def);
>>>> +
>>>>
>>>>  /**
>>>>   * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
>>>> --
>>>> 2.23.0
>>>>
>>
>> So other than these fairly minor points, this is indeed what I was looking for.
>>
>> Regards,
>>
>>         Hans


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2019-09-20 10:39 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-12 13:35 Proposal for a v4l2_ctrl_new_std_compound() function Hans Verkuil
2019-09-13 16:08 ` kbuild test robot
2019-09-17 16:19 ` [PATCH] RFC: v4l2-ctrls: Inmplement v4l2_ctrl_new_std_compound() Ricardo Ribalda Delgado
2019-09-17 16:21   ` Ricardo Ribalda Delgado
2019-09-20 10:06     ` Hans Verkuil
2019-09-20 10:33       ` Ricardo Ribalda Delgado
2019-09-20 10:38         ` Hans Verkuil

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).