| Message ID | 20191111153517.13862-2-andrea.merello@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: max31856: provide more configuration options | expand |
On Tue, Nov 12, 2019 at 12:35 AM 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. > Looks good to me. Although this ideal could be a device tree property as well, but not a big deal. Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com> > Cc: Hartmut Knaack <knaack.h@gmx.de> > Cc: Lars-Peter Clausen <lars@metafoo.de> > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: Patrick Havelange <patrick.havelange@essensium.com> > Cc: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com> > Cc: Matt Weber <matthew.weber@rockwellcollins.com> > Cc: Matt Ranostay <matt.ranostay@konsulko.com> > Cc: Chuhong Yuan <hslester96@gmail.com> > Cc: Daniel Gomez <dagmcr@gmail.com> > Cc: linux-iio@vger.kernel.org > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > --- > drivers/iio/temperature/max31856.c | 50 ++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > > diff --git a/drivers/iio/temperature/max31856.c b/drivers/iio/temperature/max31856.c > index 73ed550e3fc9..d97ba9ee1598 100644 > --- a/drivers/iio/temperature/max31856.c > +++ b/drivers/iio/temperature/max31856.c > @@ -23,6 +23,7 @@ > #define MAX31856_CR0_1SHOT BIT(6) > #define MAX31856_CR0_OCFAULT BIT(4) > #define MAX31856_CR0_OCFAULT_MASK GENMASK(5, 4) > +#define MAX31856_CR0_FILTER_50HZ BIT(0) > #define MAX31856_TC_TYPE_MASK GENMASK(3, 0) > #define MAX31856_FAULT_OVUV BIT(1) > #define MAX31856_FAULT_OPEN BIT(0) > @@ -63,6 +64,7 @@ static const struct iio_chan_spec max31856_channels[] = { > struct max31856_data { > struct spi_device *spi; > u32 thermocouple_type; > + bool filter_50hz; > }; > > static int max31856_read(struct max31856_data *data, u8 reg, > @@ -123,6 +125,11 @@ static int max31856_init(struct max31856_data *data) > reg_cr0_val &= ~MAX31856_CR0_1SHOT; > reg_cr0_val |= MAX31856_CR0_AUTOCONVERT; > > + if (data->filter_50hz) > + reg_cr0_val |= MAX31856_CR0_FILTER_50HZ; > + else > + reg_cr0_val &= ~MAX31856_CR0_FILTER_50HZ; > + > return max31856_write(data, MAX31856_CR0_REG, reg_cr0_val); > } > > @@ -249,12 +256,54 @@ static ssize_t show_fault_oc(struct device *dev, > return show_fault(dev, MAX31856_FAULT_OPEN, buf); > } > > +static ssize_t show_filter(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct max31856_data *data = iio_priv(indio_dev); > + > + return sprintf(buf, "%d\n", data->filter_50hz ? 50 : 60); > +} > + > +static ssize_t set_filter(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t len) > +{ > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > + struct max31856_data *data = iio_priv(indio_dev); > + unsigned int freq; > + int ret; > + > + ret = kstrtouint(buf, 10, &freq); > + if (ret) > + return ret; > + > + switch (freq) { > + case 50: > + data->filter_50hz = true; > + break; > + case 60: > + data->filter_50hz = false; > + break; > + default: > + return -EINVAL; > + } > + > + max31856_init(data); > + return len; > +} > + > static IIO_DEVICE_ATTR(fault_ovuv, 0444, show_fault_ovuv, NULL, 0); > static IIO_DEVICE_ATTR(fault_oc, 0444, show_fault_oc, NULL, 0); > +static IIO_DEVICE_ATTR(in_temp_filter_notch_center_frequency, 0644, > + show_filter, set_filter, 0); > > static struct attribute *max31856_attributes[] = { > &iio_dev_attr_fault_ovuv.dev_attr.attr, > &iio_dev_attr_fault_oc.dev_attr.attr, > + &iio_dev_attr_in_temp_filter_notch_center_frequency.dev_attr.attr, > NULL, > }; > > @@ -280,6 +329,7 @@ static int max31856_probe(struct spi_device *spi) > > data = iio_priv(indio_dev); > data->spi = spi; > + data->filter_50hz = false; > > spi_set_drvdata(spi, indio_dev); > > -- > 2.17.1 >
On Tue, 12 Nov 2019 07:59:19 +0900 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > On Tue, Nov 12, 2019 at 12:35 AM 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. > > > > Looks good to me. Although this ideal could be a device tree property > as well, but not a big deal. > > Reviewed-by: Matt Ranostay <matt.ranostay@konsulko.com> I've no objection to someone following up with DT support for this as they might have a device that only supports one frequency of powersupply. However, let's leave it until we have a user that cares. I'm going to let the patch sit on the list a little longer though as it introduces new ABI and others may want to review. If I seem to have lost it in 2 weeks time give me a ping (normally I don't lose patches, but it's happened a few times in the past!) Thanks, Jonathan > > > Cc: Hartmut Knaack <knaack.h@gmx.de> > > Cc: Lars-Peter Clausen <lars@metafoo.de> > > Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> > > Cc: Colin Ian King <colin.king@canonical.com> > > Cc: Patrick Havelange <patrick.havelange@essensium.com> > > Cc: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com> > > Cc: Matt Weber <matthew.weber@rockwellcollins.com> > > Cc: Matt Ranostay <matt.ranostay@konsulko.com> > > Cc: Chuhong Yuan <hslester96@gmail.com> > > Cc: Daniel Gomez <dagmcr@gmail.com> > > Cc: linux-iio@vger.kernel.org > > Signed-off-by: Andrea Merello <andrea.merello@gmail.com> > > --- > > drivers/iio/temperature/max31856.c | 50 ++++++++++++++++++++++++++++++ > > 1 file changed, 50 insertions(+) > > > > diff --git a/drivers/iio/temperature/max31856.c b/drivers/iio/temperature/max31856.c > > index 73ed550e3fc9..d97ba9ee1598 100644 > > --- a/drivers/iio/temperature/max31856.c > > +++ b/drivers/iio/temperature/max31856.c > > @@ -23,6 +23,7 @@ > > #define MAX31856_CR0_1SHOT BIT(6) > > #define MAX31856_CR0_OCFAULT BIT(4) > > #define MAX31856_CR0_OCFAULT_MASK GENMASK(5, 4) > > +#define MAX31856_CR0_FILTER_50HZ BIT(0) > > #define MAX31856_TC_TYPE_MASK GENMASK(3, 0) > > #define MAX31856_FAULT_OVUV BIT(1) > > #define MAX31856_FAULT_OPEN BIT(0) > > @@ -63,6 +64,7 @@ static const struct iio_chan_spec max31856_channels[] = { > > struct max31856_data { > > struct spi_device *spi; > > u32 thermocouple_type; > > + bool filter_50hz; > > }; > > > > static int max31856_read(struct max31856_data *data, u8 reg, > > @@ -123,6 +125,11 @@ static int max31856_init(struct max31856_data *data) > > reg_cr0_val &= ~MAX31856_CR0_1SHOT; > > reg_cr0_val |= MAX31856_CR0_AUTOCONVERT; > > > > + if (data->filter_50hz) > > + reg_cr0_val |= MAX31856_CR0_FILTER_50HZ; > > + else > > + reg_cr0_val &= ~MAX31856_CR0_FILTER_50HZ; > > + > > return max31856_write(data, MAX31856_CR0_REG, reg_cr0_val); > > } > > > > @@ -249,12 +256,54 @@ static ssize_t show_fault_oc(struct device *dev, > > return show_fault(dev, MAX31856_FAULT_OPEN, buf); > > } > > > > +static ssize_t show_filter(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > + struct max31856_data *data = iio_priv(indio_dev); > > + > > + return sprintf(buf, "%d\n", data->filter_50hz ? 50 : 60); > > +} > > + > > +static ssize_t set_filter(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, > > + size_t len) > > +{ > > + struct iio_dev *indio_dev = dev_to_iio_dev(dev); > > + struct max31856_data *data = iio_priv(indio_dev); > > + unsigned int freq; > > + int ret; > > + > > + ret = kstrtouint(buf, 10, &freq); > > + if (ret) > > + return ret; > > + > > + switch (freq) { > > + case 50: > > + data->filter_50hz = true; > > + break; > > + case 60: > > + data->filter_50hz = false; > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + max31856_init(data); > > + return len; > > +} > > + > > static IIO_DEVICE_ATTR(fault_ovuv, 0444, show_fault_ovuv, NULL, 0); > > static IIO_DEVICE_ATTR(fault_oc, 0444, show_fault_oc, NULL, 0); > > +static IIO_DEVICE_ATTR(in_temp_filter_notch_center_frequency, 0644, > > + show_filter, set_filter, 0); > > > > static struct attribute *max31856_attributes[] = { > > &iio_dev_attr_fault_ovuv.dev_attr.attr, > > &iio_dev_attr_fault_oc.dev_attr.attr, > > + &iio_dev_attr_in_temp_filter_notch_center_frequency.dev_attr.attr, > > NULL, > > }; > > > > @@ -280,6 +329,7 @@ static int max31856_probe(struct spi_device *spi) > > > > data = iio_priv(indio_dev); > > data->spi = spi; > > + data->filter_50hz = false; > > > > spi_set_drvdata(spi, indio_dev); > > > > -- > > 2.17.1 > >
diff --git a/drivers/iio/temperature/max31856.c b/drivers/iio/temperature/max31856.c index 73ed550e3fc9..d97ba9ee1598 100644 --- a/drivers/iio/temperature/max31856.c +++ b/drivers/iio/temperature/max31856.c @@ -23,6 +23,7 @@ #define MAX31856_CR0_1SHOT BIT(6) #define MAX31856_CR0_OCFAULT BIT(4) #define MAX31856_CR0_OCFAULT_MASK GENMASK(5, 4) +#define MAX31856_CR0_FILTER_50HZ BIT(0) #define MAX31856_TC_TYPE_MASK GENMASK(3, 0) #define MAX31856_FAULT_OVUV BIT(1) #define MAX31856_FAULT_OPEN BIT(0) @@ -63,6 +64,7 @@ static const struct iio_chan_spec max31856_channels[] = { struct max31856_data { struct spi_device *spi; u32 thermocouple_type; + bool filter_50hz; }; static int max31856_read(struct max31856_data *data, u8 reg, @@ -123,6 +125,11 @@ static int max31856_init(struct max31856_data *data) reg_cr0_val &= ~MAX31856_CR0_1SHOT; reg_cr0_val |= MAX31856_CR0_AUTOCONVERT; + if (data->filter_50hz) + reg_cr0_val |= MAX31856_CR0_FILTER_50HZ; + else + reg_cr0_val &= ~MAX31856_CR0_FILTER_50HZ; + return max31856_write(data, MAX31856_CR0_REG, reg_cr0_val); } @@ -249,12 +256,54 @@ static ssize_t show_fault_oc(struct device *dev, return show_fault(dev, MAX31856_FAULT_OPEN, buf); } +static ssize_t show_filter(struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct max31856_data *data = iio_priv(indio_dev); + + return sprintf(buf, "%d\n", data->filter_50hz ? 50 : 60); +} + +static ssize_t set_filter(struct device *dev, + struct device_attribute *attr, + const char *buf, + size_t len) +{ + struct iio_dev *indio_dev = dev_to_iio_dev(dev); + struct max31856_data *data = iio_priv(indio_dev); + unsigned int freq; + int ret; + + ret = kstrtouint(buf, 10, &freq); + if (ret) + return ret; + + switch (freq) { + case 50: + data->filter_50hz = true; + break; + case 60: + data->filter_50hz = false; + break; + default: + return -EINVAL; + } + + max31856_init(data); + return len; +} + static IIO_DEVICE_ATTR(fault_ovuv, 0444, show_fault_ovuv, NULL, 0); static IIO_DEVICE_ATTR(fault_oc, 0444, show_fault_oc, NULL, 0); +static IIO_DEVICE_ATTR(in_temp_filter_notch_center_frequency, 0644, + show_filter, set_filter, 0); static struct attribute *max31856_attributes[] = { &iio_dev_attr_fault_ovuv.dev_attr.attr, &iio_dev_attr_fault_oc.dev_attr.attr, + &iio_dev_attr_in_temp_filter_notch_center_frequency.dev_attr.attr, NULL, }; @@ -280,6 +329,7 @@ static int max31856_probe(struct spi_device *spi) data = iio_priv(indio_dev); data->spi = spi; + data->filter_50hz = false; spi_set_drvdata(spi, indio_dev);
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. Cc: Hartmut Knaack <knaack.h@gmx.de> Cc: Lars-Peter Clausen <lars@metafoo.de> Cc: Peter Meerwald-Stadler <pmeerw@pmeerw.net> Cc: Colin Ian King <colin.king@canonical.com> Cc: Patrick Havelange <patrick.havelange@essensium.com> Cc: Paresh Chaudhary <paresh.chaudhary@rockwellcollins.com> Cc: Matt Weber <matthew.weber@rockwellcollins.com> Cc: Matt Ranostay <matt.ranostay@konsulko.com> Cc: Chuhong Yuan <hslester96@gmail.com> Cc: Daniel Gomez <dagmcr@gmail.com> Cc: linux-iio@vger.kernel.org Signed-off-by: Andrea Merello <andrea.merello@gmail.com> --- drivers/iio/temperature/max31856.c | 50 ++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+)