Re: [PATCH 1/2] ASoC: nau8825: non-clock jack detection for power saving at standby

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2904 bytes --]

On Fri, Apr 29, 2016 at 04:15:17PM +0800, John Hsu wrote:

> @@ -614,8 +623,10 @@ int nau8825_enable_jack_detect(struct snd_soc_codec *codec,
>  		NAU8825_HSD_AUTO_MODE | NAU8825_SPKR_DWN1R | NAU8825_SPKR_DWN1L,
>  		NAU8825_HSD_AUTO_MODE | NAU8825_SPKR_DWN1R | NAU8825_SPKR_DWN1L);
>  
> -	regmap_update_bits(regmap, NAU8825_REG_INTERRUPT_MASK,
> -		NAU8825_IRQ_HEADSET_COMPLETE_EN | NAU8825_IRQ_EJECT_EN, 0);
> +	/* Change jack type detection interruption to non-clock architecture
> +	 * for power saving less 1mW. Move these configuration about inter-
> +	 * ruption at auto mode to auto irq setup function.
> +	 */
>  

This comment is about the change you're making to the code rather than
something that should be in the code.

> +	/* Clear all interruption status */
> +	nau8825_int_status_clear_all(regmap);
> +
> +	/* Mask all interruptions except jack insertion interruption */
> +	regmap_write(regmap, NAU8825_REG_INTERRUPT_DIS_CTRL, 0xfffe);

So if any other interrupts occur then things will break...

> -	regmap_read(regmap, NAU8825_REG_IRQ_STATUS, &active_irq);
> +	if (regmap_read(regmap, NAU8825_REG_IRQ_STATUS, &active_irq)) {
> +		dev_err(nau8825->dev, "failed to clear interruption\n");
> +		return IRQ_NONE;
> +	}

This is a read, not a write, and it's better to report the error code if
the read failed.  This should probably be a separate patch.

>  static const struct regmap_config nau8825_regmap_config = {
> -	.val_bits = 16,
> -	.reg_bits = 16,
> +	.val_bits = NAU8825_REG_DATA_LEN,
> +	.reg_bits = NAU8825_REG_ADDR_LEN,

This appears to be an unrelated change which it's not clear helps the
reader, these defines only seem to be used in htis one place.

> @@ -1134,7 +1244,26 @@ static int nau8825_configure_sysclk(struct nau8825 *nau8825, int clk_id,
>  	struct regmap *regmap = nau8825->regmap;
>  	int ret;
>  
> +	if (!nau8825_is_jack_inserted(nau8825->regmap) &&
> +		clk_id != NAU8825_CLK_DIS) {
> +		/* For power saving less 1mW, the jack type detection inter-
> +		 * ruption changes to non-clock architecture. Therefore, the
> +		 * clock should be disabled and not allowed to config any clock
> +		 * when no headset connected.
> +		 */
> +		dev_warn(nau8825->dev, "Souldn't enable any clock when no headset connected\n");
> +		return 0;
> +	}

This is ignoring the attempt to set up a clock but returning success
which is going to break things, printing the warning is dubious (a
system could be built without detection for example, or a speaker driver
connected) but probably OK in itself but the fact that we don't tell the
caller may make things worse.

> +		nau8825_eject_jack(nau8825);
> +		snd_soc_jack_report(nau8825->jack, 0, SND_JACK_HEADSET);
> +	}
> +	enable_irq(nau8825->irq);

The interrupt is optional (that bug appears to be already present in the
driver but should be fixed).

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]