Message ID | 20230929-ad2s1210-mainline-v3-22-fa4364281745@baylibre.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp4241129vqu; Fri, 29 Sep 2023 11:52:21 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHky1ojWbYhy2oqcg7NRIfTrribEc01KO5WX/JXf5gyBYijQpvFUwLJB6kT1D1foOJhzO2G X-Received: by 2002:a05:6e02:1561:b0:352:5f9c:76c4 with SMTP id k1-20020a056e02156100b003525f9c76c4mr4119763ilu.12.1696013540849; Fri, 29 Sep 2023 11:52:20 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1696013540; cv=none; d=google.com; s=arc-20160816; b=c/5BIkNlimvlOYU7z6QkNefx7J1jp9Z6vqigugQzuHzyt0rHAeC/uyua0sW2oQn32e 3oaGUplpB6gGY6fL93enbK09fNW8yDPJkbaR2t/ZAnihYTejv1f5le1rEk6fEqrrkz5V /Kt0KzOHWk+742nsNR49F01GIRqEE8JxleDl8XBjORqsssnkTmx3y3DQXbOzIdfiSKr9 /dbKptV2MO2HBEO0NzbfP/ZwFkIbMdoH4+xGrjvFAeBUo8Ppvqhr67k5CBQGw/UNkb46 3W7bpNWdV8piRaFwwpbbgQHltVFrkLyi2OZW4XDn7wehExmdkuW3ziaRISqWthdenIID 4rjA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=5bKd2JyQzTBZqficOTxYW/B6OToPwlC7WYvtiMW5PiI=; fh=/JQ89VrFJ6cddpMA/8uKU4hR6LyRWF8O9dzD336qQ7I=; b=qUbVEJ894p+vkqHbKCdLwjicmHPKu0xfIIWL2iztnsFdZulgbACBrooYm597qKDItU osOwkfbgFXpz6IqQw9KepupeKbweIvBWKFsC99CsQmKM3DBpqCunwZu/nrx4XeEpKrnH emsYtEyJrwI7sU5K0RlKcwlZ5M4KHogV2gN1FTvOPT3u+B2iGFUpgbFtE6zbw/qzVTcp hJux7VfEw5o+4YuvXMuCrdi5lbaGkxPd24unxdtUP7IJNhrPI2SM0nahcJ9EGQnUaFzR RwYvXQSEFfZcM22NDTYwy8SWAji+nhejZFSl+aaWJ8/uRzT9O2k3ci1bOkmNRFMcgjdw Eveg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@baylibre-com.20230601.gappssmtp.com header.s=20230601 header.b="0xS7Th/B"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from snail.vger.email (snail.vger.email. [23.128.96.37]) by mx.google.com with ESMTPS id c192-20020a6335c9000000b00578a9192d90si22333265pga.140.2023.09.29.11.52.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Sep 2023 11:52:20 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37; Authentication-Results: mx.google.com; dkim=pass header.i=@baylibre-com.20230601.gappssmtp.com header.s=20230601 header.b="0xS7Th/B"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 4CA8C8370801; Fri, 29 Sep 2023 10:27:16 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233948AbjI2R0z (ORCPT <rfc822;pwkd43@gmail.com> + 19 others); Fri, 29 Sep 2023 13:26:55 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45994 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233759AbjI2R0V (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 29 Sep 2023 13:26:21 -0400 Received: from mail-oo1-xc36.google.com (mail-oo1-xc36.google.com [IPv6:2607:f8b0:4864:20::c36]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B2FDB1711 for <linux-kernel@vger.kernel.org>; Fri, 29 Sep 2023 10:26:07 -0700 (PDT) Received: by mail-oo1-xc36.google.com with SMTP id 006d021491bc7-57be3d8e738so3881582eaf.1 for <linux-kernel@vger.kernel.org>; Fri, 29 Sep 2023 10:26:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20230601.gappssmtp.com; s=20230601; t=1696008367; x=1696613167; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=5bKd2JyQzTBZqficOTxYW/B6OToPwlC7WYvtiMW5PiI=; b=0xS7Th/BhhkVpTokJBaAWC3jdPj1Om6SEpgZsMToLFxqwaZ8uyRennXxR75UipWJBZ 4xHlVXKvbTaL6GZC7cw2HHYEtH80/N+155zwuxeEe1MjKuPsBn6Krv19Ldlq7M4e41BT ch1qX/rehIOSb9mjnhR60T8c30hT5XFfPaa0RZQih5G68R84JwctDO57fVTOG3TG7ufo G2aCY36Wdo55BAFsTcUpE2qfrkVpFQVhsqrfMWJSnaVNVll1xTWjdjs0Kl5OpSgRGN3G zjffcE8nEeRoAsupUaZkGKa/S0eCqHwqvRUKK79H9QsuPDQu2MlXDOILFvWrsmjEWEiB JWPw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696008367; x=1696613167; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=5bKd2JyQzTBZqficOTxYW/B6OToPwlC7WYvtiMW5PiI=; b=TDoouFYHLEKnPnxUldtlLoeD8oalRHcmUIYqFAiRm8gFqr+0qPaB60Wx6Ezrc3eUyL 9QGVDA1bbUfuWazSCareDVEZ1cmi+GUWuxK5IZkXUKm4wb96DYwCqrwUyoQoMMGziZ1x 6Zj5RTsZBJo9qxgw0z632LPUIW1voLkNb7IjUC7wwMXzmf1yRI+lBZ8ZLP7p96RRt2xW w5D5JBi/wwl1SCkFm29yo9VS1RmDKxjMYypHQ0tBkewqwU2qgfPZB5ppoggOCLDpj4ZI /pC/Vz0IwRBOUmOy5+Oe7dKmJrK8rdK+if8ZSJHrxoh/DDtNg4Uq+olcHaedQ+Fern1M NraQ== X-Gm-Message-State: AOJu0YylZsc+WewEnNhghYoUIn7oIGiTBQ5pdNDuuKH9Smb5IPmY7usS SeEuf7a8H2cK/sQ2OwjOZhCU0g== X-Received: by 2002:a4a:9b43:0:b0:57b:5c28:4169 with SMTP id e3-20020a4a9b43000000b0057b5c284169mr4937720ook.1.1696008366821; Fri, 29 Sep 2023 10:26:06 -0700 (PDT) Received: from freyr.lechnology.com (ip98-183-112-25.ok.ok.cox.net. [98.183.112.25]) by smtp.gmail.com with ESMTPSA id f128-20020a4a5886000000b0057bb326cad4sm2272915oob.33.2023.09.29.10.26.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 29 Sep 2023 10:26:06 -0700 (PDT) From: David Lechner <dlechner@baylibre.com> To: linux-iio@vger.kernel.org, devicetree@vger.kernel.org, linux-staging@lists.linux.dev Cc: David Lechner <david@lechnology.com>, Jonathan Cameron <jic23@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Conor Dooley <conor+dt@kernel.org>, Michael Hennerich <Michael.Hennerich@analog.com>, =?utf-8?q?Nuno_S=C3=A1?= <nuno.sa@analog.com>, Axel Haslam <ahaslam@baylibre.com>, Philip Molloy <pmolloy@baylibre.com>, linux-kernel@vger.kernel.org, David Lechner <dlechner@baylibre.com> Subject: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr Date: Fri, 29 Sep 2023 12:23:27 -0500 Message-ID: <20230929-ad2s1210-mainline-v3-22-fa4364281745@baylibre.com> X-Mailer: git-send-email 2.42.0 In-Reply-To: <20230929-ad2s1210-mainline-v3-0-fa4364281745@baylibre.com> References: <20230929-ad2s1210-mainline-v3-0-fa4364281745@baylibre.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" X-Mailer: b4 0.12.3 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Fri, 29 Sep 2023 10:27:16 -0700 (PDT) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1778399094205394280 X-GMAIL-MSGID: 1778399094205394280 |
Series |
iio: resolver: move ad2s1210 out of staging
|
|
Commit Message
David Lechner
Sept. 29, 2023, 5:23 p.m. UTC
From: David Lechner <david@lechnology.com> From: David Lechner <dlechner@baylibre.com> The AD2S1210 has a programmable threshold for the loss of signal (LOS) fault. This fault is triggered when either the sine or cosine input falls below the threshold voltage. This patch converts the custom device LOS threshold attribute to an event falling edge threshold attribute on a new monitor signal channel. The monitor signal is an internal signal that combines the amplitudes of the sine and cosine inputs as well as the current angle and position output. This signal is used to detect faults in the input signals. The attribute now uses millivolts instead of the raw register value in accordance with the IIO ABI. Emitting the event will be implemented in a later patch. Signed-off-by: David Lechner <dlechner@baylibre.com> --- v3 changes: This is a new patch in v3 drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 4 deletions(-)
Comments
On Fri, 29 Sep 2023 12:23:27 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > fault. This fault is triggered when either the sine or cosine input > falls below the threshold voltage. > > This patch converts the custom device LOS threshold attribute to an > event falling edge threshold attribute on a new monitor signal channel. > The monitor signal is an internal signal that combines the amplitudes > of the sine and cosine inputs as well as the current angle and position > output. This signal is used to detect faults in the input signals. > > The attribute now uses millivolts instead of the raw register value in > accordance with the IIO ABI. > > Emitting the event will be implemented in a later patch. > > Signed-off-by: David Lechner <dlechner@baylibre.com> I think I'm fine with treating these internal signals like this, but I would ideally like someone from Analog devices to take a look at how these are being done and make sure our interpretations of the signals make sense to them. We are pushing the boundaries a little here (though we have done similar before for fault events I think.) Jonathan
On Fri, 29 Sep 2023 12:23:27 -0500 David Lechner <dlechner@baylibre.com> wrote: > From: David Lechner <david@lechnology.com> > > From: David Lechner <dlechner@baylibre.com> > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > fault. This fault is triggered when either the sine or cosine input > falls below the threshold voltage. > > This patch converts the custom device LOS threshold attribute to an > event falling edge threshold attribute on a new monitor signal channel. > The monitor signal is an internal signal that combines the amplitudes > of the sine and cosine inputs as well as the current angle and position > output. This signal is used to detect faults in the input signals. Hmm. Looking forwards, I'm less sure that we should be shoving all these error conditions onto one channel. Fundamentally we have sine and cosine inputs. I think we should treat those as separate channels and include a third differential channel between them. So this one becomes a double event (you need to signal it on both cosine and sine channels). The DOS overange is similar. The DOS mismatch is a threshold on the differential channel giving events/in_altvoltage0_thresh_falling_value events/in_altvoltage1_thresh_falling_value (these match) events/in_altvoltage0_thresh_rising_value events/in_altvoltage1_thresh_rising_value (matches previous which is fine) events/in_altvoltage1-0_mag_rising_value Does that work here? Avoids smashing different types of signals together. We could even do the LOT as differential between two angle channels (tracking one and measured one) but meh that's getting complex. Note this will rely on channel labels to make the above make any sense at all. Jonathan > > The attribute now uses millivolts instead of the raw register value in > accordance with the IIO ABI. > > Emitting the event will be implemented in a later patch. > > Signed-off-by: David Lechner <dlechner@baylibre.com> > --- > > v3 changes: This is a new patch in v3 > > drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++-- > 1 file changed, 71 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > index 5cc8106800d6..7abbc184c351 100644 > --- a/drivers/staging/iio/resolver/ad2s1210.c > +++ b/drivers/staging/iio/resolver/ad2s1210.c > @@ -66,6 +66,11 @@ > #define PHASE_360_DEG_TO_RAD_INT 6 > #define PHASE_360_DEG_TO_RAD_MICRO 283185 > > +/* Threshold voltage registers have 1 LSB == 38 mV */ > +#define THRESHOLD_MILLIVOLT_PER_LSB 38 > +/* max voltage for threshold registers is 0x7F * 38 mV */ > +#define THRESHOLD_RANGE_STR "[0 38 4826]" > + > enum ad2s1210_mode { > MOD_POS = 0b00, > MOD_VEL = 0b01, > @@ -448,6 +453,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = { > 1237, /* 16-bit: same as 14-bit */ > }; > > +static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st, > + unsigned int reg, int *val) > +{ > + unsigned int reg_val; > + int ret; > + > + mutex_lock(&st->lock); > + ret = regmap_read(st->regmap, reg, ®_val); > + mutex_unlock(&st->lock); > + > + if (ret < 0) > + return ret; > + > + *val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB; > + return IIO_VAL_INT; > +} > + > +static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st, > + unsigned int reg, int val) > +{ > + unsigned int reg_val; > + int ret; > + > + reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB; > + > + mutex_lock(&st->lock); > + ret = regmap_write(st->regmap, reg, reg_val); > + mutex_unlock(&st->lock); > + > + return ret; > +} > + > static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st, > int *val, int *val2) > { > @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, > static IIO_DEVICE_ATTR(fault, 0644, > ad2s1210_show_fault, ad2s1210_clear_fault, 0); > > -static IIO_DEVICE_ATTR(los_thrd, 0644, > - ad2s1210_show_reg, ad2s1210_store_reg, > - AD2S1210_REG_LOS_THRD); > static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644, > ad2s1210_show_reg, ad2s1210_store_reg, > AD2S1210_REG_DOS_OVR_THRD); > @@ -745,6 +779,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = { > }, > }; > > +static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = { > + { > + /* Sine/cosine below LOS threshold fault. */ > + .type = IIO_EV_TYPE_THRESH, > + .dir = IIO_EV_DIR_FALLING, > + /* Loss of signal threshold. */ > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > + }, > +}; > + > static const struct iio_chan_spec ad2s1210_channels[] = { > { > .type = IIO_ANGL, > @@ -803,12 +847,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > .scan_index = -1, > .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY), > .info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY), > + }, { > + /* monitor signal */ > + .type = IIO_ALTVOLTAGE, > + .indexed = 1, > + .channel = 0, > + .scan_index = -1, > + .event_spec = ad2s1210_monitor_signal_event_spec, > + .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec), > }, > }; > > static struct attribute *ad2s1210_attributes[] = { > &iio_dev_attr_fault.dev_attr.attr, > - &iio_dev_attr_los_thrd.dev_attr.attr, > &iio_dev_attr_dos_ovr_thrd.dev_attr.attr, > &iio_dev_attr_dos_mis_thrd.dev_attr.attr, > &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr, > @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, > __stringify(PHASE_44_DEG_TO_RAD_MICRO) " " > __stringify(PHASE_360_DEG_TO_RAD_INT) "." > __stringify(PHASE_360_DEG_TO_RAD_MICRO)); > +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); > > static struct attribute *ad2s1210_event_attributes[] = { > &iio_const_attr_in_phase0_mag_value_available.dev_attr.attr, > + &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, > NULL, > @@ -904,6 +957,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev, > default: > return -EINVAL; > } > + case IIO_ALTVOLTAGE: > + if (chan->output) > + return -EINVAL; > + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING) > + return ad2s1210_get_voltage_threshold(st, > + AD2S1210_REG_LOS_THRD, val); > + return -EINVAL; > case IIO_PHASE: > return ad2s1210_get_phase_lock_range(st, val, val2); > default: > @@ -930,6 +990,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev, > default: > return -EINVAL; > } > + case IIO_ALTVOLTAGE: > + if (chan->output) > + return -EINVAL; > + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING) > + return ad2s1210_set_voltage_threshold(st, > + AD2S1210_REG_LOS_THRD, val); > + return -EINVAL; > case IIO_PHASE: > return ad2s1210_set_phase_lock_range(st, val, val2); > default: >
On Sat, 30 Sep 2023 16:42:51 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 29 Sep 2023 12:23:27 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > > fault. This fault is triggered when either the sine or cosine input > > falls below the threshold voltage. > > > > This patch converts the custom device LOS threshold attribute to an > > event falling edge threshold attribute on a new monitor signal channel. > > The monitor signal is an internal signal that combines the amplitudes > > of the sine and cosine inputs as well as the current angle and position > > output. This signal is used to detect faults in the input signals. > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > error conditions onto one channel. Fundamentally we have > sine and cosine inputs. I think we should treat those as separate channels > and include a third differential channel between them. > > So this one becomes a double event (you need to signal it on both > cosine and sine channels). The DOS overange is similar. > The DOS mismatch is a threshold on the differential channel giving > > events/in_altvoltage0_thresh_falling_value > events/in_altvoltage1_thresh_falling_value (these match) > events/in_altvoltage0_thresh_rising_value > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > events/in_altvoltage1-0_mag_rising_value Sorry, got the syntax wrong :( Should have checked the ABI docs. events/in_altvoltage1-altvoltage0_mag_rising_value > > Does that work here? Avoids smashing different types of signals together. > We could even do the LOT as differential between two angle channels > (tracking one and measured one) but meh that's getting complex. > > Note this will rely on channel labels to make the above make any sense at all. > > Jonathan > > > > > > The attribute now uses millivolts instead of the raw register value in > > accordance with the IIO ABI. > > > > Emitting the event will be implemented in a later patch. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > --- > > > > v3 changes: This is a new patch in v3 > > > > drivers/staging/iio/resolver/ad2s1210.c | 75 +++++++++++++++++++++++++++++++-- > > 1 file changed, 71 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c > > index 5cc8106800d6..7abbc184c351 100644 > > --- a/drivers/staging/iio/resolver/ad2s1210.c > > +++ b/drivers/staging/iio/resolver/ad2s1210.c > > @@ -66,6 +66,11 @@ > > #define PHASE_360_DEG_TO_RAD_INT 6 > > #define PHASE_360_DEG_TO_RAD_MICRO 283185 > > > > +/* Threshold voltage registers have 1 LSB == 38 mV */ > > +#define THRESHOLD_MILLIVOLT_PER_LSB 38 > > +/* max voltage for threshold registers is 0x7F * 38 mV */ > > +#define THRESHOLD_RANGE_STR "[0 38 4826]" > > + > > enum ad2s1210_mode { > > MOD_POS = 0b00, > > MOD_VEL = 0b01, > > @@ -448,6 +453,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = { > > 1237, /* 16-bit: same as 14-bit */ > > }; > > > > +static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st, > > + unsigned int reg, int *val) > > +{ > > + unsigned int reg_val; > > + int ret; > > + > > + mutex_lock(&st->lock); > > + ret = regmap_read(st->regmap, reg, ®_val); > > + mutex_unlock(&st->lock); > > + > > + if (ret < 0) > > + return ret; > > + > > + *val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB; > > + return IIO_VAL_INT; > > +} > > + > > +static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st, > > + unsigned int reg, int val) > > +{ > > + unsigned int reg_val; > > + int ret; > > + > > + reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB; > > + > > + mutex_lock(&st->lock); > > + ret = regmap_write(st->regmap, reg, reg_val); > > + mutex_unlock(&st->lock); > > + > > + return ret; > > +} > > + > > static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st, > > int *val, int *val2) > > { > > @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, > > static IIO_DEVICE_ATTR(fault, 0644, > > ad2s1210_show_fault, ad2s1210_clear_fault, 0); > > > > -static IIO_DEVICE_ATTR(los_thrd, 0644, > > - ad2s1210_show_reg, ad2s1210_store_reg, > > - AD2S1210_REG_LOS_THRD); > > static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644, > > ad2s1210_show_reg, ad2s1210_store_reg, > > AD2S1210_REG_DOS_OVR_THRD); > > @@ -745,6 +779,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = { > > }, > > }; > > > > +static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = { > > + { > > + /* Sine/cosine below LOS threshold fault. */ > > + .type = IIO_EV_TYPE_THRESH, > > + .dir = IIO_EV_DIR_FALLING, > > + /* Loss of signal threshold. */ > > + .mask_separate = BIT(IIO_EV_INFO_VALUE), > > + }, > > +}; > > + > > static const struct iio_chan_spec ad2s1210_channels[] = { > > { > > .type = IIO_ANGL, > > @@ -803,12 +847,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = { > > .scan_index = -1, > > .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY), > > .info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY), > > + }, { > > + /* monitor signal */ > > + .type = IIO_ALTVOLTAGE, > > + .indexed = 1, > > + .channel = 0, > > + .scan_index = -1, > > + .event_spec = ad2s1210_monitor_signal_event_spec, > > + .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec), > > }, > > }; > > > > static struct attribute *ad2s1210_attributes[] = { > > &iio_dev_attr_fault.dev_attr.attr, > > - &iio_dev_attr_los_thrd.dev_attr.attr, > > &iio_dev_attr_dos_ovr_thrd.dev_attr.attr, > > &iio_dev_attr_dos_mis_thrd.dev_attr.attr, > > &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr, > > @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, > > __stringify(PHASE_44_DEG_TO_RAD_MICRO) " " > > __stringify(PHASE_360_DEG_TO_RAD_INT) "." > > __stringify(PHASE_360_DEG_TO_RAD_MICRO)); > > +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); > > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); > > IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); > > > > static struct attribute *ad2s1210_event_attributes[] = { > > &iio_const_attr_in_phase0_mag_value_available.dev_attr.attr, > > + &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, > > &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, > > &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, > > NULL, > > @@ -904,6 +957,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev, > > default: > > return -EINVAL; > > } > > + case IIO_ALTVOLTAGE: > > + if (chan->output) > > + return -EINVAL; > > + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING) > > + return ad2s1210_get_voltage_threshold(st, > > + AD2S1210_REG_LOS_THRD, val); > > + return -EINVAL; > > case IIO_PHASE: > > return ad2s1210_get_phase_lock_range(st, val, val2); > > default: > > @@ -930,6 +990,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev, > > default: > > return -EINVAL; > > } > > + case IIO_ALTVOLTAGE: > > + if (chan->output) > > + return -EINVAL; > > + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING) > > + return ad2s1210_set_voltage_threshold(st, > > + AD2S1210_REG_LOS_THRD, val); > > + return -EINVAL; > > case IIO_PHASE: > > return ad2s1210_set_phase_lock_range(st, val, val2); > > default: > > >
On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Fri, 29 Sep 2023 12:23:27 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > > fault. This fault is triggered when either the sine or cosine input > > falls below the threshold voltage. > > > > This patch converts the custom device LOS threshold attribute to an > > event falling edge threshold attribute on a new monitor signal channel. > > The monitor signal is an internal signal that combines the amplitudes > > of the sine and cosine inputs as well as the current angle and position > > output. This signal is used to detect faults in the input signals. > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > error conditions onto one channel. Fundamentally we have > sine and cosine inputs. I think we should treat those as separate channels > and include a third differential channel between them. At first, I did consider a differential channel as you suggested in v2. However, the datasheet is quite clear that the LOS and DOS faults (and only those faults) come from a signal it calls the "monitor signal". This signal is defined as: Monitor = A1 * sin(theta) * sin(phi) + A2 * cos(theta) * cos(phi) where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the cosine input and phi is the position output. So mathematically speaking, there is no signal that is the difference between the two inputs. (See "Theory of Operation" section in the datasheet.) But if we want to hide these internal details and don't care about a strict definition of "differential", then what is suggested below seems fine. > > So this one becomes a double event (you need to signal it on both > cosine and sine channels). The DOS overange is similar. > The DOS mismatch is a threshold on the differential channel giving > > events/in_altvoltage0_thresh_falling_value > events/in_altvoltage1_thresh_falling_value (these match) > events/in_altvoltage0_thresh_rising_value > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > events/in_altvoltage1-altvoltage0_mag_rising_value > > Does that work here? Avoids smashing different types of signals together. > We could even do the LOT as differential between two angle channels > (tracking one and measured one) but meh that's getting complex.> > Note this will rely on channel labels to make the above make any sense at all. I think this could be OK - I think what matters most is having some documentation that maps the faults and registers on the chip to the iio names. Where would the sine/cosine clipping fault fit in though? I got a bit too creative and used X_OR_Y to differentiate it (see discussion in "staging: iio: resolver: ad2s1210: implement fault events"). Strictly speaking, it should probably be a type: threshold, direction: either event on both the sine and cosine input channels (another double event) since it occurs if either of the signal exceeds the power or ground rail voltage. But we already have threshold rising and threshold falling on these channels with a different meaning. I guess it could call it magnitude instead of a threshold?
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 5e99f692d4e32e3250ab18d511894ca797407aec]
url: https://github.com/intel-lab-lkp/linux/commits/David-Lechner/dt-bindings-iio-resolver-add-devicetree-bindings-for-ad2s1210/20230930-014031
base: 5e99f692d4e32e3250ab18d511894ca797407aec
patch link: https://lore.kernel.org/r/20230929-ad2s1210-mainline-v3-22-fa4364281745%40baylibre.com
patch subject: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS threshold to event attr
config: i386-randconfig-062-20231003 (https://download.01.org/0day-ci/archive/20231003/202310032242.jYDq0057-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231003/202310032242.jYDq0057-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310032242.jYDq0057-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
drivers/staging/iio/resolver/ad2s1210.c:896:1: sparse: sparse: symbol 'iio_const_attr_in_phase0_mag_value_available' was not declared. Should it be static?
>> drivers/staging/iio/resolver/ad2s1210.c:901:1: sparse: sparse: symbol 'iio_const_attr_in_altvoltage0_thresh_falling_value_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:902:1: sparse: sparse: symbol 'iio_dev_attr_in_angl1_thresh_rising_value_available' was not declared. Should it be static?
drivers/staging/iio/resolver/ad2s1210.c:903:1: sparse: sparse: symbol 'iio_dev_attr_in_angl1_thresh_rising_hysteresis_available' was not declared. Should it be static?
> -----Original Message----- > From: Jonathan Cameron <jic23@kernel.org> > Sent: Samstag, 30. September 2023 17:32 > To: David Lechner <dlechner@baylibre.com> > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > staging@lists.linux.dev; David Lechner <david@lechnology.com>; Rob Herring > <robh+dt@kernel.org>; Krzysztof Kozlowski > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno > <Nuno.Sa@analog.com>; Axel Haslam <ahaslam@baylibre.com>; Philip Molloy > <pmolloy@baylibre.com>; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS > threshold to event attr > > > On Fri, 29 Sep 2023 12:23:27 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > From: David Lechner <david@lechnology.com> > > > > From: David Lechner <dlechner@baylibre.com> > > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > > fault. This fault is triggered when either the sine or cosine input > > falls below the threshold voltage. > > > > This patch converts the custom device LOS threshold attribute to an > > event falling edge threshold attribute on a new monitor signal channel. > > The monitor signal is an internal signal that combines the amplitudes > > of the sine and cosine inputs as well as the current angle and > > position output. This signal is used to detect faults in the input signals. > > > > The attribute now uses millivolts instead of the raw register value in > > accordance with the IIO ABI. > > > > Emitting the event will be implemented in a later patch. > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > I think I'm fine with treating these internal signals like this, but I would ideally > like someone from Analog devices to take a look at how these are being done > and make sure our interpretations of the signals make sense to them. We are > pushing the boundaries a little here (though we have done similar before for > fault events I think.) Hi Jonathan, David and I we also had some internal discussion related to this. I'm sure these fault events and thresholds are understood correctly. Doing it this or the other way, it needs to be properly documented in order to make sense. So from my perspective whatever makes the most sense from a IIO ABI perspective, is the way to forward. -Michael > > Jonathan
On Wed, 4 Oct 2023 11:01:56 +0000 "Hennerich, Michael" <Michael.Hennerich@analog.com> wrote: > > -----Original Message----- > > From: Jonathan Cameron <jic23@kernel.org> > > Sent: Samstag, 30. September 2023 17:32 > > To: David Lechner <dlechner@baylibre.com> > > Cc: linux-iio@vger.kernel.org; devicetree@vger.kernel.org; linux- > > staging@lists.linux.dev; David Lechner <david@lechnology.com>; Rob Herring > > <robh+dt@kernel.org>; Krzysztof Kozlowski > > <krzysztof.kozlowski+dt@linaro.org>; Conor Dooley <conor+dt@kernel.org>; > > Hennerich, Michael <Michael.Hennerich@analog.com>; Sa, Nuno > > <Nuno.Sa@analog.com>; Axel Haslam <ahaslam@baylibre.com>; Philip Molloy > > <pmolloy@baylibre.com>; linux-kernel@vger.kernel.org > > Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS > > threshold to event attr > > > > > > On Fri, 29 Sep 2023 12:23:27 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > From: David Lechner <david@lechnology.com> > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > > > fault. This fault is triggered when either the sine or cosine input > > > falls below the threshold voltage. > > > > > > This patch converts the custom device LOS threshold attribute to an > > > event falling edge threshold attribute on a new monitor signal channel. > > > The monitor signal is an internal signal that combines the amplitudes > > > of the sine and cosine inputs as well as the current angle and > > > position output. This signal is used to detect faults in the input signals. > > > > > > The attribute now uses millivolts instead of the raw register value in > > > accordance with the IIO ABI. > > > > > > Emitting the event will be implemented in a later patch. > > > > > > Signed-off-by: David Lechner <dlechner@baylibre.com> > > > > I think I'm fine with treating these internal signals like this, but I would ideally > > like someone from Analog devices to take a look at how these are being done > > and make sure our interpretations of the signals make sense to them. We are > > pushing the boundaries a little here (though we have done similar before for > > fault events I think.) > > Hi Jonathan, > David and I we also had some internal discussion related to this. > I'm sure these fault events and thresholds are understood correctly. > Doing it this or the other way, it needs to be properly documented in order to make sense. > So from my perspective whatever makes the most sense from a IIO ABI > perspective, is the way to forward. Great - as long as keep to a logical mapping I quite like the events approach. Most of these faults are real thresholds on things being measured (even if those 'things' are signals from which stuff is derived for the main measurements the device is making.) Jonathan > > -Michael > > > > > Jonathan >
On Mon, 2 Oct 2023 11:09:11 -0500 David Lechner <dlechner@baylibre.com> wrote: > On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Fri, 29 Sep 2023 12:23:27 -0500 > > David Lechner <dlechner@baylibre.com> wrote: > > > > > From: David Lechner <david@lechnology.com> > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > > > fault. This fault is triggered when either the sine or cosine input > > > falls below the threshold voltage. > > > > > > This patch converts the custom device LOS threshold attribute to an > > > event falling edge threshold attribute on a new monitor signal channel. > > > The monitor signal is an internal signal that combines the amplitudes > > > of the sine and cosine inputs as well as the current angle and position > > > output. This signal is used to detect faults in the input signals. > > > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > > error conditions onto one channel. Fundamentally we have > > sine and cosine inputs. I think we should treat those as separate channels > > and include a third differential channel between them. > > At first, I did consider a differential channel as you suggested in > v2. However, the datasheet is quite clear that the LOS and DOS faults > (and only those faults) come from a signal it calls the "monitor > signal". This signal is defined as: > > Monitor = A1 * sin(theta) * sin(phi) + A2 * cos(theta) * cos(phi) > > where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the > cosine input and phi is the position output. So mathematically > speaking, there is no signal that is the difference between the two > inputs. (See "Theory of Operation" section in the datasheet.) Hmm. That's certainly a bit more complex than I expected. Relying on the brief description led me astray. It's related to the differences in the measured and as if theta == phi and A1 == A2 (ideal) then it will be A1. I can see it's relevant to DOS, but not LOS. The description of LOS seems to overlap a number of different things unfortunately. > > But if we want to hide these internal details and don't care about a > strict definition of "differential", then what is suggested below > seems fine. Probably best to introduce that monitor signal though we'll have to be a bit vague about what it is which has the side effect that anyone trying to understand what on earth these faults are is going to be confused (having read the datasheet section a couple of times I'm not 100% sure...) > > > > > So this one becomes a double event (you need to signal it on both > > cosine and sine channels). The DOS overange is similar. > > The DOS mismatch is a threshold on the differential channel giving > > > > events/in_altvoltage0_thresh_falling_value > > events/in_altvoltage1_thresh_falling_value (these match) > > events/in_altvoltage0_thresh_rising_value > > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > > events/in_altvoltage1-altvoltage0_mag_rising_value > > > > Does that work here? Avoids smashing different types of signals together. > > We could even do the LOT as differential between two angle channels > > (tracking one and measured one) but meh that's getting complex.> > > Note this will rely on channel labels to make the above make any sense at all. > > I think this could be OK - I think what matters most is having some > documentation that maps the faults and registers on the chip to the > iio names. Where would the sine/cosine clipping fault fit in though? I > got a bit too creative and used X_OR_Y to differentiate it (see > discussion in "staging: iio: resolver: ad2s1210: implement fault > events"). Strictly speaking, it should probably be a type: threshold, > direction: either event on both the sine and cosine input channels > (another double event) since it occurs if either of the signal exceeds > the power or ground rail voltage. But we already have threshold rising > and threshold falling on these channels with a different meaning. I > guess it could call it magnitude instead of a threshold? Tricky indeed. Though I guess we only hit the clipping case after LOS or DOS fires or if their thresholds are set too wide (is that even possible?). So it is useful to report it as we are already in error? Or can we combine the cases by treating it as a cap on the threshold controls for LOS and DOS? Even when they aren't just there for error reporting, designers seem to always come up with new create signals to use for event detection and sometimes it's a real struggle to map them to something general. Jonathan
On Thu, Oct 5, 2023 at 9:37 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Mon, 2 Oct 2023 11:09:11 -0500 > David Lechner <dlechner@baylibre.com> wrote: > > > On Sat, Sep 30, 2023 at 10:42 AM Jonathan Cameron <jic23@kernel.org> wrote: > > > > > > On Fri, 29 Sep 2023 12:23:27 -0500 > > > David Lechner <dlechner@baylibre.com> wrote: > > > > > > > From: David Lechner <david@lechnology.com> > > > > > > > > From: David Lechner <dlechner@baylibre.com> > > > > > > > > The AD2S1210 has a programmable threshold for the loss of signal (LOS) > > > > fault. This fault is triggered when either the sine or cosine input > > > > falls below the threshold voltage. > > > > > > > > This patch converts the custom device LOS threshold attribute to an > > > > event falling edge threshold attribute on a new monitor signal channel. > > > > The monitor signal is an internal signal that combines the amplitudes > > > > of the sine and cosine inputs as well as the current angle and position > > > > output. This signal is used to detect faults in the input signals. > > > > > > Hmm. Looking forwards, I'm less sure that we should be shoving all these > > > error conditions onto one channel. Fundamentally we have > > > sine and cosine inputs. I think we should treat those as separate channels > > > and include a third differential channel between them. > > > > At first, I did consider a differential channel as you suggested in > > v2. However, the datasheet is quite clear that the LOS and DOS faults > > (and only those faults) come from a signal it calls the "monitor > > signal". This signal is defined as: > > > > Monitor = A1 * sin(theta) * sin(phi) + A2 * cos(theta) * cos(phi) > > > > where A1 * sin(theta) is the the sine input, A2 * cos(theta) is the > > cosine input and phi is the position output. So mathematically > > speaking, there is no signal that is the difference between the two > > inputs. (See "Theory of Operation" section in the datasheet.) > > Hmm. That's certainly a bit more complex than I expected. > Relying on the brief description led me astray. > > It's related to the differences in the measured and as if > theta == phi and A1 == A2 (ideal) then it will be A1. > > I can see it's relevant to DOS, but not LOS. The description of LOS > seems to overlap a number of different things unfortunately. > One thing to watch out for in the datasheet is the difference between the fault output pins and the fault bits read over the bus. The LOS output pin does indicate one or more of multiple faults, but we are not currently using that. We are only looking at the fault bits which are more granular. > > > > > > But if we want to hide these internal details and don't care about a > > strict definition of "differential", then what is suggested below > > seems fine. > > Probably best to introduce that monitor signal though we'll have > to be a bit vague about what it is which has the side effect that > anyone trying to understand what on earth these faults are is going > to be confused (having read the datasheet section a couple of times > I'm not 100% sure...) > > > > > > > > > So this one becomes a double event (you need to signal it on both > > > cosine and sine channels). The DOS overange is similar. > > > The DOS mismatch is a threshold on the differential channel giving > > > > > > events/in_altvoltage0_thresh_falling_value > > > events/in_altvoltage1_thresh_falling_value (these match) > > > events/in_altvoltage0_thresh_rising_value > > > events/in_altvoltage1_thresh_rising_value (matches previous which is fine) > > > events/in_altvoltage1-altvoltage0_mag_rising_value > > > > > > Does that work here? Avoids smashing different types of signals together. > > > We could even do the LOT as differential between two angle channels > > > (tracking one and measured one) but meh that's getting complex.> > > > Note this will rely on channel labels to make the above make any sense at all. > > > > I think this could be OK - I think what matters most is having some > > documentation that maps the faults and registers on the chip to the > > iio names. Where would the sine/cosine clipping fault fit in though? I > > got a bit too creative and used X_OR_Y to differentiate it (see > > discussion in "staging: iio: resolver: ad2s1210: implement fault > > events"). Strictly speaking, it should probably be a type: threshold, > > direction: either event on both the sine and cosine input channels > > (another double event) since it occurs if either of the signal exceeds > > the power or ground rail voltage. But we already have threshold rising > > and threshold falling on these channels with a different meaning. I > > guess it could call it magnitude instead of a threshold? > > Tricky indeed. Though I guess we only hit the clipping case after > LOS or DOS fires or if their thresholds are set too wide (is that > even possible?). I suppose it _could_ be possible on the high side if the AVDD voltage supply was selected to be less than the 4.4V max of the threshold voltage registers. > So it is useful to report it as we are already in > error? Or can we combine the cases by treating it as a cap on the > threshold controls for LOS and DOS? I found the clipping error useful while developing this driver since it help identify that we had a gain setting wrong on the excitation output (on the circuit board) which in turn caused the inputs to be overdriven. But, yes when this happened, it also always triggered at least one or more of the LOS and DOS faults as well. > > Even when they aren't just there for error reporting, designers > seem to always come up with new create signals to use for event > detection and sometimes it's a real struggle to map them to > something general. > > Jonathan > >
diff --git a/drivers/staging/iio/resolver/ad2s1210.c b/drivers/staging/iio/resolver/ad2s1210.c index 5cc8106800d6..7abbc184c351 100644 --- a/drivers/staging/iio/resolver/ad2s1210.c +++ b/drivers/staging/iio/resolver/ad2s1210.c @@ -66,6 +66,11 @@ #define PHASE_360_DEG_TO_RAD_INT 6 #define PHASE_360_DEG_TO_RAD_MICRO 283185 +/* Threshold voltage registers have 1 LSB == 38 mV */ +#define THRESHOLD_MILLIVOLT_PER_LSB 38 +/* max voltage for threshold registers is 0x7F * 38 mV */ +#define THRESHOLD_RANGE_STR "[0 38 4826]" + enum ad2s1210_mode { MOD_POS = 0b00, MOD_VEL = 0b01, @@ -448,6 +453,38 @@ static const int ad2s1210_lot_threshold_urad_per_lsb[] = { 1237, /* 16-bit: same as 14-bit */ }; +static int ad2s1210_get_voltage_threshold(struct ad2s1210_state *st, + unsigned int reg, int *val) +{ + unsigned int reg_val; + int ret; + + mutex_lock(&st->lock); + ret = regmap_read(st->regmap, reg, ®_val); + mutex_unlock(&st->lock); + + if (ret < 0) + return ret; + + *val = reg_val * THRESHOLD_MILLIVOLT_PER_LSB; + return IIO_VAL_INT; +} + +static int ad2s1210_set_voltage_threshold(struct ad2s1210_state *st, + unsigned int reg, int val) +{ + unsigned int reg_val; + int ret; + + reg_val = val / THRESHOLD_MILLIVOLT_PER_LSB; + + mutex_lock(&st->lock); + ret = regmap_write(st->regmap, reg, reg_val); + mutex_unlock(&st->lock); + + return ret; +} + static int ad2s1210_get_lot_high_threshold(struct ad2s1210_state *st, int *val, int *val2) { @@ -706,9 +743,6 @@ static int ad2s1210_write_raw(struct iio_dev *indio_dev, static IIO_DEVICE_ATTR(fault, 0644, ad2s1210_show_fault, ad2s1210_clear_fault, 0); -static IIO_DEVICE_ATTR(los_thrd, 0644, - ad2s1210_show_reg, ad2s1210_store_reg, - AD2S1210_REG_LOS_THRD); static IIO_DEVICE_ATTR(dos_ovr_thrd, 0644, ad2s1210_show_reg, ad2s1210_store_reg, AD2S1210_REG_DOS_OVR_THRD); @@ -745,6 +779,16 @@ static const struct iio_event_spec ad2s1210_phase_event_spec[] = { }, }; +static const struct iio_event_spec ad2s1210_monitor_signal_event_spec[] = { + { + /* Sine/cosine below LOS threshold fault. */ + .type = IIO_EV_TYPE_THRESH, + .dir = IIO_EV_DIR_FALLING, + /* Loss of signal threshold. */ + .mask_separate = BIT(IIO_EV_INFO_VALUE), + }, +}; + static const struct iio_chan_spec ad2s1210_channels[] = { { .type = IIO_ANGL, @@ -803,12 +847,19 @@ static const struct iio_chan_spec ad2s1210_channels[] = { .scan_index = -1, .info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY), .info_mask_separate_available = BIT(IIO_CHAN_INFO_FREQUENCY), + }, { + /* monitor signal */ + .type = IIO_ALTVOLTAGE, + .indexed = 1, + .channel = 0, + .scan_index = -1, + .event_spec = ad2s1210_monitor_signal_event_spec, + .num_event_specs = ARRAY_SIZE(ad2s1210_monitor_signal_event_spec), }, }; static struct attribute *ad2s1210_attributes[] = { &iio_dev_attr_fault.dev_attr.attr, - &iio_dev_attr_los_thrd.dev_attr.attr, &iio_dev_attr_dos_ovr_thrd.dev_attr.attr, &iio_dev_attr_dos_mis_thrd.dev_attr.attr, &iio_dev_attr_dos_rst_max_thrd.dev_attr.attr, @@ -847,11 +898,13 @@ IIO_CONST_ATTR(in_phase0_mag_value_available, __stringify(PHASE_44_DEG_TO_RAD_MICRO) " " __stringify(PHASE_360_DEG_TO_RAD_INT) "." __stringify(PHASE_360_DEG_TO_RAD_MICRO)); +IIO_CONST_ATTR(in_altvoltage0_thresh_falling_value_available, THRESHOLD_RANGE_STR); IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_value_available, 0); IIO_DEVICE_ATTR_RO(in_angl1_thresh_rising_hysteresis_available, 0); static struct attribute *ad2s1210_event_attributes[] = { &iio_const_attr_in_phase0_mag_value_available.dev_attr.attr, + &iio_const_attr_in_altvoltage0_thresh_falling_value_available.dev_attr.attr, &iio_dev_attr_in_angl1_thresh_rising_value_available.dev_attr.attr, &iio_dev_attr_in_angl1_thresh_rising_hysteresis_available.dev_attr.attr, NULL, @@ -904,6 +957,13 @@ static int ad2s1210_read_event_value(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_ALTVOLTAGE: + if (chan->output) + return -EINVAL; + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING) + return ad2s1210_get_voltage_threshold(st, + AD2S1210_REG_LOS_THRD, val); + return -EINVAL; case IIO_PHASE: return ad2s1210_get_phase_lock_range(st, val, val2); default: @@ -930,6 +990,13 @@ static int ad2s1210_write_event_value(struct iio_dev *indio_dev, default: return -EINVAL; } + case IIO_ALTVOLTAGE: + if (chan->output) + return -EINVAL; + if (type == IIO_EV_TYPE_THRESH && dir == IIO_EV_DIR_FALLING) + return ad2s1210_set_voltage_threshold(st, + AD2S1210_REG_LOS_THRD, val); + return -EINVAL; case IIO_PHASE: return ad2s1210_set_phase_lock_range(st, val, val2); default: