-
Notifications
You must be signed in to change notification settings - Fork 845
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ad485x upstreaming #2527
base: staging/linus
Are you sure you want to change the base?
ad485x upstreaming #2527
Conversation
IIO_ENUM_AVAILABLE("packet_format", \ | ||
IIO_SHARED_BY_ALL, &ad4857_packet_fmt), \ | ||
{}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The packet format also needs more clarification what is it about? (we also need the ABI doc we the new extra non standard attributes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this packet format thingy may be a very "funny" discussion if we really need to support it. I'm not sure how useful it is the 32 bits format rather than being used in test pattern. I'm not seeing too much benefit on the channel id or span id information (can already get info with other attributes). The OR/UR is the one that could be more useful but is there someone using it? Do we really need to have it close to the sample? If not, there's the status register and... Also, I think this can be implemented using IIO events (likely what we will be asked). So what comes to mind could be:
- for test_pattern (could be implemented as ext_info I think - not for now I guess) we can easily look at our word side and dynamically set the proper packet size. So, to me, this is effectively the only place where 32bits would make sense (assuming we don't implement OR/UR for now).
- For oversampling we can have both 20/24 bit averaged data. But from this wording on the datasheet:
" Oversampling is useful in applications requiring lower noise and higher dynamic
range per output data-word, which the AD4858 supports with 24-bit output
resolution and reduced average output data rates"
So from the above it looks like it could make sense to default to 24bit packets if oversampling is enabled.
- Now the question is OR/UR. If that is something we can support with events, we could see when one of OR/UR is being requested to either enable 24 packets (no oversampling) or 32 bit packets (oversampling on).
Just some thoughts... Anyways, I think this will be interesting to see what Jonathan as to say about it.
}; | ||
MODULE_DEVICE_TABLE(spi, ad7949_spi_id); | ||
|
||
static struct spi_driver ad485x_driver = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just so it's not forgotten when upstreaming... The driver should be named some of the supported devices (instead of having the 'x' catch all). With that the APIs and variables should be updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really mandatory? I would not do that if from the beginning the driver adds support for an entire family. There are plenty of similar examples: drivers/iio/adc/ad799x.c, drivers/iio/adc/ep93xx_adc.c, drivers/iio/adc/ina2xx-adc.c, drivers/iio/adc/lpc18xx_adc.c...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How old are those drivers? Don't think it's "mandatory" but it is something typically asked (in IIO at least). Up to you but bear in mind that you can be asked to do this anyways.
|
||
for (i = 0; i < lane_num; i++) { | ||
c = ad485x_find_opt(&pn_status[i][0], 32, &s); | ||
opt_delay = s + c / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're still not taking into account the possibility of having no valid point...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above comment is still not addressed
9f9bebf
to
36688f2
Compare
v2:
|
36688f2
to
8872926
Compare
v3:
|
8872926
to
b8c8edd
Compare
v4:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some more comments from me :)
.driver = { | ||
.name = "ad485x", | ||
.of_match_table = ad485x_of_match, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id table missing here...
IIO_ENUM_AVAILABLE("packet_format", \ | ||
IIO_SHARED_BY_ALL, &ad4857_packet_fmt), \ | ||
{}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this packet format thingy may be a very "funny" discussion if we really need to support it. I'm not sure how useful it is the 32 bits format rather than being used in test pattern. I'm not seeing too much benefit on the channel id or span id information (can already get info with other attributes). The OR/UR is the one that could be more useful but is there someone using it? Do we really need to have it close to the sample? If not, there's the status register and... Also, I think this can be implemented using IIO events (likely what we will be asked). So what comes to mind could be:
- for test_pattern (could be implemented as ext_info I think - not for now I guess) we can easily look at our word side and dynamically set the proper packet size. So, to me, this is effectively the only place where 32bits would make sense (assuming we don't implement OR/UR for now).
- For oversampling we can have both 20/24 bit averaged data. But from this wording on the datasheet:
" Oversampling is useful in applications requiring lower noise and higher dynamic
range per output data-word, which the AD4858 supports with 24-bit output
resolution and reduced average output data rates"
So from the above it looks like it could make sense to default to 24bit packets if oversampling is enabled.
- Now the question is OR/UR. If that is something we can support with events, we could see when one of OR/UR is being requested to either enable 24 packets (no oversampling) or 32 bit packets (oversampling on).
Just some thoughts... Anyways, I think this will be interesting to see what Jonathan as to say about it.
drivers/iio/adc/ad485x.c
Outdated
IIO_ENUM("softspan", \ | ||
IIO_SEPARATE, &ad485x_softspan_enum), \ | ||
IIO_ENUM_AVAILABLE("softspan", \ | ||
IIO_SHARED_BY_TYPE, &ad485x_softspan_enum) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be removed from ext_info when upstreaming.
drivers/iio/adc/ad485x.c
Outdated
if (ret) | ||
return ret; | ||
*val = ad485x_softspan_range_mv[softspan]; | ||
*val2 = (1 << chan->scan_type.realbits) * 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe BIT(chan->scan_type.realbits)
so there's no danger for future signed integer overflows...
if (ret < 0) | ||
return ret; | ||
|
||
return regmap_write(st->regmap, AD485X_REG_CHX_OFFSET_MSB(ch), msb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
still needs the lock. Look at the new fancy API for it
|
||
gain = (reg_val & 0xFF) << 8; | ||
ret = regmap_read(st->regmap, AD485X_REG_CHX_GAIN_LSB(ch), | ||
®_val); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw, do we support bulk reads in the device? Thinking about regmap_bulk_read()
|
||
ret = iio_backend_interface_type_get(st->back, &interface_type); | ||
if (ret) | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: new line
drivers/iio/adc/ad485x.c
Outdated
if (ret) | ||
return ret; | ||
} else { | ||
ret = iio_backend_packet_size_set(st->back, AD4858_PACKET_SIZE_32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave a look into this and when upstreaming I really prefer other naming. Packet size is very specific... You're also right that this is not quite sample_size as we may have more data (kind of aggregated stuff). But this is effectively the data size at the bus/interface level. So we can try that direction... something iio_backend_data_size_set() or iio_backend_interface_bits_per_word() or even iio_backend_bus_bits_per_word().
Anyways, not important for now....
for (delay = 0; delay < AD485X_MAX_IODELAY; delay++) { | ||
ret = iio_backend_iodelay_set(st->back, i, delay); | ||
if (ret) | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment here could make sense to not raise questions on why we just check channel 0 in the LVDS case. Pretty much because all channels use the same lane/data lines so we just need to check the status on 1 channel, right? A comment in here or in the lane_num assignment
drivers/iio/adc/ad485x.c
Outdated
cnt = 0; | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think here we should have if (!max_cnt) return -EIO
. It may happen that we have no valid point in which case we need to error out.
1586006
to
a5ad55a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- reorganize commits in preparation for upstream
- add bindings and ABI commits
db17e4b
to
21157b0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
v5
- add bindings and ABI for packet format
- implement regulators
- implement scale and offset and drop softspan
ext_info
- add mutex locks where needed
- fix checkpatch issues
- add
id_table
- use
sign_extend32
- add new lines after
return ret;
- rename iio backend and axi adc
packet_size
todata_size
if (ret < 0)
->if (ret)
drivers/iio/adc/adi-axi-adc.c
Outdated
if (ret) | ||
return ret; | ||
|
||
*type = (val & ADI_AXI_ADC_REG_CONFIG_CMOS_OR_LVDS_N) ? 1 : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope :).
The above should be an indication that the order of the patches are wrong. The backend patch needs to come first and then please us the proper enum iio_backend_interface_type
value so it's more readable:
if (val & ADI_AXI_ADC_REG_CONFIG_CMOS_OR_LVDS_N)
*type = IIO_BACKEND_INTERFACE_CMOS
else
*type = IIO_BACKEND_INTERFACE_LVDS
drivers/iio/adc/adi-axi-adc.c
Outdated
|
||
ret = regmap_write(st->regmap, ADI_AXI_ADC_REG_CNTRL_3, size); | ||
if (ret) | ||
return ret; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return regmap_write()...
@@ -289,6 +304,7 @@ static const struct iio_backend_ops adi_axi_adc_generic = { | |||
.test_pattern_set = axi_adc_test_pattern_set, | |||
.chan_status = axi_adc_chan_status, | |||
.interface_type_get = axi_adc_interface_type_get, | |||
.data_size_set = axi_adc_data_size_set, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this patch (and the interface type one) are simple enough that they can be squashed together...
enum iio_backend_interface_type { | ||
IIO_BACKEND_LVDS, | ||
IIO_BACKEND_CMOS | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think IIO_BACKEND_INTERFACE_* may be a better name. Also squash this with the other op being added. I've done it before (as the code is typically simple). Example of me adding multiple ops:
maxItems: 1 | ||
|
||
pwm-names: | ||
const: cnv |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that we only have one pwm, do we need a name?
drivers/iio/adc/ad485x.c
Outdated
if (product_id != st->info->product_id) { | ||
dev_warn(&st->spi->dev, "Unknown product ID: 0x%02X\n", | ||
product_id); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: no need for {}
unsigned int (*scales)[2]; | ||
int offset_avail[2]; | ||
int *offsets; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the above two structures please remove the tab alignment and have a single space. Otherwise, adding new members may just introduce git diff noise if we have to re-align all the members.
{ | ||
return iio_backend_op_call(back, data_size_set, size); | ||
} | ||
EXPORT_SYMBOL_NS_GPL(iio_backend_data_size_set, IIO_BACKEND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you're not documenting the function. Also size should likely size_t or unsigned. Then, either the variable itself or the docs need to say what if the size is bits, bytes, etc... I guess size in bytes is the sensible choice in here.
include/linux/iio/backend.h
Outdated
@@ -120,6 +120,7 @@ struct iio_backend_ops { | |||
const struct iio_chan_spec *chan, char *buf); | |||
int (*interface_type_get)(struct iio_backend *back, | |||
enum iio_backend_interface_type *type); | |||
int (*data_size_set)(struct iio_backend *back, int size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you need to update the docs
Contact: [email protected] | ||
Description: | ||
This attribute configures the packet size. | ||
Reading returns the actual size used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prepare yourself to justify this attribute :)
21157b0
to
c335843
Compare
Signed-off-by: Antoniu Miclaus <[email protected]>
2e3c5ac
to
f248ed8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
implemented latest review changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the most important thing for now is the scale handling... I may be missing something but it does not look correct
drivers/iio/industrialio-backend.c
Outdated
@@ -464,6 +464,20 @@ int iio_backend_interface_type_get(struct iio_backend *back, | |||
} | |||
EXPORT_SYMBOL_NS_GPL(iio_backend_interface_type_get, IIO_BACKEND); | |||
|
|||
/** | |||
* iio_backend_data_size_set - set the data size / packet format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't mention packet... This needs better explanation. Mention this sets the data width/size in the data bus but keep it generic which means speak about data. Packet wording is very much the usecase of our device
* iio_backend_data_size_set - set the data size / packet format | ||
* @back: Backend device | ||
* @size: packet size in bits | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just "Size in bits". Also keep things consistent with the docs in the file. Like capital 'S' as in Backend...
This interface could also use some more explanation. Like:
"Some frontend devices can dynamically control the word/data size on the interface/data bus. Hence, the backend device needs to be aware of it so data can be correctly transferred."
drivers/iio/adc/adi-axi-adc.c
Outdated
break; | ||
default: | ||
return -EINVAL; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something worth asking hdl guys. Would something like this work?
if (size <= 20)
val = 0;
else if (size <= 24)
val = 1;
else if (size <= 32)
val = 3;
else
return -EINVAL;
See what I mean? Do the values need to be strict or let's say that a word size of 16bits would work on a 20bit configuration?
* 0 on success, negative error number on failure. | ||
*/ | ||
int iio_backend_data_size_set(struct iio_backend *back, ssize_t size) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Error out already in case !size
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just size_t
. No need to be signed
*/ | ||
if (!st->offsets[chan->channel] && !j) { | ||
j++; | ||
continue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the comment still does not explain that strange logic with j
. Why do we continue in case j == 0
? That's the odd thing in the logic that a comment would help IMO.
I suspect this has something to do with differential scales... If so, and looking now at the arrays, things are very hard to read and understand. It would be way more helpful to really have the real vales of the scales. Like (0 5000) for single ended and (-5000 5000) for differential.
I'm starting to think you need to double test the scale story as something looks fishy...
if (i == info->num_scales) | ||
return -EIO; | ||
|
||
__ad485x_get_scale(st, info->scale_table[i][0], val, val2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's imagine you get the value 2
from the device. If I ready things right, that will give 5000 (and I'm assuming this one is differential). From what I can see 5000 is the value used in __ad485x_get_scale()
, right? Isn't that wrong? In the differential case our full scale should be 10000... Look at
https://elixir.bootlin.com/linux/v6.10/source/drivers/iio/dac/ltc2688.c
as an example...
In your case it may be useful to have an helper struct like:
struct foo {
int min;
int max;
unsigned int reg;
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand here. Shouldn't the scale be the same in both single-ended and differential mode? And by looking at the offset you can determine which of the 2 modes it is actually. If we read both reg value 1 or reg value 2 the scale is the same (the range is 5V). Only the offset differs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand here. Shouldn't the scale be the same in both single-ended and differential mode?
Not really. Typically the scale is FS / 2^n_bits right? So in case you have 0 - 5V single ended´, you'll have a 5V full scale. In case you have -5 to 5, you have 10V full scale.
And by looking at the offset you can determine which of the 2 modes it is actually
Ok, I also misunderstood/misread things a bit (I now see the proper FS is already mapped on the register value). Actually is not the scale that depends on the offset. Its the other away around. In fact you should not allow users to write the offset value or have it in the read available because that's just wrong. You cannot set your offset at -32768 and not change the scale because that's wrong. In case you have a single ended span, your offset is actually 0. Think on the biggest value you can represent and do the calculation with a negative offset. Imagine we have 5v fs and 12 bit ADC. A 5V read you'll have a raw value of 4095 and so your voltage is given by 4095 * 5 / 4096 (not the scale). If you have a negative offset before the raw value you'll get the wrong result. Now if you have differential 5V, your math should be (-2048 + 4095) * 10 / 4096.
And a raw value of 0 should represent the minimum value on your scale. So from the above you would have the correct 0 and -5V when properly applying the right offset.
In conclusion, you only have a negative offset in case you have a differential span and you should reflect that when reading IIO_CHAN_INFO_OFFSET.
Again, look at how things are handled in ltc2688:
https://elixir.bootlin.com/linux/v6.10/source/drivers/iio/dac/ltc2688.c#L175
It's a dac but it still applies. It also has this multiple span capability. Only difference is that being a dac (and possibly in control of other HW) the desired span is set at devicetree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. I had a look at the example you provided and it makes sense to do the things that way for the read side. Although the example you provided does not implement the scale/offset adjustment from the userspace, only the readings. And we want to be able to adjust the span from the userspace for each channel independently. How should I do that? I need both offset and scale values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we want to be able to adjust the span from the userspace for each channel independently. How should I do that? I need both offset and scale values.
You should just allow to set scale. I don#t think userspace should have anything to do with offset. The offset will depend on the selected span. If single-ended, it's 0... If differential, it's negative. Just adjust the offset reading in accordance with the current span. I would not over-complicate things
drivers/iio/adc/ad485x.c
Outdated
return -EINVAL; | ||
} | ||
|
||
ret = iio_backend_data_size_set(st->back, format); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
val
...
drivers/iio/adc/ad485x.c
Outdated
break; | ||
case 2: | ||
val = 32; | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about 16 bits?
drivers/iio/adc/ad485x.c
Outdated
for (i = 0; i < lane_num; i++) { | ||
c = ad485x_find_opt(&pn_status[i][0], AD485X_MAX_IODELAY, &s); | ||
if (c < 0) | ||
return -EIO; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it's the same but for correctness... return c
f248ed8
to
12e32e1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changelog:
- drop packet mentioning
- fix header comments for data_size_set
- rework size set in axi-adc
- handle error in interface_type_get
- add more locks
- drop redundant
return IIO_VAL_INT
- add scoped_guard
- improve comment on the
j
logic - handle 16 bit size in ad485x
return c
NOTE:- still need to maybe rework offset/scale handling
12e32e1
to
929993a
Compare
Add backend support for obtaining the interface type used. Signed-off-by: Antoniu Miclaus <[email protected]>
Add backend support for setting the data size used. Signed-off-by: Antoniu Miclaus <[email protected]>
Add support for getting the interface (CMOS or LVDS) used by the AXI ADC ip. Signed-off-by: Antoniu Miclaus <[email protected]>
Add support for selecting the data format within the AXI ADC ip. Signed-off-by: Antoniu Miclaus <[email protected]>
Add devicetree bindings for ad458x DAS family. Signed-off-by: Antoniu Miclaus <[email protected]>
Add support for the AD485X DAS familiy. Signed-off-by: Antoniu Miclaus <[email protected]>
Add documentation for the packet size. Signed-off-by: Antoniu Miclaus <[email protected]>
929993a
to
13a1fff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think two main things for now is packet_format and the scale / offset handling. Anyways, take my inputs and see what you prefer to do :) and let's bring the discussion upstream... Likely you'll need to prepare a nice cover stating what we want to have with the span (if not going with DT approach) and justification for the packet format non standard attr.
return ret; | ||
|
||
if (*type > IIO_BACKEND_INTERFACE_CMOS) | ||
return -EINVAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok sorry for the above. I think I somehow though this was doing a set
and not a get
. For now, I would likely leave it up to frontend to deal with the returned type. Also not -EINVAL
is not too accurate as the frontend as no fault about this :)
* 0 on success, negative error number on failure. | ||
*/ | ||
int iio_backend_data_size_set(struct iio_backend *back, ssize_t size) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just size_t
. No need to be signed
IIO_ENUM_AVAILABLE("packet_format", | ||
IIO_SHARED_BY_ALL, &ad4858_packet_fmt), | ||
{}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have an ABI file for this already?
if (i == info->num_scales) | ||
return -EIO; | ||
|
||
__ad485x_get_scale(st, info->scale_table[i][0], val, val2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And we want to be able to adjust the span from the userspace for each channel independently. How should I do that? I need both offset and scale values.
You should just allow to set scale. I don#t think userspace should have anything to do with offset. The offset will depend on the selected span. If single-ended, it's 0... If differential, it's negative. Just adjust the offset reading in accordance with the current span. I would not over-complicate things
AD485X_SOFTSPAN_0V_40V); | ||
} | ||
|
||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the latest comment, I would drop ad485x_set_offset()
* Adjust the softspan value (differential or single ended) | ||
* based on the scale value selected and current offset of | ||
* the channel. | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, I think this can be simplified... offset should have nothing to do. Ok, I think I see what's the issue... Only by looking at the scale you can't know if you should use single-ended or differential in cases you have the same scale value). But still what you're doing in setting the offset is very questionable. Like, change the value and then all of the sudden our scale also changes. Maybe this is odd enough (changing differential vs single-ended scales) that we should just have scale as a DT parameter? Or is there a real request for having it as runtime? I see three alternatives for now:
- Dt parameter. By far the easiest solution :)
- Other (also questionable) approach would be to have the read_available to report negative scales for the differential ones (so we could use the sign to determine single ended or not. Maybe having this documented would be enough. And note that reading the scale attr (not the the available one) should still only report positive scales.
- Keep it like this but don't assume any default when setting the offset. If you can change the offset in a way that scale can be kept, cool. If not, I would return an error and force users to then first set the scale they want to have. Also, you should only allow two values for the offset. If not suggesting anything dumb, this pretty much means that you can only toggle the offset for scales that exist both single-ended and differential (example: [-5 5] and [0 10]. On the scale side of things you'd only update the offset value in case the scale being requested to be unique (either it exists only in the single-ended or differential form).
Option 3) looks rather complex if it makes sense at all...
if (ret) | ||
return ret; | ||
|
||
format &= AD485X_PACKET_FORMAT_MASK; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FIELD_GET()
?
case 0: | ||
val = 16; | ||
break; | ||
case 1: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some defines would help probably...
|
||
static const char *const ad4857_packet_fmts[] = { | ||
"16-bit", "24-bit" | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be prepared to really justify the packet format stuff :)
ID_AD4852, | ||
ID_AD4851, | ||
ID_AD4858I, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the above used?
PR Description
Support for AD4858, AD4857, AD4856, AD4855, AD4854, AD4853, AD4852, AD4851, AD4858I, on top of Linux 6.10-rc5, to be sent upstream.
PR Type
PR Checklist