Re: [PATCH] ASoC: tas5805m: Add TAS5805M amplifier driver

[Mark Brown]

On Tue, May 05, 2020 at 10:36:29AM +0000, Leslie Hsia(åéé_Pegatron) wrote:

> +struct tas5805m_priv {
> +       struct regmap *regmap;
> +       /* mutex for getting the mutex and release */
> +       struct mutex lock;
> +};

This actually appears to be for device initialization somehow - the
comment isn't super enlightening.  It's not clear to me that there are
any potential races here - the PM stuff and device probe and removal are
already locked further up the stack.

> +/* Initialize the TAS5805M and set the volume to -6.5db,
> + * and set it to Play mode.
> + */
> +static const struct reg_sequence tas5805m_init_dsp[] = {
> +       { TAS5805M_REG_DEV_CTL2, TAS5805M_DEV_STAT_DSLEEP },
> +       { TAS5805M_REG_DEV_CTL2, TAS5805M_DEV_STAT_HIZ },
> +       { TAS5805M_REG_DEV_CTL2, TAS5805M_DEV_STAT_DSLEEP },
> +       /* set volume to -6.5dB */
> +       { TAS5805M_REG_VOL_CTL,  TAS5805M_DIG_VOL_DB },
> +       { TAS5805M_REG_DEV_CTL2, TAS5805M_DEV_STAT_PLAY },
> +};

You should use the chip defaults unless the configuration is so obvious
that almost all users would want the same setting.  This avoids the
kernel having to take decisions about which use case to support, a
volume that makes sense for one system may not make sense for others.

> +/* Setting the TAS5805M state and save the config in the default page. */
> +static int tas5805m_set_device_state(struct tas5805m_priv *tas5805m,
> +                                       int state)
> +{
> +       int ret = 0;
> +
> +       ret = regmap_write(tas5805m->regmap, TAS5805M_REG_DEV_RESET,
> +                               TAS5805M_DEV_STAT_RESET);
> +       if (ret != 0)
> +               return -EINVAL;
> +
> +       /* Saving the config to the default page of the default book */
> +       ret = regmap_write(tas5805m->regmap,
> +                       TAS5805M_REG_DEV_BOOK,
> +                       TAS5805M_DEV_BOOK_DEFAULT_PAGE);

regmap has support for paging, you should probably describe the pages in
the regmap config.  Right now I don't think things are going to work
well since there are no pages described but caching is enabled which
means that regmap will cache values that are getting paged out.

> +static int tas5805m_i2c_remove(struct i2c_client *i2c)
> +{
> +       return 0;
> +}

You can just delete empty functions like this, if things can safely be
left empty then they can just be removed entirely.

> +MODULE_DEVICE_TABLE(acpi, tas5805m_acpi_match);
> +#else
> +#define st_accel_acpi_match NULL
> +#endif

This is redundant, ACPI_PTR() won't reference the value unless ACPI is
enabled.

> +#else
> +#define tas5805m_dsp_power_pm_ops NULL
> +#endif
> +
> +static struct i2c_driver tas5805m_i2c_driver = {
> +       .driver = {
> +               .name = TAS5805M_DRV_NAME,
> +               .acpi_match_table = ACPI_PTR(tas5805m_acpi_match),
> +               .pm = tas5805m_dsp_power_pm_ops,

Use SET_SYSTEM_SLEEP_PM_OPS() and remove the else case above.

Attachment: signature.asc
Description: PGP signature