The mask for the channel selection is incorrect as it's specified to be
16b wide by is actually only 4.
Also, the 16 lower bits in the SARADC_CONV_CON register are write
protected. Whatever their value is can only be written to the hardware
block if their associated bit in the higher 16 bits is set. Considering
that the channel bitmask is 4b wide but that we can write e.g. 0 in
there, we shouldn't use the value shifted by 16 as a mask but rather the
bitmask for that value shifted by 16. This is currently NOT an issue
because the only SoC with SARADCv2 IP is the RK3588 which has a reset
defined in the SoC DTSI. When that is the case, the reset is asserted
before every channel conversion is started. This means the registers are
reset so effectively, we do not need to write zeros so the wrong mask
still works because where we should be writing zeroes, there are already
zeroes. However, let's fix this in case there comes a day there's an SoC
which doesn't require to reset the controller before every channel
conversion is started.
Lastly, let's use the appropriate function from the reset subsystem
for getting an optional exclusive reset instead of rolling out our own
logic.
Those three patches should not be changing any behavior.
Signed-off-by: Quentin Schulz <[email protected]>
---
Quentin Schulz (3):
iio: adc: rockchip_saradc: fix bitmask for channels on SARADCv2
iio: adc: rockchip_saradc: use mask for write_enable bitfield
iio: adc: rockchip_saradc: replace custom logic with devm_reset_control_get_optional_exclusive
drivers/iio/adc/rockchip_saradc.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
---
base-commit: 39133352cbed6626956d38ed72012f49b0421e7b
change-id: 20240222-saradcv2-chan-mask-593585865256
Best regards,
--
Quentin Schulz <[email protected]>
On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <[email protected]> wrote:
>
> The mask for the channel selection is incorrect as it's specified to be
> 16b wide by is actually only 4.
>
> Also, the 16 lower bits in the SARADC_CONV_CON register are write
> protected. Whatever their value is can only be written to the hardware
> block if their associated bit in the higher 16 bits is set. Considering
> that the channel bitmask is 4b wide but that we can write e.g. 0 in
> there, we shouldn't use the value shifted by 16 as a mask but rather the
> bitmask for that value shifted by 16. This is currently NOT an issue
> because the only SoC with SARADCv2 IP is the RK3588 which has a reset
> defined in the SoC DTSI. When that is the case, the reset is asserted
> before every channel conversion is started. This means the registers are
> reset so effectively, we do not need to write zeros so the wrong mask
> still works because where we should be writing zeroes, there are already
> zeroes. However, let's fix this in case there comes a day there's an SoC
> which doesn't require to reset the controller before every channel
> conversion is started.
>
> Lastly, let's use the appropriate function from the reset subsystem
> for getting an optional exclusive reset instead of rolling out our own
> logic.
>
> Those three patches should not be changing any behavior.
Nice series, I have the comments in patch 3, but no need to resend
until Jonathan asks for. He might address that whilst applying.
Reviewed-by: Andy Shevchenko <[email protected]>
--
With Best Regards,
Andy Shevchenko
On Fri, 23 Feb 2024 15:01:30 +0200
Andy Shevchenko <[email protected]> wrote:
> On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leilnet> wrote:
> >
> > The mask for the channel selection is incorrect as it's specified to be
> > 16b wide by is actually only 4.
> >
> > Also, the 16 lower bits in the SARADC_CONV_CON register are write
> > protected. Whatever their value is can only be written to the hardware
> > block if their associated bit in the higher 16 bits is set. Considering
> > that the channel bitmask is 4b wide but that we can write e.g. 0 in
> > there, we shouldn't use the value shifted by 16 as a mask but rather the
> > bitmask for that value shifted by 16. This is currently NOT an issue
> > because the only SoC with SARADCv2 IP is the RK3588 which has a reset
> > defined in the SoC DTSI. When that is the case, the reset is asserted
> > before every channel conversion is started. This means the registers are
> > reset so effectively, we do not need to write zeros so the wrong mask
> > still works because where we should be writing zeroes, there are already
> > zeroes. However, let's fix this in case there comes a day there's an SoC
> > which doesn't require to reset the controller before every channel
> > conversion is started.
> >
> > Lastly, let's use the appropriate function from the reset subsystem
> > for getting an optional exclusive reset instead of rolling out our own
> > logic.
> >
> > Those three patches should not be changing any behavior.
>
> Nice series, I have the comments in patch 3, but no need to resend
> until Jonathan asks for. He might address that whilst applying.
>
> Reviewed-by: Andy Shevchenko <[email protected]>
>
I've take the series through the togreg branch as we are so near the
merge windows. Note (despite Linus not liking it :)) I use links
in my git tags so anyone really searching for Quentin can find
the discussion.
I'm usually a bit lazy on cleaning out what I would consider unnecessary
CC tags but given Andy comment on these I've dropped the extra
one.
Patches 1 and 2 marked for stable.
Jonathan