| Message ID | 1523941201-15665-2-git-send-email-Vijendar.Mukunda@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Apr 17, 2018 at 10:29:51AM +0530, Vijendar Mukunda wrote: > +#define I2S_SP_INSTANCE 1 > +#define I2S_BT_INSTANCE 2 This is obviously very specific to the system you're working with and therefore doesn't belong in the generic driver. The device should be dealing with its own configuration, it shouldn't need to know about what specifically is connected to it. It's not even clear what they're doing in this driver given that there doesn't appear to be any use of the information, it feels like this is something that the machine driver should be encapsulating. Like I said with previous reviews this use of magic numbers for the interfaces is a bit of a red flag, internally within a driver they're fine but they shouldn't leak out too much except with things like numbering an array.
On Tuesday 17 April 2018 09:39 PM, Mark Brown wrote: > On Tue, Apr 17, 2018 at 10:29:51AM +0530, Vijendar Mukunda wrote: > >> +#define I2S_SP_INSTANCE 1 >> +#define I2S_BT_INSTANCE 2 > > This is obviously very specific to the system you're working with and > therefore doesn't belong in the generic driver. The device should be > dealing with its own configuration, it shouldn't need to know about what > specifically is connected to it. It's not even clear what they're doing > in this driver given that there doesn't appear to be any use of the > information, it feels like this is something that the machine driver > should be encapsulating. > > Like I said with previous reviews this use of magic numbers for the > interfaces is a bit of a red flag, internally within a driver they're > fine but they shouldn't leak out too much except with things like > numbering an array. > I will remove macros from designware header file and I will re spin the patch set
diff --git a/include/sound/designware_i2s.h b/include/sound/designware_i2s.h index 830f5ca..8113759 100644 --- a/include/sound/designware_i2s.h +++ b/include/sound/designware_i2s.h @@ -44,6 +44,10 @@ struct i2s_platform_data { int channel; u32 snd_fmts; u32 snd_rates; + /* i2s_instance parameter returns I2S controller instance value + * when multiple I2S controllers instantiated + */ + u32 i2s_instance; #define DW_I2S_QUIRK_COMP_REG_OFFSET (1 << 0) #define DW_I2S_QUIRK_COMP_PARAM1 (1 << 1) @@ -74,5 +78,7 @@ struct i2s_dma_data { #define FOUR_CHANNEL_SUPPORT 4 /* up to 3.1 */ #define SIX_CHANNEL_SUPPORT 6 /* up to 5.1 */ #define EIGHT_CHANNEL_SUPPORT 8 /* up to 7.1 */ +#define I2S_SP_INSTANCE 1 +#define I2S_BT_INSTANCE 2 #endif /* __SOUND_DESIGNWARE_I2S_H */ diff --git a/sound/soc/dwc/dwc-i2s.c b/sound/soc/dwc/dwc-i2s.c index 65112b9..58f81a4 100644 --- a/sound/soc/dwc/dwc-i2s.c +++ b/sound/soc/dwc/dwc-i2s.c @@ -553,6 +553,7 @@ static int dw_configure_dai_by_pd(struct dw_i2s_dev *dev, if (dev->quirks & DW_I2S_QUIRK_16BIT_IDX_OVERRIDE) idx = 1; + dev->i2s_instance = pdata->i2s_instance; /* Set DMA slaves info */ dev->play_dma_data.pd.data = pdata->play_dma_data; dev->capture_dma_data.pd.data = pdata->capture_dma_data; diff --git a/sound/soc/dwc/local.h b/sound/soc/dwc/local.h index 91dc70a..e89e464 100644 --- a/sound/soc/dwc/local.h +++ b/sound/soc/dwc/local.h @@ -98,6 +98,7 @@ struct dw_i2s_dev { u32 ccr; u32 xfer_resolution; u32 fifo_th; + u32 i2s_instance; /* data related to DMA transfers b/w i2s and DMAC */ union dw_i2s_snd_dma_data play_dma_data;