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:   Thu, 10 Feb 2022 12:36:02 -0500
From:   Liam Beguin <liambeguin@gmail.com>
To:     Peter Rosin <peda@axentia.se>
Cc:     jic23@kernel.org, andy.shevchenko@gmail.com, lars@metafoo.de,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        devicetree@vger.kernel.org, robh+dt@kernel.org
Subject: Re: [PATCH v14 02/11] iio: afe: rescale: add INT_PLUS_{MICRO,NANO}
 support
Message-ID: <YgVNAj7kz1QbKkhy@shaak>
References: <20220208020441.3081162-1-liambeguin@gmail.com>
 <20220208020441.3081162-3-liambeguin@gmail.com>
 <fbb84b08-5d3d-3684-fdee-5ce367280857@axentia.se>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fbb84b08-5d3d-3684-fdee-5ce367280857@axentia.se>
Precedence: bulk

Hi Peter,

On Thu, Feb 10, 2022 at 05:42:25PM +0100, Peter Rosin wrote:
> Hi!
> 
> As usual, sorry for my low bandwidth.

No worries, I understand. It's been pretty slow on my end as well.

Thanks for still making time for this :-)

> On 2022-02-08 03:04, Liam Beguin wrote:
> > Some ADCs use IIO_VAL_INT_PLUS_{NANO,MICRO} scale types.
> > Add support for these to allow using the iio-rescaler with them.
> > 
> > Signed-off-by: Liam Beguin <liambeguin@gmail.com>
> > Reviewed-by: Peter Rosin <peda@axentia.se>
> > ---
> >  drivers/iio/afe/iio-rescale.c | 35 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)

...

> > @@ -41,6 +45,37 @@ int rescale_process_scale(struct rescale *rescale, int scale_type,
> >  		tmp *= rescale->numerator;
> >  		tmp = div_s64(tmp, 1000000000LL);
> >  		*val = tmp;
> > +		return scale_type;
> > +	case IIO_VAL_INT_PLUS_NANO:
> > +	case IIO_VAL_INT_PLUS_MICRO:
> > +		mult = scale_type == IIO_VAL_INT_PLUS_NANO ? GIGA : MEGA;
> 
> By now, we all agree that the big numbers in this context have nothing
> to do with unit prefixes of physical quantities, so the macros are not
> really appropriate. However, in this case we have IIO_VAL_INT_PLUS_NANO
> and IIO_VAL_INT_PLUS_MICRO. Not using "NANO : MICRO" here, and instead
> go with "GIGA : MEGA" is just plain silly, if you ask me.
> 
> So, either "NANO : MICRO" or "1000000000 : 1000000".
> 
> I'm not sold on the macros. I frankly don't see all that much value
> in them and am perfectly fine with raw numbers. To me, it just looks
> like someone has read somewhere that constants should not appear in
> the code, and from that concluded that #define TEN 10 is a good thing
> without thinking very much about it. There is also the possibility
> that someone who has never seen these defines thinks MEGA is 2^20
> instead of 10^6, because that is a much more likely candidate for a
> define in the frist place (not everybody knows all the digits of
> 1048576 by heart and 1 << 20 many times require extra brackets that
> might make it look more complicated than it needs to be).
> 
> Back to this case; the connection to the naming of IIO_VAL_INT_PLUS_NANO
> (and ..._MICRO) makes it ok to use the defines. So if you feel strongly
> about not using "1000000000 : 1000000" I'm ok with that.

I like that the macros make the number of zeros more "obvious" in a
sense, but honestly at this point, I'd rather go back to not using them.
Depending on who you ask, one way makes more sense than the other, at
least with the raw values, there's no ambiguity.

Cheers,
Liam

> Cheers,
> Peter