Message-ID: <33073300-e238-0319-0a97-094a25dcf86b@gmail.com>
Date:   Mon, 11 Sep 2023 08:32:31 +0300
From:   Matti Vaittinen <mazziesaccount@...il.com>
To:     Jonathan Cameron <jic23@...nel.org>
Cc:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Matti Vaittinen <matti.vaittinen@...rohmeurope.com>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Angel Iglesias <ang.iglesiasg@...il.com>,
        Andreas Klinger <ak@...klinger.de>, linux-iio@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

On 9/8/23 21:44, Jonathan Cameron wrote:
> On Fri, 8 Sep 2023 09:12:51 +0300
> Matti Vaittinen <mazziesaccount@...il.com> wrote:
> 
>> On 9/7/23 16:57, Andy Shevchenko wrote:
>>> On Thu, Sep 07, 2023 at 08:57:17AM +0300, Matti Vaittinen wrote:
>>>> On 9/6/23 18:48, Andy Shevchenko wrote:
>>>>> On Wed, Sep 06, 2023 at 03:37:48PM +0300, Matti Vaittinen wrote:
>>>
>>> ...
>>>    
>>>>>> +struct bm1390_data {
>>>>>> +	int64_t timestamp, old_timestamp;
>>>>>
>>>>> Out of a sudden int64_t instead of u64?
>>>>
>>>> Judging the iio_push_to_buffers_with_timestamp() and iio_get_time_ns(), IIO
>>>> operates with signed timestamps. One being s64, the other int64_t.
>>>>   
>>>>>> +	struct iio_trigger *trig;
>>>>>> +	struct regmap *regmap;
>>>>>> +	struct device *dev;
>>>>>> +	struct bm1390_data_buf buf;
>>>>>> +	int irq;
>>>>>> +	unsigned int state;
>>>>>> +	bool trigger_enabled;
>>>>>   
>>>>>> +	u8 watermark;
>>>>>
>>>>> Or u8 instead of uint8_t?
>>>>
>>>> So, uint8_t is preferred? I don't really care all that much which of these
>>>> to use - which may even show up as a lack of consistency... I think I did
>>>> use uint8_t when I learned about it - but at some point someone somewhere
>>>> asked me to use u8 instead.. This somewhere might have been u-boot though...
>>>>
>>>> So, are you Suggesting I should replace u8 with uint8_t? Can do if it
>>>> matters.
>>>
>>> Consistency matters, since I do not know the intention behind, I suggest use
>>> either, but be consistent in the entire code. However, uXX are specific Linux
>>> kernel internal types and some maintainers prefer them. Also you may grep for
>>> the frequency of intXX_t vs. sXX or their unsigned counterparts.
>>>    
>>>>>> +	/* Prevent accessing sensor during FIFO read sequence */
>>>>>> +	struct mutex mutex;
>>>>>> +};
>>>
>>> ...
>>>    
>>>>>> +static int bm1390_read_temp(struct bm1390_data *data, int *temp)
>>>>>> +{
>>>>>> +	u8 temp_reg[2] __aligned(2);
>>>>>
>>>>> Why?! Just use proper bitwise type.
>>>>
>>>> What is the proper bitwise type in this case?
>>>>
>>>> I'll explain my reasoning:
>>>> What we really have in hardware (BM1390) and read from it is 8bit registers.
>>>> This is u8 to me. And as we read two consecutive registers, we use u8
>>>> arr[2]. In my eyes it describes the data perfectly well, right?
>>>
>>> Two different registers?! Why bulk is used in that case?
>>> To me looks like you are reading 16-bit (or one that fits in 16-bit) register
>>> in BE notation.
>>
>> As I wrote, it is two 8 bit registers at consecutive addresses. And this
>> is what u8 arr[2]; also says.
>>
>> Bulk read is mandatory as HW has a special feature of preventing the
>> update of these registers when a read is ongoing. If we do two reads we
>> risk of getting one of the registers updated between the accesses -
>> resulting incorrect value.
> 
> Far as the kernel is concerned this is a __be16 and we use a bulk read to
> fill it from two registers.  If the use showed them having unrelated uses
> then it would be fair to handle it as an array of u8.

After you explained me that we don't need the extra hassle with the 
negative values (specific two's complement handling) - I do agree. If we 
needed to check the high bit in reg X to detect if special handling is 
required (as I thought) - then I would have still thought that checking 
the high bit from u8 which matches the spec would be much clearer. But 
yes - in this case, after your explanation - I do agree. Thanks :)

> 
>>
>>>    
>>>>>> +	__be16 *temp_raw;
>>>>>> +	int ret;
>>>>>> +	s16 val;
>>>>>> +	bool negative;
>>>>>> +
>>>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_TEMP_HI, &temp_reg,
>>>>>> +			       sizeof(temp_reg));
>>>>>> +	if (ret)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	if (temp_reg[0] & 0x80)
>>>>>> +		negative = true;
>>>>>> +	else
>>>>>> +		negative = false;
>>>>>   
>>>>>> +	temp_raw = (__be16 *)&temp_reg[0];
>>>>>
>>>>> Heck no. Make temp_reg be properly typed.
>>>>
>>>> As I explained above, to me it seems ur arr[2} is _the_ proper type to
>>>> represent data we read from the device.
>>>>
>>>> What we need to do is to convert the 16bits of data to an integer we can
>>>> give to the rest of the system. And, we happen to know how to 'manipulate'
>>>> the data to get it in format of understandable integer. As we have these 16
>>>> bits in memory, aligned to 2 byte boundary - why shouldn't we just
>>>> 'manipulate' the data and say - here we have your integer, please be my
>>>> guest and use it?
>>>
>>> Because it smell like a hack and is a hack here.
>>> Either it's a single BE16 register, or it's two different registers that by
>>> very unknown reason you are reading in a bulk.
>>
>> It is two registers. The bulk read might warrant a comment - although I
>> believe this is nothing unusual. If it is a hack or not is an opinion.
>> To me it looks like a code that explicitly shows what data is and how it
>> is being handled. It does what it is supposed to do and shows it in all
>> dirty details.
> 
> Read it directly into a __be16

