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