* Re: [PATCH 4/6] input: serio.h: add SERIO_EXTRON_DA_HD_PLUS
From: Hans Verkuil @ 2024-08-28 11:25 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-media
In-Reply-To: <ccbb099a35cb788c7304795927f508cfc2342ff8.1723190258.git.hverkuil-cisco@xs4all.nl>
Hi Dmitry,
It's a trivial patch, but it would be great if you can give an Acked-by
or Reviewed-by.
Regards,
Hans
On 09/08/2024 09:57, Hans Verkuil wrote:
> Add a new serio ID for the Extron DA HD 4K Plus series of 4K HDMI
> Distribution Amplifiers. These devices support CEC over the serial
> port, so a new serio ID is needed to be able to associate the CEC
> driver.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> include/uapi/linux/serio.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> index ed2a96f43ce4..5a2af0942c9f 100644
> --- a/include/uapi/linux/serio.h
> +++ b/include/uapi/linux/serio.h
> @@ -83,5 +83,6 @@
> #define SERIO_PULSE8_CEC 0x40
> #define SERIO_RAINSHADOW_CEC 0x41
> #define SERIO_FSIA6B 0x42
> +#define SERIO_EXTRON_DA_HD_4K_PLUS 0x43
>
> #endif /* _UAPI_SERIO_H */
^ permalink raw reply
* Re: [PATCH 1/6] media: videodev2.h: add V4L2_CAP_EDID
From: Sebastian Fricke @ 2024-08-28 12:37 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-input, Erling Ljunggren
In-Reply-To: <485324508040defc0ba0fb211a6596dc65f2d994.1723190258.git.hverkuil-cisco@xs4all.nl>
Hello,
On 09.08.2024 09:57, Hans Verkuil wrote:
>From: Erling Ljunggren <hljunggr@cisco.com>
>
>Add capability flag to indicate that the device is an EDID-only device.
>
>Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
>Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Regards,
Sebastian
>---
> include/uapi/linux/videodev2.h | 1 +
> 1 file changed, 1 insertion(+)
>
>diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>index 4e91362da6da..987c821aed79 100644
>--- a/include/uapi/linux/videodev2.h
>+++ b/include/uapi/linux/videodev2.h
>@@ -502,6 +502,7 @@ struct v4l2_capability {
> #define V4L2_CAP_META_CAPTURE 0x00800000 /* Is a metadata capture device */
>
> #define V4L2_CAP_READWRITE 0x01000000 /* read/write systemcalls */
>+#define V4L2_CAP_EDID 0x02000000 /* Is an EDID-only device */
> #define V4L2_CAP_STREAMING 0x04000000 /* streaming I/O ioctls */
> #define V4L2_CAP_META_OUTPUT 0x08000000 /* Is a metadata output device */
>
>--
>2.43.0
>
>
^ permalink raw reply
* Re: [PATCH 2/6] media: v4l2-dev: handle V4L2_CAP_EDID
From: Sebastian Fricke @ 2024-08-28 12:38 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-input, Erling Ljunggren
In-Reply-To: <f88991fc4c197ef0e32f05b2f509980183aef012.1723190258.git.hverkuil-cisco@xs4all.nl>
Hello,
On 09.08.2024 09:57, Hans Verkuil wrote:
>From: Erling Ljunggren <hljunggr@cisco.com>
>
>When the V4L2_CAP_EDID capability flag is set,
>ioctls for enum inputs/outputs and get/set edid are automatically set.
>
>Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
>Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Regards,
Sebastian
>---
> drivers/media/v4l2-core/v4l2-dev.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
>diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
>index be2ba7ca5de2..570ba00e00b3 100644
>--- a/drivers/media/v4l2-core/v4l2-dev.c
>+++ b/drivers/media/v4l2-core/v4l2-dev.c
>@@ -557,6 +557,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
> bool has_streaming = vdev->device_caps & V4L2_CAP_STREAMING;
>+ bool is_edid = vdev->device_caps & V4L2_CAP_EDID;
>
> bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
>
>@@ -784,6 +785,20 @@ static void determine_valid_ioctls(struct video_device *vdev)
> SET_VALID_IOCTL(ops, VIDIOC_S_TUNER, vidioc_s_tuner);
> SET_VALID_IOCTL(ops, VIDIOC_S_HW_FREQ_SEEK, vidioc_s_hw_freq_seek);
> }
>+ if (is_edid) {
>+ SET_VALID_IOCTL(ops, VIDIOC_G_EDID, vidioc_g_edid);
>+ if (is_tx) {
>+ SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
>+ SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
>+ SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
>+ }
>+ if (is_rx) {
>+ SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
>+ SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
>+ SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
>+ SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
>+ }
>+ }
>
> bitmap_andnot(vdev->valid_ioctls, valid_ioctls, vdev->valid_ioctls,
> BASE_VIDIOC_PRIVATE);
>--
>2.43.0
>
>
^ permalink raw reply
* Re: [PATCH 3/6] media: docs: Add V4L2_CAP_EDID
From: Sebastian Fricke @ 2024-08-28 12:38 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-input, Erling Ljunggren
In-Reply-To: <5b880a060574363d5ea351e20e0766abb476f6db.1723190258.git.hverkuil-cisco@xs4all.nl>
Hello,
On 09.08.2024 09:57, Hans Verkuil wrote:
>From: Erling Ljunggren <hljunggr@cisco.com>
>
>Add documentation for the new edid capability.
>
>Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
>Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Regards,
Sebastian
>---
> Documentation/userspace-api/media/v4l/biblio.rst | 11 +++++++++++
> .../userspace-api/media/v4l/vidioc-querycap.rst | 11 +++++++++++
> .../userspace-api/media/videodev2.h.rst.exceptions | 1 +
> 3 files changed, 23 insertions(+)
>
>diff --git a/Documentation/userspace-api/media/v4l/biblio.rst b/Documentation/userspace-api/media/v4l/biblio.rst
>index 72aef1759b60..35674eeae20d 100644
>--- a/Documentation/userspace-api/media/v4l/biblio.rst
>+++ b/Documentation/userspace-api/media/v4l/biblio.rst
>@@ -334,6 +334,17 @@ VESA DMT
>
> :author: Video Electronics Standards Association (http://www.vesa.org)
>
>+.. _vesaeddc:
>+
>+E-DDC
>+=====
>+
>+
>+:title: VESA Enhanced Display Data Channel (E-DDC) Standard
>+:subtitle: Version 1.3
>+
>+:author: Video Electronics Standards Association (http://www.vesa.org)
>+
> .. _vesaedid:
>
> EDID
>diff --git a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
>index 6c57b8428356..3d11d86d9cbf 100644
>--- a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
>+++ b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
>@@ -244,6 +244,17 @@ specification the ioctl returns an ``EINVAL`` error code.
> - 0x01000000
> - The device supports the :c:func:`read()` and/or
> :c:func:`write()` I/O methods.
>+ * - ``V4L2_CAP_EDID``
>+ - 0x02000000
>+ - The device stores the EDID for a video input, or retrieves the EDID for a video
>+ output. It is a standalone EDID device, so no video streaming etc. will take place.
>+
>+ For a video input this is typically an eeprom that supports the
>+ :ref:`VESA Enhanced Display Data Channel Standard <vesaeddc>`. It can be something
>+ else as well, for example a micro controller.
>+
>+ For a video output this is typically read from an external device such as an
>+ HDMI splitter accessed by a serial port.
> * - ``V4L2_CAP_STREAMING``
> - 0x04000000
> - The device supports the :ref:`streaming <mmap>` I/O method.
>diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>index bdc628e8c1d6..d67fd4038d22 100644
>--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
>@@ -197,6 +197,7 @@ replace define V4L2_CAP_META_OUTPUT device-capabilities
> replace define V4L2_CAP_DEVICE_CAPS device-capabilities
> replace define V4L2_CAP_TOUCH device-capabilities
> replace define V4L2_CAP_IO_MC device-capabilities
>+replace define V4L2_CAP_EDID device-capabilities
>
> # V4L2 pix flags
> replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
>--
>2.43.0
>
>
^ permalink raw reply
* Re: [PATCH 1/6] media: videodev2.h: add V4L2_CAP_EDID
From: Ricardo Ribalda Delgado @ 2024-08-28 12:51 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-input, Erling Ljunggren
In-Reply-To: <485324508040defc0ba0fb211a6596dc65f2d994.1723190258.git.hverkuil-cisco@xs4all.nl>
Hi Hans
On Fri, Aug 9, 2024 at 10:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> From: Erling Ljunggren <hljunggr@cisco.com>
>
> Add capability flag to indicate that the device is an EDID-only device.
>
> Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> include/uapi/linux/videodev2.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 4e91362da6da..987c821aed79 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -502,6 +502,7 @@ struct v4l2_capability {
> #define V4L2_CAP_META_CAPTURE 0x00800000 /* Is a metadata capture device */
>
> #define V4L2_CAP_READWRITE 0x01000000 /* read/write systemcalls */
> +#define V4L2_CAP_EDID 0x02000000 /* Is an EDID-only device */
Would it make sense to add a check for "EDID-only", I mean, if the
driver sets this cap, then it should not set V4L2_CAP_STREAMING or others.
The test could be in the core or even in v4l2-compliance.
Regards!
> #define V4L2_CAP_STREAMING 0x04000000 /* streaming I/O ioctls */
> #define V4L2_CAP_META_OUTPUT 0x08000000 /* Is a metadata output device */
>
> --
> 2.43.0
>
>
--
Ricardo Ribalda
^ permalink raw reply
* Re: [PATCH] input: tegra: Use of_property_read_variable_u32_array() and of_property_present()
From: Thierry Reding @ 2024-08-28 15:05 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rob Herring (Arm), Laxman Dewangan, Jonathan Hunter, linux-input,
linux-tegra, linux-kernel
In-Reply-To: <Zs3yfc1pJDkAwhzc@google.com>
[-- Attachment #1: Type: text/plain, Size: 5741 bytes --]
On Tue, Aug 27, 2024 at 08:36:29AM GMT, Dmitry Torokhov wrote:
> On Tue, Aug 27, 2024 at 04:23:48PM +0200, Thierry Reding wrote:
> > On Wed, Jul 31, 2024 at 02:14:01PM GMT, Rob Herring (Arm) wrote:
> > > There's no need to get the length of an DT array property before
> > > parsing the array. of_property_read_variable_u32_array() takes a
> > > minimum and maximum length and returns the actual length (or error
> > > code).
> > >
> > > This is part of a larger effort to remove callers of of_get_property()
> > > and similar functions. of_get_property() leaks the DT property data
> > > pointer which is a problem for dynamically allocated nodes which may
> > > be freed.
> > > ---
> > > drivers/input/keyboard/tegra-kbc.c | 72 +++++++++++-------------------
> > > 1 file changed, 27 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c
> > > index a1765ed8c825..53f39fc155ea 100644
> > > --- a/drivers/input/keyboard/tegra-kbc.c
> > > +++ b/drivers/input/keyboard/tegra-kbc.c
> > > @@ -491,12 +491,10 @@ static int tegra_kbc_parse_dt(struct tegra_kbc *kbc)
> > > struct device_node *np = kbc->dev->of_node;
> > > u32 prop;
> > > int i;
> > > - u32 num_rows = 0;
> > > - u32 num_cols = 0;
> > > + int num_rows;
> > > + int num_cols;
> > > u32 cols_cfg[KBC_MAX_GPIO];
> > > u32 rows_cfg[KBC_MAX_GPIO];
> > > - int proplen;
> > > - int ret;
> > >
> > > if (!of_property_read_u32(np, "nvidia,debounce-delay-ms", &prop))
> > > kbc->debounce_cnt = prop;
> > > @@ -510,56 +508,23 @@ static int tegra_kbc_parse_dt(struct tegra_kbc *kbc)
> > > of_property_read_bool(np, "nvidia,wakeup-source")) /* legacy */
> > > kbc->wakeup = true;
> > >
> > > - if (!of_get_property(np, "nvidia,kbc-row-pins", &proplen)) {
> > > - dev_err(kbc->dev, "property nvidia,kbc-row-pins not found\n");
> > > - return -ENOENT;
> > > - }
> > > - num_rows = proplen / sizeof(u32);
> > > -
> > > - if (!of_get_property(np, "nvidia,kbc-col-pins", &proplen)) {
> > > - dev_err(kbc->dev, "property nvidia,kbc-col-pins not found\n");
> > > - return -ENOENT;
> > > - }
> > > - num_cols = proplen / sizeof(u32);
> > > -
> > > - if (num_rows > kbc->hw_support->max_rows) {
> > > - dev_err(kbc->dev,
> > > - "Number of rows is more than supported by hardware\n");
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (num_cols > kbc->hw_support->max_columns) {
> > > - dev_err(kbc->dev,
> > > - "Number of cols is more than supported by hardware\n");
> > > - return -EINVAL;
> > > - }
> > > -
> > > - if (!of_get_property(np, "linux,keymap", &proplen)) {
> > > + if (!of_property_present(np, "linux,keymap")) {
> > > dev_err(kbc->dev, "property linux,keymap not found\n");
> > > return -ENOENT;
> > > }
> > >
> > > - if (!num_rows || !num_cols || ((num_rows + num_cols) > KBC_MAX_GPIO)) {
> > > - dev_err(kbc->dev,
> > > - "keypad rows/columns not properly specified\n");
> > > - return -EINVAL;
> > > - }
> > > -
> > > /* Set all pins as non-configured */
> > > for (i = 0; i < kbc->num_rows_and_columns; i++)
> > > kbc->pin_cfg[i].type = PIN_CFG_IGNORE;
> > >
> > > - ret = of_property_read_u32_array(np, "nvidia,kbc-row-pins",
> > > - rows_cfg, num_rows);
> > > - if (ret < 0) {
> > > + num_rows = of_property_read_variable_u32_array(np, "nvidia,kbc-row-pins",
> > > + rows_cfg, 1, KBC_MAX_GPIO);
> > > + if (num_rows < 0) {
> > > dev_err(kbc->dev, "Rows configurations are not proper\n");
> > > - return -EINVAL;
> > > - }
> > > -
> > > - ret = of_property_read_u32_array(np, "nvidia,kbc-col-pins",
> > > - cols_cfg, num_cols);
> > > - if (ret < 0) {
> > > - dev_err(kbc->dev, "Cols configurations are not proper\n");
> > > + return num_rows;
> > > + } else if (num_rows > kbc->hw_support->max_rows) {
> > > + dev_err(kbc->dev,
> > > + "Number of rows is more than supported by hardware\n");
> > > return -EINVAL;
> > > }
> > >
> > > @@ -568,11 +533,28 @@ static int tegra_kbc_parse_dt(struct tegra_kbc *kbc)
> > > kbc->pin_cfg[rows_cfg[i]].num = i;
> > > }
> > >
> > > + num_cols = of_property_read_variable_u32_array(np, "nvidia,kbc-col-pins",
> > > + cols_cfg, 1, KBC_MAX_GPIO);
> > > + if (num_cols < 0) {
> > > + dev_err(kbc->dev, "Cols configurations are not proper\n");
> > > + return num_cols;
> > > + } else if (num_cols > kbc->hw_support->max_columns) {
> > > + dev_err(kbc->dev,
> > > + "Number of cols is more than supported by hardware\n");
> > > + return -EINVAL;
> > > + }
> > > +
> > > for (i = 0; i < num_cols; i++) {
> > > kbc->pin_cfg[cols_cfg[i]].type = PIN_CFG_COL;
> > > kbc->pin_cfg[cols_cfg[i]].num = i;
> > > }
> > >
> > > + if (!num_rows || !num_cols || ((num_rows + num_cols) > KBC_MAX_GPIO)) {
> > > + dev_err(kbc->dev,
> > > + "keypad rows/columns not properly specified\n");
> > > + return -EINVAL;
> > > + }
> >
> > Previously we wouldn't try to initialize the columns when the
> > rows/columns were invalid, so this block could move before the last for
> > loop above.
> >
> > But it doesn't really matter given that these are exceptions and really
> > shouldn't happen, so:
> >
> > Acked-by: Thierry Reding <treding@nvidia.com>
>
> I don't quite like of_property_read_variable_u32_array() because it is
> OF-specific. device_property_count_u32() will return the number of
> elements in an array. But I guess this driver will only be used on an OF
> system...
Yeah, this hardware only exists on fairly old Tegra devices and I know
of no plans to support anything other that DT on those.
Thierry
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH 4/6] input: serio.h: add SERIO_EXTRON_DA_HD_PLUS
From: Dmitry Torokhov @ 2024-08-28 16:11 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-input
In-Reply-To: <ccbb099a35cb788c7304795927f508cfc2342ff8.1723190258.git.hverkuil-cisco@xs4all.nl>
On Fri, Aug 09, 2024 at 09:57:36AM +0200, Hans Verkuil wrote:
> Add a new serio ID for the Extron DA HD 4K Plus series of 4K HDMI
> Distribution Amplifiers. These devices support CEC over the serial
> port, so a new serio ID is needed to be able to associate the CEC
> driver.
>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Please feel free to merge with the rest of the patches.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 4/6] input: serio.h: add SERIO_EXTRON_DA_HD_PLUS
From: Dmitry Torokhov @ 2024-08-28 16:13 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-input, linux-media
In-Reply-To: <157c368c-f01b-4378-be1f-4af6396d03f9@xs4all.nl>
Hi Hans,
On Wed, Aug 28, 2024 at 01:25:45PM +0200, Hans Verkuil wrote:
> Hi Dmitry,
>
> It's a trivial patch, but it would be great if you can give an Acked-by
> or Reviewed-by.
My apologies, I missed this.
Please make sure to CC me directly, not just the input list. Hmm, I see
that serio.h is missing from the MAINTAINERS file, let me add it...
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v25 31/33] ALSA: usb-audio: Add USB offload route kcontrol
From: Wesley Cheng @ 2024-08-28 18:59 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-usb, linux-input,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <4149884a-7c60-40d8-848b-8876f16d6d7f@linux.intel.com>
Hi Pierre,
On 8/26/2024 2:09 AM, Pierre-Louis Bossart wrote:
>
>> +config SND_USB_OFFLOAD_MIXER
>> + tristate "Qualcomm USB Audio Offload mixer control"
>> + help
>> + Say Y to enable the Qualcomm USB audio offloading mixer controls.
>> + This exposes an USB offload capable kcontrol to signal to
>> + applications about which platform sound card can support USB
>> + audio offload. This can potentially be used to fetch further
>> + information about the offloading status from the platform sound
>> + card.
> I would remove reference to Qualcomm for this Kconfig, all the code
> seems generic to me? Probably a left-over from the previous version.
Ah, yes will remove any vendor stuff.
Thanks
Wesley Cheng
^ permalink raw reply
* Re: [PATCH v25 00/33] Introduce QC USB SND audio offloading support
From: Wesley Cheng @ 2024-08-28 19:00 UTC (permalink / raw)
To: Pierre-Louis Bossart, srinivas.kandagatla, mathias.nyman, perex,
conor+dt, dmitry.torokhov, corbet, broonie, lgirdwood, tiwai,
krzk+dt, Thinh.Nguyen, bgoswami, robh, gregkh
Cc: linux-kernel, devicetree, linux-sound, linux-usb, linux-input,
linux-arm-msm, linux-doc, alsa-devel
In-Reply-To: <f4e609c0-92ff-4724-8243-bfe5de50d308@linux.intel.com>
Hi Pierre,
On 8/26/2024 2:28 AM, Pierre-Louis Bossart wrote:
>> Changelog
>> --------------------------------------------
>> Changes in v25:
>> - Cleanups on typos mentioned within the xHCI layers
>> - Modified the xHCI interrupter search if clients specify interrupter index
>> - Moved mixer_usb_offload into its own module, so that other vendor offload USB
>> modules can utilize it also.
>> - Added support for USB audio devices that may have multiple PCM streams, as
>> previous implementation only assumed a single PCM device. SOC USB will be
>> able to handle an array of PCM indexes supported by the USB audio device.
>> - Added some additional checks in the QC USB offload driver to check that device
>> has at least one playback stream before allowing to bind
>> - Reordered DT bindings to fix the error found by Rob's bot. The patch that
>> added USB_RX was after the example was updated.
>> - Updated comments within SOC USB to clarify terminology and to keep it consistent
>> - Added SND_USB_JACK type for notifying of USB device audio connections
> I went through the code and didn't find anything that looked like a
> major blocker. There are still a number of cosmetic things you'd want to
> fix such as using checkpatch.pl --strict --codespell to look for obvious
> style issues and typos, see selection below. git am also complains about
> EOF lines.
Thanks for the consistent reviews. Will fix the checkpatch errors and mis-spells. I didn't have codespell added so fixed that and resolved the typos.
Thanks
Wesley Cheng
> Overall this is starting to look good and ready for other reviewers to
> look at.
>
>
>
> WARNING: 'reaquire' may be misspelled - perhaps 'reacquire'?
> #54: FILE: drivers/usb/host/xhci-ring.c:3037:
> + * for non OS owned interrupter event ring. It may drop and reaquire
> xhci->lock
> ^^^^^^^^
> WARNING: 'compliation' may be misspelled - perhaps 'compilation'?
> #16:
> module compliation added by Wesley Cheng to complete original concept code
> ^^^^^^^^^^^
> CHECK: Prefer kzalloc(sizeof(*sgt)...) over kzalloc(sizeof(struct
> sg_table)...)
> #105: FILE: drivers/usb/host/xhci-sideband.c:35:
> + sgt = kzalloc(sizeof(struct sg_table), GFP_KERNEL);
>
> CHECK: struct mutex definition without comment
> #557: FILE: include/linux/usb/xhci-sideband.h:35:
> + struct mutex mutex;
>
> WARNING: 'straightfoward' may be misspelled - perhaps 'straightforward'?
> #22:
> straightfoward, as the ASoC components have direct references to the ASoC
> ^^^^^^^^^^^^^^
> CHECK: Unnecessary parentheses around 'card == sdev->card_idx'
> #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217:
> + if ((card == sdev->card_idx) &&
> + (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) {
>
> CHECK: Unnecessary parentheses around 'pcm ==
> sdev->ppcm_idx[sdev->num_playback - 1]'
> #142: FILE: sound/soc/qcom/qdsp6/q6usb.c:217:
> + if ((card == sdev->card_idx) &&
> + (pcm == sdev->ppcm_idx[sdev->num_playback - 1])) {
>
> WARNING: 'seqeunces' may be misspelled - perhaps 'sequences'?
> #8:
> seqeunces. This allows for platform USB SND modules to properly initialize
> ^^^^^^^^^
>
> WARNING: 'exisiting' may be misspelled - perhaps 'existing'?
> #12:
> exisiting parameters.
> ^^^^^^^^^
>
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #1020: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:98:
> +};
> +#define QMI_UAUDIO_STREAM_REQ_MSG_V01_MAX_MSG_LEN 46
>
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #1054: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:132:
> +};
> +#define QMI_UAUDIO_STREAM_RESP_MSG_V01_MAX_MSG_LEN 202
>
> CHECK: Please use a blank line after function/struct/union/enum declarations
> #1081: FILE: sound/usb/qcom/usb_audio_qmi_v01.h:159:
> +};
> +#define QMI_UAUDIO_STREAM_IND_MSG_V01_MAX_MSG_LEN 181
>
> CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues
> #100: FILE: sound/usb/mixer_usb_offload.c:19:
> +#define PCM_IDX(n) (n & 0xffff)
>
> CHECK: Macro argument 'n' may be better as '(n)' to avoid precedence issues
> #101: FILE: sound/usb/mixer_usb_offload.c:20:
> +#define CARD_IDX(n) (n >> 16)
>
^ permalink raw reply
* Re: [PATCH 1/6] media: videodev2.h: add V4L2_CAP_EDID
From: Hans Verkuil @ 2024-08-29 7:11 UTC (permalink / raw)
To: Ricardo Ribalda Delgado; +Cc: linux-media, linux-input, Erling Ljunggren
In-Reply-To: <CAPybu_2UFq6Rnyi+wPEGtXYLW5gF9a6yKiOBZwN95XymAxkxjQ@mail.gmail.com>
On 28/08/2024 14:51, Ricardo Ribalda Delgado wrote:
> Hi Hans
>
> On Fri, Aug 9, 2024 at 10:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>>
>> From: Erling Ljunggren <hljunggr@cisco.com>
>>
>> Add capability flag to indicate that the device is an EDID-only device.
>>
>> Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> include/uapi/linux/videodev2.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 4e91362da6da..987c821aed79 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -502,6 +502,7 @@ struct v4l2_capability {
>> #define V4L2_CAP_META_CAPTURE 0x00800000 /* Is a metadata capture device */
>>
>> #define V4L2_CAP_READWRITE 0x01000000 /* read/write systemcalls */
>> +#define V4L2_CAP_EDID 0x02000000 /* Is an EDID-only device */
>
> Would it make sense to add a check for "EDID-only", I mean, if the
> driver sets this cap, then it should not set V4L2_CAP_STREAMING or others.
>
> The test could be in the core or even in v4l2-compliance.
Good point. It is easy to add this to v4l2-compliance.
I noticed that the v4l2 core doesn't check for invalid caps combinations. It is
left to v4l2-compliance to check that. So I think I'll keep the check there.
Regards,
Hans
>
> Regards!
>
>
>> #define V4L2_CAP_STREAMING 0x04000000 /* streaming I/O ioctls */
>> #define V4L2_CAP_META_OUTPUT 0x08000000 /* Is a metadata output device */
>>
>> --
>> 2.43.0
>>
>>
>
>
> --
> Ricardo Ribalda
>
^ permalink raw reply
* Re: [PATCH 1/6] media: videodev2.h: add V4L2_CAP_EDID
From: Ricardo Ribalda Delgado @ 2024-08-29 7:25 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, linux-input, Erling Ljunggren
In-Reply-To: <d78f8808-2583-4a73-952a-8dfdcdf9b6e0@xs4all.nl>
On Thu, Aug 29, 2024 at 9:11 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
>
> On 28/08/2024 14:51, Ricardo Ribalda Delgado wrote:
> > Hi Hans
> >
> > On Fri, Aug 9, 2024 at 10:14 AM Hans Verkuil <hverkuil-cisco@xs4all.nl> wrote:
> >>
> >> From: Erling Ljunggren <hljunggr@cisco.com>
> >>
> >> Add capability flag to indicate that the device is an EDID-only device.
> >>
> >> Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >> include/uapi/linux/videodev2.h | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >> index 4e91362da6da..987c821aed79 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -502,6 +502,7 @@ struct v4l2_capability {
> >> #define V4L2_CAP_META_CAPTURE 0x00800000 /* Is a metadata capture device */
> >>
> >> #define V4L2_CAP_READWRITE 0x01000000 /* read/write systemcalls */
> >> +#define V4L2_CAP_EDID 0x02000000 /* Is an EDID-only device */
> >
> > Would it make sense to add a check for "EDID-only", I mean, if the
> > driver sets this cap, then it should not set V4L2_CAP_STREAMING or others.
> >
> > The test could be in the core or even in v4l2-compliance.
>
> Good point. It is easy to add this to v4l2-compliance.
>
> I noticed that the v4l2 core doesn't check for invalid caps combinations. It is
> left to v4l2-compliance to check that. So I think I'll keep the check there.
cool! Thanks
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Regards,
>
> Hans
>
> >
> > Regards!
> >
> >
> >> #define V4L2_CAP_STREAMING 0x04000000 /* streaming I/O ioctls */
> >> #define V4L2_CAP_META_OUTPUT 0x08000000 /* Is a metadata output device */
> >>
> >> --
> >> 2.43.0
> >>
> >>
> >
> >
> > --
> > Ricardo Ribalda
> >
>
--
Ricardo Ribalda
^ permalink raw reply
* Re: [PATCH 2/6] media: v4l2-dev: handle V4L2_CAP_EDID
From: Ricardo Ribalda Delgado @ 2024-08-29 7:27 UTC (permalink / raw)
To: Sebastian Fricke; +Cc: Hans Verkuil, linux-media, linux-input, Erling Ljunggren
In-Reply-To: <20240828123825.vnp4gqvyy7ohrhgw@basti-XPS-13-9310>
On Wed, Aug 28, 2024 at 2:38 PM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Hello,
>
> On 09.08.2024 09:57, Hans Verkuil wrote:
> >From: Erling Ljunggren <hljunggr@cisco.com>
> >
> >When the V4L2_CAP_EDID capability flag is set,
> >ioctls for enum inputs/outputs and get/set edid are automatically set.
> >
> >Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
> >Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Regards,
> Sebastian
> >---
> > drivers/media/v4l2-core/v4l2-dev.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> >diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> >index be2ba7ca5de2..570ba00e00b3 100644
> >--- a/drivers/media/v4l2-core/v4l2-dev.c
> >+++ b/drivers/media/v4l2-core/v4l2-dev.c
> >@@ -557,6 +557,7 @@ static void determine_valid_ioctls(struct video_device *vdev)
> > bool is_tx = vdev->vfl_dir != VFL_DIR_RX;
> > bool is_io_mc = vdev->device_caps & V4L2_CAP_IO_MC;
> > bool has_streaming = vdev->device_caps & V4L2_CAP_STREAMING;
> >+ bool is_edid = vdev->device_caps & V4L2_CAP_EDID;
> >
> > bitmap_zero(valid_ioctls, BASE_VIDIOC_PRIVATE);
> >
> >@@ -784,6 +785,20 @@ static void determine_valid_ioctls(struct video_device *vdev)
> > SET_VALID_IOCTL(ops, VIDIOC_S_TUNER, vidioc_s_tuner);
> > SET_VALID_IOCTL(ops, VIDIOC_S_HW_FREQ_SEEK, vidioc_s_hw_freq_seek);
> > }
> >+ if (is_edid) {
> >+ SET_VALID_IOCTL(ops, VIDIOC_G_EDID, vidioc_g_edid);
> >+ if (is_tx) {
> >+ SET_VALID_IOCTL(ops, VIDIOC_G_OUTPUT, vidioc_g_output);
> >+ SET_VALID_IOCTL(ops, VIDIOC_S_OUTPUT, vidioc_s_output);
> >+ SET_VALID_IOCTL(ops, VIDIOC_ENUMOUTPUT, vidioc_enum_output);
> >+ }
> >+ if (is_rx) {
> >+ SET_VALID_IOCTL(ops, VIDIOC_ENUMINPUT, vidioc_enum_input);
> >+ SET_VALID_IOCTL(ops, VIDIOC_G_INPUT, vidioc_g_input);
> >+ SET_VALID_IOCTL(ops, VIDIOC_S_INPUT, vidioc_s_input);
> >+ SET_VALID_IOCTL(ops, VIDIOC_S_EDID, vidioc_s_edid);
> >+ }
> >+ }
> >
> > bitmap_andnot(vdev->valid_ioctls, valid_ioctls, vdev->valid_ioctls,
> > BASE_VIDIOC_PRIVATE);
> >--
> >2.43.0
> >
> >
>
--
Ricardo Ribalda
^ permalink raw reply
* Re: [PATCH 3/6] media: docs: Add V4L2_CAP_EDID
From: Ricardo Ribalda Delgado @ 2024-08-29 7:27 UTC (permalink / raw)
To: Sebastian Fricke; +Cc: Hans Verkuil, linux-media, linux-input, Erling Ljunggren
In-Reply-To: <20240828123844.4dcpfsgii736hrq5@basti-XPS-13-9310>
On Wed, Aug 28, 2024 at 2:39 PM Sebastian Fricke
<sebastian.fricke@collabora.com> wrote:
>
> Hello,
>
> On 09.08.2024 09:57, Hans Verkuil wrote:
> >From: Erling Ljunggren <hljunggr@cisco.com>
> >
> >Add documentation for the new edid capability.
> >
> >Signed-off-by: Erling Ljunggren <hljunggr@cisco.com>
> >Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>
> Reviewed-by: Sebastian Fricke <sebastian.fricke@collabora.com>
Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> Regards,
> Sebastian
>
> >---
> > Documentation/userspace-api/media/v4l/biblio.rst | 11 +++++++++++
> > .../userspace-api/media/v4l/vidioc-querycap.rst | 11 +++++++++++
> > .../userspace-api/media/videodev2.h.rst.exceptions | 1 +
> > 3 files changed, 23 insertions(+)
> >
> >diff --git a/Documentation/userspace-api/media/v4l/biblio.rst b/Documentation/userspace-api/media/v4l/biblio.rst
> >index 72aef1759b60..35674eeae20d 100644
> >--- a/Documentation/userspace-api/media/v4l/biblio.rst
> >+++ b/Documentation/userspace-api/media/v4l/biblio.rst
> >@@ -334,6 +334,17 @@ VESA DMT
> >
> > :author: Video Electronics Standards Association (http://www.vesa.org)
> >
> >+.. _vesaeddc:
> >+
> >+E-DDC
> >+=====
> >+
> >+
> >+:title: VESA Enhanced Display Data Channel (E-DDC) Standard
> >+:subtitle: Version 1.3
> >+
> >+:author: Video Electronics Standards Association (http://www.vesa.org)
> >+
> > .. _vesaedid:
> >
> > EDID
> >diff --git a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
> >index 6c57b8428356..3d11d86d9cbf 100644
> >--- a/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
> >+++ b/Documentation/userspace-api/media/v4l/vidioc-querycap.rst
> >@@ -244,6 +244,17 @@ specification the ioctl returns an ``EINVAL`` error code.
> > - 0x01000000
> > - The device supports the :c:func:`read()` and/or
> > :c:func:`write()` I/O methods.
> >+ * - ``V4L2_CAP_EDID``
> >+ - 0x02000000
> >+ - The device stores the EDID for a video input, or retrieves the EDID for a video
> >+ output. It is a standalone EDID device, so no video streaming etc. will take place.
> >+
> >+ For a video input this is typically an eeprom that supports the
> >+ :ref:`VESA Enhanced Display Data Channel Standard <vesaeddc>`. It can be something
> >+ else as well, for example a micro controller.
> >+
> >+ For a video output this is typically read from an external device such as an
> >+ HDMI splitter accessed by a serial port.
> > * - ``V4L2_CAP_STREAMING``
> > - 0x04000000
> > - The device supports the :ref:`streaming <mmap>` I/O method.
> >diff --git a/Documentation/userspace-api/media/videodev2.h.rst.exceptions b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >index bdc628e8c1d6..d67fd4038d22 100644
> >--- a/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >+++ b/Documentation/userspace-api/media/videodev2.h.rst.exceptions
> >@@ -197,6 +197,7 @@ replace define V4L2_CAP_META_OUTPUT device-capabilities
> > replace define V4L2_CAP_DEVICE_CAPS device-capabilities
> > replace define V4L2_CAP_TOUCH device-capabilities
> > replace define V4L2_CAP_IO_MC device-capabilities
> >+replace define V4L2_CAP_EDID device-capabilities
> >
> > # V4L2 pix flags
> > replace define V4L2_PIX_FMT_PRIV_MAGIC :c:type:`v4l2_pix_format`
> >--
> >2.43.0
> >
> >
>
--
Ricardo Ribalda
^ permalink raw reply
* Re: [PATCH v5 0/4] HID: hidraw: HIDIOCREVOKE introduction
From: Benjamin Tissoires @ 2024-08-29 8:41 UTC (permalink / raw)
To: Jiri Kosina, Shuah Khan, Peter Hutterer, bentiss
Cc: linux-input, linux-kernel, linux-kselftest
In-Reply-To: <20240827-hidraw-revoke-v5-0-d004a7451aea@kernel.org>
On Tue, 27 Aug 2024 17:19:28 +0900, bentiss@kernel.org wrote:
> The is the v5 of the HIDIOCREVOKE patches.
>
> After a small discussion with Peter, we decided to:
> - drop the BPF hooks that are problematic (Linus doesn't want
> "ALLOW_ERROR_INJECTION" to be used as "normal" fmodret bpf hooks)
> - punt those BPF hooks later once we get the API right
> - I'll be the one sending that new version, given that it's easier for
> me ATM
>
> [...]
Applied to hid/hid.git (for-6.12/hidraw), thanks!
[1/4] HID: hidraw: add HIDIOCREVOKE ioctl
https://git.kernel.org/hid/hid/c/b31c9d9dc343
[2/4] selftests/hid: extract the utility part of hid_bpf.c into its own header
https://git.kernel.org/hid/hid/c/375e9bde9fc0
[3/4] selftests/hid: Add initial hidraw tests skeleton
https://git.kernel.org/hid/hid/c/8163892a629c
[4/4] selftests/hid: Add HIDIOCREVOKE tests
https://git.kernel.org/hid/hid/c/321f7798cfb8
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* [PATCH 1/2] input: touchscreem: ad7877: add match table
From: Antoniu Miclaus @ 2024-08-29 9:19 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Hennerich, Antoniu Miclaus, linux-input, devicetree,
linux-kernel
Add match table for the ad7877 driver and define the compatible string.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
drivers/input/touchscreen/ad7877.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index a0598e9c7aff..7886454a19c6 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -805,10 +805,17 @@ static int ad7877_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(ad7877_pm, ad7877_suspend, ad7877_resume);
+static const struct of_device_id ad7877_of_match[] = {
+ { .compatible = "adi,ad7877", },
+ { }
+};
+MODULE_DEVICE_TABLE(of, ad7877_of_match);
+
static struct spi_driver ad7877_driver = {
.driver = {
.name = "ad7877",
.dev_groups = ad7877_groups,
+ .of_match_table = ad7877_of_match,
.pm = pm_sleep_ptr(&ad7877_pm),
},
.probe = ad7877_probe,
--
2.46.0
^ permalink raw reply related
* [PATCH 2/2] dt-bindings: touchscreen: ad7877: add bindings
From: Antoniu Miclaus @ 2024-08-29 9:19 UTC (permalink / raw)
To: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Hennerich, Antoniu Miclaus, linux-input, devicetree,
linux-kernel
In-Reply-To: <20240829092007.25850-1-antoniu.miclaus@analog.com>
Add device tree bindings for the ad7877 driver.
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
.../input/touchscreen/adi,ad7877.yaml | 58 +++++++++++++++++++
1 file changed, 58 insertions(+)
create mode 100644 Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
diff --git a/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
new file mode 100644
index 000000000000..5fc5124c5999
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/touchscreen/adi,ad7877.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD7877 Touch Screen Controller
+
+maintainers:
+ - Antoniu Miclaus <antoniu.miclaus@analog.com>
+
+description: |
+ Analog Devices Touch Screen Controller
+ https://www.analog.com/media/en/technical-documentation/data-sheets/AD7877.pdf
+
+allOf:
+ - $ref: touchscreen.yaml#
+ - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad7877
+
+ reg:
+ maxItems: 1
+
+ interrupts:
+ maxItems: 1
+
+ spi-max-frequency:
+ description: AD7877 SPI bus clock frequency.
+ minimum: 10000
+ maximum: 20000000
+
+required:
+ - compatible
+ - reg
+ - interrupts
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ touchscreen@0 {
+ compatible = "adi,ad7877";
+ reg = <0>;
+ spi-max-frequency = <20000000>;
+ interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio>;
+ };
+ };
+...
--
2.46.0
^ permalink raw reply related
* Re: [PATCH v6 2/7] mfd: add base driver for qnap-mcu devices
From: Lee Jones @ 2024-08-29 13:38 UTC (permalink / raw)
To: Heiko Stuebner
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, dmitry.torokhov, pavel,
ukleinek, devicetree, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-3-heiko@sntech.de>
On Sun, 25 Aug 2024, Heiko Stuebner wrote:
> These microcontroller units are used in network-attached-storage devices
> made by QNAP and provide additional functionality to the system.
>
> This adds the base driver that implements the serial protocol via
> serdev and additionally hooks into the poweroff handlers to turn
> off the parts of the system not supplied by the general PMIC.
>
> Turning off (at least the TSx33 devices using Rockchip SoCs) consists of
> two separate actions. Turning off the MCU alone does not turn off the main
> SoC and turning off only the SoC/PMIC does not turn off the hard-drives.
> Also if the MCU is not turned off, the system also won't start again until
> it is unplugged from power.
>
> So on shutdown the MCU needs to be turned off separately before the
> main PMIC.
>
> The protocol spoken by the MCU is sadly not documented, but was
> obtained by listening to the chatter on the serial port, as thankfully
> the "hal_app" program from QNAPs firmware allows triggering all/most
> MCU actions from the command line.
>
> The implementation of how to talk to the serial device got some
> inspiration from the rave-sp servdev driver.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> MAINTAINERS | 6 +
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 2 +
> drivers/mfd/qnap-mcu.c | 339 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/qnap-mcu.h | 27 +++
> 5 files changed, 387 insertions(+)
> create mode 100644 drivers/mfd/qnap-mcu.c
> create mode 100644 include/linux/mfd/qnap-mcu.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 3e7359ac005c..0fbd2d953da4 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18654,6 +18654,12 @@ L: linux-media@vger.kernel.org
> S: Odd Fixes
> F: drivers/media/tuners/qm1d1c0042*
>
> +QNAP MCU DRIVER
> +M: Heiko Stuebner <heiko@sntech.de>
> +S: Maintained
> +F: drivers/mfd/qnap-mcu.c
> +F: include/linux/qnap-mcu.h
> +
> QNX4 FILESYSTEM
> M: Anders Larsen <al@alarsen.net>
> S: Maintained
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index bc8be2e593b6..d43d2747ac4e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -2362,6 +2362,19 @@ config MFD_INTEL_M10_BMC_PMCI
> additional drivers must be enabled in order to use the functionality
> of the device.
>
> +config MFD_QNAP_MCU
> + tristate "QNAP microcontroller unit core driver"
> + depends on SERIAL_DEV_BUS
> + select MFD_CORE
> + help
> + Select this to get support for the QNAP MCU device found in
> + several devices of QNAP network attached storage devices that
> + implements additional functionality for the device, like
> + fan- and LED control.
Is this intentional formatting or did you forget to finish the word?
> + This driver implements the base serial protocol to talk to the
> + device and provides functions for the other parts to hook into.
> +
> config MFD_RSMU_I2C
> tristate "Renesas Synchronization Management Unit with I2C"
> depends on I2C && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 02b651cd7535..fc8b825725ff 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -286,5 +286,7 @@ obj-$(CONFIG_MFD_INTEL_M10_BMC_PMCI) += intel-m10-bmc-pmci.o
> obj-$(CONFIG_MFD_ATC260X) += atc260x-core.o
> obj-$(CONFIG_MFD_ATC260X_I2C) += atc260x-i2c.o
>
> +obj-$(CONFIG_MFD_QNAP_MCU) += qnap-mcu.o
> +
> obj-$(CONFIG_MFD_RSMU_I2C) += rsmu_i2c.o rsmu_core.o
> obj-$(CONFIG_MFD_RSMU_SPI) += rsmu_spi.o rsmu_core.o
> diff --git a/drivers/mfd/qnap-mcu.c b/drivers/mfd/qnap-mcu.c
> new file mode 100644
> index 000000000000..ab6036e9a843
> --- /dev/null
> +++ b/drivers/mfd/qnap-mcu.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Core driver for the microcontroller unit in QNAP NAS devices that is
> + * connected via a dedicated UART port.
> + *
> + * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
> + */
> +
> +#include <linux/cleanup.h>
> +#include <linux/export.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/qnap-mcu.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/reboot.h>
> +#include <linux/serdev.h>
> +#include <linux/slab.h>
> +
> +/* The longest command found so far is 5 bytes long */
> +#define QNAP_MCU_MAX_CMD_SIZE 5
> +#define QNAP_MCU_MAX_DATA_SIZE 36
> +#define QNAP_MCU_CHECKSUM_SIZE 1
> +
> +#define QNAP_MCU_RX_BUFFER_SIZE \
> + (QNAP_MCU_MAX_DATA_SIZE + QNAP_MCU_CHECKSUM_SIZE)
> +
> +#define QNAP_MCU_TX_BUFFER_SIZE \
> + (QNAP_MCU_MAX_CMD_SIZE + QNAP_MCU_CHECKSUM_SIZE)
> +
> +#define QNAP_MCU_ACK_LEN 2
> +#define QNAP_MCU_VERSION_LEN 4
> +
> +/**
> + * struct qnap_mcu_reply - Reply to a command
> + *
> + * @data: Buffer to store reply payload in
> + * @length: Expected reply length, including the checksum
> + * @received: Received number of bytes, so far
> + * @done: Triggered when the entire reply has been received
> + */
> +struct qnap_mcu_reply {
> + u8 *data;
> + size_t length;
> + size_t received;
> + struct completion done;
> +};
> +
> +/**
> + * struct qnap_mcu - QNAP NAS embedded controller
> + *
> + * @serdev: Pointer to underlying serdev
> + * @bus_lock: Lock to serialize access to the device
> + * @reply: Reply data structure
> + * @variant: Device variant specific information
> + * @version: MCU firmware version
> + */
> +struct qnap_mcu {
> + struct serdev_device *serdev;
> + struct mutex bus_lock;
> + struct qnap_mcu_reply reply;
> + const struct qnap_mcu_variant *variant;
> + u8 version[QNAP_MCU_VERSION_LEN];
> +};
> +
> +/*
> + * The QNAP-MCU uses a basic XOR checksum.
> + * It is always the last byte and XORs the whole previous message.
> + */
> +static u8 qnap_mcu_csum(const u8 *buf, size_t size)
> +{
> + u8 csum = 0;
> +
> + while (size--)
> + csum ^= *buf++;
> +
> + return csum;
> +}
> +
> +static int qnap_mcu_write(struct qnap_mcu *mcu, const u8 *data, u8 data_size)
> +{
> + unsigned char tx[QNAP_MCU_TX_BUFFER_SIZE];
> + size_t length = data_size + QNAP_MCU_CHECKSUM_SIZE;
> +
> + if (length > sizeof(tx)) {
> + dev_err(&mcu->serdev->dev, "data too big for transmit buffer");
> + return -EINVAL;
> + }
> +
> + memcpy(tx, data, data_size);
> + tx[data_size] = qnap_mcu_csum(data, data_size);
> +
> + return serdev_device_write(mcu->serdev, tx, length, HZ);
> +}
> +
> +static size_t qnap_mcu_receive_buf(struct serdev_device *serdev, const u8 *buf, size_t size)
> +{
> + struct device *dev = &serdev->dev;
> + struct qnap_mcu *mcu = dev_get_drvdata(dev);
> + struct qnap_mcu_reply *reply = &mcu->reply;
> + const u8 *src = buf;
> + const u8 *end = buf + size;
> +
> + if (!reply->length) {
> + dev_warn(dev, "Received %zu bytes, we were not waiting for\n",
> + size);
This seems like an odd place to break when the line above is clearly longer.
Consistency is key.
> + return size;
> + }
> +
> + while (src < end) {
> + reply->data[reply->received] = *src++;
> + reply->received++;
> +
> + if (reply->received == reply->length) {
> + complete(&reply->done);
> +
> + /*
> + * We report the consumed number of bytes. If there
> + * are still bytes remaining (though there shouldn't)
> + * the serdev layer will re-execute this handler with
> + * the remainder of the Rx bytes.
> + */
> + return src - buf;
> + }
> + }
> +
> + /*
> + * The only way to get out of the above loop and end up here
> + * is through consuming all of the supplied data, so here we
> + * report that we processed it all.
> + */
> + return size;
> +}
> +
> +static const struct serdev_device_ops qnap_mcu_serdev_device_ops = {
> + .receive_buf = qnap_mcu_receive_buf,
> + .write_wakeup = serdev_device_write_wakeup,
> +};
> +
> +int qnap_mcu_exec(struct qnap_mcu *mcu,
> + const u8 *cmd_data, size_t cmd_data_size,
> + u8 *reply_data, size_t reply_data_size)
> +{
> + unsigned char rx[QNAP_MCU_RX_BUFFER_SIZE];
> + size_t length = reply_data_size + QNAP_MCU_CHECKSUM_SIZE;
> + struct qnap_mcu_reply *reply = &mcu->reply;
> + int ret;
> +
> + if (length > sizeof(rx)) {
> + dev_err(&mcu->serdev->dev, "expected data too big for receive buffer");
> + return -EINVAL;
> + }
> +
> + mutex_lock(&mcu->bus_lock);
> +
> + /* Initialize reply fields */
This comment is superfluous / obvious.
> + reply->data = rx,
> + reply->length = length,
> + reply->received = 0,
> + reinit_completion(&reply->done);
> +
> + qnap_mcu_write(mcu, cmd_data, cmd_data_size);
> +
> + if (!wait_for_completion_timeout(&reply->done,
> + msecs_to_jiffies(500))) {
Another line-wrap weirdness - stick to 100 chars throughout and have done.
> + dev_err(&mcu->serdev->dev, "Command timeout\n");
> + ret = -ETIMEDOUT;
> + } else {
> + u8 crc = qnap_mcu_csum(rx, reply_data_size);
> +
> + if (crc != rx[reply_data_size]) {
> + dev_err(&mcu->serdev->dev,
> + "Checksum 0x%02x wrong for data\n", crc);
Invalid checksum received?
> + ret = -EIO;
> + } else {
> + memcpy(reply_data, rx, reply_data_size);
> + ret = 0;
Why not pre-initialise it when during declaration?
> + }
> + }
> +
> + /* We don't expect any characters from the device now */
> + reply->length = 0;
> +
> + mutex_unlock(&mcu->bus_lock);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(qnap_mcu_exec);
> +
> +int qnap_mcu_exec_with_ack(struct qnap_mcu *mcu,
> + const u8 *cmd_data, size_t cmd_data_size)
> +{
> + u8 ack[QNAP_MCU_ACK_LEN];
> + int ret;
> +
> + ret = qnap_mcu_exec(mcu, cmd_data, cmd_data_size, ack, sizeof(ack));
> + if (ret)
> + return ret;
> +
> + /* Should return @0 */
> + if (ack[0] != 0x40 || ack[1] != 0x30) {
> + dev_err(&mcu->serdev->dev, "Did not receive ack\n");
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(qnap_mcu_exec_with_ack);
> +
> +const struct qnap_mcu_variant *qnap_mcu_get_variant_data(struct qnap_mcu *mcu)
> +{
> + return mcu->variant;
> +}
> +EXPORT_SYMBOL_GPL(qnap_mcu_get_variant_data);
Why not pass this through platform data?
> +static int qnap_mcu_get_version(struct qnap_mcu *mcu)
> +{
> + const u8 cmd[] = { 0x25, 0x56 }; /* %, V */
> + u8 rx[14];
> + int ret;
> +
> + /* Reply is the 2 command-bytes + 4 bytes describing the version */
> + ret = qnap_mcu_exec(mcu, cmd, sizeof(cmd), rx, QNAP_MCU_VERSION_LEN + 2);
> + if (ret)
> + return ret;
> +
> + memcpy(mcu->version, &rx[2], QNAP_MCU_VERSION_LEN);
> +
> + return 0;
> +}
> +
> +/*
> + * The MCU controls power to the peripherals but not the CPU.
> + *
> + * So using the pmic to power off the system keeps the MCU and hard-drives
"PMIC"
> + * running. This also then prevents the system from turning back on until
> + * the MCU is turned off by unplugging the power-cable.
"power cable" is not hyphenated. Neither is "hard drives".
> + * Turning off the MCU alone on the other hand turns off the hard-drives,
> + * LEDs, etc while the main SoC stays running - including its network ports.
> + */
> +static int qnap_mcu_power_off(struct sys_off_data *data)
> +{
> + const u8 cmd[] = { 0x40, 0x43, 0x30 }; /* @, C, 0 */
> + struct qnap_mcu *mcu = data->cb_data;
> + int ret;
> +
> + ret = qnap_mcu_exec_with_ack(mcu, cmd, sizeof(cmd));
> + if (ret) {
> + dev_err(&mcu->serdev->dev, "MCU poweroff failed %d\n", ret);
> + return NOTIFY_STOP;
> + }
> +
> + return NOTIFY_DONE;
> +}
> +
> +static const struct qnap_mcu_variant qnap_ts433_mcu = {
> + .baud_rate = 115200,
> + .num_drives = 4,
> + .fan_pwm_min = 51, /* Specified in original model.conf */
> + .fan_pwm_max = 255,
> + .usb_led = true,
> +};
> +
> +static const struct of_device_id qnap_mcu_dt_ids[] = {
> + { .compatible = "qnap,ts433-mcu", .data = &qnap_ts433_mcu },
> + { /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, qnap_mcu_dt_ids);
> +
> +static const struct mfd_cell qnap_mcu_subdevs[] = {
> + { .name = "qnap-mcu-input", },
> + { .name = "qnap-mcu-leds", },
> + { .name = "qnap-mcu-hwmon", }
> +};
Please swap these two structures around.
It's common to place of_device_id either just before or just after
.probe() depending on whether it's used or not. Ooo, thinking on the
spot, it looks like qnap_mcu_dt_ids doesn't need to be up here at all.
Please place it _below_ .probe().
> +static int qnap_mcu_probe(struct serdev_device *serdev)
> +{
> + struct device *dev = &serdev->dev;
> + struct qnap_mcu *mcu;
> + int ret;
> +
> + mcu = devm_kzalloc(dev, sizeof(*mcu), GFP_KERNEL);
> + if (!mcu)
> + return -ENOMEM;
> +
> + mcu->serdev = serdev;
> + dev_set_drvdata(dev, mcu);
> +
> + mcu->variant = of_device_get_match_data(dev);
> + if (!mcu->variant)
> + return -ENODEV;
> +
> + mutex_init(&mcu->bus_lock);
> + init_completion(&mcu->reply.done);
> +
> + serdev_device_set_client_ops(serdev, &qnap_mcu_serdev_device_ops);
> + ret = devm_serdev_device_open(dev, serdev);
> + if (ret)
> + return ret;
> +
> + serdev_device_set_baudrate(serdev, mcu->variant->baud_rate);
> + serdev_device_set_flow_control(serdev, false);
> +
> + ret = serdev_device_set_parity(serdev, SERDEV_PARITY_NONE);
> + if (ret) {
> + dev_err(dev, "Failed to set parity\n");
> + return ret;
> + }
> +
> + ret = qnap_mcu_get_version(mcu);
> + if (ret)
> + return ret;
> +
> + ret = devm_register_sys_off_handler(dev,
> + SYS_OFF_MODE_POWER_OFF_PREPARE,
> + SYS_OFF_PRIO_DEFAULT,
> + &qnap_mcu_power_off, mcu);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to register poweroff handler\n");
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, qnap_mcu_subdevs,
> + ARRAY_SIZE(qnap_mcu_subdevs), NULL, 0, NULL);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to add child devices\n");
> +
> + return 0;
> +}
> +
> +static struct serdev_device_driver qnap_mcu_drv = {
> + .probe = qnap_mcu_probe,
> + .driver = {
> + .name = "qnap-mcu",
> + .of_match_table = qnap_mcu_dt_ids,
> + },
> +};
> +module_serdev_device_driver(qnap_mcu_drv);
> +
> +MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
> +MODULE_DESCRIPTION("QNAP MCU core driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/qnap-mcu.h b/include/linux/mfd/qnap-mcu.h
> new file mode 100644
> index 000000000000..903b923537c7
> --- /dev/null
> +++ b/include/linux/mfd/qnap-mcu.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Core definitions for QNAP MCU MFD driver.
> + * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
> + */
> +
> +#ifndef _LINUX_QNAP_MCU_H_
> +#define _LINUX_QNAP_MCU_H_
> +
> +struct qnap_mcu;
> +
> +struct qnap_mcu_variant {
> + u32 baud_rate;
> + int num_drives;
> + int fan_pwm_min;
> + int fan_pwm_max;
> + bool usb_led;
> +};
> +
> +int qnap_mcu_exec(struct qnap_mcu *mcu,
> + const u8 *cmd_data, size_t cmd_data_size,
> + u8 *reply_data, size_t reply_data_size);
> +int qnap_mcu_exec_with_ack(struct qnap_mcu *mcu,
> + const u8 *cmd_data, size_t cmd_data_size);
> +const struct qnap_mcu_variant *qnap_mcu_get_variant_data(struct qnap_mcu *mcu);
> +
> +#endif /* _LINUX_QNAP_MCU_H_ */
> --
> 2.43.0
>
>
--
Lee Jones [李琼斯]
^ permalink raw reply
* [PATCH] dt-bindings: input: touchscreen: edt-ft5x06: Use generic node name
From: Fabio Estevam @ 2024-08-29 13:44 UTC (permalink / raw)
To: dmitry.torokhov
Cc: robh, krzk+dt, conor+dt, linux-input, devicetree, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
Node names should be generic.
Improve the binding example by using 'touchscreen' as the node name.
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
.../devicetree/bindings/input/touchscreen/edt-ft5x06.yaml | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
index 51d48d4130d3..70a922e213f2 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/edt-ft5x06.yaml
@@ -126,7 +126,7 @@ examples:
i2c {
#address-cells = <1>;
#size-cells = <0>;
- edt-ft5x06@38 {
+ touchscreen@38 {
compatible = "edt,edt-ft5406";
reg = <0x38>;
interrupt-parent = <&gpio2>;
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 2/2] dt-bindings: touchscreen: ad7877: add bindings
From: Conor Dooley @ 2024-08-29 16:16 UTC (permalink / raw)
To: Antoniu Miclaus
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Hennerich, linux-input, devicetree, linux-kernel
In-Reply-To: <20240829092007.25850-2-antoniu.miclaus@analog.com>
[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]
On Thu, Aug 29, 2024 at 12:19:36PM +0300, Antoniu Miclaus wrote:
> Add device tree bindings for the ad7877 driver.
>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> .../input/touchscreen/adi,ad7877.yaml | 58 +++++++++++++++++++
> 1 file changed, 58 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> new file mode 100644
> index 000000000000..5fc5124c5999
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/touchscreen/adi,ad7877.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD7877 Touch Screen Controller
> +
> +maintainers:
> + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> +
> +description: |
> + Analog Devices Touch Screen Controller
> + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7877.pdf
> +
> +allOf:
> + - $ref: touchscreen.yaml#
> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +unevaluatedProperties: false
So, all of the properties in those two files are valid for this
touchscreen controller?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v6 3/7] leds: add driver for LEDs from qnap-mcu devices
From: Lee Jones @ 2024-08-29 16:27 UTC (permalink / raw)
To: Heiko Stuebner
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, dmitry.torokhov, pavel,
ukleinek, devicetree, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240825203235.1122198-4-heiko@sntech.de>
On Sun, 25 Aug 2024, Heiko Stuebner wrote:
> This adds a driver that connects to the qnap-mcu mfd driver and provides
> access to the LEDs on it.
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> MAINTAINERS | 1 +
> drivers/leds/Kconfig | 11 ++
> drivers/leds/Makefile | 1 +
> drivers/leds/leds-qnap-mcu.c | 226 +++++++++++++++++++++++++++++++++++
> 4 files changed, 239 insertions(+)
> create mode 100644 drivers/leds/leds-qnap-mcu.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0fbd2d953da4..4dff0e237f22 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -18657,6 +18657,7 @@ F: drivers/media/tuners/qm1d1c0042*
> QNAP MCU DRIVER
> M: Heiko Stuebner <heiko@sntech.de>
> S: Maintained
> +F: drivers/leds/leds-qnap-mcu.c
> F: drivers/mfd/qnap-mcu.c
> F: include/linux/qnap-mcu.h
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index 8d9d8da376e4..9a337478dd80 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -580,6 +580,17 @@ config LEDS_PCA995X
> LED driver chips accessed via the I2C bus. Supported
> devices include PCA9955BTW, PCA9952TW and PCA9955TW.
>
> +config LEDS_QNAP_MCU
> + tristate "LED Support for QNAP MCU controllers"
> + depends on LEDS_CLASS
> + depends on MFD_QNAP_MCU
> + help
> + This option enables support for LEDs available on embedded
> + controllers used in QNAP NAS devices.
> +
> + This driver can also be built as a module. If so, the module
> + will be called qnap-mcu-leds.
> +
> config LEDS_WM831X_STATUS
> tristate "LED support for status LEDs on WM831x PMICs"
> depends on LEDS_CLASS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 18afbb5a23ee..c6f74865d729 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -79,6 +79,7 @@ obj-$(CONFIG_LEDS_PCA995X) += leds-pca995x.o
> obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o
> obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
> obj-$(CONFIG_LEDS_PWM) += leds-pwm.o
> +obj-$(CONFIG_LEDS_QNAP_MCU) += leds-qnap-mcu.o
> obj-$(CONFIG_LEDS_REGULATOR) += leds-regulator.o
> obj-$(CONFIG_LEDS_SC27XX_BLTC) += leds-sc27xx-bltc.o
> obj-$(CONFIG_LEDS_SUN50I_A100) += leds-sun50i-a100.o
> diff --git a/drivers/leds/leds-qnap-mcu.c b/drivers/leds/leds-qnap-mcu.c
> new file mode 100644
> index 000000000000..0723ec52e4c5
> --- /dev/null
> +++ b/drivers/leds/leds-qnap-mcu.c
> @@ -0,0 +1,226 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
Superfluous line.
> +/*
> + * Driver for LEDs found on QNAP MCU devices
> + *
> + * Copyright (C) 2024 Heiko Stuebner <heiko@sntech.de>
> + */
> +
> +#include <linux/leds.h>
> +#include <linux/mfd/qnap-mcu.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <uapi/linux/uleds.h>
> +
> +enum qnap_mcu_err_led_mode {
> + QNAP_MCU_ERR_LED_ON = 0,
> + QNAP_MCU_ERR_LED_OFF = 1,
> + QNAP_MCU_ERR_LED_BLINK_FAST = 2,
> + QNAP_MCU_ERR_LED_BLINK_SLOW = 3,
> +};
> +
> +struct qnap_mcu_err_led {
> + struct qnap_mcu *mcu;
> + struct led_classdev cdev;
> + char name[LED_MAX_NAME_SIZE];
> + u8 num;
> + u8 mode;
> +};
> +
> +static inline struct qnap_mcu_err_led *
> + cdev_to_qnap_mcu_err_led(struct led_classdev *led_cdev)
> +{
> + return container_of(led_cdev, struct qnap_mcu_err_led, cdev);
> +}
> +
> +static int qnap_mcu_err_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
'value' is a terrible variable name.
Please describe the data. 'brightness' seems appropriate.
> +{
> + struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
Really not fan of these magic values being used raw like this.
> + /* Don't disturb a possible set blink-mode if LED is already on */
Why not save cycles and return if blink mode is already enabled?
> + if (value == 0)
> + err_led->mode = QNAP_MCU_ERR_LED_OFF;
> + else if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
> + err_led->mode = QNAP_MCU_ERR_LED_ON;
Then you can do:
err_led->mode = value ? QNAP_MCU_ERR_LED_ON : QNAP_MCU_ERR_LED_OFF;
> + cmd[3] = 0x30 + err_led->mode;
> +
> + return qnap_mcu_exec_with_ack(err_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_err_led_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
> +
> + /* LED is off, nothing to do */
> + if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
> + return 0;
> +
> + if (*delay_on < 500) {
Setting delay_on based on the current value of delay_on sounds sketchy.
> + *delay_on = 100;
> + *delay_off = 100;
> + err_led->mode = QNAP_MCU_ERR_LED_BLINK_FAST;
> + } else {
> + *delay_on = 500;
> + *delay_off = 500;
> + err_led->mode = QNAP_MCU_ERR_LED_BLINK_SLOW;
> + }
How do you change from a fast to a slow blinking LED and back again?
> + cmd[3] = 0x30 + err_led->mode;
> +
> + return qnap_mcu_exec_with_ack(err_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_register_err_led(struct device *dev, struct qnap_mcu *mcu, int num)
What's num? I should be able to answer that by the nomenclature.
> +{
> + struct qnap_mcu_err_led *err_led;
> + int ret;
> +
> + err_led = devm_kzalloc(dev, sizeof(*err_led), GFP_KERNEL);
> + if (!err_led)
> + return -ENOMEM;
> +
> + err_led->mcu = mcu;
> + err_led->num = num;
> + err_led->mode = QNAP_MCU_ERR_LED_OFF;
> +
> + snprintf(err_led->name, LED_MAX_NAME_SIZE, "hdd%d:red:status", num + 1);
scnprintf()?
> + err_led->cdev.name = err_led->name;
> +
> + err_led->cdev.brightness_set_blocking = qnap_mcu_err_led_set;
> + err_led->cdev.blink_set = qnap_mcu_err_led_blink_set;
> + err_led->cdev.brightness = 0;
> + err_led->cdev.max_brightness = 1;
> +
> + ret = devm_led_classdev_register(dev, &err_led->cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register hdd led %d", num);
"HDD LED"
> + return qnap_mcu_err_led_set(&err_led->cdev, 0);
> +}
> +
> +enum qnap_mcu_usb_led_mode {
> + QNAP_MCU_USB_LED_ON = 1,
> + QNAP_MCU_USB_LED_OFF = 3,
> + QNAP_MCU_USB_LED_BLINK = 2,
> +};
> +
> +struct qnap_mcu_usb_led {
> + struct qnap_mcu *mcu;
> + struct led_classdev cdev;
> + u8 mode;
> +};
> +
> +static inline struct qnap_mcu_usb_led *
> + cdev_to_qnap_mcu_usb_led(struct led_classdev *led_cdev)
> +{
> + return container_of(led_cdev, struct qnap_mcu_usb_led, cdev);
> +}
> +
> +static int qnap_mcu_usb_led_set(struct led_classdev *led_cdev,
> + enum led_brightness value)
> +{
> + struct qnap_mcu_usb_led *usb_led = cdev_to_qnap_mcu_usb_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x43, 0 };
> +
> + /*
> + * If the led is off, turn it on. Otherwise don't disturb
"LED"
> + * a possible set blink-mode.
> + */
> + if (value == 0)
> + usb_led->mode = QNAP_MCU_USB_LED_OFF;
> + else if (usb_led->mode == QNAP_MCU_USB_LED_OFF)
> + usb_led->mode = QNAP_MCU_USB_LED_ON;
Same suggestions as above.
> + /* byte 3 is shared between the usb led target and setting the mode */
"Byte" for two reasons. Firstly because it's the correct capitalisation
of Byte and secondly because it's the start of a sentence.
"USB" and "LED" throughout please.
> + cmd[2] = 0x44 | usb_led->mode;
> +
> + return qnap_mcu_exec_with_ack(usb_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_usb_led_blink_set(struct led_classdev *led_cdev,
> + unsigned long *delay_on,
> + unsigned long *delay_off)
> +{
> + struct qnap_mcu_usb_led *usb_led = cdev_to_qnap_mcu_usb_led(led_cdev);
> + u8 cmd[] = { 0x40, 0x43, 0 };
> +
> + /* LED is off, nothing to do */
> + if (usb_led->mode == QNAP_MCU_USB_LED_OFF)
> + return 0;
> +
> + *delay_on = 250;
> + *delay_off = 250;
> + usb_led->mode = QNAP_MCU_USB_LED_BLINK;
> +
> + /* byte 3 is shared between the usb led target and setting the mode */
> + cmd[2] = 0x44 | usb_led->mode;
> +
> + return qnap_mcu_exec_with_ack(usb_led->mcu, cmd, sizeof(cmd));
> +}
> +
> +static int qnap_mcu_register_usb_led(struct device *dev, struct qnap_mcu *mcu)
> +{
> + struct qnap_mcu_usb_led *usb_led;
> + int ret;
> +
> + usb_led = devm_kzalloc(dev, sizeof(*usb_led), GFP_KERNEL);
> + if (!usb_led)
> + return -ENOMEM;
> +
> + usb_led->mcu = mcu;
> + usb_led->mode = QNAP_MCU_USB_LED_OFF;
> + usb_led->cdev.name = "usb:blue:disk";
> + usb_led->cdev.brightness_set_blocking = qnap_mcu_usb_led_set;
> + usb_led->cdev.blink_set = qnap_mcu_usb_led_blink_set;
> + usb_led->cdev.brightness = 0;
> + usb_led->cdev.max_brightness = 1;
> +
> + ret = devm_led_classdev_register(dev, &usb_led->cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to register usb led");
> +
> + return qnap_mcu_usb_led_set(&usb_led->cdev, 0);
> +}
> +
> +static int qnap_mcu_leds_probe(struct platform_device *pdev)
> +{
> + struct qnap_mcu *mcu = dev_get_drvdata(pdev->dev.parent);
> + const struct qnap_mcu_variant *variant = qnap_mcu_get_variant_data(mcu);
Grab this from platform_data.
> + int ret, i;
> +
> + for (i = 0; i < variant->num_drives; i++) {
You can use:
for (int i = 0; ..
> + ret = qnap_mcu_register_err_led(&pdev->dev, mcu, i);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register error led %d\n", i);
> + }
> +
> + if (variant->usb_led) {
> + ret = qnap_mcu_register_usb_led(&pdev->dev, mcu);
> + if (ret)
> + return dev_err_probe(&pdev->dev, ret,
> + "failed to register usb led %d\n", i);
The 'i' here looks like a copy/paste error.
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver qnap_mcu_leds_driver = {
> + .probe = qnap_mcu_leds_probe,
> + .driver = {
> + .name = "qnap-mcu-leds",
> + },
> +};
> +module_platform_driver(qnap_mcu_leds_driver);
> +
> +MODULE_ALIAS("platform:qnap-mcu-leds");
> +MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
> +MODULE_DESCRIPTION("QNAP MCU LEDs driver");
> +MODULE_LICENSE("GPL");
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH 01/18] Input: zforce_ts - use devm_add_action_or_reset()
From: Dmitry Torokhov @ 2024-08-29 17:54 UTC (permalink / raw)
To: Heiko Stübner
Cc: linux-input, Sudip Mukherjee, Andreas Kemnade, linux-kernel,
Sudip Mukherjee
In-Reply-To: <11576391.MucGe3eQFb@diego>
On Sat, Aug 24, 2024 at 01:13:28PM +0200, Heiko Stübner wrote:
> Am Samstag, 24. August 2024, 07:50:25 CEST schrieb Dmitry Torokhov:
> > From: Sudip Mukherjee <sudipm.mukherjee@gmail.com>
> >
> > If devm_add_action() fails we are explicitly calling the cleanup to free
> > the resources allocated. Lets use the helper devm_add_action_or_reset()
> > and return directly in case of error, as we know that the cleanup
> > function has been already called by the helper if there was any error.
> >
> > Signed-off-by: Sudip Mukherjee <sudip.mukherjee@codethink.co.uk>
>
> mistmatch between From and first Signed-off-by
Yeah, I got this patch a while ago...
>
> Other than that:
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Is this for this patch only or for the whole series?
>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/touchscreen/zforce_ts.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
> > index fdf2d1e770c8..ffbd55c6e1d4 100644
> > --- a/drivers/input/touchscreen/zforce_ts.c
> > +++ b/drivers/input/touchscreen/zforce_ts.c
> > @@ -803,15 +803,12 @@ static int zforce_probe(struct i2c_client *client)
> > udelay(100);
> > }
> >
> > - ret = devm_add_action(&client->dev, zforce_reset, ts);
> > + ret = devm_add_action_or_reset(&client->dev, zforce_reset, ts);
> > if (ret) {
> > dev_err(&client->dev, "failed to register reset action, %d\n",
> > ret);
> >
> > /* hereafter the regulator will be disabled by the action */
> > - if (!IS_ERR(ts->reg_vdd))
> > - regulator_disable(ts->reg_vdd);
> > -
> > return ret;
> > }
> >
> >
>
>
>
>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: alps - use guard notation when acquiring mutex
From: Dmitry Torokhov @ 2024-08-29 17:57 UTC (permalink / raw)
To: Pali Rohár; +Cc: linux-input, Erick Archer, linux-kernel
In-Reply-To: <20240825201347.pdphq33cmng4ltds@pali>
Hi Pali,
On Sun, Aug 25, 2024 at 10:13:47PM +0200, Pali Rohár wrote:
>
> Hello, I'm not familiar with new guards and their usage. But if this is
> a preferred way for handling mutexes then go ahead.
>
> I just looked at the code and I do not see any obvious error neither in
> old nor in new version.
Is this a Reviewed-by or Acked-by? If neither that is fine too, just
want to make sure.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 2/2] dt-bindings: touchscreen: ad7877: add bindings
From: Dmitry Torokhov @ 2024-08-29 18:07 UTC (permalink / raw)
To: Conor Dooley
Cc: Antoniu Miclaus, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michael Hennerich, linux-input, devicetree, linux-kernel
In-Reply-To: <20240829-mossy-dispense-bab38650455f@spud>
On Thu, Aug 29, 2024 at 05:16:30PM +0100, Conor Dooley wrote:
> On Thu, Aug 29, 2024 at 12:19:36PM +0300, Antoniu Miclaus wrote:
> > Add device tree bindings for the ad7877 driver.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> > .../input/touchscreen/adi,ad7877.yaml | 58 +++++++++++++++++++
> > 1 file changed, 58 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> > new file mode 100644
> > index 000000000000..5fc5124c5999
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/touchscreen/adi,ad7877.yaml
> > @@ -0,0 +1,58 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/touchscreen/adi,ad7877.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD7877 Touch Screen Controller
> > +
> > +maintainers:
> > + - Antoniu Miclaus <antoniu.miclaus@analog.com>
> > +
> > +description: |
> > + Analog Devices Touch Screen Controller
> > + https://www.analog.com/media/en/technical-documentation/data-sheets/AD7877.pdf
> > +
> > +allOf:
> > + - $ref: touchscreen.yaml#
> > + - $ref: /schemas/spi/spi-peripheral-props.yaml#
>
> > +
> > +unevaluatedProperties: false
>
> So, all of the properties in those two files are valid for this
> touchscreen controller?
No, the driver does not support transformations (swapping axes,
inverting, etc) but that is driver limitation, not property of the
hardware, which DT supposed to be, right?
Still I think we need to extend the driver to do that before adding DT
match tables and adding DT bindings (or maybe together with it).
The driver also need to have proper GPIO controller support instead of
having ad-hoc sysfs attributes, and converting in now before there are
mainline users would be a good time.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 1/2] dt-bindings: input: touchscreen: iqs7211: Use 'touchscreen' as node name
From: Fabio Estevam @ 2024-08-29 18:30 UTC (permalink / raw)
To: dmitry.torokhov
Cc: robh, krzk+dt, conor+dt, linux-input, devicetree, Fabio Estevam
From: Fabio Estevam <festevam@denx.de>
'touchscreen' is the recommended node name, not 'touch'.
Fix it accordingly.
Signed-off-by: Fabio Estevam <festevam@denx.de>
---
.../devicetree/bindings/input/touchscreen/azoteq,iqs7211.yaml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/touchscreen/azoteq,iqs7211.yaml b/Documentation/devicetree/bindings/input/touchscreen/azoteq,iqs7211.yaml
index 8cf371b99f19..e4dbbafb3779 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/azoteq,iqs7211.yaml
+++ b/Documentation/devicetree/bindings/input/touchscreen/azoteq,iqs7211.yaml
@@ -666,7 +666,7 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
- touch@56 {
+ touchscreen@56 {
compatible = "azoteq,iqs7210a";
reg = <0x56>;
irq-gpios = <&gpio 4 GPIO_ACTIVE_LOW>;
@@ -704,7 +704,7 @@ examples:
#address-cells = <1>;
#size-cells = <0>;
- touch@56 {
+ touchscreen@56 {
compatible = "azoteq,iqs7211e";
reg = <0x56>;
irq-gpios = <&gpio 4 (GPIO_ACTIVE_LOW |
--
2.34.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox