linux-kernel - Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to retrieve number of bytes in buffer

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <61247547-690c-fb8b-3a45-cd60754836a7@gmail.com>
Date:   Mon, 28 Aug 2023 09:24:25 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     Mehdi Djait <mehdi.djait.k@...il.com>,
        krzysztof.kozlowski+dt@...aro.org, robh+dt@...nel.org,
        lars@...afoo.de, linux-iio@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v8 6/7] iio: accel: kionix-kx022a: Add a function to
 retrieve number of bytes in buffer

On 8/27/23 21:09, Jonathan Cameron wrote:
> 
>>>>>> I see. I think my confusion can be easily cured by renaming the callback to
>>>>>>
>>>>>>          get_amount_bytes_in_fifo()
>>>>>>
>>>>>> or
>>>>>>
>>>>>>          get_bytes_in_fifo()
>>>>>>
>>>>>> or alike.
>>>>>
>>>>> or leave it as is. The function is documented:
>>>>   
>>>>> + * @get_fifo_bytes: function pointer to get number of bytes in the FIFO buffer
>>>>
>>>> Do you find it unambiguous? I do not.
>>>>
>>>> Still needs more words to explain if it's a capacity of FIFO or is it amount of
>>>> valid bytes for the current transfer or what?
>>>
>>> how about change the description to:
>>> function pointer to get amount  of acceleration data bytes currently
>>> stored in the sensor's FIFO buffer
>>>
>>> and change the function to "get_amount_bytes_in_fifo()"
>>
>> Sounds good to me, thank you!
>>
> Bikeshedding time ;)
> 
> I don't like "amount" in this - it ends up adding little meaning
> and to me it is ugly English.  It's making it clear that we are dealing
> with some sort of count but that is already true of get_bytes_in_fifo()
> So to my reading it adds nothing wrt to removing ambiguity.
> 
> get_number_of_bytes_in_fifo() flows better but also adds nothing over
> get_bytes_in_fifo()
> 
> You could make it clear it is something that changes over time.
> 
> get_current_bytes_in_fifo()
> 
> Which at least implies it changes - though it doesn't rule out a weird
> variable max size fifo.
> 
> get_fifo_bytes_available() might be the clearest option and is the one
> I would prefer.  It's still a little messy as it could mean
> 'number of bytes of data that haven't been used yet in the fifo and
>   are available for samples in the future'.
> 
> Sigh.  Maybe least ambiguous is something longer like.
> 
> get_fifo_bytes_available_to_read()
> get_fifo_bytes_available_out()
> 
> Honestly I don't care that much what you go with :)

If this was a democracy (which it isn't) - my vote would go for "leave 
as it is" because the concept of a data collecting fifo where amount of 
data acquired in FIFO is readable from a register is common enough. I 
think that people who work on a driver like this should guess what this 
is for. Besides, if anything more than looking at the code is needed, 
then the plain guessing won't do and one needs anyway to open the 
data-sheet.

 From my perspective this series adds a nice value and is good to go.

Just my 10 cents though :)

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~