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@xxxxxxxxxxxxxx> > 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@xxxxxxxxxxxxxx> > > > > 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@xxxxxxxxxxxxxx> > > > > > > > --- > > > > > > > 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
