| 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: <CANk7y0iDOCop1j=kL=nwWLf5UjDuy_dc9PjZ3tVgD9b6UimU9A@mail.gmail.com>
Date: Thu, 19 Aug 2021 22:47:50 +0530
From: Puranjay Mohan <puranjay12@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: "Hennerich, Michael" <Michael.Hennerich@...log.com>,
devicetree <devicetree@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
"Bogdan, Dragos" <Dragos.Bogdan@...log.com>,
"Berghe, Darius" <Darius.Berghe@...log.com>,
Andy Shevchenko <andy.shevchenko@...il.com>,
Alexandru Ardelean <ardeleanalex@...il.com>
Subject: Re: [PATCH v12 2/2] iio: accel: Add driver support for ADXL355
On Wed, Aug 18, 2021 at 2:38 PM Jonathan Cameron <jic23@...nel.org> wrote:
>
> On Sun, 15 Aug 2021 21:59:42 +0530
> Puranjay Mohan <puranjay12@...il.com> wrote:
>
> > On Sun, Aug 15, 2021 at 9:14 PM Jonathan Cameron <jic23@...nel.org> wrote:
> > >
> > > On Wed, 11 Aug 2021 13:00:27 +0530
> > > Puranjay Mohan <puranjay12@...il.com> wrote:
> > >
> > > > ADXL355 is a 3-axis MEMS Accelerometer. It offers low noise density,
> > > > low 0g offset drift, low power with selectable measurement ranges.
> > > > It also features programmable high-pass and low-pass filters.
> > > >
> > > > Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/adxl354_adxl355.pdf
> > > > Reviewed-by: Alexandru Ardelean <ardeleanalex@...il.com>
> > > > Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> > > > Signed-off-by: Puranjay Mohan <puranjay12@...il.com>
> > > Hi Puranjay,
> > >
> > > I took one last look at this so I can apply it without looking again assuming
> > > the dt review is fine. Noticed one issue with error handling, but I can tidy
> > > that up whilst applying assuming you aren't doing a v13 for some other reason.
> > > If you are please incorporate these changes as well.
> > >
> >
> > Hi Jonathan, It would be great if you could make these changes while
> > applying. I am not doing a v13 as all comments have been covered
> > earlier.
> > I shall be thankful to you.
> I ended up tweaking those 3 functions a little more than I was originally planning
> so please check it all looks good to you.
>
> Series applied to the togreg branch of iio.git and pushed out as testing
> for 0-day to poke at. Note these won't hit mainline until 5.16 now.
>
> Thanks,
>
> Jonathan
>
Hi Jonathan,
the changes you made look good to me.
Thanks,
> >
> > > Thanks,
> > >
> > > Jonathan
> > >
> > >
> > > ...
> > >
> > > > +
> > > > +static int adxl355_set_odr(struct adxl355_data *data,
> > > > + enum adxl355_odr odr)
> > > > +{
> > > > + int ret = 0;
> > > > +
> > > > + mutex_lock(&data->lock);
> > > > +
> > > > + if (data->odr == odr)
> > > > + goto out_unlock;
> > > > +
> > > > + ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> > > > + if (ret < 0)
> > > > + goto out_unlock;
> > > > +
> > > > + ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
> > > > + ADXL355_FILTER_ODR_MSK,
> > > > + FIELD_PREP(ADXL355_FILTER_ODR_MSK, odr));
> > > > + if (ret < 0)
> > > > + goto out_unlock;
> > > > +
> > > > + data->odr = odr;
> > > > + adxl355_fill_3db_frequency_table(data);
> > > > +
> > > > +out_unlock:
> > > > + ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > >
> > > As below, we should do this because it risks returning success when a failure
> > > actually occured. Again, unless you are respinning for some other reason I'll
> > > add the logic whilst applying.
> > >
> > > > + mutex_unlock(&data->lock);
> > > > + return ret;
> > > > +}
> > > > +
> > > > +static int adxl355_set_hpf_3db(struct adxl355_data *data,
> > > > + enum adxl355_hpf_3db hpf)
> > > > +{
> > > > + int ret = 0;
> > > > +
> > > > + mutex_lock(&data->lock);
> > > > +
> > > > + if (data->hpf_3db == hpf)
> > > > + goto unlock;
> > > > +
> > > > + ret = adxl355_set_op_mode(data, ADXL355_STANDBY);
> > > > + if (ret < 0)
> > > > + goto set_opmode_unlock;
> > > > +
> > > > + ret = regmap_update_bits(data->regmap, ADXL355_FILTER_REG,
> > > > + ADXL355_FILTER_HPF_MSK,
> > > > + FIELD_PREP(ADXL355_FILTER_HPF_MSK, hpf));
> > > > + if (!ret)
> > > > + data->hpf_3db = hpf;
> > > > +
> > > > +set_opmode_unlock:
> > > > + ret = adxl355_set_op_mode(data, ADXL355_MEASUREMENT);
> > >
> > > We can't do this as it might potentially eat an error that meant the regmap
> > > update didn't occur. To avoid that a little dance is needed using a second
> > > return value and we only set ret = ret2 if ret == 0
> > >
> > > Alternatively we just have a separate error handling path which doesn't set
> > > ret for the adxl355_set_op_mode(). I'll probably go with that as it's more
> > > code but easier to read.
> > >
> > >
> > >
> > > > +unlock:
> > > > + mutex_unlock(&data->lock);
> > > > + return ret;
> > > > +}
> > > > +
> > >
> > > ...
> > >
> > > > +static int adxl355_write_raw(struct iio_dev *indio_dev,
> > > > + struct iio_chan_spec const *chan,
> > > > + int val, int val2, long mask)
> > > > +{
> > > > + struct adxl355_data *data = iio_priv(indio_dev);
> > > > + int odr_idx, hpf_idx, calibbias;
> > > > +
> > > > + switch (mask) {
> > > > + case IIO_CHAN_INFO_SAMP_FREQ:
> > > > + odr_idx = adxl355_find_match(adxl355_odr_table,
> > > > + ARRAY_SIZE(adxl355_odr_table),
> > > > + val, val2);
> > > > + if (odr_idx < 0)
> > > > + return odr_idx;
> > > > +
> > > > + return adxl355_set_odr(data, odr_idx);
> > > > + case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> > > > + hpf_idx = adxl355_find_match(data->adxl355_hpf_3db_table,
> > > > + ARRAY_SIZE(data->adxl355_hpf_3db_table),
> > >
> > > Mixing different indentation styles isn't very nice for readability.
> > > I'll tweak this whilst applying.
> > >
> > > > + val, val2);
> > > > + if (hpf_idx < 0)
> > > > + return hpf_idx;
> > > > +
> > > > + return adxl355_set_hpf_3db(data, hpf_idx);
> > > > + case IIO_CHAN_INFO_CALIBBIAS:
> > > > + calibbias = clamp_t(int, val, S16_MIN, S16_MAX);
> > > > +
> > > > + return adxl355_set_calibbias(data, chan->address, calibbias);
> > > > + default:
> > > > + return -EINVAL;
> > > > + }
> > > > +}
> > > ...
> >
> >
> >
>
--
Thanks and Regards
Yours Truly,
Puranjay Mohan
Powered by blists - more mailing lists