On 11/05/16 03:35, Matt Ranostay wrote: > On Sun, May 8, 2016 at 12:29 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >> On 05/05/16 09:17, Matt Ranostay wrote: >>> On Wed, May 4, 2016 at 3:39 AM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote: >>>> On 24/04/16 21:43, Matt Ranostay wrote: >>>> Give us a bit more description - what is it, what is it for, what interface is >>>> supplied. Err what does EC standard for? >>>>> Signed-off-by: Matt Ranostay <mranostay@xxxxxxxxx> >>>> I know next to nothing about these sensors and don't have the datasheet to hand. >>>> >>>> This is measuring 3 different types of concentration (I think) and reporting them >>>> simple as channels 0-2. What are they are should we not be distinguishing between >>>> them in some more informative fashion? >>> >>> Should I add some .extend_name for each channel? >> probably not. What are these 3 channels actually measuring? >> As I understand it from search the datasheet for tds... >> We have >> >> electric conductivity (that's not a concentration channel to my mind)... >> Perhaps have it as a new channel type in of itself... > Ok no problem of doing that. Just that I was using a known conversion > value of EC (salts) to ppm. > I have no issue with adding yet another weird iio chan type :) > > Would a IIO_EC make more sense than IIO_US_PER_CM (microseimens per > centimeter)? The latter could be used by some meters in the future. Not a clue! Unless someone else chimes in I'm happy to go with whatever you recommend - the more generic the better. Though seimens/m would probably be better. The places we let in non 'base' units were generally to match hwmon - a decision I now regret as it means people have to read the docs rather than 'knowing' what the units will be. > >> >> total dissolved solids (TDS) which kind of sounds concentration like to me... >> However, we might want to consider an appropriate modifier to say 'of what?'. >> >> PSS is salinity? Looks like this is a temperature adjusted concentration scale? >> (I'm reading wikipedia and not really following it... > Yeah that is correct.. like the pH sensor temperature affects the > reading greatly. > >> >> It might make sense to define a whole new channel type for this... I'm not keen. on having modifiers effect the units so channel type may be the only way to do it >> coherently.. >> >> I have this nasty feeling chemical measurements are going to introduce all sorts >> of weird ways of measuring 'concentrations' according to what actually happens >> to work for a given chemical.. >> >> Ah well. 27 channel types so far - bit of room left before we discover allowing >> 256 in the event descriptors wasn't thinking forwards enough... >> >>> Horribly I have to admit my pH sensor based board doesn't give valid >>> values for this sensor till I resign the ground plane under the IC :/. >>> But I can confirm it does report some "values"... >> :) >>> >>>> >>>> Code itself is mostly fine... >>>> >>>> Jonathan >>>>> --- >>>>> .../bindings/iio/chemical/atlas,ec-sm.txt | 22 ++++ >>>>> drivers/iio/chemical/Kconfig | 8 +- >>>>> drivers/iio/chemical/atlas-ph-sensor.c | 113 ++++++++++++++++++++- >>>>> 3 files changed, 138 insertions(+), 5 deletions(-) >>>>> create mode 100644 Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt b/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt >>>>> new file mode 100644 >>>>> index 0000000..2962bd9 >>>>> --- /dev/null >>>>> +++ b/Documentation/devicetree/bindings/iio/chemical/atlas,ec-sm.txt >>>>> @@ -0,0 +1,22 @@ >>>>> +* 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 >>>>> + - interrupt-parent: should be the phandle for the interrupt controller >>>>> + - 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/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig >>>>> index f73290f..4bcc025 100644 >>>>> --- a/drivers/iio/chemical/Kconfig >>>>> +++ b/drivers/iio/chemical/Kconfig >>>>> @@ -5,15 +5,17 @@ >>>>> menu "Chemical Sensors" >>>>> >>>>> config ATLAS_PH_SENSOR >>>>> - tristate "Atlas Scientific OEM pH-SM sensor" >>>>> + tristate "Atlas Scientific OEM SM sensors" >>>>> depends on I2C >>>>> select REGMAP_I2C >>>>> select IIO_BUFFER >>>>> select IIO_TRIGGERED_BUFFER >>>>> select IRQ_WORK >>>>> help >>>>> - Say Y here to build I2C interface support for the Atlas >>>>> - Scientific OEM pH-SM sensor. >>>>> + Say Y here to build I2C interface support for the following >>>>> + Atlas Scientific OEM SM sensors: >>>>> + * pH SM sensor >>>>> + * EC SM sensor >>>>> >>>>> To compile this driver as module, choose M here: the >>>>> module will be called atlas-ph-sensor. >>>>> diff --git a/drivers/iio/chemical/atlas-ph-sensor.c b/drivers/iio/chemical/atlas-ph-sensor.c >>>>> index c57f9c2..6dddd75 100644 >>>>> --- a/drivers/iio/chemical/atlas-ph-sensor.c >>>>> +++ b/drivers/iio/chemical/atlas-ph-sensor.c >>>>> @@ -50,13 +50,28 @@ >>>>> #define ATLAS_REG_PH_CALIB_STATUS_MID BIT(1) >>>>> #define ATLAS_REG_PH_CALIB_STATUS_HIGH BIT(2) >>>>> >>>>> +#define ATLAS_REG_EC_CALIB_STATUS 0x0f >>>>> +#define ATLAS_REG_EC_CALIB_STATUS_MASK 0x0f >>>>> +#define ATLAS_REG_EC_CALIB_STATUS_DRY BIT(0) >>>>> +#define ATLAS_REG_EC_CALIB_STATUS_SINGLE BIT(1) >>>>> +#define ATLAS_REG_EC_CALIB_STATUS_LOW BIT(2) >>>>> +#define ATLAS_REG_EC_CALIB_STATUS_HIGH BIT(3) >>>>> + >>>>> #define ATLAS_REG_PH_TEMP_DATA 0x0e >>>>> #define ATLAS_REG_PH_DATA 0x16 >>>>> >>>>> +#define ATLAS_REG_EC_PROBE 0x08 >>>>> +#define ATLAS_REG_EC_TEMP_DATA 0x10 >>>>> +#define ATLAS_REG_EC_DATA 0x18 >>>>> +#define ATLAS_REG_TDS_DATA 0x1c >>>>> +#define ATLAS_REG_PSS_DATA 0x20 >>>>> + >>>>> #define ATLAS_PH_INT_TIME_IN_US 450000 >>>>> +#define ATLAS_EC_INT_TIME_IN_US 650000 >>>>> >>>>> enum { >>>>> ATLAS_PH_SM, >>>>> + ATLAS_EC_SM, >>>>> }; >>>>> >>>>> struct atlas_data { >>>>> @@ -66,12 +81,13 @@ struct atlas_data { >>>>> struct regmap *regmap; >>>>> struct irq_work work; >>>>> >>>>> - __be32 buffer[4]; /* 32-bit pH data + 32-bit pad + 64-bit timestamp */ >>>>> + __be32 buffer[6]; /* 96-bit data + 32-bit pad + 64-bit timestamp */ >>>>> }; >>>>> >>>>> static const struct regmap_range atlas_volatile_ranges[] = { >>>>> regmap_reg_range(ATLAS_REG_INT_CONTROL, ATLAS_REG_INT_CONTROL), >>>>> regmap_reg_range(ATLAS_REG_PH_DATA, ATLAS_REG_PH_DATA + 4), >>>>> + regmap_reg_range(ATLAS_REG_EC_DATA, ATLAS_REG_PSS_DATA + 4), >>>>> }; >>>>> >>>>> static const struct regmap_access_table atlas_volatile_table = { >>>>> @@ -86,7 +102,7 @@ static const struct regmap_config atlas_regmap_config = { >>>>> .val_bits = 8, >>>>> >>>>> .volatile_table = &atlas_volatile_table, >>>>> - .max_register = ATLAS_REG_PH_DATA + 4, >>>>> + .max_register = ATLAS_REG_PSS_DATA + 4, >>>>> .cache_type = REGCACHE_RBTREE, >>>>> }; >>>>> >>>>> @@ -115,6 +131,38 @@ static const struct iio_chan_spec atlas_ph_channels[] = { >>>>> }, >>>>> }; >>>>> >>>>> +#define ATLAS_EC_CHANNEL(idx, addr) \ >>>>> + {\ >>>>> + .type = IIO_CONCENTRATION, \ >>>>> + .indexed = 1, \ >>>>> + .channel = idx, \ >>>>> + .address = addr, \ >>>>> + .info_mask_separate = \ >>>>> + BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE), \ >>>>> + .scan_index = idx, \ >>>>> + .scan_type = { \ >>>>> + .sign = 'u', \ >>>>> + .realbits = 32, \ >>>>> + .storagebits = 32, \ >>>>> + .endianness = IIO_BE, \ >>>>> + }, \ >>>>> + } >>>>> + >>>>> +static const struct iio_chan_spec atlas_ec_channels[] = { >>>>> + ATLAS_EC_CHANNEL(0, ATLAS_REG_EC_DATA), >>>>> + ATLAS_EC_CHANNEL(1, ATLAS_REG_TDS_DATA), >>>>> + ATLAS_EC_CHANNEL(2, ATLAS_REG_PSS_DATA), >>>> This needs some explanation. 3 concentration channels of different types? >>>> What are they... >>>>> + IIO_CHAN_SOFT_TIMESTAMP(3), >>>>> + { >>>>> + .type = IIO_TEMP, >>>>> + .address = ATLAS_REG_EC_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; >>>>> @@ -142,6 +190,44 @@ static int atlas_check_ph_calibration(struct atlas_data *data) >>>>> return 0; >>>>> } >>>>> >>>>> +static int atlas_check_ec_calibration(struct atlas_data *data) >>>>> +{ >>>>> + struct device *dev = &data->client->dev; >>>>> + int ret; >>>>> + unsigned int val; >>>>> + >>>>> + ret = regmap_bulk_read(data->regmap, ATLAS_REG_EC_PROBE, &val, 2); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + dev_info(dev, "probe set to K = %d.%.2d", be16_to_cpu(val) / 100, >>>>> + be16_to_cpu(val) % 100); >>>>> + >>>>> + ret = regmap_read(data->regmap, ATLAS_REG_EC_CALIB_STATUS, &val); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + if (!(val & ATLAS_REG_EC_CALIB_STATUS_MASK)) { >>>>> + dev_warn(dev, "device has not been calibrated\n"); >>>>> + return 0; >>>>> + } >>>>> + >>>>> + if (!(val & ATLAS_REG_EC_CALIB_STATUS_DRY)) >>>>> + dev_warn(dev, "device missing dry point calibration\n"); >>>>> + >>>>> + if (val & ATLAS_REG_EC_CALIB_STATUS_SINGLE) { >>>>> + dev_warn(dev, "device using single point calibration\n"); >>>>> + } else { >>>>> + if (!(val & ATLAS_REG_EC_CALIB_STATUS_LOW)) >>>>> + dev_warn(dev, "device missing low point calibration\n"); >>>>> + >>>>> + if (!(val & ATLAS_REG_EC_CALIB_STATUS_HIGH)) >>>>> + dev_warn(dev, "device missing high point calibration\n"); >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> struct atlas_device { >>>>> const struct iio_chan_spec *channels; >>>>> int num_channels; >>>>> @@ -159,6 +245,14 @@ static struct atlas_device atlas_devices[] = { >>>>> .calibration = &atlas_check_ph_calibration, >>>>> .delay = ATLAS_PH_INT_TIME_IN_US, >>>>> }, >>>>> + [ATLAS_EC_SM] = { >>>>> + .channels = atlas_ec_channels, >>>>> + .num_channels = 5, >>>> Use array_size - obviously correct that way so I don't have to go count... >>>>> + .data_reg = ATLAS_REG_EC_DATA, >>>>> + .calibration = &atlas_check_ec_calibration, >>>>> + .delay = ATLAS_EC_INT_TIME_IN_US, >>>>> + }, >>>>> + >>>>> }; >>>>> >>>>> static int atlas_set_powermode(struct atlas_data *data, int on) >>>>> @@ -294,6 +388,7 @@ static int atlas_read_raw(struct iio_dev *indio_dev, >>>>> (u8 *) ®, sizeof(reg)); >>>> I guess it's not complex enough to justify splitting read_raw for the two >>>> devices. Close though so you may want to if you support a third type in here. >>>> >>>>> break; >>>>> case IIO_PH: >>>>> + case IIO_CONCENTRATION: >>>>> mutex_lock(&indio_dev->mlock); >>>>> >>>>> if (iio_buffer_enabled(indio_dev)) >>>>> @@ -324,6 +419,18 @@ static int atlas_read_raw(struct iio_dev *indio_dev, >>>>> *val = 1; /* 0.001 */ >>>>> *val2 = 1000; >>>>> break; >>>>> + case IIO_CONCENTRATION: >>>>> + switch (chan->address) { >>>>> + case ATLAS_REG_EC_DATA: >>>>> + *val = 0; /* 0.00000000064 */ >>>>> + *val2 = 640; >>>>> + return IIO_VAL_INT_PLUS_NANO; >>>>> + default: >>>>> + *val = 0; /* 0.000000001 */ >>>>> + *val2 = 1000; >>>>> + return IIO_VAL_INT_PLUS_NANO; >>>>> + } >>>>> + break; >>>>> default: >>>>> return -EINVAL; >>>>> } >>>>> @@ -517,12 +624,14 @@ static const struct dev_pm_ops atlas_pm_ops = { >>>>> >>>>> static const struct i2c_device_id atlas_id[] = { >>>>> { "atlas-ph-sm", ATLAS_PH_SM}, >>>>> + { "atlas-ec-sm", ATLAS_EC_SM}, >>>>> {} >>>>> }; >>>>> MODULE_DEVICE_TABLE(i2c, atlas_id); >>>>> >>>>> 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, }, >>>>> { } >>>>> }; >>>>> MODULE_DEVICE_TABLE(of, atlas_dt_ids); >>>>> >>>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in >>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >> -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
