Re: [PATCH v12 07/15] media: v4l2: Add audio capture and output support
On Thu, Jan 18, 2024 at 10:15 PM Shengjiu Wang wrote: > > Audio signal processing has the requirement for memory to > memory similar as Video. > > This patch is to add this support in v4l2 framework, defined > new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and > V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format > for audio case usage. > > The created audio device is named "/dev/v4l-audioX". > > Signed-off-by: Shengjiu Wang > --- > .../userspace-api/media/v4l/buffer.rst| 6 ++ > .../media/v4l/dev-audio-mem2mem.rst | 71 +++ > .../userspace-api/media/v4l/devices.rst | 1 + > .../media/v4l/vidioc-enum-fmt.rst | 2 + > .../userspace-api/media/v4l/vidioc-g-fmt.rst | 4 ++ > .../media/videodev2.h.rst.exceptions | 2 + > .../media/common/videobuf2/videobuf2-v4l2.c | 4 ++ > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 +++ > drivers/media/v4l2-core/v4l2-dev.c| 17 + > drivers/media/v4l2-core/v4l2-ioctl.c | 53 ++ > include/media/v4l2-dev.h | 2 + > include/media/v4l2-ioctl.h| 34 + > include/uapi/linux/videodev2.h| 17 + > 13 files changed, 222 insertions(+) > create mode 100644 > Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst For drivers/media/common/videobuf2: Acked-by: Tomasz Figa Best regards, Tomasz
Re: [PATCH v12 07/15] media: v4l2: Add audio capture and output support
On Sat, Feb 17, 2024 at 6:42 PM Mauro Carvalho Chehab
wrote:
>
> Em Thu, 18 Jan 2024 20:32:00 +0800
> Shengjiu Wang escreveu:
>
> > Audio signal processing has the requirement for memory to
> > memory similar as Video.
> >
> > This patch is to add this support in v4l2 framework, defined
> > new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and
> > V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format
> > for audio case usage.
> >
> > The created audio device is named "/dev/v4l-audioX".
> >
> > Signed-off-by: Shengjiu Wang
> > ---
> > .../userspace-api/media/v4l/buffer.rst| 6 ++
> > .../media/v4l/dev-audio-mem2mem.rst | 71 +++
> > .../userspace-api/media/v4l/devices.rst | 1 +
> > .../media/v4l/vidioc-enum-fmt.rst | 2 +
> > .../userspace-api/media/v4l/vidioc-g-fmt.rst | 4 ++
> > .../media/videodev2.h.rst.exceptions | 2 +
> > .../media/common/videobuf2/videobuf2-v4l2.c | 4 ++
> > drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 9 +++
> > drivers/media/v4l2-core/v4l2-dev.c| 17 +
> > drivers/media/v4l2-core/v4l2-ioctl.c | 53 ++
> > include/media/v4l2-dev.h | 2 +
> > include/media/v4l2-ioctl.h| 34 +
> > include/uapi/linux/videodev2.h| 17 +
> > 13 files changed, 222 insertions(+)
> > create mode 100644
> > Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst
> >
> > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst
> > b/Documentation/userspace-api/media/v4l/buffer.rst
> > index 52bbee81c080..a3754ca6f0d6 100644
> > --- a/Documentation/userspace-api/media/v4l/buffer.rst
> > +++ b/Documentation/userspace-api/media/v4l/buffer.rst
> > @@ -438,6 +438,12 @@ enum v4l2_buf_type
> > * - ``V4L2_BUF_TYPE_META_OUTPUT``
> >- 14
>
> >- Buffer for metadata output, see :ref:`metadata`.
> > +* - ``V4L2_BUF_TYPE_AUDIO_CAPTURE``
> > + - 15
> > + - Buffer for audio capture, see :ref:`audio`.
> > +* - ``V4L2_BUF_TYPE_AUDIO_OUTPUT``
> > + - 16
>
> Hmm... alsa APi define input/output as:
> enum {
> SNDRV_PCM_STREAM_PLAYBACK = 0,
> SNDRV_PCM_STREAM_CAPTURE,
> SNDRV_PCM_STREAM_LAST = SNDRV_PCM_STREAM_CAPTURE,
> };
>
>
> I would use a namespace as close as possible to the
> ALSA API. Also, we're not talking about V4L2, but, instead
> audio. so, not sure if I like the prefix to start with
> V4L2_. Maybe ALSA_?
>
> So, a better namespace would be:
>
> ${prefix}_BUF_TYPE_PCM_STREAM_PLAYBACK
> and
> ${prefix}_BUF_TYPE_PCM_STREAM_CAPTURE
>
The API is still V4L2, and all the other non-video buf types also use
the V4L2_ prefix, so perhaps that's good here as well?
Whether AUDIO or PCM_STREAM makes more sense goes outside of my
expertise. Subjectively, a PCM stream sounds more specific than an
audio stream. Do those buf types also support non-PCM audio streams?
> > + - Buffer for audio output, see :ref:`audio`.
> >
> >
> > .. _buffer-flags:
> > diff --git a/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst
> > b/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst
> > new file mode 100644
> > index ..68faecfe3a02
> > --- /dev/null
> > +++ b/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst
> > @@ -0,0 +1,71 @@
> > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> > +
> > +.. _audiomem2mem:
> > +
> > +
> > +Audio Memory-To-Memory Interface
> > +
> > +
> > +An audio memory-to-memory device can compress, decompress, transform, or
> > +otherwise convert audio data from one format into another format, in
> > memory.
> > +Such memory-to-memory devices set the ``V4L2_CAP_AUDIO_M2M`` capability.
> > +Examples of memory-to-memory devices are audio codecs, audio preprocessing,
> > +audio postprocessing.
> > +
> > +A memory-to-memory audio node supports both output (sending audio frames
> > from
> > +memory to the hardware) and capture (receiving the processed audio frames
> > +from the hardware into memory) stream I/O. An application will have to
> > +setup the stream I/O for both sides and finally call
> > +:ref:`VIDIOC_STREAMON ` for both capture and output to
> > +start the hardware.
> > +
> > +Memory-to-memory devices function as a shared resource: you can
> > +open the audio node multiple times, each application setting up their
> > +own properties that are local to the file handle, and each can use
> > +it independently from the others. The driver will arbitrate access to
> > +the hardware and reprogram it whenever another file handler gets access.
> > +
> > +Audio memory-to-memory devices are accessed through character device
> > +special files named ``/dev/v4l-audio``
> > +
> > +Querying Capabilities
> > +=
> > +
> > +Device nodes supporting the audio
Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT
On Wed, Nov 15, 2023 at 8:49 PM Laurent Pinchart wrote: > > Hi Hans, > > On Wed, Nov 15, 2023 at 12:19:31PM +0100, Hans Verkuil wrote: > > On 11/15/23 11:55, Laurent Pinchart wrote: > > > On Wed, Nov 15, 2023 at 09:09:42AM +0100, Hans Verkuil wrote: > > >> On 13/11/2023 13:44, Laurent Pinchart wrote: > > >>> On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote: > > On 13/11/2023 12:43, Laurent Pinchart wrote: > > > On Mon, Nov 13, 2023 at 11:28:51AM +, Sakari Ailus wrote: > > >> 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: > > >>> 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 > > > > > > Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-) > > > > > >>> is always interpreted as a 64 bit integer and nothing else. As it > > >>> should. > > > > > > The most common case for control handling in drivers is taking the > > > integer value and converting it to a register value, using > > > device-specific encoding of the register value. It can be a > > > fixed-point > > > format or something else, depending on the device. My point is that > > > drivers routinely convert a "plain" integer to something else, and > > > that > > > has never been considered as a cause of concern. I don't see why it > > > would be different in this series. > > > > > >>> 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. > > > > > > It's hidden so well that libcamera has a database of the sensor it > > > supports, with formulas to map a real gain value to the >
Re: [PATCH v9 10/15] media: uapi: Add V4L2_CTRL_TYPE_FIXED_POINT
On Wed, Nov 15, 2023 at 5:09 PM Hans Verkuil wrote: > > Hi Laurent, > > On 13/11/2023 13:44, Laurent Pinchart wrote: > > Hi Hans, > > > > On Mon, Nov 13, 2023 at 01:05:12PM +0100, Hans Verkuil wrote: > >> On 13/11/2023 12:43, Laurent Pinchart wrote: > >>> On Mon, Nov 13, 2023 at 11:28:51AM +, Sakari Ailus wrote: > 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: > > 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 > >>> > >>> Yes, I meant V4L2_CTRL_TYPE_INTEGER64. Too hasty copy & paste :-) > >>> > > is always interpreted as a 64 bit integer and nothing else. As it > > should. > >>> > >>> The most common case for control handling in drivers is taking the > >>> integer value and converting it to a register value, using > >>> device-specific encoding of the register value. It can be a fixed-point > >>> format or something else, depending on the device. My point is that > >>> drivers routinely convert a "plain" integer to something else, and that > >>> has never been considered as a cause of concern. I don't see why it > >>> would be different in this series. > >>> > > 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. > >>> > >>> It's hidden so well that libcamera has a database of the sensor it > >>> supports, with formulas to map a real gain value to the > >>> V4L2_CID_ANALOGUE_GAIN control. The encoding of the integer value does > >>> matter, and the kernel doesn't expose it. We may or may not consider > >>> that as a shortcoming of the V4L2 control API, but in any case it's the > >>> situation we have today. > >>> > I wonder if Laurent meant digital gain. > >>> > >>> No, I meant analog. It applies to digital gain too though. > >>> > 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
Re: [RFC PATCH v2 4/7] media: v4l2: Add audio capture and output support
On Tue, Aug 1, 2023 at 6:47 PM Shengjiu Wang wrote:
>
> On Fri, Jul 28, 2023 at 3:59 PM Tomasz Figa wrote:
> >
> > Hi Shengjiu,
> >
> > On Tue, Jul 25, 2023 at 02:12:17PM +0800, Shengjiu Wang wrote:
> > > Audio signal processing has the requirement for memory to
> > > memory similar as Video.
> > >
> > > This patch is to add this support in v4l2 framework, defined
> > > new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and
> > > V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format
> > > for audio case usage.
> > >
> > > The created audio device is named "/dev/audioX".
> > >
> > > Signed-off-by: Shengjiu Wang
> > > ---
> > > .../media/common/videobuf2/videobuf2-v4l2.c | 4 ++
> > > drivers/media/v4l2-core/v4l2-dev.c| 17 ++
> > > drivers/media/v4l2-core/v4l2-ioctl.c | 52 +++
> > > include/media/v4l2-dev.h | 2 +
> > > include/media/v4l2-ioctl.h| 34
> > > include/uapi/linux/videodev2.h| 19 +++
> > > 6 files changed, 128 insertions(+)
> > >
> >
> > Thanks for the patch! Please check my comments inline.
>
> Thanks for reviewing.
>
> Sorry for sending again for using the plain text mode.
>
> >
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > index c7a54d82a55e..12f2be2773a2 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > > @@ -785,6 +785,10 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> > > v4l2_create_buffers *create)
> > > case V4L2_BUF_TYPE_META_OUTPUT:
> > > requested_sizes[0] = f->fmt.meta.buffersize;
> > > break;
> > > + case V4L2_BUF_TYPE_AUDIO_CAPTURE:
> > > + case V4L2_BUF_TYPE_AUDIO_OUTPUT:
> > > + requested_sizes[0] = f->fmt.audio.buffersize;
> > > + break;
> > > default:
> > > return -EINVAL;
> > > }
> > > diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> > > b/drivers/media/v4l2-core/v4l2-dev.c
> > > index f81279492682..67484f4c6eaf 100644
> > > --- a/drivers/media/v4l2-core/v4l2-dev.c
> > > +++ b/drivers/media/v4l2-core/v4l2-dev.c
> > > @@ -553,6 +553,7 @@ static void determine_valid_ioctls(struct
> > > video_device *vdev)
> > > bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
> > > bool is_meta = vdev->vfl_type == VFL_TYPE_VIDEO &&
> > > (vdev->device_caps & meta_caps);
> > > + bool is_audio = vdev->vfl_type == VFL_TYPE_AUDIO;
> > > bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
> > > bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> > > bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
> > > @@ -664,6 +665,19 @@ static void determine_valid_ioctls(struct
> > > video_device *vdev)
> > > SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_meta_out);
> > > SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT,
> > > vidioc_try_fmt_meta_out);
> > > }
> > > + if (is_audio && is_rx) {
> > > + /* audio capture specific ioctls */
> > > + SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT,
> > > vidioc_enum_fmt_audio_cap);
> > > + SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_audio_cap);
> > > + SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_audio_cap);
> > > + SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT,
> > > vidioc_try_fmt_audio_cap);
> > > + } else if (is_audio && is_tx) {
> > > + /* audio output specific ioctls */
> > > + SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT,
> > > vidioc_enum_fmt_audio_out);
> > > + SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_audio_out);
> > > + SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_audio_out);
> > > + SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT,
> > > vidioc_try_fmt_audio_out);
> > > + }
> > > if (is_vbi) {
> > > /* vbi specific ioctls */
> > > if ((is_rx && (ops->vidioc_g_fmt_vbi_cap ||
> > > @@ -927,6 +941,9 @@ int __video_register_device(struct vid
Re: [RFC PATCH v2 4/7] media: v4l2: Add audio capture and output support
Hi Shengjiu,
On Tue, Jul 25, 2023 at 02:12:17PM +0800, Shengjiu Wang wrote:
> Audio signal processing has the requirement for memory to
> memory similar as Video.
>
> This patch is to add this support in v4l2 framework, defined
> new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and
> V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format
> for audio case usage.
>
> The created audio device is named "/dev/audioX".
>
> Signed-off-by: Shengjiu Wang
> ---
> .../media/common/videobuf2/videobuf2-v4l2.c | 4 ++
> drivers/media/v4l2-core/v4l2-dev.c| 17 ++
> drivers/media/v4l2-core/v4l2-ioctl.c | 52 +++
> include/media/v4l2-dev.h | 2 +
> include/media/v4l2-ioctl.h| 34
> include/uapi/linux/videodev2.h| 19 +++
> 6 files changed, 128 insertions(+)
>
Thanks for the patch! Please check my comments inline.
> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> index c7a54d82a55e..12f2be2773a2 100644
> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> @@ -785,6 +785,10 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create)
> case V4L2_BUF_TYPE_META_OUTPUT:
> requested_sizes[0] = f->fmt.meta.buffersize;
> break;
> + case V4L2_BUF_TYPE_AUDIO_CAPTURE:
> + case V4L2_BUF_TYPE_AUDIO_OUTPUT:
> + requested_sizes[0] = f->fmt.audio.buffersize;
> + break;
> default:
> return -EINVAL;
> }
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c
> b/drivers/media/v4l2-core/v4l2-dev.c
> index f81279492682..67484f4c6eaf 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -553,6 +553,7 @@ static void determine_valid_ioctls(struct video_device
> *vdev)
> bool is_tch = vdev->vfl_type == VFL_TYPE_TOUCH;
> bool is_meta = vdev->vfl_type == VFL_TYPE_VIDEO &&
> (vdev->device_caps & meta_caps);
> + bool is_audio = vdev->vfl_type == VFL_TYPE_AUDIO;
> bool is_rx = vdev->vfl_dir != VFL_DIR_TX;
> bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
> @@ -664,6 +665,19 @@ static void determine_valid_ioctls(struct video_device
> *vdev)
> SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_meta_out);
> SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_meta_out);
> }
> + if (is_audio && is_rx) {
> + /* audio capture specific ioctls */
> + SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT,
> vidioc_enum_fmt_audio_cap);
> + SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_audio_cap);
> + SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_audio_cap);
> + SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_audio_cap);
> + } else if (is_audio && is_tx) {
> + /* audio output specific ioctls */
> + SET_VALID_IOCTL(ops, VIDIOC_ENUM_FMT,
> vidioc_enum_fmt_audio_out);
> + SET_VALID_IOCTL(ops, VIDIOC_G_FMT, vidioc_g_fmt_audio_out);
> + SET_VALID_IOCTL(ops, VIDIOC_S_FMT, vidioc_s_fmt_audio_out);
> + SET_VALID_IOCTL(ops, VIDIOC_TRY_FMT, vidioc_try_fmt_audio_out);
> + }
> if (is_vbi) {
> /* vbi specific ioctls */
> if ((is_rx && (ops->vidioc_g_fmt_vbi_cap ||
> @@ -927,6 +941,9 @@ int __video_register_device(struct video_device *vdev,
> case VFL_TYPE_TOUCH:
> name_base = "v4l-touch";
> break;
> + case VFL_TYPE_AUDIO:
> + name_base = "audio";
I think it was mentioned before that "audio" could be confusing. Wasn't
there actually some other kind of /dev/audio device long ago?
Seems like for touch, "v4l-touch" was introduced. Maybe it would also
make sense to call it "v4l-audio" for audio?
> + break;
> default:
> pr_err("%s called with unknown type: %d\n",
> __func__, type);
> diff --git a/drivers/media/v4l2-core/v4l2-ioctl.c
> b/drivers/media/v4l2-core/v4l2-ioctl.c
> index 01ba27f2ef87..aa9d872bba8d 100644
> --- a/drivers/media/v4l2-core/v4l2-ioctl.c
> +++ b/drivers/media/v4l2-core/v4l2-ioctl.c
> @@ -188,6 +188,8 @@ const char *v4l2_type_names[] = {
> [V4L2_BUF_TYPE_SDR_OUTPUT] = "sdr-out",
> [V4L2_BUF_TYPE_META_CAPTURE] = "meta-cap",
> [V4L2_BUF_TYPE_META_OUTPUT]= "meta-out",
> + [V4L2_BUF_TYPE_AUDIO_CAPTURE] = "audio-cap",
> + [V4L2_BUF_TYPE_AUDIO_OUTPUT] = "audio-out",
> };
> EXPORT_SYMBOL(v4l2_type_names);
>
> @@ -276,6 +278,7 @@ static void v4l_print_format(const void *arg, bool
> write_only)
> const struct v4l2_sliced_vbi_format *sliced;
> const struct v4l2_window *win;
> const struct
Re: [PATCH mm-unstable v1 16/20] mm/frame-vector: remove FOLL_FORCE usage
On Mon, Nov 28, 2022 at 5:19 PM David Hildenbrand wrote:
>
> On 28.11.22 09:17, Hans Verkuil wrote:
> > Hi David,
> >
> > On 27/11/2022 11:35, David Hildenbrand wrote:
> >> On 16.11.22 11:26, David Hildenbrand wrote:
> >>> FOLL_FORCE is really only for ptrace access. According to commit
> >>> 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are always
> >>> writable"), get_vaddr_frames() currently pins all pages writable as a
> >>> workaround for issues with read-only buffers.
> >>>
> >>> FOLL_FORCE, however, seems to be a legacy leftover as it predates
> >>> commit 707947247e95 ("media: videobuf2-vmalloc: get_userptr: buffers are
> >>> always writable"). Let's just remove it.
> >>>
> >>> Once the read-only buffer issue has been resolved, FOLL_WRITE could
> >>> again be set depending on the DMA direction.
> >>>
> >>> Cc: Hans Verkuil
> >>> Cc: Marek Szyprowski
> >>> Cc: Tomasz Figa
> >>> Cc: Marek Szyprowski
> >>> Cc: Mauro Carvalho Chehab
> >>> Signed-off-by: David Hildenbrand
> >>> ---
> >>>drivers/media/common/videobuf2/frame_vector.c | 2 +-
> >>>1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/media/common/videobuf2/frame_vector.c
> >>> b/drivers/media/common/videobuf2/frame_vector.c
> >>> index 542dde9d2609..062e98148c53 100644
> >>> --- a/drivers/media/common/videobuf2/frame_vector.c
> >>> +++ b/drivers/media/common/videobuf2/frame_vector.c
> >>> @@ -50,7 +50,7 @@ int get_vaddr_frames(unsigned long start, unsigned int
> >>> nr_frames,
> >>>start = untagged_addr(start);
> >>> ret = pin_user_pages_fast(start, nr_frames,
> >>> - FOLL_FORCE | FOLL_WRITE | FOLL_LONGTERM,
> >>> + FOLL_WRITE | FOLL_LONGTERM,
> >>> (struct page **)(vec->ptrs));
> >>>if (ret > 0) {
> >>>vec->got_ref = true;
> >>
> >>
> >> Hi Andrew,
> >>
> >> see the discussion at [1] regarding a conflict and how to proceed with
> >> upstreaming. The conflict would be easy to resolve, however, also
> >> the patch description doesn't make sense anymore with [1].
> >
> > Might it be easier and less confusing if you post a v2 of this series
> > with my patch first? That way it is clear that 1) my patch has to come
> > first, and 2) that it is part of a single series and should be merged
> > by the mm subsystem.
> >
> > Less chances of things going wrong that way.
> >
> > Just mention in the v2 cover letter that the first patch was added to
> > make it easy to backport that fix without being hampered by merge
> > conflicts if it was added after your frame_vector.c patch.
>
> Yes, that's the way I would naturally do, it, however, Andrew prefers
> delta updates for minor changes.
>
> @Andrew, whatever you prefer!
>
> Thanks!
>
However you folks proceed with taking this patch, feel free to add my
Acked-by. Thanks!
Best regards,
Tomasz
> --
> Thanks,
>
> David / dhildenb
>
Re: [RFC PATCH v3 0/6] Restricted DMA
On Wed, Jan 13, 2021 at 12:56 PM Florian Fainelli wrote: > > > > On 1/12/2021 6:29 PM, Tomasz Figa wrote: > > Hi Florian, > > > > On Wed, Jan 13, 2021 at 3:01 AM Florian Fainelli > > wrote: > >> > >> On 1/11/21 11:48 PM, Claire Chang wrote: > >>> On Fri, Jan 8, 2021 at 1:59 AM Florian Fainelli > >>> wrote: > >>>> > >>>> On 1/7/21 9:42 AM, Claire Chang wrote: > >>>> > >>>>>> Can you explain how ATF gets involved and to what extent it does help, > >>>>>> besides enforcing a secure region from the ARM CPU's perpsective? Does > >>>>>> the PCIe root complex not have an IOMMU but can somehow be denied > >>>>>> access > >>>>>> to a region that is marked NS=0 in the ARM CPU's MMU? If so, that is > >>>>>> still some sort of basic protection that the HW enforces, right? > >>>>> > >>>>> We need the ATF support for memory MPU (memory protection unit). > >>>>> Restricted DMA (with reserved-memory in dts) makes sure the predefined > >>>>> memory > >>>>> region is for PCIe DMA only, but we still need MPU to locks down PCIe > >>>>> access to > >>>>> that specific regions. > >>>> > >>>> OK so you do have a protection unit of some sort to enforce which region > >>>> in DRAM the PCIE bridge is allowed to access, that makes sense, > >>>> otherwise the restricted DMA region would only be a hint but nothing you > >>>> can really enforce. This is almost entirely analogous to our systems > >>>> then. > >>> > >>> Here is the example of setting the MPU: > >>> https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 > >>> > >>>> > >>>> There may be some value in standardizing on an ARM SMCCC call then since > >>>> you already support two different SoC vendors. > >>>> > >>>>> > >>>>>> > >>>>>> On Broadcom STB SoCs we have had something similar for a while however > >>>>>> and while we don't have an IOMMU for the PCIe bridge, we do have a a > >>>>>> basic protection mechanism whereby we can configure a region in DRAM to > >>>>>> be PCIe read/write and CPU read/write which then gets used as the PCIe > >>>>>> inbound region for the PCIe EP. By default the PCIe bridge is not > >>>>>> allowed access to DRAM so we must call into a security agent to allow > >>>>>> the PCIe bridge to access the designated DRAM region. > >>>>>> > >>>>>> We have done this using a private CMA area region assigned via Device > >>>>>> Tree, assigned with a and requiring the PCIe EP driver to use > >>>>>> dma_alloc_from_contiguous() in order to allocate from this device > >>>>>> private CMA area. The only drawback with that approach is that it > >>>>>> requires knowing how much memory you need up front for buffers and DMA > >>>>>> descriptors that the PCIe EP will need to process. The problem is that > >>>>>> it requires driver modifications and that does not scale over the > >>>>>> number > >>>>>> of PCIe EP drivers, some we absolutely do not control, but there is no > >>>>>> need to bounce buffer. Your approach scales better across PCIe EP > >>>>>> drivers however it does require bounce buffering which could be a > >>>>>> performance hit. > >>>>> > >>>>> Only the streaming DMA (map/unmap) needs bounce buffering. > >>>> > >>>> True, and typically only on transmit since you don't really control > >>>> where the sk_buff are allocated from, right? On RX since you need to > >>>> hand buffer addresses to the WLAN chip prior to DMA, you can allocate > >>>> them from a pool that already falls within the restricted DMA region, > >>>> right? > >>>> > >>> > >>> Right, but applying bounce buffering to RX will make it more secure. > >>> The device won't be able to modify the content after unmap. Just like what > >>> iommu_unmap does. > >> > >> Sure, however the goals of using bounce buffe
Re: [RFC PATCH v3 0/6] Restricted DMA
Hi Florian, On Wed, Jan 13, 2021 at 3:01 AM Florian Fainelli wrote: > > On 1/11/21 11:48 PM, Claire Chang wrote: > > On Fri, Jan 8, 2021 at 1:59 AM Florian Fainelli > > wrote: > >> > >> On 1/7/21 9:42 AM, Claire Chang wrote: > >> > Can you explain how ATF gets involved and to what extent it does help, > besides enforcing a secure region from the ARM CPU's perpsective? Does > the PCIe root complex not have an IOMMU but can somehow be denied access > to a region that is marked NS=0 in the ARM CPU's MMU? If so, that is > still some sort of basic protection that the HW enforces, right? > >>> > >>> We need the ATF support for memory MPU (memory protection unit). > >>> Restricted DMA (with reserved-memory in dts) makes sure the predefined > >>> memory > >>> region is for PCIe DMA only, but we still need MPU to locks down PCIe > >>> access to > >>> that specific regions. > >> > >> OK so you do have a protection unit of some sort to enforce which region > >> in DRAM the PCIE bridge is allowed to access, that makes sense, > >> otherwise the restricted DMA region would only be a hint but nothing you > >> can really enforce. This is almost entirely analogous to our systems then. > > > > Here is the example of setting the MPU: > > https://github.com/ARM-software/arm-trusted-firmware/blob/master/plat/mediatek/mt8183/drivers/emi_mpu/emi_mpu.c#L132 > > > >> > >> There may be some value in standardizing on an ARM SMCCC call then since > >> you already support two different SoC vendors. > >> > >>> > > On Broadcom STB SoCs we have had something similar for a while however > and while we don't have an IOMMU for the PCIe bridge, we do have a a > basic protection mechanism whereby we can configure a region in DRAM to > be PCIe read/write and CPU read/write which then gets used as the PCIe > inbound region for the PCIe EP. By default the PCIe bridge is not > allowed access to DRAM so we must call into a security agent to allow > the PCIe bridge to access the designated DRAM region. > > We have done this using a private CMA area region assigned via Device > Tree, assigned with a and requiring the PCIe EP driver to use > dma_alloc_from_contiguous() in order to allocate from this device > private CMA area. The only drawback with that approach is that it > requires knowing how much memory you need up front for buffers and DMA > descriptors that the PCIe EP will need to process. The problem is that > it requires driver modifications and that does not scale over the number > of PCIe EP drivers, some we absolutely do not control, but there is no > need to bounce buffer. Your approach scales better across PCIe EP > drivers however it does require bounce buffering which could be a > performance hit. > >>> > >>> Only the streaming DMA (map/unmap) needs bounce buffering. > >> > >> True, and typically only on transmit since you don't really control > >> where the sk_buff are allocated from, right? On RX since you need to > >> hand buffer addresses to the WLAN chip prior to DMA, you can allocate > >> them from a pool that already falls within the restricted DMA region, > >> right? > >> > > > > Right, but applying bounce buffering to RX will make it more secure. > > The device won't be able to modify the content after unmap. Just like what > > iommu_unmap does. > > Sure, however the goals of using bounce buffering equally applies to RX > and TX in that this is the only layer sitting between a stack (block, > networking, USB, etc.) and the underlying device driver that scales well > in order to massage a dma_addr_t to be within a particular physical range. > > There is however room for improvement if the drivers are willing to > change their buffer allocation strategy. When you receive Wi-Fi frames > you need to allocate buffers for the Wi-Fi device to DMA into, and that > happens ahead of the DMA transfers by the Wi-Fi device. At buffer > allocation time you could very well allocate these frames from the > restricted DMA region without having to bounce buffer them since the > host CPU is in control over where and when to DMA into. > That is, however, still a trade-off between saving that one copy and protection from the DMA tampering with the packet contents when the kernel is reading them. Notice how the copy effectively makes a snapshot of the contents, guaranteeing that the kernel has a consistent view of the packet, which is not true if the DMA could modify the buffer contents in the middle of CPU accesses. Best regards, Tomasz > The issue is that each network driver may implement its own buffer > allocation strategy, some may simply call netdev_alloc_skb() which gives > zero control over where the buffer comes from unless you play tricks > with NUMA node allocations and somehow declare that your restricted DMA > region is a different NUMA node. If the driver allocates pages and then > attaches a SKB to
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Thursday 22 of August 2013 15:43:31 Mike Turquette wrote: Quoting Sascha Hauer (2013-08-22 14:00:35) On Thu, Aug 22, 2013 at 01:09:31PM +0100, Mark Rutland wrote: On Thu, Aug 22, 2013 at 08:19:10AM +0100, Mike Turquette wrote: Quoting Tomasz Figa (2013-08-21 14:34:55) On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: Quoting Mark Rutland (2013-08-19 02:35:43) On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt OK, thanks. I guess we need some discussion on dummy clocks vs skipped clocks. I think we want some consistency on this, don't we? If we really need a dummy clock, then we might also want a generic way to specify it. What do we actually mean by a dummy clock? We already have bindings for fixed-clock and co friends describe relatively simple preconfigured clocks. Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core. When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT: * If a clock is wired up and running (as presumably the dummy clock is), then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such? * If no clock is wired up, then we should be able to describe that. If a driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional. Am I missing something? I second that. Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available. I wasn't clear. The dummy clock IS a completely Linux-specific abstraction. I'm not advocating a dummy clock in DT. I am advocating consolidation of the implementation of a clock that does nothing into the clock core. This code could easily live in drivers/clk/clk.c instead of having everyone open-code it. As far as specifying a dummy clock in DT? I dunno. DT should describe real hardware so there isn't much use for a dummy clock. Sorry, I misunderstood. Good to hear we're on the same page :) I'm guessing one of the reasons for such a clock are drivers do not honor the clk.h api and they freak out when clk_get gives them a NULL pointer? I'm not sure. Sascha, could you shed some light on the matter? The original reason introducing the dummy clocks in the i.MX dtbs was to provide devices a clock which the driver requests but is not software controllable. We often have the case where the same devices are on several SoCs, but not on all of them all clocks have a bit to en/disable them. Anyway, to accomplish this we don't need dummy clocks. We can just describe the real clocks. You could use a dummy clk for the Linux implementation, but the downside is that a dummy clock has a rate of 0 always and a your clocks
Re: [PATCH v10 2/2] ASoC: fsl: Add S/PDIF machine driver
On Wednesday 21 of August 2013 12:30:59 Stephen Warren wrote:
On 08/20/2013 09:13 PM, Nicolin Chen wrote:
This patch implements a device-tree-only machine driver for Freescale
i.MX series Soc. It works with spdif_transmitter/spdif_receiver and
fsl_spdif.c drivers.
diff --git
a/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
b/Documentation/devicetree/bindings/sound/imx-audio-spdif.txt
+Optional properties:
+
+ - spdif-transmitter : The phandle of the spdif-transmitter codec
+
+ - spdif-receiver : The phandle of the spdif-receiver codec
+
+* Note: At least one of these two properties should be set in the DT
binding.
I still don't think those two properties are correct.
Exactly what node will those phandles point at?
Imagine following setup:
|| RX | Microphone DSP | Analog mic input
| S/PDIF | || ---
||
| DAI | | Amplifier | ---
|| TX || Speakers output
As you see in the diagram, the S/PDIF interface of the SoC can be
connected to some external devices that can perform sound processing or
simply handle the physical layer.
I'd say that normally both RX and TX lines would be connected to a single
codec chip that has multiple blocks inside, like sound processing,
amplifier, mixer, etc., but nothing stops you from making a crazy setup,
when RX and TX lines are connected to different chips.
There definitely should not be a DT node for any dummy CODEC,
irrespective of whether this binding calls the other node a CODEC or a
dummy CODEC.
I agree. Instead if no chip connected to particular line is specified in
device tree, it's responsibility of Linux sound core to handle this
properly by adding a dummy codec or whatever.
If these properties are to contain phandles, it would be acceptable for
the referenced node to be:
* A node representing the physical connector/jack on the board.
* A node representing some other IP block on the board, such as an HDMI
encoder/display-controller
I think those options are unlikely in general
Why? You usually codec SoC DAIs to some external chips.
Best regards,
Tomasz
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Wednesday 21 of August 2013 09:50:15 Mark Rutland wrote: On Tue, Aug 20, 2013 at 01:06:25AM +0100, Mike Turquette wrote: Quoting Mark Rutland (2013-08-19 02:35:43) On Sat, Aug 17, 2013 at 04:17:18PM +0100, Tomasz Figa wrote: On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt OK, thanks. I guess we need some discussion on dummy clocks vs skipped clocks. I think we want some consistency on this, don't we? If we really need a dummy clock, then we might also want a generic way to specify it. What do we actually mean by a dummy clock? We already have bindings for fixed-clock and co friends describe relatively simple preconfigured clocks. Some platforms have a fake clock which defines noops callbacks and basically doesn't do anything. This is analogous to the dummy regulator implementation. A central one could be registered by the clock core, as is done by the regulator core. When you say some platforms, you presumably mean the platform code in Linux? A dummy clock sounds like a completely Linux-specific abstraction rather than a description of the hardware, and I don't see why we need that in the DT: * If a clock is wired up and running (as presumably the dummy clock is), then surely it's a fixed-clock (it's running, we and we have no control over it, but we presumably know its rate) and can be described as such? * If no clock is wired up, then we should be able to describe that. If a driver believes that a clock is required when it isn't (for some level of functionality), then that driver should be fixed up to support the clock as being optional. Am I missing something? I second that. Moreover, I don't think that device tree should deal with dummy anything. It should be able to describe hardware that is available on given system, not list what hardware is not available. Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 3/4] powerpc: refactor of_get_cpu_node to support other architectures
Hi Sudeep,
This looks good to me overall, but I have one more question inline.
On Friday 16 of August 2013 18:39:50 Sudeep KarkadaNagesha wrote:
From: Sudeep KarkadaNagesha sudeep.karkadanage...@arm.com
Currently different drivers requiring to access cpu device node are
parsing the device tree themselves. Since the ordering in the DT need
not match the logical cpu ordering, the parsing logic needs to consider
that. However, this has resulted in lots of code duplication and in some
cases even incorrect logic.
It's better to consolidate them by adding support for getting cpu
device node for a given logical cpu index in DT core library. However
logical to physical index mapping can be architecture specific.
PowerPC has it's own implementation to get the cpu node for a given
logical index.
This patch refactors the current implementation of of_get_cpu_node.
This in preparation to move the implementation to DT core library.
It separates out the logical to physical mapping so that a default
matching of the physical id to the logical cpu index can be added
when moved to common code. Architecture specific code can override it.
Cc: Rob Herring rob.herr...@calxeda.com
Cc: Grant Likely grant.lik...@linaro.org
Cc: Benjamin Herrenschmidt b...@kernel.crashing.org
Signed-off-by: Sudeep KarkadaNagesha sudeep.karkadanage...@arm.com
---
arch/powerpc/kernel/prom.c | 76
-- 1 file changed, 47
insertions(+), 29 deletions(-)
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index eb23ac9..fb12be6 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -865,45 +865,63 @@ static int __init prom_reconfig_setup(void)
__initcall(prom_reconfig_setup);
#endif
+bool arch_match_cpu_phys_id(int cpu, u64 phys_id)
+{
+ return (int)phys_id == get_hard_smp_processor_id(cpu);
+}
+
+static bool __of_find_n_match_cpu_property(struct device_node *cpun,
+ const char *prop_name, int cpu, unsigned int
*thread)
+{
+ const __be32 *cell;
+ int ac, prop_len, tid;
+ u64 hwid;
+
+ ac = of_n_addr_cells(cpun);
+ cell = of_get_property(cpun, prop_name, prop_len);
+ if (!cell)
+ return false;
I wonder how would this handle uniprocessor ARM (pre-v7) cores, for which
the updated bindings[1] define #address-cells = 0 and so no reg
property.
[1] - http://thread.gmane.org/gmane.linux.ports.arm.kernel/260795
Best regards,
Tomasz
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Friday 16 of August 2013 10:56:32 Sascha Hauer wrote: On Fri, Aug 16, 2013 at 04:01:25PM +0800, Nicolin Chen wrote: Hi Sascha, Thank you for the detailed comments. On Fri, Aug 16, 2013 at 09:08:18AM +0200, Sascha Hauer wrote: Which of them the driver should use is configuration and thus normally should *not* be described in the devicetree. However, there may be no good way for the driver to know which clock to use in which case. There may be additional board requirements which are unknown to the driver. So in this case it might be valid to put the information which clock to use into the devicetree. But be aware that from the moment you put this information into the devicetree the driver is no longer free to chose the best clock, even if in future we find a good way to automatically guess the best clock. Do you have some insights in which case I would use which input clock? Is this only about which clock has the best suitable input frequency or is this also about synchronization of the audio signal with some other unit? I understand. What I'm thinking now is to let the driver find the best clock source for tx clock and a correspond divisor like this: tx0-8 OptionalTx clock source for spdif playback. If absent, will use core clock. The index from 0 to 8 is identical to the clock source list described in TxClk_Source bit of register STC. Multiple clock source are allowed for this tx clock source. The driver will select one source from them for each supported sample rate according to the clock rates of these provided clock sources. You mean tx0-7. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Why do you need a dummy clock? The driver can simply try to grab all the possible clocks and discard those that failed, so you can just keep those grounded clocks unspecified. Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Friday 16 of August 2013 12:11:51 Sascha Hauer wrote: On Fri, Aug 16, 2013 at 05:53:58PM +0800, Nicolin Chen wrote: On Fri, Aug 16, 2013 at 10:56:32AM +0200, Sascha Hauer wrote: tx0-8 OptionalTx clock source for spdif playback. If absent, will use core clock. The index from 0 to 8 is identical to the clock source list described in TxClk_Source bit of register STC. Multiple clock source are allowed for this tx clock source. The driver will select one source from them for each supported sample rate according to the clock rates of these provided clock sources. You mean tx0-7 Yes. Thank you. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? What about cases when you have full description of clocks in device tree, with one node per clock and #clock-cells = 0? Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
Hi Nicolin, On Friday 16 of August 2013 12:43:31 Nicolin Chen wrote: Hi Tomasz, Thank you for the comments. You're welcome. I'll revise them in v6. And below is my reply for you comments. Thanks for your reply. On Thu, Aug 15, 2013 at 02:18:22PM +0200, Tomasz Figa wrote: + - clock-names : Includes the following entries: + nametypecomments + core RequiredThe core clock of spdif controller + + rxOptionalRx clock source for spdif record. + If absent, will use core clock. + + txOptionalTx clock source for spdif playback. + If absent, will use core clock. + + tx-32000 OptionalTx clock source for 32000Hz sample rate + playback. If absent, will use tx clock. + + tx-44100 OptionalTx clock source for 44100Hz sample rate + playback. If absent, will use tx clock. + + tx-48000 OptionalTx clock source for 48000Hz sample rate + playback. If absent, will use tx clock. + + src0-7 OptionalClock source list for tx and rx clock + to look up their clock source indexes. + This clock list should be identical to + the list of TxClk_Source bit value of + register SPDIF_STC. If absent or failed + to look up, tx and rx clock would then + ignore the rx, tx tx-32000, + tx-44100, tx-48000 clock phandles + and select the core clock as default + tx and rx clock. I suspect a little abuse of clocks property here. From the description of core and src0-7 clocks I assume that the IP can have up to 9 clock inputs - core clock and up to 8 extra source clocks. Is it correct? If yes, this makes the tx, rx and tx-* clocks describe configuration, not hardware. IMHO it should be up to the driver which source clocks to use for tx and rx channels and for each sampling rate. First, you are right that all the properties you just commented are software configurations. And I got the point that device tree now can't allow any software configuration even if the actual hardware connection will depend on it. If so, I would like to remove those abused clocks and also drop the unused clocks in src0-7, then just remain those needed clocks src. I think that can be plausible because there'll be no more clock abuse and the driver will be able to get the source index from the name 'srcnum'. OK. And you are right about the 9 clock inputs, just there're not only 9 inputs but also an extra external clock from S/PDIF transmitter via coaxial cable or optical fiber -- RxCLK. Please check the following list: if (DPLL Locked) SPDIF_RxClk else extal 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk 0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk 0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk 0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt 0101 extal_clk 0110 spdif_clk 0111 asrc_clk 1000 spdif_extclk 1001 esai_hckt 1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk 1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk 1100 mkb_clk 1101 mlb_phy_clk Could you explain what the above values are? If they are values written to a 4-bit mux that selects RX clock source, then all the 16 clocks should be specified from device tree, even if they are duplicated. Are the clock names you used above names of clock inputs of the S/PDIF block or names of SoC-wide clocks? Can the assignment of clock inputs change? For example on one SoC if (DPLL Locked) SPDIF_RxClk else extal_clk 0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk ... 0101 extal_clk 0110 spdif_clk ... and on another if (DPLL Locked) SPDIF_RxClk else spdif_clk 0001 if (DPLL Locked) SPDIF_RxClk else extal_clk ... 0101 extal_clk 0110 spdif_clk ... (notice the swapped and 0001 inputs) Same goes for the TX mux. If it is a 3-bit, 8-input mux, then it should be described in device tree separately, as it is different than the 4-bit, 16-input RX mux. When (DPLL Locked) condition matches, the rx clock can ignore the 8 input clocks from clock mux then use the external one from a S/PDIF transmitter. What happens if the DPLL locked condition doesn't match? When this can happen? So for the below part: +Optional properties: + + - rx-clksrc-lock: This is a boolean property. If present, ClkSrc_Sel bit + of SPDIF_SRPC would be set a clock source that cares DPLL locked condition. + This again looks like software configuration
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Saturday 17 of August 2013 17:00:02 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:26:40PM +0200, Tomasz Figa wrote: You mean tx0-7. Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Why do you need a dummy clock? The driver can simply try to grab all the possible clocks and discard those that failed, so you can just keep those grounded clocks unspecified. We don't need dummy clocks. My motivation saying this that I was afraid people try to configure the driver by skipping the clocks they don't want from the devicetree. I'm not really sure if the same abuse couldn't be easily achieved by putting dummy clocks in place of those skipped clocks. Adding a note in binding documentation that says that all clocks that are fed to the IP shall be specified should be fine IMHO. Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Saturday 17 of August 2013 16:53:16 Sascha Hauer wrote: On Sat, Aug 17, 2013 at 02:28:04PM +0200, Tomasz Figa wrote: Also I would make this option required. Use a dummy clock for mux inputs that are grounded for a specific SoC. Some clocks are not from CCM and we haven't defined in imx6q-clk.txt, so in most cases we can't provide a phandle for them, eg: spdif_ext. I think it's a bit hard to force it to be 'required'. An 'optional' looks more flexible to me and a default one is ensured even if it's missing. clks 0 is the dummy clock. This can be used for all input clocks not defined by the SoC. Where does this assumption come from? Is it documented anywhere? This is how all i.MX clock bindings currently are. See Documentation/devicetree/bindings/clock/imx*-clock.txt OK, thanks. I guess we need some discussion on dummy clocks vs skipped clocks. I think we want some consistency on this, don't we? If we really need a dummy clock, then we might also want a generic way to specify it. Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 1/2] ASoC: fsl: Add S/PDIF CPU DAI driver
On Saturday 17 of August 2013 17:14:09 Sascha Hauer wrote:
On Sat, Aug 17, 2013 at 02:56:11PM +0200, Tomasz Figa wrote:
Hi Nicolin,
First, you are right that all the properties you just commented are
software configurations. And I got the point that device tree now
can't allow any software configuration even if the actual hardware
connection will depend on it.
If so, I would like to remove those abused clocks and also drop the
unused clocks in src0-7, then just remain those needed clocks src.
I think that can be plausible because there'll be no more clock
abuse
and the driver will be able to get the source index from the name
'srcnum'.
OK.
And you are right about the 9 clock inputs, just there're not only 9
inputs but also an extra external clock from S/PDIF transmitter via
coaxial cable or optical fiber -- RxCLK. Please check the following
list:
if (DPLL Locked) SPDIF_RxClk else extal
0001 if (DPLL Locked) SPDIF_RxClk else spdif_clk
0010 if (DPLL Locked) SPDIF_RxClk else asrc_clk
0011 if (DPLL Locked) SPDIF_RxClk else spdif_extclk
0100 if (DPLL Locked) SPDIF_Rxclk else esai_hckt
0101 extal_clk
0110 spdif_clk
0111 asrc_clk
1000 spdif_extclk
1001 esai_hckt
1010 if (DPLL Locked) SPDIF_RxClk else mlb_clk
1011 if (DPLL Locked) SPDIF_RxClk else mlb_phy_clk
1100 mkb_clk
1101 mlb_phy_clk
Could you explain what the above values are? If they are values
written to a 4-bit mux that selects RX clock source, then all the 16
clocks should be specified from device tree, even if they are
duplicated.
The S/PDIF core can recover the clock for the tx signal from the rx
signal. So if you have an S/PDIF input signal, then the DPLL will be
locked and the SPDIF_RxClk can be used for tx. So the above are really 8
clocks and one If DPLL locked, use it bit.
Yes, I'm aware of this and the solution you proposed is fully acceptable,
but it complicates the driver a bit.
If you look at the mux inputs above, you can see that there is no single
bit that specifies if DPLL locked, use it mode. Instead the conditional
clock sources are mixed with fixed clock sources, without a linear
translation to 0-7 range, which would have to be hardcoded in the driver.
I guess it's a matter of preference, but if the IP has a RX clock
selection mux that has 16 inputs, the natural representation in device
tree that comes to my mind is 16 clocks - one for each input of the mux.
One might choose to abstract this as 8 clocks, though, since each clock is
duplicated.
In this particular case, the driver would have to know which mux inputs
can use DPLL anyway, so some hardcoding in the driver is still going to
take place, so I guess both solutions are equally right.
Best regards,
Tomasz
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC PATCH v2 3/4] powerpc: refactor of_get_cpu_node to support other architectures
On Sunday 18 of August 2013 08:09:36 Benjamin Herrenschmidt wrote: On Sat, 2013-08-17 at 12:50 +0200, Tomasz Figa wrote: I wonder how would this handle uniprocessor ARM (pre-v7) cores, for which the updated bindings[1] define #address-cells = 0 and so no reg property. [1] - http://thread.gmane.org/gmane.linux.ports.arm.kernel/260795 Why did you do that in the binding ? That sounds like looking to create problems ... [Copying Lorenzo...] I'm not the author of the change. I was just passing by, while the question showed up in my mind. ;) Traditionally, UP setups just used 0 as the reg property on other architectures, why do differently ? Right, especially since the ARM DT topology parsing code still considers a device tree without reg property in cpu node invalid. Best regards, Tomasz ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC/PATCH 32/32] usb: gadget: drop now unnecessary flag
Hi Felipe,
On Thursday 24 of January 2013 17:46:10 Felipe Balbi wrote:
We don't need the -register_my_device flag
anymore because all UDC drivers have been
properly converted.
Let's remove every history of it.
Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/usb/chipidea/udc.c | 1 -
drivers/usb/dwc3/gadget.c | 1 -
drivers/usb/gadget/amd5536udc.c| 1 -
drivers/usb/gadget/at91_udc.c | 1 -
drivers/usb/gadget/atmel_usba_udc.c| 1 -
drivers/usb/gadget/bcm63xx_udc.c | 1 -
drivers/usb/gadget/dummy_hcd.c | 1 -
drivers/usb/gadget/fsl_qe_udc.c| 1 -
drivers/usb/gadget/fsl_udc_core.c | 1 -
drivers/usb/gadget/fusb300_udc.c | 1 -
drivers/usb/gadget/goku_udc.c | 1 -
drivers/usb/gadget/imx_udc.c | 1 -
drivers/usb/gadget/lpc32xx_udc.c | 1 -
drivers/usb/gadget/m66592-udc.c| 1 -
drivers/usb/gadget/mv_u3d_core.c | 1 -
drivers/usb/gadget/mv_udc_core.c | 1 -
drivers/usb/gadget/net2272.c | 1 -
drivers/usb/gadget/net2280.c | 1 -
drivers/usb/gadget/omap_udc.c | 1 -
drivers/usb/gadget/pch_udc.c | 1 -
drivers/usb/gadget/pxa25x_udc.c| 1 -
drivers/usb/gadget/pxa27x_udc.c| 1 -
drivers/usb/gadget/r8a66597-udc.c | 1 -
drivers/usb/gadget/s3c-hsotg.c | 1 -
drivers/usb/gadget/s3c-hsudc.c | 1 -
drivers/usb/gadget/s3c2410_udc.c | 1 -
drivers/usb/gadget/udc-core.c | 3 ---
drivers/usb/musb/musb_gadget.c | 1 -
drivers/usb/renesas_usbhs/mod_gadget.c | 1 -
include/linux/usb/gadget.h | 4
30 files changed, 35 deletions(-)
[snip]
diff --git a/drivers/usb/gadget/udc-core.c
b/drivers/usb/gadget/udc-core.c index 9195054..4ee0efc 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -173,7 +173,6 @@ int usb_add_gadget_udc(struct device *parent, struct
usb_gadget *gadget) if (!udc)
goto err1;
- if (gadget-register_my_device) {
dev_set_name(gadget-dev, gadget);
ret = device_register(gadget-dev);
@@ -211,7 +210,6 @@ err3:
put_device(udc-dev);
err2:
- if (gadget-register_my_device)
put_device(gadget-dev);
err1:
return ret;
@@ -267,7 +265,6 @@ found:
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
device_unregister(udc-dev);
- if (gadget-register_my_device)
device_unregister(gadget-dev);
Correct me if I am wrong, but doesn't this patch leave us with incorrect
indentation?
Otherwise looks good.
Best regards,
Tomasz Figa
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC/PATCH 01/32] usb: gadget: udc-core: allow udc class register gadget device
Hi Felipe,
On Thursday 24 of January 2013 17:45:39 Felipe Balbi wrote:
Currently all UDC drivers are calling
device_register() before calling
usb_add_gadget_udc(). In order to avoid
code duplication, we can allow udc-core.c
register that device.
However that would become a really large patch,
so to cope with the meanwhile and allow us
to write bite-sized patches, we're adding
a flag which will be set by UDC driver once
it removes the code for registering the
gadget device.
Once all are converted, the new flag will
be removed.
Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/usb/gadget/udc-core.c | 23 +++
include/linux/usb/gadget.h| 4
2 files changed, 23 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/gadget/udc-core.c
b/drivers/usb/gadget/udc-core.c index 2a9cd36..9195054 100644
--- a/drivers/usb/gadget/udc-core.c
+++ b/drivers/usb/gadget/udc-core.c
@@ -173,6 +173,14 @@ int usb_add_gadget_udc(struct device *parent,
struct usb_gadget *gadget) if (!udc)
goto err1;
+ if (gadget-register_my_device) {
+ dev_set_name(gadget-dev, gadget);
+
+ ret = device_register(gadget-dev);
+ if (ret)
+ goto err2;
+ }
+
device_initialize(udc-dev);
udc-dev.release = usb_udc_release;
udc-dev.class = udc_class;
@@ -180,7 +188,7 @@ int usb_add_gadget_udc(struct device *parent, struct
usb_gadget *gadget) udc-dev.parent = parent;
ret = dev_set_name(udc-dev, %s, kobject_name(parent-kobj));
if (ret)
- goto err2;
+ goto err3;
Just a nitpick: If you are at changing labels of error path, wouldn't it
be better to give them some meaningful names? Like err_del_unlock,
err_put_udc, err_put_gadget, err_ret.
Otherwise looks good. Nice idea.
Reviewed-by: Tomasz Figa tomasz.f...@gmail.com
Best regards,
Tomasz Figa
udc-gadget = gadget;
@@ -189,18 +197,22 @@ int usb_add_gadget_udc(struct device *parent,
struct usb_gadget *gadget)
ret = device_add(udc-dev);
if (ret)
- goto err3;
+ goto err4;
mutex_unlock(udc_lock);
return 0;
-err3:
+
+err4:
list_del(udc-list);
mutex_unlock(udc_lock);
-err2:
+err3:
put_device(udc-dev);
+err2:
+ if (gadget-register_my_device)
+ put_device(gadget-dev);
err1:
return ret;
}
@@ -254,6 +266,9 @@ found:
kobject_uevent(udc-dev.kobj, KOBJ_REMOVE);
device_unregister(udc-dev);
+
+ if (gadget-register_my_device)
+ device_unregister(gadget-dev);
}
EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 2e297e8..fcd9ef8 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -494,6 +494,9 @@ struct usb_gadget_ops {
* only supports HNP on a different root port.
* @b_hnp_enable: OTG device feature flag, indicating that the A-Host
* enabled HNP support.
+ * @register_my_device: Flag telling udc-core that UDC driver didn't
+ * register the gadget device to the driver model. Temporary until
+ * all UDC drivers are fixed up properly.
* @name: Identifies the controller hardware type. Used in diagnostics
* and sometimes configuration.
* @dev: Driver model state for this abstract device.
@@ -531,6 +534,7 @@ struct usb_gadget {
unsignedb_hnp_enable:1;
unsigneda_hnp_support:1;
unsigneda_alt_hnp_support:1;
+ unsignedregister_my_device:1;
const char *name;
struct device dev;
unsignedout_epnum;
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [RFC/PATCH 24/32] usb: gadget: s3c-hsotg: let udc-core manage gadget-dev
Hi Felipe,
On Thursday 24 of January 2013 17:46:02 Felipe Balbi wrote:
By simply setting a flag, we can drop some
boilerplate code.
Signed-off-by: Felipe Balbi ba...@ti.com
---
drivers/usb/gadget/s3c-hsotg.c | 14 +-
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/usb/gadget/s3c-hsotg.c
b/drivers/usb/gadget/s3c-hsotg.c index 833d85b..bd8292d 100644
--- a/drivers/usb/gadget/s3c-hsotg.c
+++ b/drivers/usb/gadget/s3c-hsotg.c
@@ -3552,17 +3552,13 @@ static int s3c_hsotg_probe(struct
platform_device *pdev)
dev_info(dev, regs %p, irq %d\n, hsotg-regs, hsotg-irq);
- device_initialize(hsotg-gadget.dev);
-
- dev_set_name(hsotg-gadget.dev, gadget);
-
hsotg-gadget.max_speed = USB_SPEED_HIGH;
hsotg-gadget.ops = s3c_hsotg_gadget_ops;
hsotg-gadget.name = dev_name(dev);
-
hsotg-gadget.dev.parent = dev;
hsotg-gadget.dev.dma_mask = dev-dma_mask;
hsotg-gadget.dev.release = s3c_hsotg_release;
+ hsotg-gadget.register_my_device = true;
/* reset the system */
@@ -3643,12 +3639,6 @@ static int s3c_hsotg_probe(struct platform_device
*pdev)
s3c_hsotg_phy_disable(hsotg);
- ret = device_add(hsotg-gadget.dev);
- if (ret) {
- put_device(hsotg-gadget.dev);
- goto err_ep_mem;
- }
-
ret = usb_add_gadget_udc(pdev-dev, hsotg-gadget);
if (ret)
goto err_ep_mem;
@@ -3687,10 +3677,8 @@ static int s3c_hsotg_remove(struct
platform_device *pdev) }
s3c_hsotg_phy_disable(hsotg);
-
clk_disable_unprepare(hsotg-clk);
- device_unregister(hsotg-gadget.dev);
return 0;
}
Looks good.
Reviewed-by: Tomasz Figa tomasz.f...@gmail.com
Best regards,
Tomasz Figa
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
