On Sun, 17 Jan 2021 13:02:31 -0800 Jyoti Bhayana <jbhayana@xxxxxxxxxx> wrote: > Hi Jonathan, > > Instead of IIO_VAL_INT_PLUS_MICRO, we can also use IIO_VAL_FRACTIONAL > right for the raw values for min/max range and resolution? But, to Sure on IIO_VAL_FRACTIONAL, that should be fine. > keep things simple though, it will be nice if the IIO driver uses > IIO_VAL_INT and throws an error if there is a negative exponent in the > following calculation for the resolution which means the res_exponent > is always the same or greater than the scale_exponent. If that tends to be true, then indeed that should work fine. > > [res] x 10^(res_exponent - exponent) > > so the IIO driver will use IIO_AVAIL_RANGE and IIO_VAL_INT and throw > error and not support that sensor if the following conditions happen: > 1) min/max range is using H32 bits > 2) resolution exponent is greater than the scale exponent. Sounds good though I've not thought through whether we can in general expect 2 to be true. Feels like it should be though! Jonathan > > > Thanks, > Jyoti > > > On Sun, Jan 17, 2021 at 3:56 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > On Sat, 16 Jan 2021 23:15:56 -0800 > > Jyoti Bhayana <jbhayana@xxxxxxxxxx> wrote: > > > > > Hi Jonathan, > > > > > > Can you clarify one thing ? If we go with option 2 which is using > > > IIO_AVAIL_RANGE for min,step,high using IIO_VAL_INT then how will it > > > ensure the possible floating value for step as we are using int type? > > As you've identified we can't go with IIO_VAL_INT if we are doing that > > approach, would need to be IIO_VAL_INT_PLUS_MICRO (possibly NANO) > > > > That is why it comes unstuck if we need to use both val and val2 to > > do 64 bit integers... > > > > For the min and max we just set the val2 value to 0. > > > > Jonathan > > > > > > > > > > Thanks, > > > Jyoti > > > > > > On Sat, Jan 16, 2021 at 11:33 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > > > > > On Mon, 11 Jan 2021 13:17:51 -0800 > > > > Jyoti Bhayana <jbhayana@xxxxxxxxxx> wrote: > > > > > > > > > Hi Jonathan, > > > > > > > > > > I know it is a bit confusing. Let me try to explain it with some > > > > > examples to hopefully clarify some things here. > > > > > SCMI Platform talks to the native/actual sensor, gets the raw values > > > > > from the native sensor and applies the scale and then sends those > > > > > values to the SCMI agent and the SCMI IIO driver. > > > > > Since the sensor readings which SCMI IIO driver gets are integer, to > > > > > convert them to float , we need to apply scale to these sensor values > > > > > which is the unit_exponent(power-of-10 multiplier in two’s-complement > > > > > format) specified in the SCMI specification > > > > > > > > > > Native Sensor -> SCMI platform->SCMI Agent->SCMI IIO Driver > > > > > > > > > > So if Native Sensor gets the sensor value > > > > > 32767 and the scale the SCMI Platform is using is 0.002392. > > > > > SCMI platform does the calculation of 32767 * 0.002392 = 78.378664 > > > > > and send the sensor value as 78378664 and the scale as .000001 to the > > > > > SCMI agent and SCMI IIO driver > > > > > > > > > > so for SCMI IIO driver the sensor reading = 78378664 and scale = .000001 > > > > > and the sensor value is sensor_reading * scale = 78378664 * .000001 > > > > > = 78.378664 > > > > > and the resolution which the SCMI Platform sends to the SCMI agent is 0.002392. > > > > > In the SCMI IIO driver, scale which is .000001 is applied to the min > > > > > range/max range and the actual sensor values. > > > > > sensor resolution which is 0.002392 is just passed to the userspace > > > > > layer so that they know the Native sensor resolution/scale > > > > > being applied by the SCMI platform. > > > > > > > > That was pretty much where I'd gotten to. > > > > Whilst the reasoning might be different it is equivalent to a sensor > > > > providing info on expected noise by giving a 'valid resolution'. > > > > In that case as well you have a sensor providing a number that looks to have > > > > more precision than it actually does. > > > > > > > > Anyhow, that similarity doesn't really matter here. > > > > > > > > I'll also add that a design that applies scale in two places is inherently > > > > less than ideal. A cleaner design would have maintained the separation > > > > between scale and raw value all the way up the stack. That would result > > > > in 0 loss of information and also be a cleaner interface. > > > > Ah well, we live with what we have :) > > > > > > > > > > > > > > Regarding your comments in the previous email, when you mentioned > > > > > "what we actually > > > > > need is non standard ABI for resolution"? Does that mean that it is ok > > > > > to have sensor resolution > > > > > as the IIO attribute shown below? > > > > > > > > > > static IIO_DEVICE_ATTR(sensor_resolution, 0444, scmi_iio_get_sensor_resolution, > > > > > NULL, 0); > > > > > > > > We could do something new (see later for why I don't think we need to) > > > > Would need to clearly reflect what it applies to and I'm not sure resolution > > > > is even an unambiguous name given sensor resolution is often described as 8bit > > > > 10bit etc. E.g. this selection table from Maxim for ADCs. > > > > https://www.maximintegrated.com/en/products/parametric/search.html?fam=prec_adc&tree=master&metaTitle=Precision%20ADCs%20(%20%205Msps)&hide=270 > > > > Of course sometimes it's also used for what you want here. > > > > > > > > Hohum. So we might be still be able to do this with standard ABI but we > > > > are going to need to do some maths in the driver. > > > > So if we were to express it via > > > > > > > > in_accel_raw_avail for example we could use the [low step high] form. > > > > > > > > low and high are straight forward as those are expressed directly from > > > > axis_min_range and axis_max_range which I think are in the same units > > > > as the _raw channel itself. > > > > > > > > For resolution, we have it expressed as [res] x 10^res_exponent > > > > and if we just put that in as the 'step' above it would have the wrong > > > > exponent (as we'd expect to still have to apply your 0.00001 from above > > > > example). > > > > > > > > Hence we express it as [res] x 10^(res_exponent - exponent) > > > > > > > > I'm going to slightly modify your example above because the two exponents > > > > are the same so it's hard to tell if I have them right way around. > > > > Hence let res = 0.00293 = 293 x 10^(-5) (I just dropped the trailing 2) > > > > > > > > scale = 10^(-6) exponent = -6 > > > > > > > > So step = 2392 x 10^(-5 + 6) = 2390 > > > > giving us > > > > > > > > [min 2390 max] for _raw_available > > > > > > > > Hence when userspace comes along and wants this in relevant base units (here > > > > m/sec^2) it applies the x10^(-6) mutliplier from _scale we get out expected value > > > > of 0.00239 m/sec^2 > > > > > > > > That should work for any case we see but the maths done in the driver will have > > > > to cope with potential negative exponents for step. > > > > > > > > One catch will be the 64 bit potential values for min and max :( > > > > > > > > > > > > > > static struct attribute *scmi_iio_attributes[] = { > > > > > &iio_dev_attr_sensor_resolution.dev_attr.attr, > > > > > NULL, > > > > > }; > > > > > > > > > > and for the min/max range, I can use the read_avail callback? > > > > > > > > I would have said yes normally but if we are going to cope with > > > > a potential floating point value for step as touched on above we > > > > may have to do it by hand in the driver. Not ideal but may > > > > be only option :( > > > > > > > > > > > > > > Also, for the min/max range, there were two options discussed in the > > > > > email thread: > > > > > option 1) Add new IIO val Type IIO_VAL_INT_H32_L32, and modify the > > > > > iio_format_value to format the 64 bit int properly for the userspace > > > > > option 2) Ignore the H32 bits and use the existing IIO_VAL_INT as just > > > > > L32 bits should be sufficient for current sensor values. > > > > > > > > Ignore is a strong way of putting it. We would definitely want to > > > > shout about it if we do get anything set in H32. > > > > > > > > If we are fairly sure that we aren't going to anything greater than > > > > 32 bits than we are fine. > > > > > > > > It should be possible to work through the worst cases given > > > > limits of say +-20g for accelerometers for example and the relatively > > > > limited exponents (5 bits). + sensible resolution. > > > > > > > > If it's fairly safe I'd like to go for option 2. as it would ensure we > > > > can do floating point for the step (which is then used to compute the > > > > resolution value for android) > > > > > > > > Thanks > > > > > > > > Jonathan > > > > > > > > > > > > > > Let me know which option you prefer for min/max range. and also please > > > > > confirm if it is ok to have an IIO attribute for resolution like > > > > > mentioned above. > > > > > > > > > > > > > > > Thanks, > > > > > Jyoti > > > > > > > > > > Thank you so much > > > > > > > > > > Jyoti > > > > > > > > > > > > > > > > > > > > On Mon, Jan 11, 2021 at 4:34 AM Jonathan Cameron > > > > > <Jonathan.Cameron@xxxxxxxxxx> wrote: > > > > > > > > > > > > On Sun, 10 Jan 2021 22:44:44 -0800 > > > > > > Jyoti Bhayana <jbhayana@xxxxxxxxxx> wrote: > > > > > > > > > > > > > Hi Jonathan, > > > > > > > > > > > > > > In section 4.7.2.5.1 of the specification, the following exponent is > > > > > > > the scale value > > > > > > > > > > > > > > uint32 axis_attributes_high > > > > > > > Bits[15:11] Exponent: The power-of-10 multiplier in two’s-complement > > > > > > > format that is applied to the sensor unit > > > > > > > specified by the SensorType field. > > > > > > > > > > > > > > and the resolution is > > > > > > > > > > > > > > uint32 axis_resolution > > > > > > > Bits[31:27] Exponent: The power-of-10 multiplier in two’s-complement format > > > > > > > that is applied to the Res field. Bits[26:0] Res: The resolution of > > > > > > > the sensor axis. > > > > > > > > > > > > > > From code in scmi_protocol.h > > > > > > > /** > > > > > > > * scmi_sensor_axis_info - describes one sensor axes > > > > > > > * @id: The axes ID. > > > > > > > * @type: Axes type. Chosen amongst one of @enum scmi_sensor_class. > > > > > > > * @scale: Power-of-10 multiplier applied to the axis unit. > > > > > > > * @name: NULL-terminated string representing axes name as advertised by > > > > > > > * SCMI platform. > > > > > > > * @extended_attrs: Flag to indicate the presence of additional extended > > > > > > > * attributes for this axes. > > > > > > > * @resolution: Extended attribute representing the resolution of the axes. > > > > > > > * Set to 0 if not reported by this axes. > > > > > > > * @exponent: Extended attribute representing the power-of-10 multiplier that > > > > > > > * is applied to the resolution field. Set to 0 if not reported by > > > > > > > * this axes. > > > > > > > * @attrs: Extended attributes representing minimum and maximum values > > > > > > > * measurable by this axes. Set to 0 if not reported by this sensor. > > > > > > > */ > > > > > > > > > > > > > > struct scmi_sensor_axis_info { > > > > > > > unsigned int id; > > > > > > > unsigned int type; > > > > > > > int scale; //This is the scale used for min/max range > > > > > > > char name[SCMI_MAX_STR_SIZE]; > > > > > > > bool extended_attrs; > > > > > > > unsigned int resolution; > > > > > > > int exponent; // This is the scale used in resolution > > > > > > > struct scmi_range_attrs attrs; > > > > > > > }; > > > > > > > > > > > > > > The scale above is the Power-of-10 multiplier which is applied to the min range > > > > > > > and the max range value > > > > > > > but the resolution is equal to resolution and multiplied by > > > > > > > Power-of-10 multiplier > > > > > > > of exponent in the above struct. > > > > > > > So as can be seen above the value of the power of 10 multiplier used > > > > > > > for min/max range > > > > > > > can be different than the value of the power of 10 multiplier used for > > > > > > > the resolution. > > > > > > > Hence, if I have to use IIO_AVAIL_RANGE to specify min/max range and as well > > > > > > > as resolution, then I have to use the float format with the scale applied. > > > > > > > > > > > > > > If there is another way which you know of and prefer, please let me know. > > > > > > I'll confess I've gotten a bit lost here. > > > > > > > > > > > > So I think where we are is how to describe the range of the sensor and why we can't > > > > > > use in_accel_x_raw_available to provide the > > > > > > > > > > > > Understood that the resolution can have different scaling. That is presumably > > > > > > to allow for the case where a device is reporting values at a finer scale than > > > > > > it's real resolution. Resolution might take into account expected noise for > > > > > > example. So it should be decoupled from the scaling of both the actual measurements > > > > > > and the axis high / low limits. > > > > > > > > > > > > However, I'd read that as saying the axis high / low limits and the actual sensor > > > > > > readings should be scaled by the exponent in axis_attributes_high. > > > > > > So I think we are fine for the range, but my earlier assumption that resolution > > > > > > was equivalent to scale in IIO (real world value for 1LSB) may be completely wrong > > > > > > as resolution may be unconnected to how you convert to a real world value? > > > > > > > > > > > > If nothing else I'd like to suggest the spec needs to be tightened a bit here > > > > > > to say exactly how we convert a value coming in to real world units (maybe > > > > > > I'm just missing it). > > > > > > > > > > > > Anyhow, I suspect we've been looking at this the wrong way and what we actually > > > > > > need is non standard ABI for resolution. > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Jyoti > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > Jyoti > > > > > > > > > > > > > > On Sat, Jan 9, 2021 at 11:01 AM Jonathan Cameron <jic23@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > On Wed, 6 Jan 2021 21:23:53 +0000 > > > > > > > > Jyoti Bhayana <jbhayana@xxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > Hi Jonathan, > > > > > > > > > > > > > > > > > > Instead of adding IIO_VAL_INT_H32_L32, I am thinking of adding IIO_VAL_FRACTIONAL_LONG > > > > > > > > > or IIO_VAL_FRACTIONAL_64 as the scale/exponent used for min/max range can be different > > > > > > > > > than the one used in resolution according to specification. > > > > > > > > > > > > > > > > That's somewhat 'odd'. Given min/max are inherently values the sensor is supposed to > > > > > > > > be able to return why give them different resolutions? Can you point me at a specific > > > > > > > > section of the spec? The axis_min_range_low etc fields don't seem to have units specified > > > > > > > > but I assumed they were in sensor units and so same scale factors? > > > > > > > > > > > > > > > > > > > > > > > > > > I am planning to use read_avail for IIO_CHAN_INFO_PROCESSED using IIO_AVAIL_RANGE > > > > > > > > > and this new IIO_VAL_FRACTIONAL_64 for min range,max range and resolution. > > > > > > > > > Instead of two values used in IIO_VAL_FRACTIONAL, IIO_VAL_FRACTIONAL_64 will use 4 values > > > > > > > > > val_high,val_low,and val2_high and val2_low. > > > > > > > > > > > > > > > > I'm not keen on the changing that internal kernel interface unless we absolutely > > > > > > > > have to. read_avail() is called from consumer drivers and they won't know anything > > > > > > > > about this new variant. > > > > > > > > > > > > > > > > > > > > > > > > > > Let me know if that is an acceptable solution. > > > > > > > > > > > > > > > > Hmm. It isn't a standard use of the ABI given the value in the buffer is (I assume) > > > > > > > > raw (needs scale applied). However, it isn't excluded by the ABI docs. Whether > > > > > > > > a standard userspace is going to expect it is not clear to me. > > > > > > > > > > > > > > > > I don't want to end up in a position where we end up with available being generally > > > > > > > > added for processed when what most people care about is what the value range they > > > > > > > > might get from a polled read is (rather than via a buffer). > > > > > > > > > > > > > > > > So I'm not that keen on this solution but if we can find a way to avoid it. > > > > > > > > > > > > > > > > Jonathan > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks, > > > > > > > > > Jyoti > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
