Le 07/10/2018 à 18:25, Jonathan Cameron a écrit :
Hi Charles-Antoine,
Hello,
Thank you for your code review, I'm trying to take everything into
account but I have some questions about it before making the V2.
+#define AD7949_OFFSET_CHANNEL_SEL 7
+#define AD7949_CFG_READ_BACK 0x1
+#define AD7949_CFG_REG_SIZE_BITS 14
+
+enum {
+ HEIGHT_14BITS = 0,
+ QUAD_16BITS,
+ HEIGHT_16BITS,
Height? I guess EIGHT was the intent.
I would just use the part numbers for this rather than a
descriptive phrase.
Thank you for the typo.
But I don't understand your remark. What do you mean by "part numbers"
here?
+ struct spi_message msg;
+ struct spi_transfer tx[] = {
+ {
+ .tx_buf = &buf_value,
+ .len = 4,
+ .bits_per_word = bits_per_word,
+ },
+ };
+
+ ad7949_adc->cfg = buf_value >> shift;
+ spi_message_init(&msg);
+ spi_message_add_tail(&tx[0], &msg);
+ ret = spi_sync(ad7949_adc->spi, &msg);
+ udelay(2);
These delays need explaining as they are non obvious and there
may be cleaner ways to handle them.
I want to add this comment:
/* This delay is to avoid a new request before the required time to
* send a new command to the device
*/
It is clear and relevant enough?
+ struct spi_message msg;
+ struct spi_transfer tx[] = {
+ {
+ .rx_buf = &buf_value,
+ .len = 4,
+ .bits_per_word = bits_per_word,
+ },
+ };
I 'think' this is the same in every call of this function, so why not
set it up in the probe function and have it ready all the time rather than
spinning up a new copy here each time.
bits_per_word depends on if the configuration readback is enabled or
not. It could be changed in runtime. So I considered we can not optimize
in that way.
Regards,
Charles-Antoine Couret
