RE: [PATCH v2 2/3] iio: dac: ad5706r: Add support for AD5706R DAC

[Torreno, Alexis Czezar]

> ...
> 
> > +#include <linux/device.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/spi/spi.h>
> 
> IWYU, please.
> Yet another typical issue with ADI patches. Really, talk to your senior colleagues
> who are experienced kernel developers before sending it to ML.
> 

I'll bring this up. As per your suggestion in the other mail, compiling common issues
like this may be needed. Lessens pitfalls for newer devs.

> ...
> 
> > +#define AD5706R_DAC_RESOLUTION		16
> > +#define AD5706R_DAC_MAX_CODE
> 	BIT(AD5706R_DAC_RESOLUTION)  /* 65536 */
> 
> Useless comment, use the same number explicitly instead
> 
> 	BIT(16)
> 
> which is much more readable.
> 
> ...
> 
> > +	st->tx_buf = cpu_to_be32((((u32)reg) << 16) |
> > +			((u32)val << (16 - (num_bytes * 8))));
> 
> What the heck is this?

I'll make this more readable.
but the format goal is:

if num_bytes = 1
    tx_buf [31:0] = [reg 31:16] [val 15:8] [ XXXX]
if num_bytes = 2
    tx_buf [31:0] = [reg 31:16] [      val 15:0        ]

> 
> ...
> 
> > +static const struct of_device_id ad5706r_of_match[] = {
> > +	{ .compatible = "adi,ad5706r" },
> > +	{ },
> 
> Even inside a single file this is inconsistent, besides being third of
> 101 IIO	patch issues.

Will note the other feedback and track the common issues.
Thank you,