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:
signature.asc
Description: PGP signature
_______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
