Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Sep 2022 17:01:15 +0300
From:   Andy Shevchenko <andriy.shevchenko@intel.com>
To:     "Farber, Eliav" <farbere@amazon.com>
Cc:     jdelvare@suse.com, linux@roeck-us.net, robh+dt@kernel.org,
        p.zabel@pengutronix.de, rtanwar@maxlinear.com,
        linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, hhhawa@amazon.com, jonnyc@amazon.com
Subject: Re: [PATCH v4 05/21] hwmon: (mr75203) fix voltage equation for
 negative source input
Message-ID: <YxdSqw6STdkbDESV@smile.fi.intel.com>
References: <20220906083356.21067-1-farbere@amazon.com>
 <20220906083356.21067-6-farbere@amazon.com>
 <Yxc3GeFc5gDKrYyP@smile.fi.intel.com>
 <29fa5c01-aad0-04ff-e1a9-1510858eff7e@amazon.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <29fa5c01-aad0-04ff-e1a9-1510858eff7e@amazon.com>
Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
Precedence: bulk

On Tue, Sep 06, 2022 at 04:27:13PM +0300, Farber, Eliav wrote:
> On 9/6/2022 3:03 PM, Andy Shevchenko wrote:
> > On Tue, Sep 06, 2022 at 08:33:40AM +0000, Eliav Farber wrote:

...

> > > -???????????? *val = (PVT_N_CONST * n - PVT_R_CONST) >> PVT_CONV_BITS;
> > > +???????????? *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 <<
> > > PVT_CONV_BITS);
> > 
> > Wondering if we can use BIT(PVT_CONV_BITS) for two (quite unlikely to
> > happen,
> > I hope) purposes:
> > 
> > 1) Somebody copies such code where PVT_CONV_BITS analogue can be 31,
> > ? which is according to C standard is UB (undefined behaviour).
> > 
> > 2) It makes shorter the line and also drops the pattern where some
> > ? dumb robot may propose a patch to basically revert the division
> > ? change.
> I originally tried to use BIT(PVT_CONV_BITS) but it gave a different
> result.
> e.g.
> If n = 2720
> *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / (1 << PVT_CONV_BITS) = 0
> *val = (PVT_N_CONST * (long)n - PVT_R_CONST) / BIT(PVT_CONV_BITS) =
> 18014398509481983
> 
> I can try fitting it in one line, either by adding a define for
> (1 << PVT_CONV_BITS) or exceeding 80 characters, but keep in mind that
> in a later patch (#15) it gets even longer (and I must use more than
> one line) since it is multiplied by a pre-scaler factor.

Don't get me wrong, it's not about style, it's about preventing
followup "fixes" of this. All the problems here due to (hidden)
unsigned type(s).

What you can do is to add a good comment on top of that line
explaining why division instead of right shift and why BIT()
may not be used (because it's unsigned).

-- 
With Best Regards,
Andy Shevchenko