Re: [RESEND PATCH v2] ALSA: hda/tas2781: Create a common header for both spi and i2c tas2781 hda driver

From: Takashi Iwai
Date: Tue Apr 15 2025 - 11:18:32 EST

Next message: Liang, Kan: "Re: [Patch v3 00/22] Arch-PEBS and PMU supports for Clearwater Forest and Panther Lake"
Previous message: Miguel Ojeda: "Re: [PATCH v4 1/5] rust: types: add `Opaque::zeroed`"
In reply to: Shenghao Ding: "[RESEND PATCH v2] ALSA: hda/tas2781: Create a common header for both spi and i2c tas2781 hda driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 15 Apr 2025 09:09:13 +0200,
Shenghao Ding wrote:
>
> Move the common macro definition of kcontrols into a common header.
>
> Signed-off-by: Shenghao Ding <shenghao-ding@xxxxxx>
>
> ---
> v2:
> - Follow IWYU principle, add sound/asound.h into the header file.
> v1:
> - Revise the year of spi and i2c tas2781 hda drivers.
> - Create a common header for both spi and i2c tas2781 hda driver to define
> the common macros and declare the common functions.

The code change itself looks fine, but do you need this change for
what purpose? Is it meant as a code cleanup? You wrote what the
patch does, but why it's needed isn't mentioned at all.

thanks,

Takashi

