Hi Jonathan,
Thanks for the feedback,
> Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC
>
> On Sun, 1 May 2022 09:15:23 +0100
> Biju Das <biju.das.jz@xxxxxxxxxxxxxx> wrote:
>
> > ADC found on RZ/G2UL SoC is almost identical to RZ/G2L SoC, but
> > RZ/G2UL has 2 analog input channels compared to 8 channels on RZ/G2L.
> > Therefore, added a new compatible to handle this difference.
> >
> > Signed-off-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Hi.
>
> Please keep the driver changes and DT update in a series with a short
> cover letter. It makes much more sense to apply them when both ready than
> to end up with them being handled separately.
OK, will send as a series on next version.
>
> A request inline to do this in a slightly different way that will prove
> more flexible if we end up supporting more variants in the future.
>
> > ---
> > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++-----
> > 1 file changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iio/adc/rzg2l_adc.c b/drivers/iio/adc/rzg2l_adc.c
> > index 7585144b9715..703b08254c9f 100644
> > --- a/drivers/iio/adc/rzg2l_adc.c
> > +++ b/drivers/iio/adc/rzg2l_adc.c
> > @@ -16,6 +16,7 @@
> > #include <linux/io.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/module.h>
> > +#include <linux/of_device.h>
> > #include <linux/platform_device.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/reset.h>
> > @@ -61,6 +62,8 @@
> > #define RZG2L_ADC_CHN_MASK 0x7
> > #define RZG2L_ADC_TIMEOUT usecs_to_jiffies(1 * 4)
> >
> > +#define RZG2UL_ADC_MAX_CHANNELS 2
> > +
> > struct rzg2l_adc_data {
> > const struct iio_chan_spec *channels;
> > u8 num_channels;
> > @@ -76,6 +79,7 @@ struct rzg2l_adc {
> > const struct rzg2l_adc_data *data;
> > struct mutex lock;
> > u16 last_val[RZG2L_ADC_MAX_CHANNELS];
> > + u8 max_channels;
> > };
> >
> > static const char * const rzg2l_adc_channel_name[] = { @@ -260,7
> > +264,9 @@ static int rzg2l_adc_read_label(struct iio_dev *iio_dev,
> > const struct iio_chan_spec *chan,
> > char *label)
> > {
> > - if (chan->channel >= RZG2L_ADC_MAX_CHANNELS)
> > + struct rzg2l_adc *adc = iio_priv(iio_dev);
> > +
> > + if (chan->channel >= adc->max_channels)
> > return -EINVAL;
> >
> > return sysfs_emit(label, "%s\n",
> > rzg2l_adc_channel_name[chan->channel]);
> > @@ -290,7 +296,7 @@ static irqreturn_t rzg2l_adc_isr(int irq, void
> *dev_id)
> > if (!intst)
> > return IRQ_NONE;
> >
> > - for_each_set_bit(ch, &intst, RZG2L_ADC_MAX_CHANNELS)
> > + for_each_set_bit(ch, &intst, adc->max_channels)
> > adc->last_val[ch] = rzg2l_adc_readl(adc, RZG2L_ADCR(ch)) &
> > RZG2L_ADCR_AD_MASK;
> >
> > /* clear the channel interrupt */
> > @@ -321,7 +327,7 @@ static int rzg2l_adc_parse_properties(struct
> platform_device *pdev, struct rzg2l
> > return -ENODEV;
> > }
> >
> > - if (num_channels > RZG2L_ADC_MAX_CHANNELS) {
> > + if (num_channels > adc->max_channels) {
> > dev_err(&pdev->dev, "num of channel children out of range\n");
> > return -EINVAL;
> > }
> > @@ -337,7 +343,7 @@ static int rzg2l_adc_parse_properties(struct
> platform_device *pdev, struct rzg2l
> > if (ret)
> > return ret;
> >
> > - if (channel >= RZG2L_ADC_MAX_CHANNELS)
> > + if (channel >= adc->max_channels)
> > return -EINVAL;
> >
> > chan_array[i].type = IIO_VOLTAGE;
> > @@ -437,6 +443,7 @@ static int rzg2l_adc_probe(struct platform_device
> > *pdev)
> >
> > adc = iio_priv(indio_dev);
> >
> > + adc->max_channels = (uintptr_t)of_device_get_match_data(dev);
>
> For IIO drivers, where possible please use the generic firmware
> properties.
> The driver already uses them for everything else. Hence
>
> device_get_match_data(dev);
OK, will use device_get_match_data(dev);
>
> > ret = rzg2l_adc_parse_properties(pdev, adc);
> > if (ret)
> > return ret;
> > @@ -540,7 +547,8 @@ static int rzg2l_adc_probe(struct platform_device
> > *pdev) }
> >
> > static const struct of_device_id rzg2l_adc_match[] = {
> > - { .compatible = "renesas,rzg2l-adc",},
> > + { .compatible = "renesas,r9a07g043-adc", .data = (void
> *)RZG2UL_ADC_MAX_CHANNELS },
> > + { .compatible = "renesas,rzg2l-adc", .data = (void
> > +*)RZG2L_ADC_MAX_CHANNELS },
> Whilst it is more work, I'd prefer that you introduce a small structure to
> represent chip type specific information then have an array of those.
> Finally store a pointer to an element of that array in here.
OK. will use SoC info structure pointing to the compatible.
>
> We almost always end up needing to add more chip type specific data over
> time and so it is better to provide the means to do so flexibly in the
> first patch where such support is added.
Agreed.
Thanks,
Biju
>
> > { /* sentinel */ }
> > };
> > MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
