Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <f5af3894-c76e-04b6-830a-2687427991b6@gmail.com>
Date:   Sat, 14 May 2022 16:32:25 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH v3 2/2] iio: adc: ad4130: add AD4130 driver
Content-Language: en-US
To:     Jonathan Cameron <jic23@kernel.org>
Cc:     Rob Herring <robh+dt@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        linux-iio@vger.kernel.org, linux-gpio@vger.kernel.org,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        Cosmin Tanislav <cosmin.tanislav@analog.com>
References: <20220419150828.191933-1-cosmin.tanislav@analog.com>
 <20220419150828.191933-3-cosmin.tanislav@analog.com>
 <20220501170807.1e728524@jic23-huawei>
 <5d932a4a-790e-ca95-c5de-c2267e1f365c@gmail.com>
 <20220507173551.1bc45a82@jic23-huawei>
 <39cb9ade-14af-c53b-bd42-06a9b965b57f@gmail.com>
 <20220507180210.4c48a85f@jic23-huawei>
From:   Cosmin Tanislav <demonsingur@gmail.com>
In-Reply-To: <20220507180210.4c48a85f@jic23-huawei>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 5/7/22 20:04, Jonathan Cameron wrote:
> On Sat, 7 May 2022 19:49:17 +0300
> Cosmin Tanislav <demonsingur@gmail.com> wrote:
> 
>> On 5/7/22 19:35, Jonathan Cameron wrote:
>>>    
>>>>>       
>>>>>> +static int ad4130_set_fifo_watermark(struct iio_dev *indio_dev, unsigned int val)
>>>>>> +{
>>>>>> +	struct ad4130_state *st = iio_priv(indio_dev);
>>>>>> +	unsigned int eff;
>>>>>> +	int ret;
>>>>>> +
>>>>>> +	if (val > AD4130_FIFO_SIZE)
>>>>>> +		return -EINVAL;
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * Always set watermark to a multiple of the number of enabled channels
>>>>>> +	 * to avoid making the FIFO unaligned.
>>>>>> +	 */
>>>>>> +	eff = rounddown(val, st->num_enabled_channels);
>>>>>> +
>>>>>> +	mutex_lock(&st->lock);
>>>>>> +
>>>>>> +	ret = regmap_update_bits(st->regmap, AD4130_REG_FIFO_CONTROL,
>>>>>> +				 AD4130_WATERMARK_MASK,
>>>>>> +				 FIELD_PREP(AD4130_WATERMARK_MASK,
>>>>>> +					    ad4130_watermark_reg_val(eff)));
>>>>>> +	if (ret)
>>>>>> +		goto out;
>>>>>> +
>>>>>> +	st->effective_watermark = eff;
>>>>>> +	st->watermark = val;
>>>>>
>>>>> Hmm this is a potential inconsistency in the IIO ABI.
>>>>> ABI docs describes watermark as being number of 'scan elements' which is
>>>>> not the clearest text we could have gone with...
>>>>>
>>>>> Now I may well have made a mistake in the following as it's rather a long time
>>>>> since I last looked at the core handling for this...
>>>>>
>>>>> The core treats it as number datum (which is same as a scan) when using
>>>>> it for the main watermark attribute and also when using watermarks with the
>>>>> kfifo (the IIO fifo is made up of objects each of which is a scan. So kfifo_len()
>>>>> returns the number of scans.
>>>>>     
>>>>> Looking very quickly at a few other drivers
>>>>> adxl367 seems to use number of samples.
>>>>> adxl372 is using number of scans.
>>>>> bmc150 hardware seems to work on basis of frame count which I 'think' is probably scans.
>>>>> fxls8962 uses 'samples count' which is not clearly defined in the datasheet but there
>>>>> is an example showing that it's scans (I think)...
>>>>> lsm6dsx - some of the fifos used with this are based on tagged data so the connection to
>>>>> what hits the front end buffers is non obvious.
>>>>>
>>>>> So, not great for consistency :(
>>>>>
>>>>> Going forwards i think we should standardize the hardware fifo watermark on what is being
>>>>> used for the software watermark which I believe is number of scans.
>>>>> Not necessary much we can do about old drivers though due to risk of breaking ABI...
>>>>> We should make the documentation clearer though.
>>>>>       
>>>>
>>>> I was confused too, but this seemed more logical to me at the time, and
>>>> since you didn't say anything regarding it on ADXL367, I did it the same
>>>> way here. I guess we can't go back and change it now on ADXL367, I'm
>>>> sorry for this. I'll fix it.
>>>
>>> I missed it.  Review is never perfect (mine definitely aren't!)
>>>
>>> Thinking more on the adxl367. We still have a window to  fix that as
>>> the driver isn't yet in a release kernel.  Would you mind spinning a
>>> patch to fix that one?  Even if we miss the rc cycle (it's a bit tight
>>> timing wise) we can sneak it in as an early fix in stable without
>>> significant risk of breaking anyone's userspace.
>>>    
>>
>> I hope Monday is not too late to do it?
> 
> Any time next week should be fine.  If it ends up later that's fine as well.
> We have at least a few weeks until the 5.18 release and then if we were to land
> this during the first few weeks of the next cycle that would be fine as well.
> No one should be insane enough to not pick up at least the first few stable
> releases of a new kernel!
> 
>> I can also try to do the changes tomorrow but I don't have the hardware
>> anymore so I won't be able to test until I get it back, which is only
>> next week.
>>
>>> There might be other drivers that have that interpretation we can't
>>> fix but if we can reduce the scope of the problem by changing the adxl367
>>> that would be great.
>>>
>>> We should also definitely improve the docs and perhaps add a note to say
>>> that due to need to maintain ABI, a few drivers use scans * number of channels
>>> rather than scans.
>>
>> I guess I could also do that at the same time.
> 
> Perfect :)
> 
> Thanks for sorting this out.
> 
> Jonathan
> 
> 

I've just had another good look at ADXL367. It seems that I'm quick at
forgetting stuff about the code I write.

In adxl367_set_fifo_samples(), fifo_watermark is multiplied by
fifo_set_size, then, if it is larger than the maximum watermark,
it is capped at the maximum watermark, and then, it is divided
by fifo_set_size again.

In the end, fifo_watermark actually seems to mean number of scans
in that driver too.

So this was a huge confusion. The different thing about AD4130 is that
the register actually means number of samples, and not number of scans,
so that's where that confussion stemmed from.

Sorry for wasting your time on this.