Re: [PATCH v5] ASoC: Add MediaTek MT6660 Speaker Amp Driver

[Mark Brown]

On Fri, Jan 10, 2020 at 04:21:36PM +0800, Jeff Chang wrote:

> +	- rt,init_setting_num : The initial register setting element number.
> +
> +	- rt,init_setting_addr : the addreses array for INIT Setting table.
> +
> +	- rt,init_setting_mask : the mask array for INIT Setting table.
> +
> +	- rt,init_setting_val : the value array for INIT Setting table.

I'm not 100% clear what this is indended to be used for but given
that it's just raw register writes it's hard to see how it can be
used well in a general driver - it basically means we have no
idea how the chip might be configured in any given system which
*might* be fine but it could break some assumption the driver
makes about the state of the system on some particular OS or
version.  If there are some system specific tuning parameters
that need to be set as raw values it would be safer to do that by
just including the data in DT (eg, "X bytes of data to be written
starting at register Y to tune component Z" type stuff) or by
exposing individual parameters for things that are documentable.
Allowing a completely unconstrained sequence of register writes
doesn't seem like a good idea.

If possible it might be best to send a version of the driver that
doesn't let this stuff be configured at all then a patch on top
of that which configures all this stuff, the rest of the driver
looks good now.

Attachment: signature.asc
Description: PGP signature