On Mon, Apr 11, 2022 at 03:56:52PM +0800, Raphael-Xu wrote:
> static void tas27xx_reset(struct tas27xx_priv *tas27xx)
> {
> if (tas27xx->reset_gpio) {
> gpiod_set_value_cansleep(tas27xx->reset_gpio, 0);
> - msleep(20);
> + usleep_range(2000, 2050);
> gpiod_set_value_cansleep(tas27xx->reset_gpio, 1);
> + usleep_range(5000, 5050);
> }
This looks like an unrelated but good fix? It should be a separate
patch.
> + TAS27XX_PWR_CTRL,
> + TAS27XX_PWR_CTRL_MASK,
> + TAS27XX_PWR_CTRL_SHUTDOWN);
> + if (ret >= 0) {
> + tas27xx->mb_power_up = false;
> + ret = 0;
mb_power_up seems to never be read - what purpose does it serve?
> - return 0;
> + if (ret < 0)
> + pr_err("%s:%u:errCode:0x%0x:set BIAS error\n",
> + __func__, __LINE__, ret);
Please use something like normal kernel logging styles - use dev_err()
like the rest of the function, no __func__ or __line__ and log the error
code as an integer. In general please try to follow the kernel coding
style.
> + mutex_unlock(&tas27xx->codec_lock);
It's not clear what this lock is protecting - it seems to be serialising
things that the core will already ensure don't run concurrently. It at
least needs some documentation. If it's not needed at all then a lot of
the diff could be dropped which would help a lot since as far as I can
see the bulk of the changes here are for adding this lock so it's hard
to see the device specific changes. I'd also suggest pulling this out
into a separate patch.
> - return 0;
> +EXIT:
> + mutex_unlock(&tas27xx->codec_lock);
Normal coding style for labels is lower case.
> {
> - struct tas27xx_priv *tas27xx =
> + struct tas27xx_priv *tas27xx =
This looks like an unneeded whitespace change? There's a lot of these
where I can't spot what the actual change is...
> }
> -#else
> -#define tas27xx_codec_suspend NULL
> -#define tas27xx_codec_resume NULL
> #endif
This (and the related change below adding ifdefs for the use) are an
unrelated stylistic change and should be in a separate patch if they
make sense though I can't see any reason for them? It's generally
considered better style not to need the ifdefs.
> static int tas27xx_mute(struct snd_soc_dai *dai, int mute, int direction)
> {
> struct snd_soc_component *component = dai->component;
> - int ret;
> + struct tas27xx_priv *tas27xx =
> + snd_soc_component_get_drvdata(component);
> + int ret = 0;
> +
> + mutex_lock(&tas27xx->codec_lock);
>
> + if (!mute) {
> + ret = snd_soc_component_read(component,
> + TAS27XX_CLK_CFG);
> + if (ret < 0) {
> + dev_err(tas27xx->dev,
> + "%s:%u:errCode:0x%x read "
> + "TAS27XX_CLK_CFG error\n",
> + __func__, __LINE__, ret);
> + goto EXIT;
> + }
> + if ((ret & TAS27XX_CLK_CFG_MASK) != TAS27XX_CLK_CFG_ENABLE) {
> + ret = snd_soc_component_update_bits(component,
> + TAS27XX_CLK_CFG,
> + TAS27XX_CLK_CFG_MASK,
> + TAS27XX_CLK_CFG_ENABLE);
> + if (ret < 0) {
> + dev_err(tas27xx->dev,
> + "%s:%u: Failed to CLK_CFG_ENABLE\n",
> + __func__, __LINE__);
> + goto EXIT;
> + }
> + usleep_range(3000, 3050);
> + }
This clock configuration on mute is suprising - what's going on here?
It's an unusal thing to do.
> ret = snd_soc_component_update_bits(component,
> - TAS27XX_TDM_CFG2,
> - TAS27XX_TDM_CFG2_RXW_MASK,
> - TAS27XX_TDM_CFG2_RXW_16BITS);
> + TAS27XX_TDM_CFG2,
> + TAS27XX_TDM_CFG2_RXW_MASK,
> + TAS27XX_TDM_CFG2_RXW_16BITS);
Unrelated indentation change.
> @@ -522,26 +648,54 @@ static int tas27xx_codec_probe(struct snd_soc_component *component)
> gpiod_set_value_cansleep(tas27xx->sdz_gpio, 1);
>
> tas27xx_reset(tas27xx);
> + usleep_range(5000, 5050);
There's already a sleep in the reset function, why does this caller need
an extra one?
> - ret = snd_soc_component_update_bits(tas27xx->component,
> - TAS27XX_TDM_CFG5,
> + ret = snd_soc_component_update_bits(component,
The changes to use a local component variable could probably usefully be
a separate patch, it obscures everything else that's going on.
> +static bool tas27xx_volatile(struct device *dev,
> + unsigned int reg)
This should be a separate change probably, it looks like a bug fix.
> +{
> + switch (reg) {
> + case TAS27XX_SW_RST:
> + case TAS27XX_PWR_CTRL:
> + case TAS27XX_PAGE:
It's suprising that the power control and paging registers would be
volatile? Same for some of the other registers...
> + case TAS27XX_DVC:
> + case TAS27XX_CHNL_0:
> + case TAS27XX_TDM_CFG0:
> + case TAS27XX_TDM_CFG1:
> + case TAS27XX_TDM_CFG2:
> + case TAS27XX_TDM_CFG3:
> + case TAS27XX_TDM_CFG5:
> + case TAS27XX_TDM_CFG6:
...like the TDM configuration.
> static const struct i2c_device_id tas27xx_i2c_id[] = {
> { "tas2764", TAS2764},
> + { "tas2780", TAS2780},
> { }
I don't see any runtime differences between the two variants - nothing
is keyed off the ID?
> static const struct of_device_id tas27xx_of_match[] = {
> { .compatible = "ti,tas2764" },
> + { .compatible = "ti,tas2780" },
> {},
> };
If it were we'd need to also have something here.
Attachment:
signature.asc
Description: PGP signature
