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
