On Mon, Mar 02, 2015 at 06:16:13PM +0000, Mark Brown wrote:
On Mon, Mar 02, 2015 at 05:57:55PM +0000, Charles Keepax wrote:
Firstly, 'Input Source' changes names to 'Tx Source' this is a lot clearer, but perhaps we shouldn't change this, as it doesn't have to change?
Yes, leave it be. If you were renaming this should be in the changelog with an explanation as to why.
Cool can do.
Secondly, the patch removes the controls 'TX Playback Switch' and 'AIF Playback Switch' as these bits are controlled by DAPM now. I wasn't sure if it would be preferrable to leave dummy controls that do nothing?
This sounds like you need a more detailed changelog entry explaining why it's a good idea to do this in the first place, never mind the proxy control - what is the purpose of this change and what benefit will it bring? Given that this is a S/PDIF bridge it's not at all obvious that the chip will be used in applications where the sort of power savings you're likely to see from powering down one of the links (as opposed to cutting the power to the chip) are likely to register. Why move those bits to DAPM control at all?
Hmm... I hadn't really thought about it like that, mostly I had viewed this somewhat as a nice modernization of the driver. I am pretty sure there are some benefits around the clocking from disabling the RX path (which is never disabled currently) as this exercises some automatic control over the PLL whilst active, which can be difficult to handle. But I guess that could also just be handled by adding a control to disable it. Let us try redrafting the commit message and we can see where that takes us I guess.
I also note that this patch is apparently written by but not CCed to Sapthagiri Baratam.
He is on the patches alias, but yes that was rather remiss of me not to directly CC him.
Thanks, Charles