Re: [PATCH 0/3] ASoC/soundwire: remove ghost peripherals from the mach table

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

>>>>> A better way to only deal with actual codecs would be to only probe
>>>>> codec drivers when the codecs report as ATTACHED and get enumerated,
>>>>> instead of during the ACPI parsing stage.
>>>>
>>>> But this doesn't solve our issue. The DAI link will still be created
>>>> and the sound card will not probe because the codec driver doesn't probe.
>>>> Our target is that the ghost device should not be added in the DAI link.
>>>> So that the sound card can probe properly.
>>
>> I don't see how the DAI link would be created if there's no device
>> registered for the ghost device?
>> I guess we're talking about separate layers...
> 
> Sorry, I sould say the DAI link will be "registered".
> With commit ("ASoC: SOF: Intel: use sof_sdw as default SDW machine driver")
> We will create a mach table with the peripherals reported by ACPI if no
> matched item found from pdata->desc->alt_machines. And the machine driver
> will register the DAI links with all the reported peripherals. For example,
> a DAI link with rt711 will be registered even if it is physically
> non-existent.

ah ok, sorry I missed the point. Your commit message threw me off with
the statements "It will cause unexpected error like duplicated links,
codec driver can't probe, etc." The problem is really that the wrong
machine table entry is selected which leads to problems at the card
creation level.

>>>>> This is a solution that was discussed a ong time ago, probably circa
>>>>> 2016, during one of the LPC miniconferences, and the direction from
>>>>> maintainers was that the probe could be used to enable resources (power,
>>>>> gpio, clocks) that might be required for the hardware codec to become
>>>>> functional and report as ATTACHED. That's the reason why the probe is
>>>>> done on all codecs exposed in ACPI, even 'ghost' ones, with an
>>>>> update_status() callback to the codec driver when the presence of that
>>>>> codec is detected on the bus.
>>>>>
>>>>> In practice I am not aware of any codec drivers doing anything with
>>>>> power/gpio/clocks in the probe stages, at least for ACPI platforms, so
>>>>> it may be a good time to revisit this direction. SDCA class drivers do
>>>>> exactly what I described, the subdevices are registered only upon
>>>>> enumeration, not during ACPI parsing. It's a much simpler design with a
>>>>> lot fewer potential races.
>>>>>
>>>>> Problems:
>>>>> - this would be a very invasive change to sdw_slave_add(), with the
>>>>> device_register() skipped and moved to the enumeration stage. It'd have
>>>>> to be opt-in and used only a newer platforms to avoid breaking the
>>>>> 'legacy' devices.
>>>>
>>>> I didn't change sdw_slave_add(). All the change in SoundWire driver is to
>>>> add a flag and a completion to share the information of whether the
>>>> enumeration of the bus is completed.
>>
>> I still think this option would be best, that way you'd only deal with
>> devices that report as ATTACHED, without needing any additional
>> wait_for_completion.
> 
> With this option, the codec driver will not probe and lead to the sound
> card doesn't probe. My goal is that the machine driver will only register
> the DAI links with the peripherals that are physically existent. So that
> the sound card will probe and audio will work. This is also beneficial to
> the case that a peripheral is broken. The other endpoints will still work
> in that case.

Right, in the case where the machine is already selected and *expects* a
device to report as attached, it doesn't matter if the codec driver
probes only on attachment.

>>> Do you still have any concerns about this series?
>>> Or maybe I can add a module parameter to disable the feature?
>>
>> The main objection I have is this piece of code:
>>
>> if (ret == -ENODATA) { /* end of device id reads */
>>  			dev_dbg(bus->dev, "No more devices to
>> enumerate\n");
>>  			ret = 0;
>> +			complete_all(&bus->enumeration_complete);
>>  			break;
>>  		}
>>
>> This assumes that ALL peripherals report as ATTACHED at the same time.
>> If for some reason a peripheral attaches later, then the entire logic
>> would be broken - or you will have to add a large-enough wait time
>> before trying to enumerate devices. That's different to the timeout for
>> the wait_for_completion, what I am referring to is a delay to let all
>> devices show-up as ATTACHED after the bus start.
> 
> Not exactly the same time, but when the bus starts and do the enumeration
> process, aren't all peripherals enumerated at that time? In other words,
> doesn't it mean all existing peripherals are enumerated when there is no
> peripheral with device numver 0?
> If for some reason a peripheral will not show up at the first bus start,
> we can add a quirk to skip the peripheral and assume it is always existent.

Well, you've got two problems here.

a) there's a difference between generations. Starting with ACE2.0, the
SoundWire bus is started much earlier than in previous generations,
where the SoundWire bus was started after firmware download.

In other words, the timing to sync and report as attached differs by
orders of magnitude. I am worried that this code might break stuff on
older platforms.

b) there's nothing in the MIPI spec that says how much time a device
should take to report as ATTACHED. A well-intended implementation
following Recommendation {2121} would e.g. try to find the sync word
with a default frame shape of 48x2, which isn't used at all by Intel.

If you want to keep the code above, you absolutely need to record the
time at which the bus started and wait TBD ms before signaling
enumeration_complete(). You would also need to deal with a case where
another device shows up with a delay, would you throw another
complete_all() then?