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