Linux-IIO Archive on lore.kernel.org
 help / color / Atom feed
From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Christian Eggers <ceggers@arri.de>
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 11:28:22 +0300
Message-ID: <CAHp75VdusOQNzrY_Ken0KW06gtRW8fCOOtYXo=9nnFKc6bgWnQ@mail.gmail.com> (raw)
In-Reply-To: <2356337.HYKpEJ1Wej@n95hx1g2>

On Tue, Aug 4, 2020 at 10:42 AM Christian Eggers <ceggers@arri.de> wrote:
> On Sunday, 2 August 2020, 20:02:35 CEST, Andy Shevchenko wrote:
> > 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.

I have successfully opened a document w/o additional UUID at the end of URI.
I think you may drop it.

...

> > > +#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]"

Side note: `git grep ...` is much faster and better.
% git grep -n -w '#define[^"/]\+[0-9]\+\.[0-9]\+' -- drivers/ include/
arch/ | wc -l
47

+ DRM, yes.

In any case...

> > > +               *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"

Hmm... Okay.

> > 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

...somewhat like above would be better. But your freedom is defined by
maintainers (not by me), so wait for their comments.

...

> > > +       }}
> > > +
> > > +       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.

Your IDE is buggy :-)
Yes, there is a benefit of doing this, at some point compiler
complains about switches that don't cover all cases.

...

> > > +       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.

But if not, it will be a bulk of several lines of code which is the
bait for all kinds of janitors and clean up scripts (I saw that IRL,
so it's not unrealistic). In that case it will be twice the churn.

-- 
With Best Regards,
Andy Shevchenko

  reply index

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
2020-08-04  8:28       ` Andy Shevchenko [this message]
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='CAHp75VdusOQNzrY_Ken0KW06gtRW8fCOOtYXo=9nnFKc6bgWnQ@mail.gmail.com' \
    --to=andy.shevchenko@gmail.com \
    --cc=Jonathan.Cameron@huawei.com \
    --cc=ceggers@arri.de \
    --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

Linux-IIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iio/0 linux-iio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iio linux-iio/ https://lore.kernel.org/linux-iio \
		linux-iio@vger.kernel.org
	public-inbox-index linux-iio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-iio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git