On Mon, 2018-10-08 at 23:18 +0200, Couret Charles-Antoine wrote:
> 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?
A lot of drivers define something like:
enum {
ID_AD7949,
ID_AD7682,
ID_AD7689,
}
which can be refered to as "part number", and then you can use these enum
values to identify behavior and configuration for each device the driver
supports.
This method is preferred, because when/if a new chip comes along that fits
into this driver (let's say ID_ADXXYZ), and may have QUAD_16BITS and
differs in some other minor aspect, it can be easier to identify via the
part-number. Or, in some cases, some chips get a newer revision (example:
ID_AD7949B) that may differ slightly (from ID_AD7949).
>
> > > + 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?
I think in such a case, a lock/mutex would be needed.
As far as I remember, kernel SPI calls should have their own locks for
single SPI transactions, so maybe some locks for accessing the chip during
a set of SPI transactions would be neater.
>
> >
> > > + 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.
>
Alex
> Regards,
> Charles-Antoine Couret
>
