| Message ID | 1569891524-18875-2-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] ASoC: amd: No need PCI-MSI interrupts | expand |
On Tue, 01 Oct 2019, Ravulapati Vishnu vardhan rao wrote: > Removed platform based endpoint registering in ACP-PCI driver. > Now Registering PCM DMA and multiple I2S instances: SP and BT endpoint > devices automatically by using MFD framework. This is a hack. Why are you using the MFD framework outside of drivers/mfd? If this driver is an MFD, then please create an MFD driver. If it's not, please do not use the MFD API. > Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> > --- > sound/soc/amd/raven/acp3x.h | 8 +++ > sound/soc/amd/raven/pci-acp3x.c | 123 ++++++++++++++++++++++++++-------------- > 2 files changed, 90 insertions(+), 41 deletions(-) > > diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h > index 4f2cadd..c122dc6 100644 > --- a/sound/soc/amd/raven/acp3x.h > +++ b/sound/soc/amd/raven/acp3x.h > @@ -7,13 +7,21 @@ > > #include "chip_offset_byte.h" > > +#define ACP3x_DEVS 3 > #define ACP3x_PHY_BASE_ADDRESS 0x1240000 > #define ACP3x_I2S_MODE 0 > #define ACP3x_REG_START 0x1240000 > #define ACP3x_REG_END 0x1250200 > +#define ACP3x_I2STDM_REG_START 0x1242400 > +#define ACP3x_I2STDM_REG_END 0x1242410 > +#define ACP3x_BT_TDM_REG_START 0x1242800 > +#define ACP3x_BT_TDM_REG_END 0x1242810 > #define I2S_MODE 0x04 > +#define I2S_RX_THRESHOLD 27 > +#define I2S_TX_THRESHOLD 28 > #define BT_TX_THRESHOLD 26 > #define BT_RX_THRESHOLD 25 > +#define ACP_ERR_INTR_MASK 29 > #define ACP3x_POWER_ON 0x00 > #define ACP3x_POWER_ON_IN_PROGRESS 0x01 > #define ACP3x_POWER_OFF 0x02 > diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c > index 8f6bf00..d9f5bc0 100644 > --- a/sound/soc/amd/raven/pci-acp3x.c > +++ b/sound/soc/amd/raven/pci-acp3x.c > @@ -9,13 +9,21 @@ > #include <linux/io.h> > #include <linux/platform_device.h> > #include <linux/interrupt.h> > +#include <linux/mfd/core.h> > > #include "acp3x.h" > > +struct i2s_platform_data { > + unsigned int cap; > + int channel; > + u32 snd_rates; > +}; > struct acp3x_dev_data { > + struct device *parent; > + struct mfd_cell *cell; > + struct resource *res; > void __iomem *acp3x_base; > bool acp3x_audio_mode; > - struct resource *res; > struct platform_device *pdev; > }; > > @@ -23,9 +31,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, > const struct pci_device_id *pci_id) > { > int ret; > - u32 addr, val; > + resource_size_t addr; > + int val, i, r; > struct acp3x_dev_data *adata; > - struct platform_device_info pdevinfo; > + struct device *dev; > + struct i2s_platform_data *i2s_pdata; > unsigned int irqflags; > > if (pci_enable_device(pci)) { > @@ -56,55 +66,87 @@ static int snd_acp3x_probe(struct pci_dev *pci, > } > pci_set_master(pci); > pci_set_drvdata(pci, adata); > - > + adata->parent = &pci->dev; > val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); > switch (val) { > case I2S_MODE: > adata->res = devm_kzalloc(&pci->dev, > - sizeof(struct resource) * 2, > - GFP_KERNEL); > - if (!adata->res) { > + sizeof(struct resource) * 4, > + GFP_KERNEL); > + adata->cell = devm_kzalloc(&pci->dev, > + sizeof(struct mfd_cell) * ACP3x_DEVS, > + GFP_KERNEL); > + if (!adata->cell) { > ret = -ENOMEM; > goto unmap_mmio; > } > > - adata->res[0].name = "acp3x_i2s_iomem"; > - adata->res[0].flags = IORESOURCE_MEM; > - adata->res[0].start = addr; > - adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START); > - > - adata->res[1].name = "acp3x_i2s_irq"; > - adata->res[1].flags = IORESOURCE_IRQ; > - adata->res[1].start = pci->irq; > - adata->res[1].end = pci->irq; > - > - adata->acp3x_audio_mode = ACP3x_I2S_MODE; > - > - memset(&pdevinfo, 0, sizeof(pdevinfo)); > - pdevinfo.name = "acp3x_rv_i2s"; > - pdevinfo.id = 0; > - pdevinfo.parent = &pci->dev; > - pdevinfo.num_res = 2; > - pdevinfo.res = adata->res; > - pdevinfo.data = &irqflags; > - pdevinfo.size_data = sizeof(irqflags); > - > - adata->pdev = platform_device_register_full(&pdevinfo); > - if (IS_ERR(adata->pdev)) { > - dev_err(&pci->dev, "cannot register %s device\n", > - pdevinfo.name); > - ret = PTR_ERR(adata->pdev); > - goto unmap_mmio; > + i2s_pdata = devm_kzalloc(&pci->dev, > + sizeof(struct i2s_platform_data) * ACP3x_DEVS, > + GFP_KERNEL); > + if (i2s_pdata == NULL) { > + kfree(adata->res); > + kfree(adata->cell); > + return -ENOMEM; > } > + adata->res[0].name = "acp3x_i2s_iomem"; > + adata->res[0].flags = IORESOURCE_MEM; > + adata->res[0].start = addr; > + adata->res[0].end = addr + > + (ACP3x_REG_END - ACP3x_REG_START); > + i2s_pdata[0].cap = 0; > + i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000; > + > + adata->res[1].name = "acp3x_i2s_sp_play_cap"; > + adata->res[1].flags = IORESOURCE_MEM; > + adata->res[1].start = addr + ACP3x_I2STDM_REG_START; > + adata->res[1].end = addr + ACP3x_I2STDM_REG_END; > + i2s_pdata[1].cap = 0; > + i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000; > + > + adata->res[2].name = "acp3x_i2s_bt_play_cap"; > + adata->res[2].flags = IORESOURCE_MEM; > + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START; > + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END; > + i2s_pdata[2].cap = 0; > + i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000; > + > + adata->res[3].name = "acp3x_i2s_irq"; > + adata->res[3].flags = IORESOURCE_IRQ; > + adata->res[3].start = pci->irq; > + adata->res[3].end = adata->res[3].start; > + > + adata->acp3x_audio_mode = ACP3x_I2S_MODE; > + > + adata->cell[0].name = "acp3x_rv_i2s_dma"; > + adata->cell[0].num_resources = 4; > + adata->cell[0].resources = &adata->res[0]; > + adata->cell[0].platform_data = &irqflags; > + adata->cell[0].pdata_size = sizeof(irqflags); > + > + adata->cell[1].name = "acp3x_i2s_playcap"; > + adata->cell[1].num_resources = 1; > + adata->cell[1].resources = &adata->res[1]; > + adata->cell[1].platform_data = &i2s_pdata[0]; > + adata->cell[1].pdata_size = > + sizeof(struct i2s_platform_data); > + > + adata->cell[2].name = "acp3x_i2s_playcap"; > + adata->cell[2].num_resources = 1; > + adata->cell[2].resources = &adata->res[2]; > + adata->cell[2].platform_data = &i2s_pdata[1]; > + adata->cell[2].pdata_size = > + sizeof(struct i2s_platform_data); > + r = mfd_add_hotplug_devices(adata->parent, > + adata->cell, ACP3x_DEVS); > break; > - default: > - dev_err(&pci->dev, "Invalid ACP audio mode : %d\n", val); > - ret = -ENODEV; > - goto unmap_mmio; > } > return 0; > > unmap_mmio: > + mfd_remove_devices(adata->parent); > + kfree(adata->res); > + kfree(adata->cell); > iounmap(adata->acp3x_base); > release_regions: > pci_release_regions(pci); > @@ -117,10 +159,8 @@ static int snd_acp3x_probe(struct pci_dev *pci, > static void snd_acp3x_remove(struct pci_dev *pci) > { > struct acp3x_dev_data *adata = pci_get_drvdata(pci); > - > - platform_device_unregister(adata->pdev); > + mfd_remove_devices(adata->parent); > iounmap(adata->acp3x_base); > - > pci_release_regions(pci); > pci_disable_device(pci); > } > @@ -142,6 +182,7 @@ static struct pci_driver acp3x_driver = { > > module_pci_driver(acp3x_driver); > > +MODULE_AUTHOR("Vishnuvardhanrao.Ravulapati@amd.com"); > MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com"); > MODULE_DESCRIPTION("AMD ACP3x PCI driver"); > MODULE_LICENSE("GPL v2");
Hi Jones, I am very Thankful to your review comments. Actually The driver is not totally based on MFD. It just uses mfd_add_hotplug_devices() and mfd_remove_devices() for adding the devices automatically. Remaining code has nothing to do with MFD framework. So I thought It would not break the coding style and moved ahead by using the MFD API by adding its header file. If it is any violation of coding standard then I can move it to drivers/mfd. This patch could be a show stopper for us.Please suggest us how can we move ahead ASAP. Thank you once again for your inputs. Regards, Vishnu On 01/10/19 12:15 PM, Lee Jones wrote: > On Tue, 01 Oct 2019, Ravulapati Vishnu vardhan rao wrote: > >> Removed platform based endpoint registering in ACP-PCI driver. >> Now Registering PCM DMA and multiple I2S instances: SP and BT endpoint >> devices automatically by using MFD framework. > > This is a hack. > > Why are you using the MFD framework outside of drivers/mfd? > > If this driver is an MFD, then please create an MFD driver. > > If it's not, please do not use the MFD API. > >> Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> >> --- >> sound/soc/amd/raven/acp3x.h | 8 +++ >> sound/soc/amd/raven/pci-acp3x.c | 123 ++++++++++++++++++++++++++-------------- >> 2 files changed, 90 insertions(+), 41 deletions(-) >> >> diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h >> index 4f2cadd..c122dc6 100644 >> --- a/sound/soc/amd/raven/acp3x.h >> +++ b/sound/soc/amd/raven/acp3x.h >> @@ -7,13 +7,21 @@ >> >> #include "chip_offset_byte.h" >> >> +#define ACP3x_DEVS 3 >> #define ACP3x_PHY_BASE_ADDRESS 0x1240000 >> #define ACP3x_I2S_MODE 0 >> #define ACP3x_REG_START 0x1240000 >> #define ACP3x_REG_END 0x1250200 >> +#define ACP3x_I2STDM_REG_START 0x1242400 >> +#define ACP3x_I2STDM_REG_END 0x1242410 >> +#define ACP3x_BT_TDM_REG_START 0x1242800 >> +#define ACP3x_BT_TDM_REG_END 0x1242810 >> #define I2S_MODE 0x04 >> +#define I2S_RX_THRESHOLD 27 >> +#define I2S_TX_THRESHOLD 28 >> #define BT_TX_THRESHOLD 26 >> #define BT_RX_THRESHOLD 25 >> +#define ACP_ERR_INTR_MASK 29 >> #define ACP3x_POWER_ON 0x00 >> #define ACP3x_POWER_ON_IN_PROGRESS 0x01 >> #define ACP3x_POWER_OFF 0x02 >> diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c >> index 8f6bf00..d9f5bc0 100644 >> --- a/sound/soc/amd/raven/pci-acp3x.c >> +++ b/sound/soc/amd/raven/pci-acp3x.c >> @@ -9,13 +9,21 @@ >> #include <linux/io.h> >> #include <linux/platform_device.h> >> #include <linux/interrupt.h> >> +#include <linux/mfd/core.h> >> >> #include "acp3x.h" >> >> +struct i2s_platform_data { >> + unsigned int cap; >> + int channel; >> + u32 snd_rates; >> +}; >> struct acp3x_dev_data { >> + struct device *parent; >> + struct mfd_cell *cell; >> + struct resource *res; >> void __iomem *acp3x_base; >> bool acp3x_audio_mode; >> - struct resource *res; >> struct platform_device *pdev; >> }; >> >> @@ -23,9 +31,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, >> const struct pci_device_id *pci_id) >> { >> int ret; >> - u32 addr, val; >> + resource_size_t addr; >> + int val, i, r; >> struct acp3x_dev_data *adata; >> - struct platform_device_info pdevinfo; >> + struct device *dev; >> + struct i2s_platform_data *i2s_pdata; >> unsigned int irqflags; >> >> if (pci_enable_device(pci)) { >> @@ -56,55 +66,87 @@ static int snd_acp3x_probe(struct pci_dev *pci, >> } >> pci_set_master(pci); >> pci_set_drvdata(pci, adata); >> - >> + adata->parent = &pci->dev; >> val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); >> switch (val) { >> case I2S_MODE: >> adata->res = devm_kzalloc(&pci->dev, >> - sizeof(struct resource) * 2, >> - GFP_KERNEL); >> - if (!adata->res) { >> + sizeof(struct resource) * 4, >> + GFP_KERNEL); >> + adata->cell = devm_kzalloc(&pci->dev, >> + sizeof(struct mfd_cell) * ACP3x_DEVS, >> + GFP_KERNEL); >> + if (!adata->cell) { >> ret = -ENOMEM; >> goto unmap_mmio; >> } >> >> - adata->res[0].name = "acp3x_i2s_iomem"; >> - adata->res[0].flags = IORESOURCE_MEM; >> - adata->res[0].start = addr; >> - adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START); >> - >> - adata->res[1].name = "acp3x_i2s_irq"; >> - adata->res[1].flags = IORESOURCE_IRQ; >> - adata->res[1].start = pci->irq; >> - adata->res[1].end = pci->irq; >> - >> - adata->acp3x_audio_mode = ACP3x_I2S_MODE; >> - >> - memset(&pdevinfo, 0, sizeof(pdevinfo)); >> - pdevinfo.name = "acp3x_rv_i2s"; >> - pdevinfo.id = 0; >> - pdevinfo.parent = &pci->dev; >> - pdevinfo.num_res = 2; >> - pdevinfo.res = adata->res; >> - pdevinfo.data = &irqflags; >> - pdevinfo.size_data = sizeof(irqflags); >> - >> - adata->pdev = platform_device_register_full(&pdevinfo); >> - if (IS_ERR(adata->pdev)) { >> - dev_err(&pci->dev, "cannot register %s device\n", >> - pdevinfo.name); >> - ret = PTR_ERR(adata->pdev); >> - goto unmap_mmio; >> + i2s_pdata = devm_kzalloc(&pci->dev, >> + sizeof(struct i2s_platform_data) * ACP3x_DEVS, >> + GFP_KERNEL); >> + if (i2s_pdata == NULL) { >> + kfree(adata->res); >> + kfree(adata->cell); >> + return -ENOMEM; >> } >> + adata->res[0].name = "acp3x_i2s_iomem"; >> + adata->res[0].flags = IORESOURCE_MEM; >> + adata->res[0].start = addr; >> + adata->res[0].end = addr + >> + (ACP3x_REG_END - ACP3x_REG_START); >> + i2s_pdata[0].cap = 0; >> + i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000; >> + >> + adata->res[1].name = "acp3x_i2s_sp_play_cap"; >> + adata->res[1].flags = IORESOURCE_MEM; >> + adata->res[1].start = addr + ACP3x_I2STDM_REG_START; >> + adata->res[1].end = addr + ACP3x_I2STDM_REG_END; >> + i2s_pdata[1].cap = 0; >> + i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000; >> + >> + adata->res[2].name = "acp3x_i2s_bt_play_cap"; >> + adata->res[2].flags = IORESOURCE_MEM; >> + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START; >> + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END; >> + i2s_pdata[2].cap = 0; >> + i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000; >> + >> + adata->res[3].name = "acp3x_i2s_irq"; >> + adata->res[3].flags = IORESOURCE_IRQ; >> + adata->res[3].start = pci->irq; >> + adata->res[3].end = adata->res[3].start; >> + >> + adata->acp3x_audio_mode = ACP3x_I2S_MODE; >> + >> + adata->cell[0].name = "acp3x_rv_i2s_dma"; >> + adata->cell[0].num_resources = 4; >> + adata->cell[0].resources = &adata->res[0]; >> + adata->cell[0].platform_data = &irqflags; >> + adata->cell[0].pdata_size = sizeof(irqflags); >> + >> + adata->cell[1].name = "acp3x_i2s_playcap"; >> + adata->cell[1].num_resources = 1; >> + adata->cell[1].resources = &adata->res[1]; >> + adata->cell[1].platform_data = &i2s_pdata[0]; >> + adata->cell[1].pdata_size = >> + sizeof(struct i2s_platform_data); >> + >> + adata->cell[2].name = "acp3x_i2s_playcap"; >> + adata->cell[2].num_resources = 1; >> + adata->cell[2].resources = &adata->res[2]; >> + adata->cell[2].platform_data = &i2s_pdata[1]; >> + adata->cell[2].pdata_size = >> + sizeof(struct i2s_platform_data); >> + r = mfd_add_hotplug_devices(adata->parent, >> + adata->cell, ACP3x_DEVS); >> break; >> - default: >> - dev_err(&pci->dev, "Invalid ACP audio mode : %d\n", val); >> - ret = -ENODEV; >> - goto unmap_mmio; >> } >> return 0; >> >> unmap_mmio: >> + mfd_remove_devices(adata->parent); >> + kfree(adata->res); >> + kfree(adata->cell); >> iounmap(adata->acp3x_base); >> release_regions: >> pci_release_regions(pci); >> @@ -117,10 +159,8 @@ static int snd_acp3x_probe(struct pci_dev *pci, >> static void snd_acp3x_remove(struct pci_dev *pci) >> { >> struct acp3x_dev_data *adata = pci_get_drvdata(pci); >> - >> - platform_device_unregister(adata->pdev); >> + mfd_remove_devices(adata->parent); >> iounmap(adata->acp3x_base); >> - >> pci_release_regions(pci); >> pci_disable_device(pci); >> } >> @@ -142,6 +182,7 @@ static struct pci_driver acp3x_driver = { >> >> module_pci_driver(acp3x_driver); >> >> +MODULE_AUTHOR("Vishnuvardhanrao.Ravulapati@amd.com"); >> MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com"); >> MODULE_DESCRIPTION("AMD ACP3x PCI driver"); >> MODULE_LICENSE("GPL v2"); >
On Tue, 01 Oct 2019, vishnu wrote: > Hi Jones, > > I am very Thankful to your review comments. > > Actually The driver is not totally based on MFD. It just uses > mfd_add_hotplug_devices() and mfd_remove_devices() for adding the > devices automatically. > > Remaining code has nothing to do with MFD framework. > > So I thought It would not break the coding style and moved ahead by > using the MFD API by adding its header file. > > If it is any violation of coding standard then I can move it to > drivers/mfd. > > This patch could be a show stopper for us.Please suggest us how can we > move ahead ASAP. Either move the MFD parts to drivers/mfd, or stop using the MFD API.
> -----Original Message----- > From: Lee Jones <lee.jones@linaro.org> > Sent: Tuesday, October 1, 2019 8:00 AM > To: RAVULAPATI, VISHNU VARDHAN RAO > <Vishnuvardhanrao.Ravulapati@amd.com> > Cc: RAVULAPATI, VISHNU VARDHAN RAO > <Vishnuvardhanrao.Ravulapati@amd.com>; Deucher, Alexander > <Alexander.Deucher@amd.com>; Liam Girdwood <lgirdwood@gmail.com>; > Mark Brown <broonie@kernel.org>; Jaroslav Kysela <perex@perex.cz>; > Takashi Iwai <tiwai@suse.com>; Mukunda, Vijendar > <Vijendar.Mukunda@amd.com>; Maruthi Srinivas Bayyavarapu > <Maruthi.Bayyavarapu@amd.com>; Mehta, Sanju > <Sanju.Mehta@amd.com>; Colin Ian King <colin.king@canonical.com>; Dan > Carpenter <dan.carpenter@oracle.com>; moderated list:SOUND - SOC LAYER > / DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; > open list <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH 2/7] ASoC: amd: Registering device endpoints using MFD > framework > > On Tue, 01 Oct 2019, vishnu wrote: > > > Hi Jones, > > > > I am very Thankful to your review comments. > > > > Actually The driver is not totally based on MFD. It just uses > > mfd_add_hotplug_devices() and mfd_remove_devices() for adding the > > devices automatically. > > > > Remaining code has nothing to do with MFD framework. > > > > So I thought It would not break the coding style and moved ahead by > > using the MFD API by adding its header file. > > > > If it is any violation of coding standard then I can move it to > > drivers/mfd. > > > > This patch could be a show stopper for us.Please suggest us how can we > > move ahead ASAP. > > Either move the MFD parts to drivers/mfd, or stop using the MFD API. There are more drivers outside of drivers/mfd using this API than drivers in drivers/mfd. In a lot of cases it doesn't make sense to move the driver to drivers/mfd. Alex > > -- > Lee Jones [李琼斯] > Linaro Services Technical Lead > Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | > Twitter | Blog
On Tue, 01 Oct 2019, Deucher, Alexander wrote: > > -----Original Message----- > > From: Lee Jones <lee.jones@linaro.org> > > Sent: Tuesday, October 1, 2019 8:00 AM > > To: RAVULAPATI, VISHNU VARDHAN RAO > > <Vishnuvardhanrao.Ravulapati@amd.com> > > Cc: RAVULAPATI, VISHNU VARDHAN RAO > > <Vishnuvardhanrao.Ravulapati@amd.com>; Deucher, Alexander > > <Alexander.Deucher@amd.com>; Liam Girdwood <lgirdwood@gmail.com>; > > Mark Brown <broonie@kernel.org>; Jaroslav Kysela <perex@perex.cz>; > > Takashi Iwai <tiwai@suse.com>; Mukunda, Vijendar > > <Vijendar.Mukunda@amd.com>; Maruthi Srinivas Bayyavarapu > > <Maruthi.Bayyavarapu@amd.com>; Mehta, Sanju > > <Sanju.Mehta@amd.com>; Colin Ian King <colin.king@canonical.com>; Dan > > Carpenter <dan.carpenter@oracle.com>; moderated list:SOUND - SOC LAYER > > / DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; > > open list <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH 2/7] ASoC: amd: Registering device endpoints using MFD > > framework > > > > On Tue, 01 Oct 2019, vishnu wrote: > > > > > Hi Jones, > > > > > > I am very Thankful to your review comments. > > > > > > Actually The driver is not totally based on MFD. It just uses > > > mfd_add_hotplug_devices() and mfd_remove_devices() for adding the > > > devices automatically. > > > > > > Remaining code has nothing to do with MFD framework. > > > > > > So I thought It would not break the coding style and moved ahead by > > > using the MFD API by adding its header file. > > > > > > If it is any violation of coding standard then I can move it to > > > drivers/mfd. > > > > > > This patch could be a show stopper for us.Please suggest us how can we > > > move ahead ASAP. > > > > Either move the MFD parts to drivers/mfd, or stop using the MFD API. > > There are more drivers outside of drivers/mfd using this API than > drivers in drivers/mfd. People do wrong things all the time. It doesn't make them right. > In a lot of cases it doesn't make sense to move the driver to drivers/mfd. In those cases, the platform_device_*() API should be used.
> -----Original Message----- > From: Lee Jones <lee.jones@linaro.org> > Sent: Wednesday, October 2, 2019 8:38 AM > To: Deucher, Alexander <Alexander.Deucher@amd.com> > Cc: RAVULAPATI, VISHNU VARDHAN RAO > <Vishnuvardhanrao.Ravulapati@amd.com>; Liam Girdwood > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Jaroslav > Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Mukunda, > Vijendar <Vijendar.Mukunda@amd.com>; Maruthi Srinivas Bayyavarapu > <Maruthi.Bayyavarapu@amd.com>; Mehta, Sanju > <Sanju.Mehta@amd.com>; Colin Ian King <colin.king@canonical.com>; Dan > Carpenter <dan.carpenter@oracle.com>; moderated list:SOUND - SOC LAYER > / DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; > open list <linux-kernel@vger.kernel.org> > Subject: Re: [PATCH 2/7] ASoC: amd: Registering device endpoints using MFD > framework > > On Tue, 01 Oct 2019, Deucher, Alexander wrote: > > > > -----Original Message----- > > > From: Lee Jones <lee.jones@linaro.org> > > > Sent: Tuesday, October 1, 2019 8:00 AM > > > To: RAVULAPATI, VISHNU VARDHAN RAO > > > <Vishnuvardhanrao.Ravulapati@amd.com> > > > Cc: RAVULAPATI, VISHNU VARDHAN RAO > > > <Vishnuvardhanrao.Ravulapati@amd.com>; Deucher, Alexander > > > <Alexander.Deucher@amd.com>; Liam Girdwood > <lgirdwood@gmail.com>; > > > Mark Brown <broonie@kernel.org>; Jaroslav Kysela <perex@perex.cz>; > > > Takashi Iwai <tiwai@suse.com>; Mukunda, Vijendar > > > <Vijendar.Mukunda@amd.com>; Maruthi Srinivas Bayyavarapu > > > <Maruthi.Bayyavarapu@amd.com>; Mehta, Sanju > <Sanju.Mehta@amd.com>; > > > Colin Ian King <colin.king@canonical.com>; Dan Carpenter > > > <dan.carpenter@oracle.com>; moderated list:SOUND - SOC LAYER / > > > DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; > open > > > list <linux-kernel@vger.kernel.org> > > > Subject: Re: [PATCH 2/7] ASoC: amd: Registering device endpoints > > > using MFD framework > > > > > > On Tue, 01 Oct 2019, vishnu wrote: > > > > > > > Hi Jones, > > > > > > > > I am very Thankful to your review comments. > > > > > > > > Actually The driver is not totally based on MFD. It just uses > > > > mfd_add_hotplug_devices() and mfd_remove_devices() for adding > the > > > > devices automatically. > > > > > > > > Remaining code has nothing to do with MFD framework. > > > > > > > > So I thought It would not break the coding style and moved ahead > > > > by using the MFD API by adding its header file. > > > > > > > > If it is any violation of coding standard then I can move it to > > > > drivers/mfd. > > > > > > > > This patch could be a show stopper for us.Please suggest us how > > > > can we move ahead ASAP. > > > > > > Either move the MFD parts to drivers/mfd, or stop using the MFD API. > > > > There are more drivers outside of drivers/mfd using this API than > > drivers in drivers/mfd. > > People do wrong things all the time. It doesn't make them right. > > > In a lot of cases it doesn't make sense to move the driver to drivers/mfd. > > In those cases, the platform_device_*() API should be used. Why do we have both? It's not clear to me on when we should use one vs the other. These are not platforms per se, they are PCI devices that happen to have other devices on them. On previous projects, I was told to use mfd and no objections were raised at that time. Alex
On Wed, 02 Oct 2019, Deucher, Alexander wrote: > > -----Original Message----- > > From: Lee Jones <lee.jones@linaro.org> > > Sent: Wednesday, October 2, 2019 8:38 AM > > To: Deucher, Alexander <Alexander.Deucher@amd.com> > > Cc: RAVULAPATI, VISHNU VARDHAN RAO > > <Vishnuvardhanrao.Ravulapati@amd.com>; Liam Girdwood > > <lgirdwood@gmail.com>; Mark Brown <broonie@kernel.org>; Jaroslav > > Kysela <perex@perex.cz>; Takashi Iwai <tiwai@suse.com>; Mukunda, > > Vijendar <Vijendar.Mukunda@amd.com>; Maruthi Srinivas Bayyavarapu > > <Maruthi.Bayyavarapu@amd.com>; Mehta, Sanju > > <Sanju.Mehta@amd.com>; Colin Ian King <colin.king@canonical.com>; Dan > > Carpenter <dan.carpenter@oracle.com>; moderated list:SOUND - SOC LAYER > > / DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; > > open list <linux-kernel@vger.kernel.org> > > Subject: Re: [PATCH 2/7] ASoC: amd: Registering device endpoints using MFD > > framework > > > > On Tue, 01 Oct 2019, Deucher, Alexander wrote: > > > > > > -----Original Message----- > > > > From: Lee Jones <lee.jones@linaro.org> > > > > Sent: Tuesday, October 1, 2019 8:00 AM > > > > To: RAVULAPATI, VISHNU VARDHAN RAO > > > > <Vishnuvardhanrao.Ravulapati@amd.com> > > > > Cc: RAVULAPATI, VISHNU VARDHAN RAO > > > > <Vishnuvardhanrao.Ravulapati@amd.com>; Deucher, Alexander > > > > <Alexander.Deucher@amd.com>; Liam Girdwood > > <lgirdwood@gmail.com>; > > > > Mark Brown <broonie@kernel.org>; Jaroslav Kysela <perex@perex.cz>; > > > > Takashi Iwai <tiwai@suse.com>; Mukunda, Vijendar > > > > <Vijendar.Mukunda@amd.com>; Maruthi Srinivas Bayyavarapu > > > > <Maruthi.Bayyavarapu@amd.com>; Mehta, Sanju > > <Sanju.Mehta@amd.com>; > > > > Colin Ian King <colin.king@canonical.com>; Dan Carpenter > > > > <dan.carpenter@oracle.com>; moderated list:SOUND - SOC LAYER / > > > > DYNAMIC AUDIO POWER MANAGEM... <alsa-devel@alsa-project.org>; > > open > > > > list <linux-kernel@vger.kernel.org> > > > > Subject: Re: [PATCH 2/7] ASoC: amd: Registering device endpoints > > > > using MFD framework > > > > > > > > On Tue, 01 Oct 2019, vishnu wrote: > > > > > > > > > Hi Jones, > > > > > > > > > > I am very Thankful to your review comments. > > > > > > > > > > Actually The driver is not totally based on MFD. It just uses > > > > > mfd_add_hotplug_devices() and mfd_remove_devices() for adding > > the > > > > > devices automatically. > > > > > > > > > > Remaining code has nothing to do with MFD framework. > > > > > > > > > > So I thought It would not break the coding style and moved ahead > > > > > by using the MFD API by adding its header file. > > > > > > > > > > If it is any violation of coding standard then I can move it to > > > > > drivers/mfd. > > > > > > > > > > This patch could be a show stopper for us.Please suggest us how > > > > > can we move ahead ASAP. > > > > > > > > Either move the MFD parts to drivers/mfd, or stop using the MFD API. > > > > > > There are more drivers outside of drivers/mfd using this API than > > > drivers in drivers/mfd. > > > > People do wrong things all the time. It doesn't make them right. > > > > > In a lot of cases it doesn't make sense to move the driver to drivers/mfd. > > > > In those cases, the platform_device_*() API should be used. > > Why do we have both? It's not clear to me on when we should use one The platform_device_*() API is the de facto API to use for registering devices. mfd_*() is a framework built on-top of that for devices which register sub-devices that do not reasonably reside elsewhere. The mfd_*() helper functions should only be used by MFD devices. > vs the other. These are not platforms per se, they are PCI devices > that happen to have other devices on them. On previous projects, I > was told to use mfd and no objections were raised at that time. Who told you to use MFD API outside of drivers/mfd? That's a hack.
Hi Lee, We have two instances BT and I2S. We need to create devices with same name added with number of device like example: acp3x_i2s_playcap.1.auto<http://1.auto> acp3x_i2s_playcap.2.auto<http://2.auto> by using MFD we can make it happen automatically by giving "acp3x_i2s_playcap" and other extension will be added by MFD add device API. This helps us by rectifying the renaming issue which we get by using Platform_dev_create API`s.If we have to use platform related APIs then we need to give different naming conventions while creating the devices and cant use it in loop as we have 3 devices we need to call three explicitly.This make our code lengthy. If we use MFD it would help us a lot. Please suggest us how can we proceed. Thanks, Vishnu Get Outlook for Android<https://aka.ms/ghei36>
On Thu, 10 Oct 2019, RAVULAPATI, VISHNU VARDHAN RAO wrote: > Hi Lee, > > We have two instances BT and I2S. > We need to create devices with same name added with number of device > like example: > acp3x_i2s_playcap.1.auto<http://1.auto> > acp3x_i2s_playcap.2.auto<http://2.auto> > > by using MFD we can make it happen automatically by giving > "acp3x_i2s_playcap" and other extension will be added by MFD add device API. The auto extension is handed by the platform_deivce_alloc() API. platform_device_alloc("acp3x_i2s_playcap", PLATFORM_DEVID_AUTO); > This helps us by rectifying the renaming issue which we get by using > Platform_dev_create API`s.If we have to use platform related APIs then > we need to give different naming conventions while creating the devices > and cant use it in loop as we have 3 devices we need to call three > explicitly.This make our code lengthy. > If we use MFD it would help us a lot. > > Please suggest us how can we proceed. You have 2 choices available to you based on whether your device is an MFD or not: If yes, move it (or a part of it) to drivers/mfd. If no, then use the platform_device_*() API.
Hi Lee, Okay.We will proceed with existing platform device model. Mark, I will resend the patches with the review comments addressed. Thanks, Vishnu On 14/10/19 12:33 PM, Lee Jones wrote: > On Thu, 10 Oct 2019, RAVULAPATI, VISHNU VARDHAN RAO wrote: > >> Hi Lee, >> >> We have two instances BT and I2S. >> We need to create devices with same name added with number of device >> like example: >> acp3x_i2s_playcap.1.auto<http://1.auto> >> acp3x_i2s_playcap.2.auto<http://2.auto> >> >> by using MFD we can make it happen automatically by giving >> "acp3x_i2s_playcap" and other extension will be added by MFD add device API. > > The auto extension is handed by the platform_deivce_alloc() API. > > platform_device_alloc("acp3x_i2s_playcap", PLATFORM_DEVID_AUTO); > >> This helps us by rectifying the renaming issue which we get by using >> Platform_dev_create API`s.If we have to use platform related APIs then >> we need to give different naming conventions while creating the devices >> and cant use it in loop as we have 3 devices we need to call three >> explicitly.This make our code lengthy. >> If we use MFD it would help us a lot. >> >> Please suggest us how can we proceed. > > You have 2 choices available to you based on whether your device is an > MFD or not: > > If yes, move it (or a part of it) to drivers/mfd. > If no, then use the platform_device_*() API. >
diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h index 4f2cadd..c122dc6 100644 --- a/sound/soc/amd/raven/acp3x.h +++ b/sound/soc/amd/raven/acp3x.h @@ -7,13 +7,21 @@ #include "chip_offset_byte.h" +#define ACP3x_DEVS 3 #define ACP3x_PHY_BASE_ADDRESS 0x1240000 #define ACP3x_I2S_MODE 0 #define ACP3x_REG_START 0x1240000 #define ACP3x_REG_END 0x1250200 +#define ACP3x_I2STDM_REG_START 0x1242400 +#define ACP3x_I2STDM_REG_END 0x1242410 +#define ACP3x_BT_TDM_REG_START 0x1242800 +#define ACP3x_BT_TDM_REG_END 0x1242810 #define I2S_MODE 0x04 +#define I2S_RX_THRESHOLD 27 +#define I2S_TX_THRESHOLD 28 #define BT_TX_THRESHOLD 26 #define BT_RX_THRESHOLD 25 +#define ACP_ERR_INTR_MASK 29 #define ACP3x_POWER_ON 0x00 #define ACP3x_POWER_ON_IN_PROGRESS 0x01 #define ACP3x_POWER_OFF 0x02 diff --git a/sound/soc/amd/raven/pci-acp3x.c b/sound/soc/amd/raven/pci-acp3x.c index 8f6bf00..d9f5bc0 100644 --- a/sound/soc/amd/raven/pci-acp3x.c +++ b/sound/soc/amd/raven/pci-acp3x.c @@ -9,13 +9,21 @@ #include <linux/io.h> #include <linux/platform_device.h> #include <linux/interrupt.h> +#include <linux/mfd/core.h> #include "acp3x.h" +struct i2s_platform_data { + unsigned int cap; + int channel; + u32 snd_rates; +}; struct acp3x_dev_data { + struct device *parent; + struct mfd_cell *cell; + struct resource *res; void __iomem *acp3x_base; bool acp3x_audio_mode; - struct resource *res; struct platform_device *pdev; }; @@ -23,9 +31,11 @@ static int snd_acp3x_probe(struct pci_dev *pci, const struct pci_device_id *pci_id) { int ret; - u32 addr, val; + resource_size_t addr; + int val, i, r; struct acp3x_dev_data *adata; - struct platform_device_info pdevinfo; + struct device *dev; + struct i2s_platform_data *i2s_pdata; unsigned int irqflags; if (pci_enable_device(pci)) { @@ -56,55 +66,87 @@ static int snd_acp3x_probe(struct pci_dev *pci, } pci_set_master(pci); pci_set_drvdata(pci, adata); - + adata->parent = &pci->dev; val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); switch (val) { case I2S_MODE: adata->res = devm_kzalloc(&pci->dev, - sizeof(struct resource) * 2, - GFP_KERNEL); - if (!adata->res) { + sizeof(struct resource) * 4, + GFP_KERNEL); + adata->cell = devm_kzalloc(&pci->dev, + sizeof(struct mfd_cell) * ACP3x_DEVS, + GFP_KERNEL); + if (!adata->cell) { ret = -ENOMEM; goto unmap_mmio; } - adata->res[0].name = "acp3x_i2s_iomem"; - adata->res[0].flags = IORESOURCE_MEM; - adata->res[0].start = addr; - adata->res[0].end = addr + (ACP3x_REG_END - ACP3x_REG_START); - - adata->res[1].name = "acp3x_i2s_irq"; - adata->res[1].flags = IORESOURCE_IRQ; - adata->res[1].start = pci->irq; - adata->res[1].end = pci->irq; - - adata->acp3x_audio_mode = ACP3x_I2S_MODE; - - memset(&pdevinfo, 0, sizeof(pdevinfo)); - pdevinfo.name = "acp3x_rv_i2s"; - pdevinfo.id = 0; - pdevinfo.parent = &pci->dev; - pdevinfo.num_res = 2; - pdevinfo.res = adata->res; - pdevinfo.data = &irqflags; - pdevinfo.size_data = sizeof(irqflags); - - adata->pdev = platform_device_register_full(&pdevinfo); - if (IS_ERR(adata->pdev)) { - dev_err(&pci->dev, "cannot register %s device\n", - pdevinfo.name); - ret = PTR_ERR(adata->pdev); - goto unmap_mmio; + i2s_pdata = devm_kzalloc(&pci->dev, + sizeof(struct i2s_platform_data) * ACP3x_DEVS, + GFP_KERNEL); + if (i2s_pdata == NULL) { + kfree(adata->res); + kfree(adata->cell); + return -ENOMEM; } + adata->res[0].name = "acp3x_i2s_iomem"; + adata->res[0].flags = IORESOURCE_MEM; + adata->res[0].start = addr; + adata->res[0].end = addr + + (ACP3x_REG_END - ACP3x_REG_START); + i2s_pdata[0].cap = 0; + i2s_pdata[0].snd_rates = SNDRV_PCM_RATE_8000_96000; + + adata->res[1].name = "acp3x_i2s_sp_play_cap"; + adata->res[1].flags = IORESOURCE_MEM; + adata->res[1].start = addr + ACP3x_I2STDM_REG_START; + adata->res[1].end = addr + ACP3x_I2STDM_REG_END; + i2s_pdata[1].cap = 0; + i2s_pdata[1].snd_rates = SNDRV_PCM_RATE_8000_96000; + + adata->res[2].name = "acp3x_i2s_bt_play_cap"; + adata->res[2].flags = IORESOURCE_MEM; + adata->res[2].start = addr + ACP3x_BT_TDM_REG_START; + adata->res[2].end = addr + ACP3x_BT_TDM_REG_END; + i2s_pdata[2].cap = 0; + i2s_pdata[2].snd_rates = SNDRV_PCM_RATE_8000_96000; + + adata->res[3].name = "acp3x_i2s_irq"; + adata->res[3].flags = IORESOURCE_IRQ; + adata->res[3].start = pci->irq; + adata->res[3].end = adata->res[3].start; + + adata->acp3x_audio_mode = ACP3x_I2S_MODE; + + adata->cell[0].name = "acp3x_rv_i2s_dma"; + adata->cell[0].num_resources = 4; + adata->cell[0].resources = &adata->res[0]; + adata->cell[0].platform_data = &irqflags; + adata->cell[0].pdata_size = sizeof(irqflags); + + adata->cell[1].name = "acp3x_i2s_playcap"; + adata->cell[1].num_resources = 1; + adata->cell[1].resources = &adata->res[1]; + adata->cell[1].platform_data = &i2s_pdata[0]; + adata->cell[1].pdata_size = + sizeof(struct i2s_platform_data); + + adata->cell[2].name = "acp3x_i2s_playcap"; + adata->cell[2].num_resources = 1; + adata->cell[2].resources = &adata->res[2]; + adata->cell[2].platform_data = &i2s_pdata[1]; + adata->cell[2].pdata_size = + sizeof(struct i2s_platform_data); + r = mfd_add_hotplug_devices(adata->parent, + adata->cell, ACP3x_DEVS); break; - default: - dev_err(&pci->dev, "Invalid ACP audio mode : %d\n", val); - ret = -ENODEV; - goto unmap_mmio; } return 0; unmap_mmio: + mfd_remove_devices(adata->parent); + kfree(adata->res); + kfree(adata->cell); iounmap(adata->acp3x_base); release_regions: pci_release_regions(pci); @@ -117,10 +159,8 @@ static int snd_acp3x_probe(struct pci_dev *pci, static void snd_acp3x_remove(struct pci_dev *pci) { struct acp3x_dev_data *adata = pci_get_drvdata(pci); - - platform_device_unregister(adata->pdev); + mfd_remove_devices(adata->parent); iounmap(adata->acp3x_base); - pci_release_regions(pci); pci_disable_device(pci); } @@ -142,6 +182,7 @@ static struct pci_driver acp3x_driver = { module_pci_driver(acp3x_driver); +MODULE_AUTHOR("Vishnuvardhanrao.Ravulapati@amd.com"); MODULE_AUTHOR("Maruthi.Bayyavarapu@amd.com"); MODULE_DESCRIPTION("AMD ACP3x PCI driver"); MODULE_LICENSE("GPL v2");
Removed platform based endpoint registering in ACP-PCI driver. Now Registering PCM DMA and multiple I2S instances: SP and BT endpoint devices automatically by using MFD framework. Signed-off-by: Ravulapati Vishnu vardhan rao <Vishnuvardhanrao.Ravulapati@amd.com> --- sound/soc/amd/raven/acp3x.h | 8 +++ sound/soc/amd/raven/pci-acp3x.c | 123 ++++++++++++++++++++++++++-------------- 2 files changed, 90 insertions(+), 41 deletions(-)