Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 8 Jun 2023 12:36:10 +0530
From:   Vinod Koul <vkoul@kernel.org>
To:     Bard Liao <yung-chuan.liao@linux.intel.com>
Cc:     alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        pierre-louis.bossart@linux.intel.com, bard.liao@intel.com
Subject: Re: [PATCH 2/4] soundwire: introduce SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY
Message-ID: <ZIF94vZHzeGXfyin@matsya>
References: <20230531033736.792464-1-yung-chuan.liao@linux.intel.com>
 <20230531033736.792464-3-yung-chuan.liao@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230531033736.792464-3-yung-chuan.liao@linux.intel.com>
Precedence: bulk

On 31-05-23, 11:37, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> This patch adds a new Device Number allocation strategy, with the IDA
> used only for devices that are wake-capable.
> 
> "regular" devices such as amplifiers will use Device Numbers
> [1..min_ida-1].
> 
> "wake-capable" devices such as jack or microphone codecs will use
> Device Numbers [min_ida..11].
> 
> This hybrid strategy extends the number of supported devices in a
> system by only constraining the allocation if required, e.g. in the
> case of Intel LunarLake platforms the wake-capable devices are
> required to have a unique address to use the HDaudio SDI and HDAudio
> WAKEEN/WAKESTS registers.

This seems to be a consequence of Intel hardware decisions, so I guess
best suited place for this is Intel controller, do we really want to
have this in core logic?

> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/bus.c       | 26 +++++++++++++++++++++-----
>  include/linux/soundwire/sdw.h |  4 ++++
>  2 files changed, 25 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
> index e8c1c55a2a73..6f465cce8369 100644
> --- a/drivers/soundwire/bus.c
> +++ b/drivers/soundwire/bus.c
> @@ -159,7 +159,9 @@ static int sdw_delete_slave(struct device *dev, void *data)
>  
>  	if (slave->dev_num) { /* clear dev_num if assigned */
>  		clear_bit(slave->dev_num, bus->assigned);
> -		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA)
> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
> +		    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
> +		     slave->prop.wake_capable))
>  			ida_free(&sdw_peripheral_ida, slave->dev_num);
>  	}
>  	list_del_init(&slave->node);
> @@ -699,17 +701,31 @@ EXPORT_SYMBOL(sdw_compare_devid);
>  /* called with bus_lock held */
>  static int sdw_get_device_num(struct sdw_slave *slave)
>  {
> +	struct sdw_bus *bus = slave->bus;
>  	int bit;
>  
> -	if (slave->bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA) {
> +	if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA ||
> +	    (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
> +	     slave->prop.wake_capable)) {
>  		bit = ida_alloc_range(&sdw_peripheral_ida,
> -				      slave->bus->dev_num_ida_min, SDW_MAX_DEVICES,
> +				      bus->dev_num_ida_min, SDW_MAX_DEVICES,
>  				      GFP_KERNEL);
>  		if (bit < 0)
>  			goto err;
>  	} else {
> -		bit = find_first_zero_bit(slave->bus->assigned, SDW_MAX_DEVICES);
> -		if (bit == SDW_MAX_DEVICES) {
> +		int max_devices = SDW_MAX_DEVICES;
> +
> +		if (bus->dev_num_alloc == SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY &&
> +		    !slave->prop.wake_capable) {
> +			max_devices = bus->dev_num_ida_min - 1;
> +
> +			/* range check */
> +			if (max_devices < 1 || max_devices > SDW_MAX_DEVICES)
> +				return -EINVAL;
> +		}
> +
> +		bit = find_first_zero_bit(bus->assigned, max_devices);
> +		if (bit == max_devices) {
>  			bit = -ENODEV;
>  			goto err;
>  		}
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 4656d6d0f3bb..8a7541ac735e 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -869,10 +869,14 @@ struct sdw_master_ops {
>   * @SDW_DEV_NUM_ALLOC_DEFAULT: unconstrained first-come-first-serve allocation,
>   * using range [1, 11]
>   * @SDW_DEV_NUM_ALLOC_IDA: IDA-based allocation, using range [ida_min, 11]
> + * @SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY: Hybrid allocation where wake-capable devices rely on
> + * IDA-based allocation and range [ida_min, 11], while regular devices rely on default
> + * allocation in range [1, ida_min - 1]
>   */
>  enum sdw_dev_num_alloc {
>  	SDW_DEV_NUM_ALLOC_DEFAULT = 0,
>  	SDW_DEV_NUM_ALLOC_IDA,
> +	SDW_DEV_NUM_ALLOC_IDA_WAKE_ONLY,
>  };
>  
>  /**
> -- 
> 2.25.1

-- 
~Vinod