From: "Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>
To: Jonathan Cameron <jic23@kernel.org>,
Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Dmitry Osipenko <dmitry.osipenko@collabora.com>,
Shreeya Patel <shreeya.patel@collabora.com>,
Paul Gazzillo <paul@pgazz.com>,
Zhigang Shi <Zhigang.Shi@liteon.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>
Subject: Re: [PATCH v5 7/8] iio: light: ROHM BU27034 Ambient Light Sensor
Date: Mon, 27 Mar 2023 07:16:05 +0000 [thread overview]
Message-ID: <959a5fed-c1c8-db21-5a05-963d0f9a999a@fi.rohmeurope.com> (raw)
In-Reply-To: <20230326171951.0e815ec3@jic23-huawei>
On 3/26/23 19:19, Jonathan Cameron wrote:
> On Wed, 22 Mar 2023 11:07:56 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
>> ROHM BU27034 is an ambient light sensor with 3 channels and 3 photo diodes
>> capable of detecting a very wide range of illuminance. Typical application
>> is adjusting LCD and backlight power of TVs and mobile phones.
>>
>> Add initial support for the ROHM BU27034 ambient light sensor.
>>
>> NOTE:
>> - Driver exposes 4 channels. One IIO_LIGHT channel providing the
>> calculated lux values based on measured data from diodes #0 and
>> #1. In addition, 3 IIO_INTENSITY channels are emitting the raw
>> register data from all diodes for more intense user-space
>> computations.
>> - Sensor has GAIN values that can be adjusted from 1x to 4096x.
>> - Sensor has adjustible measurement times of 5, 55, 100, 200 and
>> 400 mS. Driver does not support 5 mS which has special
>> limitations.
>> - Driver exposes standard 'scale' adjustment which is
>> implemented by:
>> 1) Trying to adjust only the GAIN
>> 2) If GAIN adjustment alone can't provide requested
>> scale, adjusting both the time and the gain is
>> attempted.
>> - Driver exposes writable INT_TIME property that can be used
>> for adjusting the measurement time. Time adjustment will also
>> cause the driver to try to adjust the GAIN so that the
>> overall scale is kept as close to the original as possible.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>
> Hi Matti,
>
> A few minor comments inline. I'll take a closer look at the rest of the
> series when the discussions around the tests and devices to be used
> for them settle down.
>
> Thanks,
>
> Jonathan
>
>> +
>> +static u64 bu27034_fixp_calc_t1(unsigned int coeff, unsigned int ch0,
>> + unsigned int ch1, unsigned int gain0,
>> + unsigned int gain1)
>> +{
>> + unsigned int helper, tmp;
>> + u64 helper64;
>> +
>> + /*
>> + * Here we could overflow even the 64bit value. Hence we
>> + * multiply with gain0 only after the divisions - even though
>> + * it may result loss of accuracy
>> + */
>> + helper64 = (u64)coeff * (u64)ch1 * (u64)ch1;
>> + helper = coeff * ch1 * ch1;
>> + tmp = helper * gain0;
>> +
>> + if (helper == helper64 && (tmp / gain0 == helper))
>
> Similar to below. Don't bother with the non 64 bit version.
>
>> + return tmp / (gain1 * gain1) / ch0;
>> +
>> + helper = gain1 * gain1;
>> + if (helper > ch0) {
>> + do_div(helper64, helper);
>> +
>> + return gain_mul_div_helper(helper64, gain0, ch0);
>> + }
>> +
>> + do_div(helper64, ch0);
>> +
>> + return gain_mul_div_helper(helper64, gain0, helper);
>> +}
>> +
>> +static u64 bu27034_fixp_calc_t23(unsigned int coeff, unsigned int ch,
>> + unsigned int gain)
>> +{
>> + unsigned int helper;
>> + u64 helper64;
>> +
>> + helper64 = (u64)coeff * (u64)ch;
>> + helper = coeff * ch;
>> +
>> + if (helper == helper64)
>> + return helper / gain;
>> +
>> + do_div(helper64, gain);
>> +
>> + return helper64;
>
> I suspect that this is a premature bit of optimization so I'd just
> do it in 64 bits always.
Probably so.
>
> Also, if you did want to do this, check_mul_overflow() etc would help.
> (linux/overflow.h)
Thanks! I'll check the overflow.h
The only reason why I did it like this is that I've been bitten badly by
the do_div() in the past. It was actually quite expensive payment for a
lesson learnt - my do_div() usage ended up in a customer devices in the
field - and with a bit of bad luck we got a huge number to be divided
with a small one... And the do_div() implementation for the architecture
was a loop subtracting the divider.
The thing ended up halting the customer devices for many seconds,
messing up lots of things. On top of that the project was huge - Amount
of SW-developers was four figures. It took weeks until the bug report
found it's way also to my desk - at which point I finally found the
mistake I had made... And I didn't feel proud of it :|
And yes, we don't prevent the "divide big number by a tiny one" - issue
by this check here. OTOH, I think I didn't see the loop-based do_div()
implementation anymore either. It's just the habit of only using
do_div() as a last resort. But yes, we probably can unconditionally use
the do_div() here. I'll see what we have in the overflow.h though.
Thanks for the review again!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
next prev parent reply other threads:[~2023-03-27 7:16 UTC|newest]
Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-22 9:05 [PATCH v5 0/8] Support ROHM BU27034 ALS sensor Matti Vaittinen
2023-03-22 9:05 ` [PATCH v5 1/8] drivers: kunit: Generic helpers for test device creation Matti Vaittinen
2023-03-22 12:04 ` Greg Kroah-Hartman
2023-03-22 12:07 ` Greg Kroah-Hartman
2023-03-22 13:48 ` Matti Vaittinen
2023-03-22 18:57 ` Greg Kroah-Hartman
2023-03-23 7:17 ` Vaittinen, Matti
2023-03-23 8:58 ` Greg Kroah-Hartman
2023-03-23 9:20 ` Matti Vaittinen
2023-03-23 10:25 ` Greg Kroah-Hartman
2023-03-23 10:43 ` Matti Vaittinen
2023-03-23 10:01 ` Matti Vaittinen
2023-03-23 10:27 ` Greg Kroah-Hartman
2023-03-23 11:00 ` Matti Vaittinen
2023-03-23 10:12 ` Maxime Ripard
2023-03-23 10:21 ` Greg Kroah-Hartman
2023-03-23 12:16 ` Matti Vaittinen
2023-03-23 12:29 ` Maxime Ripard
2023-03-23 13:02 ` Matti Vaittinen
2023-03-23 16:36 ` Maxime Ripard
2023-03-24 6:11 ` Matti Vaittinen
2023-03-24 6:34 ` David Gow
2023-03-24 6:51 ` Matti Vaittinen
2023-03-24 9:52 ` David Gow
2023-03-24 10:05 ` Matti Vaittinen
2023-03-24 10:17 ` Matti Vaittinen
2023-03-25 4:35 ` David Gow
2023-03-25 7:26 ` Matti Vaittinen
2023-03-24 12:46 ` Maxime Ripard
2023-03-24 12:31 ` Maxime Ripard
2023-03-25 5:40 ` David Gow
2023-03-29 19:43 ` Maxime Ripard
2023-03-25 17:50 ` Jonathan Cameron
2023-03-26 17:16 ` Lars-Peter Clausen
2023-04-01 15:30 ` Jonathan Cameron
2023-03-29 19:46 ` Maxime Ripard
2023-04-01 15:36 ` Jonathan Cameron
2023-03-24 12:36 ` Maxime Ripard
2023-03-24 12:43 ` Greg Kroah-Hartman
2023-03-24 13:02 ` Maxime Ripard
2023-03-24 13:42 ` Greg Kroah-Hartman
2023-03-22 12:08 ` Greg Kroah-Hartman
2023-03-23 7:30 ` David Gow
2023-03-23 8:35 ` Matti Vaittinen
2023-03-23 9:02 ` Greg Kroah-Hartman
2023-03-23 10:07 ` Maxime Ripard
2023-03-22 9:06 ` [PATCH v5 2/8] drm/tests: helpers: Use generic helpers Matti Vaittinen
2023-03-22 9:06 ` [PATCH v5 3/8] dt-bindings: iio: light: Support ROHM BU27034 Matti Vaittinen
2023-03-22 9:06 ` [PATCH v5 4/8] iio: light: Add gain-time-scale helpers Matti Vaittinen
2023-03-22 9:07 ` [PATCH v5 5/8] iio: test: test " Matti Vaittinen
2023-03-24 6:29 ` Matti Vaittinen
2023-03-22 9:07 ` [PATCH v5 6/8] MAINTAINERS: Add IIO " Matti Vaittinen
2023-03-22 9:07 ` [PATCH v5 7/8] iio: light: ROHM BU27034 Ambient Light Sensor Matti Vaittinen
2023-03-26 16:19 ` Jonathan Cameron
2023-03-27 7:16 ` Vaittinen, Matti [this message]
2023-03-22 9:08 ` [PATCH v5 8/8] MAINTAINERS: Add ROHM BU27034 Matti Vaittinen
2023-03-22 10:01 ` [PATCH v5 0/8] Support ROHM BU27034 ALS sensor Andy Shevchenko
2023-03-22 10:34 ` Javier Martinez Canillas
2023-03-22 10:59 ` Matti Vaittinen
2023-03-22 11:02 ` Andy Shevchenko
2023-03-23 9:28 ` Maxime Ripard
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=959a5fed-c1c8-db21-5a05-963d0f9a999a@fi.rohmeurope.com \
--to=matti.vaittinen@fi.rohmeurope.com \
--cc=Zhigang.Shi@liteon.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=dmitry.osipenko@collabora.com \
--cc=jic23@kernel.org \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mazziesaccount@gmail.com \
--cc=paul@pgazz.com \
--cc=shreeya.patel@collabora.com \
/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).