> ---
> sound/pci/hda/tas2781_hda.h | 44 +++++++++++++++++++++++++++++++++
> sound/pci/hda/tas2781_hda_i2c.c | 29 ++--------------------
> sound/pci/hda/tas2781_hda_spi.c | 35 ++------------------------
> 3 files changed, 48 insertions(+), 60 deletions(-)
> create mode 100644 sound/pci/hda/tas2781_hda.h
>
> diff --git a/sound/pci/hda/tas2781_hda.h b/sound/pci/hda/tas2781_hda.h
> new file mode 100644
> index 000000000000..fc741fac419a
> --- /dev/null
> +++ b/sound/pci/hda/tas2781_hda.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-only
> + *
> + * HDA audio driver for Texas Instruments TAS2781 smart amp
> + *
> + * Copyright (C) 2025 Texas Instruments, Inc.
> + */
> +#ifndef __TAS2781_HDA_H__
> +#define __TAS2781_HDA_H__
> +
> +#include <sound/asound.h>
> +
> +/*
> + * No standard control callbacks for SNDRV_CTL_ELEM_IFACE_CARD
> + * Define two controls, one is Volume control callbacks, the other is
> + * flag setting control callbacks.
> + */
> +
> +/* Volume control callbacks for tas2781 */
> +#define ACARD_SINGLE_RANGE_EXT_TLV(xname, xreg, xshift, xmin, xmax, xinvert, \
> + xhandler_get, xhandler_put, tlv_array) { \
> + .iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = (xname), \
> + .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
> + SNDRV_CTL_ELEM_ACCESS_READWRITE, \
> + .tlv.p = (tlv_array), \
> + .info = snd_soc_info_volsw, \
> + .get = xhandler_get, .put = xhandler_put, \
> + .private_value = (unsigned long)&(struct soc_mixer_control) { \
> + .reg = xreg, .rreg = xreg, \
> + .shift = xshift, .rshift = xshift,\
> + .min = xmin, .max = xmax, .invert = xinvert, \
> + } \
> +}
> +
> +/* Flag control callbacks for tas2781 */
> +#define ACARD_SINGLE_BOOL_EXT(xname, xdata, xhandler_get, xhandler_put) { \
> + .iface = SNDRV_CTL_ELEM_IFACE_CARD, \
> + .name = xname, \
> + .info = snd_ctl_boolean_mono_info, \
> + .get = xhandler_get, \
> + .put = xhandler_put, \
> + .private_value = xdata, \
> +}
> +
> +#endif
> diff --git a/sound/pci/hda/tas2781_hda_i2c.c b/sound/pci/hda/tas2781_hda_i2c.c
> index 29dc4f500580..9d94ae5fcfe0 100644
> --- a/sound/pci/hda/tas2781_hda_i2c.c
> +++ b/sound/pci/hda/tas2781_hda_i2c.c
> @@ -2,7 +2,7 @@
> //
> // TAS2781 HDA I2C driver
> //
> -// Copyright 2023 - 2024 Texas Instruments, Inc.
> +// Copyright 2023 - 2025 Texas Instruments, Inc.
> //
> // Author: Shenghao Ding <shenghao-ding@xxxxxx>
> // Current maintainer: Baojun Xu <baojun.xu@xxxxxx>
> @@ -30,35 +30,10 @@
> #include "hda_component.h"
> #include "hda_jack.h"
> #include "hda_generic.h"
> +#include "tas2781_hda.h"
>
> #define TASDEVICE_SPEAKER_CALIBRATION_SIZE 20
>
> -/* No standard control callbacks for SNDRV_CTL_ELEM_IFACE_CARD
> - * Define two controls, one is Volume control callbacks, the other is
> - * flag setting control callbacks.
> - */
> -
> -/* Volume control callbacks for tas2781 */
> -#define ACARD_SINGLE_RANGE_EXT_TLV(xname, xreg, xshift, xmin, xmax, xinvert, \
> - xhandler_get, xhandler_put, tlv_array) \
> -{ .iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = (xname),\
> - .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ |\
> - SNDRV_CTL_ELEM_ACCESS_READWRITE,\
> - .tlv.p = (tlv_array), \
> - .info = snd_soc_info_volsw, \
> - .get = xhandler_get, .put = xhandler_put, \
> - .private_value = (unsigned long)&(struct soc_mixer_control) \
> - {.reg = xreg, .rreg = xreg, .shift = xshift, \
> - .rshift = xshift, .min = xmin, .max = xmax, \
> - .invert = xinvert} }
> -
> -/* Flag control callbacks for tas2781 */
> -#define ACARD_SINGLE_BOOL_EXT(xname, xdata, xhandler_get, xhandler_put) \
> -{ .iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = xname, \
> - .info = snd_ctl_boolean_mono_info, \
> - .get = xhandler_get, .put = xhandler_put, \
> - .private_value = xdata }
> -
> enum calib_data {
> R0_VAL = 0,
> INV_R0,
> diff --git a/sound/pci/hda/tas2781_hda_spi.c b/sound/pci/hda/tas2781_hda_spi.c
> index 399f2e4c3b62..c6be2be1b53e 100644
> --- a/sound/pci/hda/tas2781_hda_spi.c
> +++ b/sound/pci/hda/tas2781_hda_spi.c
> @@ -2,7 +2,7 @@
> //
> // TAS2781 HDA SPI driver
> //
> -// Copyright 2024 Texas Instruments, Inc.
> +// Copyright 2024 - 2025 Texas Instruments, Inc.
> //
> // Author: Baojun Xu <baojun.xu@xxxxxx>
>
> @@ -38,38 +38,7 @@
> #include "hda_component.h"
> #include "hda_jack.h"
> #include "hda_generic.h"
> -
> -/*
> - * No standard control callbacks for SNDRV_CTL_ELEM_IFACE_CARD
> - * Define two controls, one is Volume control callbacks, the other is
> - * flag setting control callbacks.
> - */
> -
> -/* Volume control callbacks for tas2781 */
> -#define ACARD_SINGLE_RANGE_EXT_TLV(xname, xreg, xshift, xmin, xmax, xinvert, \
> - xhandler_get, xhandler_put, tlv_array) { \
> - .iface = SNDRV_CTL_ELEM_IFACE_CARD, .name = (xname), \
> - .access = SNDRV_CTL_ELEM_ACCESS_TLV_READ | \
> - SNDRV_CTL_ELEM_ACCESS_READWRITE, \
> - .tlv.p = (tlv_array), \
> - .info = snd_soc_info_volsw, \
> - .get = xhandler_get, .put = xhandler_put, \
> - .private_value = (unsigned long)&(struct soc_mixer_control) { \
> - .reg = xreg, .rreg = xreg, \
> - .shift = xshift, .rshift = xshift,\
> - .min = xmin, .max = xmax, .invert = xinvert, \
> - } \
> -}
> -
> -/* Flag control callbacks for tas2781 */
> -#define ACARD_SINGLE_BOOL_EXT(xname, xdata, xhandler_get, xhandler_put) { \
> - .iface = SNDRV_CTL_ELEM_IFACE_CARD, \
> - .name = xname, \
> - .info = snd_ctl_boolean_mono_info, \
> - .get = xhandler_get, \
> - .put = xhandler_put, \
> - .private_value = xdata, \
> -}
> +#include "tas2781_hda.h"
>
> struct tas2781_hda {
> struct tasdevice_priv *priv;
> --
> 2.34.1
>

Next message: Liang, Kan: "Re: [Patch v3 00/22] Arch-PEBS and PMU supports for Clearwater Forest and Panther Lake"
Previous message: Miguel Ojeda: "Re: [PATCH v4 1/5] rust: types: add `Opaque::zeroed`"
In reply to: Shenghao Ding: "[RESEND PATCH v2] ALSA: hda/tas2781: Create a common header for both spi and i2c tas2781 hda driver"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]