Re: [PATCH 2/3] iio: pressure: Support ROHM BU1390

[Jonathan Cameron]

On Fri, 8 Sep 2023 09:12:51 +0300
Matti Vaittinen <mazziesaccount@xxxxxxxxx> 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.

> 
> >   
> >>>> +	__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

> 
> 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.


> 
> > ...
> >   
> >>>> +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.

Jonathan