| Message ID | 20220625103853.2470346-8-marcus.folkesson@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,01/10] iio: adc: mcp3911: correct "microchip,device-addr" property | expand |
On Sat, 25 Jun 2022 12:38:51 +0200 Marcus Folkesson <marcus.folkesson@gmail.com> wrote: > The MCP3911 incorporates a phase delay generator, > which ensures that the two ADCs are converting the > inputs with a fixed delay between them. > Expose it to userspace. > > Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> Until now I think we've only had the phase modifier for output channels. So at minimum need to add documentation for it in Documentation/ABI/testing/sysfs-bus-iio However, the snag is that it's defined in terms of radians. The usecase here assumes that the sensor is measuring some sort of wave form, but unfortunately we don't know what that is - hence the setting is in terms of clock delay. As such, though the datasheet calls if phase, I think that is stretching the meaning too far in the IIO ABI. We probably need something new. Years ago, for devices that are actually a single ADC and a MUX where we pretend in IIO that the channels are sampled synchronously we talked about provided the timing delay information to userspace. Nothing ever came of it, but that is effectively the same concept as you have here. So, it's a time measurement so units will need to be seconds - userspace has no idea of the clk speed of a device. For two channels the relationship is straight forward, but I wonder for 3 channel devices how we would handle it. The two different sources of this delay might lead to different controls being optimal. Naming wise, perhaps samplingdelay? If you have actual ADCs that operate independently then relationship to a base reference point will be independent. So for a 3 channel device you'd have in_voltage0_samplingdelay 0 in_voltage1_samplingdelay Phase register 1 code / DMCLK in_voltage2_samplingdelay Phase register 2 code / DMCLK But for a device that is a mux in front of one actual ADC then the timing is likely to be relative to previous channel Hence if all turned on... in_voltage0_samplingdelay 0 in_voltage1_samplingdelay Phase register 1 code / DMCLK in_voltage2_samplingdelay Phase register 2 code + Phase register 1 code / DMCLK If only 0 and 2 enabled. in_voltage0_samplingdelay 0 in_voltage2_samplingdelay Phase register X code However we can probably just make that problem for the driver. Sometimes we'll have to reject or approximate particular combinations of enabled channels and requested delays. One corner case that is nasty will be if there is just one controllable delay. In that case it would seem natural to have just one attribute, but the delay would be cumulative across multiple enabled channels. For that I think we'd just need different ABI. in_voltage_intersampledelay maybe? With two channels the various options would all work but we should think ahead... There is another complexity. These values apply to the buffered data, not otherwise. Moving them into bufferX/ would nicely associate them with the enabled channels and make it more obvious that there is a coupling there However, it is more complex to add attributes to the buffers.. If we think that is the right way to go for ABI it wouldn't be too hard to add to the core - but will need a new callback. So my gut feeling is that this should be bufferX/in_voltage0_samplingdelay 0 bufferX/in_voltage1_samplingdelay Phase register 1 code / DMCLK seconds but it is a rather nasty layering violation. That will require us adding a new callback read_scan_el_raw() and appropriate enum etc. Things will get more complex for 3 channel deviceson multibuffer devices or when there are in kernel consumers (as those may effect the enabled channels but aren't visible in bufferX). However, I don't see it being that likely we'll get that combination of features any time soon (famous last words!) Gut feeling is that adding this feature (and discussion of ABI) will take a while, but it shouldn't block picking up the rest of the series in the meantime. Jonathan > --- > > Notes: > v2: > - Fix formatting (Andy Schevchenko) > > drivers/iio/adc/mcp3911.c | 24 +++++++++++++++++++++++- > 1 file changed, 23 insertions(+), 1 deletion(-) > > diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c > index ede1ad97ed4d..a0609d7663e1 100644 > --- a/drivers/iio/adc/mcp3911.c > +++ b/drivers/iio/adc/mcp3911.c > @@ -155,6 +155,17 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, > > ret = IIO_VAL_INT; > break; > + > + case IIO_CHAN_INFO_PHASE: > + ret = mcp3911_read(adc, > + MCP3911_REG_PHASE, val, 2); > + if (ret) > + goto out; > + > + *val = sign_extend32(*val, 12); > + ret = IIO_VAL_INT; > + break; > + > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > ret = mcp3911_read(adc, > MCP3911_REG_CONFIG, val, 2); > @@ -225,6 +236,15 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, > MCP3911_STATUSCOM_EN_OFFCAL, 2); > break; > > + case IIO_CHAN_INFO_PHASE: > + if (val2 != 0 || val > 0xfff) { > + ret = -EINVAL; > + goto out; > + } > + > + /* Write phase */ > + ret = mcp3911_write(adc, MCP3911_REG_PHASE, val, 2); > + break; > case IIO_CHAN_INFO_OVERSAMPLING_RATIO: > for (int i = 0; i < sizeof(mcp3911_osr_table); i++) { > if (val == mcp3911_osr_table[i]) { > @@ -248,7 +268,9 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, > .channel = idx, \ > .scan_index = idx, \ > .scan_index = idx, \ > - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ > + .info_mask_shared_by_type = \ > + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)| \ > + BIT(IIO_CHAN_INFO_PHASE), \ > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ > BIT(IIO_CHAN_INFO_OFFSET) | \ > BIT(IIO_CHAN_INFO_SCALE), \
diff --git a/drivers/iio/adc/mcp3911.c b/drivers/iio/adc/mcp3911.c index ede1ad97ed4d..a0609d7663e1 100644 --- a/drivers/iio/adc/mcp3911.c +++ b/drivers/iio/adc/mcp3911.c @@ -155,6 +155,17 @@ static int mcp3911_read_raw(struct iio_dev *indio_dev, ret = IIO_VAL_INT; break; + + case IIO_CHAN_INFO_PHASE: + ret = mcp3911_read(adc, + MCP3911_REG_PHASE, val, 2); + if (ret) + goto out; + + *val = sign_extend32(*val, 12); + ret = IIO_VAL_INT; + break; + case IIO_CHAN_INFO_OVERSAMPLING_RATIO: ret = mcp3911_read(adc, MCP3911_REG_CONFIG, val, 2); @@ -225,6 +236,15 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, MCP3911_STATUSCOM_EN_OFFCAL, 2); break; + case IIO_CHAN_INFO_PHASE: + if (val2 != 0 || val > 0xfff) { + ret = -EINVAL; + goto out; + } + + /* Write phase */ + ret = mcp3911_write(adc, MCP3911_REG_PHASE, val, 2); + break; case IIO_CHAN_INFO_OVERSAMPLING_RATIO: for (int i = 0; i < sizeof(mcp3911_osr_table); i++) { if (val == mcp3911_osr_table[i]) { @@ -248,7 +268,9 @@ static int mcp3911_write_raw(struct iio_dev *indio_dev, .channel = idx, \ .scan_index = idx, \ .scan_index = idx, \ - .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO), \ + .info_mask_shared_by_type = \ + BIT(IIO_CHAN_INFO_OVERSAMPLING_RATIO)| \ + BIT(IIO_CHAN_INFO_PHASE), \ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \ BIT(IIO_CHAN_INFO_OFFSET) | \ BIT(IIO_CHAN_INFO_SCALE), \
The MCP3911 incorporates a phase delay generator, which ensures that the two ADCs are converting the inputs with a fixed delay between them. Expose it to userspace. Signed-off-by: Marcus Folkesson <marcus.folkesson@gmail.com> --- Notes: v2: - Fix formatting (Andy Schevchenko) drivers/iio/adc/mcp3911.c | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-)