Yes. This makes sense also to me now that we don't really need the:
 >>>>>> +	if (temp_reg[0] & 0x80)
 >>>>>> +		negative = true;
 >>>>>> +	else
 >>>>>> +		negative = false;

shown above.

> 
>>
>> Still, I am open to suggestions but I'd prefer seeing a real improvement
>> instead of claiming that the hardware is something it is not (eg, having
>> 16bit registers or should be read by individual accesses).
>>
>> The code in this form is no
>>> go.
>>>    
>>>> Well, I am keen to learn the 'correct bitwise type' you talk about - can you
>>>> please explain me what this correct type for two 8bit integers is? Maybe I
>>>> can improve.
>>>
>>> If the registers are not of the same nature the bulk access is wrong.
>>> Use one by one reads.
>>
>> Of same nature? As I said, there is 2 8bit registers at consecutive
>> addresses. They have no other 'nature' as far as I can tell.
>>
>> Data in these registers in not in standard format - it needs to be
>> manipulated to make it an ordinary integer. The code shows this very
>> clearly by not reading it in any standard integer.
> 
> I'm not convinced it does.  All support arch are 2s complement.
> be16_to_cpu() is fine for what you have unless I'm missing something.
> If it weren't we would have signed versions of those macros.

No. You don't miss a thing. I didn't know if it's ok to treat i6bit 
two's complement just as a native 16bit signed integer.

>>> ...
>>>    
>>>>>> +static int bm1390_pressure_read(struct bm1390_data *data, u32 *pressure)
>>>>>> +{
>>>>>> +	int ret;
>>>>>> +	u8 raw[3];
>>>>>> +
>>>>>> +	ret = regmap_bulk_read(data->regmap, BM1390_REG_PRESSURE_BASE,
>>>>>> +			       &raw[0], sizeof(raw));
>>>
>>> &raw[0] --> raw
>>>    
>>>>>> +	if (ret < 0)
>>>>>> +		return ret;
>>>>>> +
>>>>>> +	*pressure = (u32)(raw[2] >> 2 | raw[1] << 6 | raw[0] << 14);
>>>
>>> This, btw, looks like le24, but I'm puzzled with right shift.
>>> I need to read datasheet carefully to understand this.
>>
>> It's not just le24. We, again, have data placed in registers so that it
>> needs some suffling. The data-sheet does decent job explaining it
>> though. AFAIR, there was a 'gap' in bits so it needed some more suffling
>> to sift the bits so that they're consecutive. I think this indeed is
>> something that needs to be looked up from data-sheet to understand why
>> this play with bits is done.
> 
> 
> These cases are harder to argue.  I'm fine with either approach for this one
> as a get_unaligned_le24() >> 2 would give same answer unless I'm also missing
> something but it isn't that obvious.

I need to look at the spec and the get_unaligned_le24() to be able to 
say something :) I wrote this code before my summer holidays but didn't 
have the time to test and send the patches until now. Will get back to 
this IC later this week.

Thanks for the feedback guys!

Yours,
	-- Matti

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

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

Powered by blists - more mailing lists