Re: [PATCH v2 5/5] iio: afe: iio-rescale: Re-use generic struct s32_fract

[Liam Beguin <liambeguin@gmail.com>]

Hi Andy,

On Mon, Jan 24, 2022 at 05:18:32PM +0200, Andy Shevchenko wrote:
> On Sat, Jan 15, 2022 at 06:52:03PM +0000, Jonathan Cameron wrote:
> > On Mon, 10 Jan 2022 21:31:04 +0200
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > 
> > > Instead of custom data type re-use generic struct s32_fract.
> > > No changes intended.
> > > 
> > > The new member is put to be the first one to avoid additional
> > > pointer arithmetic. Besides that one may switch to use fract
> > > member to perform container_of(), which will be no-op in this
> > > case, to get struct rescale.
> > > 
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > 
> > I'm not totally sold on this series showing there is a strong case for
> > these macros so interested to hear what others think.
> 
> So far no news :-)

Like I mentioned briefly in the other thread[1], I don't really see the
advantage for the AFE driver given that it's almost just like renaming
the parameters.

For the other drivers affected by the change, it drops the definition of
the structure which is nice. So overall, it's a plus IMO :-)

[1] https://lore.kernel.org/linux-iio/20220108205319.2046348-1-liambeguin@gmail.com/

Cheers,
Liam

> > Boiler plate removal is always nice of course...
> 
> That's what I considered nice as well.
> 
> ...
> 
> > > I found this better in order how code is structurally (re)organized.
> > > I may rebase this on top of ongoing AFE series.
> > > 
> > > Also reveals possibility to switch to rational best approximation.
> > > But this is another story...
> > 
> > Now that may well justify introducing this shared infrastructure :)
> 
> We also have mult_frac() macro which can be extended by mult_fract() for
> these structures.
> 
> ...
> 
> > >  	rescale = iio_priv(indio_dev);
> > > -
> > > +	rescale->source = source;
> > 
> > There seems to be more reorganizing going on in here than is necessary
> > for the function of this patch. At very least, description should
> > call it out.  Why move setting source?
> 
> Yeah, I agree that this may be in a separate change before of after the series.
> I will split.
> 
> > >  	rescale->cfg = of_device_get_match_data(dev);
> > > -	rescale->numerator = 1;
> > > -	rescale->denominator = 1;
> > >  
> > > -	ret = rescale->cfg->props(dev, rescale);
> > > +	fract = &rescale->fract;
> > > +	fract->numerator = 1;
> > > +	fract->denominator = 1;
> 
> > > -	rescale->source = source;
> > > -
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>