[PATCH v2] sound/soc: adds TAS5754M digital input amplifier component driver

[Mark Brown]

On Wed, Jan 19, 2022 at 01:50:33PM +0100, Joerg Schambacher wrote:

> Thanks for your useful feedback. I guess my comments on my first reply got lost somewhere on the way ....

Please don't top post, reply in line with needed context.  This allows
readers to readily follow the flow of conversation and understand what
you are talking about and also helps ensure that everything in the
discussion is being addressed.

Please fix your mail client to word wrap within paragraphs at something
substantially less than 80 columns.  Doing this makes your messages much
easier to read and reply to.

> On Wed, Nov 17, 2021 at 03:13:47PM +0000, Mark Brown wrote:
> > On Fri, Oct 29, 2021 at 11:57:30AM +0200, Joerg Schambacher wrote:

> > > +	mclk = clk_get_rate(tas5754m->sclk);
> > > +	bclk = sample_len * 2 * params_rate(params);

> > snd_soc_params_to_bclk().

> snd_soc_params_to_bclk() does not always gives the necessary value

In what way does it not give the needed value, and is that perhaps a
symptom of the constraints not being accurate?

> > > +	if (mute) {
> > > +		snd_soc_component_write(component, TAS5754M_MUTE, 0x11);
> > > +	} else {
> > > +		usleep_range(1000, 2000);
> > > +		snd_soc_component_write(component, TAS5754M_MUTE, 0x00);

> > Why the sleep here?

> Wait for settling of the clocks before unmute

Why would you need to wait for the clocks (which clocks?) to settle
before the unmute, this sounds like something that needs to be addressed
in whatever is providing the clocks.

> > If the register map can be copied can't the two drivers be combined?

> The TI apps team recommended to write a separate driver as there are some differences. I have also aligned some names to the TAS575xM spec in the next patch

What concrete differences are there here?  "The TI apps team said so"
isn't really upstream discussion or review...

> > > +#define TAS5754M_VIRT_BASE 0x000
> > > +#define TAS5754M_PAGE_LEN  0x80
> > > +#define TAS5754M_PAGE_BASE(n)  (TAS5754M_VIRT_BASE + (TAS5754M_PAGE_LEN * n))

> > > +#define TAS5754M_PAGE              0

> > There's no mention of paging in the regmap description for the driver
> > which feels like it's asking for problems.

> I think, it's defined in the correct way. Where/when exactly do you see a problem?

If there is paging going on and the regmap code doesn't know about it
then that makes it seem likely that the regmap code is going to get
confused about what's going on with the device.  What makes you say that
"it's defined in the correct way" if there's no mention of paging in the
regmap config?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20220126/d49201ff/attachment.sig>