Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
References: <cover.1701168726.git.cmo@melexis.com> <c9db99819adb0cdd602394b27f97a3b8fe081148.1701168726.git.cmo@melexis.com>
 <20231204142224.51f2ccdf@jic23-huawei>
In-Reply-To: <20231204142224.51f2ccdf@jic23-huawei>
From:   Crt Mori <cmo@melexis.com>
Date:   Mon, 4 Dec 2023 16:34:30 +0100
Message-ID: <CAKv63usxdfOviH=M6iUiNTtBFZVOseWUGz63Q-oJniBDFvTpSQ@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] iio: temperature: mlx90635 MLX90635 IR Temperature sensor
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Lars-Peter Clausen <lars@metafoo.de>,
        Andrew Hepp <andrew.hepp@ahepp.dev>, linux-iio@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, 4 Dec 2023 at 15:22, Jonathan Cameron <jic23@kernel.org> wrote:
>
...
> > switches to Continuous power mode where measurements constantly change
> > without triggering.
> >
> > Signed-off-by: Crt Mori<cmo@melexis.com>
>
> Hi Crt,
>
> I don't understand some of the regcache_cache_only() manipulation in here.
> If I understand the aim correctly it is to allow us to write settings whilst
> powered down (in sleep_step) that will then by synced to the device when it enters
> continuous mode?
>
> If so, I'd expect to only see manipulation of whether the caching is or or
> not at places where we transition state.  You currently have them in various
> other place. In some cases I scan see it's to allow a temporary change of
> state, but it's not obvious.  So perhaps a comment ever time you manually
> tweak whether writes hit the device or just stick in the regacache.
> That comment can explain why each of them is needed.
>
> A few other comments inline,
>
> Thanks,
>
> Jonathan
>

While in Sleep Step mode, the EEPROM is powered down, but the cache
buffers those values. Still when you try to write or read a volatile
register (which should not be prevented by cache enabled as per my
opinion, but code says differently) in that mode, it returns -EBUSY
(as we discovered by code), so this kind of manipulation is needed to
enable write and read operations from volatile registers. And you need
to trigger the measurement (burst mode) in that state, but since you
cannot read EEPROM, yet still need its values to calculate the final
temperature, the cache is used for this case. There is nothing to
re-cache when we get back as all registers I read/write to are marked
as volatile, so they would not be cached anyway.

Thanks for the review - I still have some questions below (and explanation,
but not sure where to put those).

> > diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
...
> > + * @lock: Internal mutex for multiple reads for single measurement
>
> Multiple reads shouldn't be a problem, unless someone else can do something
> destructive in between.  Perhaps a little more detail on why multiple reads matter?
>

You trigger device to perform measurement in Sleep Step mode, so to
ensure both object and ambient temperature reads are from the same
triggered measurement, the mutex needs to be held. If for example in
between you would retrigger the measurement, then you would operate on
"invalid" data (shouldn't differ much, but I wanted to prevent that as
it might be 0).

> > + * @regmap: Regmap of the device
> > + * @emissivity: Object emissivity from 0 to 1000 where 1000 = 1.
> > + * @regulator: Regulator of the device
> > + * @powerstatus: Current POWER status of the device
> > + * @interaction_ts: Timestamp of the last temperature read that is used
> > + *               for power management in jiffies
> > + */
...
> > +     mutex_lock(&data->lock);
> > +     if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP) {
> > +             regcache_cache_only(data->regmap, false);
> > +             ret = mlx90635_perform_measurement_burst(data);
>
> Why is a burst needed here?  Perhaps a comment?
>

Burst is from 90632 terminology (and our chip register map), but maybe
more general would be "trigger_measurement"?

> > +static int mlx90635_get_refresh_rate(struct mlx90635_data *data,
> > +                                  unsigned int *refresh_rate)
> > +{
> > +     unsigned int reg;
> > +     int ret;
> > +
> > +     if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP)
> > +             regcache_cache_only(data->regmap, false);
>
> Definitely needs a comment on why this is needed in this case.
>

Here and below (where we turn it back to true?), but then I assume in
all other instances as well? Maybe a more general comment in the
sleep_step mode function?

> > +
> > +     ret = regmap_read(data->regmap, MLX90635_REG_CTRL1, &reg);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP)
> > +             regcache_cache_only(data->regmap, true);
> > +
> > +     *refresh_rate = FIELD_GET(MLX90635_CTRL1_REFRESH_RATE_MASK, reg);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct {
> > +     int val;
> > +     int val2;
> > +} mlx90635_freqs[] = {
> > +     {0, 200000},
> Prefer spaces after { and before }

ok.

> > +     {0, 500000},
> > +     {0, 900000},
> > +     {1, 700000},
> > +     {3, 0},
> > +     {4, 800000},
> > +     {6, 900000},
> > +     {8, 900000}
> > +};
...
> > +             if (i == ARRAY_SIZE(mlx90635_freqs))
> > +                     return -EINVAL;
> > +
> > +             if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP)
> > +                     regcache_cache_only(data->regmap, false);
>
> So here you want the rate to get through even though we otherwise have the
> device powered down?  Is that because some registers are safe for writes
> and not others?  If so you may need some locking to stop a race where you
> turn on writes here and someone else writes.
>

Yes, exactly the case. Read/Write into registers (REG_) is possible in
all modes, but read of EEPROM is not (to save power the EEPROM is
turned off). I do not see how write race would get us into trouble
here since it is only 1, and as long as chip powerstatus is not
changed we should end up in correct state. I can wrap a mutex around
though.


> > +
> > +             ret = regmap_write_bits(data->regmap, MLX90635_REG_CTRL1,
> > +                                     MLX90635_CTRL1_REFRESH_RATE_MASK, i);
> > +
> > +             if (data->powerstatus == MLX90635_PWR_STATUS_SLEEP_STEP)
> > +                     regcache_cache_only(data->regmap, true);
> > +             return ret;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +}
>