Le 23/05/2016 à 19:08, Mark Brown a écrit :
On Sun, May 22, 2016 at 11:29:55PM +0200, Emmanuel Fusté wrote:
There is nothing PCM5102A specific here, and it is pretty generic. Wouldn't it be better to write instead a simple-i2s-codec for all the classics I2S "hifi" DACs which will get the I2S/DAI parameters from DT ? PCM510x, PCM5122 in HW mode, ES9023, a bunch of ES90xx implementations etc... will use exactly the same code with only format and rate variation. And for the rate, it is implementation dependent, even in the case of pcm5102a.
If we do that then we have no idea what the hardware actually is and we're creating more effort on the DT side, the DT has to specify all the parameters for the device rather than just the name. Given how trivial the code is it's not clear that this is a win.
All the parameters are only rate and format. And even rate is implementation dependent. Knowing the name of the hardware give nothing. You still have to look at the schematic or look at the board to know valid rates. Actually is anything but a PCM5102A codec driver. It is a driver for an I2S device accepting PCM rate from 8khz to 192khz with bck rate of 48fs or 64fs (32fs is normally supported too by the hw mode used in hifiberry).
Yes this push more effort on the DT side, but I think this is better to describe in DT the actual "I2S only" hardware capabilities of the implementation of the device guided by wiring or design choice. Doing code cut&paste and two static assignment adjustment to create a "new" driver is completely overkill and nonflexible.
instead of the proposed dt, we could have :
/* pcm5102a in HW mode with external SCK (4wire mode) * at 128fs, 192fs or 256fs */ pcm5102a-4w: pcm5102a { compatible = "simple-i2s-codec"; rate = <SNDRV_PCM_RATE_8000_192000>; format = < SNDRV_PCM_FMTBIT_S16_LE |SNDRV_PCM_FMTBIT_S24_LE |SNDRV_PCM_FMTBIT_S32_LE>; }; or /* pcm5102a in HW mode with PLL on BCK (3wire mode) * 16to384khz but only with bck rate of 64fs */ pcm5102a-4w: pcm5102a { compatible = "simple-i2s-codec"; rate = <SNDRV_PCM_RATE_8000_192000 ^ SNDRV_PCM_RATE_8000>; format = <SNDRV_PCM_FMTBIT_S32_LE>; }; Or /* ES9023 in asynchronous mode mclk > 37Mhz * I did not try bck rate <64fs as the sabre family did not generally * support it. */ es9023: es9023 { compatible = "simple-i2s-codec"; rate = <SNDRV_PCM_RATE_8000_192000>; format = <SNDRV_PCM_FMTBIT_S32_LE>; };
We could use I2S friendly property instead of "format" : bckrates = <48, 64>;
With two or tree more property (gpio for mute, de-emphasis and filter selection), you could cover 98% of I2S DAC with hardware only interfaces. It cover too startup mode of most hifi I2C or SPI controllable DAC. Which is a good starting point for chip for which a driver is not already written.
Emmanuel.