| Message ID | 20220110193104.75225-5-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/5] math.h: Introduce data types for fractional numbers | expand |
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. Boiler plate removal is always nice of course... One trivial comment inline on this one. > --- > > 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 :) > > v2: no changes > > drivers/iio/afe/iio-rescale.c | 74 +++++++++++++++++------------------ > 1 file changed, 37 insertions(+), 37 deletions(-) > > diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c > index 774eb3044edd..0368bca8a485 100644 > --- a/drivers/iio/afe/iio-rescale.c > +++ b/drivers/iio/afe/iio-rescale.c > @@ -11,6 +11,7 @@ > #include <linux/gcd.h> > #include <linux/iio/consumer.h> > #include <linux/iio/iio.h> > +#include <linux/math.h> > #include <linux/module.h> > #include <linux/of.h> > #include <linux/of_device.h> > @@ -21,17 +22,16 @@ struct rescale; > > struct rescale_cfg { > enum iio_chan_type type; > - int (*props)(struct device *dev, struct rescale *rescale); > + int (*props)(struct device *dev, struct s32_fract *fract); > }; > > struct rescale { > + struct s32_fract fract; > const struct rescale_cfg *cfg; > struct iio_channel *source; > struct iio_chan_spec chan; > struct iio_chan_spec_ext_info *ext_info; > bool chan_processed; > - s32 numerator; > - s32 denominator; > }; > > static int rescale_read_raw(struct iio_dev *indio_dev, > @@ -39,6 +39,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > int *val, int *val2, long mask) > { > struct rescale *rescale = iio_priv(indio_dev); > + struct s32_fract *fract = &rescale->fract; > unsigned long long tmp; > int ret; > > @@ -67,19 +68,19 @@ static int rescale_read_raw(struct iio_dev *indio_dev, > } > switch (ret) { > case IIO_VAL_FRACTIONAL: > - *val *= rescale->numerator; > - *val2 *= rescale->denominator; > + *val *= fract->numerator; > + *val2 *= fract->denominator; > return ret; > case IIO_VAL_INT: > - *val *= rescale->numerator; > - if (rescale->denominator == 1) > + *val *= fract->numerator; > + if (fract->denominator == 1) > return ret; > - *val2 = rescale->denominator; > + *val2 = fract->denominator; > return IIO_VAL_FRACTIONAL; > case IIO_VAL_FRACTIONAL_LOG2: > tmp = *val * 1000000000LL; > - do_div(tmp, rescale->denominator); > - tmp *= rescale->numerator; > + do_div(tmp, fract->denominator); > + tmp *= fract->numerator; > do_div(tmp, 1000000000LL); > *val = tmp; > return ret; > @@ -175,7 +176,7 @@ static int rescale_configure_channel(struct device *dev, > } > > static int rescale_current_sense_amplifier_props(struct device *dev, > - struct rescale *rescale) > + struct s32_fract *fract) > { > u32 sense; > u32 gain_mult = 1; > @@ -199,22 +200,22 @@ static int rescale_current_sense_amplifier_props(struct device *dev, > * numerator/denominator from overflowing. > */ > factor = gcd(sense, 1000000); > - rescale->numerator = 1000000 / factor; > - rescale->denominator = sense / factor; > + fract->numerator = 1000000 / factor; > + fract->denominator = sense / factor; > > - factor = gcd(rescale->numerator, gain_mult); > - rescale->numerator /= factor; > - rescale->denominator *= gain_mult / factor; > + factor = gcd(fract->numerator, gain_mult); > + fract->numerator /= factor; > + fract->denominator *= gain_mult / factor; > > - factor = gcd(rescale->denominator, gain_div); > - rescale->numerator *= gain_div / factor; > - rescale->denominator /= factor; > + factor = gcd(fract->denominator, gain_div); > + fract->numerator *= gain_div / factor; > + fract->denominator /= factor; > > return 0; > } > > static int rescale_current_sense_shunt_props(struct device *dev, > - struct rescale *rescale) > + struct s32_fract *fract) > { > u32 shunt; > u32 factor; > @@ -228,35 +229,33 @@ static int rescale_current_sense_shunt_props(struct device *dev, > } > > factor = gcd(shunt, 1000000); > - rescale->numerator = 1000000 / factor; > - rescale->denominator = shunt / factor; > + fract->numerator = 1000000 / factor; > + fract->denominator = shunt / factor; > > return 0; > } > > static int rescale_voltage_divider_props(struct device *dev, > - struct rescale *rescale) > + struct s32_fract *fract) > { > int ret; > u32 factor; > > - ret = device_property_read_u32(dev, "output-ohms", > - &rescale->denominator); > + ret = device_property_read_u32(dev, "output-ohms", &fract->denominator); > if (ret) { > dev_err(dev, "failed to read output-ohms: %d\n", ret); > return ret; > } > > - ret = device_property_read_u32(dev, "full-ohms", > - &rescale->numerator); > + ret = device_property_read_u32(dev, "full-ohms", &fract->numerator); > if (ret) { > dev_err(dev, "failed to read full-ohms: %d\n", ret); > return ret; > } > > - factor = gcd(rescale->numerator, rescale->denominator); > - rescale->numerator /= factor; > - rescale->denominator /= factor; > + factor = gcd(fract->numerator, fract->denominator); > + fract->numerator /= factor; > + fract->denominator /= factor; > > return 0; > } > @@ -299,6 +298,7 @@ static int rescale_probe(struct platform_device *pdev) > struct iio_dev *indio_dev; > struct iio_channel *source; > struct rescale *rescale; > + struct s32_fract *fract; > int sizeof_ext_info; > int sizeof_priv; > int i; > @@ -322,24 +322,24 @@ static int rescale_probe(struct platform_device *pdev) > return -ENOMEM; > > 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? > 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; > + > + ret = rescale->cfg->props(dev, fract); > if (ret) > return ret; > > - if (!rescale->numerator || !rescale->denominator) { > + if (!fract->numerator || !fract->denominator) { > dev_err(dev, "invalid scaling factor.\n"); > return -EINVAL; > } > > platform_set_drvdata(pdev, indio_dev); > > - rescale->source = source; > - > indio_dev->name = dev_name(dev); > indio_dev->info = &rescale_info; > indio_dev->modes = INDIO_DIRECT_MODE;
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 :-) > 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; > > -
On Mon, Jan 24, 2022 at 05:18:33PM +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: > > 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 :-) > > > Boiler plate removal is always nice of course... > > That's what I considered nice as well. Ah, and thanks for review!
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 > >
On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote: > 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. I tend to disagree, perhaps I wasn't so clear in my points. The change reveals that the layering can be improved. In OOP the object A which is inherited (or encapsulated as we see here) allows to clearly get what kind of data the methods are operating with / on. As you may see the two functions and the method declaration appears to have interest only in the fraction data for rescaling. The cleanup I consider helpful in the terms of layering and OOP. > 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 :-) Thanks! > [1] https://lore.kernel.org/linux-iio/20220108205319.2046348-1-liambeguin@gmail.com/
On 2022-01-25 14:17, Andy Shevchenko wrote: > On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote: >> 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. > > I tend to disagree, perhaps I wasn't so clear in my points. > > The change reveals that the layering can be improved. In OOP > the object A which is inherited (or encapsulated as we see here) > allows to clearly get what kind of data the methods are operating > with / on. As you may see the two functions and the method > declaration appears to have interest only in the fraction data > for rescaling. The cleanup I consider helpful in the terms > of layering and OOP. Hi! [Sorry for the delay, I have been far too busy for far too long] While this is all true for the current set of front-ends, it is not all that far-fetched to imagine some kind of future front-end that requires some other parameter, such that the rescaling fraction is no longer the only thing of interest. So, I have the feeling that changing the type of the 2nd argument of the ->props functions to just the fraction instead of the bigger object is conceptually wrong and something that will later turn out to have been a bad idea. Regarding the new xyz_fract types, I have no strong opinion. But as long as there are no helper functions for the new types, I see little value in them. To me, they look mostly like something that newcomers (and others) will not know about or expect, and it will just be another thing that you have to look out for during review, to keep new numerators/denominators from appearing and causing extra rounds of review for something that is mostly a bikeshed issue. My guess is that many times where fractions are used, they are used since fp math is not available. And that means that there will be a lot of special handling and shortcuts done since various things about accuracy and precision can be assumed. I think it will be hard to do all that centrally and in a comprehensible way. But if it is done it will of course also be possible to eliminate bugs since people may have taken too many shortcuts. But simply changing to xyz_fract and then not take it further than that will not change much. Also, there is already a v4l2_fract which is exposed in UAPI (maybe there are others?). I don't see how we bring that one in line with this new struct xyz_fract scheme? Cheers, Peter
On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote: > On 2022-01-25 14:17, Andy Shevchenko wrote: > > On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote: > >> 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: ... > >>>> 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. > > > > I tend to disagree, perhaps I wasn't so clear in my points. > > > > The change reveals that the layering can be improved. In OOP > > the object A which is inherited (or encapsulated as we see here) > > allows to clearly get what kind of data the methods are operating > > with / on. As you may see the two functions and the method > > declaration appears to have interest only in the fraction data > > for rescaling. The cleanup I consider helpful in the terms > > of layering and OOP. > [Sorry for the delay, I have been far too busy for far too long] Anyway, thanks for review! My answers below. > While this is all true for the current set of front-ends, it is not > all that far-fetched to imagine some kind of future front-end that > requires some other parameter, such that the rescaling fraction is no > longer the only thing of interest. So, I have the feeling that changing > the type of the 2nd argument of the ->props functions to just the > fraction instead of the bigger object is conceptually wrong and > something that will later turn out to have been a bad idea. How? Technically as I mentioned currently it's the case to use only the date from fraction. The bigger object would be needed in case of using data that is not fraction. Either way it would be easy to add a container_of() than supply unneeded data to the method. TL;DR: It makes possible not to mix bananas with wooden boxes. > Regarding the new xyz_fract types, I have no strong opinion. But as > long as there are no helper functions for the new types, I see little > value in them. To me, they look mostly like something that newcomers > (and others) will not know about or expect, and it will just be > another thing that you have to look out for during review, to keep new > numerators/denominators from appearing and causing extra rounds of > review for something that is mostly a bikeshed issue. > > My guess is that many times where fractions are used, they are used > since fp math is not available. And that means that there will be a > lot of special handling and shortcuts done since various things about > accuracy and precision can be assumed. I think it will be hard to do > all that centrally and in a comprehensible way. But if it is done it > will of course also be possible to eliminate bugs since people may > have taken too many shortcuts. But simply changing to xyz_fract and > then not take it further than that will not change much. I understand your point. I will provide a mult_fract() macro and apply it to AFE to show the usability of this. Would it work better? I haven't checked the other possible improvements based on new type, but I truly believe that the types themselves are good to have. > Also, there is already a v4l2_fract which is exposed in UAPI (maybe > there are others?). I don't see how we bring that one in line with this > new struct xyz_fract scheme? UAPI is another story. It might be possible to extend, but I would like to avoid expanding the proposed types to UAPI (we don't have many users and I believe they more or less unique, so v4l2 is rather exception than usual).
On 2022-01-25 19:17, Andy Shevchenko wrote: > On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote: >> On 2022-01-25 14:17, Andy Shevchenko wrote: >>> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote: >>>> 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: > > ... > >>>>>> 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. >>> >>> I tend to disagree, perhaps I wasn't so clear in my points. >>> >>> The change reveals that the layering can be improved. In OOP >>> the object A which is inherited (or encapsulated as we see here) >>> allows to clearly get what kind of data the methods are operating >>> with / on. As you may see the two functions and the method >>> declaration appears to have interest only in the fraction data >>> for rescaling. The cleanup I consider helpful in the terms >>> of layering and OOP. > >> [Sorry for the delay, I have been far too busy for far too long] > > Anyway, thanks for review! My answers below. > >> While this is all true for the current set of front-ends, it is not >> all that far-fetched to imagine some kind of future front-end that >> requires some other parameter, such that the rescaling fraction is no >> longer the only thing of interest. So, I have the feeling that changing >> the type of the 2nd argument of the ->props functions to just the >> fraction instead of the bigger object is conceptually wrong and >> something that will later turn out to have been a bad idea. > > How? Technically as I mentioned currently it's the case to use > only the date from fraction. The bigger object would be needed > in case of using data that is not fraction. Either way it would > be easy to add a container_of() than supply unneeded data to > the method. It's easy to both remove and to add back "the bigger object". I just don't see the point of the churn. Technically you can probably rearrange stuff in probe and remove the 2nd argument to ->props() altogether and chase pointers from the dev object instead. I don't see the point of that either. It doesn't really make things simpler, it doesn't really make things easier to read. To me, it's just pointless churn. > > TL;DR: It makes possible not to mix bananas with wooden boxes. Which is all good until you need to ship an apple in the box with the bananas. (e.g. if you for some reason need the bananas to get ripe real quick, apples produce ethylene) > >> Regarding the new xyz_fract types, I have no strong opinion. But as >> long as there are no helper functions for the new types, I see little >> value in them. To me, they look mostly like something that newcomers >> (and others) will not know about or expect, and it will just be >> another thing that you have to look out for during review, to keep new >> numerators/denominators from appearing and causing extra rounds of >> review for something that is mostly a bikeshed issue. >> >> My guess is that many times where fractions are used, they are used >> since fp math is not available. And that means that there will be a >> lot of special handling and shortcuts done since various things about >> accuracy and precision can be assumed. I think it will be hard to do >> all that centrally and in a comprehensible way. But if it is done it >> will of course also be possible to eliminate bugs since people may >> have taken too many shortcuts. But simply changing to xyz_fract and >> then not take it further than that will not change much. > > I understand your point. I will provide a mult_fract() macro and > apply it to AFE to show the usability of this. Would it work better? In my experience, burying stuff in macros will make it harder to follow what is really happening. Something that has proven hard as-is in this driver. While reviewing the changes from Liam, I have repeatedly looked up the various division variants to find out what they are actually doing. Here, it's not the individual steps that are difficult. I feel that if we start adding macros for fractions they will soon multiply and I'm not really looking forward to also have a set of similar fraction macros to juggle. But sure, feel free to suggest something. But please hold until the current work from Liam is merged. That series is clearly more important, and I'm not really interested in neither adding more work for him nor a cleanup of the current code without those pending changes. Cheers, Peter
On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: > On 2022-01-25 19:17, Andy Shevchenko wrote: > > On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote: > >> On 2022-01-25 14:17, Andy Shevchenko wrote: > >>> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote: > >>>> 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: ... > >>>>>> 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. > >>> > >>> I tend to disagree, perhaps I wasn't so clear in my points. > >>> > >>> The change reveals that the layering can be improved. In OOP > >>> the object A which is inherited (or encapsulated as we see here) > >>> allows to clearly get what kind of data the methods are operating > >>> with / on. As you may see the two functions and the method > >>> declaration appears to have interest only in the fraction data > >>> for rescaling. The cleanup I consider helpful in the terms > >>> of layering and OOP. > > > >> [Sorry for the delay, I have been far too busy for far too long] > > > > Anyway, thanks for review! My answers below. > > > >> While this is all true for the current set of front-ends, it is not > >> all that far-fetched to imagine some kind of future front-end that > >> requires some other parameter, such that the rescaling fraction is no > >> longer the only thing of interest. So, I have the feeling that changing > >> the type of the 2nd argument of the ->props functions to just the > >> fraction instead of the bigger object is conceptually wrong and > >> something that will later turn out to have been a bad idea. > > > > How? Technically as I mentioned currently it's the case to use > > only the date from fraction. The bigger object would be needed > > in case of using data that is not fraction. Either way it would > > be easy to add a container_of() than supply unneeded data to > > the method. > > It's easy to both remove and to add back "the bigger object". I just > don't see the point of the churn. Technically you can probably rearrange > stuff in probe and remove the 2nd argument to ->props() altogether and > chase pointers from the dev object instead. I don't see the point of > that either. It doesn't really make things simpler, it doesn't really > make things easier to read. To me, it's just pointless churn. Since you still haven't got a point the conclusions are wrong. The point is (I dunno how more clear to make it) is to have proper layering from the (current) design perspective. If we go to the road full of "if it will come XYZ then this sucks". The future is uncertain and neither of us may prove the current change good or bad in terms of the future (unknown and uncertain) changes. Preventing this patch to land is to tell "Oh, my design is bad, but I will keep it, because in the future everything may change". So, why don't you make this future to be now? > > TL;DR: It makes possible not to mix bananas with wooden boxes. > > Which is all good until you need to ship an apple in the box with the > bananas. (e.g. if you for some reason need the bananas to get ripe real > quick, apples produce ethylene) Really. arguments about the future changes are weak. If you have patches in mind, send them, We will see in practice what you meant. > >> Regarding the new xyz_fract types, I have no strong opinion. But as > >> long as there are no helper functions for the new types, I see little > >> value in them. To me, they look mostly like something that newcomers > >> (and others) will not know about or expect, and it will just be > >> another thing that you have to look out for during review, to keep new > >> numerators/denominators from appearing and causing extra rounds of > >> review for something that is mostly a bikeshed issue. > >> > >> My guess is that many times where fractions are used, they are used > >> since fp math is not available. And that means that there will be a > >> lot of special handling and shortcuts done since various things about > >> accuracy and precision can be assumed. I think it will be hard to do > >> all that centrally and in a comprehensible way. But if it is done it > >> will of course also be possible to eliminate bugs since people may > >> have taken too many shortcuts. But simply changing to xyz_fract and > >> then not take it further than that will not change much. > > > > I understand your point. I will provide a mult_fract() macro and > > apply it to AFE to show the usability of this. Would it work better? > > In my experience, burying stuff in macros will make it harder to follow > what is really happening. Something that has proven hard as-is in this > driver. While reviewing the changes from Liam, I have repeatedly looked > up the various division variants to find out what they are actually > doing. Here, it's not the individual steps that are difficult. I feel > that if we start adding macros for fractions they will soon multiply and > I'm not really looking forward to also have a set of similar fraction > macros to juggle. The problem here is that every driver would like to do this differently and since it's related to the calculation we will have all possible error prone implementations which do miscalculations (yes, one may not notice traditional off-by-one until it becomes a huge issue by using additional conversion formulas or so). > But sure, feel free to suggest something. But please hold until the > current work from Liam is merged. > That series is clearly more > important, and I'm not really interested in neither adding more work for > him nor a cleanup of the current code without those pending changes. I'm very well fine with that. As I mentioned from the beginning, I may rebase this on top of the Liam's work.
On 2022-01-26 13:04, Andy Shevchenko wrote: > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: >> It's easy to both remove and to add back "the bigger object". I just >> don't see the point of the churn. Technically you can probably rearrange >> stuff in probe and remove the 2nd argument to ->props() altogether and >> chase pointers from the dev object instead. I don't see the point of >> that either. It doesn't really make things simpler, it doesn't really >> make things easier to read. To me, it's just pointless churn. > > Since you still haven't got a point the conclusions are wrong. > The point is (I dunno how more clear to make it) is to have proper > layering from the (current) design perspective. I think got the gist of it. I simply do not agree with your conclusion about what the "proper layering" should be. Cheers, Peter
On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote: > On 2022-01-26 13:04, Andy Shevchenko wrote: > > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: > >> It's easy to both remove and to add back "the bigger object". I just > >> don't see the point of the churn. Technically you can probably rearrange > >> stuff in probe and remove the 2nd argument to ->props() altogether and > >> chase pointers from the dev object instead. I don't see the point of > >> that either. It doesn't really make things simpler, it doesn't really > >> make things easier to read. To me, it's just pointless churn. > > > > Since you still haven't got a point the conclusions are wrong. > > The point is (I dunno how more clear to make it) is to have proper > > layering from the (current) design perspective. > > I think got the gist of it. I simply do not agree with your conclusion > about what the "proper layering" should be. And I see no real argument against it. With the patch applied I see a better structure of the code and exactly necessary data to be passed to the method. Which makes me think that current implementation is either a leftover or was something like "let's take a bigger object _just in case_", which I can't take as a proper layering.
Hi Andy, On Wed, Jan 26, 2022 at 02:04:53PM +0200, Andy Shevchenko wrote: > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: > > On 2022-01-25 19:17, Andy Shevchenko wrote: > > > On Tue, Jan 25, 2022 at 03:54:07PM +0100, Peter Rosin wrote: > > >> On 2022-01-25 14:17, Andy Shevchenko wrote: > > >>> On Mon, Jan 24, 2022 at 04:28:09PM -0500, Liam Beguin wrote: > > >>>> 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: > > ... ... > The problem here is that every driver would like to do this differently > and since it's related to the calculation we will have all possible error > prone implementations which do miscalculations (yes, one may not notice > traditional off-by-one until it becomes a huge issue by using additional > conversion formulas or so). > > > But sure, feel free to suggest something. But please hold until the > > current work from Liam is merged. > > That series is clearly more > > important, and I'm not really interested in neither adding more work for > > him nor a cleanup of the current code without those pending changes. > > I'm very well fine with that. As I mentioned from the beginning, I may rebase > this on top of the Liam's work. I appreciate that! I'll make time to wrap things up so I don't hold you up. Cheers, Liam > -- > With Best Regards, > Andy Shevchenko > >
On 2022-01-26 14:01, Andy Shevchenko wrote: > On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote: >> On 2022-01-26 13:04, Andy Shevchenko wrote: >>> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: >>>> It's easy to both remove and to add back "the bigger object". I just >>>> don't see the point of the churn. Technically you can probably rearrange >>>> stuff in probe and remove the 2nd argument to ->props() altogether and >>>> chase pointers from the dev object instead. I don't see the point of >>>> that either. It doesn't really make things simpler, it doesn't really >>>> make things easier to read. To me, it's just pointless churn. >>> >>> Since you still haven't got a point the conclusions are wrong. >>> The point is (I dunno how more clear to make it) is to have proper >>> layering from the (current) design perspective. >> >> I think got the gist of it. I simply do not agree with your conclusion >> about what the "proper layering" should be. > > And I see no real argument against it. With the patch applied I see > a better structure of the code and exactly necessary data to be passed > to the method. Which makes me think that current implementation is > either a leftover or was something like "let's take a bigger object > _just in case_", which I can't take as a proper layering. The bigger object, or the one and only object as the current code is written, is given to ->props() by design. BTW, you don't seem to understand the ->props() functions. There is no data "passed to" the ->props() functions. These functions instead fill in properties. Currently this boils down to the scaling fraction, but I can imagine other properties. On 2022-01-25 19:17, Andy Shevchenko wrote: > The bigger object would be needed > in case of using data that is not fraction. Either way it would > be easy to add a container_of() than supply unneeded data to > the method. You argued that it is easy to "break out" to the bigger object in case it's needed. Which in my book is a sign of poor layering. It's way easier to *not* change things, it's perfectly fine as-is. The argument against making the change you propose is that it does not make this small driver any easier to understand. It would still just change things for the sake of changing them, and I do not see the point of erasing the existing future-proofing when it has no cost. To sum up, I'm ok with introducing fract_s32 in this driver, but I don't agree with the signature change of ->props(). Cheers, Peter
On 2022-01-26 13:04, Andy Shevchenko wrote: > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: >> It's easy to both remove and to add back "the bigger object". I just >> don't see the point of the churn. Technically you can probably rearrange >> stuff in probe and remove the 2nd argument to ->props() altogether and >> chase pointers from the dev object instead. I don't see the point of >> that either. It doesn't really make things simpler, it doesn't really >> make things easier to read. To me, it's just pointless churn. > > Since you still haven't got a point the conclusions are wrong. > The point is (I dunno how more clear to make it) is to have proper > layering from the (current) design perspective. > > If we go to the road full of "if it will come XYZ then this sucks". > The future is uncertain and neither of us may prove the current > change good or bad in terms of the future (unknown and uncertain) > changes. > > Preventing this patch to land is to tell "Oh, my design is bad, > but I will keep it, because in the future everything may change". > So, why don't you make this future to be now? > >>> TL;DR: It makes possible not to mix bananas with wooden boxes. >> >> Which is all good until you need to ship an apple in the box with the >> bananas. (e.g. if you for some reason need the bananas to get ripe real >> quick, apples produce ethylene) > > Really. arguments about the future changes are weak. If you have > patches in mind, send them, We will see in practice what you meant. I can do one better - here are links to patches from 7-8 months ago. https://lore.kernel.org/lkml/20210530005917.20953-1-liambeguin@gmail.com/ https://lore.kernel.org/lkml/20210530005917.20953-6-liambeguin@gmail.com/ Or, if you prefer, the latest revisions. https://lore.kernel.org/lkml/20220108205319.2046348-9-liambeguin@gmail.com/ https://lore.kernel.org/lkml/20220108205319.2046348-14-liambeguin@gmail.com/ You have made review comments on that series. My previous arguments were based on gut feel, and I'm sorry for not thinking of the offset in the referred series before. Cheers, Peter
On Thu, Jan 27, 2022 at 12:03:49PM +0100, Peter Rosin wrote: > On 2022-01-26 14:01, Andy Shevchenko wrote: > > On Wed, Jan 26, 2022 at 01:35:09PM +0100, Peter Rosin wrote: > >> On 2022-01-26 13:04, Andy Shevchenko wrote: > >>> On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: > >>>> It's easy to both remove and to add back "the bigger object". I just > >>>> don't see the point of the churn. Technically you can probably rearrange > >>>> stuff in probe and remove the 2nd argument to ->props() altogether and > >>>> chase pointers from the dev object instead. I don't see the point of > >>>> that either. It doesn't really make things simpler, it doesn't really > >>>> make things easier to read. To me, it's just pointless churn. > >>> > >>> Since you still haven't got a point the conclusions are wrong. > >>> The point is (I dunno how more clear to make it) is to have proper > >>> layering from the (current) design perspective. > >> > >> I think got the gist of it. I simply do not agree with your conclusion > >> about what the "proper layering" should be. > > > > And I see no real argument against it. With the patch applied I see > > a better structure of the code and exactly necessary data to be passed > > to the method. Which makes me think that current implementation is > > either a leftover or was something like "let's take a bigger object > > _just in case_", which I can't take as a proper layering. > > The bigger object, or the one and only object as the current code is > written, is given to ->props() by design. > > BTW, you don't seem to understand the ->props() functions. There is no > data "passed to" the ->props() functions. These functions instead fill > in properties. Currently this boils down to the scaling fraction, but I > can imagine other properties. Currently the object of the properties is the same as struct __Txx_fract. In the future it may indeed be expanded. In such case I see that the layering might look like struct ..._props { struct __Txx_fract fract; ... }; Of course it depends on the properties themselves, but at least that's how I believe the OOP paradigm works. Am I mistaken? > On 2022-01-25 19:17, Andy Shevchenko wrote: > > The bigger object would be needed > > in case of using data that is not fraction. Either way it would > > be easy to add a container_of() than supply unneeded data to > > the method. > > You argued that it is easy to "break out" to the bigger object in case > it's needed. Which in my book is a sign of poor layering. > It's way easier to *not* change things, it's perfectly fine as-is. > > The argument against making the change you propose is that it does not > make this small driver any easier to understand. It would still just > change things for the sake of changing them, and I do not see the point > of erasing the existing future-proofing when it has no cost. > > To sum up, I'm ok with introducing fract_s32 in this driver, but I > don't agree with the signature change of ->props(). Thanks for valuable comments! I postponed the change in any case, let Liam to finish his part first, which we agreed is more important.
On Thu, Jan 27, 2022 at 01:11:14PM +0100, Peter Rosin wrote: > On 2022-01-26 13:04, Andy Shevchenko wrote: > > On Wed, Jan 26, 2022 at 11:26:50AM +0100, Peter Rosin wrote: > >> It's easy to both remove and to add back "the bigger object". I just > >> don't see the point of the churn. Technically you can probably rearrange > >> stuff in probe and remove the 2nd argument to ->props() altogether and > >> chase pointers from the dev object instead. I don't see the point of > >> that either. It doesn't really make things simpler, it doesn't really > >> make things easier to read. To me, it's just pointless churn. > > > > Since you still haven't got a point the conclusions are wrong. > > The point is (I dunno how more clear to make it) is to have proper > > layering from the (current) design perspective. > > > > If we go to the road full of "if it will come XYZ then this sucks". > > The future is uncertain and neither of us may prove the current > > change good or bad in terms of the future (unknown and uncertain) > > changes. > > > > Preventing this patch to land is to tell "Oh, my design is bad, > > but I will keep it, because in the future everything may change". > > So, why don't you make this future to be now? > > > >>> TL;DR: It makes possible not to mix bananas with wooden boxes. > >> > >> Which is all good until you need to ship an apple in the box with the > >> bananas. (e.g. if you for some reason need the bananas to get ripe real > >> quick, apples produce ethylene) > > > > Really. arguments about the future changes are weak. If you have > > patches in mind, send them, We will see in practice what you meant. > > I can do one better - here are links to patches from 7-8 months ago. > > https://lore.kernel.org/lkml/20210530005917.20953-1-liambeguin@gmail.com/ > https://lore.kernel.org/lkml/20210530005917.20953-6-liambeguin@gmail.com/ > > Or, if you prefer, the latest revisions. > > https://lore.kernel.org/lkml/20220108205319.2046348-9-liambeguin@gmail.com/ > https://lore.kernel.org/lkml/20220108205319.2046348-14-liambeguin@gmail.com/ > > You have made review comments on that series. > > My previous arguments were based on gut feel, and I'm sorry for not > thinking of the offset in the referred series before. No problem and thanks for your comments!
diff --git a/drivers/iio/afe/iio-rescale.c b/drivers/iio/afe/iio-rescale.c index 774eb3044edd..0368bca8a485 100644 --- a/drivers/iio/afe/iio-rescale.c +++ b/drivers/iio/afe/iio-rescale.c @@ -11,6 +11,7 @@ #include <linux/gcd.h> #include <linux/iio/consumer.h> #include <linux/iio/iio.h> +#include <linux/math.h> #include <linux/module.h> #include <linux/of.h> #include <linux/of_device.h> @@ -21,17 +22,16 @@ struct rescale; struct rescale_cfg { enum iio_chan_type type; - int (*props)(struct device *dev, struct rescale *rescale); + int (*props)(struct device *dev, struct s32_fract *fract); }; struct rescale { + struct s32_fract fract; const struct rescale_cfg *cfg; struct iio_channel *source; struct iio_chan_spec chan; struct iio_chan_spec_ext_info *ext_info; bool chan_processed; - s32 numerator; - s32 denominator; }; static int rescale_read_raw(struct iio_dev *indio_dev, @@ -39,6 +39,7 @@ static int rescale_read_raw(struct iio_dev *indio_dev, int *val, int *val2, long mask) { struct rescale *rescale = iio_priv(indio_dev); + struct s32_fract *fract = &rescale->fract; unsigned long long tmp; int ret; @@ -67,19 +68,19 @@ static int rescale_read_raw(struct iio_dev *indio_dev, } switch (ret) { case IIO_VAL_FRACTIONAL: - *val *= rescale->numerator; - *val2 *= rescale->denominator; + *val *= fract->numerator; + *val2 *= fract->denominator; return ret; case IIO_VAL_INT: - *val *= rescale->numerator; - if (rescale->denominator == 1) + *val *= fract->numerator; + if (fract->denominator == 1) return ret; - *val2 = rescale->denominator; + *val2 = fract->denominator; return IIO_VAL_FRACTIONAL; case IIO_VAL_FRACTIONAL_LOG2: tmp = *val * 1000000000LL; - do_div(tmp, rescale->denominator); - tmp *= rescale->numerator; + do_div(tmp, fract->denominator); + tmp *= fract->numerator; do_div(tmp, 1000000000LL); *val = tmp; return ret; @@ -175,7 +176,7 @@ static int rescale_configure_channel(struct device *dev, } static int rescale_current_sense_amplifier_props(struct device *dev, - struct rescale *rescale) + struct s32_fract *fract) { u32 sense; u32 gain_mult = 1; @@ -199,22 +200,22 @@ static int rescale_current_sense_amplifier_props(struct device *dev, * numerator/denominator from overflowing. */ factor = gcd(sense, 1000000); - rescale->numerator = 1000000 / factor; - rescale->denominator = sense / factor; + fract->numerator = 1000000 / factor; + fract->denominator = sense / factor; - factor = gcd(rescale->numerator, gain_mult); - rescale->numerator /= factor; - rescale->denominator *= gain_mult / factor; + factor = gcd(fract->numerator, gain_mult); + fract->numerator /= factor; + fract->denominator *= gain_mult / factor; - factor = gcd(rescale->denominator, gain_div); - rescale->numerator *= gain_div / factor; - rescale->denominator /= factor; + factor = gcd(fract->denominator, gain_div); + fract->numerator *= gain_div / factor; + fract->denominator /= factor; return 0; } static int rescale_current_sense_shunt_props(struct device *dev, - struct rescale *rescale) + struct s32_fract *fract) { u32 shunt; u32 factor; @@ -228,35 +229,33 @@ static int rescale_current_sense_shunt_props(struct device *dev, } factor = gcd(shunt, 1000000); - rescale->numerator = 1000000 / factor; - rescale->denominator = shunt / factor; + fract->numerator = 1000000 / factor; + fract->denominator = shunt / factor; return 0; } static int rescale_voltage_divider_props(struct device *dev, - struct rescale *rescale) + struct s32_fract *fract) { int ret; u32 factor; - ret = device_property_read_u32(dev, "output-ohms", - &rescale->denominator); + ret = device_property_read_u32(dev, "output-ohms", &fract->denominator); if (ret) { dev_err(dev, "failed to read output-ohms: %d\n", ret); return ret; } - ret = device_property_read_u32(dev, "full-ohms", - &rescale->numerator); + ret = device_property_read_u32(dev, "full-ohms", &fract->numerator); if (ret) { dev_err(dev, "failed to read full-ohms: %d\n", ret); return ret; } - factor = gcd(rescale->numerator, rescale->denominator); - rescale->numerator /= factor; - rescale->denominator /= factor; + factor = gcd(fract->numerator, fract->denominator); + fract->numerator /= factor; + fract->denominator /= factor; return 0; } @@ -299,6 +298,7 @@ static int rescale_probe(struct platform_device *pdev) struct iio_dev *indio_dev; struct iio_channel *source; struct rescale *rescale; + struct s32_fract *fract; int sizeof_ext_info; int sizeof_priv; int i; @@ -322,24 +322,24 @@ static int rescale_probe(struct platform_device *pdev) return -ENOMEM; rescale = iio_priv(indio_dev); - + rescale->source = source; 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; + + ret = rescale->cfg->props(dev, fract); if (ret) return ret; - if (!rescale->numerator || !rescale->denominator) { + if (!fract->numerator || !fract->denominator) { dev_err(dev, "invalid scaling factor.\n"); return -EINVAL; } platform_set_drvdata(pdev, indio_dev); - rescale->source = source; - indio_dev->name = dev_name(dev); indio_dev->info = &rescale_info; indio_dev->modes = INDIO_DIRECT_MODE;
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 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... v2: no changes drivers/iio/afe/iio-rescale.c | 74 +++++++++++++++++------------------ 1 file changed, 37 insertions(+), 37 deletions(-)