From: Christian Eggers <ceggers@arri.de>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Rob Herring <robh+dt@kernel.org>,
Jonathan Cameron <Jonathan.Cameron@huawei.com>,
Jonathan Cameron <jic23@kernel.org>,
"Hartmut Knaack" <knaack.h@gmx.de>,
Lars-Peter Clausen <lars@metafoo.de>,
"Peter Meerwald-Stadler" <pmeerw@pmeerw.net>,
linux-iio <linux-iio@vger.kernel.org>,
devicetree <devicetree@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5 2/2] iio: light: as73211: New driver
Date: Tue, 4 Aug 2020 09:40:11 +0200 [thread overview]
Message-ID: <2356337.HYKpEJ1Wej@n95hx1g2> (raw)
In-Reply-To: <CAHp75Vev64E86OWm+eV=1o4ZDs0Xh_Y1z6V54GmpRwWmjD7=eA@mail.gmail.com>
On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote:
> Thanks for an update, my comments below.
Thanks for the review. Please see below for my questions.
Best regards
Christian
> On Sun, Aug 2, 2020 at 7:40 PM Christian Eggers <ceggers@arri.de> wrote:
> > Datasheet:
> > https://ams.com/documents/20143/36005/AS73211_DS000556_3-01.pdf/a65474c0-
> > b302-c2fd-e30a-c98df87616df
> Do we need the UUID after the document file name?
I have send AMS an inquiry. Not sure whether I will get an answer. I will wait
a few days until sending v6.
> > +#define AS73211_OFFSET_TEMP (-66.9)
> > +#define AS73211_SCALE_TEMP 0.05
>
> In the kernel we don't do float arithmetic. How these are being used?
Does this restriction also apply for compile time constants? I am quite
sure that all calculations using these defines will be evaluated at compile
time. If found a number of other places where probably the same is done:
find . -name '*.c' | xargs grep "#define.*[0-9]\.[0-9]" | grep -v '"' | grep -v "\/\*.*[0-9]\.[0-9]"
> > + *val2 = (AS73211_OFFSET_TEMP - (int)AS73211_OFFSET_TEMP) *
> > 1000000;
> >
> > + *val2 = (AS73211_SCALE_TEMP -
> > (int)AS73211_SCALE_TEMP) * 1000000;
> Magic 1000000 multiplier.
I think that in the context of IIO_VAL_INT_PLUS_MICRO this isn't quite magic. Using
1000000 directly seems quite usual:
find drivers/iio/ -type f | xargs grep "val2 = .*1000000"
> I think here you got them always 0. And to fix that you need to
> redefine (with also units included in the name) above constants like
> #define ..._OFFSET_TEMP_mC 66500
> ... _SCALE_TEMP_?? 50
a scale factor has no unit
>
> Consider to use definitions from
> https://elixir.bootlin.com/linux/latest/source/include/linux/units.h
There are only definition for milli celsius. For IIO_VAL_INT_PLUS_MICRO I would
require micro celsius.
If I have the freedom, I would keep it as it is. Else I would suggest the following:
#define AS73211_OFFSET_TEMP_INT (-66)
#define AS73211_OFFSET_TEMP_MICRO 900000
#define AS73211_SCALE_TEMP_INT 0
#define AS73211_SCALE_TEMP_MICRO 50000
> > + }}
> > +
> > + return -EINVAL;
>
> Make it default case.
changed. Is there any benefit? My IDE's syntax checker now complains
"No return, in a function returning non-void". But gcc is happy with this.
> > + ret = devm_iio_device_register(dev, indio_dev);
> > + if (ret < 0)
> > + return ret;
> > +
> > + return 0;
>
> return devm_iio_device_register();
changed. I prefer the original pattern as it would produce less changed lines
if something needs to inserted later.
next prev parent reply other threads:[~2020-08-04 7:42 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-02 16:37 [PATCH v5 0/2] iio: light: Support AMS AS73211 digital XYZ sensor Christian Eggers
2020-08-02 16:37 ` [PATCH v5 1/2] dt-bindings: iio: light: add AMS AS73211 support Christian Eggers
2020-08-02 16:37 ` [PATCH v5 2/2] iio: light: as73211: New driver Christian Eggers
2020-08-02 18:02 ` Andy Shevchenko
2020-08-04 7:40 ` Christian Eggers [this message]
2020-08-04 8:28 ` Andy Shevchenko
2020-08-04 8:34 ` Lars-Peter Clausen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=2356337.HYKpEJ1Wej@n95hx1g2 \
--to=ceggers@arri.de \
--cc=Jonathan.Cameron@huawei.com \
--cc=andy.shevchenko@gmail.com \
--cc=devicetree@vger.kernel.org \
--cc=jic23@kernel.org \
--cc=knaack.h@gmx.de \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pmeerw@pmeerw.net \
--cc=robh+dt@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).