linux-kernel - Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver

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: <CAN8YU5M6S11Bbe3LW74RCD3zqL6xQ+ZF5JtzMKSMMn7pxLL27g@mail.gmail.com>
Date:   Mon, 19 Jul 2021 09:12:56 +0200
From:   Andrea Merello <andrea.merello@...il.com>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Jonathan Cameron <jic23@...nel.org>,
        Lars-Peter Clausen <lars@...afoo.de>,
        Rob Herring <robh+dt@...nel.org>,
        Matt Ranostay <matt.ranostay@...sulko.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-iio <linux-iio@...r.kernel.org>,
        Andrea Merello <andrea.merello@....it>
Subject: Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver

Just few inline comments; implicitly OK for others.

Il giorno ven 16 lug 2021 alle ore 14:39 Andy Shevchenko
<andy.shevchenko@...il.com> ha scritto:
>
>
> > > Useless parentheses. If the LEN is a plain number, use decimal, if
> > > it's limited by register width, use the form of (BIT(x) - 1). In such
> > > a case it's easy to see how many bits are used for it.
> >
> > It's byte number, defined by how many 8-bits registers make up the
> > UID. I'll go for a decimal and I'll drop the parentheses.
>
> 15 seems the right one then?

Isn't it 16? From my understanding of the datasheet registers involved
are from 0x50 to 0x5F.

>
> > > > +       if (res && res != -ERESTARTSYS) {
> > >
> > > Shouldn't RESTARTSYS be handled on a regmap level?
> >
> > Can you please elaborate on this?
>
> I meant if you need to take care about this it seems to me that it has to be
> thought of on regmap level. I.o.w. what is the rationale behind this additional
> check?

The regmap_bus write() and read() implementations wait for an
interruptible completion, which is completed when a response from the
IMU is received. In practice by hitting Ctrl-C at the "right" moment I
got my kernel log polluted with dev_err() telling me the regmap
operation failed, but in this specific case there was nothing wrong:
it's just being aborted. Still, in all other error case I would like
to know. This is the rationale behind this check. The ERESTARTSYS
error have anyway to actually propagate in order to notify the caller
that the read/write just didn't complete.

If you mean move the check+dev_err() in bno055_sl.c regmap_bus read()
and write() ops, that is fine; my original point for putting it where
it is now, was because I was wondering whether this has to be common
to the (not yet here) I2C support code.

> ...
>
> > > Sounds like NIH hex2bin().
> >
> > Indeed.. I've failed to find out this helper. Looking at the code it
> > seems it wouldn't work as drop-in replacement here, because of spaces
> > in the HEX string. But I might just decide to format the HEX string
> > without spaces in order to being able to use hex2bin().
>
> I'm not even sure why it's in ASCII instead being directly binary file.

That was almost a coin-flip for me. Just, being a few bytes, I decided
to make them printable: If I load this driver for the 1st time, and
start poking around in it's sysfs, cat-ting random stuff to give a
look, I would just find a HEX line more friendly that a binary chunk
on my console.

.. But If you think it's better, I'll go for binary without any hesitation..

>
> > > IIO core should do this, besides the fact that it must use sysfs_emit().
> > > Ditto for the similar.
> >
> > Ok for sysfs_emit(), thanks. But what do you mean with "IIO core
> > should do this"? Can you please elaborate?
>
> I believe that IIO has a generic method to print tables via sysfs. AFAIR it is
> done via "_avail".

Ah, do you refer to the read_avail() operation in iio_info?
I'll try to go with it; I wasn't aware of that, thank you.