From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S935035AbdCaBSv (ORCPT ); Thu, 30 Mar 2017 21:18:51 -0400 Received: from mail-cys01nam02on0103.outbound.protection.outlook.com ([104.47.37.103]:56598 "EHLO NAM02-CY1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S934695AbdCaBSt (ORCPT ); Thu, 30 Mar 2017 21:18:49 -0400 Authentication-Results: vger.kernel.org; dkim=none (message not signed) header.d=none;vger.kernel.org; dmarc=none action=none header.from=maximintegrated.com; MIME-Version: 1.0 In-Reply-To: <20170329142304.qovnyqd6izackny2@sirena.org.uk> References: <1490748828-31701-1-git-send-email-ryans.lee@maximintegrated.com> <20170329142304.qovnyqd6izackny2@sirena.org.uk> From: Ryan Lee Date: Thu, 30 Mar 2017 18:18:40 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier To: Mark Brown CC: Liam Girdwood , , , , , Kuninori Morimoto , Arnd Bergmann , , , , , , Axel Lin , , Srinivas Kandagatla , , , , , , Dylan Reid Content-Type: text/plain; charset="UTF-8" X-Originating-IP: [74.125.83.52] X-ClientProxiedBy: CO2PR07CA0068.namprd07.prod.outlook.com (10.174.192.36) To CY1PR11MB0842.namprd11.prod.outlook.com (10.163.237.20) X-MS-Office365-Filtering-Correlation-Id: 10de88b5-5054-4ab7-926e-08d477d3e082 X-MS-Office365-Filtering-HT: Tenant X-Microsoft-Antispam: UriScan:;BCL:0;PCL:0;RULEID:(22001)(48565401081)(201703131423075)(201703031133081);SRVR:CY1PR11MB0842; X-Microsoft-Exchange-Diagnostics: 1;CY1PR11MB0842;3:QGPj93lc8BUpoJW3q7B5I0WFfG+qzpwmhyuKPB6ZkywaLck/GX7T3c9uisiNA95qPffhCUUiy5EAr+hUZL3EVkf71pqe5xPuTNADvX4BZd2ioRe1AaeEViMkk80OUIBNpspoxLBgurJuWksObQoT0jChnXe7bnuHX/LZGRy6LiZ8HYpDXdtUUeUcvOEvqvbJCA8/Y1c6lZM/8lguBRk4q/u4fa9IcJBx1unrXf8u2WI9XB2vgGWU9EuT6sEIlG5/xRG3ixYCoKQbVzNr0lZPEFwn8HvEJKQP1W5FY3tZWiRMhYYCd62cUJZa2bLii4DnOCOwvGKYsNTDMAHlNcYBDc50/AqadFHrevEs7cIx8e8=;25:VUIcv5ddl6me3w+OxzlkDvzKr0CTSgeIQhp8yX1FIqW/Iykszmf1wtIVOhMNI6/DJoQMS3uOqmiFAwjFFmEIXdLTNjwPwTacV7qAy2WAUeMSYorreFIFavGwrZtS5s9bPjzWNdRLYyz7JOwv3ALcD4OhGiQtmY2UBR3wvV5xtOw2ise6DzJ1wtiQ46PZCl+4cGpgylAq/dR5FmWvCXlyCdPEwcGcx9oFnQG86EZvh5SSUTzFtN4ObacklgQQ3uppgWrZo+ZQokxPagHnM0ixfpC2XKMwg+7UMWHleB4vdC0FmP/BW1Kz1pkzbVP69li4DJXPVaKuiAUuK3M1MLtpXnfjFZrd6TaqmFEN0AySifFZJnStYM+twHE8USWWCt5pdNilw3oi6QP7DPbSqvxBgFlU07AmBoxU7HQAwW92+7f8Fsin34Vv8sNgPv4p7Xu2lD8OHro+xgDPwxamM6QCAQ== X-Microsoft-Exchange-Diagnostics: 1;CY1PR11MB0842;31:zbqKPSqkjc9BBD4NFMZCo8vf14wfvn37ZSK4toGsxPZtKHxyAqI9B13FSJry3ItHFO+hWUcKMUhZy63kEROTnC819L7Mz4kI9S3RnlnlXRdfz77hXJDXwXyabq8rVsd9wn72fAIY0mP7WvBkaC7X7QW3zkCc39bVcVYR+tNBoymScoqAoYlMTG5N6/w3uSReNrxPMfBApfdlEx13BlKilfkInYvNw3eKoV6Y9GBXx3qRnuqbECMjReZzAwd11IExvk39EbPUwSv6cIkWuBnm6A==;20:PxiCb3OMAgZkLjlWU4rweBFOOIhJ5feklADId235yL4InFUoZBaUtdCaPF03eES5SLaFcdPnxFZGUQEN8ty8ToqGb6W8r/CLmwunCWvHhiReYhuyr2w1/BkMYkI1SmShFRCwvUedyFcUsnkc2ZzkmxAQEhHfhvQv5P5sb/Xlf/JG9ehBykb8YeeqztG+DpHxcEU0oy49EfmHO2LOORSXCT5KIgM5hmpb3+F6EWotixD1S0FgAV5Tfoec1Zkd6X2PBnGWiji6fApjOCGNQW8dxJ34YTtdfFiHVF/OWTbkenS8jxPGkpwhLBcoTJZGQGBwDx0uS2u/pItj5q9nP5sIw4k0scA7QqU8oapGRMIEf0eYK+TnRFxJLF21+L0MqsaudnQCeUB1hJwNAE7DgH+hXPoHzf7ZLXanhiQCtsu8UaNvKfPV4eN2lYyffbC4eWniNBiFk9kmWdoJ6jKoA26giDaHcxlKweoOjFlPrkCrDjdi7voUqp/wgu1/aiLvHy9G X-Microsoft-Antispam-PRVS: X-Exchange-Antispam-Report-Test: UriScan:; X-Exchange-Antispam-Report-CFA-Test: BCL:0;PCL:0;RULEID:(6040450)(601004)(2401047)(8121501046)(5005006)(10201501046)(3002001)(93006081)(93001081)(6055026)(6041248)(20161123560025)(20161123555025)(20161123562025)(20161123564025)(201703131423075)(201703011903075)(201702281528075)(201703061421075)(6072148);SRVR:CY1PR11MB0842;BCL:0;PCL:0;RULEID:;SRVR:CY1PR11MB0842; X-Microsoft-Exchange-Diagnostics: 1;CY1PR11MB0842;4:br0kXmQWy4ihfFF5EMfHU89hSjZdyIwQDsa5zs01jmlvSc84IBr8d/GkvBfqQepiIRUQJx9Sy+uRvL1yJkqptoe5BBHNH7XaT3+pwadrKozI/AEX8IRhjP2iQHKasn1ot2aO44oNOfEnuorwtT8dkxEAPRwStcQiNhc/p0WA0Xs6Bzby3PKiM98t4OHKe/A9Y3WGt6O0wgbWkYNGN5KYvhSCIWAAT3LcFSXWvaExzRqczH+Gp6qirl5hTgA+7NwJ/GqswScCzB+rPfAwPm22pxYz36XKRWu2FN51kPK5YE3VLX4JXHQ7vGTCjvwDk2Gp8NueteuhNnv+Iqn17LMsheRkcBp56GX+eDHRG2fSzQkSSBtXQMskmChJIUDKfgSoCO3Jj9PGwi83Kk1anV06TK6fOHkH2taciIA3voNGa6GHLQhTtQcahk/qxaNxFM6ev/PR8S2CI0BbgEIsSm3pKmGuS3/ZqtZH7pea0ckCXUkpQoEP79mBaKKwo3RC/TcF53n9jskFoPGVXPd2s9lzX04GYXm5PeyBl8IiFrAh6kQhiy944kaiT3nCJAdyMCCCTcCKCEkd6igURjxedVdW3ZAuvdGnobd8DZcT43fbQyJFRh+IJ06+7TaXNsTMmPVz+BUi9A7xHIsOv/tSksVYLLwJJ8LfZFR1zBY9gTjlfiIe/LBRPSIUoX9cRU/MXiItCCJuxWfRjJdOEuvcU5f6TQXyYrqaEZSJGP3RfF87UnLKsyLnsrSI4ika2YHwKxVzxFAQUkwCnksZNU80yaUHuA== X-Forefront-PRVS: 02638D901B X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10019020)(4630300001)(6009001)(39860400002)(39840400002)(39450400003)(39400400002)(39410400002)(39850400002)(24454002)(377454003)(2950100002)(5660300001)(61726006)(229853002)(6246003)(6116002)(9896002)(3846002)(8676002)(81166006)(6862004)(450100002)(107886003)(38730400002)(4326008)(42186005)(110136004)(2906002)(9686003)(5820100001)(50986999)(54906002)(76176999)(63696999)(23676002)(53546009)(55446002)(54356999)(498394004)(53936002)(305945005)(47776003)(66066001)(50466002)(7736002)(61266001)(93516999)(189998001)(98316002)(86362001);DIR:OUT;SFP:1102;SCL:1;SRVR:CY1PR11MB0842;H:mail-pg0-f52.google.com;FPR:;SPF:None;MLV:sfv;LANG:en; X-Microsoft-Exchange-Diagnostics: =?utf-8?B?MTtDWTFQUjExTUIwODQyOzIzOjVtU1hIQWRYK3JaV0N5M1MxS1hNUmduZVZq?= =?utf-8?B?b0VEY1FJY2x4RWJKQ2VjWk9sSnZ6R2JwcWlhcitIelNxODVBdGxXNGp2a2xV?= =?utf-8?B?Y3YrazB0dXhhK0hDekM3alJDb1JBREdHVWpiZmZ2LytDOFlyWDJqNXFGbUt4?= =?utf-8?B?bEFiM0FBemNQNi9TQVd3dWtHN0Q4ZFp6WmF0clRNVDNDbGFmcTRKMU5WbVBj?= =?utf-8?B?WXRpSGljK1RxelJzYWZEelk0T0NRYkR6OHZMSlcxaDFCNVliYTZQTWlCQk9s?= =?utf-8?B?aElVOGg3ay9XTEhCV3JLcitoWW9WdmJQTzJid3JUOGF1d1RhWmg1RFJYdWZ4?= =?utf-8?B?aStHbzFDNXZjdGpEbjZDWjM3cTYxeklJeUE0ditEOE5wSWd2Qnk4VnRSbHN5?= =?utf-8?B?VE9obVBKWmFMaTBsSUUrUDkreGNZS0RVNlppWUwvY202aUxnNkZGTHBJNDFV?= =?utf-8?B?czljdHNTaFVjTWVqK2pVRy8vcG5CRmc5Rjh1UkxIdWt3SERYTUhBdXZEZVkr?= =?utf-8?B?Z3BERnRhMmxwMlBiL1c0MDFGK2dmRWNzNm9pT2VBWUlKbFZheW13RUNnSkU5?= =?utf-8?B?WTNySVVRVVBFekVJVUp3Tkc3NHc0TTVPTjlBWXJ5b1JDT0RIazlocUNhbjdw?= =?utf-8?B?MHRjRlorcWxMM0t2cFBYeW9SbC8wWHlkcFZpc1JwZEdjWmZ0Y0NWMmJNTHVE?= =?utf-8?B?a3BVeStLVG5FNVNpcytBbzBEbkg1QlJJYllVU2lqakR6Y2lZdjF0Qkc2MUZJ?= =?utf-8?B?SDZFZmM5bjY2M1pQWS9qUnh2WC94VGVwUCtKR0s2bXFYcHlEd2o4RjViMjMv?= =?utf-8?B?T3U3dUlLaHd0UmhMWVlBZUxIM2lCMWkzV0JWUkVnbWhHTGV6SFZyZzhsMHlo?= =?utf-8?B?MldQYXduSkRZUXV5ZEwwc21GUUs3WDkzam9CK21vMFZ1cWEwbjJtdlFWZjZa?= =?utf-8?B?SVlpcW9ZcWNVYlBJMWJ2TWFHM2JvZ25UL2crV2ZKVTJ1SEpsT21ST2tCRWJV?= =?utf-8?B?SEdES0J5cmZVT3ZNdW9KcUNnbnVPdE5HSGlON25JWXdWRWNDcUZjVFg1OGdx?= =?utf-8?B?TWt1eTVXU0M5SUlrMTdyQ281RCtnTlFNOGNSN2VUREJobzhzNzMzVWZsbUZm?= =?utf-8?B?d0VLOStMNzR3eTlwZDVON0FTN1kwQy9nZ05YbWdmTTNIa1B2ZVFHUkhsV1M0?= =?utf-8?B?ODZsL0JEVTUzZWNtM0FCOWJmRzcrUGdNaGN4clVYK2xETXRGZ1d6bzRSUUYz?= =?utf-8?B?SXlUb3VwM1JRYllTalJxSEFnVzJyNE9TTEQ2TjU3WXhBSklWR0FFYlI1cEQ2?= =?utf-8?B?VGdIaUlBTG1WRFZHWkMxSXFrTVFMQ0FMWUo4NUgzVjBmMnBEbUFNSGVtZXMz?= =?utf-8?B?UTdpOTdhVm0rVTJPUmg5Y1I2OUJ2dUx6cnJnZEpoZlVNbE9yQ1pNU1BsYUR3?= =?utf-8?B?QVV0M0VySENYOG0va3RSVEw5bVJQY0Uzd0V0bG0yVVdlUEdzL1gxdm02VHpR?= =?utf-8?B?QXhYczY3TTMra2t3RUF5Tk9GaUxKUlQ4NWVYVlV3eTBJQTVWZFdVNXJYS3pt?= =?utf-8?B?anhud0hvbUZKNU5FaE85cUNPYlJsSnNjekhhaW5GY0g4R09ldlJiSys1YUZj?= =?utf-8?B?b09nQklmT3lMVXJjQVFVK2tLRGtKbjNpSmRycSs5VWNSS0lRbGM3WmhRcWtV?= =?utf-8?Q?scrOhweT/hozdMub5I=3D?= X-Microsoft-Exchange-Diagnostics: 1;CY1PR11MB0842;6:a86CTDL0rBRBaajVlgBASeBFbdCnDnnga04iNQrIz2EV9N2fL6hli9KWlXAqsMOt7pk4adL9g7wh7iUeLfRKSqpUJ5rjRDOwnSg4nl6/r1+yv9QqGRhOvKBB/PvjfeVBRFYFmVUGlR9GFB0VeaQMvKcJRBS8tpKWh/SlY54ZpQATs3MERlyNyi4f8w0/zvf8K058lRdT0W8yE70MbhijfXn53s4LQvX0dAVIJVfUlcXSNvY9MxYVpCNGePSziRx6/3dfjVwjRQhr71P48bEQZc5G/lm7D3GHCyetR5YXb5MdH571mxjf9sl3Iapt4K+811gNzXcYsESmDW7ZNB8zPq8zAUpTPCNvbQzElktRHfNT1m92uo28hBWDhYepPlQO60HBPkiP/cnzZd8gcVR4M95FUUk+ypKMELqkBZFDDAs=;5:0INqIZEOVlDkdBoVnbhaaTcSmsiLBt0xkc2fKaRAUyB7qMrap+SHt1Wu9wqvoTSv62QQb2kKfScTxtiWcBS9pKiM4bSn4Bsk+pXoGz/HN28qLcs7rjxSQPguw3wE+x9mNBBtUuLaXijCLS7UfsD+ww==;24:wer3IJHWeNg/+h8OOfMsb4/N4hCaiQNjSdxtcebDfxJCGGwSIIS1uS7tx1SovJ1si4kr6iFkJwKnkO5UQHst9S+eS5ucSkQrilTdxYPIhs4= SpamDiagnosticOutput: 1:99 SpamDiagnosticMetadata: NSPM X-Microsoft-Exchange-Diagnostics: 1;CY1PR11MB0842;7:vRajkm3jDDxnW1zTwlJ5QTzzXl/2YZzN7IAAVSYYOJC45S58AS9XEyMGufl3k60/4UpOaltB1lomvv/9rkDabg2sF0kBM2Wbfo3yYT8euavvSm+XjpN2+ObniE81SfwZmTxWhxuiIrN9Dwx52QsUVTdPauVMHKHo1/3SC/UgYwuxZ1/YDvW6pwGODZ5r5fVkn8zzzPy2I1xl2PsxRRRjsrq8x+piccgcB+BZmxNRl1olpjFVzK5+xyMxwhriCJhIS/V2HPRZHCopG1+P9bf/YFKvhjOR6y3vz6w/09PKjzDDJ1j48IYSmYFulDXbgdmD3qxGuwtYX4UL3ErRCelLtg== X-OriginatorOrg: maximintegrated.com X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Mar 2017 01:18:45.4520 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR11MB0842 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Mar 29, 2017 at 7:23 AM, Mark Brown wrote: > On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote: > >> + case SND_SOC_DAIFMT_CBS_CFM: >> + mode = MAX98927_PCM_MASTER_MODE_HYBRID; >> + default: >> + dev_err(codec->dev, "DAI clock mode unsupported"); > > Either we don't support _CBS_CFM or there's a missing break there. This was removed since CVS_CFM is not supported. > >> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { >> + case SND_SOC_DAIFMT_I2S: >> + max98927->iface |= SND_SOC_DAIFMT_I2S; >> + >> + break; > > Please use the kernel coding style as you do in the rest of this > function. Modified as requested. > >> + int reg = MAX98927_R0022_PCM_CLK_SETUP; >> + int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK; > >> + regmap_update_bits(max98927->regmap, reg, mask, value); > > reg and mask have exactly one user (as you'd expect), just use the > constants directly here to make things clearer. Removed reg and mask. > >> + switch (snd_pcm_format_width(params_format(params))) { >> + case 16: >> + chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16; >> + max98927->ch_size = 16; > > You could just assign ch_size directly. Sorry I don't fully understand this. Anyway 'ch_size' will be directly updated from snd_pcm_format_width information after modification. > >> + /* sampling rate configuration */ >> + switch (params_rate(params)) { >> + case 8000: >> + sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000; > > sampling_rate is only ever set to a real value in this switch statement, > you're not oring with anything else so you can just use an assignment > which would be a lot clearer (it's not obvious what else might go in > there) and the initial assignment to 0 for this and chan_sz can only > mask errors. Removed 'oring'. > >> + /* set sampling rate of IV */ >> + if (max98927->interleave_mode && >> + sampling_rate > MAX98927_PCM_SR_SET1_SR_16000) >> + regmap_update_bits(max98927->regmap, > > Please use the kernel coding style, indent the second line of the if > with the ( so that it doesn't look like part of the conditional code. Modified as requested. > >> + switch (reg) { >> + case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B: > > >> + return true; >> + } >> + return false; > > It'd be clearer to put the return false in the switch statement like the > return true, and also consistent with the _volatile_reg() function. Modified as requested. > >> +static const char * const max98927_speaker_source_text[] = { >> + "i2s", "reserved", "tone", "pdm" >> +}; > > This looks like it should be a DAPM control. Removed this and added it to 'dai_set_format' function. > >> +static const char * const max98927_monomix_output_text[] = { >> + "ch_0", "ch_1", "ch_1_2_div" >> +}; > > Similarly here. Removed this since it was already added to DAPM control. > >> + SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL, >> + 0, (1<> + max98927_digital_tlv), > > All volume controls should end with Volume as per control-names.rst. Modified as requested. > >> + SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN, >> + MAX98927_BROWNOUT_DSP_SHIFT, 1, 0), > > All on/off switches should end with Switch as per control-names.rst. Modified as requested. > >> +static int max98927_probe(struct snd_soc_codec *codec) >> +{ > >> + /* Check Revision ID */ >> + ret = regmap_read(max98927->regmap, >> + MAX98927_R01FF_REV_ID, ®); > > Basic device identification and setup should be done at the chip level > probe not at the CODEC level so that if there are problems we fail as > early as possible and so that diagnostic information is available to > users as soon as possible, even if there's no sound card for the device > in the system. Moved to i2c_probe function. > >> + /* Set inital volume (+13dB) */ > > As with all other CODEC drivers you should leave the hardware defaults > alone, what makes sense for your system may not make sense for other > systems and the hardware defaults are a fixed thing. We recommend to use static +13dB gain when our speaker protection algorithm is running on DSP. It is not hardware default but mostly +13dB is being used as default. > >> + /* Boost Output Voltage & Current limit */ >> + regmap_write(max98927->regmap, >> + MAX98927_R0040_BOOST_CTRL0, >> + 0x1C); >> + regmap_write(max98927->regmap, >> + MAX98927_R0042_BOOST_CTRL1, >> + 0x3E); > > This should be system specific, these values might be unsafe in some > systems. Same as above, most user will use maximum voltage & current limit value to take advantage of 10V boost amplifier. I added one more control to change the current limit value. > >> +err: >> + if (max98927) >> + devm_kfree(&i2c->dev, max98927); > > There is no need to explicitly free devm_ allocated memory, that's the > point of devm. Removed the code. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ryan Lee Subject: Re: [PATCH v3] ASoC: Add support for Maxim Integrated MAX98927 Amplifier Date: Thu, 30 Mar 2017 18:18:40 -0700 Message-ID: References: <1490748828-31701-1-git-send-email-ryans.lee@maximintegrated.com> <20170329142304.qovnyqd6izackny2@sirena.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20170329142304.qovnyqd6izackny2-GFdadSzt00ze9xe1eoZjHA@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Mark Brown Cc: Liam Girdwood , robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org, mark.rutland-5wv7dgnIgG8@public.gmane.org, perex-/Fr2/VpizcU@public.gmane.org, tiwai-IBi9RG/b67k@public.gmane.org, Kuninori Morimoto , Arnd Bergmann , ckeepax-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org, lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org, bardliao-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org, nh6z-fFIq/eER6g8@public.gmane.org, KCHSU0-KrzQf0k3Iz9BDgjK7y7TUQ@public.gmane.org, Axel Lin , romain.perier-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org, Srinivas Kandagatla , oder_chiou-Rasf1IRRPZFBDgjK7y7TUQ@public.gmane.org, Paul.Handrigan-jGc1dHjMKG3QT0dZR+AlfA@public.gmane.org, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Dylan Reid List-Id: devicetree@vger.kernel.org On Wed, Mar 29, 2017 at 7:23 AM, Mark Brown wrote: > On Wed, Mar 29, 2017 at 09:53:48AM +0900, Ryan Lee wrote: > >> + case SND_SOC_DAIFMT_CBS_CFM: >> + mode = MAX98927_PCM_MASTER_MODE_HYBRID; >> + default: >> + dev_err(codec->dev, "DAI clock mode unsupported"); > > Either we don't support _CBS_CFM or there's a missing break there. This was removed since CVS_CFM is not supported. > >> + switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { >> + case SND_SOC_DAIFMT_I2S: >> + max98927->iface |= SND_SOC_DAIFMT_I2S; >> + >> + break; > > Please use the kernel coding style as you do in the rest of this > function. Modified as requested. > >> + int reg = MAX98927_R0022_PCM_CLK_SETUP; >> + int mask = MAX98927_PCM_CLK_SETUP_BSEL_MASK; > >> + regmap_update_bits(max98927->regmap, reg, mask, value); > > reg and mask have exactly one user (as you'd expect), just use the > constants directly here to make things clearer. Removed reg and mask. > >> + switch (snd_pcm_format_width(params_format(params))) { >> + case 16: >> + chan_sz = MAX98927_PCM_MODE_CFG_CHANSZ_16; >> + max98927->ch_size = 16; > > You could just assign ch_size directly. Sorry I don't fully understand this. Anyway 'ch_size' will be directly updated from snd_pcm_format_width information after modification. > >> + /* sampling rate configuration */ >> + switch (params_rate(params)) { >> + case 8000: >> + sampling_rate |= MAX98927_PCM_SR_SET1_SR_8000; > > sampling_rate is only ever set to a real value in this switch statement, > you're not oring with anything else so you can just use an assignment > which would be a lot clearer (it's not obvious what else might go in > there) and the initial assignment to 0 for this and chan_sz can only > mask errors. Removed 'oring'. > >> + /* set sampling rate of IV */ >> + if (max98927->interleave_mode && >> + sampling_rate > MAX98927_PCM_SR_SET1_SR_16000) >> + regmap_update_bits(max98927->regmap, > > Please use the kernel coding style, indent the second line of the if > with the ( so that it doesn't look like part of the conditional code. Modified as requested. > >> + switch (reg) { >> + case MAX98927_R0001_INT_RAW1 ... MAX98927_R0028_ICC_RX_EN_B: > > >> + return true; >> + } >> + return false; > > It'd be clearer to put the return false in the switch statement like the > return true, and also consistent with the _volatile_reg() function. Modified as requested. > >> +static const char * const max98927_speaker_source_text[] = { >> + "i2s", "reserved", "tone", "pdm" >> +}; > > This looks like it should be a DAPM control. Removed this and added it to 'dai_set_format' function. > >> +static const char * const max98927_monomix_output_text[] = { >> + "ch_0", "ch_1", "ch_1_2_div" >> +}; > > Similarly here. Removed this since it was already added to DAPM control. > >> + SOC_SINGLE_TLV("Digital Gain", MAX98927_R0036_AMP_VOL_CTRL, >> + 0, (1<> + max98927_digital_tlv), > > All volume controls should end with Volume as per control-names.rst. Modified as requested. > >> + SOC_SINGLE("Amp DSP Enable", MAX98927_R0052_BROWNOUT_EN, >> + MAX98927_BROWNOUT_DSP_SHIFT, 1, 0), > > All on/off switches should end with Switch as per control-names.rst. Modified as requested. > >> +static int max98927_probe(struct snd_soc_codec *codec) >> +{ > >> + /* Check Revision ID */ >> + ret = regmap_read(max98927->regmap, >> + MAX98927_R01FF_REV_ID, ®); > > Basic device identification and setup should be done at the chip level > probe not at the CODEC level so that if there are problems we fail as > early as possible and so that diagnostic information is available to > users as soon as possible, even if there's no sound card for the device > in the system. Moved to i2c_probe function. > >> + /* Set inital volume (+13dB) */ > > As with all other CODEC drivers you should leave the hardware defaults > alone, what makes sense for your system may not make sense for other > systems and the hardware defaults are a fixed thing. We recommend to use static +13dB gain when our speaker protection algorithm is running on DSP. It is not hardware default but mostly +13dB is being used as default. > >> + /* Boost Output Voltage & Current limit */ >> + regmap_write(max98927->regmap, >> + MAX98927_R0040_BOOST_CTRL0, >> + 0x1C); >> + regmap_write(max98927->regmap, >> + MAX98927_R0042_BOOST_CTRL1, >> + 0x3E); > > This should be system specific, these values might be unsafe in some > systems. Same as above, most user will use maximum voltage & current limit value to take advantage of 10V boost amplifier. I added one more control to change the current limit value. > >> +err: >> + if (max98927) >> + devm_kfree(&i2c->dev, max98927); > > There is no need to explicitly free devm_ allocated memory, that's the > point of devm. Removed the code. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html