| Message ID | 20211011184715.61573-1-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/3] ASoc: amd: create platform device for VG machine driver | expand |
On 10/11/21 11:19 PM, Mark Brown wrote: > On Tue, Oct 12, 2021 at 12:17:01AM +0530, Vijendar Mukunda wrote: > >> pdevinfo[2].res = &adata->res[2]; >> >> + pdevinfo[3].name = "acp5x_nu8821_cs35l41_mach"; >> + pdevinfo[3].id = 0; >> + pdevinfo[3].parent = &pci->dev; > > This appears to unconditionally assume that any machine with this > hardware is going to have exactly the same CODEC/amp combination - that > seems like it's going to go badly at some point. Shouldn't there be > some other check that we're instantiating the right machine driver? > we will make the platform device as generic one something like "acp5x_mach". Currently we have only one target platform with above codec combinations for this APU series. When we get new codecs combinations to support, we will address it in machine driver by adding DMI checks for distinguishing codec combinations. > BTW your clock appears to be set quite a way into the future (possibly > about 24 hours?) which is confusing things. > Will fix the system time settings.
On 10/11/21 11:59 PM, Mark Brown wrote: > On Mon, Oct 11, 2021 at 11:48:40AM +0530, Mukunda,Vijendar wrote: >> On 10/11/21 11:19 PM, Mark Brown wrote: > >>>> + pdevinfo[3].name = "acp5x_nu8821_cs35l41_mach"; > >>> This appears to unconditionally assume that any machine with this >>> hardware is going to have exactly the same CODEC/amp combination - that >>> seems like it's going to go badly at some point. Shouldn't there be >>> some other check that we're instantiating the right machine driver? > >> we will make the platform device as generic one something like "acp5x_mach". > >> Currently we have only one target platform with above codec combinations >> for this APU series. > >> When we get new codecs combinations to support, we will address it in >> machine driver by adding DMI checks for distinguishing codec combinations. > > If that's the case then this machine driver that's being instantiated is > poorly named, and I expect you're going to get issues with assuming a > default here and trying to instantiate this machine on unsuitable > hardware. It'd be better to at least put a bit of the framework in now > and positively indentify systems that can run this machine driver. Will address it by adding DMI checks in machine driver code. > > It really would be good if ACPI system vendors were to adopt a more > standards based approach to platform enumeration, this really isn't > good. Something more standards based like DT has would be much more > scalable. >
On Tue, Oct 12, 2021 at 12:17:01AM +0530, Vijendar Mukunda wrote: > pdevinfo[2].res = &adata->res[2]; > > + pdevinfo[3].name = "acp5x_nu8821_cs35l41_mach"; > + pdevinfo[3].id = 0; > + pdevinfo[3].parent = &pci->dev; This appears to unconditionally assume that any machine with this hardware is going to have exactly the same CODEC/amp combination - that seems like it's going to go badly at some point. Shouldn't there be some other check that we're instantiating the right machine driver? BTW your clock appears to be set quite a way into the future (possibly about 24 hours?) which is confusing things.
On Mon, Oct 11, 2021 at 11:48:40AM +0530, Mukunda,Vijendar wrote: > On 10/11/21 11:19 PM, Mark Brown wrote: > >> + pdevinfo[3].name = "acp5x_nu8821_cs35l41_mach"; > > This appears to unconditionally assume that any machine with this > > hardware is going to have exactly the same CODEC/amp combination - that > > seems like it's going to go badly at some point. Shouldn't there be > > some other check that we're instantiating the right machine driver? > we will make the platform device as generic one something like "acp5x_mach". > Currently we have only one target platform with above codec combinations > for this APU series. > When we get new codecs combinations to support, we will address it in > machine driver by adding DMI checks for distinguishing codec combinations. If that's the case then this machine driver that's being instantiated is poorly named, and I expect you're going to get issues with assuming a default here and trying to instantiate this machine on unsuitable hardware. It'd be better to at least put a bit of the framework in now and positively indentify systems that can run this machine driver. It really would be good if ACPI system vendors were to adopt a more standards based approach to platform enumeration, this really isn't good. Something more standards based like DT has would be much more scalable.
diff --git a/sound/soc/amd/vangogh/acp5x.h b/sound/soc/amd/vangogh/acp5x.h index a808635f9740..fe5e1fa98974 100644 --- a/sound/soc/amd/vangogh/acp5x.h +++ b/sound/soc/amd/vangogh/acp5x.h @@ -23,7 +23,7 @@ #define ACP_ERR_INTR_MASK 0x20000000 #define ACP_EXT_INTR_STAT_CLEAR_MASK 0xFFFFFFFF -#define ACP5x_DEVS 3 +#define ACP5x_DEVS 4 #define ACP5x_REG_START 0x1240000 #define ACP5x_REG_END 0x1250200 #define ACP5x_I2STDM_REG_START 0x1242400 diff --git a/sound/soc/amd/vangogh/pci-acp5x.c b/sound/soc/amd/vangogh/pci-acp5x.c index a57b762d9f2e..137e64c47882 100644 --- a/sound/soc/amd/vangogh/pci-acp5x.c +++ b/sound/soc/amd/vangogh/pci-acp5x.c @@ -213,6 +213,9 @@ static int snd_acp5x_probe(struct pci_dev *pci, pdevinfo[2].num_res = 1; pdevinfo[2].res = &adata->res[2]; + pdevinfo[3].name = "acp5x_nu8821_cs35l41_mach"; + pdevinfo[3].id = 0; + pdevinfo[3].parent = &pci->dev; for (i = 0; i < ACP5x_DEVS; i++) { adata->pdev[i] = platform_device_register_full(&pdevinfo[i]);
Create platform device for Vangogh machine driver. Signed-off-by: Vijendar Mukunda <Vijendar.Mukunda@amd.com> --- sound/soc/amd/vangogh/acp5x.h | 2 +- sound/soc/amd/vangogh/pci-acp5x.c | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-)