Re: [PATCH v5 3/4] ALSA: cs35l41: Add shared boost feature

[Charles Keepax]

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