Re: [PATCH 1/3] iio: max31856: add option for setting mains filter rejection frequency

[Andrea Merello <andrea.merello@gmail.com>]

Il giorno sab 2 nov 2019 alle ore 15:17 Jonathan Cameron
<jic23@kernel.org> ha scritto:
>
> On Mon, 28 Oct 2019 08:32:48 +0100
> Andrea Merello <andrea.merello@gmail.com> wrote:
>
> > Il giorno dom 27 ott 2019 alle ore 10:23 Jonathan Cameron
> > <jic23@kernel.org> ha scritto:
> > >
> > > On Wed, 23 Oct 2019 10:29:07 +0200
> > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > >
> > > > Il giorno mar 22 ott 2019 alle ore 11:35 Jonathan Cameron
> > > > <jic23@kernel.org> ha scritto:
> > > > >
> > > > > On Fri, 18 Oct 2019 15:46:32 +0200
> > > > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > > > >
> > > > > > Il giorno gio 17 ott 2019 alle ore 14:32 Jonathan Cameron
> > > > > > <jonathan.cameron@huawei.com> ha scritto:
> > > > > > >
> > > > > > > On Wed, 16 Oct 2019 15:14:20 +0200
> > > > > > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > > > > > >
> > > > > > > > Il giorno dom 6 ott 2019 alle ore 09:54 Jonathan Cameron
> > > > > > > > <jic23@kernel.org> ha scritto:
> > > > > > > > >
> > > > > > > > > On Mon, 23 Sep 2019 14:17:12 +0200
> > > > > > > > > Andrea Merello <andrea.merello@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > > This sensor has an embedded notch filter for reducing interferences caused
> > > > > > > > > > by the power mains. This filter can be tuned to reject either 50Hz or 60Hz
> > > > > > > > > > (and harmonics).
> > > > > > > > > >
> > > > > > > > > > Currently the said setting is left alone (the sensor defaults to 60Hz).
> > > > > > > > > > This patch introduces a IIO attribute that allows the user to set the said
> > > > > > > > > > filter to the desired frequency.
> > > > > > > > > >
> > > > > > > > > > NOTE: this has been intentionally not tied to any DT property to allow
> > > > > > > > > > the configuration of this setting from userspace, e.g. with a GUI or by
> > > > > > > > > > reading a configuration file, or maybe reading a GPIO tied to a physical
> > > > > > > > > > switch or accessing some device that can autodetect the line frequency.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com>
> > > > > > > > > This one is not something that can be expect to be known from the setup
> > > > > > > > > of the device as it will depend on local mains frequency.
> > > > > > > > >
> > > > > > > > > So fine, to have it as a userspace control, but the name is too generic.
> > > > > > > > > We already have a number of filter attributes and we should try to
> > > > > > > > > work out how to bring it inline with them.
> > > > > > > >
> > > > > > > > Sure
> > > > > > > >
> > > > > > > > > in_X_filter_low_pass_3db_frequency
> > > > > > > > > in_X_filter_high_pass_3db_frequency
> > > > > > > > >
> > > > > > > > > So would,
> > > > > > > > > in_X_filter_notch_center_frequency work?
> > > > > > > > > ( I suppose we should use the American spelling ;)
> > > > > > > >
> > > > > > > > Yes, it seems OK in this case. I will produce a V2 with this modification.
> > > > > > > >
> > > > > > > > > This kind of ignores the harmonics aspect but at least documents the
> > > > > > > > > main frequency being blocked.
> > > > > > > >
> > > > > > > > I think this is perfectly fine: the user wants to set the notch filter
> > > > > > > > center frequency to either 60Hz or 50Hz to match the line frequency,
> > > > > > > > then she/he expects the filter to simply "work" somehow; IMO the
> > > > > > > > harmonic thing does not needed to be explicit.
> > > > > > > >
> > > > > > > > > There is a slight complexity that we have devices that have dual
> > > > > > > > > notchfilters (50 and 60Hz) and ones where you can turn it off entirely.
> > > > > > > > >
> > > > > > > > > I suppose no value would count as off and we could perhaps use a list
> > > > > > > > > for both on at the same time (though that's a bit horrible).
> > > > > > > >
> > > > > > > > IMHO it seems reasonable. Maybe for all-off and both-on conditions we
> > > > > > > > could also use magic strings like i.e. "all" and "off".
> > > > > > >
> > > > > > > I go with 'maybe' on that one.  Need to think about whether that is just
> > > > > > > a partial solution as we will probably find a device with 3 options that only
> > > > > > > supports any 2 at one time.  That would still need a more complex interface.
> > > > > > >
> > > > > > > Will think on this.
> > > > > >
> > > > > > I'll keep this patch on hold, waiting for your thoughts. Take the time
> > > > > > you need :)
> > > > > >
> > > > > > BTW IMHO:
> > > > > >
> > > > > > If we want to address the most possible generic case, then we may
> > > > > > standardize a set of possible attributes for filters (like "enable",
> > > > > > "center_frequency", "width",  "Q" , etc). Of course most filters will
> > > > > > not allow for setting most of those attributes.
> > > > >
> > > > > Absolutely.  We currently have a few defined for low and high pass
> > > > > filters, but if there are more complex features to define we should
> > > > > do so.
> > > > >
> > > > > >
> > > > > > Then i.e.  in our case we could have one single filter that allows for
> > > > > > setting the frequency to either 50hz or 60hz; in other cases we could
> > > > > > have i.e. two filters (with 50hz and 60hz center freq respectively),
> > > > > > and they would allow to set only the "enable" attribute; in case you
> > > > > > can i.e. enable only two of three filters, when you try to enable the
> > > > > > 3rd it just refuse that. In this scenario probably "center_frequency"
> > > > > > could be just a regular value (not a list).
> > > > >
> > > > > Agreed.  The question is whether to index filters.  So allow more
> > > > > than one of a given type on a given channel. So far we have
> > > > > only had the one and there isn't a nice way to support multiple
> > > > > as we currently don't have any indexed parameters of a single channel.
> > > >
> > > > Yes, being able to indexing filters was the underlying assumption..
> > > >
> > > > > I haven't seen parts that actually do have this level of sophisticated
> > > > > filtering built in, with the exception of mains filters like this one.
> > > >
> > > > Yes, I didn't too indeed.
> > > >
> > > > > I think we have to allow for the possibility so if you are happy to do
> > > > > so please propose the ABI additions to support multiple filters of
> > > > > a type. I would suggest keeping them per type though
> > > > >
> > > > > e.g.
> > > > >
> > > > > in_X_filter_low_passY_3db_frequency etc
> > > > > with Y as the optional index.
> > > >
> > > > Seems reasonable.
> > > >
> > > > Well the idea is the one you've just explained here, that is adding an
> > > > optional index for filters (per type and per ch); I'm not getting what
> > > > do you mean about proposing it..
> > > Like all new ABI, needs a formal documentation patch.  Sometimes
> > > we get more review on those than on discussions deep in a thread like
> > > this.
> >
> > OK, this is honestly a bit out of my scope, but - if this is OK for
> > you - I may try to do that after getting to some end with those
> > in-flight patches..
>
> That is fine, but we will need docs for anything that is added
> even if it's the version without an index.
>
> Don't have to be detailed etc, just a couple of lines for
> Documentation/ABI/testing/sysfs-bus-iio to define what it is and
> what it's units are.

