On Sun, 25 Jul 2021 13:18:53 +0100
"Lad, Prabhakar" [off-list ref] wrote:

Hi Jonathan,

Thank you for the review.

On Sat, Jul 24, 2021 at 7:03 PM Jonathan Cameron [off-list ref] wrote:

quoted

On Mon, 19 Jul 2021 09:58:38 +0100
Lad Prabhakar [off-list ref] wrote:
 

quoted

Add ADC driver support for Renesas RZ/G2L A/D converter in SW
trigger mode.

A/D Converter block is a successive approximation analog-to-digital
converter with a 12-bit accuracy and supports a maximum of 8 input
channels.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>  

Hi Lad,

A few additional comments inline.

Thanks,

Jonathan
 

...

quoted

quoted

+#define RZG2L_ADFIL                  0x2c
+#define RZG2L_ADCR(n)                        (0x30 + ((n) * 0x4))
+#define RZG2L_ADCR_AD_MASK           GENMASK(11, 0)
+
+#define RZG2L_ADC_MAX_CHANNELS               8
+#define RZG2L_ADC_CHN_MASK           0x7
+#define RZG2L_ADC_TIMEOUT            usecs_to_jiffies(1 * 4)
+
+struct rzg2l_adc_data {
+     const struct iio_chan_spec *channels;
+     u8 num_channels;
+};
+
+struct rzg2l_adc {
+     void __iomem *base;
+     struct clk *pclk;
+     struct clk *adclk;
+     struct reset_control *presetn;
+     struct reset_control *adrstn;
+     struct completion completion;
+     const struct rzg2l_adc_data *data;
+     u16 last_val[RZG2L_ADC_MAX_CHANNELS];
+};
+
+static const char * const rzg2l_adc_channel_name[] = {
+     "adc0",  

Is it useful to expose these as labels to userspace?
Seems unnecessary given they map directly to the channel
numbers.
 

Exposing  them as this may vary depending on the wiring on the board,
so it  would be better for user space to know which channels are
available.

Hmm.  One thing to take into account is the IIO ABI doesn't require
channel numbers to be consecutive.  There are a few drivers where
they aren't due to channel optionality.

Perhaps that would make sense here?  If not, I'm fine with leaving
these as you have it.  They do no harm.

quoted

quoted

+     "adc1",
+     "adc2",
+     "adc3",
+     "adc4",
+     "adc5",
+     "adc6",
+     "adc7",
+};
+

...

quoted

quoted

+static int rzg2l_adc_conversion_setup(struct rzg2l_adc *adc, u8 ch)
+{
+     u32 reg;
+
+     if (rzg2l_adc_readl(adc, RZG2L_ADM(0)) & RZG2L_ADM0_ADBSY)
+             return -EBUSY;
+
+     rzg2l_set_trigger(adc);
+
+     /* select channel */
+     reg = rzg2l_adc_readl(adc, RZG2L_ADM(2));
+     reg &= RZG2L_ADM2_CHSEL_CLEAR;
+     reg |= BIT(ch);
+     rzg2l_adc_writel(adc, RZG2L_ADM(2), reg);
+
+     reg = rzg2l_adc_readl(adc, RZG2L_ADM(3));
+     reg &= RZG2L_ADM3_ADIL_CLEAR;
+     reg |= RZG2L_ADM3_ADCMP;
+     reg |= RZG2L_ADM3_ADSMP;  

No loss in readability in combining the two lines above and shorter
code is always good.  Having the mask on a separate line makes
sense but the |= pair doesn't.
 

Agreed will move to the same line.

quoted

What is a bit unusual here is you clear some bits then write different
ones.  That is presumably relying on the fact that the ADCCMP part
is always set to the same value.  That seems unwise to assume from
a long term maintainability point of view.
 

The ADIL bits have to be set to zero, so I am clearing the ADIL bits
(bit 24-bits 31) and the ADCMP/ADSMP should be set to specific values
oxe/0x578 respectively.

Understood, but from this 'local' bit of code it's not obvious that they don't
have other bits say, perhaps ADSMP is set to 0x483 for example by some
other code? (obviously it isn't, but it's nice to not have to sanity check
the rest of the driver to be sure!)  So normal thing to do would be to also
mask those bits out. 

I'm a bit curious on whether there are other bits in this register that make it
useful to do the read modify write cycle? (doesn't seem to be a public
datasheet from a quick google...)

quoted

quoted

+     rzg2l_adc_writel(adc, RZG2L_ADM(3), reg);
+
+     reg = rzg2l_adc_readl(adc, RZG2L_ADIVC);
+     reg &= RZG2L_ADIVC_DIVADC_CLEAR;
+     reg |= RZG2L_ADIVC_DIVADC_4;
+     rzg2l_adc_writel(adc, RZG2L_ADIVC, reg);
+
+     reg = rzg2l_adc_readl(adc, RZG2L_ADINT);
+     reg &= ~RZG2L_ADINT_INTS;
+     reg &= RZG2L_ADINT_CH_CLEAR;
+     reg |= RZG2L_ADINT_CSEEN;
+     reg |= BIT(ch);
+     rzg2l_adc_writel(adc, RZG2L_ADINT, reg);
+
+     return 0;
+}
+

...

Thanks,

Jonathan