RE: [EXTERNAL] Re: [PATCH v1 2/2] ASoC: tas2781: Add tas5832 support

[Ding, Shenghao]

> >>>
> >>>>>>>>>>> static const struct of_device_id tasdevice_of_match[] = {
> >>>
> >>>>>>>>>>>       { .compatible = "ti,tas5827" },
> >>>>>>>>>>>       { .compatible = "ti,tas5828" },
> >>>>>>>>>>>       { .compatible = "ti,tas5830" },
> >>>>>>>>>>> +     { .compatible = "ti,tas5832" },
> >>>>>>>>>>
> > ...
> >>>
> >>> I don't know what you are trying to get from them, but I²C
> >>> enumeration on DT platforms works in a way that it takes two tables
> >>> into account, hence, if there is no compatible (with given part
> >>> number) there will be no matching name.
> >>>
> >>> AFAIK it has to have a compatible to make it work in such a case.
> >>> What did I miss?
> >>
> >> Show me please then how the OF table is used here at all to get the name.
> >
> > This code will also be used for ACPI devices. To ensure consistent bin
> > file naming between ACPI and DTS devices, the mapping table
> > tasdevice_id[] has been introduced.
> 
> We need to stop discussing with irrelevant arguments, really.
> 
> I asked to show me how this is going to be used in DT. No answer to this part.
> 
> Now you claim this OF ID will be used for ACPI devices, but YOU HAVE already
> ACPI table there, so that's clearly invalid argument. Do you understand how
> patch/email review works? Where the comments appear and what is being
> discussed?
> 
> > The code retrieves the corresponding name of the chip from
> > tasdevice_id[] based on its order in tasdevice_of_match[] or
> tasdevice_acpi_match[].
> 
> Based on the order in table? No, that's crazy buggy solution to tie order of
> entries in both tables. And it makes no sense... and I am sure code does not
> do it, so again irrelevant argument.

It should be noted that the current implementation relies on id_table 
(rather than OF_ID) to obtain the chip name and chip ID. The chip ID i
s specifically reserved for handling exceptional cases, for example, 
the workaround for the TAS2781’s lower speaker impedance issue.

Since the 58xx family lacks a dedicated register to query the chip ID, 
both the supported chip ID and name are stored in id_table.
 This approach allows compatibility with both DT-based (Device Tree) 
and ACPI-based (Advanced Configuration and Power Interface) device.

	if (ACPI_HANDLE(&i2c->dev)) {
		acpi_id = acpi_match_device(i2c->dev.driver->acpi_match_table,
				&i2c->dev);
		if (!acpi_id) {
			dev_err(&i2c->dev, "No driver data\n");
			ret = -EINVAL;
			goto err;
		}
		tas_priv->chip_id = acpi_id->driver_data;
		tas_priv->isacpi = true;
	} else {
		tas_priv->chip_id = (uintptr_t)i2c_get_match_data(i2c);
		tas_priv->isacpi = false;
	}


> 
> NAK, because all the replies so far are off-topic.
> 
> Best regards,
> Krzysztof