On Mon, Oct 30, 2023 at 04:05:09PM -0500, Pierre-Louis Bossart wrote:
> On 10/30/23 12:40, 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.
> >> 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
That's a huge range... I think the concern is more the standard ranges
than the reserved ranges isn't it? That's the bit that the SoundWire
core or other generic code will be actively managing.
> 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?
That would work for drivers that use that, but note that drivers can
just provide a function here so you pretty much need to probe each
individual register. It's done that way because we figured when the
interface was originally defined that the compiler would probably
generate better code from a switch statement in most cases than us
trying to fine tune an algorithm. Probing like that is going to be
unsustaniable for the full ranges above but shouldn't be too bad for the
potentially standard bits, we could guard it with a config option if
it's noticable.
Another option would be a kselftest that looks at the readable registers
in debugfs, we do build up a cache of the readable ranges there when the
access map or register contents are first read. That's potentially less
visible but OTOH easier to ask for on review, I was debating asking for
mixer-test output on all CODEC driver submissions (similar to what the
media subsystem does with their validation suite).
Attachment:
signature.asc
Description: PGP signature
