On Fri, 1 Jan 2021 22:41:35 +0000 "Denis, Tomislav AVL DiTEST" <Tomislav.Denis@xxxxxxx> wrote: > Hi Jonathan, > > Thanks for great review and hints that you gave me. > A few comments inline. > > BR, > > Tomislav > Replies inline. > > > +title: Texas Instruments ADS131E0x 4-, 6-, and 8-Channel ADCs > > > > Not currently supporting 6 channel variants? > > It should be working without problem! But since I don't have HW to test I've left it out. > Personally I'd just be an optimist and put it in. Seems very unlikely it won't work given you have variants on either side of it. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [0, 1, 2] > > > + default: 0 > > > + > > > + ti,datarate: > > > + description: | > > > + ADC data rate in kSPS > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + enum: [1, 2, 4, 8, 16, 32, 64] > > > + default: 1 > > > > Why is this a devicetree element rather than runtime controllable? > > Number of data bytes per channel depends on data rate. To be able to change data rate > dynamically would require to change scan_type.realbits also dynamically! I am not sure > if that make sense and also if it is possible to do it? Indeed not possible to do runtime variable resolution currently. We have talked about implementing it a few times, but it's rather fiddly to do hence hasn't happened yet. hmm. It might be better to control the channel resolution in the device tree as that feels a bit less like something that ought to be runtime controllable. >From a quick look at the datasheet it looks like you can have the same data width for 32 and 64 ksps (16 bits) and the same for all the other rates (24 bits) However, given you are using the same number of storage bits anyway, you could be more cynical and claim 24 bits for all channels and just rely on the upper bits being 0 for the 16 bit cases. That way this would just become a userspace sampling frequency control with slightly missleading apparent precision + the need to stash the realbits value you are using for scale somewhere else. If we do this we end up with entirely standard userspace interface and no need for this control in DT. Jonathan
