Re: [PATCH v3 8/8] iio: tcs3472: implement wait time and sampling frequency

[Jonathan Cameron <jic23@kernel.org>]

> 
> Hi Jonathan,
Hi Aldo,

Please crop away anything you aren't responding to to help us
focus on the important bits.

> 
> Thanks for the review. I'd like to ask a design
> question about Sashiko's finding on the cached data->enable.
> 
> Sashiko pointed out:
> 
>  > @@ -434,16 +611,15 @@ static const struct iio_info tcs3472_info = {
>  >  static int tcs3472_powerdown(struct tcs3472_data *data)
>  >  {
>  >  	int ret;
>  > -	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  >
>  >  	guard(mutex)(&data->lock);
>  >
>  >  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  > -					data->enable & ~enable_mask);
>  > +					data->enable & ~TCS3472_ENABLE_RUN);
>  >  	if (ret)
>  >  		return ret;
>  >
>  > -	data->enable &= ~enable_mask;
>  > +	data->enable &= ~TCS3472_ENABLE_RUN;  
> Does this permanently lose the WEN configuration after a suspend/resume cycle?
> Because TCS3472_ENABLE_WEN is part of the TCS3472_ENABLE_RUN mask, this clears
> the WEN bit in the cached data->enable. Upon resume, tcs3472_resume() attempts
> to restore the configuration by reading this cache:
>      value = data->enable | TCS3472_ENABLE_PON | TCS3472_ENABLE_AEN;
> But since the WEN bit was permanently cleared in the cache during suspend, the
> device always resumes with WEN disabled, discarding the user's sampling
> frequency configuration.
> 
> 
> 
> 
> I see two possible fixes:
> 
> 1.  In tcs3472_powerdown(), clear only PON and AEN from data->enable
>      (keeping WEN as the "user's last desired state"). This is minimal
>      but introduces a divergence between data->enable and the actual
>      hardware ENABLE register, which breaks the simple "cache of the
>      HW register" semantic.

Suspend / resume is one place where it's common to not cache the 'current'
value rather than the value we expect after coming back via resume().
If you do go this way that field should not be updated at all in suspend
because we will need it's full value in resume.

Sometimes people have a second cache for just this case though. Maybe
that works here. It would I think be the whole register rather than
just the flag you have in option 2.


> 2.  Add a separate field (e.g. bool target_wen) that holds the user's
>      desired WEN state, set in tcs3472_set_sampling_freq() and read
>      back in tcs3472_resume(). data->enable then strictly remains a
>      pure cache of the HW register.
> 
> 
> With solution2, the functions would behave as follows:
> 
>    - tcs3472_set_sampling_freq(): when activating wait state, sets
>      target_wen = true; when disabling it (frequency too high), sets
>      target_wen = false. data->enable is then updated to match what
>      is written to the chip.
> 
>    - tcs3472_cycle_time_us(): reads target_wen instead of
>      (data->enable & WEN) to decide whether to include wtime_us in
>      the cycle. This avoids reporting a stale wait time during a
>      suspended state.
> 
>    - tcs3472_powerdown(): unchanged in semantics clears PON, AEN
>      and WEN from both the chip and data->enable. target_wen is left
>      untouched, since it represents the user's wish, not the HW
>      state.
> 
>    - tcs3472_resume(): rebuilds the ENABLE value from
>      PON | AEN | (target_wen ? WEN : 0), writes it to the chip, then
>      updates data->enable to match.
> 
>    - tcs3472_probe(): initializes target_wen from the chip's current
>      WEN bit (read at probe time).
> 
> Solution 2 feels cleaner to me, but it adds a field and a bit of
> synchronization between target_wen and (data->enable & WEN). Do you
> have another approach in mind?

As long as it ends up easy to follow I'm not that fussed what solution
you use.  Both of these will work.  It does slightly feel like caching
the value we want on resume for the whole register might be the simplest
path but I haven't tried coding it up so may be missing something.

thanks,

Jonathan

> 
> Thanks,
> Aldo
> 
> 
> 
>