From: Nick <nick.li@foursemi.com> Date: 2025-07-03 03:57:30
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs210x.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
@@ -0,0 +1,95 @@+# SPDX-License-Identifier: GPL-2.0-only+%YAML1.2+---+$id:http://devicetree.org/schemas/sound/foursemi,fs210x.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:FourSemi FS2104/5S Digital Audio Amplifier++maintainers:+-Nick <nick.li@foursemi.com>++description:|+The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,+Digital Input Class-D Power Amplifier with Enhanced Signal Processing.+The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,+Digital Input Class-D Power Amplifier with Enhanced Signal Processing.++properties:+compatible:+enum:+-foursemi,fs2104+-foursemi,fs2105s++reg:+maxItems:1+description:|+I2C address of the amplifier. Refer to datasheet for possible values:+0x68/0x69/0x6A/0x6B++clocks:+items:+-description:The clock of I2S BCLK++clock-names:+items:+-const:bclk++'#sound-dai-cells':+const:0++pvdd-supply:+description:|+Regulator for power supply(PVDD in datasheet).++dvdd-supply:+description:|+Regulator for digital supply(DVDD in datasheet).++sdz-gpios:+maxItems:1+description:|+SDZ(Shut Down) pin is active low, it will power down the chip,+and then reset the chip to shut down state.++fs,fwm-name:+description:|+The name of firmware that should be loaded for this+instance. The firmware is gernerated by FourSemi's tuning tool.+$ref:/schemas/types.yaml#/definitions/string++fs,dai-name:+description:|+The name of FS210x DAI, it can be set different name when+there are multiple FS210x devices in the system.+$ref:/schemas/types.yaml#/definitions/string++required:+-compatible+-reg+-sdz-gpios+-fs,fwm-name+-'#sound-dai-cells'++additionalProperties:false++examples:+-|+#include <dt-bindings/gpio/gpio.h>+i2c {+#address-cells = <1>;+#size-cells = <0>;+audio-codec@68 {+compatible = "foursemi,fs2105s";+reg = <0x68>;+pvdd-supply = <&pvdd_supply>;+dvdd-supply = <&dvdd_supply>;+sdz-gpios = <&gpio 18 GPIO_ACTIVE_LOW>;+pinctrl-names = "default";+pinctrl-0 = <&fs210x_pins_default>;+fs,fwm-name = "fs2105s-btl-2p0-0s.bin";+clocks = <&clocks 18>;+clock-names = "bclk";+#sound-dai-cells = <0>;+};+};
From: Nick <nick.li@foursemi.com> Date: 2025-07-03 03:57:32
From: Nick Li <nick.li@foursemi.com>
The FS2104/5S are FourSemi digital audio amplifiers
with I2C control. They are Inductor-Less, Stereo, Closed-Loop,
Digital Input Class-D Power Amplifiers with Enhanced Signal Processing.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
sound/soc/codecs/Kconfig | 11 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/fs210x.c | 1616 +++++++++++++++++++++++++++++++++++++
sound/soc/codecs/fs210x.h | 79 ++
4 files changed, 1708 insertions(+)
create mode 100644 sound/soc/codecs/fs210x.c
create mode 100644 sound/soc/codecs/fs210x.h
@@ -0,0 +1,1616 @@+// SPDX-License-Identifier: GPL-2.0+/*+*fs210x.c--DriverfortheFS2104/5SAudioAmplifier+*+*Copyright(C)2016-2025ShanghaiFourSemiSemiconductorCo.,Ltd.+*/++#include<linux/clk.h>+#include<linux/delay.h>+#include<linux/i2c.h>+#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/of.h>+#include<linux/of_gpio.h>+#include<linux/regmap.h>+#include<linux/regulator/consumer.h>+#include<linux/workqueue.h>+#include<sound/soc.h>+#include<sound/tlv.h>++#include"fs210x.h"+#include"fs-amp-lib.h"++#define FS210X_DRV_VERSION "v1.0.6"+#define FS210X_DEFAULT_FWM_NAME "fs210x_fwm.bin"+#define FS210X_DEFAULT_DAI_NAME "fs210x-aif"+#define FS2105S_DEVICE_ID 0x20 /* FS2105S */+#define FS210X_DEVICE_ID 0x45 /* FS2104 */+#define FS210X_REG_MAX 0xF8+#define FS210X_VOLUME_MIN 0+#define FS210X_VOLUME_MAX 511+#define FS210X_INIT_SCENE 0+#define FS210X_DEFAULT_SCENE 1+#define FS210X_START_DELAY_MS 5+#define FS210X_FAULT_CHECK_INTERVAL_MS 2000+#define FS2105S_RATES (SNDRV_PCM_RATE_32000 | \+SNDRV_PCM_RATE_44100|\+SNDRV_PCM_RATE_48000|\+SNDRV_PCM_RATE_88200|\+SNDRV_PCM_RATE_96000)+#define FS210X_RATES (SNDRV_PCM_RATE_16000 | FS2105S_RATES)+#define FS210X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \+SNDRV_PCM_FMTBIT_S24_LE|\+SNDRV_PCM_FMTBIT_S24_3LE|\+SNDRV_PCM_FMTBIT_S32_LE)+#define FS210X_NUM_SUPPLIES ARRAY_SIZE(fs210x_supply_names)++staticconstchar*constfs210x_supply_names[]={+"pvdd",+"dvdd",+};++structfs210x_platform_data{+constchar*fwm_name;+constchar*dai_name;+};++structfs210x_priv{+structi2c_client*i2c;+structdevice*dev;+structregmap*regmap;+structfs210x_platform_datapdata;+structregulator_bulk_datasupplies[FS210X_NUM_SUPPLIES];+structgpio_desc*gpio_sdz;+structdelayed_workstart_work;+structdelayed_workfault_check_work;+structfs_amp_libamp_lib;+conststructfs_amp_scene*cur_scene;+structclk*clk_bclk;+unsignedintbclk;+unsignedintsrate;+intscene_id;+u16devid;+u16vol[2];/* L/R channels volume */+boolis_inited;+boolis_suspended;+boolis_bclk_on;+boolis_playing;+};++staticDEFINE_MUTEX(fs210x_mutex);++staticconststructfs_pll_divfs210x_pll_div[]={+/* bclk, pll1, pll2, pll3 */+{512000,0x006C,0x0120,0x0001},+{768000,0x016C,0x00C0,0x0001},+{1024000,0x016C,0x0090,0x0001},+{1536000,0x016C,0x0060,0x0001},+{2048000,0x016C,0x0090,0x0002},+{2304000,0x016C,0x0080,0x0002},+{3072000,0x016C,0x0090,0x0003},+{4096000,0x016C,0x0090,0x0004},+{4608000,0x016C,0x0080,0x0004},+{6144000,0x016C,0x0090,0x0006},+{8192000,0x016C,0x0090,0x0008},+{9216000,0x016C,0x0090,0x0009},+{12288000,0x016C,0x0090,0x000C},+{16384000,0x016C,0x0090,0x0010},+{18432000,0x016C,0x0090,0x0012},+{24576000,0x016C,0x0090,0x0018},+{1411200,0x016C,0x0060,0x0001},+{2116800,0x016C,0x0080,0x0002},+{2822400,0x016C,0x0090,0x0003},+{4233600,0x016C,0x0080,0x0004},+{5644800,0x016C,0x0090,0x0006},+{8467200,0x016C,0x0090,0x0009},+{11289600,0x016C,0x0090,0x000C},+{16934400,0x016C,0x0090,0x0012},+{22579200,0x016C,0x0090,0x0018},+{2000000,0x017C,0x0093,0x0002},+};++staticintfs210x_bclk_set(structfs210x_priv*fs210x,boolon)+{+intret=0;++if(!fs210x||!fs210x->dev)+return-EINVAL;++if(!fs210x->clk_bclk)+return0;++if((fs210x->is_bclk_on^on)==0)+return0;++dev_dbg(fs210x->dev,"bclk switch %s\n",on?"on":"off");++if(on){+clk_set_rate(fs210x->clk_bclk,fs210x->bclk);+ret=clk_prepare_enable(fs210x->clk_bclk);+fs210x->is_bclk_on=true;+usleep_range(2000,2050);/* >= 2ms */+}else{+clk_disable_unprepare(fs210x->clk_bclk);+fs210x->is_bclk_on=false;+}++returnret;+}++staticintfs210x_reg_write(structfs210x_priv*fs210x,+u8reg,u16val)+{+intret;++ret=regmap_write(fs210x->regmap,reg,val);+if(ret){+dev_err(fs210x->dev,"Failed to write %02Xh: %d\n",reg,ret);+returnret;+}++dev_dbg(fs210x->dev,"RW: %02x %04x\n",reg,val);++return0;+}++staticintfs210x_reg_read(structfs210x_priv*fs210x,+u8reg,u16*pval)+{+unsignedintval;+intret;++ret=regmap_read(fs210x->regmap,reg,&val);+if(ret){+dev_err(fs210x->dev,"Failed to read %02Xh: %d\n",reg,ret);+returnret;+}++if(pval)+*pval=(u16)val;++dev_dbg(fs210x->dev,"RR: %02x %04x\n",reg,val);++return0;+}++staticintfs210x_reg_update_bits(structfs210x_priv*fs210x,+u8reg,u16mask,u16val)+{+intret;++ret=regmap_update_bits(fs210x->regmap,reg,mask,val);+if(ret){+dev_err(fs210x->dev,"Failed to update %02Xh: %d\n",reg,ret);+returnret;+}++dev_dbg(fs210x->dev,"RU: %02x %04x %04x\n",reg,mask,val);++return0;+}++staticintfs210x_reg_bulk_write(structfs210x_priv*fs210x,+u8reg,constvoid*val,u32size)+{+intret;++ret=regmap_bulk_write(fs210x->regmap,reg,val,size/2);+if(ret){+dev_err(fs210x->dev,"Failed to bulk write %02Xh: %d\n",+reg,ret);+returnret;+}++dev_dbg(fs210x->dev,"BW: %02x %d\n",reg,size);++return0;+}++staticintfs210x_reg_dump(structfs210x_priv*fs210x)+{+u16val[8];/* 8 registers per line */+u8idx,reg;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++#define FS210X_REG_DUMP_MAX 0xCF+for(reg=0x00;reg<=FS210X_REG_DUMP_MAX;reg++){+/* Print 8 register values per line */+idx=(reg&0x7);+ret=fs210x_reg_read(fs210x,reg,val+idx);+if(ret)+break;+if(idx!=0x7&®!=FS210X_REG_DUMP_MAX)+continue;+dev_info(fs210x->dev,+"%02Xh: %04x %04x %04x %04x %04x %04x %04x %04x\n",+(reg&0xF8),val[0],val[1],val[2],+val[3],val[4],val[5],val[6],val[7]);+}++returnret;+}++staticinlineintfs210x_write_reg_val(structfs210x_priv*fs210x,+conststructfs_reg_val*regv)+{+returnfs210x_reg_write(fs210x,regv->reg,regv->val);+}++staticinlineintfs210x_write_reg_bits(structfs210x_priv*fs210x,+conststructfs_reg_bits*regu)+{+returnfs210x_reg_update_bits(fs210x,+regu->reg,+regu->mask,+regu->val);+}++staticinlineintfs210x_set_cmd_pkg(structfs210x_priv*fs210x,+conststructfs_cmd_pkg*pkg,+unsignedint*offset)+{+intdelay_us;++if(pkg->cmd>=0x00&&pkg->cmd<=FS210X_REG_MAX){+*offset=sizeof(pkg->regv);+returnfs210x_write_reg_val(fs210x,&pkg->regv);+}elseif(pkg->cmd==FS_CMD_UPDATE){+*offset=sizeof(pkg->regb);+returnfs210x_write_reg_bits(fs210x,&pkg->regb);+}elseif(pkg->cmd==FS_CMD_DELAY){+if(pkg->regv.val>FS_CMD_DELAY_MS_MAX)+return-ENOTSUPP;+delay_us=pkg->regv.val*1000;+usleep_range(delay_us,delay_us+50);+*offset=sizeof(pkg->regv);+return0;+}++dev_err(fs210x->dev,"Invalid pkg cmd: %d\n",pkg->cmd);++return-ENOTSUPP;+}++staticintfs210x_reg_write_table(structfs210x_priv*fs210x,+conststructfs_reg_table*reg)+{+conststructfs_cmd_pkg*pkg;+unsignedintindex,offset;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++if(!reg||reg->size==0)+return-EFAULT;++for(index=0;index<reg->size;index+=offset){+pkg=(structfs_cmd_pkg*)(reg->buf+index);+ret=fs210x_set_cmd_pkg(fs210x,pkg,&offset);+if(ret){+dev_err(fs210x->dev,"Failed to set cmd pkg: %02X-%d\n",+pkg->cmd,ret);+returnret;+}+}++if(index!=reg->size){+dev_err(fs210x->dev,"Invalid reg table size: %d-%d\n",+index,reg->size);+return-EFAULT;+}++return0;+}++staticintfs210x_dev_play(structfs210x_priv*fs210x)+{+intret;++if(!fs210x->is_inited)+return-EFAULT;++if(fs210x->is_playing)+return0;++ret=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PLAY);+if(!ret)+fs210x->is_playing=true;++usleep_range(10000,10050);/* >= 10ms */++returnret;+}++staticintfs210x_dev_stop(structfs210x_priv*fs210x)+{+intret;++if(!fs210x->is_inited)+return-EFAULT;++if(!fs210x->is_playing)+return0;++ret=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PWDN);+fs210x->is_playing=false;++usleep_range(30000,30050);/* >= 30ms */++returnret;+}++staticintfs210x_set_reg_table(structfs210x_priv*fs210x,+conststructfs_amp_scene*scene)+{+conststructfs_amp_scene*cur_scene;+conststructfs_reg_table*reg;++if(!fs210x||!fs210x->dev||!scene)+return-EINVAL;++cur_scene=fs210x->cur_scene;+if(!scene->reg||cur_scene==scene){+dev_dbg(fs210x->dev,"Skip writing reg table\n");+return0;+}++reg=scene->reg;+dev_dbg(fs210x->dev,"reg table size: %d\n",reg->size);++returnfs210x_reg_write_table(fs210x,reg);+}++staticintfs210x_set_woofer_table(structfs210x_priv*fs210x)+{+conststructfs_file_table*woofer;+conststructfs_fwm_table*table;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++/* NOTE: fs2105s has woofer ram only */+if(fs210x->devid!=FS2105S_DEVICE_ID)+return0;++table=fs210x->amp_lib.table[FS_INDEX_WOOFER];+if(!table){+dev_dbg(fs210x->dev,"Skip writing woofer table\n");+return0;+}++woofer=(structfs_file_table*)table->buf;+dev_dbg(fs210x->dev,"woofer table size: %d\n",woofer->size);+/* Unit of woofer data is u32(4 bytes) */+if(woofer->size==0||(woofer->size&0x3)){+dev_err(fs210x->dev,"Invalid woofer size: %d\n",+woofer->size);+return-EINVAL;+}++ret=fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS2105S_46H_CAM_BURST_W);+ret|=fs210x_reg_bulk_write(fs210x,FS210X_42H_DACEQWL,+woofer->buf,woofer->size);++returnret;+}++staticintfs210x_set_effect_table(structfs210x_priv*fs210x,+conststructfs_amp_scene*scene)+{+conststructfs_amp_scene*cur_scene;+conststructfs_file_table*effect;+inthalf_size;+intret;++if(!fs210x||!fs210x->dev||!scene)+return-EINVAL;++cur_scene=fs210x->cur_scene;+if(!scene->effect||cur_scene==scene){+dev_dbg(fs210x->dev,"Skip writing effect table\n");+return0;+}++effect=scene->effect;+dev_dbg(fs210x->dev,"effect table size: %d\n",effect->size);++/* Unit of effect data is u32(4 bytes), 2 channels */+if(effect->size==0||(effect->size&0x7)){+dev_err(fs210x->dev,"Invalid effect size: %d\n",+effect->size);+return-EINVAL;+}++half_size=effect->size/2;++/* Left channel */+ret=fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS210X_46H_CAM_BURST_L);+ret|=fs210x_reg_bulk_write(fs210x,FS210X_42H_DACEQWL,+effect->buf,half_size);+if(ret)+returnret;++/* Right channel */+ret=fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS210X_46H_CAM_BURST_R);+ret|=fs210x_reg_bulk_write(fs210x,FS210X_42H_DACEQWL,+effect->buf+half_size,half_size);++returnret;+}++staticintfs210x_access_dsp_ram(structfs210x_priv*fs210x,boolenable)+{+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++if(enable){+ret=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_HIZ);+ret|=fs210x_reg_write(fs210x,FS210X_0BH_ACCKEY,+FS210X_0BH_ACCKEY_ON);+}else{+ret=fs210x_reg_write(fs210x,FS210X_0BH_ACCKEY,+FS210X_0BH_ACCKEY_OFF);+ret|=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PWDN);+}++usleep_range(10000,10050);/* >= 10ms */++returnret;+}++staticintfs210x_write_dsp_effect(structfs210x_priv*fs210x,+conststructfs_amp_scene*scene,+intscene_id)+{+intret;++if(!fs210x||!scene)+return-EINVAL;++ret=fs210x_access_dsp_ram(fs210x,true);+if(ret){+dev_err(fs210x->dev,"Failed to access dsp: %d\n",ret);+gototag_exit;+}++ret=fs210x_set_effect_table(fs210x,scene);+if(ret){+dev_err(fs210x->dev,"Failed to set effect: %d\n",ret);+gototag_exit;+}++if(scene_id==FS210X_INIT_SCENE)+ret=fs210x_set_woofer_table(fs210x);++tag_exit:+fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS210X_46H_CAM_CLEAR);+fs210x_access_dsp_ram(fs210x,false);++returnret;+}++staticintfs210x_check_scene(structfs210x_priv*fs210x,+intscene_id,bool*skip_set)+{+structfs_amp_lib*amp_lib;++if(!fs210x||!skip_set)+return-EINVAL;++amp_lib=&fs210x->amp_lib;+if(amp_lib->scene_count==0||!amp_lib->scene){+dev_err(fs210x->dev,"There's no scene data\n");+return-EINVAL;+}++if(scene_id<0||scene_id>=amp_lib->scene_count){+dev_err(fs210x->dev,"Invalid scene_id: %d\n",scene_id);+return-EINVAL;+}++if(fs210x->scene_id==scene_id){+dev_dbg(fs210x->dev,"Skip to set same scene\n");+return0;+}++*skip_set=false;++return0;+}++staticintfs210x_set_scene(structfs210x_priv*fs210x,intscene_id)+{+conststructfs_amp_scene*scene;+boolskip_set=true;+boolis_playing;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++ret=fs210x_check_scene(fs210x,scene_id,&skip_set);+if(ret||skip_set)+returnret;++scene=fs210x->amp_lib.scene+scene_id;+dev_info(fs210x->dev,"Switch scene.%d: %s\n",+scene_id,scene->name);++is_playing=fs210x->is_playing;+if(is_playing)+fs210x_dev_stop(fs210x);++ret=fs210x_set_reg_table(fs210x,scene);+if(ret){+dev_err(fs210x->dev,"Failed to set reg: %d\n",ret);+returnret;+}++ret=fs210x_write_dsp_effect(fs210x,scene,scene_id);+if(ret){+dev_err(fs210x->dev,"Failed to write ram: %d\n",ret);+returnret;+}++fs210x->cur_scene=scene;+fs210x->scene_id=scene_id;++if(is_playing)+fs210x_dev_play(fs210x);++return0;+}++staticinlineintvolume_is_valid(u16vol)+{+return(vol<=FS210X_VOLUME_MAX);+}++staticintfs210x_set_pcm_volume(structfs210x_priv*fs210x)+{+u16vol[2];+intret;++if(fs210x->devid==FS2105S_DEVICE_ID){+/*+*FS2105SEvolumeregisters:39h(Left)/3Ah(Right):+*VOL[15:7]:0dBto-95.8125dBinstepsof-0.1875dB+*1FFh:0dB+*000h:-95.8125dB+*/+vol[0]=fs210x->vol[0]<<FS2105S_39H_VOL_SHIFT;+vol[1]=fs210x->vol[1]<<FS2105S_3AH_VOL_SHIFT;+}else{+/*+*FS2104volumeregisters:39h(Left)/3Ah(Right):+*VOL[15:6]:0dBto-131.8125dBinstepsof-0.1875dB+*2BFh:0dB+*0C0h:-95.8125dB+*000h:-131.8125dB+*OffsetbetweenFS2104andFS2105S:2BFh-1FFh=0C0h+*/+#define VOL_OFFSET (0x2BF - 0x1FF)+vol[0]=(fs210x->vol[0]+VOL_OFFSET)<<FS210X_39H_VOL_SHIFT;+vol[1]=(fs210x->vol[1]+VOL_OFFSET)<<FS210X_3AH_VOL_SHIFT;+}++ret=fs210x_reg_write(fs210x,FS210X_39H_LVOLCTRL,vol[0]);+ret|=fs210x_reg_write(fs210x,FS210X_3AH_RVOLCTRL,vol[1]);++returnret;+}++staticintfs210x_init_chip(structfs210x_priv*fs210x)+{+intscene_id;+intret;++/* i2c reset */+ret=fs210x_reg_write(fs210x,FS210X_10H_PWRCTRL,+FS210X_10H_I2C_RESET);+if(ret)+gototag_power_down;++/* Wait >= 10ms for i2c resetting */+usleep_range(10000,10050);++/* Backup scene id */+scene_id=fs210x->scene_id;+fs210x->scene_id=-1;++/* Init registers/RAM by init scene */+ret=fs210x_set_scene(fs210x,FS210X_INIT_SCENE);+if(ret)+gototag_power_down;++/*+*Ifthefirmwarehaseffectscene(s),+*weloadeffectscenebydefaultsceneorscene_id+*/+if(fs210x->amp_lib.scene_count>1){+if(scene_id<FS210X_DEFAULT_SCENE)+scene_id=FS210X_DEFAULT_SCENE;+ret=fs210x_set_scene(fs210x,scene_id);+if(ret)+gototag_power_down;+}++ret=fs210x_set_pcm_volume(fs210x);++tag_power_down:+/* Power down the device */+ret|=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PWDN);+if(!ret)+fs210x->is_inited=true;++returnret;+}++staticvoidfs210x_sdz_pin_set(structfs210x_priv*fs210x,boolactive)+{+if(!fs210x||!fs210x->gpio_sdz)+return;++/*+*TheSDZpin(ShutDown)isactivelow,whenitispulledlow,+*itwilltriggertwoproceduressequentially:+*-Powerdownthechip;+*-Resetthechiptoshutdownstate.+*NOTE:+*TheI2CcommunicationisnackuntiltheSDZpin+*ispulledupandwait10msatleast.+*/+if(active){+gpiod_set_value_cansleep(fs210x->gpio_sdz,1);+dev_dbg(fs210x->dev,"SDZ pin is OFF\n");+usleep_range(30000,30050);/* >= 30ms */+}else{+gpiod_set_value_cansleep(fs210x->gpio_sdz,0);+dev_dbg(fs210x->dev,"SDZ pin is ON\n");+usleep_range(10000,10050);/* >= 10ms */+}+}++staticintfs210x_set_i2s_params(structfs210x_priv*fs210x)+{+conststructfs_i2s_srateparams[]={+{16000,0x3},+{32000,0x7},+{44100,0x8},+{48000,0x9},+{88200,0xA},+{96000,0xB},+};+u16val;+inti,ret;++for(i=0;i<ARRAY_SIZE(params);i++){+if(params[i].srate!=fs210x->srate)+continue;+val=params[i].i2ssr<<FS210X_17H_I2SSR_SHIFT;+ret=fs210x_reg_update_bits(fs210x,+FS210X_17H_I2SCTRL,+FS210X_17H_I2SSR_MASK,+val);+returnret;+}++dev_err(fs210x->dev,"Invalid sample rate: %d\n",fs210x->srate);++return-EINVAL;+}++staticintfs210x_get_pll_div(structfs210x_priv*fs210x,+conststructfs_pll_div**pll_div)+{+inti;++if(!fs210x||!pll_div)+return-EINVAL;++for(i=0;i<ARRAY_SIZE(fs210x_pll_div);i++){+if(fs210x_pll_div[i].bclk!=fs210x->bclk)+continue;+*pll_div=fs210x_pll_div+i;+return0;+}++dev_err(fs210x->dev,"No PLL table for bclk: %d\n",fs210x->bclk);++return-EFAULT;+}++staticintfs210x_set_hw_params(structfs210x_priv*fs210x)+{+conststructfs_pll_div*pll_div;+intret;++ret=fs210x_set_i2s_params(fs210x);+if(ret){+dev_err(fs210x->dev,"Failed to set i2s params: %d\n",ret);+returnret;+}++/* Set pll params */+ret=fs210x_get_pll_div(fs210x,&pll_div);+if(ret)+returnret;++ret=fs210x_reg_write(fs210x,FS210X_A1H_PLLCTRL1,pll_div->pll1);+ret|=fs210x_reg_write(fs210x,FS210X_A2H_PLLCTRL2,pll_div->pll2);+ret|=fs210x_reg_write(fs210x,FS210X_A3H_PLLCTRL3,pll_div->pll3);++returnret;+}++staticintfs210x_mute(structfs210x_priv*fs210x,boolmute)+{+intret;++if(mute){+cancel_delayed_work_sync(&fs210x->fault_check_work);+cancel_delayed_work_sync(&fs210x->start_work);+mutex_lock(&fs210x_mutex);+ret=fs210x_dev_stop(fs210x);+mutex_unlock(&fs210x_mutex);+returnret;+}++/*+*Accordingtothepowerup/downsequenceofFS210x,+*theFS210xrequeststheI2Sclockhasbeenpresent+*andstable(>=2ms)beforeitplaying.+*/+if(fs210x->clk_bclk){+mutex_lock(&fs210x_mutex);+ret=fs210x_dev_play(fs210x);+mutex_unlock(&fs210x_mutex);+}else{+schedule_delayed_work(&fs210x->start_work,+msecs_to_jiffies(FS210X_START_DELAY_MS));+ret=0;+}++schedule_delayed_work(&fs210x->fault_check_work,+msecs_to_jiffies(FS210X_FAULT_CHECK_INTERVAL_MS));++returnret;+}++staticintfs210x_dai_set_fmt(structsnd_soc_dai*dai,unsignedintfmt)+{+structfs210x_priv*fs210x;++fs210x=snd_soc_component_get_drvdata(dai->component);++dev_dbg(fs210x->dev,"fmt: %x\n",fmt);++switch(fmt&SND_SOC_DAIFMT_MASTER_MASK){+caseSND_SOC_DAIFMT_CBC_CFC:+/* Only supports slave mode */+break;+default:+dev_err(fs210x->dev,"Only supports slave mode\n");+return-EINVAL;+}++return0;+}++staticintfs210x_dai_hw_params(structsnd_pcm_substream*substream,+structsnd_pcm_hw_params*params,+structsnd_soc_dai*dai)+{+structfs210x_priv*fs210x;+intchn_num;+intret;++if(substream->stream!=SNDRV_PCM_STREAM_PLAYBACK)+return0;++fs210x=snd_soc_component_get_drvdata(dai->component);++fs210x->srate=params_rate(params);+fs210x->bclk=snd_soc_params_to_bclk(params);+chn_num=params_channels(params);+if(chn_num==1)/* mono */+fs210x->bclk*=2;/* I2S bus has 2 channels */++dev_info(fs210x->dev,"hw params: %d-%d-%d\n",+fs210x->srate,chn_num,fs210x->bclk);++/* The FS2105S can't support 16kHz sample rate. */+if(fs210x->devid==FS2105S_DEVICE_ID&&fs210x->srate==16000)+return-ENOTSUPP;++mutex_lock(&fs210x_mutex);+ret=fs210x_set_hw_params(fs210x);+mutex_unlock(&fs210x_mutex);+if(ret)+dev_err(fs210x->dev,"Failed to set hw params: %d\n",ret);++returnret;+}++staticintfs210x_dai_mute(structsnd_soc_dai*dai,intmute,intstream)+{+structfs210x_priv*fs210x;+intret;++if(stream!=SNDRV_PCM_STREAM_PLAYBACK)+return0;++fs210x=snd_soc_component_get_drvdata(dai->component);++if(!fs210x->is_inited)+return-EFAULT;++if(mute){+ret=fs210x_mute(fs210x,true);+fs210x_bclk_set(fs210x,false);+dev_info(fs210x->dev,"playback mute\n");+}else{+fs210x_bclk_set(fs210x,true);+ret=fs210x_mute(fs210x,false);+dev_info(fs210x->dev,"playback unmute\n");+}++returnret;+}++staticvoidfs210x_start_work(structwork_struct*work)+{+structfs210x_priv*fs210x;+intret;++fs210x=container_of(work,structfs210x_priv,start_work.work);++mutex_lock(&fs210x_mutex);++ret=fs210x_dev_play(fs210x);+if(ret)+dev_err(fs210x->dev,"Failed to start playing: %d\n",ret);++mutex_unlock(&fs210x_mutex);+}++staticvoidfs210x_fault_check_work(structwork_struct*work)+{+structfs210x_priv*fs210x;+u16status;+intret;++fs210x=container_of(work,structfs210x_priv,fault_check_work.work);++mutex_lock(&fs210x_mutex);+if(!fs210x->is_inited||!fs210x->is_playing){+mutex_unlock(&fs210x_mutex);+return;+}++ret=fs210x_reg_read(fs210x,FS210X_05H_ANASTAT,&status);+mutex_unlock(&fs210x_mutex);+if(ret)+return;++if(!(status&FS210X_05H_PVDD_MASK))+dev_err(fs210x->dev,"PVDD fault\n");+if(status&FS210X_05H_OCDL_MASK)+dev_err(fs210x->dev,"OC detected\n");+if(status&FS210X_05H_UVDL_MASK)+dev_err(fs210x->dev,"UV detected\n");+if(status&FS210X_05H_OVDL_MASK)+dev_err(fs210x->dev,"OV detected\n");+if(status&FS210X_05H_OTPDL_MASK)+dev_err(fs210x->dev,"OT detected\n");+if(status&FS210X_05H_OCRDL_MASK)+dev_err(fs210x->dev,"OCR detected\n");+if(status&FS210X_05H_OCLDL_MASK)+dev_err(fs210x->dev,"OCL detected\n");+if(status&FS210X_05H_DCRDL_MASK)+dev_err(fs210x->dev,"DCR detected\n");+if(status&FS210X_05H_DCLDL_MASK)+dev_err(fs210x->dev,"DCL detected\n");+if(status&FS210X_05H_SRDL_MASK)+dev_err(fs210x->dev,"SR detected\n");+if(status&FS210X_05H_OTWDL_MASK)+dev_err(fs210x->dev,"OTW detected\n");+if(!(status&FS210X_05H_AMPS_MASK))+dev_err(fs210x->dev,"Amplifier unready\n");+if(!(status&FS210X_05H_PLLS_MASK))+dev_err(fs210x->dev,"PLL unlock\n");+if(!(status&FS210X_05H_ANAS_MASK))+dev_err(fs210x->dev,"Analog power fault\n");++if(status!=FS210X_05H_ANASTAT_OK)+fs210x_reg_dump(fs210x);++schedule_delayed_work(&fs210x->fault_check_work,+msecs_to_jiffies(FS210X_FAULT_CHECK_INTERVAL_MS));+}++staticintfs210x_get_drvdata_from_kctrl(structsnd_kcontrol*kctrl,+structfs210x_priv**fs210x)+{+structsnd_soc_component*cmpnt;++if(!kctrl){+pr_err("fs210x: kcontrol is null\n");+return-EINVAL;+}++cmpnt=snd_soc_kcontrol_component(kctrl);+if(!cmpnt){+pr_err("fs210x: component is null\n");+return-EINVAL;+}++*fs210x=snd_soc_component_get_drvdata(cmpnt);++return0;+}++staticintfs210x_pcm_volume_info(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_info*uinfo)+{+uinfo->type=SNDRV_CTL_ELEM_TYPE_INTEGER;+uinfo->count=2;++uinfo->value.integer.min=FS210X_VOLUME_MIN;+uinfo->value.integer.max=FS210X_VOLUME_MAX;++return0;+}++staticintfs210x_pcm_volume_get(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret){+pr_err("pcm_volume_get: fs210x is null\n");+return-EINVAL;+}++mutex_lock(&fs210x_mutex);+ucontrol->value.integer.value[0]=fs210x->vol[0];+ucontrol->value.integer.value[1]=fs210x->vol[1];+mutex_unlock(&fs210x_mutex);++return0;+}++staticintfs210x_pcm_volume_put(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+long*pval;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("pcm_volume_put: fs210x is null\n");+return-EINVAL;+}++pval=ucontrol->value.integer.value;+if(!volume_is_valid(*pval)||!volume_is_valid(*(pval+1))){+dev_err(fs210x->dev,"Invalid volume: %ld-%ld\n",+*pval,*(pval+1));+return-EINVAL;+}++fs210x->vol[0]=(u16)*pval;+fs210x->vol[1]=(u16)*(pval+1);+dev_dbg(fs210x->dev,"Set volume: %d-%d\n",+fs210x->vol[0],fs210x->vol[1]);++mutex_lock(&fs210x_mutex);++if(fs210x->is_suspended){+mutex_unlock(&fs210x_mutex);+return0;+}++ret=fs210x_set_pcm_volume(fs210x);+if(ret)+dev_err(fs210x->dev,"Failed to set volume: %d\n",ret);++mutex_unlock(&fs210x_mutex);++returnret;+}++staticintfs210x_effect_scene_info(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_info*uinfo)+{+conststructfs_amp_scene*scene;+structfs210x_priv*fs210x;+constchar*name="N/A";+intidx,count;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("scene_effect_info: fs210x is null\n");+return-EINVAL;+}++uinfo->type=SNDRV_CTL_ELEM_TYPE_ENUMERATED;+uinfo->count=1;++count=fs210x->amp_lib.scene_count-1;/* Skip init scene */+if(count<1){+uinfo->value.enumerated.items=0;+return0;+}++uinfo->value.enumerated.items=count;+if(uinfo->value.enumerated.item>=count)+uinfo->value.enumerated.item=count-1;++idx=uinfo->value.enumerated.item;+scene=fs210x->amp_lib.scene+idx+1;+if(scene->name)+name=scene->name;++strscpy(uinfo->value.enumerated.name,name,strlen(name)+1);+dev_dbg(fs210x->dev,"Scene name.%d: %s\n",idx,name);++return0;+}++staticintfs210x_effect_scene_get(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+intindex;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("scene_effect_get: fs210x is null\n");+return-EINVAL;+}++/* The id of effect scene is from 1 to N. */+if(fs210x->scene_id<1)+return-EINVAL;++mutex_lock(&fs210x_mutex);+/*+*FS210xhasscene(s)asbelow:+*initscene:id=0+*effectscene(s):id=1~N(optional)+*effect_index=scene_id-1+*/+index=fs210x->scene_id-1;+ucontrol->value.integer.value[0]=index;+dev_dbg(fs210x->dev,"Get scene index: %d\n",index);+mutex_unlock(&fs210x_mutex);++return0;+}++staticintfs210x_effect_scene_put(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+intscene_id;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("scene_effect_put: fs210x is null\n");+return-EINVAL;+}++mutex_lock(&fs210x_mutex);++/*+*FS210xhasscene(s)asbelow:+*initscene:id=0(It'ssetinfs210x_init_chip()only)+*effectscene(s):id=1~N(optional)+*scene_id=effect_index+1.+*/+scene_id=ucontrol->value.integer.value[0]+1;+if(fs210x->is_suspended){+fs210x->scene_id=scene_id;+mutex_unlock(&fs210x_mutex);+return0;+}++ret=fs210x_set_scene(fs210x,scene_id);+if(ret)+dev_err(fs210x->dev,"Failed to set scene: %d\n",ret);++mutex_unlock(&fs210x_mutex);++returnret;+}++staticconststructsnd_soc_dai_opsfs210x_dai_ops={+.set_fmt=fs210x_dai_set_fmt,+.hw_params=fs210x_dai_hw_params,+.mute_stream=fs210x_dai_mute,+};++staticconststructsnd_soc_dai_driverfs210x_dai={+.name=FS210X_DEFAULT_DAI_NAME,+.playback={+.stream_name="Playback",+.channels_min=1,+.channels_max=2,+.rates=FS210X_RATES,+.formats=FS210X_FORMATS,+},+.capture={+.stream_name="Capture",+.channels_min=1,+.channels_max=2,+.rates=FS210X_RATES,+.formats=FS210X_FORMATS,+},+.ops=&fs210x_dai_ops,+.symmetric_rate=1,+.symmetric_sample_bits=1,+};++staticconststructsnd_kcontrol_newfs210x_controls[]={+FS_SOC_ENUM_EXT("PCM Playback Volume",+fs210x_pcm_volume_info,+fs210x_pcm_volume_get,+fs210x_pcm_volume_put),+};++staticconststructsnd_kcontrol_newfs210x_scene_control[]={+FS_SOC_ENUM_EXT("Effect Scene",+fs210x_effect_scene_info,+fs210x_effect_scene_get,+fs210x_effect_scene_put),+};++staticconststructsnd_soc_dapm_widgetfs210x_dapm_widgets[]={+SND_SOC_DAPM_AIF_IN("AIF IN","Playback",0,SND_SOC_NOPM,0,0),+SND_SOC_DAPM_AIF_OUT("AIF OUT","Capture",0,SND_SOC_NOPM,0,0),+SND_SOC_DAPM_OUTPUT("OUTL"),+SND_SOC_DAPM_OUTPUT("OUTR"),+SND_SOC_DAPM_INPUT("SDO"),+};++staticconststructsnd_soc_dapm_routefs210x_dapm_routes[]={+{"OUTL",NULL,"AIF IN"},+{"OUTR",NULL,"AIF IN"},+{"AIF OUT",NULL,"SDO"},+};++staticintfs210x_add_optional_controls(structfs210x_priv*fs210x,+structsnd_soc_component*cmpnt)+{+intcount;++if(!fs210x||!cmpnt)+return-EINVAL;++/*+*Ifthefirmwarehasnosceneoronlyinitscene,+*weskipaddingthismixercontrol.+*/+if(fs210x->amp_lib.scene_count<2)+return0;++count=ARRAY_SIZE(fs210x_scene_control);++returnsnd_soc_add_component_controls(cmpnt,+fs210x_scene_control,+count);+}++staticintfs210x_get_bclk(structfs210x_priv*fs210x,+structsnd_soc_component*cmpnt)+{+structclk*bclk;+intret;++bclk=devm_clk_get(fs210x->dev,"bclk");+if(IS_ERR_OR_NULL(bclk)){+ret=bclk?PTR_ERR(bclk):-ENODATA;+if(ret==-EPROBE_DEFER)+returnret;+/*+*IftheSOCdoesn'thavethebclkclocksource,+*weskipsettingthebclkclock.+*/+return0;+}++fs210x->clk_bclk=bclk;+dev_dbg(fs210x->dev,"Got I2S bclk clock\n");++return0;+}++staticintfs210x_probe(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;+intret;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return-EINVAL;++fs210x->amp_lib.dev=fs210x->dev;+fs210x->amp_lib.devid=fs210x->devid;++ret=fs_amp_load_firmware(&fs210x->amp_lib,fs210x->pdata.fwm_name);+if(ret){+dev_err(fs210x->dev,"Failed to load firmware: %d\n",ret);+returnret;+}++ret=fs210x_add_optional_controls(fs210x,cmpnt);+if(ret){+dev_err(fs210x->dev,"Failed to add opt-ctrl: %d\n",ret);+returnret;+}++INIT_DELAYED_WORK(&fs210x->fault_check_work,fs210x_fault_check_work);+INIT_DELAYED_WORK(&fs210x->start_work,fs210x_start_work);++fs210x_get_bclk(fs210x,cmpnt);++mutex_lock(&fs210x_mutex);+ret=fs210x_init_chip(fs210x);+mutex_unlock(&fs210x_mutex);+if(ret)+dev_err(fs210x->dev,"Failed to probe: %d\n",ret);++returnret;+}++staticvoidfs210x_remove(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return;++cancel_delayed_work_sync(&fs210x->start_work);+cancel_delayed_work_sync(&fs210x->fault_check_work);++dev_dbg(fs210x->dev,"Codec removed\n");+}++#ifdef CONFIG_PM+staticintfs210x_suspend(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;+intret;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return-EINVAL;++cancel_delayed_work_sync(&fs210x->start_work);+cancel_delayed_work_sync(&fs210x->fault_check_work);++mutex_lock(&fs210x_mutex);+fs210x->cur_scene=NULL;+fs210x->is_inited=false;+fs210x->is_playing=false;+fs210x->is_suspended=true;++fs210x_sdz_pin_set(fs210x,true);+mutex_unlock(&fs210x_mutex);++ret=regulator_bulk_disable(FS210X_NUM_SUPPLIES,fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to suspend: %d\n",ret);+returnret;+}++dev_info(fs210x->dev,"pm suspended\n");++return0;+}++staticintfs210x_resume(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;+intret;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return-EINVAL;++ret=regulator_bulk_enable(FS210X_NUM_SUPPLIES,fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to enable supplies: %d\n",ret);+returnret;+}++mutex_lock(&fs210x_mutex);+fs210x_sdz_pin_set(fs210x,false);++fs210x->is_suspended=false;+ret=fs210x_init_chip(fs210x);+mutex_unlock(&fs210x_mutex);++dev_info(fs210x->dev,"pm resumed\n");++returnret;+}+#else+#define fs210x_suspend NULL+#define fs210x_resume NULL+#endif // CONFIG_PM++staticconststructsnd_soc_component_driverfs210x_soc_component_dev={+.probe=fs210x_probe,+.remove=fs210x_remove,+.suspend=fs210x_suspend,+.resume=fs210x_resume,+.controls=fs210x_controls,+.num_controls=ARRAY_SIZE(fs210x_controls),+.dapm_widgets=fs210x_dapm_widgets,+.num_dapm_widgets=ARRAY_SIZE(fs210x_dapm_widgets),+.dapm_routes=fs210x_dapm_routes,+.num_dapm_routes=ARRAY_SIZE(fs210x_dapm_routes),+.use_pmdown_time=1,+};++staticconststructregmap_configfs210x_regmap={+.reg_bits=8,+.val_bits=16,+.max_register=FS210X_REG_MAX,+.val_format_endian=REGMAP_ENDIAN_BIG,+.cache_type=REGCACHE_NONE,+};++staticintfs210x_detect_device(structfs210x_priv*fs210x)+{+u16devid;+intret;++ret=fs210x_reg_read(fs210x,FS210X_03H_DEVID,&devid);+if(ret)+returnret;++fs210x->devid=HI_U16(devid);++switch(fs210x->devid){+caseFS210X_DEVICE_ID:+dev_info(fs210x->dev,"FS2104 detected\n");+break;+caseFS2105S_DEVICE_ID:+dev_info(fs210x->dev,"FS2105S detected\n");+break;+default:+dev_err(fs210x->dev,"DEVID: 0x%04X dismatch\n",devid);+return-ENODEV;+}++return0;+}++staticintfs210x_parse_dts(structfs210x_priv*fs210x,+structfs210x_platform_data*pdata)+{+structdevice_node*node=fs210x->dev->of_node;+inti,ret;++if(!node)+return0;++ret=of_property_read_string(node,"fs,fwm-name",&pdata->fwm_name);+if(ret)+pdata->fwm_name=FS210X_DEFAULT_FWM_NAME;++ret=of_property_read_string(node,"fs,dai-name",&pdata->dai_name);+if(ret)+pdata->dai_name=FS210X_DEFAULT_DAI_NAME;++fs210x->gpio_sdz=devm_gpiod_get_optional(fs210x->dev,+"sdz",GPIOD_OUT_HIGH);+if(IS_ERR_OR_NULL(fs210x->gpio_sdz)){+ret=fs210x->gpio_sdz?PTR_ERR(fs210x->gpio_sdz):-ENODATA;+fs210x->gpio_sdz=NULL;+if(ret==-EPROBE_DEFER)+returnret;+dev_dbg(fs210x->dev,"Assuming gpio_sdz is unused\n");+}else{+dev_dbg(fs210x->dev,"gpio_sdz: %d\n",+desc_to_gpio(fs210x->gpio_sdz));+}++for(i=0;i<FS210X_NUM_SUPPLIES;i++)+fs210x->supplies[i].supply=fs210x_supply_names[i];++ret=devm_regulator_bulk_get(fs210x->dev,+ARRAY_SIZE(fs210x->supplies),+fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to get supplies: %d\n",ret);+returnret;+}++return0;+}++staticintfs210x_parse_platdata(structfs210x_priv*fs210x)+{+structfs210x_platform_data*pdata;+intret;++pdata=&fs210x->pdata;+ret=fs210x_parse_dts(fs210x,pdata);+if(ret){+dev_err(fs210x->dev,"Failed to parse dts: %d\n",ret);+returnret;+}++return0;+}++staticvoidfs210x_deinit(structfs210x_priv*fs210x)+{+fs210x_sdz_pin_set(fs210x,true);+regulator_bulk_disable(FS210X_NUM_SUPPLIES,fs210x->supplies);+}++staticintfs210x_init(structfs210x_priv*fs210x)+{+intret;++ret=fs210x_parse_platdata(fs210x);+if(ret){+dev_err(fs210x->dev,"Failed to parse platdata: %d\n",ret);+returnret;+}++ret=regulator_bulk_enable(FS210X_NUM_SUPPLIES,fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to enable supplies: %d\n",ret);+returnret;+}++/* Make sure the SDZ pin is pulled down enough time. */+usleep_range(10000,10050);/* >= 10ms */+fs210x_sdz_pin_set(fs210x,false);++ret=fs210x_detect_device(fs210x);+if(ret){+fs210x_deinit(fs210x);+returnret;+}++fs210x->scene_id=-1;/* Invalid scene */+fs210x->cur_scene=NULL;+fs210x->is_playing=false;+fs210x->is_inited=false;+fs210x->is_suspended=false;+fs210x->vol[0]=FS210X_VOLUME_MAX;+fs210x->vol[1]=FS210X_VOLUME_MAX;++return0;+}++staticintfs210x_register_snd_component(structfs210x_priv*fs210x)+{+structsnd_soc_dai_driver*dai_drv;+intret;++dai_drv=devm_kmemdup(fs210x->dev,&fs210x_dai,+sizeof(fs210x_dai),GFP_KERNEL);+if(!dai_drv)+return-ENOMEM;++dai_drv->name=fs210x->pdata.dai_name;+if(fs210x->devid==FS2105S_DEVICE_ID){+dai_drv->playback.rates=FS2105S_RATES;+dai_drv->capture.rates=FS2105S_RATES;+}++ret=snd_soc_register_component(fs210x->dev,+&fs210x_soc_component_dev,+dai_drv,1);+returnret;+}++staticintfs210x_i2c_probe(structi2c_client*client)+{+structfs210x_priv*fs210x;+intret;++dev_info(&client->dev,"version: %s\n",FS210X_DRV_VERSION);++fs210x=devm_kzalloc(&client->dev,sizeof(*fs210x),GFP_KERNEL);+if(!fs210x)+return-ENOMEM;++fs210x->i2c=client;+fs210x->dev=&client->dev;+i2c_set_clientdata(client,fs210x);++fs210x->regmap=devm_regmap_init_i2c(client,&fs210x_regmap);+if(IS_ERR_OR_NULL(fs210x->regmap)){+dev_err(fs210x->dev,"Failed to get regmap\n");+ret=fs210x->regmap?PTR_ERR(fs210x->regmap):-ENODATA;+returnret;+}++mutex_lock(&fs210x_mutex);+ret=fs210x_init(fs210x);+mutex_unlock(&fs210x_mutex);+if(ret)+returnret;++ret=fs210x_register_snd_component(fs210x);+if(ret){+fs210x_deinit(fs210x);+returnret;+}++return0;+}++staticvoidfs210x_i2c_remove(structi2c_client*client)+{+structfs210x_priv*fs210x=i2c_get_clientdata(client);++snd_soc_unregister_component(fs210x->dev);+fs210x_deinit(fs210x);+}++staticconststructi2c_device_idfs210x_i2c_id[]={+{"fs2104"},+{"fs2105s"},+{}+};+MODULE_DEVICE_TABLE(i2c,fs210x_i2c_id);++#ifdef CONFIG_OF+staticconststructof_device_idfs210x_of_match[]={+{.compatible="foursemi,fs2104",},+{.compatible="foursemi,fs2105s",},+{},+};+MODULE_DEVICE_TABLE(of,fs210x_of_match);+#endif // CONFIG_OF++staticstructi2c_driverfs210x_i2c_driver={+.driver={+.name="fs210x",+.of_match_table=of_match_ptr(fs210x_of_match),+},+.id_table=fs210x_i2c_id,+.probe=fs210x_i2c_probe,+.remove=fs210x_i2c_remove,+};++module_i2c_driver(fs210x_i2c_driver);++MODULE_AUTHOR("Nick Li <nick.li@foursemi.com>");+MODULE_DESCRIPTION("FS2104/5S Audio Amplifier Driver");+MODULE_VERSION(FS210X_DRV_VERSION);+MODULE_LICENSE("GPL v2");
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs210x.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2025-07-03 06:36:50
On 03/07/2025 05:56, Nick wrote:
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
Your patches are not correctly ordered. Vendor prefix is supposed to be
before is being used. Bindings are before their users.
@@ -0,0 +1,95 @@+# SPDX-License-Identifier: GPL-2.0-only+%YAML 1.2+---+$id: http://devicetree.org/schemas/sound/foursemi,fs210x.yaml#+$schema: http://devicetree.org/meta-schemas/core.yaml#++title: FourSemi FS2104/5S Digital Audio Amplifier++maintainers:+ - Nick <nick.li@foursemi.com>++description: |+ The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.+ The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.++properties:+ compatible:+ enum:+ - foursemi,fs2104+ - foursemi,fs2105s
Your driver suggests these are compatible, so express that with fallback.
quoted hunk
++ reg:+ maxItems: 1+ description: |
Do not need '|' unless you need to preserve formatting.
quoted hunk
+ I2C address of the amplifier. Refer to datasheet for possible values:+ 0x68/0x69/0x6A/0x6B++ clocks:+ items:+ - description: The clock of I2S BCLK++ clock-names:+ items:+ - const: bclk++ '#sound-dai-cells':+ const: 0++ pvdd-supply:+ description: |
Do not need '|' unless you need to preserve formatting.
quoted hunk
+ Regulator for power supply(PVDD in datasheet).++ dvdd-supply:+ description: |
Do not need '|' unless you need to preserve formatting.
quoted hunk
+ Regulator for digital supply(DVDD in datasheet).++ sdz-gpios:
Use standard properties - see gpio-consumer-common
quoted hunk
+ maxItems: 1+ description: |+ SDZ(Shut Down) pin is active low, it will power down the chip,+ and then reset the chip to shut down state.++ fs,fwm-name:
No, use standard properties (see writing bindings)
quoted hunk
+ description: |+ The name of firmware that should be loaded for this+ instance. The firmware is gernerated by FourSemi's tuning tool.+ $ref: /schemas/types.yaml#/definitions/string++ fs,dai-name:
No, it's already in dai-common
quoted hunk
+ description: |+ The name of FS210x DAI, it can be set different name when+ there are multiple FS210x devices in the system.+ $ref: /schemas/types.yaml#/definitions/string++required:+ - compatible+ - reg+ - sdz-gpios+ - fs,fwm-name+ - '#sound-dai-cells'+
Missing ref to dai common
+additionalProperties: false
And this should be unevaluatedProperties instead
Best regards,
Krzysztof
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2025-07-03 07:10:41
On Thu, Jul 03, 2025 at 11:56:38AM +0800, Nick wrote:
quoted hunk
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs210x.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.
Best regards,
Krzysztof
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2025-07-03 07:15:41
On 03/07/2025 09:10, Krzysztof Kozlowski wrote:
On Thu, Jul 03, 2025 at 11:56:38AM +0800, Nick wrote:
quoted
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs210x.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.
Just use fsleep(), it'll use the most sensible delay type for the delay.
In general this applies to all delays in the driver, but especially with
a variable delay like this.
+static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)
quoted hunk
+ ret = fs210x_reg_write(fs210x, FS210X_39H_LVOLCTRL, vol[0]);+ ret |= fs210x_reg_write(fs210x, FS210X_3AH_RVOLCTRL, vol[1]);+
This looks pretty generic, why is it not using a standard control type?
+ ret = fs210x_set_pcm_volume(fs210x);
The driver should use the device defaults rather than having to
Shouldn't this be integrated with the chip init/reset?
quoted hunk
+static int fs210x_mute(struct fs210x_priv *fs210x, bool mute)+{+ int ret;++ if (mute) {+ cancel_delayed_work_sync(&fs210x->fault_check_work);+ cancel_delayed_work_sync(&fs210x->start_work);+ mutex_lock(&fs210x_mutex);+ ret = fs210x_dev_stop(fs210x);+ mutex_unlock(&fs210x_mutex);+ return ret;+ }
Mute is expected to be a really fast operation, this is stopping the
device entirely and fiddling about with locks (which were held where?).
This looks like the device just doesn't support mute.
quoted hunk
+ /*+ * According to the power up/down sequence of FS210x,+ * the FS210x requests the I2S clock has been present+ * and stable(>= 2ms) before it playing.+ */+ if (fs210x->clk_bclk) {+ mutex_lock(&fs210x_mutex);+ ret = fs210x_dev_play(fs210x);+ mutex_unlock(&fs210x_mutex);+ } else {
This is definitely not appropriate for mute, it should be in the power
management flow - either set_bias_level() or a DAPM widget.
+ case SND_SOC_DAIFMT_CBC_CFC:
+ /* Only supports slave mode */
+static int fs210x_pcm_volume_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{+ struct fs210x_priv *fs210x;+ long *pval;+ int ret;++ ret = fs210x_get_drvdata_from_kctrl(kcontrol, &fs210x);+ if (ret || !fs210x->dev) {+ pr_err("pcm_volume_put: fs210x is null\n");+ return -EINVAL;+ }+
_put() callbacks should return 1 when the control changes values, though
as indicated above I'm not clear this needs to be a custom control at
all. Please run mixer-test against your driver, it will check for this
and other issues.
quoted hunk
+static int fs210x_effect_scene_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{
quoted hunk
+ /*+ * FS210x has scene(s) as below:+ * init scene: id = 0(It's set in fs210x_init_chip() only)+ * effect scene(s): id = 1~N (optional)+ * scene_id = effect_index + 1.+ */+ scene_id = ucontrol->value.integer.value[0] + 1;+ if (fs210x->is_suspended) {+ fs210x->scene_id = scene_id;+ mutex_unlock(&fs210x_mutex);+ return 0;+ }
This doesn't validate the passed value at all.
quoted hunk
+static int fs210x_probe(struct snd_soc_component *cmpnt)+{+ struct fs210x_priv *fs210x;+ int ret;
This sort of initialisation and resource acquisition that doesn't
require any audio stuff should be done at the bus level so that we don't
register with ASoC until all the inputs are ready.
quoted hunk
+#ifdef CONFIG_PM+static int fs210x_suspend(struct snd_soc_component *cmpnt)+{+ struct fs210x_priv *fs210x;+ int ret;++ fs210x = snd_soc_component_get_drvdata(cmpnt);+ if (!fs210x || !fs210x->dev)+ return -EINVAL;++ cancel_delayed_work_sync(&fs210x->start_work);+ cancel_delayed_work_sync(&fs210x->fault_check_work);++ mutex_lock(&fs210x_mutex);
We don't need to prevent new work being scheduled?
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-04 06:01:43
On Thu, Jul 03, 2025 at 08:36:42AM +0200, Krzysztof Kozlowski wrote:
On 03/07/2025 05:56, Nick wrote:
quoted
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
Your patches are not correctly ordered. Vendor prefix is supposed to be
before is being used. Bindings are before their users.
OK, I will update the orders of patches in next version.
OK, I will update the file name to foursemi,fs2105s.yaml
quoted
@@ -0,0 +1,95 @@+# SPDX-License-Identifier: GPL-2.0-only+%YAML 1.2+---+$id: http://devicetree.org/schemas/sound/foursemi,fs210x.yaml#+$schema: http://devicetree.org/meta-schemas/core.yaml#++title: FourSemi FS2104/5S Digital Audio Amplifier++maintainers:+ - Nick <nick.li@foursemi.com>++description: |+ The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.+ The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.++properties:+ compatible:+ enum:+ - foursemi,fs2104+ - foursemi,fs2105s
Your driver suggests these are compatible, so express that with fallback.
OK, I will update these code to:
oneOf:
- items:
...
- enum:
...
quoted
++ reg:+ maxItems: 1+ description: |
Do not need '|' unless you need to preserve formatting.
OK, I will delete it in next version.
quoted
+ I2C address of the amplifier. Refer to datasheet for possible values:+ 0x68/0x69/0x6A/0x6B++ clocks:+ items:+ - description: The clock of I2S BCLK++ clock-names:+ items:+ - const: bclk++ '#sound-dai-cells':+ const: 0++ pvdd-supply:+ description: |
Do not need '|' unless you need to preserve formatting.
OK, I will delete it in next version.
quoted
+ Regulator for power supply(PVDD in datasheet).++ dvdd-supply:+ description: |
Do not need '|' unless you need to preserve formatting.
OK, I will delete it.
quoted
+ Regulator for digital supply(DVDD in datasheet).++ sdz-gpios:
Use standard properties - see gpio-consumer-common
OK, I will update this property to reset-gpios, and update the driver.
quoted
+ maxItems: 1+ description: |+ SDZ(Shut Down) pin is active low, it will power down the chip,+ and then reset the chip to shut down state.++ fs,fwm-name:
No, use standard properties (see writing bindings)
OK, I will update this property to firmware-name, and update the driver.
quoted
+ description: |+ The name of firmware that should be loaded for this+ instance. The firmware is gernerated by FourSemi's tuning tool.+ $ref: /schemas/types.yaml#/definitions/string++ fs,dai-name:
No, it's already in dai-common
OK, delete it in next version, and update the driver.
quoted
+ description: |+ The name of FS210x DAI, it can be set different name when+ there are multiple FS210x devices in the system.+ $ref: /schemas/types.yaml#/definitions/string++required:+ - compatible+ - reg+ - sdz-gpios+ - fs,fwm-name+ - '#sound-dai-cells'+
Missing ref to dai common
OK, reference will be added:
allOf:
- $ref: dai-common.yaml#
quoted
+additionalProperties: false
And this should be unevaluatedProperties instead
OK, I will update it to:
unevaluatedProperties: false
Thanks for your patient response very mush.
Best regards,
Nick
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-04 07:19:34
On Thu, Jul 03, 2025 at 09:10:38AM +0200, Krzysztof Kozlowski wrote:
On Thu, Jul 03, 2025 at 11:56:38AM +0800, Nick wrote:
quoted
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs210x.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.
OK, I will check the patches with option --strict, and fix the warnings/errors
in next version.
Thanks.
Best regards,
Nick
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-04 07:27:13
On Thu, Jul 03, 2025 at 09:15:36AM +0200, Krzysztof Kozlowski wrote:
On 03/07/2025 09:10, Krzysztof Kozlowski wrote:
quoted
On Thu, Jul 03, 2025 at 11:56:38AM +0800, Nick wrote:
quoted
From: Nick Li <nick.li@foursemi.com>
This patch adds bindings for FourSemi FS2104/5S audio amplifiers
which can support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs210x.yaml | 95 +++++++++++++++++++
1 file changed, 95 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs210x.yaml
Please run scripts/checkpatch.pl on the patches and fix reported
warnings. After that, run also 'scripts/checkpatch.pl --strict' on the
patches and (probably) fix more warnings. Some warnings can be ignored,
especially from --strict run, but the code here looks like it needs a
fix. Feel free to get in touch if the warning is not clear.
... and that was my third email today... I apologize I did not send
these as one review email, that's not really professional from my side
but somehow coffee did not work good this morning.
Thank you for taking your valuable time to review the code.
Best regards,
Nick
We do have a lot of logging support in regmap which is more
controllable.
OK, we will remove this debug log, it's for test.
quoted
+static int fs210x_reg_dump(struct fs210x_priv *fs210x)
+{
This is duplicating regmap's debugfs.
We use it to print reigsters when the device accors a fault in the checking
monitor, especially for dealing with probabilistic issues,
these logs are beneficial for analyzing the problem.
Just use fsleep(), it'll use the most sensible delay type for the delay.
In general this applies to all delays in the driver, but especially with
a variable delay like this.
OK, we will replace usleep_range to fsleep
quoted
+static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)
quoted
+ ret = fs210x_reg_write(fs210x, FS210X_39H_LVOLCTRL, vol[0]);+ ret |= fs210x_reg_write(fs210x, FS210X_3AH_RVOLCTRL, vol[1]);+
This looks pretty generic, why is it not using a standard control type?
1. We use regmap as REGMAP_NONE, because most of the registers settings
are in the firmware, if we use a standard control,the driver shouldn't
cache the registers after suspending the device(it will be reset).
2. The volume registers of FS2104 and FS2105S are different,
if we us a stardard control, we need two controls,
and register it by checking the device type.
so we customize the volume control.
quoted
+ ret = fs210x_set_pcm_volume(fs210x);
The driver should use the device defaults rather than having to
The volume contorl can be used to set different volumes,
the volume will be masked in fs210x->vol[2],
we restore the volume when the driver resumes(reinitializes) the deivce.
Shouldn't this be integrated with the chip init/reset?
1. We implement this function(reset and wait times) to clarify that
pulling up/down the SDZ/reset pin must to wait enougth delay time.
quoted
+static int fs210x_mute(struct fs210x_priv *fs210x, bool mute)+{+ int ret;++ if (mute) {+ cancel_delayed_work_sync(&fs210x->fault_check_work);+ cancel_delayed_work_sync(&fs210x->start_work);+ mutex_lock(&fs210x_mutex);+ ret = fs210x_dev_stop(fs210x);+ mutex_unlock(&fs210x_mutex);+ return ret;+ }
Mute is expected to be a really fast operation, this is stopping the
device entirely and fiddling about with locks (which were held where?).
This looks like the device just doesn't support mute.
To avoid the pop noise, the device will fade out when we mute/stop it,
and the power down sequence of FS210x requests the driver to wait
enougth times before the BCLK is off or starting device in next time.
quoted
+ /*+ * According to the power up/down sequence of FS210x,+ * the FS210x requests the I2S clock has been present+ * and stable(>= 2ms) before it playing.+ */+ if (fs210x->clk_bclk) {+ mutex_lock(&fs210x_mutex);+ ret = fs210x_dev_play(fs210x);+ mutex_unlock(&fs210x_mutex);+ } else {
This is definitely not appropriate for mute, it should be in the power
management flow - either set_bias_level() or a DAPM widget.
1. Because the device uses BCLK clock as the clock source,
we need to start the device in the life cycle of the clock,
also we need to start device after the PLL setting(set in dai->hw_params)
so we start the device in here: dai->mute_stream(unmute)
2. If the SOC(s) doesn't have the clock(bclk) for us to configure,
It meams no clock bclk defined in the DTS,
and the clock is activated in dai->trigger start usually,
so we will use a delay work to start the device in here.
Any good ideas about satisfying this power up/down sequence?
quoted
+ case SND_SOC_DAIFMT_CBC_CFC:
+ /* Only supports slave mode */
No dev_info() prints in the normal playback/record flow, that just spams
the logs too easily.
OK.
quoted
+ /* The FS2105S can't support 16kHz sample rate. */+ if (fs210x->devid == FS2105S_DEVICE_ID && fs210x->srate == 16000)+ return -ENOTSUPP;
This should be reported in constraints too.
OK.
quoted
+ if (!(status & FS210X_05H_AMPS_MASK))
+ dev_err(fs210x->dev, "Amplifier unready\n");
Does this get triggered during the normal start/stop flow?
It will get triggered when:
1. BCLK clock is closed before stoping device
2. BCLK clock is opened after starting device
We should avoid these power up/down sequence, it may cause pop noise,
If they happens, it should be reported and fixed?
Good idea, or set the interval times by the DTS property.
We are considering adding a DTS property:
foursemi,monitor-period-ms
quoted
+static int fs210x_pcm_volume_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{+ struct fs210x_priv *fs210x;+ long *pval;+ int ret;++ ret = fs210x_get_drvdata_from_kctrl(kcontrol, &fs210x);+ if (ret || !fs210x->dev) {+ pr_err("pcm_volume_put: fs210x is null\n");+ return -EINVAL;+ }+
_put() callbacks should return 1 when the control changes values, though
as indicated above I'm not clear this needs to be a custom control at
all. Please run mixer-test against your driver, it will check for this
and other issues.
OK, will fix it in next version.
quoted
+static int fs210x_effect_scene_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{
quoted
+ /*+ * FS210x has scene(s) as below:+ * init scene: id = 0(It's set in fs210x_init_chip() only)+ * effect scene(s): id = 1~N (optional)+ * scene_id = effect_index + 1.+ */+ scene_id = ucontrol->value.integer.value[0] + 1;+ if (fs210x->is_suspended) {+ fs210x->scene_id = scene_id;+ mutex_unlock(&fs210x_mutex);+ return 0;+ }
This doesn't validate the passed value at all.
OK, I will fix it.
quoted
+static int fs210x_probe(struct snd_soc_component *cmpnt)+{+ struct fs210x_priv *fs210x;+ int ret;
This sort of initialisation and resource acquisition that doesn't
require any audio stuff should be done at the bus level so that we don't
register with ASoC until all the inputs are ready.
OK, we will do these work in i2c probe
quoted
+#ifdef CONFIG_PM+static int fs210x_suspend(struct snd_soc_component *cmpnt)+{+ struct fs210x_priv *fs210x;+ int ret;++ fs210x = snd_soc_component_get_drvdata(cmpnt);+ if (!fs210x || !fs210x->dev)+ return -EINVAL;++ cancel_delayed_work_sync(&fs210x->start_work);+ cancel_delayed_work_sync(&fs210x->fault_check_work);++ mutex_lock(&fs210x_mutex);
We don't need to prevent new work being scheduled?
Could you please explain more details to help me understand and test this case?
Thanks.
Best regards,
Nick
From: Mark Brown <broonie@kernel.org> Date: 2025-07-04 14:37:37
On Fri, Jul 04, 2025 at 07:12:47PM +0800, Nick Li wrote:
On Thu, Jul 03, 2025 at 03:59:34PM +0100, Mark Brown wrote:
quoted
On Thu, Jul 03, 2025 at 11:56:37AM +0800, Nick wrote:
quoted
quoted
+static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)
quoted
quoted
+ ret = fs210x_reg_write(fs210x, FS210X_39H_LVOLCTRL, vol[0]);
+ ret |= fs210x_reg_write(fs210x, FS210X_3AH_RVOLCTRL, vol[1]);
quoted
This looks pretty generic, why is it not using a standard control type?
1. We use regmap as REGMAP_NONE, because most of the registers settings
are in the firmware, if we use a standard control,the driver shouldn't
cache the registers after suspending the device(it will be reset).
You can exclude registers from a cache, including excluding most of
them, or manually handle registers over suspend/resume. It's generally
better to share the userspace facing interfaces.
2. The volume registers of FS2104 and FS2105S are different,
if we us a stardard control, we need two controls,
and register it by checking the device type.
so we customize the volume control.
If the different devices have different controls you should just
register different controls.
quoted
quoted
+ ret = fs210x_set_pcm_volume(fs210x);
quoted
The driver should use the device defaults rather than having to
The volume contorl can be used to set different volumes,
the volume will be masked in fs210x->vol[2],
we restore the volume when the driver resumes(reinitializes) the deivce.
You're not just restoring the values on resume, you're also overwriting
them on probe.
Shouldn't this be integrated with the chip init/reset?
1. We implement this function(reset and wait times) to clarify that
pulling up/down the SDZ/reset pin must to wait enougth delay time.
That doesn't really answer the question?
quoted
quoted
+ * According to the power up/down sequence of FS210x,+ * the FS210x requests the I2S clock has been present+ * and stable(>= 2ms) before it playing.+ */+ if (fs210x->clk_bclk) {+ mutex_lock(&fs210x_mutex);+ ret = fs210x_dev_play(fs210x);+ mutex_unlock(&fs210x_mutex);+ } else {
quoted
This is definitely not appropriate for mute, it should be in the power
management flow - either set_bias_level() or a DAPM widget.
1. Because the device uses BCLK clock as the clock source,
we need to start the device in the life cycle of the clock,
also we need to start device after the PLL setting(set in dai->hw_params)
so we start the device in here: dai->mute_stream(unmute)
All the power management happens after hw_params(), this isn't an issue.
2. If the SOC(s) doesn't have the clock(bclk) for us to configure,
It meams no clock bclk defined in the DTS,
and the clock is activated in dai->trigger start usually,
so we will use a delay work to start the device in here.
Any good ideas about satisfying this power up/down sequence?
There's not great options here, and you're going to loose the start of
playback especially with devices that don't start clocking until audio
starts. You really need the CPU vendors you're working with to
implement SND_SOC_DAIFMT_CONT or expose their clocks via the clock API
but not all hardware is able to do this. I think given how limited your
hardware is here you really need something in trigger() or some new
callback that runs later than that, the delayed work you've got there is
trying to fudge things to run after trigger.
quoted
quoted
+ if (!(status & FS210X_05H_AMPS_MASK))
+ dev_err(fs210x->dev, "Amplifier unready\n");
quoted
Does this get triggered during the normal start/stop flow?
It will get triggered when:
1. BCLK clock is closed before stoping device
2. BCLK clock is opened after starting device
We should avoid these power up/down sequence, it may cause pop noise,
If they happens, it should be reported and fixed?
If they don't happen in normal operation it's fine to have them, the
concern was that this would be triggered during normal operation as part
of the startup or shutdown sequence.
We don't need to prevent new work being scheduled?
Could you please explain more details to help me understand and test this case?
What if for example playback is starting up at the same time as the
system enters suspend - the CODEC startup might get run after the
delayed work is cancelled but before the lock is taken.
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-07 08:35:23
On Fri, Jul 04, 2025 at 03:37:32PM +0100, Mark Brown wrote:
On Fri, Jul 04, 2025 at 07:12:47PM +0800, Nick Li wrote:
quoted
On Thu, Jul 03, 2025 at 03:59:34PM +0100, Mark Brown wrote:
quoted
On Thu, Jul 03, 2025 at 11:56:37AM +0800, Nick wrote:
quoted
quoted
quoted
+static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)
quoted
quoted
quoted
+ ret = fs210x_reg_write(fs210x, FS210X_39H_LVOLCTRL, vol[0]);
+ ret |= fs210x_reg_write(fs210x, FS210X_3AH_RVOLCTRL, vol[1]);
quoted
quoted
This looks pretty generic, why is it not using a standard control type?
quoted
1. We use regmap as REGMAP_NONE, because most of the registers settings
are in the firmware, if we use a standard control,the driver shouldn't
cache the registers after suspending the device(it will be reset).
You can exclude registers from a cache, including excluding most of
them, or manually handle registers over suspend/resume. It's generally
better to share the userspace facing interfaces.
OK, we will update the volume control to:
SOC_DOUBLE_TLV(...)
and use the regmap to cache the volumes.
quoted
2. The volume registers of FS2104 and FS2105S are different,
if we us a stardard control, we need two controls,
and register it by checking the device type.
so we customize the volume control.
If the different devices have different controls you should just
register different controls.
OK.
quoted
quoted
quoted
+ ret = fs210x_set_pcm_volume(fs210x);
quoted
quoted
The driver should use the device defaults rather than having to
quoted
The volume contorl can be used to set different volumes,
the volume will be masked in fs210x->vol[2],
we restore the volume when the driver resumes(reinitializes) the deivce.
You're not just restoring the values on resume, you're also overwriting
them on probe.
Yes, the volume will be set to the default value at the first time of initialization on probe,
and it may be updated by volume control later.
It's a good way to use the regmap cache(REGCACHE_RBTREE) to cache the volumes.
Shouldn't this be integrated with the chip init/reset?
quoted
1. We implement this function(reset and wait times) to clarify that
pulling up/down the SDZ/reset pin must to wait enougth delay time.
That doesn't really answer the question?
It's ok to integrate it into the chip init/reset.
quoted
quoted
quoted
+ * According to the power up/down sequence of FS210x,+ * the FS210x requests the I2S clock has been present+ * and stable(>= 2ms) before it playing.+ */+ if (fs210x->clk_bclk) {+ mutex_lock(&fs210x_mutex);+ ret = fs210x_dev_play(fs210x);+ mutex_unlock(&fs210x_mutex);+ } else {
quoted
quoted
This is definitely not appropriate for mute, it should be in the power
management flow - either set_bias_level() or a DAPM widget.
quoted
1. Because the device uses BCLK clock as the clock source,
we need to start the device in the life cycle of the clock,
also we need to start device after the PLL setting(set in dai->hw_params)
so we start the device in here: dai->mute_stream(unmute)
All the power management happens after hw_params(), this isn't an issue.
We will evaluate the way to reduce the cost time of starting/stoping device:
Do PM by DAPM widget or set_bias_on.
quoted
2. If the SOC(s) doesn't have the clock(bclk) for us to configure,
It meams no clock bclk defined in the DTS,
and the clock is activated in dai->trigger start usually,
so we will use a delay work to start the device in here.
quoted
Any good ideas about satisfying this power up/down sequence?
There's not great options here, and you're going to loose the start of
playback especially with devices that don't start clocking until audio
starts. You really need the CPU vendors you're working with to
implement SND_SOC_DAIFMT_CONT or expose their clocks via the clock API
but not all hardware is able to do this. I think given how limited your
hardware is here you really need something in trigger() or some new
callback that runs later than that, the delayed work you've got there is
trying to fudge things to run after trigger.
We will start the start_work in dai->trigger if there is no clock bclk
for us to control, and disable fading in/out in firmware to reduce
the time consumption if it is needed(As a backup plan).
quoted
quoted
quoted
+ if (!(status & FS210X_05H_AMPS_MASK))
+ dev_err(fs210x->dev, "Amplifier unready\n");
quoted
quoted
Does this get triggered during the normal start/stop flow?
quoted
It will get triggered when:
1. BCLK clock is closed before stoping device
2. BCLK clock is opened after starting device
We should avoid these power up/down sequence, it may cause pop noise,
If they happens, it should be reported and fixed?
If they don't happen in normal operation it's fine to have them, the
concern was that this would be triggered during normal operation as part
of the startup or shutdown sequence.
We can remove the registers dumping, and set the AMPS status to dev_dbg level.
We don't need to prevent new work being scheduled?
quoted
Could you please explain more details to help me understand and test this case?
What if for example playback is starting up at the same time as the
system enters suspend - the CODEC startup might get run after the
delayed work is cancelled but before the lock is taken.
Understood, thanks a lot.
We evaluate to remove the resume/suspend(cold start up/down device):
The power consumptions between register settings and pins controlling are very close,
and it can help us to reduce the time consumption.
Best regards,
Nick
From: Mark Brown <broonie@kernel.org> Date: 2025-07-07 20:42:14
On Mon, Jul 07, 2025 at 04:34:03PM +0800, Nick Li wrote:
On Fri, Jul 04, 2025 at 03:37:32PM +0100, Mark Brown wrote:
quoted
On Fri, Jul 04, 2025 at 07:12:47PM +0800, Nick Li wrote:
quoted
On Thu, Jul 03, 2025 at 03:59:34PM +0100, Mark Brown wrote:
quoted
On Thu, Jul 03, 2025 at 11:56:37AM +0800, Nick wrote:
quoted
quoted
the volume will be masked in fs210x->vol[2],
we restore the volume when the driver resumes(reinitializes) the deivce.
quoted
You're not just restoring the values on resume, you're also overwriting
them on probe.
Yes, the volume will be set to the default value at the first time of initialization on probe,
and it may be updated by volume control later.
It's a good way to use the regmap cache(REGCACHE_RBTREE) to cache the volumes.
Great. Note that for new things you should generally use _MAPLE
instead, it's a more modern data structure - _RBTREE can still make
sense for some specialist use cases but this is unlikely to be one of
them.
quoted
There's not great options here, and you're going to loose the start of
playback especially with devices that don't start clocking until audio
starts. You really need the CPU vendors you're working with to
implement SND_SOC_DAIFMT_CONT or expose their clocks via the clock API
but not all hardware is able to do this. I think given how limited your
hardware is here you really need something in trigger() or some new
callback that runs later than that, the delayed work you've got there is
trying to fudge things to run after trigger.
We will start the start_work in dai->trigger if there is no clock bclk
for us to control, and disable fading in/out in firmware to reduce
the time consumption if it is needed(As a backup plan).
Great. You could also make the fade in/out user selectable so the
application can trade off between pops/clicks and cut off audio.
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-08 11:31:15
The FS2104/5S are FourSemi digital audio amplifiers
with I2C control. They are Inductor-Less, Stereo, Closed-Loop,
Digital Input Class-D Power Amplifiers with Enhanced Signal Processing.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
sound/soc/codecs/Kconfig | 11 +
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/fs210x.c | 1610 +++++++++++++++++++++++++++++++++++++
sound/soc/codecs/fs210x.h | 79 ++
4 files changed, 1702 insertions(+)
create mode 100644 sound/soc/codecs/fs210x.c
create mode 100644 sound/soc/codecs/fs210x.h
@@ -0,0 +1,1610 @@+// SPDX-License-Identifier: GPL-2.0+/*+*fs210x.c--DriverfortheFS2104/5SAudioAmplifier+*+*Copyright(C)2016-2025ShanghaiFourSemiSemiconductorCo.,Ltd.+*/++#include<linux/clk.h>+#include<linux/delay.h>+#include<linux/i2c.h>+#include<linux/module.h>+#include<linux/mutex.h>+#include<linux/of.h>+#include<linux/of_gpio.h>+#include<linux/regmap.h>+#include<linux/regulator/consumer.h>+#include<linux/workqueue.h>+#include<sound/soc.h>+#include<sound/tlv.h>++#include"fs210x.h"+#include"fs-amp-lib.h"++#define FS210X_DRV_VERSION "v1.0.6"+#define FS210X_DEFAULT_FWM_NAME "fs210x_fwm.bin"+#define FS210X_DEFAULT_DAI_NAME "fs210x-aif"+#define FS2105S_DEVICE_ID 0x20 /* FS2105S */+#define FS210X_DEVICE_ID 0x45 /* FS2104 */+#define FS210X_REG_MAX 0xF8+#define FS210X_VOLUME_MIN 0+#define FS210X_VOLUME_MAX 511+#define FS210X_INIT_SCENE 0+#define FS210X_DEFAULT_SCENE 1+#define FS210X_START_DELAY_MS 5+#define FS210X_FAULT_CHECK_INTERVAL_MS 2000+#define FS2105S_RATES (SNDRV_PCM_RATE_32000 | \+SNDRV_PCM_RATE_44100|\+SNDRV_PCM_RATE_48000|\+SNDRV_PCM_RATE_88200|\+SNDRV_PCM_RATE_96000)+#define FS210X_RATES (SNDRV_PCM_RATE_16000 | FS2105S_RATES)+#define FS210X_FORMATS (SNDRV_PCM_FMTBIT_S16_LE | \+SNDRV_PCM_FMTBIT_S24_LE|\+SNDRV_PCM_FMTBIT_S24_3LE|\+SNDRV_PCM_FMTBIT_S32_LE)+#define FS210X_NUM_SUPPLIES ARRAY_SIZE(fs210x_supply_names)++staticconstchar*constfs210x_supply_names[]={+"pvdd",+"dvdd",+};++structfs210x_platform_data{+constchar*fwm_name;+};++structfs210x_priv{+structi2c_client*i2c;+structdevice*dev;+structregmap*regmap;+structfs210x_platform_datapdata;+structregulator_bulk_datasupplies[FS210X_NUM_SUPPLIES];+structgpio_desc*gpio_sdz;+structdelayed_workstart_work;+structdelayed_workfault_check_work;+structfs_amp_libamp_lib;+conststructfs_amp_scene*cur_scene;+structclk*clk_bclk;+unsignedintbclk;+unsignedintsrate;+intscene_id;+u16devid;+u16vol[2];/* L/R channels volume */+boolis_inited;+boolis_suspended;+boolis_bclk_on;+boolis_playing;+};++staticDEFINE_MUTEX(fs210x_mutex);++staticconststructfs_pll_divfs210x_pll_div[]={+/* bclk, pll1, pll2, pll3 */+{512000,0x006C,0x0120,0x0001},+{768000,0x016C,0x00C0,0x0001},+{1024000,0x016C,0x0090,0x0001},+{1536000,0x016C,0x0060,0x0001},+{2048000,0x016C,0x0090,0x0002},+{2304000,0x016C,0x0080,0x0002},+{3072000,0x016C,0x0090,0x0003},+{4096000,0x016C,0x0090,0x0004},+{4608000,0x016C,0x0080,0x0004},+{6144000,0x016C,0x0090,0x0006},+{8192000,0x016C,0x0090,0x0008},+{9216000,0x016C,0x0090,0x0009},+{12288000,0x016C,0x0090,0x000C},+{16384000,0x016C,0x0090,0x0010},+{18432000,0x016C,0x0090,0x0012},+{24576000,0x016C,0x0090,0x0018},+{1411200,0x016C,0x0060,0x0001},+{2116800,0x016C,0x0080,0x0002},+{2822400,0x016C,0x0090,0x0003},+{4233600,0x016C,0x0080,0x0004},+{5644800,0x016C,0x0090,0x0006},+{8467200,0x016C,0x0090,0x0009},+{11289600,0x016C,0x0090,0x000C},+{16934400,0x016C,0x0090,0x0012},+{22579200,0x016C,0x0090,0x0018},+{2000000,0x017C,0x0093,0x0002},+};++staticintfs210x_bclk_set(structfs210x_priv*fs210x,boolon)+{+intret=0;++if(!fs210x||!fs210x->dev)+return-EINVAL;++if(!fs210x->clk_bclk)+return0;++if((fs210x->is_bclk_on^on)==0)+return0;++dev_dbg(fs210x->dev,"bclk switch %s\n",on?"on":"off");++if(on){+clk_set_rate(fs210x->clk_bclk,fs210x->bclk);+ret=clk_prepare_enable(fs210x->clk_bclk);+fs210x->is_bclk_on=true;+usleep_range(2000,2050);/* >= 2ms */+}else{+clk_disable_unprepare(fs210x->clk_bclk);+fs210x->is_bclk_on=false;+}++returnret;+}++staticintfs210x_reg_write(structfs210x_priv*fs210x,+u8reg,u16val)+{+intret;++ret=regmap_write(fs210x->regmap,reg,val);+if(ret){+dev_err(fs210x->dev,"Failed to write %02Xh: %d\n",reg,ret);+returnret;+}++dev_dbg(fs210x->dev,"RW: %02x %04x\n",reg,val);++return0;+}++staticintfs210x_reg_read(structfs210x_priv*fs210x,+u8reg,u16*pval)+{+unsignedintval;+intret;++ret=regmap_read(fs210x->regmap,reg,&val);+if(ret){+dev_err(fs210x->dev,"Failed to read %02Xh: %d\n",reg,ret);+returnret;+}++if(pval)+*pval=(u16)val;++dev_dbg(fs210x->dev,"RR: %02x %04x\n",reg,val);++return0;+}++staticintfs210x_reg_update_bits(structfs210x_priv*fs210x,+u8reg,u16mask,u16val)+{+intret;++ret=regmap_update_bits(fs210x->regmap,reg,mask,val);+if(ret){+dev_err(fs210x->dev,"Failed to update %02Xh: %d\n",reg,ret);+returnret;+}++dev_dbg(fs210x->dev,"RU: %02x %04x %04x\n",reg,mask,val);++return0;+}++staticintfs210x_reg_bulk_write(structfs210x_priv*fs210x,+u8reg,constvoid*val,u32size)+{+intret;++ret=regmap_bulk_write(fs210x->regmap,reg,val,size/2);+if(ret){+dev_err(fs210x->dev,"Failed to bulk write %02Xh: %d\n",+reg,ret);+returnret;+}++dev_dbg(fs210x->dev,"BW: %02x %d\n",reg,size);++return0;+}++staticintfs210x_reg_dump(structfs210x_priv*fs210x)+{+u16val[8];/* 8 registers per line */+u8idx,reg;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++#define FS210X_REG_DUMP_MAX 0xCF+for(reg=0x00;reg<=FS210X_REG_DUMP_MAX;reg++){+/* Print 8 register values per line */+idx=(reg&0x7);+ret=fs210x_reg_read(fs210x,reg,val+idx);+if(ret)+break;+if(idx!=0x7&®!=FS210X_REG_DUMP_MAX)+continue;+dev_info(fs210x->dev,+"%02Xh: %04x %04x %04x %04x %04x %04x %04x %04x\n",+(reg&0xF8),val[0],val[1],val[2],+val[3],val[4],val[5],val[6],val[7]);+}++returnret;+}++staticinlineintfs210x_write_reg_val(structfs210x_priv*fs210x,+conststructfs_reg_val*regv)+{+returnfs210x_reg_write(fs210x,regv->reg,regv->val);+}++staticinlineintfs210x_write_reg_bits(structfs210x_priv*fs210x,+conststructfs_reg_bits*regu)+{+returnfs210x_reg_update_bits(fs210x,+regu->reg,+regu->mask,+regu->val);+}++staticinlineintfs210x_set_cmd_pkg(structfs210x_priv*fs210x,+conststructfs_cmd_pkg*pkg,+unsignedint*offset)+{+intdelay_us;++if(pkg->cmd>=0x00&&pkg->cmd<=FS210X_REG_MAX){+*offset=sizeof(pkg->regv);+returnfs210x_write_reg_val(fs210x,&pkg->regv);+}elseif(pkg->cmd==FS_CMD_UPDATE){+*offset=sizeof(pkg->regb);+returnfs210x_write_reg_bits(fs210x,&pkg->regb);+}elseif(pkg->cmd==FS_CMD_DELAY){+if(pkg->regv.val>FS_CMD_DELAY_MS_MAX)+return-EOPNOTSUPP;+delay_us=pkg->regv.val*1000;+usleep_range(delay_us,delay_us+50);+*offset=sizeof(pkg->regv);+return0;+}++dev_err(fs210x->dev,"Invalid pkg cmd: %d\n",pkg->cmd);++return-EOPNOTSUPP;+}++staticintfs210x_reg_write_table(structfs210x_priv*fs210x,+conststructfs_reg_table*reg)+{+conststructfs_cmd_pkg*pkg;+unsignedintindex,offset;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++if(!reg||reg->size==0)+return-EFAULT;++for(index=0;index<reg->size;index+=offset){+pkg=(structfs_cmd_pkg*)(reg->buf+index);+ret=fs210x_set_cmd_pkg(fs210x,pkg,&offset);+if(ret){+dev_err(fs210x->dev,"Failed to set cmd pkg: %02X-%d\n",+pkg->cmd,ret);+returnret;+}+}++if(index!=reg->size){+dev_err(fs210x->dev,"Invalid reg table size: %d-%d\n",+index,reg->size);+return-EFAULT;+}++return0;+}++staticintfs210x_dev_play(structfs210x_priv*fs210x)+{+intret;++if(!fs210x->is_inited)+return-EFAULT;++if(fs210x->is_playing)+return0;++ret=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PLAY);+if(!ret)+fs210x->is_playing=true;++usleep_range(10000,10050);/* >= 10ms */++returnret;+}++staticintfs210x_dev_stop(structfs210x_priv*fs210x)+{+intret;++if(!fs210x->is_inited)+return-EFAULT;++if(!fs210x->is_playing)+return0;++ret=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PWDN);+fs210x->is_playing=false;++usleep_range(30000,30050);/* >= 30ms */++returnret;+}++staticintfs210x_set_reg_table(structfs210x_priv*fs210x,+conststructfs_amp_scene*scene)+{+conststructfs_amp_scene*cur_scene;+conststructfs_reg_table*reg;++if(!fs210x||!fs210x->dev||!scene)+return-EINVAL;++cur_scene=fs210x->cur_scene;+if(!scene->reg||cur_scene==scene){+dev_dbg(fs210x->dev,"Skip writing reg table\n");+return0;+}++reg=scene->reg;+dev_dbg(fs210x->dev,"reg table size: %d\n",reg->size);++returnfs210x_reg_write_table(fs210x,reg);+}++staticintfs210x_set_woofer_table(structfs210x_priv*fs210x)+{+conststructfs_file_table*woofer;+conststructfs_fwm_table*table;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++/* NOTE: fs2105s has woofer ram only */+if(fs210x->devid!=FS2105S_DEVICE_ID)+return0;++table=fs210x->amp_lib.table[FS_INDEX_WOOFER];+if(!table){+dev_dbg(fs210x->dev,"Skip writing woofer table\n");+return0;+}++woofer=(structfs_file_table*)table->buf;+dev_dbg(fs210x->dev,"woofer table size: %d\n",woofer->size);+/* Unit of woofer data is u32(4 bytes) */+if(woofer->size==0||(woofer->size&0x3)){+dev_err(fs210x->dev,"Invalid woofer size: %d\n",+woofer->size);+return-EINVAL;+}++ret=fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS2105S_46H_CAM_BURST_W);+ret|=fs210x_reg_bulk_write(fs210x,FS210X_42H_DACEQWL,+woofer->buf,woofer->size);++returnret;+}++staticintfs210x_set_effect_table(structfs210x_priv*fs210x,+conststructfs_amp_scene*scene)+{+conststructfs_amp_scene*cur_scene;+conststructfs_file_table*effect;+inthalf_size;+intret;++if(!fs210x||!fs210x->dev||!scene)+return-EINVAL;++cur_scene=fs210x->cur_scene;+if(!scene->effect||cur_scene==scene){+dev_dbg(fs210x->dev,"Skip writing effect table\n");+return0;+}++effect=scene->effect;+dev_dbg(fs210x->dev,"effect table size: %d\n",effect->size);++/* Unit of effect data is u32(4 bytes), 2 channels */+if(effect->size==0||(effect->size&0x7)){+dev_err(fs210x->dev,"Invalid effect size: %d\n",+effect->size);+return-EINVAL;+}++half_size=effect->size/2;++/* Left channel */+ret=fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS210X_46H_CAM_BURST_L);+ret|=fs210x_reg_bulk_write(fs210x,FS210X_42H_DACEQWL,+effect->buf,half_size);+if(ret)+returnret;++/* Right channel */+ret=fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS210X_46H_CAM_BURST_R);+ret|=fs210x_reg_bulk_write(fs210x,FS210X_42H_DACEQWL,+effect->buf+half_size,half_size);++returnret;+}++staticintfs210x_access_dsp_ram(structfs210x_priv*fs210x,boolenable)+{+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++if(enable){+ret=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_HIZ);+ret|=fs210x_reg_write(fs210x,FS210X_0BH_ACCKEY,+FS210X_0BH_ACCKEY_ON);+}else{+ret=fs210x_reg_write(fs210x,FS210X_0BH_ACCKEY,+FS210X_0BH_ACCKEY_OFF);+ret|=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PWDN);+}++usleep_range(10000,10050);/* >= 10ms */++returnret;+}++staticintfs210x_write_dsp_effect(structfs210x_priv*fs210x,+conststructfs_amp_scene*scene,+intscene_id)+{+intret;++if(!fs210x||!scene)+return-EINVAL;++ret=fs210x_access_dsp_ram(fs210x,true);+if(ret){+dev_err(fs210x->dev,"Failed to access dsp: %d\n",ret);+gototag_exit;+}++ret=fs210x_set_effect_table(fs210x,scene);+if(ret){+dev_err(fs210x->dev,"Failed to set effect: %d\n",ret);+gototag_exit;+}++if(scene_id==FS210X_INIT_SCENE)+ret=fs210x_set_woofer_table(fs210x);++tag_exit:+fs210x_reg_write(fs210x,FS210X_46H_DACEQA,+FS210X_46H_CAM_CLEAR);+fs210x_access_dsp_ram(fs210x,false);++returnret;+}++staticintfs210x_check_scene(structfs210x_priv*fs210x,+intscene_id,bool*skip_set)+{+structfs_amp_lib*amp_lib;++if(!fs210x||!skip_set)+return-EINVAL;++amp_lib=&fs210x->amp_lib;+if(amp_lib->scene_count==0||!amp_lib->scene){+dev_err(fs210x->dev,"There's no scene data\n");+return-EINVAL;+}++if(scene_id<0||scene_id>=amp_lib->scene_count){+dev_err(fs210x->dev,"Invalid scene_id: %d\n",scene_id);+return-EINVAL;+}++if(fs210x->scene_id==scene_id){+dev_dbg(fs210x->dev,"Skip to set same scene\n");+return0;+}++*skip_set=false;++return0;+}++staticintfs210x_set_scene(structfs210x_priv*fs210x,intscene_id)+{+conststructfs_amp_scene*scene;+boolskip_set=true;+boolis_playing;+intret;++if(!fs210x||!fs210x->dev)+return-EINVAL;++ret=fs210x_check_scene(fs210x,scene_id,&skip_set);+if(ret||skip_set)+returnret;++scene=fs210x->amp_lib.scene+scene_id;+dev_info(fs210x->dev,"Switch scene.%d: %s\n",+scene_id,scene->name);++is_playing=fs210x->is_playing;+if(is_playing)+fs210x_dev_stop(fs210x);++ret=fs210x_set_reg_table(fs210x,scene);+if(ret){+dev_err(fs210x->dev,"Failed to set reg: %d\n",ret);+returnret;+}++ret=fs210x_write_dsp_effect(fs210x,scene,scene_id);+if(ret){+dev_err(fs210x->dev,"Failed to write ram: %d\n",ret);+returnret;+}++fs210x->cur_scene=scene;+fs210x->scene_id=scene_id;++if(is_playing)+fs210x_dev_play(fs210x);++return0;+}++staticinlineintvolume_is_valid(u16vol)+{+return(vol<=FS210X_VOLUME_MAX);+}++staticintfs210x_set_pcm_volume(structfs210x_priv*fs210x)+{+u16vol[2];+intret;++if(fs210x->devid==FS2105S_DEVICE_ID){+/*+*FS2105SEvolumeregisters:39h(Left)/3Ah(Right):+*VOL[15:7]:0dBto-95.8125dBinstepsof-0.1875dB+*1FFh:0dB+*000h:-95.8125dB+*/+vol[0]=fs210x->vol[0]<<FS2105S_39H_VOL_SHIFT;+vol[1]=fs210x->vol[1]<<FS2105S_3AH_VOL_SHIFT;+}else{+/*+*FS2104volumeregisters:39h(Left)/3Ah(Right):+*VOL[15:6]:0dBto-131.8125dBinstepsof-0.1875dB+*2BFh:0dB+*0C0h:-95.8125dB+*000h:-131.8125dB+*OffsetbetweenFS2104andFS2105S:2BFh-1FFh=0C0h+*/+#define VOL_OFFSET (0x2BF - 0x1FF)+vol[0]=(fs210x->vol[0]+VOL_OFFSET)<<FS210X_39H_VOL_SHIFT;+vol[1]=(fs210x->vol[1]+VOL_OFFSET)<<FS210X_3AH_VOL_SHIFT;+}++ret=fs210x_reg_write(fs210x,FS210X_39H_LVOLCTRL,vol[0]);+ret|=fs210x_reg_write(fs210x,FS210X_3AH_RVOLCTRL,vol[1]);++returnret;+}++staticintfs210x_init_chip(structfs210x_priv*fs210x)+{+intscene_id;+intret;++/* i2c reset */+ret=fs210x_reg_write(fs210x,FS210X_10H_PWRCTRL,+FS210X_10H_I2C_RESET);+if(ret)+gototag_power_down;++/* Wait >= 10ms for i2c resetting */+usleep_range(10000,10050);++/* Backup scene id */+scene_id=fs210x->scene_id;+fs210x->scene_id=-1;++/* Init registers/RAM by init scene */+ret=fs210x_set_scene(fs210x,FS210X_INIT_SCENE);+if(ret)+gototag_power_down;++/*+*Ifthefirmwarehaseffectscene(s),+*weloadeffectscenebydefaultsceneorscene_id+*/+if(fs210x->amp_lib.scene_count>1){+if(scene_id<FS210X_DEFAULT_SCENE)+scene_id=FS210X_DEFAULT_SCENE;+ret=fs210x_set_scene(fs210x,scene_id);+if(ret)+gototag_power_down;+}++ret=fs210x_set_pcm_volume(fs210x);++tag_power_down:+/* Power down the device */+ret|=fs210x_reg_write(fs210x,FS210X_11H_SYSCTRL,+FS210X_11H_DPS_PWDN);+if(!ret)+fs210x->is_inited=true;++returnret;+}++staticvoidfs210x_sdz_pin_set(structfs210x_priv*fs210x,boolactive)+{+if(!fs210x||!fs210x->gpio_sdz)+return;++/*+*TheSDZpin(ShutDown)isactivelow,whenitispulledlow,+*itwilltriggertwoproceduressequentially:+*-Powerdownthechip;+*-Resetthechiptoshutdownstate.+*NOTE:+*TheI2CcommunicationisnackuntiltheSDZpin+*ispulledupandwait10msatleast.+*/+if(active){+gpiod_set_value_cansleep(fs210x->gpio_sdz,1);+dev_dbg(fs210x->dev,"SDZ pin is OFF\n");+usleep_range(30000,30050);/* >= 30ms */+}else{+gpiod_set_value_cansleep(fs210x->gpio_sdz,0);+dev_dbg(fs210x->dev,"SDZ pin is ON\n");+usleep_range(10000,10050);/* >= 10ms */+}+}++staticintfs210x_set_i2s_params(structfs210x_priv*fs210x)+{+conststructfs_i2s_srateparams[]={+{16000,0x3},+{32000,0x7},+{44100,0x8},+{48000,0x9},+{88200,0xA},+{96000,0xB},+};+u16val;+inti,ret;++for(i=0;i<ARRAY_SIZE(params);i++){+if(params[i].srate!=fs210x->srate)+continue;+val=params[i].i2ssr<<FS210X_17H_I2SSR_SHIFT;+ret=fs210x_reg_update_bits(fs210x,+FS210X_17H_I2SCTRL,+FS210X_17H_I2SSR_MASK,+val);+returnret;+}++dev_err(fs210x->dev,"Invalid sample rate: %d\n",fs210x->srate);++return-EINVAL;+}++staticintfs210x_get_pll_div(structfs210x_priv*fs210x,+conststructfs_pll_div**pll_div)+{+inti;++if(!fs210x||!pll_div)+return-EINVAL;++for(i=0;i<ARRAY_SIZE(fs210x_pll_div);i++){+if(fs210x_pll_div[i].bclk!=fs210x->bclk)+continue;+*pll_div=fs210x_pll_div+i;+return0;+}++dev_err(fs210x->dev,"No PLL table for bclk: %d\n",fs210x->bclk);++return-EFAULT;+}++staticintfs210x_set_hw_params(structfs210x_priv*fs210x)+{+conststructfs_pll_div*pll_div;+intret;++ret=fs210x_set_i2s_params(fs210x);+if(ret){+dev_err(fs210x->dev,"Failed to set i2s params: %d\n",ret);+returnret;+}++/* Set pll params */+ret=fs210x_get_pll_div(fs210x,&pll_div);+if(ret)+returnret;++ret=fs210x_reg_write(fs210x,FS210X_A1H_PLLCTRL1,pll_div->pll1);+ret|=fs210x_reg_write(fs210x,FS210X_A2H_PLLCTRL2,pll_div->pll2);+ret|=fs210x_reg_write(fs210x,FS210X_A3H_PLLCTRL3,pll_div->pll3);++returnret;+}++staticintfs210x_mute(structfs210x_priv*fs210x,boolmute)+{+intret;++if(mute){+cancel_delayed_work_sync(&fs210x->fault_check_work);+cancel_delayed_work_sync(&fs210x->start_work);+mutex_lock(&fs210x_mutex);+ret=fs210x_dev_stop(fs210x);+mutex_unlock(&fs210x_mutex);+returnret;+}++/*+*Accordingtothepowerup/downsequenceofFS210x,+*theFS210xrequeststheI2Sclockhasbeenpresent+*andstable(>=2ms)beforeitplaying.+*/+if(fs210x->clk_bclk){+mutex_lock(&fs210x_mutex);+ret=fs210x_dev_play(fs210x);+mutex_unlock(&fs210x_mutex);+}else{+schedule_delayed_work(&fs210x->start_work,+msecs_to_jiffies(FS210X_START_DELAY_MS));+ret=0;+}++schedule_delayed_work(&fs210x->fault_check_work,+msecs_to_jiffies(FS210X_FAULT_CHECK_INTERVAL_MS));++returnret;+}++staticintfs210x_dai_set_fmt(structsnd_soc_dai*dai,unsignedintfmt)+{+structfs210x_priv*fs210x;++fs210x=snd_soc_component_get_drvdata(dai->component);++dev_dbg(fs210x->dev,"fmt: %x\n",fmt);++switch(fmt&SND_SOC_DAIFMT_MASTER_MASK){+caseSND_SOC_DAIFMT_CBC_CFC:+/* Only supports slave mode */+break;+default:+dev_err(fs210x->dev,"Only supports slave mode\n");+return-EINVAL;+}++return0;+}++staticintfs210x_dai_hw_params(structsnd_pcm_substream*substream,+structsnd_pcm_hw_params*params,+structsnd_soc_dai*dai)+{+structfs210x_priv*fs210x;+intchn_num;+intret;++if(substream->stream!=SNDRV_PCM_STREAM_PLAYBACK)+return0;++fs210x=snd_soc_component_get_drvdata(dai->component);++fs210x->srate=params_rate(params);+fs210x->bclk=snd_soc_params_to_bclk(params);+chn_num=params_channels(params);+if(chn_num==1)/* mono */+fs210x->bclk*=2;/* I2S bus has 2 channels */++dev_info(fs210x->dev,"hw params: %d-%d-%d\n",+fs210x->srate,chn_num,fs210x->bclk);++/* The FS2105S can't support 16kHz sample rate. */+if(fs210x->devid==FS2105S_DEVICE_ID&&fs210x->srate==16000)+return-EOPNOTSUPP;++mutex_lock(&fs210x_mutex);+ret=fs210x_set_hw_params(fs210x);+mutex_unlock(&fs210x_mutex);+if(ret)+dev_err(fs210x->dev,"Failed to set hw params: %d\n",ret);++returnret;+}++staticintfs210x_dai_mute(structsnd_soc_dai*dai,intmute,intstream)+{+structfs210x_priv*fs210x;+intret;++if(stream!=SNDRV_PCM_STREAM_PLAYBACK)+return0;++fs210x=snd_soc_component_get_drvdata(dai->component);++if(!fs210x->is_inited)+return-EFAULT;++if(mute){+ret=fs210x_mute(fs210x,true);+fs210x_bclk_set(fs210x,false);+dev_info(fs210x->dev,"playback mute\n");+}else{+fs210x_bclk_set(fs210x,true);+ret=fs210x_mute(fs210x,false);+dev_info(fs210x->dev,"playback unmute\n");+}++returnret;+}++staticvoidfs210x_start_work(structwork_struct*work)+{+structfs210x_priv*fs210x;+intret;++fs210x=container_of(work,structfs210x_priv,start_work.work);++mutex_lock(&fs210x_mutex);++ret=fs210x_dev_play(fs210x);+if(ret)+dev_err(fs210x->dev,"Failed to start playing: %d\n",ret);++mutex_unlock(&fs210x_mutex);+}++staticvoidfs210x_fault_check_work(structwork_struct*work)+{+structfs210x_priv*fs210x;+u16status;+intret;++fs210x=container_of(work,structfs210x_priv,fault_check_work.work);++mutex_lock(&fs210x_mutex);+if(!fs210x->is_inited||!fs210x->is_playing){+mutex_unlock(&fs210x_mutex);+return;+}++ret=fs210x_reg_read(fs210x,FS210X_05H_ANASTAT,&status);+mutex_unlock(&fs210x_mutex);+if(ret)+return;++if(!(status&FS210X_05H_PVDD_MASK))+dev_err(fs210x->dev,"PVDD fault\n");+if(status&FS210X_05H_OCDL_MASK)+dev_err(fs210x->dev,"OC detected\n");+if(status&FS210X_05H_UVDL_MASK)+dev_err(fs210x->dev,"UV detected\n");+if(status&FS210X_05H_OVDL_MASK)+dev_err(fs210x->dev,"OV detected\n");+if(status&FS210X_05H_OTPDL_MASK)+dev_err(fs210x->dev,"OT detected\n");+if(status&FS210X_05H_OCRDL_MASK)+dev_err(fs210x->dev,"OCR detected\n");+if(status&FS210X_05H_OCLDL_MASK)+dev_err(fs210x->dev,"OCL detected\n");+if(status&FS210X_05H_DCRDL_MASK)+dev_err(fs210x->dev,"DCR detected\n");+if(status&FS210X_05H_DCLDL_MASK)+dev_err(fs210x->dev,"DCL detected\n");+if(status&FS210X_05H_SRDL_MASK)+dev_err(fs210x->dev,"SR detected\n");+if(status&FS210X_05H_OTWDL_MASK)+dev_err(fs210x->dev,"OTW detected\n");+if(!(status&FS210X_05H_AMPS_MASK))+dev_err(fs210x->dev,"Amplifier unready\n");+if(!(status&FS210X_05H_PLLS_MASK))+dev_err(fs210x->dev,"PLL unlock\n");+if(!(status&FS210X_05H_ANAS_MASK))+dev_err(fs210x->dev,"Analog power fault\n");++if(status!=FS210X_05H_ANASTAT_OK)+fs210x_reg_dump(fs210x);++schedule_delayed_work(&fs210x->fault_check_work,+msecs_to_jiffies(FS210X_FAULT_CHECK_INTERVAL_MS));+}++staticintfs210x_get_drvdata_from_kctrl(structsnd_kcontrol*kctrl,+structfs210x_priv**fs210x)+{+structsnd_soc_component*cmpnt;++if(!kctrl){+pr_err("fs210x: kcontrol is null\n");+return-EINVAL;+}++cmpnt=snd_soc_kcontrol_component(kctrl);+if(!cmpnt){+pr_err("fs210x: component is null\n");+return-EINVAL;+}++*fs210x=snd_soc_component_get_drvdata(cmpnt);++return0;+}++staticintfs210x_pcm_volume_info(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_info*uinfo)+{+uinfo->type=SNDRV_CTL_ELEM_TYPE_INTEGER;+uinfo->count=2;++uinfo->value.integer.min=FS210X_VOLUME_MIN;+uinfo->value.integer.max=FS210X_VOLUME_MAX;++return0;+}++staticintfs210x_pcm_volume_get(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret){+pr_err("pcm_volume_get: fs210x is null\n");+return-EINVAL;+}++mutex_lock(&fs210x_mutex);+ucontrol->value.integer.value[0]=fs210x->vol[0];+ucontrol->value.integer.value[1]=fs210x->vol[1];+mutex_unlock(&fs210x_mutex);++return0;+}++staticintfs210x_pcm_volume_put(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+long*pval;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("pcm_volume_put: fs210x is null\n");+return-EINVAL;+}++pval=ucontrol->value.integer.value;+if(!volume_is_valid(*pval)||!volume_is_valid(*(pval+1))){+dev_err(fs210x->dev,"Invalid volume: %ld-%ld\n",+*pval,*(pval+1));+return-EINVAL;+}++fs210x->vol[0]=(u16)*pval;+fs210x->vol[1]=(u16)*(pval+1);+dev_dbg(fs210x->dev,"Set volume: %d-%d\n",+fs210x->vol[0],fs210x->vol[1]);++mutex_lock(&fs210x_mutex);++if(fs210x->is_suspended){+mutex_unlock(&fs210x_mutex);+return0;+}++ret=fs210x_set_pcm_volume(fs210x);+if(ret)+dev_err(fs210x->dev,"Failed to set volume: %d\n",ret);++mutex_unlock(&fs210x_mutex);++returnret;+}++staticintfs210x_effect_scene_info(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_info*uinfo)+{+conststructfs_amp_scene*scene;+structfs210x_priv*fs210x;+constchar*name="N/A";+intidx,count;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("scene_effect_info: fs210x is null\n");+return-EINVAL;+}++uinfo->type=SNDRV_CTL_ELEM_TYPE_ENUMERATED;+uinfo->count=1;++count=fs210x->amp_lib.scene_count-1;/* Skip init scene */+if(count<1){+uinfo->value.enumerated.items=0;+return0;+}++uinfo->value.enumerated.items=count;+if(uinfo->value.enumerated.item>=count)+uinfo->value.enumerated.item=count-1;++idx=uinfo->value.enumerated.item;+scene=fs210x->amp_lib.scene+idx+1;+if(scene->name)+name=scene->name;++strscpy(uinfo->value.enumerated.name,name,strlen(name)+1);+dev_dbg(fs210x->dev,"Scene name.%d: %s\n",idx,name);++return0;+}++staticintfs210x_effect_scene_get(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+intindex;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("scene_effect_get: fs210x is null\n");+return-EINVAL;+}++/* The id of effect scene is from 1 to N. */+if(fs210x->scene_id<1)+return-EINVAL;++mutex_lock(&fs210x_mutex);+/*+*FS210xhasscene(s)asbelow:+*initscene:id=0+*effectscene(s):id=1~N(optional)+*effect_index=scene_id-1+*/+index=fs210x->scene_id-1;+ucontrol->value.integer.value[0]=index;+dev_dbg(fs210x->dev,"Get scene index: %d\n",index);+mutex_unlock(&fs210x_mutex);++return0;+}++staticintfs210x_effect_scene_put(structsnd_kcontrol*kcontrol,+structsnd_ctl_elem_value*ucontrol)+{+structfs210x_priv*fs210x;+intscene_id;+intret;++ret=fs210x_get_drvdata_from_kctrl(kcontrol,&fs210x);+if(ret||!fs210x->dev){+pr_err("scene_effect_put: fs210x is null\n");+return-EINVAL;+}++mutex_lock(&fs210x_mutex);++/*+*FS210xhasscene(s)asbelow:+*initscene:id=0(It'ssetinfs210x_init_chip()only)+*effectscene(s):id=1~N(optional)+*scene_id=effect_index+1.+*/+scene_id=ucontrol->value.integer.value[0]+1;+if(fs210x->is_suspended){+fs210x->scene_id=scene_id;+mutex_unlock(&fs210x_mutex);+return0;+}++ret=fs210x_set_scene(fs210x,scene_id);+if(ret)+dev_err(fs210x->dev,"Failed to set scene: %d\n",ret);++mutex_unlock(&fs210x_mutex);++returnret;+}++staticconststructsnd_soc_dai_opsfs210x_dai_ops={+.set_fmt=fs210x_dai_set_fmt,+.hw_params=fs210x_dai_hw_params,+.mute_stream=fs210x_dai_mute,+};++staticconststructsnd_soc_dai_driverfs210x_dai={+.name=FS210X_DEFAULT_DAI_NAME,+.playback={+.stream_name="Playback",+.channels_min=1,+.channels_max=2,+.rates=FS210X_RATES,+.formats=FS210X_FORMATS,+},+.capture={+.stream_name="Capture",+.channels_min=1,+.channels_max=2,+.rates=FS210X_RATES,+.formats=FS210X_FORMATS,+},+.ops=&fs210x_dai_ops,+.symmetric_rate=1,+.symmetric_sample_bits=1,+};++staticconststructsnd_kcontrol_newfs210x_controls[]={+FS_SOC_ENUM_EXT("PCM Playback Volume",+fs210x_pcm_volume_info,+fs210x_pcm_volume_get,+fs210x_pcm_volume_put),+};++staticconststructsnd_kcontrol_newfs210x_scene_control[]={+FS_SOC_ENUM_EXT("Effect Scene",+fs210x_effect_scene_info,+fs210x_effect_scene_get,+fs210x_effect_scene_put),+};++staticconststructsnd_soc_dapm_widgetfs210x_dapm_widgets[]={+SND_SOC_DAPM_AIF_IN("AIF IN","Playback",0,SND_SOC_NOPM,0,0),+SND_SOC_DAPM_AIF_OUT("AIF OUT","Capture",0,SND_SOC_NOPM,0,0),+SND_SOC_DAPM_OUTPUT("OUTL"),+SND_SOC_DAPM_OUTPUT("OUTR"),+SND_SOC_DAPM_INPUT("SDO"),+};++staticconststructsnd_soc_dapm_routefs210x_dapm_routes[]={+{"OUTL",NULL,"AIF IN"},+{"OUTR",NULL,"AIF IN"},+{"AIF OUT",NULL,"SDO"},+};++staticintfs210x_add_optional_controls(structfs210x_priv*fs210x,+structsnd_soc_component*cmpnt)+{+intcount;++if(!fs210x||!cmpnt)+return-EINVAL;++/*+*Ifthefirmwarehasnosceneoronlyinitscene,+*weskipaddingthismixercontrol.+*/+if(fs210x->amp_lib.scene_count<2)+return0;++count=ARRAY_SIZE(fs210x_scene_control);++returnsnd_soc_add_component_controls(cmpnt,+fs210x_scene_control,+count);+}++staticintfs210x_get_bclk(structfs210x_priv*fs210x,+structsnd_soc_component*cmpnt)+{+structclk*bclk;+intret;++bclk=devm_clk_get(fs210x->dev,"bclk");+if(IS_ERR_OR_NULL(bclk)){+ret=bclk?PTR_ERR(bclk):-ENODATA;+if(ret==-EPROBE_DEFER)+returnret;+/*+*IftheSOCdoesn'thavethebclkclocksource,+*weskipsettingthebclkclock.+*/+return0;+}++fs210x->clk_bclk=bclk;+dev_dbg(fs210x->dev,"Got I2S bclk clock\n");++return0;+}++staticintfs210x_probe(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;+intret;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return-EINVAL;++fs210x->amp_lib.dev=fs210x->dev;+fs210x->amp_lib.devid=fs210x->devid;++ret=fs_amp_load_firmware(&fs210x->amp_lib,fs210x->pdata.fwm_name);+if(ret){+dev_err(fs210x->dev,"Failed to load firmware: %d\n",ret);+returnret;+}++ret=fs210x_add_optional_controls(fs210x,cmpnt);+if(ret){+dev_err(fs210x->dev,"Failed to add opt-ctrl: %d\n",ret);+returnret;+}++INIT_DELAYED_WORK(&fs210x->fault_check_work,fs210x_fault_check_work);+INIT_DELAYED_WORK(&fs210x->start_work,fs210x_start_work);++fs210x_get_bclk(fs210x,cmpnt);++mutex_lock(&fs210x_mutex);+ret=fs210x_init_chip(fs210x);+mutex_unlock(&fs210x_mutex);+if(ret)+dev_err(fs210x->dev,"Failed to probe: %d\n",ret);++returnret;+}++staticvoidfs210x_remove(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return;++cancel_delayed_work_sync(&fs210x->start_work);+cancel_delayed_work_sync(&fs210x->fault_check_work);++dev_dbg(fs210x->dev,"Codec removed\n");+}++#ifdef CONFIG_PM+staticintfs210x_suspend(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;+intret;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return-EINVAL;++cancel_delayed_work_sync(&fs210x->start_work);+cancel_delayed_work_sync(&fs210x->fault_check_work);++mutex_lock(&fs210x_mutex);+fs210x->cur_scene=NULL;+fs210x->is_inited=false;+fs210x->is_playing=false;+fs210x->is_suspended=true;++fs210x_sdz_pin_set(fs210x,true);+mutex_unlock(&fs210x_mutex);++ret=regulator_bulk_disable(FS210X_NUM_SUPPLIES,fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to suspend: %d\n",ret);+returnret;+}++dev_info(fs210x->dev,"pm suspended\n");++return0;+}++staticintfs210x_resume(structsnd_soc_component*cmpnt)+{+structfs210x_priv*fs210x;+intret;++fs210x=snd_soc_component_get_drvdata(cmpnt);+if(!fs210x||!fs210x->dev)+return-EINVAL;++ret=regulator_bulk_enable(FS210X_NUM_SUPPLIES,fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to enable supplies: %d\n",ret);+returnret;+}++mutex_lock(&fs210x_mutex);+fs210x_sdz_pin_set(fs210x,false);++fs210x->is_suspended=false;+ret=fs210x_init_chip(fs210x);+mutex_unlock(&fs210x_mutex);++dev_info(fs210x->dev,"pm resumed\n");++returnret;+}+#else+#define fs210x_suspend NULL+#define fs210x_resume NULL+#endif // CONFIG_PM++staticconststructsnd_soc_component_driverfs210x_soc_component_dev={+.probe=fs210x_probe,+.remove=fs210x_remove,+.suspend=fs210x_suspend,+.resume=fs210x_resume,+.controls=fs210x_controls,+.num_controls=ARRAY_SIZE(fs210x_controls),+.dapm_widgets=fs210x_dapm_widgets,+.num_dapm_widgets=ARRAY_SIZE(fs210x_dapm_widgets),+.dapm_routes=fs210x_dapm_routes,+.num_dapm_routes=ARRAY_SIZE(fs210x_dapm_routes),+.use_pmdown_time=1,+};++staticconststructregmap_configfs210x_regmap={+.reg_bits=8,+.val_bits=16,+.max_register=FS210X_REG_MAX,+.val_format_endian=REGMAP_ENDIAN_BIG,+.cache_type=REGCACHE_NONE,+};++staticintfs210x_detect_device(structfs210x_priv*fs210x)+{+u16devid;+intret;++ret=fs210x_reg_read(fs210x,FS210X_03H_DEVID,&devid);+if(ret)+returnret;++fs210x->devid=HI_U16(devid);++switch(fs210x->devid){+caseFS210X_DEVICE_ID:+dev_info(fs210x->dev,"FS2104 detected\n");+break;+caseFS2105S_DEVICE_ID:+dev_info(fs210x->dev,"FS2105S detected\n");+break;+default:+dev_err(fs210x->dev,"DEVID: 0x%04X dismatch\n",devid);+return-ENODEV;+}++return0;+}++staticintfs210x_parse_dts(structfs210x_priv*fs210x,+structfs210x_platform_data*pdata)+{+structdevice_node*node=fs210x->dev->of_node;+inti,ret;++if(!node)+return0;++ret=of_property_read_string(node,"firmware-name",&pdata->fwm_name);+if(ret)+pdata->fwm_name=FS210X_DEFAULT_FWM_NAME;++fs210x->gpio_sdz=devm_gpiod_get_optional(fs210x->dev,+"reset",GPIOD_OUT_HIGH);+if(IS_ERR_OR_NULL(fs210x->gpio_sdz)){+ret=fs210x->gpio_sdz?PTR_ERR(fs210x->gpio_sdz):-ENODATA;+fs210x->gpio_sdz=NULL;+if(ret==-EPROBE_DEFER)+returnret;+dev_dbg(fs210x->dev,"Assuming reset-gpios is unused\n");+}else{+dev_dbg(fs210x->dev,"reset-gpios: %d\n",+desc_to_gpio(fs210x->gpio_sdz));+}++for(i=0;i<FS210X_NUM_SUPPLIES;i++)+fs210x->supplies[i].supply=fs210x_supply_names[i];++ret=devm_regulator_bulk_get(fs210x->dev,+ARRAY_SIZE(fs210x->supplies),+fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to get supplies: %d\n",ret);+returnret;+}++return0;+}++staticintfs210x_parse_platdata(structfs210x_priv*fs210x)+{+structfs210x_platform_data*pdata;+intret;++pdata=&fs210x->pdata;+ret=fs210x_parse_dts(fs210x,pdata);+if(ret){+dev_err(fs210x->dev,"Failed to parse dts: %d\n",ret);+returnret;+}++return0;+}++staticvoidfs210x_deinit(structfs210x_priv*fs210x)+{+fs210x_sdz_pin_set(fs210x,true);+regulator_bulk_disable(FS210X_NUM_SUPPLIES,fs210x->supplies);+}++staticintfs210x_init(structfs210x_priv*fs210x)+{+intret;++ret=fs210x_parse_platdata(fs210x);+if(ret){+dev_err(fs210x->dev,"Failed to parse platdata: %d\n",ret);+returnret;+}++ret=regulator_bulk_enable(FS210X_NUM_SUPPLIES,fs210x->supplies);+if(ret){+dev_err(fs210x->dev,"Failed to enable supplies: %d\n",ret);+returnret;+}++/* Make sure the SDZ pin is pulled down enough time. */+usleep_range(10000,10050);/* >= 10ms */+fs210x_sdz_pin_set(fs210x,false);++ret=fs210x_detect_device(fs210x);+if(ret){+fs210x_deinit(fs210x);+returnret;+}++fs210x->scene_id=-1;/* Invalid scene */+fs210x->cur_scene=NULL;+fs210x->is_playing=false;+fs210x->is_inited=false;+fs210x->is_suspended=false;+fs210x->vol[0]=FS210X_VOLUME_MAX;+fs210x->vol[1]=FS210X_VOLUME_MAX;++return0;+}++staticintfs210x_register_snd_component(structfs210x_priv*fs210x)+{+structsnd_soc_dai_driver*dai_drv;+intret;++dai_drv=devm_kmemdup(fs210x->dev,&fs210x_dai,+sizeof(fs210x_dai),GFP_KERNEL);+if(!dai_drv)+return-ENOMEM;++if(fs210x->devid==FS2105S_DEVICE_ID){+dai_drv->playback.rates=FS2105S_RATES;+dai_drv->capture.rates=FS2105S_RATES;+}++ret=snd_soc_register_component(fs210x->dev,+&fs210x_soc_component_dev,+dai_drv,1);+returnret;+}++staticintfs210x_i2c_probe(structi2c_client*client)+{+structfs210x_priv*fs210x;+intret;++dev_info(&client->dev,"version: %s\n",FS210X_DRV_VERSION);++fs210x=devm_kzalloc(&client->dev,sizeof(*fs210x),GFP_KERNEL);+if(!fs210x)+return-ENOMEM;++fs210x->i2c=client;+fs210x->dev=&client->dev;+i2c_set_clientdata(client,fs210x);++fs210x->regmap=devm_regmap_init_i2c(client,&fs210x_regmap);+if(IS_ERR_OR_NULL(fs210x->regmap)){+dev_err(fs210x->dev,"Failed to get regmap\n");+ret=fs210x->regmap?PTR_ERR(fs210x->regmap):-ENODATA;+returnret;+}++mutex_lock(&fs210x_mutex);+ret=fs210x_init(fs210x);+mutex_unlock(&fs210x_mutex);+if(ret)+returnret;++ret=fs210x_register_snd_component(fs210x);+if(ret){+fs210x_deinit(fs210x);+returnret;+}++return0;+}++staticvoidfs210x_i2c_remove(structi2c_client*client)+{+structfs210x_priv*fs210x=i2c_get_clientdata(client);++snd_soc_unregister_component(fs210x->dev);+fs210x_deinit(fs210x);+}++staticconststructi2c_device_idfs210x_i2c_id[]={+{"fs2104"},+{"fs2105s"},+{}+};+MODULE_DEVICE_TABLE(i2c,fs210x_i2c_id);++#ifdef CONFIG_OF+staticconststructof_device_idfs210x_of_match[]={+{.compatible="foursemi,fs2104",},+{.compatible="foursemi,fs2105s",},+{},+};+MODULE_DEVICE_TABLE(of,fs210x_of_match);+#endif // CONFIG_OF++staticstructi2c_driverfs210x_i2c_driver={+.driver={+.name="fs210x",+.of_match_table=of_match_ptr(fs210x_of_match),+},+.id_table=fs210x_i2c_id,+.probe=fs210x_i2c_probe,+.remove=fs210x_i2c_remove,+};++module_i2c_driver(fs210x_i2c_driver);++MODULE_AUTHOR("Nick Li <nick.li@foursemi.com>");+MODULE_DESCRIPTION("FS2104/5S Audio Amplifier Driver");+MODULE_VERSION(FS210X_DRV_VERSION);+MODULE_LICENSE("GPL");
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-08 11:31:17
Add a DT schema for describing FourSemi FS2104/5S
audio amplifiers which support both I2S and I2C interface.
Signed-off-by: Nick Li <nick.li@foursemi.com>
---
.../bindings/sound/foursemi,fs2105s.yaml | 100 ++++++++++++++++++
1 file changed, 100 insertions(+)
create mode 100644 Documentation/devicetree/bindings/sound/foursemi,fs2105s.yaml
@@ -0,0 +1,100 @@+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)+%YAML1.2+---+$id:http://devicetree.org/schemas/sound/foursemi,fs2105s.yaml#+$schema:http://devicetree.org/meta-schemas/core.yaml#++title:FourSemi FS2104/5S Digital Audio Amplifier++maintainers:+-Nick Li <nick.li@foursemi.com>++description:+The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,+Digital Input Class-D Power Amplifier with Enhanced Signal Processing.+The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,+Digital Input Class-D Power Amplifier with Enhanced Signal Processing.++properties:+compatible:+oneOf:+-items:+-enum:+-foursemi,fs2104+-const:foursemi,fs2105s+-enum:+-foursemi,fs2105s++reg:+maxItems:1+description:+I2C address of the device. Refer to datasheet for possible values++clocks:+description:The clock of I2S BCLK++clock-names:+items:+-const:bclk++interrupts:+maxItems:1++'#sound-dai-cells':+const:0++pvdd-supply:+description:+Regulator for power supply(PVDD in datasheet).++dvdd-supply:+description:+Regulator for digital supply(DVDD in datasheet).++reset-gpios:+maxItems:1+description:+It's the SDZ pin in datasheet, the pin is active low,+it will power down and reset the chip to shut down state.++firmware-name:+maxItems:1+description:|+The firmware(*.bin) contains:+a. Register initialization settings+b. DSP effect parameters+c. Multi-scene sound effect configurations(optional)+It's gernerated by FourSemi's tuning tool.++required:+-compatible+-reg+-reset-gpios+-firmware-name+-'#sound-dai-cells'++allOf:+-$ref:dai-common.yaml#++unevaluatedProperties:false++examples:+-|+#include <dt-bindings/gpio/gpio.h>+i2c {+#address-cells = <1>;+#size-cells = <0>;+audio-codec@68 {+compatible = "foursemi,fs2105s";+reg = <0x68>;+pvdd-supply = <&pvdd_supply>;+dvdd-supply = <&dvdd_supply>;+reset-gpios = <&gpio 18 GPIO_ACTIVE_LOW>;+firmware-name = "fs2105s-btl-2p0-0s.bin";+pinctrl-names = "default";+pinctrl-0 = <&fs210x_pins_default>;+clocks = <&clocks 18>;+clock-names = "bclk";+#sound-dai-cells = <0>;+};+};
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2025-07-09 10:40:36
On Tue, Jul 08, 2025 at 07:28:59PM +0800, Nick Li wrote:
quoted hunk
+description:+ The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.+ The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.++properties:+ compatible:+ oneOf:+ - items:+ - enum:+ - foursemi,fs2104+ - const: foursemi,fs2105s+ - enum:+ - foursemi,fs2105s++ reg:+ maxItems: 1+ description:+ I2C address of the device. Refer to datasheet for possible values
Now the description is entirely redundant, brings no value. Drop.
quoted hunk
++ clocks:+ description: The clock of I2S BCLK
This was different... Previous code was correct, this is not correct.
And nothing in changelog explains this. Do not make random changes after
review.
quoted hunk
++ clock-names:+ items:+ - const: bclk++ interrupts:+ maxItems: 1++ '#sound-dai-cells':+ const: 0++ pvdd-supply:+ description:+ Regulator for power supply(PVDD in datasheet).++ dvdd-supply:+ description:+ Regulator for digital supply(DVDD in datasheet).++ reset-gpios:+ maxItems: 1+ description:+ It's the SDZ pin in datasheet, the pin is active low,+ it will power down and reset the chip to shut down state.++ firmware-name:+ maxItems: 1+ description: |+ The firmware(*.bin) contains:+ a. Register initialization settings+ b. DSP effect parameters+ c. Multi-scene sound effect configurations(optional)+ It's gernerated by FourSemi's tuning tool.++required:+ - compatible+ - reg+ - reset-gpios+ - firmware-name+ - '#sound-dai-cells'
Keep the same order as in list of properties. OTOH, missing supplies.
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2025-07-09 10:42:38
On Tue, Jul 08, 2025 at 07:28:59PM +0800, Nick Li wrote:
Add a DT schema for describing FourSemi FS2104/5S
audio amplifiers which support both I2S and I2C interface.
Another unexpected change from v1: subject: why did you add "schema"?
I asked to drop it and gave you reference explaining this. Did you read
it?
So again, same template:
A nit, subject: drop second/last, redundant "schema for". The
"dt-bindings" prefix is already stating that these are bindings in
schema format, because they cannot be anything else.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Best regards,
Krzysztof
++static int fs210x_add_optional_controls(struct fs210x_priv *fs210x,+ struct snd_soc_component *cmpnt)+{+ int count;++ if (!fs210x || !cmpnt)+ return -EINVAL;++ /*+ * If the firmware has no scene or only init scene,+ * we skip adding this mixer control.+ */+ if (fs210x->amp_lib.scene_count < 2)+ return 0;++ count = ARRAY_SIZE(fs210x_scene_control);++ return snd_soc_add_component_controls(cmpnt,+ fs210x_scene_control,+ count);+}++static int fs210x_get_bclk(struct fs210x_priv *fs210x,+ struct snd_soc_component *cmpnt)+{+ struct clk *bclk;+ int ret;++ bclk = devm_clk_get(fs210x->dev, "bclk");+ if (IS_ERR_OR_NULL(bclk)) {+ ret = bclk ? PTR_ERR(bclk) : -ENODATA;
Same pattern as regulators, eh...
+ if (ret == -EPROBE_DEFER)
No. Stop handling own probe deferrals. Look how other drivers do it.
quoted hunk
+ return ret;+ /*+ * If the SOC doesn't have the bclk clock source,+ * we skip setting the bclk clock.+ */+ return 0;
What is the point of this entire code? You got NULL, so assign NULL. Can
clk API handle NULLs? Answer this to yourself and write obvious, simple
code.
Drop. Really, your debugs here and further are meaningless. You debug
static, well know, things - DTB. No, debug unexpected pieces, not
something which is 100% predictable because DT schema told you this
already.
quoted hunk
++ return 0;+}++static int fs210x_probe(struct snd_soc_component *cmpnt)+{+ struct fs210x_priv *fs210x;+ int ret;++ fs210x = snd_soc_component_get_drvdata(cmpnt);+ if (!fs210x || !fs210x->dev)+ return -EINVAL;++ fs210x->amp_lib.dev = fs210x->dev;+ fs210x->amp_lib.devid = fs210x->devid;++ ret = fs_amp_load_firmware(&fs210x->amp_lib, fs210x->pdata.fwm_name);+ if (ret) {+ dev_err(fs210x->dev, "Failed to load firmware: %d\n", ret);+ return ret;+ }++ ret = fs210x_add_optional_controls(fs210x, cmpnt);+ if (ret) {+ dev_err(fs210x->dev, "Failed to add opt-ctrl: %d\n", ret);+ return ret;+ }++ INIT_DELAYED_WORK(&fs210x->fault_check_work, fs210x_fault_check_work);+ INIT_DELAYED_WORK(&fs210x->start_work, fs210x_start_work);++ fs210x_get_bclk(fs210x, cmpnt);++ mutex_lock(&fs210x_mutex);+ ret = fs210x_init_chip(fs210x);+ mutex_unlock(&fs210x_mutex);+ if (ret)+ dev_err(fs210x->dev, "Failed to probe: %d\n", ret);
Oh wait, it is the FOURTH time you print same error message. Sorry, this
error handling is terrible. Obfuscated and overcomplicated. Error should
be printed only once. Look at other recent codecs.
This is over-complicated way of getting simple optional gpio.
quoted hunk
++ for (i = 0; i < FS210X_NUM_SUPPLIES; i++)+ fs210x->supplies[i].supply = fs210x_supply_names[i];++ ret = devm_regulator_bulk_get(fs210x->dev,+ ARRAY_SIZE(fs210x->supplies),+ fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to get supplies: %d\n", ret);
Syntax is return dev_err_probe.
quoted hunk
+ return ret;+ }++ return 0;+}++static int fs210x_parse_platdata(struct fs210x_priv *fs210x)
I do not understand why you have so many functions doing simple OF
parsing. fs210x_init, fs210x_parse_platdata, fs210x_parse_dts... and
this one here does nothing.
quoted hunk
+{+ struct fs210x_platform_data *pdata;+ int ret;++ pdata = &fs210x->pdata;+ ret = fs210x_parse_dts(fs210x, pdata);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse dts: %d\n", ret);+ return ret;+ }++ return 0;+}++static void fs210x_deinit(struct fs210x_priv *fs210x)+{+ fs210x_sdz_pin_set(fs210x, true);+ regulator_bulk_disable(FS210X_NUM_SUPPLIES, fs210x->supplies);+}++static int fs210x_init(struct fs210x_priv *fs210x)+{+ int ret;++ ret = fs210x_parse_platdata(fs210x);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse platdata: %d\n", ret);
So you print SAME ERROR three times?
quoted hunk
+ return ret;+ }++ ret = regulator_bulk_enable(FS210X_NUM_SUPPLIES, fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to enable supplies: %d\n", ret);+ return ret;+ }++ /* Make sure the SDZ pin is pulled down enough time. */+ usleep_range(10000, 10050); /* >= 10ms */+ fs210x_sdz_pin_set(fs210x, false);++ ret = fs210x_detect_device(fs210x);+ if (ret) {+ fs210x_deinit(fs210x);+ return ret;+ }++ fs210x->scene_id = -1; /* Invalid scene */+ fs210x->cur_scene = NULL;+ fs210x->is_playing = false;+ fs210x->is_inited = false;+ fs210x->is_suspended = false;+ fs210x->vol[0] = FS210X_VOLUME_MAX;+ fs210x->vol[1] = FS210X_VOLUME_MAX;++ return 0;+}++static int fs210x_register_snd_component(struct fs210x_priv *fs210x)+{+ struct snd_soc_dai_driver *dai_drv;+ int ret;++ dai_drv = devm_kmemdup(fs210x->dev, &fs210x_dai,+ sizeof(fs210x_dai), GFP_KERNEL);+ if (!dai_drv)+ return -ENOMEM;++ if (fs210x->devid == FS2105S_DEVICE_ID) {+ dai_drv->playback.rates = FS2105S_RATES;+ dai_drv->capture.rates = FS2105S_RATES;+ }++ ret = snd_soc_register_component(fs210x->dev,+ &fs210x_soc_component_dev,+ dai_drv, 1);+ return ret;+}++static int fs210x_i2c_probe(struct i2c_client *client)+{+ struct fs210x_priv *fs210x;+ int ret;++ dev_info(&client->dev, "version: %s\n", FS210X_DRV_VERSION);++ fs210x = devm_kzalloc(&client->dev, sizeof(*fs210x), GFP_KERNEL);+ if (!fs210x)+ return -ENOMEM;++ fs210x->i2c = client;+ fs210x->dev = &client->dev;+ i2c_set_clientdata(client, fs210x);++ fs210x->regmap = devm_regmap_init_i2c(client, &fs210x_regmap);+ if (IS_ERR_OR_NULL(fs210x->regmap)) {
Can devm_regmap_init_i2c() return NULL? No, it cannot.
+ dev_err(fs210x->dev, "Failed to get regmap\n");
+ ret = fs210x->regmap ? PTR_ERR(fs210x->regmap) : -ENODATA;
Syntax is return dev_err_probe and drop NULL check.
quoted hunk
+ return ret;+ }++ mutex_lock(&fs210x_mutex);+ ret = fs210x_init(fs210x);+ mutex_unlock(&fs210x_mutex);
Why do you need to lock it? Who and how can access device at this point?
From: Mark Brown <broonie@kernel.org> Date: 2025-07-09 11:29:34
On Tue, Jul 08, 2025 at 07:29:01PM +0800, Nick Li wrote:
The FS2104/5S are FourSemi digital audio amplifiers
with I2C control. They are Inductor-Less, Stereo, Closed-Loop,
Digital Input Class-D Power Amplifiers with Enhanced Signal Processing.
This looks broadly OK - there's a few more fairly small issues below in
addition to those that Krzysztof flagged.
quoted hunk
+static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)+{+ u16 vol[2];+ int ret;++ if (fs210x->devid == FS2105S_DEVICE_ID) {
A swtich statement is better style here, it makes it easier to add more
variants.
+ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
_CLOCK_PROVIDER_MASK
+ case SND_SOC_DAIFMT_CBC_CFC:
+ /* Only supports slave mode */
+static int fs210x_dai_hw_params(struct snd_pcm_substream *substream,+ struct snd_pcm_hw_params *params,+ struct snd_soc_dai *dai)+{+ struct fs210x_priv *fs210x;+ int chn_num;+ int ret;++ if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)+ return 0;
There's no configuration for capture? Should the driver be setting
symmetric_rates and then not reconfiguring if active instead, it looks a
lot like the driver is relying on userspace starting playback before
capture to configure?
quoted hunk
+static int fs210x_pcm_volume_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{
The driver should return 1 if there's a change in the values and 0
otherwise (plus it can skip a bunch of work if there's nothing to do).
The mixer-test test will spot this for you.
quoted hunk
+static int fs210x_effect_scene_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{
quoted hunk
+ ret = fs210x_set_scene(fs210x, scene_id);+ if (ret)+ dev_err(fs210x->dev, "Failed to set scene: %d\n", ret);++ mutex_unlock(&fs210x_mutex);++ return ret;
Why is this file-scope? Why two independent codecs cannot work in parallel?
The driver module may be loaded asynchronously,
if the reset pin/supplies are shared by the devices,
we should protect the process of detecting devices.
We tend to have each device is configured in a continuous manner.
++static int fs210x_add_optional_controls(struct fs210x_priv *fs210x,+ struct snd_soc_component *cmpnt)+{+ int count;++ if (!fs210x || !cmpnt)+ return -EINVAL;++ /*+ * If the firmware has no scene or only init scene,+ * we skip adding this mixer control.+ */+ if (fs210x->amp_lib.scene_count < 2)+ return 0;++ count = ARRAY_SIZE(fs210x_scene_control);++ return snd_soc_add_component_controls(cmpnt,+ fs210x_scene_control,+ count);+}++static int fs210x_get_bclk(struct fs210x_priv *fs210x,+ struct snd_soc_component *cmpnt)+{+ struct clk *bclk;+ int ret;++ bclk = devm_clk_get(fs210x->dev, "bclk");+ if (IS_ERR_OR_NULL(bclk)) {+ ret = bclk ? PTR_ERR(bclk) : -ENODATA;
Same pattern as regulators, eh...
Ok, we will update it.
quoted
+ if (ret == -EPROBE_DEFER)
No. Stop handling own probe deferrals. Look how other drivers do it.
Broonie recommanded to get clock in bus probe before,
and we will call it in i2c probe,
is it possible the clock isn't ready when we get it in bus probe?
we found some drivers do the probe deferral after getting clock.
quoted
+ return ret;+ /*+ * If the SOC doesn't have the bclk clock source,+ * we skip setting the bclk clock.+ */+ return 0;
What is the point of this entire code? You got NULL, so assign NULL. Can
clk API handle NULLs? Answer this to yourself and write obvious, simple
code.
Before we calling clk API in fs210x_bclk_set, we check the clk_bclk is NULL or not firstly,
In clk_set_rate/clk_prepare_enable/clk_disable_prepare, they will check it:
if (!clk) or if (IS_ERR_OR_NULL(clk))
Drop. Really, your debugs here and further are meaningless. You debug
static, well know, things - DTB. No, debug unexpected pieces, not
something which is 100% predictable because DT schema told you this
already.
We will drop the debug log.
quoted
++ return 0;+}++static int fs210x_probe(struct snd_soc_component *cmpnt)+{+ struct fs210x_priv *fs210x;+ int ret;++ fs210x = snd_soc_component_get_drvdata(cmpnt);+ if (!fs210x || !fs210x->dev)+ return -EINVAL;++ fs210x->amp_lib.dev = fs210x->dev;+ fs210x->amp_lib.devid = fs210x->devid;++ ret = fs_amp_load_firmware(&fs210x->amp_lib, fs210x->pdata.fwm_name);+ if (ret) {+ dev_err(fs210x->dev, "Failed to load firmware: %d\n", ret);+ return ret;+ }++ ret = fs210x_add_optional_controls(fs210x, cmpnt);+ if (ret) {+ dev_err(fs210x->dev, "Failed to add opt-ctrl: %d\n", ret);+ return ret;+ }++ INIT_DELAYED_WORK(&fs210x->fault_check_work, fs210x_fault_check_work);+ INIT_DELAYED_WORK(&fs210x->start_work, fs210x_start_work);++ fs210x_get_bclk(fs210x, cmpnt);++ mutex_lock(&fs210x_mutex);+ ret = fs210x_init_chip(fs210x);+ mutex_unlock(&fs210x_mutex);+ if (ret)+ dev_err(fs210x->dev, "Failed to probe: %d\n", ret);
Oh wait, it is the FOURTH time you print same error message. Sorry, this
error handling is terrible. Obfuscated and overcomplicated. Error should
be printed only once. Look at other recent codecs.
OK, we will check and reduce the error logs when the API has error logs.
This is over-complicated way of getting simple optional gpio.
We want to cover the following possibilities:
1. The reset gpio is unused
2. The reset pin is shared by multiple deivces
3. The reset pins are independent
4. The gpio pin is unready
quoted
++ for (i = 0; i < FS210X_NUM_SUPPLIES; i++)+ fs210x->supplies[i].supply = fs210x_supply_names[i];++ ret = devm_regulator_bulk_get(fs210x->dev,+ ARRAY_SIZE(fs210x->supplies),+ fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to get supplies: %d\n", ret);
Syntax is return dev_err_probe.
We can port the driver into older kernel easily without dev_err_probe,
the older kernel may not have this API.
If it is recommended, we will update it.
quoted
+ return ret;+ }++ return 0;+}++static int fs210x_parse_platdata(struct fs210x_priv *fs210x)
I do not understand why you have so many functions doing simple OF
parsing. fs210x_init, fs210x_parse_platdata, fs210x_parse_dts... and
this one here does nothing.
We parsed the acpi table in parse_platdata before v1,
but we didn't have the environment to check, then we removed the code.
If it's possible, we will add it in the future.
Also we tend to implment the functions shortly to reduce the complexity.
quoted
+{+ struct fs210x_platform_data *pdata;+ int ret;++ pdata = &fs210x->pdata;+ ret = fs210x_parse_dts(fs210x, pdata);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse dts: %d\n", ret);+ return ret;+ }++ return 0;+}++static void fs210x_deinit(struct fs210x_priv *fs210x)+{+ fs210x_sdz_pin_set(fs210x, true);+ regulator_bulk_disable(FS210X_NUM_SUPPLIES, fs210x->supplies);+}++static int fs210x_init(struct fs210x_priv *fs210x)+{+ int ret;++ ret = fs210x_parse_platdata(fs210x);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse platdata: %d\n", ret);
So you print SAME ERROR three times?
We will check and reduce the logs when the api has logs.
quoted
+ return ret;+ }++ ret = regulator_bulk_enable(FS210X_NUM_SUPPLIES, fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to enable supplies: %d\n", ret);+ return ret;+ }++ /* Make sure the SDZ pin is pulled down enough time. */+ usleep_range(10000, 10050); /* >= 10ms */+ fs210x_sdz_pin_set(fs210x, false);++ ret = fs210x_detect_device(fs210x);+ if (ret) {+ fs210x_deinit(fs210x);+ return ret;+ }++ fs210x->scene_id = -1; /* Invalid scene */+ fs210x->cur_scene = NULL;+ fs210x->is_playing = false;+ fs210x->is_inited = false;+ fs210x->is_suspended = false;+ fs210x->vol[0] = FS210X_VOLUME_MAX;+ fs210x->vol[1] = FS210X_VOLUME_MAX;++ return 0;+}++static int fs210x_register_snd_component(struct fs210x_priv *fs210x)+{+ struct snd_soc_dai_driver *dai_drv;+ int ret;++ dai_drv = devm_kmemdup(fs210x->dev, &fs210x_dai,+ sizeof(fs210x_dai), GFP_KERNEL);+ if (!dai_drv)+ return -ENOMEM;++ if (fs210x->devid == FS2105S_DEVICE_ID) {+ dai_drv->playback.rates = FS2105S_RATES;+ dai_drv->capture.rates = FS2105S_RATES;+ }++ ret = snd_soc_register_component(fs210x->dev,+ &fs210x_soc_component_dev,+ dai_drv, 1);+ return ret;+}++static int fs210x_i2c_probe(struct i2c_client *client)+{+ struct fs210x_priv *fs210x;+ int ret;++ dev_info(&client->dev, "version: %s\n", FS210X_DRV_VERSION);++ fs210x = devm_kzalloc(&client->dev, sizeof(*fs210x), GFP_KERNEL);+ if (!fs210x)+ return -ENOMEM;++ fs210x->i2c = client;+ fs210x->dev = &client->dev;+ i2c_set_clientdata(client, fs210x);++ fs210x->regmap = devm_regmap_init_i2c(client, &fs210x_regmap);+ if (IS_ERR_OR_NULL(fs210x->regmap)) {
Can devm_regmap_init_i2c() return NULL? No, it cannot.
OK, we will remove the judgment of NULL pointor
quoted
+ dev_err(fs210x->dev, "Failed to get regmap\n");
+ ret = fs210x->regmap ? PTR_ERR(fs210x->regmap) : -ENODATA;
Syntax is return dev_err_probe and drop NULL check.
Refer to the reply in regulator get.
quoted
+ return ret;+ }++ mutex_lock(&fs210x_mutex);+ ret = fs210x_init(fs210x);+ mutex_unlock(&fs210x_mutex);
Why do you need to lock it? Who and how can access device at this point?
If the system has more than 1 devices:
the module may be loaded asynchronously, if the gpio/supplies are shared,
it's better to protect the detection with lock?
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-10 08:03:18
On Wed, Jul 09, 2025 at 12:42:35PM +0200, Krzysztof Kozlowski wrote:
On Tue, Jul 08, 2025 at 07:28:59PM +0800, Nick Li wrote:
quoted
Add a DT schema for describing FourSemi FS2104/5S
audio amplifiers which support both I2S and I2C interface.
Another unexpected change from v1: subject: why did you add "schema"?
I asked to drop it and gave you reference explaining this. Did you read
it?
So again, same template:
A nit, subject: drop second/last, redundant "schema for". The
"dt-bindings" prefix is already stating that these are bindings in
schema format, because they cannot be anything else.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Sorry, I misunderstood, I will fix it by dropping "schema for ".
Best regards,
Nick
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-10 08:11:48
On Wed, Jul 09, 2025 at 12:40:33PM +0200, Krzysztof Kozlowski wrote:
On Tue, Jul 08, 2025 at 07:28:59PM +0800, Nick Li wrote:
quoted
+description:+ The FS2104 is a 15W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.+ The FS2105S is a 30W Inductor-Less, Stereo, Closed-Loop,+ Digital Input Class-D Power Amplifier with Enhanced Signal Processing.++properties:+ compatible:+ oneOf:+ - items:+ - enum:+ - foursemi,fs2104+ - const: foursemi,fs2105s+ - enum:+ - foursemi,fs2105s++ reg:+ maxItems: 1+ description:+ I2C address of the device. Refer to datasheet for possible values
Now the description is entirely redundant, brings no value. Drop.
OK.
quoted
++ clocks:+ description: The clock of I2S BCLK
This was different... Previous code was correct, this is not correct.
And nothing in changelog explains this. Do not make random changes after
review.
OK, I will recover it to version v1.
quoted
++ clock-names:+ items:+ - const: bclk++ interrupts:+ maxItems: 1++ '#sound-dai-cells':+ const: 0++ pvdd-supply:+ description:+ Regulator for power supply(PVDD in datasheet).++ dvdd-supply:+ description:+ Regulator for digital supply(DVDD in datasheet).++ reset-gpios:+ maxItems: 1+ description:+ It's the SDZ pin in datasheet, the pin is active low,+ it will power down and reset the chip to shut down state.++ firmware-name:+ maxItems: 1+ description: |+ The firmware(*.bin) contains:+ a. Register initialization settings+ b. DSP effect parameters+ c. Multi-scene sound effect configurations(optional)+ It's gernerated by FourSemi's tuning tool.++required:+ - compatible+ - reg+ - reset-gpios+ - firmware-name+ - '#sound-dai-cells'
Keep the same order as in list of properties. OTOH, missing supplies.
OK, we will fix the order, but the supplies may not be used as regulator,
we mark them as required, is it OK?
Best regards,
Nick
From: Krzysztof Kozlowski <krzk@kernel.org> Date: 2025-07-10 08:27:22
On 10/07/2025 09:56, Nick Li wrote:
quoted
quoted
+};++static DEFINE_MUTEX(fs210x_mutex);
Why is this file-scope? Why two independent codecs cannot work in parallel?
The driver module may be loaded asynchronously,
if the reset pin/supplies are shared by the devices,
we should protect the process of detecting devices.
No, that's not the job of this driver. Your driver must not protect from
imaginary resource conflicts, because it will not even solve it
properly. It is impossible. What if foo,AS9911 codec also shares these
pins supplies?
And supplies needs synchronization? About pins you got point, but here
clearly you are wrong.
So no, drop all this global mutex, move it to device state container and
DOCUMENT what it exactly protects (see checkpatch).
We tend to have each device is configured in a continuous manner.
No. That's wrong assumption and wrong idea. We want the async.
++ /*+ * If the firmware has no scene or only init scene,+ * we skip adding this mixer control.+ */+ if (fs210x->amp_lib.scene_count < 2)+ return 0;++ count = ARRAY_SIZE(fs210x_scene_control);++ return snd_soc_add_component_controls(cmpnt,+ fs210x_scene_control,+ count);+}++static int fs210x_get_bclk(struct fs210x_priv *fs210x,+ struct snd_soc_component *cmpnt)+{+ struct clk *bclk;+ int ret;++ bclk = devm_clk_get(fs210x->dev, "bclk");+ if (IS_ERR_OR_NULL(bclk)) {+ ret = bclk ? PTR_ERR(bclk) : -ENODATA;
Same pattern as regulators, eh...
Ok, we will update it.
quoted
quoted
+ if (ret == -EPROBE_DEFER)
No. Stop handling own probe deferrals. Look how other drivers do it.
Broonie recommanded to get clock in bus probe before,
and we will call it in i2c probe,
is it possible the clock isn't ready when we get it in bus probe?
we found some drivers do the probe deferral after getting clock.
Look how others drivers do it. You should not handle it differently -
you always return. The core handles deferred probe.
quoted
quoted
+ return ret;+ /*+ * If the SOC doesn't have the bclk clock source,+ * we skip setting the bclk clock.+ */+ return 0;
What is the point of this entire code? You got NULL, so assign NULL. Can
clk API handle NULLs? Answer this to yourself and write obvious, simple
code.
Before we calling clk API in fs210x_bclk_set, we check the clk_bclk is NULL or not firstly,
But it makes no sense. Clock core does it.
In clk_set_rate/clk_prepare_enable/clk_disable_prepare, they will check it:
if (!clk) or if (IS_ERR_OR_NULL(clk))
? What does it mean?
...
quoted
quoted
++static int fs210x_parse_dts(struct fs210x_priv *fs210x,+ struct fs210x_platform_data *pdata)+{+ struct device_node *node = fs210x->dev->of_node;+ int i, ret;++ if (!node)+ return 0;++ ret = of_property_read_string(node, "firmware-name", &pdata->fwm_name);+ if (ret)+ pdata->fwm_name = FS210X_DEFAULT_FWM_NAME;++ fs210x->gpio_sdz = devm_gpiod_get_optional(fs210x->dev,+ "reset", GPIOD_OUT_HIGH);+ if (IS_ERR_OR_NULL(fs210x->gpio_sdz)) {+ ret = fs210x->gpio_sdz ? PTR_ERR(fs210x->gpio_sdz) : -ENODATA;
Weird dance. Why assigning to ret enodata?
If we get the gpio_sdz and it's NULL, we assume it's unused.
If the error code is unbefitting, which one should we use?
No error code. You requested optional for a reason.
This is over-complicated way of getting simple optional gpio.
We want to cover the following possibilities:
1. The reset gpio is unused
And simple optional call is all you need.
2. The reset pin is shared by multiple deivces
You cannot. They cannot be shared, try by yourself. It is not a
supported setup.
You can switch to reset gpio driver, see my slides from last year OSSNA.
3. The reset pins are independent
I don't understand that.
4. The gpio pin is unready
There is no such thing.
The only thing you need to do is devm_gpiod_get_optional(), if IS_ERR()
return dev_err_probe.
ONLY.
For shared GPIOs, you cannot use it at all, see reset gpios driver
usecases in some Qcom WSA codecs.
quoted
quoted
++ for (i = 0; i < FS210X_NUM_SUPPLIES; i++)+ fs210x->supplies[i].supply = fs210x_supply_names[i];++ ret = devm_regulator_bulk_get(fs210x->dev,+ ARRAY_SIZE(fs210x->supplies),+ fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to get supplies: %d\n", ret);
Syntax is return dev_err_probe.
We can port the driver into older kernel easily without dev_err_probe,
But we don't want that. We work only on upstream.
the older kernel may not have this API.
No, we must not accept poor code because you have customer who wants to
work on obsolete and buggy and vulnerable kernel.
If it is recommended, we will update it.
It is really, really a strong requirement. Actually, it is beneficial
that it won't be possible to port to ancient kernels, because you won't
be tempted to use some 10 year old patterns in other places.
quoted
quoted
+ return ret;+ }++ return 0;+}++static int fs210x_parse_platdata(struct fs210x_priv *fs210x)
I do not understand why you have so many functions doing simple OF
parsing. fs210x_init, fs210x_parse_platdata, fs210x_parse_dts... and
this one here does nothing.
We parsed the acpi table in parse_platdata before v1,
but we didn't have the environment to check, then we removed the code.
If it's possible, we will add it in the future.
Also we tend to implment the functions shortly to reduce the complexity.
quoted
quoted
+{+ struct fs210x_platform_data *pdata;+ int ret;++ pdata = &fs210x->pdata;+ ret = fs210x_parse_dts(fs210x, pdata);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse dts: %d\n", ret);+ return ret;+ }++ return 0;+}++static void fs210x_deinit(struct fs210x_priv *fs210x)+{+ fs210x_sdz_pin_set(fs210x, true);+ regulator_bulk_disable(FS210X_NUM_SUPPLIES, fs210x->supplies);+}++static int fs210x_init(struct fs210x_priv *fs210x)+{+ int ret;++ ret = fs210x_parse_platdata(fs210x);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse platdata: %d\n", ret);
So you print SAME ERROR three times?
We will check and reduce the logs when the api has logs.
quoted
quoted
+ return ret;+ }++ ret = regulator_bulk_enable(FS210X_NUM_SUPPLIES, fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to enable supplies: %d\n", ret);+ return ret;+ }++ /* Make sure the SDZ pin is pulled down enough time. */+ usleep_range(10000, 10050); /* >= 10ms */+ fs210x_sdz_pin_set(fs210x, false);++ ret = fs210x_detect_device(fs210x);+ if (ret) {+ fs210x_deinit(fs210x);+ return ret;+ }++ fs210x->scene_id = -1; /* Invalid scene */+ fs210x->cur_scene = NULL;+ fs210x->is_playing = false;+ fs210x->is_inited = false;+ fs210x->is_suspended = false;+ fs210x->vol[0] = FS210X_VOLUME_MAX;+ fs210x->vol[1] = FS210X_VOLUME_MAX;++ return 0;+}++static int fs210x_register_snd_component(struct fs210x_priv *fs210x)+{+ struct snd_soc_dai_driver *dai_drv;+ int ret;++ dai_drv = devm_kmemdup(fs210x->dev, &fs210x_dai,+ sizeof(fs210x_dai), GFP_KERNEL);+ if (!dai_drv)+ return -ENOMEM;++ if (fs210x->devid == FS2105S_DEVICE_ID) {+ dai_drv->playback.rates = FS2105S_RATES;+ dai_drv->capture.rates = FS2105S_RATES;+ }++ ret = snd_soc_register_component(fs210x->dev,+ &fs210x_soc_component_dev,+ dai_drv, 1);+ return ret;+}++static int fs210x_i2c_probe(struct i2c_client *client)+{+ struct fs210x_priv *fs210x;+ int ret;++ dev_info(&client->dev, "version: %s\n", FS210X_DRV_VERSION);++ fs210x = devm_kzalloc(&client->dev, sizeof(*fs210x), GFP_KERNEL);+ if (!fs210x)+ return -ENOMEM;++ fs210x->i2c = client;+ fs210x->dev = &client->dev;+ i2c_set_clientdata(client, fs210x);++ fs210x->regmap = devm_regmap_init_i2c(client, &fs210x_regmap);+ if (IS_ERR_OR_NULL(fs210x->regmap)) {
Can devm_regmap_init_i2c() return NULL? No, it cannot.
OK, we will remove the judgment of NULL pointor
quoted
quoted
+ dev_err(fs210x->dev, "Failed to get regmap\n");
+ ret = fs210x->regmap ? PTR_ERR(fs210x->regmap) : -ENODATA;
Syntax is return dev_err_probe and drop NULL check.
Refer to the reply in regulator get.
quoted
quoted
+ return ret;+ }++ mutex_lock(&fs210x_mutex);+ ret = fs210x_init(fs210x);+ mutex_unlock(&fs210x_mutex);
Why do you need to lock it? Who and how can access device at this point?
If the system has more than 1 devices:
the module may be loaded asynchronously, if the gpio/supplies are shared,
What? No. It's just cannot happen. Core handles it.
it's better to protect the detection with lock?
You protected here nothing.
1. Concurrent SHARED GPIO reset: you replaced concurrent into
step-by-step-breaking-your-device-because-other-just-probed-and-reset-you
2. supplies: core handles it.
Do you see such needs anywhere in other recent codecs who share pins? I
understand it might be tricky to find it... but trust me, there is no
except legacy poor choices...
Best regards,
Krzysztof
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-10 08:58:00
On Wed, Jul 09, 2025 at 12:29:30PM +0100, Mark Brown wrote:
On Tue, Jul 08, 2025 at 07:29:01PM +0800, Nick Li wrote:
quoted
The FS2104/5S are FourSemi digital audio amplifiers
with I2C control. They are Inductor-Less, Stereo, Closed-Loop,
Digital Input Class-D Power Amplifiers with Enhanced Signal Processing.
This looks broadly OK - there's a few more fairly small issues below in
addition to those that Krzysztof flagged.
quoted
+static int fs210x_set_pcm_volume(struct fs210x_priv *fs210x)+{+ u16 vol[2];+ int ret;++ if (fs210x->devid == FS2105S_DEVICE_ID) {
A swtich statement is better style here, it makes it easier to add more
variants.
Good idea.
quoted
+ switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) {
_CLOCK_PROVIDER_MASK
quoted
+ case SND_SOC_DAIFMT_CBC_CFC:
+ /* Only supports slave mode */
+static int fs210x_dai_hw_params(struct snd_pcm_substream *substream,+ struct snd_pcm_hw_params *params,+ struct snd_soc_dai *dai)+{+ struct fs210x_priv *fs210x;+ int chn_num;+ int ret;++ if (substream->stream != SNDRV_PCM_STREAM_PLAYBACK)+ return 0;
There's no configuration for capture? Should the driver be setting
symmetric_rates and then not reconfiguring if active instead, it looks a
lot like the driver is relying on userspace starting playback before
capture to configure?
In capture path:
The device can output EC reference signal for AEC,
it can't output the signal without playing audio(start device in playback).
We had set symmetric_rates = 1 in fs210x_dai
quoted
+static int fs210x_pcm_volume_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{
The driver should return 1 if there's a change in the values and 0
otherwise (plus it can skip a bunch of work if there's nothing to do).
The mixer-test test will spot this for you.
BTW, what's the mixer-test? We tested it by using alsamixer and amixer.
quoted
+static int fs210x_effect_scene_put(struct snd_kcontrol *kcontrol,+ struct snd_ctl_elem_value *ucontrol)+{
quoted
+ ret = fs210x_set_scene(fs210x, scene_id);+ if (ret)+ dev_err(fs210x->dev, "Failed to set scene: %d\n", ret);++ mutex_unlock(&fs210x_mutex);++ return ret;
Same issue with flagging changes.
Got it, we will update what you mentioned in here and previous mail,
the next version may need to test for a few days, I'm sorry for my late response.
Thanks.
Best regards,
Nick
Keep the same order as in list of properties. OTOH, missing supplies.
OK, we will fix the order, but the supplies may not be used as regulator,
Hm? What does it mean in terms of hardware?
quoted
we mark them as required, is it OK?
How codec driver can work without power?
The power may be connected to the baterry/adapter directly,
it may not be under the control of the software,
in this case, the supplies are use as dummy regulators?
Best regards,
Nick
From: Nick Li <nick.li@foursemi.com> Date: 2025-07-10 11:21:16
On Thu, Jul 10, 2025 at 10:27:15AM +0200, Krzysztof Kozlowski wrote:
On 10/07/2025 09:56, Nick Li wrote:
quoted
quoted
quoted
+};++static DEFINE_MUTEX(fs210x_mutex);
Why is this file-scope? Why two independent codecs cannot work in parallel?
The driver module may be loaded asynchronously,
if the reset pin/supplies are shared by the devices,
we should protect the process of detecting devices.
No, that's not the job of this driver. Your driver must not protect from
imaginary resource conflicts, because it will not even solve it
properly. It is impossible. What if foo,AS9911 codec also shares these
pins supplies?
And supplies needs synchronization? About pins you got point, but here
clearly you are wrong.
So no, drop all this global mutex, move it to device state container and
DOCUMENT what it exactly protects (see checkpatch).
OK, we will use a private lock for mutual exclusion with work queues/mixers/...
quoted
We tend to have each device is configured in a continuous manner.
No. That's wrong assumption and wrong idea. We want the async.
++ /*+ * If the firmware has no scene or only init scene,+ * we skip adding this mixer control.+ */+ if (fs210x->amp_lib.scene_count < 2)+ return 0;++ count = ARRAY_SIZE(fs210x_scene_control);++ return snd_soc_add_component_controls(cmpnt,+ fs210x_scene_control,+ count);+}++static int fs210x_get_bclk(struct fs210x_priv *fs210x,+ struct snd_soc_component *cmpnt)+{+ struct clk *bclk;+ int ret;++ bclk = devm_clk_get(fs210x->dev, "bclk");+ if (IS_ERR_OR_NULL(bclk)) {+ ret = bclk ? PTR_ERR(bclk) : -ENODATA;
Same pattern as regulators, eh...
Ok, we will update it.
quoted
quoted
+ if (ret == -EPROBE_DEFER)
No. Stop handling own probe deferrals. Look how other drivers do it.
Broonie recommanded to get clock in bus probe before,
and we will call it in i2c probe,
is it possible the clock isn't ready when we get it in bus probe?
we found some drivers do the probe deferral after getting clock.
Look how others drivers do it. You should not handle it differently -
you always return. The core handles deferred probe.
OK, the core will handle it, we don't need to do.
quoted
quoted
quoted
+ return ret;+ /*+ * If the SOC doesn't have the bclk clock source,+ * we skip setting the bclk clock.+ */+ return 0;
What is the point of this entire code? You got NULL, so assign NULL. Can
clk API handle NULLs? Answer this to yourself and write obvious, simple
code.
Before we calling clk API in fs210x_bclk_set, we check the clk_bclk is NULL or not firstly,
But it makes no sense. Clock core does it.
quoted
In clk_set_rate/clk_prepare_enable/clk_disable_prepare, they will check it:
if (!clk) or if (IS_ERR_OR_NULL(clk))
? What does it mean?
Uh, I got it.
The clock core does the checking.
...
quoted
quoted
quoted
++static int fs210x_parse_dts(struct fs210x_priv *fs210x,+ struct fs210x_platform_data *pdata)+{+ struct device_node *node = fs210x->dev->of_node;+ int i, ret;++ if (!node)+ return 0;++ ret = of_property_read_string(node, "firmware-name", &pdata->fwm_name);+ if (ret)+ pdata->fwm_name = FS210X_DEFAULT_FWM_NAME;++ fs210x->gpio_sdz = devm_gpiod_get_optional(fs210x->dev,+ "reset", GPIOD_OUT_HIGH);+ if (IS_ERR_OR_NULL(fs210x->gpio_sdz)) {+ ret = fs210x->gpio_sdz ? PTR_ERR(fs210x->gpio_sdz) : -ENODATA;
Weird dance. Why assigning to ret enodata?
If we get the gpio_sdz and it's NULL, we assume it's unused.
If the error code is unbefitting, which one should we use?
No error code. You requested optional for a reason.
This is over-complicated way of getting simple optional gpio.
We want to cover the following possibilities:
1. The reset gpio is unused
And simple optional call is all you need.
OK.
quoted
2. The reset pin is shared by multiple deivces
You cannot. They cannot be shared, try by yourself. It is not a
supported setup.
It will report -EBUSY when we request the same gpio.
You can switch to reset gpio driver, see my slides from last year OSSNA.
OK, I have the honour to read it.
quoted
3. The reset pins are independent
I don't understand that.
Each device has its own reset pin, it's a general case.
quoted
4. The gpio pin is unready
There is no such thing.
OK.
The only thing you need to do is devm_gpiod_get_optional(), if IS_ERR()
return dev_err_probe.
ONLY.
OK.
For shared GPIOs, you cannot use it at all, see reset gpios driver
usecases in some Qcom WSA codecs.
OK, I have found the driver and will learn about it later.
quoted
quoted
quoted
++ for (i = 0; i < FS210X_NUM_SUPPLIES; i++)+ fs210x->supplies[i].supply = fs210x_supply_names[i];++ ret = devm_regulator_bulk_get(fs210x->dev,+ ARRAY_SIZE(fs210x->supplies),+ fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to get supplies: %d\n", ret);
Syntax is return dev_err_probe.
We can port the driver into older kernel easily without dev_err_probe,
But we don't want that. We work only on upstream.
OK.
quoted
the older kernel may not have this API.
No, we must not accept poor code because you have customer who wants to
work on obsolete and buggy and vulnerable kernel.
OK.
quoted
If it is recommended, we will update it.
It is really, really a strong requirement. Actually, it is beneficial
that it won't be possible to port to ancient kernels, because you won't
be tempted to use some 10 year old patterns in other places.
OK.
quoted
quoted
quoted
+ return ret;+ }++ return 0;+}++static int fs210x_parse_platdata(struct fs210x_priv *fs210x)
I do not understand why you have so many functions doing simple OF
parsing. fs210x_init, fs210x_parse_platdata, fs210x_parse_dts... and
this one here does nothing.
We parsed the acpi table in parse_platdata before v1,
but we didn't have the environment to check, then we removed the code.
If it's possible, we will add it in the future.
Also we tend to implment the functions shortly to reduce the complexity.
quoted
quoted
+{+ struct fs210x_platform_data *pdata;+ int ret;++ pdata = &fs210x->pdata;+ ret = fs210x_parse_dts(fs210x, pdata);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse dts: %d\n", ret);+ return ret;+ }++ return 0;+}++static void fs210x_deinit(struct fs210x_priv *fs210x)+{+ fs210x_sdz_pin_set(fs210x, true);+ regulator_bulk_disable(FS210X_NUM_SUPPLIES, fs210x->supplies);+}++static int fs210x_init(struct fs210x_priv *fs210x)+{+ int ret;++ ret = fs210x_parse_platdata(fs210x);+ if (ret) {+ dev_err(fs210x->dev, "Failed to parse platdata: %d\n", ret);
So you print SAME ERROR three times?
We will check and reduce the logs when the api has logs.
quoted
quoted
+ return ret;+ }++ ret = regulator_bulk_enable(FS210X_NUM_SUPPLIES, fs210x->supplies);+ if (ret) {+ dev_err(fs210x->dev, "Failed to enable supplies: %d\n", ret);+ return ret;+ }++ /* Make sure the SDZ pin is pulled down enough time. */+ usleep_range(10000, 10050); /* >= 10ms */+ fs210x_sdz_pin_set(fs210x, false);++ ret = fs210x_detect_device(fs210x);+ if (ret) {+ fs210x_deinit(fs210x);+ return ret;+ }++ fs210x->scene_id = -1; /* Invalid scene */+ fs210x->cur_scene = NULL;+ fs210x->is_playing = false;+ fs210x->is_inited = false;+ fs210x->is_suspended = false;+ fs210x->vol[0] = FS210X_VOLUME_MAX;+ fs210x->vol[1] = FS210X_VOLUME_MAX;++ return 0;+}++static int fs210x_register_snd_component(struct fs210x_priv *fs210x)+{+ struct snd_soc_dai_driver *dai_drv;+ int ret;++ dai_drv = devm_kmemdup(fs210x->dev, &fs210x_dai,+ sizeof(fs210x_dai), GFP_KERNEL);+ if (!dai_drv)+ return -ENOMEM;++ if (fs210x->devid == FS2105S_DEVICE_ID) {+ dai_drv->playback.rates = FS2105S_RATES;+ dai_drv->capture.rates = FS2105S_RATES;+ }++ ret = snd_soc_register_component(fs210x->dev,+ &fs210x_soc_component_dev,+ dai_drv, 1);+ return ret;+}++static int fs210x_i2c_probe(struct i2c_client *client)+{+ struct fs210x_priv *fs210x;+ int ret;++ dev_info(&client->dev, "version: %s\n", FS210X_DRV_VERSION);++ fs210x = devm_kzalloc(&client->dev, sizeof(*fs210x), GFP_KERNEL);+ if (!fs210x)+ return -ENOMEM;++ fs210x->i2c = client;+ fs210x->dev = &client->dev;+ i2c_set_clientdata(client, fs210x);++ fs210x->regmap = devm_regmap_init_i2c(client, &fs210x_regmap);+ if (IS_ERR_OR_NULL(fs210x->regmap)) {
Can devm_regmap_init_i2c() return NULL? No, it cannot.
OK, we will remove the judgment of NULL pointor
quoted
quoted
+ dev_err(fs210x->dev, "Failed to get regmap\n");
+ ret = fs210x->regmap ? PTR_ERR(fs210x->regmap) : -ENODATA;
Syntax is return dev_err_probe and drop NULL check.
Refer to the reply in regulator get.
quoted
quoted
+ return ret;+ }++ mutex_lock(&fs210x_mutex);+ ret = fs210x_init(fs210x);+ mutex_unlock(&fs210x_mutex);
Why do you need to lock it? Who and how can access device at this point?
If the system has more than 1 devices:
the module may be loaded asynchronously, if the gpio/supplies are shared,
What? No. It's just cannot happen. Core handles it.
OK.
quoted
it's better to protect the detection with lock?
You protected here nothing.
1. Concurrent SHARED GPIO reset: you replaced concurrent into
step-by-step-breaking-your-device-because-other-just-probed-and-reset-you
2. supplies: core handles it.
Do you see such needs anywhere in other recent codecs who share pins? I
understand it might be tricky to find it... but trust me, there is no
except legacy poor choices...
Hardware engineer wants all the reset(or irq) pins are shared,
they explain the soc has not enough gpio pins to be used,
especially when we use 4~8 audio amplifiers in a system,
if we use the separate reset & interrupt pins, they're too much pins.
We try to drop the related logic of shared pins.
Thank you very much.
Best regards,
Nick