Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210715141742.15072-1-andrea.merello@gmail.com>
 <20210715141742.15072-3-andrea.merello@gmail.com> <CAHp75Vf_Og2wjRy2j0gC37DgR0x9B_F5iSUj8VOtWkhWjgiOag@mail.gmail.com>
 <CAN8YU5NKGLO6a4wqaW07NAU-OdsdBohoXzMCBcskaFsCRtrGhw@mail.gmail.com> <YPF97IvnlUDtIHar@smile.fi.intel.com>
In-Reply-To: <YPF97IvnlUDtIHar@smile.fi.intel.com>
Reply-To: andrea.merello@gmail.com
From:   Andrea Merello <andrea.merello@gmail.com>
Date:   Mon, 19 Jul 2021 09:12:56 +0200
Message-ID: <CAN8YU5M6S11Bbe3LW74RCD3zqL6xQ+ZF5JtzMKSMMn7pxLL27g@mail.gmail.com>
Subject: Re: [PATCH 2/4] iio: imu: add Bosch Sensortec BNO055 core driver
To:     Andy Shevchenko <andy.shevchenko@gmail.com>
Cc:     Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Rob Herring <robh+dt@kernel.org>,
        Matt Ranostay <matt.ranostay@konsulko.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        linux-iio <linux-iio@vger.kernel.org>,
        Andrea Merello <andrea.merello@iit.it>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Just few inline comments; implicitly OK for others.

Il giorno ven 16 lug 2021 alle ore 14:39 Andy Shevchenko
<andy.shevchenko@gmail.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.