Re: [PATCH] iio: adc: rzg2l_adc: Add support for RZ/G2UL ADC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux