| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230930164656.144f6717@jic23-huawei>
Date: Sat, 30 Sep 2023 16:46:56 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
linux-staging@...ts.linux.dev,
David Lechner <david@...hnology.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Michael Hennerich <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
Axel Haslam <ahaslam@...libre.com>,
Philip Molloy <pmolloy@...libre.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 22/27] staging: iio: resolver: ad2s1210: convert LOS
threshold to event attr
On Sat, 30 Sep 2023 16:42:51 +0100
Jonathan Cameron <jic23@...nel.org> wrote:
> On Fri, 29 Sep 2023 12:23:27 -0500
> David Lechner <dlechner@...libre.com> wrote:
>
> > From: David Lechner <david@...hnology.com>
> >
> > From: David Lechner <dlechner@...libre.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@...libre.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:
> >
>
Powered by blists - more mailing lists