On Fri, Feb 10, 2023 at 02:39:56PM +0000, Lucas Tanure wrote:
On 10-02-2023 13:43, Charles Keepax wrote:
On Fri, Feb 10, 2023 at 09:19:41AM +0000, Lucas Tanure wrote:
- {CS35L41_MDSYNC_EN, 0x00001000},
David's internal patch appears to set 0x3000 on the active side, not sure where that difference snuck in, or which is the correct value. Your settings appear to make logical sense to me though, TX on the active side, RX on the passive side.
And as the patch sets TX and RX in the same chip I changed to follow the documentation.
Yeah I mean I suspect this is sensible, unless there is some reason the controller side also needs to have RX enabled. Perhaps for feedback or something from the passive side, but I imagine this is just a typo in the original patch.
- /* BST_CTL_SEL = CLASSH */
- {CS35L41_BSTCVRT_VCTRL2, 0x00000001},
BST_CTL_SEL is in BSTCVRT_VCTRL1 (or BOOST_VOLTAGE_CFG, as it is called in the datasheet, yay us for using the same names). That does not mean this write is wrong, could just be the comment, but what this does write is a bit odd so I would like David to confirm this isn't some typo in his original patch.
I can't find BOOST_VOLTAGE_CFG on my datasheet, but BST_CTL_SEL is at 0x00003804 ( BSTCVRT_VCTRL2 / VBST_CTL_2 ). This write here is to select the boost control source, which for the active should be "Class H tracking value". So I still think this is correct.
Yeah this one is a mistake on my part, I was reviewing some patches on another amp just before I think I have looked at the wrong datasheet here. You are correct those bits are infact BST_CTL_SEL. So ignore this one.
regmap_read(regmap, CS35L41_PWR_CTRL3, &pwr_ctrl3);regmap_read(regmap, CS35L41_GPIO_PAD_CONTROL, &pad_control);pwr_ctrl3 &= ~CS35L41_SYNC_EN_MASK;pwr_ctrl1 = enable << CS35L41_GLOBAL_EN_SHIFT;Are you sure this is what you want? In the case of powering up, the sequence would end up being:
mdsync_down -> sets GLOBAL_EN on mdsync_up -> sets GLOBAL_EN off -> sets GLOBAL_EN on
Feels like mdsync_down should always turn global_enable off? But again I don't know for sure. But then I guess why is there the extra write to turn it off in mdsync_up?
For the disable case (DAPM turning everything off) SYNC and Global enable are off and the code hits
if (!enable) break;
Yes, so the disable flow makes perfect sense here it is the enable flow that seemed odd.
But for for enable case (DAPM turning everything On) the code continues enabling SYNC_EN, and turning off Global enable, as requested by "4.10.1 Multidevice Synchronization Enable" page 70. But as it is a enable path Global should be enabled again.
I can't see any sign of
GLOBAL_EN bouncing in David's internal patch.
Yes, but it is required by : "4.10.1 Multidevice Synchronization Enable" page 70.
Hmm... yes that does appear to suggest bouncing the global enable. Kinda weird, I can't help but wonder if the turning global enable off is actually needed, but I guess it does say that so probably safest. It is also rather unclear on who that sequence should be performed on it says:
"When powering up a second (and each subsequent) CS35L41B onto a shared MDSYNC bus, the following protocol must be followed"
But very unclear if that sequence should be followed on only the new device, the master device, or on all devices. I will try to find some time to chase some apps guys next week see if anyone has any ideas.
Thanks, Charles