On Tue, Apr 05, 2022 at 09:12:20PM +0800, Zhu Ning wrote:
> The ES8326 codec is not compatible with ES8316 and requires a dedicated driver.
This looks mostly good - a few things below but nothing huge that should
require big restructurings or anything.
> +++ b/sound/soc/codecs/es8326.c
> @@ -0,0 +1,753 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * es8326.c -- es8326 ALSA SoC audio driver
> + * Copyright Everest Semiconductor Co., Ltd
Please make the entire comment a C++ one so things look more
intentional.
> +#include <linux/clk.h>
> +#include <linux/gpio.h>
The driver doesn't actually seem to use the GPIO APIs?
> + /* The lock protects the situation that an irq is generated
> + * while the previous irq is still being processed.
> + */
> + struct mutex lock;
It doesn't seem to, it seems to protect registrations and
deregistrations from racing against each other? The actual interrupt
handling doesn't use it.
> + SOC_SINGLE_TLV("ADC Analog PGA Gain", ES8326_PAGGAIN_23, 0, 10, 0, adc_analog_pga_tlv),
Control name should end in Volume - see control-names.rst.
> + regmap_write(es8326->regmap, ES8326_CLK_CTL_01, ES8326_CLK_ON);
> + /* Two channel ADC */
> + regmap_write(es8326->regmap, ES8326_PULLUP_CTL_F9, 0x02);
> + regmap_write(es8326->regmap, ES8326_CLK_INV_02, 0x00);
> + regmap_write(es8326->regmap, ES8326_CLK_DIV_CPC_0C, 0x1F);
> + regmap_write(es8326->regmap, ES8326_CLK_TRI_0E, 0x00);
> + regmap_write(es8326->regmap, ES8326_CLK_VMIDS1_10, 0xC8);
> + regmap_write(es8326->regmap, ES8326_CLK_VMIDS2_11, 0x88);
> + regmap_write(es8326->regmap, ES8326_CLK_CAL_TIME_12, 0x20);
> + regmap_write(es8326->regmap, ES8326_DAC_MUTE_14, 0x00);
> + regmap_write(es8326->regmap, ES8326_ANA_LOWPOWER_19, 0xF0);
> + regmap_write(es8326->regmap, ES8326_SYS_BIAS_1D, 0x08);
> + regmap_write(es8326->regmap, ES8326_DAC2HPMIX_25, 0x22);
> + regmap_write(es8326->regmap, ES8326_ADC_SCALE_29, 0x00);
> + regmap_write(es8326->regmap, ES8326_ADC1_SRC_2A, es8326->mic1_src);
> + regmap_write(es8326->regmap, ES8326_ADC2_SRC_2B, es8326->mic2_src);
> + regmap_write(es8326->regmap, ES8326_HP_CAL_4A, 0x00);
> + regmap_write(es8326->regmap, ES8326_DAC_DSM_4D, 0x08);
> + regmap_write(es8326->regmap, ES8326_DAC_RAMPRATE_4E, 0x20);
> + regmap_write(es8326->regmap, ES8326_DAC_VPPSCALE_4F, 0x15);
> + regmap_write(es8326->regmap, ES8326_HPJACK_TIMER_56, 0x88);
> + regmap_write(es8326->regmap, ES8326_HP_DET_57,
> + ES8326_HP_DET_SRC_PIN9 | es8326->jack_pol);
> + regmap_write(es8326->regmap, ES8326_INT_SOURCE_58, 0x08);
> + regmap_write(es8326->regmap, ES8326_INTOUT_IO_59, 0x45);
> + regmap_write(es8326->regmap, ES8326_RESET_00, ES8326_CSM_ON);
> + snd_soc_component_update_bits(component, ES8326_PAGGAIN_23,
> + ES8326_MIC_SEL_MASK, ES8326_MIC1_SEL);
This looks like some or all of it is doing routing which would normally
be configured from userspace using controls - the ADC1/2 sources and
DAC controls jump out for example. Some of it like the pullups might
make sense fixed, or as DT properties like the jack detect polarity is
here, but not all of it.
> +#ifdef CONFIG_OF
> +static const struct i2c_device_id es8326_i2c_id[] = {
> + {"es8326", 0 },
> + {}
> +};
> +MODULE_DEVICE_TABLE(i2c, es8326_i2c_id);
> +#endif
This should be unconditional, the ifdefs should be around
es8326_of_match instead.
Attachment:
signature.asc
Description: PGP signature
