Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Message-ID: <85890ed8-0ab3-4b14-879b-be9f0c760db8@linux.intel.com>
Date:   Mon, 30 Oct 2023 16:05:09 -0500
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] ASoC: tas2783: Add source files for tas2783 driver.
Content-Language: en-US
From:   Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
To:     Mark Brown <broonie@kernel.org>
Cc:     Baojun Xu <baojun.xu@ti.com>, lgirdwood@gmail.com, perex@perex.cz,
        alsa-devel@alsa-project.org, linux-kernel@vger.kernel.org,
        kevin-lu@ti.com, shenghao-ding@ti.com, peeyush@ti.com,
        navada@ti.com, tiwai@suse.de
References: <20231028092409.96813-1-baojun.xu@ti.com>
 <7f4465c1-5e8e-4c5f-bbff-d2c930326986@linux.intel.com>
 <9d922584-288a-4b73-83ef-477d1bc58521@sirena.org.uk>
 <9b94488e-206d-419b-92ed-e70aab11907b@linux.intel.com>
In-Reply-To: <9b94488e-206d-419b-92ed-e70aab11907b@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


On 10/30/23 12:40, Pierre-Louis Bossart wrote:
> 
> 
> On 10/30/23 12:20, Mark Brown wrote:
>> On Mon, Oct 30, 2023 at 11:05:39AM -0500, Pierre-Louis Bossart wrote:
>>
>>>> +static bool tas2783_readable_register(struct device *dev, unsigned int reg)
>>>> +{
>>>> +	switch (reg) {
>>>> +	case 0x000 ... 0x080:	/* Data port 0. */
>>
>>> No, this is wrong. All the data port 'standard' registers are "owned" by
>>> the SoundWire core and handled during the port prepare/configure/bank
>>> switch routines. Do not use them for regmap.
>>
>>> In other words, you *shall* only define vendor-specific registers in
>>> this codec driver.
>>
>> This seems to come up a moderate amount and is an understandable thing
>> to do - could you (or someone else who knows SoundWire) perhaps send a
>> patch for the regmap SoundWire integration which does some validation
>> here during registration and at least prints a warning?
> 
> Good suggestion, we could indeed check that the registers are NOT in the
> range [0,0xBF] for all ports - only the range [0xC0..FF] is allowed for
> implementation-defined values. I'll try to cook something up.

After checking, the following ranges are invalid for codec drivers:

for address < 0x1000
LSB = 0x00 - 0xBF standard or reserved

0x1800 – 0x1FFF reserved
0x48000000 - 0xFFFFFFFF reserved

is the recommendation to check the regmap_config and its 'yes_ranges'?

Presumably if the range_min or range_max is within the invalid values
above then the configuration can be tagged as problematic in the dmesg
log or rejected with an error code?