| Message ID | 20220501081523.22479-1-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC | expand |
On Sun, 1 May 2022 09:15:23 +0100 Biju Das <biju.das.jz@bp.renesas.com> 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@bp.renesas.com> 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);
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@bp.renesas.com> 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@bp.renesas.com> > > 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);
Hi Biju, Thank you for the patch. > -----Original Message----- > From: Biju Das <biju.das.jz@bp.renesas.com> > Sent: 01 May 2022 09:15 > To: Jonathan Cameron <jic23@kernel.org> > Cc: Biju Das <biju.das.jz@bp.renesas.com>; Prabhakar Mahadev Lad > <prabhakar.mahadev-lad.rj@bp.renesas.com>; Lars-Peter Clausen > <lars@metafoo.de>; linux-iio@vger.kernel.org; linux-renesas- > soc@vger.kernel.org; Geert Uytterhoeven <geert+renesas@glider.be>; Chris > Paterson <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com> > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > 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@bp.renesas.com> > --- > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > 1 file changed, 13 insertions(+), 5 deletions(-) > I wonder if this changes is really required. RZ/G2UL can still use the "renesas,rzg2l-adc". As the driver populates the channels depending the number of elements in the array passed in the DTS and not always 8 channels. For example on Renesas SMARC EVK only four channels are populated. With this we don't have to differentiate RZ/G2UL SoC if just add two channel entries in the SoC DTSI and the driver will just create two channels. @Geert - your thoughts on this. Cheers, Prabhakar > 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); > 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 }, > { /* sentinel */ } > }; > MODULE_DEVICE_TABLE(of, rzg2l_adc_match); > -- > 2.25.1
Hi Prabhakar, Thanks for the review. > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Biju, > > Thank you for the patch. > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > 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@bp.renesas.com> > > --- > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > I wonder if this changes is really required. RZ/G2UL can still use the > "renesas,rzg2l-adc". As the driver populates the channels depending the > number of elements in the array passed in the DTS and not always 8 > channels. For example on Renesas SMARC EVK only four channels are > populated. > For me that restriction is coming from board design, as SoC is capable of handling 8 channels, But board design allows only 4. But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other channels are invalid for RZ/G2UL SoC. That is the difference. > With this we don't have to differentiate RZ/G2UL SoC if just add two > channel entries in the SoC DTSI and the driver will just create two > channels. > > @Geert - your thoughts on this. > > Cheers, > Prabhakar > > > 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); > > 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 }, > > { /* sentinel */ } > > }; > > MODULE_DEVICE_TABLE(of, rzg2l_adc_match); > > -- > > 2.25.1
Hi Biju, On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > > > 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@bp.renesas.com> > > > --- > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > I wonder if this changes is really required. RZ/G2UL can still use the > > "renesas,rzg2l-adc". As the driver populates the channels depending the > > number of elements in the array passed in the DTS and not always 8 > > channels. For example on Renesas SMARC EVK only four channels are > > populated. > > For me that restriction is coming from board design, as SoC is capable of handling 8 channels, > But board design allows only 4. > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other channels are invalid for RZ/G2UL SoC. > > That is the difference. > > > With this we don't have to differentiate RZ/G2UL SoC if just add two > > channel entries in the SoC DTSI and the driver will just create two > > channels. > > > @Geert - your thoughts on this. It depends on the meaning of the channel subnodes: do they indicate (a) the number of channels present on the SoC, or (b) the number of channels used on the board? The DT bindings are not clear about that. arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and doesn't keep any disabled, which suggests (a). arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused channels, which suggests (b). Is there any (perhaps performance?) reason we can't just use the number of channels present in DT? "make dtbs_check" can still validate this against the SoC-specific compatible value. Do we need to know at runtime both the number of channels physically present and the number of channels used? If yes, we either need to use the SoC-specific compatible value, or add a num-channels property. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Biju, > > On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > ADC > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > > > > > 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@bp.renesas.com> > > > > --- > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > I wonder if this changes is really required. RZ/G2UL can still use > > > the "renesas,rzg2l-adc". As the driver populates the channels > > > depending the number of elements in the array passed in the DTS and > > > not always 8 channels. For example on Renesas SMARC EVK only four > > > channels are populated. > > > > For me that restriction is coming from board design, as SoC is capable > > of handling 8 channels, But board design allows only 4. > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other > channels are invalid for RZ/G2UL SoC. > > > > That is the difference. > > > > > With this we don't have to differentiate RZ/G2UL SoC if just add two > > > channel entries in the SoC DTSI and the driver will just create two > > > channels. > > > > > @Geert - your thoughts on this. > > It depends on the meaning of the channel subnodes: do they indicate > (a) the number of channels present on the SoC, or (b) the number of > channels used on the board? The DT bindings are not clear about that. > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and doesn't > keep any disabled, which suggests (a). > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused > channels, which suggests (b). > > Is there any (perhaps performance?) reason we can't just use the number of > channels present in DT? "make dtbs_check" can still validate this against > the SoC-specific compatible value. No. Krzysztof Kozlowski suggested to validate the number of channels present on the SoC as there is a difference in hardware capability between the SoCs. > > Do we need to know at runtime both the number of channels physically > present and the number of channels used? If yes, we either need to use > the SoC-specific compatible value, or add a num-channels property. Yes, currently driver does the validation with RZG2L_ADC_MAX_CHANNELS(8) which is wrong for RZ/G2UL as it has only 2 Channels. That is the reason, new SoC-specific compatible introduced to take care of this difference. Currently I have done the below changes, which restricts the usage of channel > 1 in DT for RZ/G2UL. + reg: true required: - reg additionalProperties: false +allOf: + - if: + properties: + compatible: + contains: + const: renesas,r9a07g043-adc + then: + patternProperties: + "^channel@[0-7]$": + type: object + properties: + reg: + description: | + The channel number. It can have up to 2 channels numbered from 0 to 1. + items: + - minimum: 0 + maximum: 1 + - if: + properties: + compatible: + contains: + const: renesas,rzg2l-adc + then: + patternProperties: + "^channel@[0-7]$": + type: object + properties: + reg: + description: | + The channel number. It can have up to 8 channels numbered from 0 to 7. + items: + - minimum: 0 + maximum: 7 Cheers, Biju
> -----Original Message----- > From: Geert Uytterhoeven <geert@linux-m68k.org> > Sent: 02 May 2022 15:29 > To: Biju Das <biju.das.jz@bp.renesas.com> > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; > Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; > linux-iio@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Chris > Paterson <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com> > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Biju, > > On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > ADC > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > > > > > 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@bp.renesas.com> > > > > --- > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > I wonder if this changes is really required. RZ/G2UL can still use > > > the "renesas,rzg2l-adc". As the driver populates the channels > > > depending the number of elements in the array passed in the DTS and > > > not always 8 channels. For example on Renesas SMARC EVK only four > > > channels are populated. > > > > For me that restriction is coming from board design, as SoC is capable > > of handling 8 channels, But board design allows only 4. > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other > channels are invalid for RZ/G2UL SoC. > > > > That is the difference. > > > > > With this we don't have to differentiate RZ/G2UL SoC if just add two > > > channel entries in the SoC DTSI and the driver will just create two > > > channels. > > > > > @Geert - your thoughts on this. > > It depends on the meaning of the channel subnodes: do they indicate > (a) the number of channels present on the SoC, or (b) the number of > channels used on the board? The DT bindings are not clear about that. > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and doesn't > keep any disabled, which suggests (a). > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused > channels, which suggests (b). > Yep its (b), since the SoC can support 8 channels the RZ/G2L SoC DTSI has 8 entries, If there comes a new EVK based on RZ/RZ/G2L SOC supporting all the channels so this holds good. > Is there any (perhaps performance?) reason we can't just use the number of > channels present in DT? "make dtbs_check" can still validate this against > the SoC-specific compatible value. > Nope performance issues. That is what the code does [0], It counts the number of available channels in DTS and depending on the count it populates the ADC channels. So for RZ/G2UL if we just add two channels in the SoC DTSI this holds good and the driver shall populate only two channels. And as you said the validation for the RZ/G2UL SoC for just two channels will be done by make dtbs_check and in the driver the condition still holds good 2 < 8. > Do we need to know at runtime both the number of channels physically > present and the number of channels used? If yes, we either need to use > the SoC-specific compatible value, or add a num-channels property. > At runtime we just need to know the number of channels used on the board. [0]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n335 Cheers, Prabhakar > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
Hi Prabhakar, On Tue, May 3, 2022 at 8:32 AM Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com> wrote: > > -----Original Message----- > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Sent: 02 May 2022 15:29 > > To: Biju Das <biju.das.jz@bp.renesas.com> > > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; > > Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; > > linux-iio@vger.kernel.org; linux-renesas-soc@vger.kernel.org; Chris > > Paterson <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com> > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > ADC > > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > > > > > > > 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@bp.renesas.com> > > > > > --- > > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > I wonder if this changes is really required. RZ/G2UL can still use > > > > the "renesas,rzg2l-adc". As the driver populates the channels > > > > depending the number of elements in the array passed in the DTS and > > > > not always 8 channels. For example on Renesas SMARC EVK only four > > > > channels are populated. > > > > > > For me that restriction is coming from board design, as SoC is capable > > > of handling 8 channels, But board design allows only 4. > > > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other > > channels are invalid for RZ/G2UL SoC. > > > > > > That is the difference. > > > > > > > With this we don't have to differentiate RZ/G2UL SoC if just add two > > > > channel entries in the SoC DTSI and the driver will just create two > > > > channels. > > > > > > > @Geert - your thoughts on this. > > > > It depends on the meaning of the channel subnodes: do they indicate > > (a) the number of channels present on the SoC, or (b) the number of > > channels used on the board? The DT bindings are not clear about that. > > > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and doesn't > > keep any disabled, which suggests (a). > > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused > > channels, which suggests (b). > > > Yep its (b), since the SoC can support 8 channels the RZ/G2L SoC DTSI has 8 entries, If there comes a new EVK based on RZ/RZ/G2L SOC supporting all the channels so this holds good. > > > Is there any (perhaps performance?) reason we can't just use the number of > > channels present in DT? "make dtbs_check" can still validate this against > > the SoC-specific compatible value. > > > Nope performance issues. That is what the code does [0], It counts the number of available channels in DTS and depending on the count it populates the ADC channels. So for RZ/G2UL if we just add two channels in the SoC DTSI this holds good and the driver shall populate only two channels. And as you said the validation for the RZ/G2UL SoC for just two channels will be done by make dtbs_check and in the driver the condition still holds good 2 < 8. > > > Do we need to know at runtime both the number of channels physically > > present and the number of channels used? If yes, we either need to use > > the SoC-specific compatible value, or add a num-channels property. > > > At runtime we just need to know the number of channels used on the board. Then I think the driver can just match against the family-specific compatible value, while "make dtbs_check" can use the SoC-specific compatible value to validate the number of channels. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
> > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > > Hi Biju, > > > > On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > ADC > > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > > ADC > > > > > > > > > > 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@bp.renesas.com> > > > > > --- > > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > I wonder if this changes is really required. RZ/G2UL can still use > > > > the "renesas,rzg2l-adc". As the driver populates the channels > > > > depending the number of elements in the array passed in the DTS > > > > and not always 8 channels. For example on Renesas SMARC EVK only > > > > four channels are populated. > > > > > > For me that restriction is coming from board design, as SoC is > > > capable of handling 8 channels, But board design allows only 4. > > > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other > > channels are invalid for RZ/G2UL SoC. > > > > > > That is the difference. > > > > > > > With this we don't have to differentiate RZ/G2UL SoC if just add > > > > two channel entries in the SoC DTSI and the driver will just > > > > create two channels. > > > > > > > @Geert - your thoughts on this. > > > > It depends on the meaning of the channel subnodes: do they indicate > > (a) the number of channels present on the SoC, or (b) the number of > > channels used on the board? The DT bindings are not clear about that. > > > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and > > doesn't keep any disabled, which suggests (a). > > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused > > channels, which suggests (b). > > > Yep its (b), since the SoC can support 8 channels the RZ/G2L SoC DTSI has > 8 entries, If there comes a new EVK based on RZ/RZ/G2L SOC supporting all > the channels so this holds good. > > > Is there any (perhaps performance?) reason we can't just use the > > number of channels present in DT? "make dtbs_check" can still validate > > this against the SoC-specific compatible value. > > > Nope performance issues. That is what the code does [0], It counts the > number of available channels in DTS and depending on the count it > populates the ADC channels. So for RZ/G2UL if we just add two channels in > the SoC DTSI this holds good and the driver shall populate only two > channels. And as you said the validation for the RZ/G2UL SoC for just two > channels will be done by make dtbs_check and in the driver the condition > still holds good 2 < 8. > > > Do we need to know at runtime both the number of channels physically > > present and the number of channels used? If yes, we either need to > > use the SoC-specific compatible value, or add a num-channels property. > > > At runtime we just need to know the number of channels used on the board. > > [0]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree > /drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n335 > DT describes hardware and here there is a hardware difference 2 channels(RZ/G2UL) vs 8 channel(RZ/G2L). Krzysztof Kozlowski, wants to take care this difference in dt-bindings by adding some validation checks. If we all are agreeing to drop dt-binding validation for channels, I am ok with that. But from driver point, still it need SoC-specific compatible value, or add a num-channels property as there is hardware difference RZG2UL_ADC_MAX_CHANNELS(2) vs RZG2L_ADC_MAX_CHANNELS(8) Currently driver validation only holds good for RZ/G2L SoC. See [1], [2], [3] and [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n324 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n340 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n263 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n293 Cheers, Biju
Hi Geert, > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com> > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Prabhakar, > > On Tue, May 3, 2022 at 8:32 AM Prabhakar Mahadev Lad <prabhakar.mahadev- > lad.rj@bp.renesas.com> wrote: > > > -----Original Message----- > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Sent: 02 May 2022 15:29 > > > To: Biju Das <biju.das.jz@bp.renesas.com> > > > Cc: Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>; > > > Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen > > > <lars@metafoo.de>; linux-iio@vger.kernel.org; > > > linux-renesas-soc@vger.kernel.org; Chris Paterson > > > <Chris.Paterson2@renesas.com>; Biju Das <biju.das@bp.renesas.com> > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > ADC > > > > > > On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for > > > > > RZ/G2UL ADC > > > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > > > ADC > > > > > > > > > > > > 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@bp.renesas.com> > > > > > > --- > > > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > I wonder if this changes is really required. RZ/G2UL can still > > > > > use the "renesas,rzg2l-adc". As the driver populates the > > > > > channels depending the number of elements in the array passed in > > > > > the DTS and not always 8 channels. For example on Renesas SMARC > > > > > EVK only four channels are populated. > > > > > > > > For me that restriction is coming from board design, as SoC is > > > > capable of handling 8 channels, But board design allows only 4. > > > > > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. > > > > Other > > > channels are invalid for RZ/G2UL SoC. > > > > > > > > That is the difference. > > > > > > > > > With this we don't have to differentiate RZ/G2UL SoC if just add > > > > > two channel entries in the SoC DTSI and the driver will just > > > > > create two channels. > > > > > > > > > @Geert - your thoughts on this. > > > > > > It depends on the meaning of the channel subnodes: do they indicate > > > (a) the number of channels present on the SoC, or (b) the number of > > > channels used on the board? The DT bindings are not clear about that. > > > > > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and > > > doesn't keep any disabled, which suggests (a). > > > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused > > > channels, which suggests (b). > > > > > Yep its (b), since the SoC can support 8 channels the RZ/G2L SoC DTSI > has 8 entries, If there comes a new EVK based on RZ/RZ/G2L SOC supporting > all the channels so this holds good. > > > > > Is there any (perhaps performance?) reason we can't just use the > > > number of channels present in DT? "make dtbs_check" can still > > > validate this against the SoC-specific compatible value. > > > > > Nope performance issues. That is what the code does [0], It counts the > number of available channels in DTS and depending on the count it > populates the ADC channels. So for RZ/G2UL if we just add two channels in > the SoC DTSI this holds good and the driver shall populate only two > channels. And as you said the validation for the RZ/G2UL SoC for just two > channels will be done by make dtbs_check and in the driver the condition > still holds good 2 < 8. > > > > > Do we need to know at runtime both the number of channels physically > > > present and the number of channels used? If yes, we either need to > > > use the SoC-specific compatible value, or add a num-channels property. > > > > > At runtime we just need to know the number of channels used on the > board. > > Then I think the driver can just match against the family-specific > compatible value, while "make dtbs_check" can use the SoC-specific > compatible value to validate the number of channels. Just wanted to confirm, currently driver validation only holds good for RZ/G2L SoC. So we want to keep as it is. As RZ/G2UL is a variant of RZ/G2L with lesser number channels. See [1], [2], [3] and [4] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n324 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n340 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n263 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n293 Regards, Biju
Hi Biju, On Tue, May 3, 2022 at 8:54 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > > On Mon, May 2, 2022 at 8:18 AM Biju Das <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > > ADC > > > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > > > ADC > > > > > > > > > > > > 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@bp.renesas.com> > > > > > > --- > > > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > I wonder if this changes is really required. RZ/G2UL can still use > > > > > the "renesas,rzg2l-adc". As the driver populates the channels > > > > > depending the number of elements in the array passed in the DTS > > > > > and not always 8 channels. For example on Renesas SMARC EVK only > > > > > four channels are populated. > > > > > > > > For me that restriction is coming from board design, as SoC is > > > > capable of handling 8 channels, But board design allows only 4. > > > > > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. Other > > > channels are invalid for RZ/G2UL SoC. > > > > > > > > That is the difference. > > > > > > > > > With this we don't have to differentiate RZ/G2UL SoC if just add > > > > > two channel entries in the SoC DTSI and the driver will just > > > > > create two channels. > > > > > > > > > @Geert - your thoughts on this. > > > > > > It depends on the meaning of the channel subnodes: do they indicate > > > (a) the number of channels present on the SoC, or (b) the number of > > > channels used on the board? The DT bindings are not clear about that. > > > > > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and > > > doesn't keep any disabled, which suggests (a). > > > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove unused > > > channels, which suggests (b). > > > > > Yep its (b), since the SoC can support 8 channels the RZ/G2L SoC DTSI has > > 8 entries, If there comes a new EVK based on RZ/RZ/G2L SOC supporting all > > the channels so this holds good. > > > > > Is there any (perhaps performance?) reason we can't just use the > > > number of channels present in DT? "make dtbs_check" can still validate > > > this against the SoC-specific compatible value. > > > > > Nope performance issues. That is what the code does [0], It counts the > > number of available channels in DTS and depending on the count it > > populates the ADC channels. So for RZ/G2UL if we just add two channels in > > the SoC DTSI this holds good and the driver shall populate only two > > channels. And as you said the validation for the RZ/G2UL SoC for just two > > channels will be done by make dtbs_check and in the driver the condition > > still holds good 2 < 8. > > > > > Do we need to know at runtime both the number of channels physically > > > present and the number of channels used? If yes, we either need to > > > use the SoC-specific compatible value, or add a num-channels property. > > > > > At runtime we just need to know the number of channels used on the board. > > > > [0]https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree > > /drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n335 > > > > DT describes hardware and here there is a hardware difference 2 channels(RZ/G2UL) vs 8 channel(RZ/G2L). Yes, that's why there are two compatible values: the SoC-specific one, and the family-specific one. > Krzysztof Kozlowski, wants to take care this difference in dt-bindings by adding some validation checks. The maximum number of channels is implied by the SoC-specific compatible value. Invalid channel numbers can be verified using "make dtbs_check". > If we all are agreeing to drop dt-binding validation for channels, I am ok with that. No, I want to keep dt-binding validation for channels. But I think the driver should not validate the number of channels, as this should have been caught at dtbs_check time (and won't work anyway). > But from driver point, still it need SoC-specific compatible value, or add a num-channels property as > there is hardware difference RZG2UL_ADC_MAX_CHANNELS(2) vs RZG2L_ADC_MAX_CHANNELS(8) From reading the hardware manual, it only matters for channels actually used. Whether the other channels are unused, or non-existent, doesn't seem to matter at all? TBH, I think they're really the same hardware block, they just didn't wire up channels 3-7 on RZ/G2UL (see e.g. bits 7 to 2 in the ADM2 register, which are not fixed to zero like the upper bits, but have an (undefined) initial value, which must be retained when written). > Currently driver validation only holds good for RZ/G2L SoC. > > See [1], [2], [3] and [4] > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n324 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n340 These can be dropped, as dtbs_check should take care of that. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n263 Looks like this condition can never happen, so the check can be removed? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n293 That check can stay (RZG2L_ADC_MAX_CHANNELS is the maximum number of channels supported by the IP block), or the limit can be replaced by the highest channel number found, or just BITS_PER_LONG (no invalid bits can be set anyway, and intst was masked by RZG2L_ADSTS_INTST_MASK = 0xff). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Biju, > > On Tue, May 3, 2022 at 8:54 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > > ADC On Mon, May 2, 2022 at 8:18 AM Biju Das > > > > <biju.das.jz@bp.renesas.com> > > > > wrote: > > > > > > Subject: RE: [PATCH] iio: adc: rzg2l_adc: Add support for > > > > > > RZ/G2UL ADC > > > > > > > Subject: [PATCH] iio: adc: rzg2l_adc: Add support for > > > > > > > RZ/G2UL ADC > > > > > > > > > > > > > > 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@bp.renesas.com> > > > > > > > --- > > > > > > > drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- > > > > > > > 1 file changed, 13 insertions(+), 5 deletions(-) > > > > > > > > > > > > > I wonder if this changes is really required. RZ/G2UL can still > > > > > > use the "renesas,rzg2l-adc". As the driver populates the > > > > > > channels depending the number of elements in the array passed > > > > > > in the DTS and not always 8 channels. For example on Renesas > > > > > > SMARC EVK only four channels are populated. > > > > > > > > > > For me that restriction is coming from board design, as SoC is > > > > > capable of handling 8 channels, But board design allows only 4. > > > > > > > > > > But on RZ/G2UL SoC, it is capable of handling only 2 channels. > > > > > Other > > > > channels are invalid for RZ/G2UL SoC. > > > > > > > > > > That is the difference. > > > > > > > > > > > With this we don't have to differentiate RZ/G2UL SoC if just > > > > > > add two channel entries in the SoC DTSI and the driver will > > > > > > just create two channels. > > > > > > > > > > > @Geert - your thoughts on this. > > > > > > > > It depends on the meaning of the channel subnodes: do they > > > > indicate > > > > (a) the number of channels present on the SoC, or (b) the number > > > > of channels used on the board? The DT bindings are not clear about > that. > > > > > > > > arch/arm64/boot/dts/renesas/r9a07g044.dtsi lists all channels and > > > > doesn't keep any disabled, which suggests (a). > > > > arch/arm64/boot/dts/renesas/rzg2l-smarc-som.dtsi does remove > > > > unused channels, which suggests (b). > > > > > > > Yep its (b), since the SoC can support 8 channels the RZ/G2L SoC > > > DTSI has > > > 8 entries, If there comes a new EVK based on RZ/RZ/G2L SOC > > > supporting all the channels so this holds good. > > > > > > > Is there any (perhaps performance?) reason we can't just use the > > > > number of channels present in DT? "make dtbs_check" can still > > > > validate this against the SoC-specific compatible value. > > > > > > > Nope performance issues. That is what the code does [0], It counts > > > the number of available channels in DTS and depending on the count > > > it populates the ADC channels. So for RZ/G2UL if we just add two > > > channels in the SoC DTSI this holds good and the driver shall > > > populate only two channels. And as you said the validation for the > > > RZ/G2UL SoC for just two channels will be done by make dtbs_check > > > and in the driver the condition still holds good 2 < 8. > > > > > > > Do we need to know at runtime both the number of channels > > > > physically present and the number of channels used? If yes, we > > > > either need to use the SoC-specific compatible value, or add a num- > channels property. > > > > > > > At runtime we just need to know the number of channels used on the > board. > > > > > > > > > > DT describes hardware and here there is a hardware difference 2 > channels(RZ/G2UL) vs 8 channel(RZ/G2L). > > Yes, that's why there are two compatible values: the SoC-specific one, and > the family-specific one. Agreed. > > > Krzysztof Kozlowski, wants to take care this difference in dt-bindings > by adding some validation checks. > > The maximum number of channels is implied by the SoC-specific compatible > value. > Invalid channel numbers can be verified using "make dtbs_check". > > > If we all are agreeing to drop dt-binding validation for channels, I am > ok with that. > > No, I want to keep dt-binding validation for channels. > But I think the driver should not validate the number of channels, as this > should have been caught at dtbs_check time (and won't work anyway). OK. > > > But from driver point, still it need SoC-specific compatible value, or > > add a num-channels property as there is hardware difference > > RZG2UL_ADC_MAX_CHANNELS(2) vs RZG2L_ADC_MAX_CHANNELS(8) > > From reading the hardware manual, it only matters for channels actually > used. Whether the other channels are unused, or non-existent, doesn't seem > to matter at all? > > TBH, I think they're really the same hardware block, they just didn't wire > up channels 3-7 on RZ/G2UL (see e.g. bits 7 to 2 in the ADM2 register, > which are not fixed to zero like the upper bits, but have an > (undefined) initial value, which must be retained when written). > OK. > > Currently driver validation only holds good for RZ/G2L SoC. > > > > See [1], [2], [3] and [4] > > >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n324 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n340 > These can be dropped, as dtbs_check should take care of that. OK, Will remove this. >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n263 > Looks like this condition can never happen, so the check can be removed? Yes, it can be removed. > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n293 > That check can stay (RZG2L_ADC_MAX_CHANNELS is the maximum number of > channels supported by the IP block), Agreed. Cheers, Biju
Hi Biju, On Tue, May 3, 2022 at 9:47 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > On Tue, May 3, 2022 at 8:54 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n324 > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n340 > > These can be dropped, as dtbs_check should take care of that. > > OK, Will remove this. Actually it's OK to keep them, as they are the upper limits supported by the hardware block. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, Thanks for the feedback. > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Biju, > > On Tue, May 3, 2022 at 9:47 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > >ADC On Tue, May 3, 2022 at 8:54 AM Biju Das > > > These can be dropped, as dtbs_check should take care of that. > > > > OK, Will remove this. > > Actually it's OK to keep them, as they are the upper limits supported by > the hardware block. You mean use upper limit of 2 for RZ/G2UL and 8 for RZ/G2L, right? For eg:- If we use, Channel0 and channel 2 :- this will be caught in dtbs as /home/biju/rzg2l-linux/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.example.dtb: adc@10059000: channel@2:reg:0:0: 2 is greater than the maximum of 1 But for run time, we don't want to this to happen for RZ/G2UL?? So if replace (channel >= RZG2L_ADC_MAX_CHANNELS) -> (channel >= adc->max_channel) will take of this. Cheers, Biju
Hi Biju, On Tue, May 3, 2022 at 10:05 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > On Tue, May 3, 2022 at 9:47 AM Biju Das <biju.das.jz@bp.renesas.com> > > wrote: > > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > >ADC On Tue, May 3, 2022 at 8:54 AM Biju Das > > > > These can be dropped, as dtbs_check should take care of that. > > > > > > OK, Will remove this. > > > > Actually it's OK to keep them, as they are the upper limits supported by > > the hardware block. > > You mean use upper limit of 2 for RZ/G2UL and 8 for RZ/G2L, right? No, I did mean RZG2L_ADC_MAX_CHANNELS, which is the upper limit of the hardware block. > For eg:- > If we use, Channel0 and channel 2 :- this will be caught in dtbs as > > /home/biju/rzg2l-linux/Documentation/devicetree/bindings/iio/adc/renesas,rzg2l-adc.example.dtb: adc@10059000: channel@2:reg:0:0: 2 is greater than the maximum of 1 > > But for run time, we don't want to this to happen for RZ/G2UL?? It will be caught at "make dtbs_check" time. And at test time, as it won't work anyway --- do not post un-tested patches ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC > > Hi Biju, > > On Tue, May 3, 2022 at 10:05 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL > > > ADC On Tue, May 3, 2022 at 9:47 AM Biju Das > > > <biju.das.jz@bp.renesas.com> > > > wrote: > > > > > Subject: Re: [PATCH] iio: adc: rzg2l_adc: Add support for > > > > >RZ/G2UL ADC On Tue, May 3, 2022 at 8:54 AM Biju Das These can > > > > >be dropped, as dtbs_check should take care of that. > > > > > > > > OK, Will remove this. > > > > > > Actually it's OK to keep them, as they are the upper limits > > > supported by the hardware block. > > > > You mean use upper limit of 2 for RZ/G2UL and 8 for RZ/G2L, right? > > No, I did mean RZG2L_ADC_MAX_CHANNELS, which is the upper limit of the > hardware block. OK, Thanks for clarification. > > > For eg:- > > If we use, Channel0 and channel 2 :- this will be caught in dtbs as > > > > /home/biju/rzg2l-linux/Documentation/devicetree/bindings/iio/adc/renes > > as,rzg2l-adc.example.dtb: adc@10059000: channel@2:reg:0:0: 2 is > > greater than the maximum of 1 > > > > But for run time, we don't want to this to happen for RZ/G2UL?? > > It will be caught at "make dtbs_check" time. > And at test time, as it won't work anyway --- do not post un-tested > patches ;-) OK, I will send V2 with this removed. https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/iio/adc/rzg2l_adc.c?h=v5.18-rc5#n263 Regards, Biju
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); 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 }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, rzg2l_adc_match);
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@bp.renesas.com> --- drivers/iio/adc/rzg2l_adc.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-)