Re: [PATCH v1 1/1] treewide: Align match_string() with sysfs_match_string()
Hi Andy,
On Sun, Jun 02, 2024 at 06:57:12PM +0300, Andy Shevchenko wrote:
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 503773707e01..9cb350de30f0 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -798,7 +798,7 @@ static bool acpi_info_matches_ids(struct acpi_device_info
> *info,
> if (!(info->valid & ACPI_VALID_HID))
> return false;
>
> - index = match_string(ids, -1, info->hardware_id.string);
> + index = __match_string(ids, -1, info->hardware_id.string);
> if (index >= 0)
> return true;
>
> @@ -809,7 +809,7 @@ static bool acpi_info_matches_ids(struct acpi_device_info
> *info,
> return false;
>
> for (i = 0; i < cid_list->count; i++) {
> - index = match_string(ids, -1, cid_list->ids[i].string);
> + index = __match_string(ids, -1, cid_list->ids[i].string);
> if (index >= 0)
> return true;
> }
Reviewed-by: Sakari Ailus # drivers/acpi
--
Sakari Ailus
Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT
LL, 0, 0,
>flags, NULL, NULL, p_def, NULL);
> }
> EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> @@ -2173,7 +2219,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct
> v4l2_ctrl_handler *hdl,
> return NULL;
> }
> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> - 0, max, 0, def, NULL, 0,
> + 0, max, 0, def, NULL, 0, 0,
>flags, NULL, qmenu_int, ptr_null, NULL);
> }
> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 59679a42b3e7..c35514c5bf88 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -211,7 +211,8 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl
> *ctrl, void *priv);
> * except for dynamic arrays. In that case it is in the range of
> * 1 to @p_array_alloc_elems.
> * @dims:The size of each dimension.
> - * @nr_of_dims:The number of dimensions in @dims.
> + * @nr_of_dims: The number of dimensions in @dims.
> + * @fraction_bits: The number of fraction bits for fixed point values.
> * @menu_skip_mask: The control's skip mask for menu controls. This makes it
> * easy to skip menu items that are not valid. If bit X is set,
> * then menu item X is skipped. Of course, this only works for
> @@ -228,6 +229,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl
> *ctrl, void *priv);
> * :math:`ceil(\frac{maximum - minimum}{step}) + 1`.
> * Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
> * @flags: The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
> * @priv:The control's private pointer. For use by the driver. It is
> * untouched by the control framework. Note that this pointer is
> * not freed when the control is deleted. Should this be needed
> @@ -286,6 +288,7 @@ struct v4l2_ctrl {
> u32 new_elems;
> u32 dims[V4L2_CTRL_MAX_DIMS];
> u32 nr_of_dims;
> + u32 fraction_bits;
> union {
> u64 step;
> u64 menu_skip_mask;
> @@ -426,6 +429,7 @@ struct v4l2_ctrl_handler {
> * @dims:The size of each dimension.
> * @elem_size: The size in bytes of the control.
> * @flags: The control's flags.
> + * @fraction_bits: The number of fraction bits for fixed point values.
> * @menu_skip_mask: The control's skip mask for menu controls. This makes it
> * easy to skip menu items that are not valid. If bit X is set,
> * then menu item X is skipped. Of course, this only works for
> @@ -455,6 +459,7 @@ struct v4l2_ctrl_config {
> u32 dims[V4L2_CTRL_MAX_DIMS];
> u32 elem_size;
> u32 flags;
> + u32 fraction_bits;
> u64 menu_skip_mask;
> const char * const *qmenu;
> const s64 *qmenu_int;
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index c3d4e490ce7c..26ecac19722a 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1944,9 +1944,27 @@ struct v4l2_query_ext_ctrl {
> __u32elems;
> __u32nr_of_dims;
> __u32dims[V4L2_CTRL_MAX_DIMS];
> - __u32reserved[32];
> + __u32fraction_bits;
u8 would suffice. Not that we'd be short of space but still...
> + __u32reserved[31];
> };
>
> +static inline __s64 v4l2_fp_compose(__s64 i, __s64 f, unsigned int
> fraction_bits)
> +{
> + return (i << fraction_bits) + f;
> +}
> +
> +static inline __s64 v4l2_fp_integer(__s64 v, unsigned int fraction_bits)
> +{
> + return v / (1LL << fraction_bits);
Why not just:
return v >> fraction_bits;
I'd use macros so you could use whatever control types with this without
casting. E.g.
#define V4L2_FP_INTEGER(v, fraction_bits) ((v) >> fraction_bits)
A more generic way to expose this could be to have base and exponent, the
base being 2 in this case. Just an idea. This would of course be a little
bit more difficult to use.
> +}
> +
> +static inline __s64 v4l2_fp_fraction(__s64 v, unsigned int fraction_bits)
> +{
> + __u64 mask = (1ULL << fraction_bits) - 1;
> +
> + return v < 0 ? -((-v) & mask) : (v & mask);
> +}
> +
> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
> struct v4l2_querymenu {
> __u32 id;
--
Kind regards,
Sakari Ailus
Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT
Hi Hans, On Mon, Nov 13, 2023 at 12:24:14PM +0100, Hans Verkuil wrote: > On 13/11/2023 12:07, Laurent Pinchart wrote: > > On Mon, Nov 13, 2023 at 11:56:49AM +0100, Hans Verkuil wrote: > >> On 13/11/2023 11:42, Laurent Pinchart wrote: > >>> On Mon, Nov 13, 2023 at 11:29:09AM +0100, Hans Verkuil wrote: > >>>> Hi Shengjiu, > >>>> > >>>> On 10/11/2023 06:48, Shengjiu Wang wrote: > >>>>> Fixed point controls are used by the user to configure > >>>>> a fixed point value in 64bits, which Q31.32 format. > >>>>> > >>>>> Signed-off-by: Shengjiu Wang > >>>> > >>>> This patch adds a new control type. This is something that also needs to > >>>> be > >>>> tested by v4l2-compliance, and for that we need to add support for this > >>>> to > >>>> one of the media test-drivers. The best place for that is the vivid > >>>> driver, > >>>> since that has already a bunch of test controls for other control types. > >>>> > >>>> See e.g. VIVID_CID_INTEGER64 in vivid-ctrls.c. > >>>> > >>>> Can you add a patch adding a fixed point test control to vivid? > >>> > >>> I don't think V4L2_CTRL_TYPE_FIXED_POINT is a good idea. This seems to > >>> relate more to units than control types. We have lots of fixed-point > >>> values in controls already, using the 32-bit and 64-bit integer control > >>> types. They use various locations for the decimal point, depending on > >>> the control. If we want to make this more explicit to users, we should > >>> work on adding unit support to the V4L2 controls. > >> > >> "Fixed Point" is not a unit, it's a type. 'Db', 'Hz' etc. are units. > > > > It's not a unit, but I think it's related to units. My point is that, > > without units support, I don't see why we need a formal definition of > > fixed-point types, and why this series couldn't just use > > VIVID_CID_INTEGER64. Drivers already interpret VIVID_CID_INTEGER64 > > values as they see fit. > > They do? That's new to me. A quick grep for V4L2_CTRL_TYPE_INTEGER64 > (I assume you meant that rather than VIVID_CID_INTEGER64) shows that it > is always interpreted as a 64 bit integer and nothing else. As it should. > > And while we do not have support for units (other than the documentation), > we do have type support in the form of V4L2_CTRL_TYPE_*. > > > > >> A quick "git grep -i "fixed point" Documentation/userspace-api/media/' > >> only shows a single driver specific control (dw100.rst). > >> > >> I'm not aware of other controls in mainline that use fixed point. > > > > The analog gain control for sensors for instance. > > Not really. The documentation is super vague: > > V4L2_CID_ANALOGUE_GAIN (integer) > > Analogue gain is gain affecting all colour components in the pixel > matrix. The > gain operation is performed in the analogue domain before A/D > conversion. > > And the integer is just a range. Internally it might map to some fixed > point value, but userspace won't see that, it's hidden in the driver AFAICT. I wonder if Laurent meant digital gain. Those are often Q numbers. The practice there has been that the default value yields gain of 1. There are probably many other examples in controls where something being controlled isn't actually an integer while integer controls are still being used for the purpose. Instead of this patch, I'd prefer to have a way to express the meaning of the control value, be it a Q number or something else, and do that independently of the type of the control. > > In the case of this particular series the control type is really a fixed point > value with a documented unit (Hz). It really is not something you want to > use type INTEGER64 for. > > > > >> Note that V4L2_CTRL_TYPE_FIXED_POINT is a Q31.32 format. By setting > >> min/max/step you can easily map that to just about any QN.M format where > >> N <= 31 and M <= 32. > >> > >> In the case of dw100 it is a bit different in that it is quite specialized > >> and it had to fit in 16 bits. -- Regards, Sakari Ailus
Re: [RFC PATCH v3 6/9] media: v4l2: Add audio capture and output support
Hi Shengjiu,
On Tue, Sep 19, 2023 at 06:31:09PM +0800, Shengjiu Wang wrote:
...
> > > +*
> > > +V4L2_AUDIO_FMT_LPCM ('LPCM')
> > > +*
Something to fix here, too...?
> > > +
> > > +Linear Pulse-Code Modulation (LPCM)
> > > +
> > > +
> > > +Description
> > > +===
> > > +
> > > +This describes audio format used by the audio memory to memory driver.
> > > +
> > > +It contains the following fields:
> > > +
> > > +.. flat-table::
> > > +:widths: 1 4
> > > +:header-rows: 1
> > > +:stub-columns: 0
> > > +
> > > +* - Field
> > > + - Description
> > > +* - u32 samplerate;
> > > + - which is the number of times per second that samples are taken.
> > > +* - u32 sampleformat;
> > > + - which determines the number of possible digital values that can
> > > be used to represent each sample
> >
> > 80 characters (or less) per line, please.
>
> Ok, will change it.
>
> >
> > Which values could this field have and what do they signify?
>
> The values are SNDRV_PCM_FORMAT_S8, SNDRV_PCM_FORMAT_U8...
> which are the PCM format, defined in ALSA.
I suppose this is documented in ALSA documentation. Could you refer to
that?
>
> >
> > > +* - u32 channels;
> > > + - channel number for each sample.
> >
> > I suppose the rest of the buffer would be samples? This should be
> > documented. I think there are also different ways the data could be
> > arrangeed and this needs to be documented, too.
>
> All data in the buffer are the samples, the 'samplerate', 'sampleformat'
> 'channels' I list here is try to describe the samples.
> I was confused how to write this document, so I list the characters.
The layout of this data in memory needs to be documented. I think a
reference to ALSA documentation would be the best.
--
Regards,
Sakari Ailus
Re: [RFC PATCH v3 6/9] media: v4l2: Add audio capture and output support
t; +to the basic :ref:`format` ioctls, the :c:func:`VIDIOC_ENUM_FMT` ioctl must
> be
> +supported as well.
> +
> +To use the :ref:`format` ioctls applications set the ``type`` field of the
> +:c:type:`v4l2_format` structure to ``V4L2_BUF_TYPE_AUDIO_CAPTURE`` or to
> +``V4L2_BUF_TYPE_AUDIO_OUTPUT``. Both drivers and applications must set the
> +remainder of the :c:type:`v4l2_format` structure to 0.
> +
> +.. c:type:: v4l2_audio_format
> +
> +.. tabularcolumns:: |p{1.4cm}|p{2.4cm}|p{13.5cm}|
> +
> +.. flat-table:: struct v4l2_audio_format
> +:header-rows: 0
> +:stub-columns: 0
> +:widths: 1 1 2
> +
> +* - __u32
> + - ``rate``
> + - The sample rate, set by the application. The range is [5512, 768000].
> +* - __u32
> + - ``format``
> + - The sample format, set by the application. format is defined as
> +SNDRV_PCM_FORMAT_S8, SNDRV_PCM_FORMAT_U8, ...,
> +* - __u32
> + - ``channels``
> + - The channel number, set by the application. channel number range is
> +[1, 32].
> +* - __u32
> + - ``buffersize``
> + - Maximum buffer size in bytes required for data. The value is set by
> the
> +driver.
> diff --git a/Documentation/userspace-api/media/v4l/devices.rst
> b/Documentation/userspace-api/media/v4l/devices.rst
> index 8bfbad65a9d4..8261f3468489 100644
> --- a/Documentation/userspace-api/media/v4l/devices.rst
> +++ b/Documentation/userspace-api/media/v4l/devices.rst
> @@ -24,3 +24,4 @@ Interfaces
> dev-event
> dev-subdev
> dev-meta
> +dev-audio
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-aud-lpcm.rst
> b/Documentation/userspace-api/media/v4l/pixfmt-aud-lpcm.rst
> new file mode 100644
> index ..f9ebe2a05f69
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-aud-lpcm.rst
> @@ -0,0 +1,31 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _v4l2-aud-fmt-lpcm:
> +
> +*
> +V4L2_AUDIO_FMT_LPCM ('LPCM')
> +*
> +
> +Linear Pulse-Code Modulation (LPCM)
> +
> +
> +Description
> +===
> +
> +This describes audio format used by the audio memory to memory driver.
> +
> +It contains the following fields:
> +
> +.. flat-table::
> +:widths: 1 4
> +:header-rows: 1
> +:stub-columns: 0
> +
> +* - Field
> + - Description
> +* - u32 samplerate;
> + - which is the number of times per second that samples are taken.
> +* - u32 sampleformat;
> + - which determines the number of possible digital values that can be
> used to represent each sample
80 characters (or less) per line, please.
Which values could this field have and what do they signify?
> +* - u32 channels;
> + - channel number for each sample.
I suppose the rest of the buffer would be samples? This should be
documented. I think there are also different ways the data could be
arrangeed and this needs to be documented, too.
--
Kind regards,
Sakari Ailus
Re: [PATCH 1/6] media: v4l2: Add audio capture and output support
ioc_g_fmt_audio_out)(struct file *file, void *fh,
> + struct v4l2_format *f);
>
> /* VIDIOC_S_FMT handlers */
> int (*vidioc_s_fmt_vid_cap)(struct file *file, void *fh,
> @@ -375,6 +401,10 @@ struct v4l2_ioctl_ops {
>struct v4l2_format *f);
> int (*vidioc_s_fmt_meta_out)(struct file *file, void *fh,
>struct v4l2_format *f);
> + int (*vidioc_s_fmt_audio_cap)(struct file *file, void *fh,
> + struct v4l2_format *f);
> + int (*vidioc_s_fmt_audio_out)(struct file *file, void *fh,
> + struct v4l2_format *f);
>
> /* VIDIOC_TRY_FMT handlers */
> int (*vidioc_try_fmt_vid_cap)(struct file *file, void *fh,
> @@ -405,6 +435,10 @@ struct v4l2_ioctl_ops {
> struct v4l2_format *f);
> int (*vidioc_try_fmt_meta_out)(struct file *file, void *fh,
> struct v4l2_format *f);
> + int (*vidioc_try_fmt_audio_cap)(struct file *file, void *fh,
> + struct v4l2_format *f);
> + int (*vidioc_try_fmt_audio_out)(struct file *file, void *fh,
> + struct v4l2_format *f);
>
> /* Buffer handlers */
> int (*vidioc_reqbufs)(struct file *file, void *fh,
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index aee75eb9e686..a7af28f4c8c3 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -153,6 +153,8 @@ enum v4l2_buf_type {
> V4L2_BUF_TYPE_SDR_OUTPUT = 12,
> V4L2_BUF_TYPE_META_CAPTURE = 13,
> V4L2_BUF_TYPE_META_OUTPUT = 14,
> + V4L2_BUF_TYPE_AUDIO_CAPTURE= 15,
> + V4L2_BUF_TYPE_AUDIO_OUTPUT = 16,
> /* Deprecated, do not use */
> V4L2_BUF_TYPE_PRIVATE = 0x80,
> };
> @@ -169,6 +171,7 @@ enum v4l2_buf_type {
>|| (type) == V4L2_BUF_TYPE_VBI_OUTPUT \
>|| (type) == V4L2_BUF_TYPE_SLICED_VBI_OUTPUT \
>|| (type) == V4L2_BUF_TYPE_SDR_OUTPUT \
> + || (type) == V4L2_BUF_TYPE_AUDIO_OUTPUT\
>|| (type) == V4L2_BUF_TYPE_META_OUTPUT)
>
> #define V4L2_TYPE_IS_CAPTURE(type) (!V4L2_TYPE_IS_OUTPUT(type))
> @@ -2404,6 +2407,20 @@ struct v4l2_meta_format {
> __u32 buffersize;
> } __attribute__ ((packed));
>
> +/**
> + * struct v4l2_audio_format - audio data format definition
> + * @rate:sample rate
> + * @format: sample format
> + * @channels:channel numbers
> + * @buffersize: maximum size in bytes required for data
> + */
> +struct v4l2_audio_format {
> + __u32 rate;
> + __u32 format;
> + __u32 channels;
> + __u32 buffersize;
> +} __attribute__ ((packed));
> +
> /**
> * struct v4l2_format - stream data format
> * @type:enum v4l2_buf_type; type of the data stream
> @@ -2412,6 +2429,7 @@ struct v4l2_meta_format {
> * @win: definition of an overlaid image
> * @vbi: raw VBI capture or output parameters
> * @sliced: sliced VBI capture or output parameters
> + * @audio: definition of an audio format
> * @raw_data:placeholder for future extensions and custom formats
> * @fmt: union of @pix, @pix_mp, @win, @vbi, @sliced, @sdr, @meta
> * and @raw_data
> @@ -2426,6 +2444,7 @@ struct v4l2_format {
> struct v4l2_sliced_vbi_format sliced; /*
> V4L2_BUF_TYPE_SLICED_VBI_CAPTURE */
> struct v4l2_sdr_format sdr; /*
> V4L2_BUF_TYPE_SDR_CAPTURE */
> struct v4l2_meta_format meta;/*
> V4L2_BUF_TYPE_META_CAPTURE */
> + struct v4l2_audio_formataudio; /*
> V4L2_BUF_TYPE_AUDIO_CAPTURE */
> __u8raw_data[200]; /* user-defined */
> } fmt;
> };
> --
> 2.34.1
>
--
Sakari Ailus
Re: [PATCH v5 5/6] mm/gup: remove vmas parameter from pin_user_pages()
On Sun, May 14, 2023 at 10:26:58PM +0100, Lorenzo Stoakes wrote: > We are now in a position where no caller of pin_user_pages() requires the > vmas parameter at all, so eliminate this parameter from the function and > all callers. > > This clears the way to removing the vmas parameter from GUP altogether. > > Acked-by: David Hildenbrand > Acked-by: Dennis Dalessandro (for > qib) > Signed-off-by: Lorenzo Stoakes Acked-by: Sakari Ailus # drivers/media -- Sakari Ailus
Re: [PATCH v2 1/2] device property: Introduce fwnode_device_is_compatible() helper
Hi Andy,
On Wed, Oct 05, 2022 at 06:29:46PM +0300, Andy Shevchenko wrote:
> The fwnode_device_is_compatible() helper searches for the
> given string in the "compatible" string array property and,
> if found, returns true.
>
> Signed-off-by: Andy Shevchenko
> ---
> include/linux/property.h | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/property.h b/include/linux/property.h
> index 1c26d263d5e4..701570423943 100644
> --- a/include/linux/property.h
> +++ b/include/linux/property.h
> @@ -55,7 +55,6 @@ int device_property_read_string(struct device *dev, const
> char *propname,
> int device_property_match_string(struct device *dev,
>const char *propname, const char *string);
>
> -bool fwnode_device_is_available(const struct fwnode_handle *fwnode);
> bool fwnode_property_present(const struct fwnode_handle *fwnode,
>const char *propname);
> int fwnode_property_read_u8_array(const struct fwnode_handle *fwnode,
> @@ -77,6 +76,15 @@ int fwnode_property_read_string(const struct fwnode_handle
> *fwnode,
> const char *propname, const char **val);
> int fwnode_property_match_string(const struct fwnode_handle *fwnode,
>const char *propname, const char *string);
> +
> +bool fwnode_device_is_available(const struct fwnode_handle *fwnode);
> +
> +static inline
> +bool fwnode_device_is_compatible(const struct fwnode_handle *fwnode, const
> char *compat)
> +{
> + return fwnode_property_match_string(fwnode, "compatible", compat) >= 0;
fwnode_property_match_string() returns zero on success, therefore >= 0 is
not needed. I'd just use !fwnode_property_match_string(...).
For both patches:
Reviewed-by: Sakari Ailus
> +}
> +
> int fwnode_property_get_reference_args(const struct fwnode_handle *fwnode,
> const char *prop, const char *nargs_prop,
> unsigned int nargs, unsigned int index,
--
Regards,
Sakari Ailus
Re: [PATCH 04/29] staging: media: ipu3: use vmap instead of reimplementing it
On Tue, Apr 14, 2020 at 03:13:23PM +0200, Christoph Hellwig wrote: > Just use vmap instead of messing with vmalloc internals. > > Signed-off-by: Christoph Hellwig > Acked-by: Peter Zijlstra (Intel) Thanks! Acked-by: Sakari Ailus -- Sakari Ailus
