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.
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);
> 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.
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.
Thanks,
Jonathan
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
