Changes from v1: * combined dt binding docs * added making interrupt line is optional patchset * broke out dt changes from DO-SM module patchset Change from v2: * switch docs to YAML format Change from v3: * fix up issues reported with dt-schema Matt Ranostay (3): iio: chemical: atlas-sensor: allow probe without interrupt line iio: chemical: atlas-sensor: add DO-SM module support dt-bindings: iio: chemical: consolidate atlas-sensor docs .../bindings/iio/chemical/atlas,ec-sm.txt | 21 ----- .../bindings/iio/chemical/atlas,orp-sm.txt | 21 ----- .../bindings/iio/chemical/atlas,ph-sm.txt | 21 ----- .../bindings/iio/chemical/atlas,sensor.yaml | 53 +++++++++++ drivers/iio/chemical/atlas-sensor.c | 91 +++++++++++++++---- 5 files changed, 125 insertions(+), 82 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt create mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml -- 2.20.1
Sensors don't actually need a interrupt line to give valid readings, and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove the required check for interrupt, and continue along in the probe function. Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> --- drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c index 2f0a6fed2589..2e34c82cb65d 100644 --- a/drivers/iio/chemical/atlas-sensor.c +++ b/drivers/iio/chemical/atlas-sensor.c @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, if (ret) return ret; - if (client->irq <= 0) { - dev_err(&client->dev, "no valid irq defined\n"); - return -EINVAL; - } - ret = chip->calibration(data); if (ret) return ret; @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, init_irq_work(&data->work, atlas_work_handler); - /* interrupt pin toggles on new conversion */ - ret = devm_request_threaded_irq(&client->dev, client->irq, - NULL, atlas_interrupt_handler, - IRQF_TRIGGER_RISING | - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, - "atlas_irq", - indio_dev); - if (ret) { - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); - goto unregister_buffer; + if (client->irq <= 0) { + /* interrupt pin toggles on new conversion */ + ret = devm_request_threaded_irq(&client->dev, client->irq, + NULL, atlas_interrupt_handler, + IRQF_TRIGGER_RISING | + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, + "atlas_irq", + indio_dev); + + if (ret) + dev_warn(&client->dev, + "request irq (%d) failed\n", client->irq); } ret = atlas_set_powermode(data, 1); -- 2.20.1
Atlas Scientific DO-SM OEM sensor reads disolved oxygen in a solution. This is reported back as mg/L which maps direc to ppm and so the IIO_CONCENTRATION channel type can be used. Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> --- drivers/iio/chemical/atlas-sensor.c | 64 +++++++++++++++++++++++++++-- 1 file changed, 60 insertions(+), 4 deletions(-) diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c index 2e34c82cb65d..919d408869be 100644 --- a/drivers/iio/chemical/atlas-sensor.c +++ b/drivers/iio/chemical/atlas-sensor.c @@ -48,6 +48,11 @@ #define ATLAS_REG_EC_CALIB_STATUS_LOW BIT(2) #define ATLAS_REG_EC_CALIB_STATUS_HIGH BIT(3) +#define ATLAS_REG_DO_CALIB_STATUS 0x09 +#define ATLAS_REG_DO_CALIB_STATUS_MASK 0x03 +#define ATLAS_REG_DO_CALIB_STATUS_PRESSURE BIT(0) +#define ATLAS_REG_DO_CALIB_STATUS_DO BIT(1) + #define ATLAS_REG_PH_TEMP_DATA 0x0e #define ATLAS_REG_PH_DATA 0x16 @@ -60,14 +65,19 @@ #define ATLAS_REG_ORP_CALIB_STATUS 0x0d #define ATLAS_REG_ORP_DATA 0x0e +#define ATLAS_REG_DO_TEMP_DATA 0x12 +#define ATLAS_REG_DO_DATA 0x22 + #define ATLAS_PH_INT_TIME_IN_MS 450 #define ATLAS_EC_INT_TIME_IN_MS 650 #define ATLAS_ORP_INT_TIME_IN_MS 450 +#define ATLAS_DO_INT_TIME_IN_MS 450 enum { ATLAS_PH_SM, ATLAS_EC_SM, ATLAS_ORP_SM, + ATLAS_DO_SM, }; struct atlas_data { @@ -121,7 +131,7 @@ static const struct iio_chan_spec atlas_ph_channels[] = { }, }; -#define ATLAS_EC_CHANNEL(_idx, _addr) \ +#define ATLAS_CONCENTRATION_CHANNEL(_idx, _addr) \ {\ .type = IIO_CONCENTRATION, \ .indexed = 1, \ @@ -152,8 +162,8 @@ static const struct iio_chan_spec atlas_ec_channels[] = { .endianness = IIO_BE, }, }, - ATLAS_EC_CHANNEL(0, ATLAS_REG_TDS_DATA), - ATLAS_EC_CHANNEL(1, ATLAS_REG_PSS_DATA), + ATLAS_CONCENTRATION_CHANNEL(0, ATLAS_REG_TDS_DATA), + ATLAS_CONCENTRATION_CHANNEL(1, ATLAS_REG_PSS_DATA), IIO_CHAN_SOFT_TIMESTAMP(3), { .type = IIO_TEMP, @@ -182,6 +192,19 @@ static const struct iio_chan_spec atlas_orp_channels[] = { IIO_CHAN_SOFT_TIMESTAMP(1), }; +static const struct iio_chan_spec atlas_do_channels[] = { + ATLAS_CONCENTRATION_CHANNEL(0, ATLAS_REG_DO_DATA), + IIO_CHAN_SOFT_TIMESTAMP(1), + { + .type = IIO_TEMP, + .address = ATLAS_REG_DO_TEMP_DATA, + .info_mask_separate = + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), + .output = 1, + .scan_index = -1 + }, +}; + static int atlas_check_ph_calibration(struct atlas_data *data) { struct device *dev = &data->client->dev; @@ -262,7 +285,31 @@ static int atlas_check_orp_calibration(struct atlas_data *data) dev_warn(dev, "device has not been calibrated\n"); return 0; -}; +} + +static int atlas_check_do_calibration(struct atlas_data *data) +{ + struct device *dev = &data->client->dev; + int ret; + unsigned int val; + + ret = regmap_read(data->regmap, ATLAS_REG_DO_CALIB_STATUS, &val); + if (ret) + return ret; + + if (!(val & ATLAS_REG_DO_CALIB_STATUS_MASK)) { + dev_warn(dev, "device has not been calibrated\n"); + return 0; + } + + if (!(val & ATLAS_REG_DO_CALIB_STATUS_PRESSURE)) + dev_warn(dev, "device missing atmospheric pressure calibration\n"); + + if (!(val & ATLAS_REG_DO_CALIB_STATUS_DO)) + dev_warn(dev, "device missing dissolved oxygen calibration\n"); + + return 0; +} struct atlas_device { const struct iio_chan_spec *channels; @@ -295,6 +342,13 @@ static struct atlas_device atlas_devices[] = { .calibration = &atlas_check_orp_calibration, .delay = ATLAS_ORP_INT_TIME_IN_MS, }, + [ATLAS_DO_SM] = { + .channels = atlas_do_channels, + .num_channels = 3, + .data_reg = ATLAS_REG_DO_DATA, + .calibration = &atlas_check_do_calibration, + .delay = ATLAS_DO_INT_TIME_IN_MS, + }, }; static int atlas_set_powermode(struct atlas_data *data, int on) @@ -507,6 +561,7 @@ static const struct i2c_device_id atlas_id[] = { { "atlas-ph-sm", ATLAS_PH_SM}, { "atlas-ec-sm", ATLAS_EC_SM}, { "atlas-orp-sm", ATLAS_ORP_SM}, + { "atlas-do-sm", ATLAS_DO_SM}, {} }; MODULE_DEVICE_TABLE(i2c, atlas_id); @@ -515,6 +570,7 @@ static const struct of_device_id atlas_dt_ids[] = { { .compatible = "atlas,ph-sm", .data = (void *)ATLAS_PH_SM, }, { .compatible = "atlas,ec-sm", .data = (void *)ATLAS_EC_SM, }, { .compatible = "atlas,orp-sm", .data = (void *)ATLAS_ORP_SM, }, + { .compatible = "atlas,do-sm", .data = (void *)ATLAS_DO_SM, }, { } }; MODULE_DEVICE_TABLE(of, atlas_dt_ids); -- 2.20.1
Since Atlas Scientific device support only varies from the compatible string is ideal all the respective docs are merged into a single doc named atlas,sensor.yaml Cc: devicetree@vger.kernel.org Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> --- .../bindings/iio/chemical/atlas,ec-sm.txt | 21 -------- .../bindings/iio/chemical/atlas,orp-sm.txt | 21 -------- .../bindings/iio/chemical/atlas,ph-sm.txt | 21 -------- .../bindings/iio/chemical/atlas,sensor.yaml | 53 +++++++++++++++++++ 4 files changed, 53 insertions(+), 63 deletions(-) delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt create mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt deleted file mode 100644 index f4320595b851..000000000000 --- a/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt +++ /dev/null @@ -1,21 +0,0 @@ -* Atlas Scientific EC-SM OEM sensor - -http://www.atlas-scientific.com/_files/_datasheets/_oem/EC_oem_datasheet.pdf - -Required properties: - - - compatible: must be "atlas,ec-sm" - - reg: the I2C address of the sensor - - interrupts: the sole interrupt generated by the device - - Refer to interrupt-controller/interrupts.txt for generic interrupt client - node bindings. - -Example: - -atlas@64 { - compatible = "atlas,ec-sm"; - reg = <0x64>; - interrupt-parent = <&gpio1>; - interrupts = <16 2>; -}; diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt deleted file mode 100644 index af1f5a9aa4da..000000000000 --- a/Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt +++ /dev/null @@ -1,21 +0,0 @@ -* Atlas Scientific ORP-SM OEM sensor - -https://www.atlas-scientific.com/_files/_datasheets/_oem/ORP_oem_datasheet.pdf - -Required properties: - - - compatible: must be "atlas,orp-sm" - - reg: the I2C address of the sensor - - interrupts: the sole interrupt generated by the device - - Refer to interrupt-controller/interrupts.txt for generic interrupt client - node bindings. - -Example: - -atlas@66 { - compatible = "atlas,orp-sm"; - reg = <0x66>; - interrupt-parent = <&gpio1>; - interrupts = <16 2>; -}; diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt deleted file mode 100644 index 79d90f060327..000000000000 --- a/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt +++ /dev/null @@ -1,21 +0,0 @@ -* Atlas Scientific pH-SM OEM sensor - -http://www.atlas-scientific.com/_files/_datasheets/_oem/pH_oem_datasheet.pdf - -Required properties: - - - compatible: must be "atlas,ph-sm" - - reg: the I2C address of the sensor - - interrupts: the sole interrupt generated by the device - - Refer to interrupt-controller/interrupts.txt for generic interrupt client - node bindings. - -Example: - -atlas@65 { - compatible = "atlas,ph-sm"; - reg = <0x65>; - interrupt-parent = <&gpio1>; - interrupts = <16 2>; -}; diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml b/Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml new file mode 100644 index 000000000000..1b0c476838d1 --- /dev/null +++ b/Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml @@ -0,0 +1,53 @@ +# SPDX-License-Identifier: GPL-2.0 +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/chemical/atlas,sensor.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Atlas Scientific OEM sensors + +maintainers: + - Matt Ranostay <matt.ranostay@konsulko.com> + +description: | + Atlas Scientific OEM sensors connected via I2C + + Datasheets: + http://www.atlas-scientific.com/_files/_datasheets/_oem/DO_oem_datasheet.pdf + http://www.atlas-scientific.com/_files/_datasheets/_oem/EC_oem_datasheet.pdf + http://www.atlas-scientific.com/_files/_datasheets/_oem/ORP_oem_datasheet.pdf + http://www.atlas-scientific.com/_files/_datasheets/_oem/pH_oem_datasheet.pdf + +properties: + compatible: + enum: + - atlas,do-sm + - atlas,ec-sm + - atlas,orp-sm + - atlas,ph-sm + + reg: + maxItems: 1 + + interrupts: + maxItems: 1 + +required: + - compatible + - reg + +additionalProperties: false + +examples: + - | + i2c { + #address-cells = <1>; + #size-cells = <0>; + + atlas@66 { + compatible = "atlas,orp-sm"; + reg = <0x66>; + interrupt-parent = <&gpio1>; + interrupts = <16 2>; + }; + }; -- 2.20.1
On Wed, 5 Feb 2020 22:13:32 -0800, Matt Ranostay wrote:
> Since Atlas Scientific device support only varies from the compatible
> string is ideal all the respective docs are merged into a single doc
> named atlas,sensor.yaml
>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com>
> ---
> .../bindings/iio/chemical/atlas,ec-sm.txt | 21 --------
> .../bindings/iio/chemical/atlas,orp-sm.txt | 21 --------
> .../bindings/iio/chemical/atlas,ph-sm.txt | 21 --------
> .../bindings/iio/chemical/atlas,sensor.yaml | 53 +++++++++++++++++++
> 4 files changed, 53 insertions(+), 63 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt
> delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt
> delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt
> create mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml
>
Reviewed-by: Rob Herring <robh@kernel.org>
On Wed, Feb 05, 2020 at 10:13:30PM -0800, Matt Ranostay wrote: > Sensors don't actually need a interrupt line to give valid readings, > and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > the required check for interrupt, and continue along in the probe > function. > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > index 2f0a6fed2589..2e34c82cb65d 100644 > --- a/drivers/iio/chemical/atlas-sensor.c > +++ b/drivers/iio/chemical/atlas-sensor.c > @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > if (ret) > return ret; > > - if (client->irq <= 0) { > - dev_err(&client->dev, "no valid irq defined\n"); > - return -EINVAL; > - } > - > ret = chip->calibration(data); > if (ret) > return ret; > @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > > init_irq_work(&data->work, atlas_work_handler); > > - /* interrupt pin toggles on new conversion */ > - ret = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, atlas_interrupt_handler, > - IRQF_TRIGGER_RISING | > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - "atlas_irq", > - indio_dev); > - if (ret) { > - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > - goto unregister_buffer; > + if (client->irq <= 0) { Should this be > 0 ? Jeremy > + /* interrupt pin toggles on new conversion */ > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, atlas_interrupt_handler, > + IRQF_TRIGGER_RISING | > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "atlas_irq", > + indio_dev); > + > + if (ret) > + dev_warn(&client->dev, > + "request irq (%d) failed\n", client->irq); > } > > ret = atlas_set_powermode(data, 1); > -- > 2.20.1 >
On Thu, Feb 6, 2020 at 4:53 PM Jeremy Fertic <jeremyfertic@gmail.com> wrote: > > On Wed, Feb 05, 2020 at 10:13:30PM -0800, Matt Ranostay wrote: > > Sensors don't actually need a interrupt line to give valid readings, > > and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > > the required check for interrupt, and continue along in the probe > > function. > > > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > > --- > > drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > > 1 file changed, 12 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > > index 2f0a6fed2589..2e34c82cb65d 100644 > > --- a/drivers/iio/chemical/atlas-sensor.c > > +++ b/drivers/iio/chemical/atlas-sensor.c > > @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > > if (ret) > > return ret; > > > > - if (client->irq <= 0) { > > - dev_err(&client->dev, "no valid irq defined\n"); > > - return -EINVAL; > > - } > > - > > ret = chip->calibration(data); > > if (ret) > > return ret; > > @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > > > > init_irq_work(&data->work, atlas_work_handler); > > > > - /* interrupt pin toggles on new conversion */ > > - ret = devm_request_threaded_irq(&client->dev, client->irq, > > - NULL, atlas_interrupt_handler, > > - IRQF_TRIGGER_RISING | > > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > - "atlas_irq", > > - indio_dev); > > - if (ret) { > > - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > > - goto unregister_buffer; > > + if (client->irq <= 0) { > > Should this be > 0 ? > Ah yes good catch :/ - Matt > Jeremy > > > + /* interrupt pin toggles on new conversion */ > > + ret = devm_request_threaded_irq(&client->dev, client->irq, > > + NULL, atlas_interrupt_handler, > > + IRQF_TRIGGER_RISING | > > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > > + "atlas_irq", > > + indio_dev); > > + > > + if (ret) > > + dev_warn(&client->dev, > > + "request irq (%d) failed\n", client->irq); > > } > > > > ret = atlas_set_powermode(data, 1); > > -- > > 2.20.1 > >
On Wed, 5 Feb 2020 22:13:32 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > Since Atlas Scientific device support only varies from the compatible > string is ideal all the respective docs are merged into a single doc > named atlas,sensor.yaml > > Cc: devicetree@vger.kernel.org > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > .../bindings/iio/chemical/atlas,ec-sm.txt | 21 -------- > .../bindings/iio/chemical/atlas,orp-sm.txt | 21 -------- > .../bindings/iio/chemical/atlas,ph-sm.txt | 21 -------- > .../bindings/iio/chemical/atlas,sensor.yaml | 53 +++++++++++++++++++ > 4 files changed, 53 insertions(+), 63 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt > delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt > delete mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt > create mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml > > diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt > deleted file mode 100644 > index f4320595b851..000000000000 > --- a/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt > +++ /dev/null > @@ -1,21 +0,0 @@ > -* Atlas Scientific EC-SM OEM sensor > - > -http://www.atlas-scientific.com/_files/_datasheets/_oem/EC_oem_datasheet.pdf > - > -Required properties: > - > - - compatible: must be "atlas,ec-sm" > - - reg: the I2C address of the sensor > - - interrupts: the sole interrupt generated by the device > - > - Refer to interrupt-controller/interrupts.txt for generic interrupt client > - node bindings. > - > -Example: > - > -atlas@64 { > - compatible = "atlas,ec-sm"; > - reg = <0x64>; > - interrupt-parent = <&gpio1>; > - interrupts = <16 2>; > -}; > diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt > deleted file mode 100644 > index af1f5a9aa4da..000000000000 > --- a/Documentation/devicetree/bindings/iio/chemical/atlas,orp-sm.txt > +++ /dev/null > @@ -1,21 +0,0 @@ > -* Atlas Scientific ORP-SM OEM sensor > - > -https://www.atlas-scientific.com/_files/_datasheets/_oem/ORP_oem_datasheet.pdf > - > -Required properties: > - > - - compatible: must be "atlas,orp-sm" > - - reg: the I2C address of the sensor > - - interrupts: the sole interrupt generated by the device > - > - Refer to interrupt-controller/interrupts.txt for generic interrupt client > - node bindings. > - > -Example: > - > -atlas@66 { > - compatible = "atlas,orp-sm"; > - reg = <0x66>; > - interrupt-parent = <&gpio1>; > - interrupts = <16 2>; > -}; > diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt > deleted file mode 100644 > index 79d90f060327..000000000000 > --- a/Documentation/devicetree/bindings/iio/chemical/atlas,ph-sm.txt > +++ /dev/null > @@ -1,21 +0,0 @@ > -* Atlas Scientific pH-SM OEM sensor > - > -http://www.atlas-scientific.com/_files/_datasheets/_oem/pH_oem_datasheet.pdf > - > -Required properties: > - > - - compatible: must be "atlas,ph-sm" > - - reg: the I2C address of the sensor > - - interrupts: the sole interrupt generated by the device > - > - Refer to interrupt-controller/interrupts.txt for generic interrupt client > - node bindings. > - > -Example: > - > -atlas@65 { > - compatible = "atlas,ph-sm"; > - reg = <0x65>; > - interrupt-parent = <&gpio1>; > - interrupts = <16 2>; > -}; > diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml b/Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml > new file mode 100644 > index 000000000000..1b0c476838d1 > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/chemical/atlas,sensor.yaml > @@ -0,0 +1,53 @@ > +# SPDX-License-Identifier: GPL-2.0 Given this is entirely your binding, would you mind using the dual license with GPL and BSD that is preferred for new bindings? Otherwise looks good to me. Jonathan > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/chemical/atlas,sensor.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Atlas Scientific OEM sensors > + > +maintainers: > + - Matt Ranostay <matt.ranostay@konsulko.com> > + > +description: | > + Atlas Scientific OEM sensors connected via I2C > + > + Datasheets: > + http://www.atlas-scientific.com/_files/_datasheets/_oem/DO_oem_datasheet.pdf > + http://www.atlas-scientific.com/_files/_datasheets/_oem/EC_oem_datasheet.pdf > + http://www.atlas-scientific.com/_files/_datasheets/_oem/ORP_oem_datasheet.pdf > + http://www.atlas-scientific.com/_files/_datasheets/_oem/pH_oem_datasheet.pdf > + > +properties: > + compatible: > + enum: > + - atlas,do-sm > + - atlas,ec-sm > + - atlas,orp-sm > + - atlas,ph-sm > + > + reg: > + maxItems: 1 > + > + interrupts: > + maxItems: 1 > + > +required: > + - compatible > + - reg > + > +additionalProperties: false > + > +examples: > + - | > + i2c { > + #address-cells = <1>; > + #size-cells = <0>; > + > + atlas@66 { > + compatible = "atlas,orp-sm"; > + reg = <0x66>; > + interrupt-parent = <&gpio1>; > + interrupts = <16 2>; > + }; > + };
On Wed, 5 Feb 2020 22:13:30 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > Sensors don't actually need a interrupt line to give valid readings, > and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > the required check for interrupt, and continue along in the probe > function. Basic aim is good, but if you don't have an interrupt doesn't make sense to register the trigger either. The interrupt enable / disable is also tied up with the buffer whereas it should probably be done via the trigger enable callback or am I missing something? Jonathan > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > --- > drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > 1 file changed, 12 insertions(+), 15 deletions(-) > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > index 2f0a6fed2589..2e34c82cb65d 100644 > --- a/drivers/iio/chemical/atlas-sensor.c > +++ b/drivers/iio/chemical/atlas-sensor.c > @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > if (ret) > return ret; > > - if (client->irq <= 0) { > - dev_err(&client->dev, "no valid irq defined\n"); > - return -EINVAL; > - } > - > ret = chip->calibration(data); > if (ret) > return ret; > @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > > init_irq_work(&data->work, atlas_work_handler); > > - /* interrupt pin toggles on new conversion */ > - ret = devm_request_threaded_irq(&client->dev, client->irq, > - NULL, atlas_interrupt_handler, > - IRQF_TRIGGER_RISING | > - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > - "atlas_irq", > - indio_dev); > - if (ret) { > - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > - goto unregister_buffer; > + if (client->irq <= 0) { > + /* interrupt pin toggles on new conversion */ > + ret = devm_request_threaded_irq(&client->dev, client->irq, > + NULL, atlas_interrupt_handler, > + IRQF_TRIGGER_RISING | > + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > + "atlas_irq", > + indio_dev); > + > + if (ret) > + dev_warn(&client->dev, > + "request irq (%d) failed\n", client->irq); > } > > ret = atlas_set_powermode(data, 1);
On Wed, 5 Feb 2020 22:13:31 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > Atlas Scientific DO-SM OEM sensor reads disolved oxygen in > a solution. This is reported back as mg/L which maps direc > to ppm and so the IIO_CONCENTRATION channel type can be used. > > Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> Hi Matt, This looks good to me. Will pick up once we've resolved the other bits in the series. Also want to give time for Rob to take a look a the yaml. Thanks, Jonathan > --- > drivers/iio/chemical/atlas-sensor.c | 64 +++++++++++++++++++++++++++-- > 1 file changed, 60 insertions(+), 4 deletions(-) > > diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > index 2e34c82cb65d..919d408869be 100644 > --- a/drivers/iio/chemical/atlas-sensor.c > +++ b/drivers/iio/chemical/atlas-sensor.c > @@ -48,6 +48,11 @@ > #define ATLAS_REG_EC_CALIB_STATUS_LOW BIT(2) > #define ATLAS_REG_EC_CALIB_STATUS_HIGH BIT(3) > > +#define ATLAS_REG_DO_CALIB_STATUS 0x09 > +#define ATLAS_REG_DO_CALIB_STATUS_MASK 0x03 > +#define ATLAS_REG_DO_CALIB_STATUS_PRESSURE BIT(0) > +#define ATLAS_REG_DO_CALIB_STATUS_DO BIT(1) > + > #define ATLAS_REG_PH_TEMP_DATA 0x0e > #define ATLAS_REG_PH_DATA 0x16 > > @@ -60,14 +65,19 @@ > #define ATLAS_REG_ORP_CALIB_STATUS 0x0d > #define ATLAS_REG_ORP_DATA 0x0e > > +#define ATLAS_REG_DO_TEMP_DATA 0x12 > +#define ATLAS_REG_DO_DATA 0x22 > + > #define ATLAS_PH_INT_TIME_IN_MS 450 > #define ATLAS_EC_INT_TIME_IN_MS 650 > #define ATLAS_ORP_INT_TIME_IN_MS 450 > +#define ATLAS_DO_INT_TIME_IN_MS 450 > > enum { > ATLAS_PH_SM, > ATLAS_EC_SM, > ATLAS_ORP_SM, > + ATLAS_DO_SM, > }; > > struct atlas_data { > @@ -121,7 +131,7 @@ static const struct iio_chan_spec atlas_ph_channels[] = { > }, > }; > > -#define ATLAS_EC_CHANNEL(_idx, _addr) \ > +#define ATLAS_CONCENTRATION_CHANNEL(_idx, _addr) \ > {\ > .type = IIO_CONCENTRATION, \ > .indexed = 1, \ > @@ -152,8 +162,8 @@ static const struct iio_chan_spec atlas_ec_channels[] = { > .endianness = IIO_BE, > }, > }, > - ATLAS_EC_CHANNEL(0, ATLAS_REG_TDS_DATA), > - ATLAS_EC_CHANNEL(1, ATLAS_REG_PSS_DATA), > + ATLAS_CONCENTRATION_CHANNEL(0, ATLAS_REG_TDS_DATA), > + ATLAS_CONCENTRATION_CHANNEL(1, ATLAS_REG_PSS_DATA), > IIO_CHAN_SOFT_TIMESTAMP(3), > { > .type = IIO_TEMP, > @@ -182,6 +192,19 @@ static const struct iio_chan_spec atlas_orp_channels[] = { > IIO_CHAN_SOFT_TIMESTAMP(1), > }; > > +static const struct iio_chan_spec atlas_do_channels[] = { > + ATLAS_CONCENTRATION_CHANNEL(0, ATLAS_REG_DO_DATA), > + IIO_CHAN_SOFT_TIMESTAMP(1), > + { > + .type = IIO_TEMP, > + .address = ATLAS_REG_DO_TEMP_DATA, > + .info_mask_separate = > + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), > + .output = 1, > + .scan_index = -1 > + }, > +}; > + > static int atlas_check_ph_calibration(struct atlas_data *data) > { > struct device *dev = &data->client->dev; > @@ -262,7 +285,31 @@ static int atlas_check_orp_calibration(struct atlas_data *data) > dev_warn(dev, "device has not been calibrated\n"); > > return 0; > -}; > +} Unrelated change so really should have been in a separate patch, but meh it's trivial so I won't fuss too much about it being here ;) > + > +static int atlas_check_do_calibration(struct atlas_data *data) > +{ > + struct device *dev = &data->client->dev; > + int ret; > + unsigned int val; > + > + ret = regmap_read(data->regmap, ATLAS_REG_DO_CALIB_STATUS, &val); > + if (ret) > + return ret; > + > + if (!(val & ATLAS_REG_DO_CALIB_STATUS_MASK)) { > + dev_warn(dev, "device has not been calibrated\n"); > + return 0; > + } > + > + if (!(val & ATLAS_REG_DO_CALIB_STATUS_PRESSURE)) > + dev_warn(dev, "device missing atmospheric pressure calibration\n"); > + > + if (!(val & ATLAS_REG_DO_CALIB_STATUS_DO)) > + dev_warn(dev, "device missing dissolved oxygen calibration\n"); > + > + return 0; > +} > > struct atlas_device { > const struct iio_chan_spec *channels; > @@ -295,6 +342,13 @@ static struct atlas_device atlas_devices[] = { > .calibration = &atlas_check_orp_calibration, > .delay = ATLAS_ORP_INT_TIME_IN_MS, > }, > + [ATLAS_DO_SM] = { > + .channels = atlas_do_channels, > + .num_channels = 3, > + .data_reg = ATLAS_REG_DO_DATA, > + .calibration = &atlas_check_do_calibration, > + .delay = ATLAS_DO_INT_TIME_IN_MS, > + }, > }; > > static int atlas_set_powermode(struct atlas_data *data, int on) > @@ -507,6 +561,7 @@ static const struct i2c_device_id atlas_id[] = { > { "atlas-ph-sm", ATLAS_PH_SM}, > { "atlas-ec-sm", ATLAS_EC_SM}, > { "atlas-orp-sm", ATLAS_ORP_SM}, > + { "atlas-do-sm", ATLAS_DO_SM}, > {} > }; > MODULE_DEVICE_TABLE(i2c, atlas_id); > @@ -515,6 +570,7 @@ static const struct of_device_id atlas_dt_ids[] = { > { .compatible = "atlas,ph-sm", .data = (void *)ATLAS_PH_SM, }, > { .compatible = "atlas,ec-sm", .data = (void *)ATLAS_EC_SM, }, > { .compatible = "atlas,orp-sm", .data = (void *)ATLAS_ORP_SM, }, > + { .compatible = "atlas,do-sm", .data = (void *)ATLAS_DO_SM, }, > { } > }; > MODULE_DEVICE_TABLE(of, atlas_dt_ids);
> On Feb 8, 2020, at 08:39, Jonathan Cameron <jic23@kernel.org> wrote: > > On Wed, 5 Feb 2020 22:13:30 -0800 > Matt Ranostay <matt.ranostay@konsulko.com> wrote: > >> Sensors don't actually need a interrupt line to give valid readings, >> and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove >> the required check for interrupt, and continue along in the probe >> function. > > Basic aim is good, but if you don't have an interrupt doesn't make > sense to register the trigger either. > > The interrupt enable / disable is also tied up with the buffer whereas > it should probably be done via the trigger enable callback or am I > missing something? It was for allowing sysfs and hrtimer triggers. But just remembered most these sensors have a low refresh rate. I can go either way on having it or not. Thoughts? - Matt > > Jonathan > >> >> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >> --- >> drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- >> 1 file changed, 12 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c >> index 2f0a6fed2589..2e34c82cb65d 100644 >> --- a/drivers/iio/chemical/atlas-sensor.c >> +++ b/drivers/iio/chemical/atlas-sensor.c >> @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, >> if (ret) >> return ret; >> >> - if (client->irq <= 0) { >> - dev_err(&client->dev, "no valid irq defined\n"); >> - return -EINVAL; >> - } >> - >> ret = chip->calibration(data); >> if (ret) >> return ret; >> @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, >> >> init_irq_work(&data->work, atlas_work_handler); >> >> - /* interrupt pin toggles on new conversion */ >> - ret = devm_request_threaded_irq(&client->dev, client->irq, >> - NULL, atlas_interrupt_handler, >> - IRQF_TRIGGER_RISING | >> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> - "atlas_irq", >> - indio_dev); >> - if (ret) { >> - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); >> - goto unregister_buffer; >> + if (client->irq <= 0) { >> + /* interrupt pin toggles on new conversion */ >> + ret = devm_request_threaded_irq(&client->dev, client->irq, >> + NULL, atlas_interrupt_handler, >> + IRQF_TRIGGER_RISING | >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >> + "atlas_irq", >> + indio_dev); >> + >> + if (ret) >> + dev_warn(&client->dev, >> + "request irq (%d) failed\n", client->irq); >> } >> >> ret = atlas_set_powermode(data, 1); >
> On Feb 8, 2020, at 18:12, Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > > >>> On Feb 8, 2020, at 08:39, Jonathan Cameron <jic23@kernel.org> wrote: >>> >>> On Wed, 5 Feb 2020 22:13:30 -0800 >>> Matt Ranostay <matt.ranostay@konsulko.com> wrote: >>> >>> Sensors don't actually need a interrupt line to give valid readings, >>> and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove >>> the required check for interrupt, and continue along in the probe >>> function. >> >> Basic aim is good, but if you don't have an interrupt doesn't make >> sense to register the trigger either. >> >> The interrupt enable / disable is also tied up with the buffer whereas >> it should probably be done via the trigger enable callback or am I >> missing something? > > It was for allowing sysfs and hrtimer triggers. But just remembered most these sensors have a low refresh rate. I can go either way on having it or not. Thoughts? > > - Matt > Also issue one issue was fixed in v5 related to this. - Matt > >> >> Jonathan >> >>> >>> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> >>> --- >>> drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- >>> 1 file changed, 12 insertions(+), 15 deletions(-) >>> >>> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c >>> index 2f0a6fed2589..2e34c82cb65d 100644 >>> --- a/drivers/iio/chemical/atlas-sensor.c >>> +++ b/drivers/iio/chemical/atlas-sensor.c >>> @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, >>> if (ret) >>> return ret; >>> >>> - if (client->irq <= 0) { >>> - dev_err(&client->dev, "no valid irq defined\n"); >>> - return -EINVAL; >>> - } >>> - >>> ret = chip->calibration(data); >>> if (ret) >>> return ret; >>> @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, >>> >>> init_irq_work(&data->work, atlas_work_handler); >>> >>> - /* interrupt pin toggles on new conversion */ >>> - ret = devm_request_threaded_irq(&client->dev, client->irq, >>> - NULL, atlas_interrupt_handler, >>> - IRQF_TRIGGER_RISING | >>> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >>> - "atlas_irq", >>> - indio_dev); >>> - if (ret) { >>> - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); >>> - goto unregister_buffer; >>> + if (client->irq <= 0) { >>> + /* interrupt pin toggles on new conversion */ >>> + ret = devm_request_threaded_irq(&client->dev, client->irq, >>> + NULL, atlas_interrupt_handler, >>> + IRQF_TRIGGER_RISING | >>> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, >>> + "atlas_irq", >>> + indio_dev); >>> + >>> + if (ret) >>> + dev_warn(&client->dev, >>> + "request irq (%d) failed\n", client->irq); >>> } >>> >>> ret = atlas_set_powermode(data, 1); >>
On Sat, 8 Feb 2020 18:12:03 -0800 Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > On Feb 8, 2020, at 08:39, Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Wed, 5 Feb 2020 22:13:30 -0800 > > Matt Ranostay <matt.ranostay@konsulko.com> wrote: > > > >> Sensors don't actually need a interrupt line to give valid readings, > >> and can triggered with CONFIG_IIO_HRTIMER_TRIGGER as well. Remove > >> the required check for interrupt, and continue along in the probe > >> function. > > > > Basic aim is good, but if you don't have an interrupt doesn't make > > sense to register the trigger either. > > > > The interrupt enable / disable is also tied up with the buffer whereas > > it should probably be done via the trigger enable callback or am I > > missing something? > > It was for allowing sysfs and hrtimer triggers. But just remembered most these sensors have a low refresh rate. I can go either way on having it or not. Thoughts? I'm a bit confused. With sysfs and hrtimer triggers why would we need the interrupt enabled? As things stand it will be as it's done in the buffer setup. I was suggesting moving it to the trigger setup so we only deal with the interrupt if we are actually using the data ready trigger. So move it the atlas_set_interrupt call from pre / post enable to the trigger state callback. Jonathan > > - Matt > > > > > > Jonathan > > > >> > >> Signed-off-by: Matt Ranostay <matt.ranostay@konsulko.com> > >> --- > >> drivers/iio/chemical/atlas-sensor.c | 27 ++++++++++++--------------- > >> 1 file changed, 12 insertions(+), 15 deletions(-) > >> > >> diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c > >> index 2f0a6fed2589..2e34c82cb65d 100644 > >> --- a/drivers/iio/chemical/atlas-sensor.c > >> +++ b/drivers/iio/chemical/atlas-sensor.c > >> @@ -572,11 +572,6 @@ static int atlas_probe(struct i2c_client *client, > >> if (ret) > >> return ret; > >> > >> - if (client->irq <= 0) { > >> - dev_err(&client->dev, "no valid irq defined\n"); > >> - return -EINVAL; > >> - } > >> - > >> ret = chip->calibration(data); > >> if (ret) > >> return ret; > >> @@ -596,16 +591,18 @@ static int atlas_probe(struct i2c_client *client, > >> > >> init_irq_work(&data->work, atlas_work_handler); > >> > >> - /* interrupt pin toggles on new conversion */ > >> - ret = devm_request_threaded_irq(&client->dev, client->irq, > >> - NULL, atlas_interrupt_handler, > >> - IRQF_TRIGGER_RISING | > >> - IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >> - "atlas_irq", > >> - indio_dev); > >> - if (ret) { > >> - dev_err(&client->dev, "request irq (%d) failed\n", client->irq); > >> - goto unregister_buffer; > >> + if (client->irq <= 0) { > >> + /* interrupt pin toggles on new conversion */ > >> + ret = devm_request_threaded_irq(&client->dev, client->irq, > >> + NULL, atlas_interrupt_handler, > >> + IRQF_TRIGGER_RISING | > >> + IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >> + "atlas_irq", > >> + indio_dev); > >> + > >> + if (ret) > >> + dev_warn(&client->dev, > >> + "request irq (%d) failed\n", client->irq); > >> } > >> > >> ret = atlas_set_powermode(data, 1); > >