From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f51.google.com (mail-wm1-f51.google.com [209.85.128.51]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id DA6B823ABA6 for ; Thu, 10 Jul 2025 15:37:42 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.51 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752161864; cv=none; b=QaMayDxw++N8Y0U0C91Baq+z5xe3p2ALvBtPMgfaIIACztE3OrRXmy9ScshNMeM1dh1mfsXGpMZZyvu38U5GKOMMLui16Gs1JSl1Q1TsjfgpUS8u5oh+udnZ5hbUK5w3tInfe/pplxmsNdczbAMqQ9nmRLLeYuKFyDvqAA32yIo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1752161864; c=relaxed/simple; bh=QwOQObB2pctndAzOHsW+e0d5hzIcqwrg+o1Xv3mv1Ck=; h=Mime-Version:Content-Type:Date:Message-Id:Cc:Subject:From:To: References:In-Reply-To; b=qIB7SnwTD/BsfWfm8rpPbDhrwj1enWDOlH6kocVxj8qU4h5R+1RXjxFrmxho0GuQgVMnPIPhxDpdBSj29c92t+HS0cfRRVXzqSbxiuY62+0JLYy2ks7PlshhEdYmuwlu9iC8oUOnqEsCaxeJTrOeuDt3umvC53bP04cMZQUqO08= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org; spf=pass smtp.mailfrom=linaro.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b=cFfR0/cm; arc=none smtp.client-ip=209.85.128.51 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=linaro.org Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="cFfR0/cm" Received: by mail-wm1-f51.google.com with SMTP id 5b1f17b1804b1-451d41e1ad1so7574135e9.1 for ; Thu, 10 Jul 2025 08:37:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1752161861; x=1752766661; darn=vger.kernel.org; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=1GCMYIRIYYP0/PAurGhaaMN0CHgGvBkGkwVspybvVpo=; b=cFfR0/cmJ5FwwMV4LCTX/c9A3wp/L+ANymfqAaVa8X0oyur3NMbaXYG2hNOlccUbQ3 nC88DrR6wGIOqZrqONe9VBhS7mdtlKrnnDY1uIOTRPD2GyfJaQdR19DpZD0YZ8gRBDIM JXu6HmvoTUrO1CdA2hW113LWh39mZgwGkbYHPHn6GOQzB53DexyJc6oEEzD6O3B6opyw AqCiL7N0OcgL09zMfdeU/ailbFvl7o+px+hNCnAnUMxL945CzST2FrxT+BqhBhnRuwra 1V2hopI13ukDL/b+yPgX7n2HM82BVMHdvDUqry489jf3Zmy3b+AdU8JT+2mmuhIYJEe/ Lo/g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1752161861; x=1752766661; h=in-reply-to:references:to:from:subject:cc:message-id:date :content-transfer-encoding:mime-version:x-gm-message-state:from:to :cc:subject:date:message-id:reply-to; bh=1GCMYIRIYYP0/PAurGhaaMN0CHgGvBkGkwVspybvVpo=; b=L2Rzc45HcYyIu6kFcqqZ84tlKARjDEqb/bZEjIcFw64jGxt33GZWxX7HbOATd1/zyZ NI816GXIzRSSAVla4bA9bd2tFVih0ZXjLhvW1gVSNMeuK8XgnL4exFuSDT12Vn81orNq nv/ey30wIKT2qv0F7SwEbjHqAAnNj2ckOHDfJWd7tebMQHpcUgvLzq94oyPd+kp94Vx4 Av0mrLJRfWTu0PmRcDD8PuFTLS/12jTd19q7688zIxnlA6zsR2De6MnZNZk4h3J3Mwlj AHIn4apaOEOOUhR+goOqfBlqEWV0sXDCECmnqYb3oUChmnaqw8jZgxHAf0ZyxbElW7V8 DnrQ== X-Forwarded-Encrypted: i=1; AJvYcCVql2tTt2cZwWB8Dhz0/oY+GF1pW4Kg2aMhLBrGudCmXXjI3fQFGWuAhGm45OBgZ/HvE6I6veultGdJ/Q==@vger.kernel.org X-Gm-Message-State: AOJu0Yyt06PxLUAiMQO3Qp47hIJLpz5ediSeXaGuGkb36Pp0xXmXw4ZH RhfAzPFQ/OhkKtwcfRx7P7hA9Z/AUEPqJWEVqBzypXIWGcOrJMi2re9jIYBCLzQXMUE= X-Gm-Gg: ASbGncuv/qd78ne+0JhP6SCoTZi2nOr6Bo+BOL16AagssyaTyXSH7GW7BnsKmylWI+v h8EY4DuBrpV3olngNDZeIoqbdTbC4khmeI2CFtm//7Mxs71F03cmf5RQa95dLQVjLEFy8gsTtUb FGCVBKSwF/lzhfN4keVSBzUICGk8GtTsTLw7pw/kk7+s+WNjsUyxJfHmgAoB2pRynGPqeI2S1IF 9SbEV8TXjqzKA/KCJfYn5srFU1U5eLEPY/rZvD1Sg0Riq/c1en4gQtYCumK5/z5M8819fx39Fjv 7ijxVtzvWsyAIE7WXuYOszNgOEO0FB+Btht3T8Y/R7NcP0O69vh+1dDzuRJEsEZvqK8= X-Google-Smtp-Source: AGHT+IGw7DVfZ3GZxj+qHwr1y0PJneaT93zpwe7OGfPSm+0CYQeA+WTJUdUF6Ib1mvex15f2hc3lew== X-Received: by 2002:a05:6000:400e:b0:3aa:34f4:d437 with SMTP id ffacd0b85a97d-3b5e4529684mr5959353f8f.37.1752161861054; Thu, 10 Jul 2025 08:37:41 -0700 (PDT) Received: from localhost ([2a00:2381:fd67:101:6c39:59e6:b76d:825]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-454cdb549absm65784495e9.1.2025.07.10.08.37.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 10 Jul 2025 08:37:40 -0700 (PDT) Precedence: bulk X-Mailing-List: linux-sound@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 10 Jul 2025 16:37:39 +0100 Message-Id: Cc: "Srinivas Kandagatla" , "Liam Girdwood" , "Mark Brown" , "Rob Herring" , "Krzysztof Kozlowski" , "Conor Dooley" , "Stephen Boyd" , "Lee Jones" , "Jaroslav Kysela" , "Takashi Iwai" , , , , , "Dmitry Baryshkov" , "Srinivas Kandagatla" Subject: Re: [PATCH 3/3] ASoC: codecs: add new pm4125 audio codec driver From: "Alexey Klimov" To: "Krzysztof Kozlowski" X-Mailer: aerc 0.20.0 References: <20250626-pm4125_audio_codec_v1-v1-0-e52933c429a0@linaro.org> <20250626-pm4125_audio_codec_v1-v1-3-e52933c429a0@linaro.org> In-Reply-To: On Thu Jun 26, 2025 at 7:19 AM BST, Krzysztof Kozlowski wrote: > On Thu, Jun 26, 2025 at 12:50:31AM +0100, Alexey Klimov wrote: >> + >> +static int pm4125_add_slave_components(struct pm4125_priv *pm4125, >> + struct device *dev, >> + struct component_match **matchptr) >> +{ >> + struct device_node *np =3D dev->of_node; >> + >> + pm4125->rxnode =3D of_parse_phandle(np, "qcom,rx-device", 0); >> + if (!pm4125->rxnode) { >> + dev_err(dev, "Couldn't parse phandle to qcom,rx-device!\n"); >> + return -ENODEV; >> + } >> + of_node_get(pm4125->rxnode); > > Where do you clean this up? Please don't tell me that this is a bug that being copied from driver to driver. I changed it to such flow for the next version since it seems that referenc= e should be decremented after of_parse_phandle() returns with it incremented: rxnode =3D of_parse_phandle(); if (!rxnode) return dev_err_probe(...); component_match_add_release(..., rxnode); of_node_put(rxnode); >> + component_match_add_release(dev, matchptr, component_release_of, >> + component_compare_of, pm4125->rxnode); >> + >> + pm4125->txnode =3D of_parse_phandle(np, "qcom,tx-device", 0); >> + if (!pm4125->txnode) { >> + dev_err(dev, "Couldn't parse phandle to qcom,tx-device\n"); >> + return -ENODEV; > > Messed indent. This should be anyway just one line as always - return > dev_err_probe. I changed it for the next version as you suggested. Thanks. >> + } >> + of_node_get(pm4125->txnode); > > And this? > >> + component_match_add_release(dev, matchptr, component_release_of, >> + component_compare_of, pm4125->txnode); >> + >> + return 0; >> +} >> + >> +static int pm4125_probe(struct platform_device *pdev) >> +{ >> + struct component_match *match =3D NULL; >> + struct device *dev =3D &pdev->dev; >> + struct pm4125_priv *pm4125; >> + struct wcd_mbhc_config *cfg; >> + int ret; >> + >> + pm4125 =3D devm_kzalloc(dev, sizeof(*pm4125), GFP_KERNEL); >> + if (!pm4125) >> + return -ENOMEM; >> + >> + dev_set_drvdata(dev, pm4125); >> + >> + cfg =3D &pm4125->mbhc_cfg; >> + cfg->swap_gnd_mic =3D pm4125_swap_gnd_mic; >> + >> + pm4125->supplies[0].supply =3D "vdd-io"; >> + pm4125->supplies[1].supply =3D "vdd-cp"; >> + pm4125->supplies[2].supply =3D "vdd-mic-bias"; >> + pm4125->supplies[3].supply =3D "vdd-pa-vpos"; >> + >> + ret =3D devm_regulator_bulk_get(dev, PM4125_MAX_BULK_SUPPLY, pm4125->s= upplies); >> + if (ret) >> + return dev_err_probe(dev, ret, "Failed to get supplies\n"); >> + >> + ret =3D regulator_bulk_enable(PM4125_MAX_BULK_SUPPLY, pm4125->supplies= ); >> + if (ret) { >> + regulator_bulk_free(PM4125_MAX_BULK_SUPPLY, pm4125->supplies); > > Double free. Thanks. >> + return dev_err_probe(dev, ret, "Failed to enable supplies\n"); >> + } >> + >> + pm4125_dt_parse_micbias_info(dev, pm4125); >> + >> + cfg->mbhc_micbias =3D MIC_BIAS_2; >> + cfg->anc_micbias =3D MIC_BIAS_2; >> + cfg->v_hs_max =3D WCD_MBHC_HS_V_MAX; >> + cfg->num_btn =3D PM4125_MBHC_MAX_BUTTONS; >> + cfg->micb_mv =3D pm4125->micb2_mv; >> + cfg->linein_th =3D 5000; >> + cfg->hs_thr =3D 1700; >> + cfg->hph_thr =3D 50; [..] >> +#if defined(CONFIG_OF) >> +static const struct of_device_id pm4125_of_match[] =3D { >> + { .compatible =3D "qcom,pm4125-codec" }, >> + { } >> +}; >> +MODULE_DEVICE_TABLE(of, pm4125_of_match); >> +#endif >> + >> +static struct platform_driver pm4125_codec_driver =3D { >> + .probe =3D pm4125_probe, >> + .remove =3D pm4125_remove, >> + .driver =3D { >> + .name =3D "pm4125_codec", >> + .of_match_table =3D of_match_ptr(pm4125_of_match), > > Drop of_match_ptr and #if. We just removed it (or trying to ) > everywhere, so why re-introducing it... Will remove it. Thanks. >> + .suppress_bind_attrs =3D true, >> + }, >> +}; >> + >> +module_platform_driver(pm4125_codec_driver); >> +MODULE_DESCRIPTION("PM4125 audio codec driver"); >> +MODULE_LICENSE("GPL"); >> diff --git a/sound/soc/codecs/pm4125.h b/sound/soc/codecs/pm4125.h >> new file mode 100644 >> index 0000000000000000000000000000000000000000..2c5e8218202d92a0adc49341= 3368991a406471b0 >> --- /dev/null >> +++ b/sound/soc/codecs/pm4125.h [...] >> +const u8 pm4125_reg_access_analog[ > > No, you cannot have data defined in the header. This is neither style of > C, nor Linux kernel, nor makes any sense. What if this will be included > by some other unit? This is some terrible downstream style. > > Heh... you actually do include it twice, so you would see all the > duplicated data for no reason at all. I pulled in the change that fixes this for the next version. Thank you. Best regards, Alexey