OK

> >
> > > >
> > > > > For now, lets just implement then using extended attributes rather
> > > > > than trying to extend the core to generate these automatically.
> > > > >
> > > > > If this turns out not to be a corner case we can try to figure
> > > > > out a sane way of generating the multiple indexed versions.
> > > >
> > > > OK. Let's try not to over-complicate things until it's really needed -
> > > > Also, maybe if we'll hit other weird devices that need something like
> > > > this, then they could have some exotic features that we haven't in
> > > > mind yet now; it might turn out that we need something even more
> > > > different, so maybe it's better to wait for real "users" of the ABI
> > > > before deciding how to change it..
> > > >
> > > > But indeed, getting back to the patch originally discussed in this
> > > > thread: if you are OK whit this, then I'll go with a
> > > > "in_temp_filter_notch_center_frequency" attribute.
> > > >
> > > > I may use a specific IIO_DEVIC_ATTR or add it to the core (without
> > > > addressing the index thing), as you prefer.
> > >
> > > Which ever makes most sense to you. Either is fine for a new
> > > attribute, though here don't we need the indexed filters?
> >
> > This specific device has only the option to choose either 50Hz
> > filtering or 60Hz filtering; no option to disable filtering or enable
> > both frequency. So I would say that here we can expose just one filter
> > for which the center frequency can be set to either 50Hz or 60Hz (like
> > the original patch did - but with proper name this time). I would say
> > that we don't need indexed filters here. Or have I missed something ?
> >
> > (So I would start with a IIO_DEVICE_ATTR for now then).
>
> That's fine.  I'd somehow gotten it into my head that the part
> had two filters :)

OK :)

> >
> > > >
> > > > BTW: Looking at other drivers, it seems that for other attributes
> > > > (e.g. oversampling_ratio, discussed in 2/3) they tend to round
> > > > required values to the closest allowed value, instead of returning an
> > > > error. In this specific case, do you want to apply the same logic? For
> > > > consistency reasons I would do that, but at the practical side,
> > > > requiring a line filter frequency which is not either 50Hz or 60Hz
> > > > seems really an error to me..
> > >
> > > It can be a bit of a tricky decision but for something like this
> > > where the precise value works, it should reject incorrect ones.
> >
> > OK
> >
> > > Oversampling is an odd one.  It should probably always round up
> > > as it's usually not a problem to average more results, it just
> > > wastes power.  That only applies if the oversampling_ratio
> > > is independent of other attributes such as sampling frequency.
> >
> > It actually affects also the sampling rate; the more samples you
> > average, the slowest output rate you achieve. But actually there is no
> > attribute for setting the sampling frequency.. it is internally
> > adjusted by the chip depending by averaging and filter line frequency
> > (don't know why).
> >
> > (if we want also this attribute, then I can craft another patch for
> > adding it; it may be usefult to report the actual sample rate, I'm not
> > sure if it makes sense to let the user set it, because we can
> > basically just switch the averaging to one of the few possible values
> > to get the sample rate to change as a side effect - assuming that one
> > doesn't want to change the line frequency filter).
>
> OK.  There are no hard rules on which attributes are the 'dominant'
> ones so it would be fine to either not expose it or to have it read
> only.  It can certainly be useful for fast devices as it lets you
> size the buffers, but for a temperature sensor like this one
> sampling_frequency probably doesn't matter to anyone.

Ok, I will sitck with just adding the filter attribute.

> >
> > BTW What about to round the requested oversampling_ratio to the
> > closest allowed value (instead of rounding up)?
>
> If a user has requested a value for oversampling ratio, it will
> be to achieve a particular improvement in noise rejection.
>
> As such I'd assume round up was most appropriate, or error on any
> value that isn't a precise value.

Both seem reasonable to me. I will think to it a bit more then I'll
make a choice :) .. and then I'll go with a V2 series with all changes
we discussed.

> >
> > > Still, we have traditionally been relaxed on this as long
> > > as writing the the 'correct' value always works as that's what
> > > userspace ABI should be doing.
> > >
> > > Jonathan
> > >
> Thanks,
>
> Jonathan