From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id B0CB4C27C4F for ; Sun, 30 Jun 2024 10:42:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:Cc:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=u+xJ+OG18POmDU/XIyutTzCrgx+d/qFcWAXcJPPXHcM=; b=4tfmoGFlbGG7a03TwnOBgO33Pc A9wIuIAp+lL/7Oh93n5Cj6o4KF2AfauIhsa67AVeOYkDsV27ef8q7gmSBDg9ttsQJNq6GECQDZPcm R/QBb8IrfwEomHGC60WQttEtRxnFSJq7AP3q/ShNNVv1MHw+ntJGQ/ovqVDAeNSzYi+Z0yBfoleWM qtEUhhLCDsWaTxGpzyPSV9REArgD3VktKb/cLKwIEYq9aj6BjzFW1RwyqIMKq6kHSAcAPhz+BtfAp rcS+JctOyoS5rjruNB8RpJUnGt4RSnTQrPWeYbxlolAuMG6xBRnESSQT9VHBl4hV7LzI7LLWM71dE /7XRE4qA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sNs0O-00000000Ff1-44bF; Sun, 30 Jun 2024 10:42:00 +0000 Received: from sin.source.kernel.org ([2604:1380:40e1:4800::1]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sNs0E-00000000FdB-0hUS for linux-arm-kernel@lists.infradead.org; Sun, 30 Jun 2024 10:41:51 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by sin.source.kernel.org (Postfix) with ESMTP id 6367FCE0AB0; Sun, 30 Jun 2024 10:41:47 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 3F05CC2BD10; Sun, 30 Jun 2024 10:41:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1719744106; bh=g8gLJ95I+E9cX6fK2eb0k1vprsOO40XO2PzIS0mdNbo=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=fujn42pJsncMk7wapKOCQDcGvDfGNRLSp2Ftbwr1klxJk+BDYn1OUAzwTAcWwZ+1z pf6fhyG/F6JjJTQl+5+7rYjev8MBh7lV1HgSW60ND5genAudxDpFWg8idTVONdiYjN veE5AU5q4uGDohbiJh9uXtrLFqeX0h5g0KL94sNaRIJZj5ekDffHooKPUZhg2k9iMr rzLuH5SSZ1RAGdMxSxQsRynHT45nAa8B2JvfCPSaAg/D5Zgz2kv+4fP7pCEIAZOdSr PUEnmyu2K6icfkI40Gwgp6jYsQFHPpyy1rt283F+lLtLV61Qc0Rvb/7v9dr6T1tM9K k10EDHuQiOeKQ== Date: Sun, 30 Jun 2024 11:41:39 +0100 From: Jonathan Cameron To: Sean Anderson Cc: Anand Ashok Dumbre , linux-iio@vger.kernel.org, Lars-Peter Clausen , Michal Simek , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] iio: xilinx-ams: Add labels Message-ID: <20240630114139.13d7bd1b@jic23-huawei> In-Reply-To: <20240623125901.03f0fe6f@jic23-huawei> References: <20240620204842.817237-1-sean.anderson@linux.dev> <20240623125901.03f0fe6f@jic23-huawei> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240630_034150_599787_1E754706 X-CRM114-Status: GOOD ( 28.02 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sun, 23 Jun 2024 12:59:01 +0100 Jonathan Cameron wrote: > On Thu, 20 Jun 2024 16:48:42 -0400 > Sean Anderson wrote: > > > Label all the channels using names from the reference manual. Some of > > the "control" channels are duplicates of other channels. The reference > > manual describes it like: > > > > > The AMS register set includes several measurement registers that are > > > written to by the PS SYSMON unit using the single-channel mode > > > (sequencer off). These voltage measurements are performed using the > > > unipolar sampling circuit with a 0 to 3V range and do not have alarms > > > or minimum/maximum registers. > > > > So I think these really are measuring the same voltages but in a > > different location. In which case, sharing labels makes sense to me. > > > > Signed-off-by: Sean Anderson > Hi Sean, > > This LGTM, but I'd like some feedback from others more familiar with the > driver and device before I apply it. If everyone is too busy and > I don't get any replies for a few weeks I may pick it up anyway. > > Feel free to poke me if I seem to have forgotten in say 2 weeks. Long enough. Applied. > > > Jonathan > > > --- > > > > drivers/iio/adc/xilinx-ams.c | 107 +++++++++++++++++++---------------- > > 1 file changed, 59 insertions(+), 48 deletions(-) > > > > diff --git a/drivers/iio/adc/xilinx-ams.c b/drivers/iio/adc/xilinx-ams.c > > index aa05f24931f9..f051358d6b50 100644 > > --- a/drivers/iio/adc/xilinx-ams.c > > +++ b/drivers/iio/adc/xilinx-ams.c > > @@ -222,7 +222,7 @@ enum ams_ps_pl_seq { > > #define PL_SEQ(x) (AMS_PS_SEQ_MAX + (x)) > > #define AMS_CTRL_SEQ_BASE (AMS_PS_SEQ_MAX * 3) > > > > -#define AMS_CHAN_TEMP(_scan_index, _addr) { \ > > +#define AMS_CHAN_TEMP(_scan_index, _addr, _name) { \ > > .type = IIO_TEMP, \ > > .indexed = 1, \ > > .address = (_addr), \ > > @@ -232,9 +232,10 @@ enum ams_ps_pl_seq { > > .event_spec = ams_temp_events, \ > > .scan_index = _scan_index, \ > > .num_event_specs = ARRAY_SIZE(ams_temp_events), \ > > + .datasheet_name = _name, \ > > } > > > > -#define AMS_CHAN_VOLTAGE(_scan_index, _addr, _alarm) { \ > > +#define AMS_CHAN_VOLTAGE(_scan_index, _addr, _alarm, _name) { \ > > .type = IIO_VOLTAGE, \ > > .indexed = 1, \ > > .address = (_addr), \ > > @@ -243,21 +244,24 @@ enum ams_ps_pl_seq { > > .event_spec = (_alarm) ? ams_voltage_events : NULL, \ > > .scan_index = _scan_index, \ > > .num_event_specs = (_alarm) ? ARRAY_SIZE(ams_voltage_events) : 0, \ > > + .datasheet_name = _name, \ > > } > > > > -#define AMS_PS_CHAN_TEMP(_scan_index, _addr) \ > > - AMS_CHAN_TEMP(PS_SEQ(_scan_index), _addr) > > -#define AMS_PS_CHAN_VOLTAGE(_scan_index, _addr) \ > > - AMS_CHAN_VOLTAGE(PS_SEQ(_scan_index), _addr, true) > > +#define AMS_PS_CHAN_TEMP(_scan_index, _addr, _name) \ > > + AMS_CHAN_TEMP(PS_SEQ(_scan_index), _addr, _name) > > +#define AMS_PS_CHAN_VOLTAGE(_scan_index, _addr, _name) \ > > + AMS_CHAN_VOLTAGE(PS_SEQ(_scan_index), _addr, true, _name) > > > > -#define AMS_PL_CHAN_TEMP(_scan_index, _addr) \ > > - AMS_CHAN_TEMP(PL_SEQ(_scan_index), _addr) > > -#define AMS_PL_CHAN_VOLTAGE(_scan_index, _addr, _alarm) \ > > - AMS_CHAN_VOLTAGE(PL_SEQ(_scan_index), _addr, _alarm) > > +#define AMS_PL_CHAN_TEMP(_scan_index, _addr, _name) \ > > + AMS_CHAN_TEMP(PL_SEQ(_scan_index), _addr, _name) > > +#define AMS_PL_CHAN_VOLTAGE(_scan_index, _addr, _alarm, _name) \ > > + AMS_CHAN_VOLTAGE(PL_SEQ(_scan_index), _addr, _alarm, _name) > > #define AMS_PL_AUX_CHAN_VOLTAGE(_auxno) \ > > - AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(_auxno)), AMS_REG_VAUX(_auxno), false) > > -#define AMS_CTRL_CHAN_VOLTAGE(_scan_index, _addr) \ > > - AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(AMS_SEQ(_scan_index))), _addr, false) > > + AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(_auxno)), AMS_REG_VAUX(_auxno), false, \ > > + "VAUX" #_auxno) > > +#define AMS_CTRL_CHAN_VOLTAGE(_scan_index, _addr, _name) \ > > + AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(AMS_SEQ(_scan_index))), _addr, false, \ > > + _name) > > > > /** > > * struct ams - This structure contains necessary state for xilinx-ams to operate > > @@ -505,6 +509,12 @@ static int ams_init_device(struct ams *ams) > > return 0; > > } > > > > +static int ams_read_label(struct iio_dev *indio_dev, > > + struct iio_chan_spec const *chan, char *label) > > +{ > > + return sysfs_emit(label, "%s\n", chan->datasheet_name); > > +} > > + > > static int ams_enable_single_channel(struct ams *ams, unsigned int offset) > > { > > u8 channel_num; > > @@ -1116,37 +1126,37 @@ static const struct iio_event_spec ams_voltage_events[] = { > > }; > > > > static const struct iio_chan_spec ams_ps_channels[] = { > > - AMS_PS_CHAN_TEMP(AMS_SEQ_TEMP, AMS_TEMP), > > - AMS_PS_CHAN_TEMP(AMS_SEQ_TEMP_REMOTE, AMS_TEMP_REMOTE), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY1, AMS_SUPPLY1), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY2, AMS_SUPPLY2), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY3, AMS_SUPPLY3), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY4, AMS_SUPPLY4), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY5, AMS_SUPPLY5), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY6, AMS_SUPPLY6), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY7, AMS_SUPPLY7), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY8, AMS_SUPPLY8), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY9, AMS_SUPPLY9), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY10, AMS_SUPPLY10), > > - AMS_PS_CHAN_VOLTAGE(AMS_SEQ_VCCAMS, AMS_VCCAMS), > > + AMS_PS_CHAN_TEMP(AMS_SEQ_TEMP, AMS_TEMP, "Temp_LPD"), > > + AMS_PS_CHAN_TEMP(AMS_SEQ_TEMP_REMOTE, AMS_TEMP_REMOTE, "Temp_FPD"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY1, AMS_SUPPLY1, "VCC_PSINTLP"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY2, AMS_SUPPLY2, "VCC_PSINTFP"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY3, AMS_SUPPLY3, "VCC_PSAUX"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY4, AMS_SUPPLY4, "VCC_PSDDR"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY5, AMS_SUPPLY5, "VCC_PSIO3"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY6, AMS_SUPPLY6, "VCC_PSIO0"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY7, AMS_SUPPLY7, "VCC_PSIO1"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY8, AMS_SUPPLY8, "VCC_PSIO2"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY9, AMS_SUPPLY9, "PS_MGTRAVCC"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_SUPPLY10, AMS_SUPPLY10, "PS_MGTRAVTT"), > > + AMS_PS_CHAN_VOLTAGE(AMS_SEQ_VCCAMS, AMS_VCCAMS, "VCC_PSADC"), > > }; > > > > static const struct iio_chan_spec ams_pl_channels[] = { > > - AMS_PL_CHAN_TEMP(AMS_SEQ_TEMP, AMS_TEMP), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY1, AMS_SUPPLY1, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY2, AMS_SUPPLY2, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VREFP, AMS_VREFP, false), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VREFN, AMS_VREFN, false), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY3, AMS_SUPPLY3, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY4, AMS_SUPPLY4, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY5, AMS_SUPPLY5, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY6, AMS_SUPPLY6, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VCCAMS, AMS_VCCAMS, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VP_VN, AMS_VP_VN, false), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY7, AMS_SUPPLY7, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY8, AMS_SUPPLY8, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY9, AMS_SUPPLY9, true), > > - AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY10, AMS_SUPPLY10, true), > > + AMS_PL_CHAN_TEMP(AMS_SEQ_TEMP, AMS_TEMP, "Temp_PL"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY1, AMS_SUPPLY1, true, "VCCINT"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY2, AMS_SUPPLY2, true, "VCCAUX"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VREFP, AMS_VREFP, false, "VREFP"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VREFN, AMS_VREFN, false, "VREFN"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY3, AMS_SUPPLY3, true, "VCCBRAM"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY4, AMS_SUPPLY4, true, "VCC_PSINTLP"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY5, AMS_SUPPLY5, true, "VCC_PSINTFP"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY6, AMS_SUPPLY6, true, "VCC_PSAUX"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VCCAMS, AMS_VCCAMS, true, "VCCAMS"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_VP_VN, AMS_VP_VN, false, "VP_VN"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY7, AMS_SUPPLY7, true, "VUser0"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY8, AMS_SUPPLY8, true, "VUser1"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY9, AMS_SUPPLY9, true, "VUser2"), > > + AMS_PL_CHAN_VOLTAGE(AMS_SEQ_SUPPLY10, AMS_SUPPLY10, true, "VUser3"), > > AMS_PL_AUX_CHAN_VOLTAGE(0), > > AMS_PL_AUX_CHAN_VOLTAGE(1), > > AMS_PL_AUX_CHAN_VOLTAGE(2), > > @@ -1166,13 +1176,13 @@ static const struct iio_chan_spec ams_pl_channels[] = { > > }; > > > > static const struct iio_chan_spec ams_ctrl_channels[] = { > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSPLL, AMS_VCC_PSPLL0), > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSBATT, AMS_VCC_PSPLL3), > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCINT, AMS_VCCINT), > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCBRAM, AMS_VCCBRAM), > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCAUX, AMS_VCCAUX), > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_PSDDRPLL, AMS_PSDDRPLL), > > - AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_INTDDR, AMS_PSINTFPDDR), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSPLL, AMS_VCC_PSPLL0, "VCC_PSPLL"), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCC_PSBATT, AMS_VCC_PSPLL3, "VCC_PSBATT"), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCINT, AMS_VCCINT, "VCCINT"), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCBRAM, AMS_VCCBRAM, "VCCBRAM"), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_VCCAUX, AMS_VCCAUX, "VCCAUX"), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_PSDDRPLL, AMS_PSDDRPLL, "VCC_PSDDR_PLL"), > > + AMS_CTRL_CHAN_VOLTAGE(AMS_SEQ_INTDDR, AMS_PSINTFPDDR, "VCC_PSINTFP_DDR"), > > }; > > > > static int ams_get_ext_chan(struct fwnode_handle *chan_node, > > @@ -1336,6 +1346,7 @@ static int ams_parse_firmware(struct iio_dev *indio_dev) > > } > > > > static const struct iio_info iio_ams_info = { > > + .read_label = ams_read_label, > > .read_raw = &ams_read_raw, > > .read_event_config = &ams_read_event_config, > > .write_event_config = &ams_write_event_config, > >