| Message ID | 20240223-saradcv2-chan-mask-v1-0-84b06a0f623a@theobroma-systems.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | iio: adc: rockchip_saradc: fix bitmasking and remove custom logic for getting reset | expand |
On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leil.net> wrote: > > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > devm_reset_control_get_optional_exclusive does what this driver is devm_reset_control_get_optional_exclusive() > trying to do in its probe function, therefore let's switch over to that do it in > subsystem function. > Cc: Quentin Schulz <foss+kernel@0leil.net> You may use the --cc option to `git send-email` instead of polluting commit messages, or move this after the '---' cutter line.
On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leil.net> 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 <andy.shevchenko@gmail.com>
Hi Andy, Thanks for the prompt feedback on the whole series. On 2/23/24 14:00, Andy Shevchenko wrote: > On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leil.net> wrote: >> >> From: Quentin Schulz <quentin.schulz@theobroma-systems.com> >> >> devm_reset_control_get_optional_exclusive does what this driver is > > devm_reset_control_get_optional_exclusive() > >> trying to do in its probe function, therefore let's switch over to that > > do it in > >> subsystem function. > >> Cc: Quentin Schulz <foss+kernel@0leil.net> > > You may use the --cc option to `git send-email` instead of polluting > commit messages, or move this after the '---' cutter line. > The whole point is that my SoB and authorship is from my professional mail address which is likely to change over time, the Cc is my personal one for development. Basically, in the event that I change my employer, I would still be reachable at that Cc address without having to modify the .mailmap after the fact (which won't make it to an earlier version of the kernel for example). Some maintainers don't really like this, some don't mind, we'll see in which category the IIO maintainer(s) fall in :) (I don't mind either way just to be clear). Cheers, Quentin
Am Freitag, 23. Februar 2024, 13:45:23 CET schrieb Quentin Schulz: > From: Quentin Schulz <quentin.schulz@theobroma-systems.com> > > devm_reset_control_get_optional_exclusive does what this driver is > trying to do in its probe function, therefore let's switch over to that > subsystem function. > > Cc: Quentin Schulz <foss+kernel@0leil.net> > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
On Fri, Feb 23, 2024 at 3:10 PM Quentin Schulz <quentin.schulz@theobroma-systems.com> wrote: > On 2/23/24 14:00, Andy Shevchenko wrote: > > On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leil.net> wrote: ... > >> Cc: Quentin Schulz <foss+kernel@0leil.net> > > > > You may use the --cc option to `git send-email` instead of polluting > > commit messages, or move this after the '---' cutter line. > > The whole point is that my SoB and authorship is from my professional > mail address which is likely to change over time, the Cc is my personal > one for development. Basically, in the event that I change my employer, > I would still be reachable at that Cc address without having to modify > the .mailmap after the fact (which won't make it to an earlier version > of the kernel for example). Some maintainers don't really like this, > some don't mind, we'll see in which category the IIO maintainer(s) fall > in :) (I don't mind either way just to be clear). My point is that Cc and other similar (non-real-tags) stuff is polluting commit messages. It means that this will be copied to the Git index to all kernel git repositories in the world from now and then, This is at bare minimum makes additional burden on git log (and parsing and so on) and moreover, wastes resources becoming less environment friendly (no jokes). Using --cc or moving to the behind the commit message will keep email copied with cleaner commit messages. Yet, all email tags are available in lore archive (lore.kernel.org). Please, really reconsider the commit messages content in the Linux kernel project and elsewhere, it will help to make the world more friendly.
On Fri, 23 Feb 2024 15:01:30 +0200 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leil.net> 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 <andy.shevchenko@gmail.com> > 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
Hello Andy and Quentin, On 2024-02-23 15:39, Andy Shevchenko wrote: > On Fri, Feb 23, 2024 at 3:10 PM Quentin Schulz > <quentin.schulz@theobroma-systems.com> wrote: >> On 2/23/24 14:00, Andy Shevchenko wrote: >> > On Fri, Feb 23, 2024 at 2:46 PM Quentin Schulz <foss+kernel@0leil.net> wrote: > > ... > >> >> Cc: Quentin Schulz <foss+kernel@0leil.net> >> > >> > You may use the --cc option to `git send-email` instead of polluting >> > commit messages, or move this after the '---' cutter line. >> >> The whole point is that my SoB and authorship is from my professional >> mail address which is likely to change over time, the Cc is my >> personal >> one for development. Basically, in the event that I change my >> employer, >> I would still be reachable at that Cc address without having to modify >> the .mailmap after the fact (which won't make it to an earlier version >> of the kernel for example). Some maintainers don't really like this, >> some don't mind, we'll see in which category the IIO maintainer(s) >> fall >> in :) (I don't mind either way just to be clear). > > My point is that Cc and other similar (non-real-tags) stuff is > polluting commit messages. It means that this will be copied to the > Git index to all kernel git repositories in the world from now and > then, This is at bare minimum makes additional burden on git log (and > parsing and so on) and moreover, wastes resources becoming less > environment friendly (no jokes). Using --cc or moving to the behind > the commit message will keep email copied with cleaner commit > messages. Yet, all email tags are available in lore archive > (lore.kernel.org). Please, really reconsider the commit messages > content in the Linux kernel project and elsewhere, it will help to > make the world more friendly. Believe it or not, I'm working on some patches for Git that, I believe, should help a lot when it comes to handling Cc: addresses. Would you like to be included in the list of recipients for those Git patches, so you could, hopefully, provide some feeback?
On Mon, Feb 26, 2024 at 10:31 PM Dragan Simic <dsimic@manjaro.org> wrote: > On 2024-02-23 15:39, Andy Shevchenko wrote: > > On Fri, Feb 23, 2024 at 3:10 PM Quentin Schulz > > <quentin.schulz@theobroma-systems.com> wrote: > >> On 2/23/24 14:00, Andy Shevchenko wrote: ... > >> I would still be reachable at that Cc address without having to modify > >> the .mailmap after the fact (which won't make it to an earlier version > >> of the kernel for example). Some maintainers don't really like this, > >> some don't mind, we'll see in which category the IIO maintainer(s) > >> fall > >> in :) (I don't mind either way just to be clear). > > > > My point is that Cc and other similar (non-real-tags) stuff is > > polluting commit messages. It means that this will be copied to the > > Git index to all kernel git repositories in the world from now and > > then, This is at bare minimum makes additional burden on git log (and > > parsing and so on) and moreover, wastes resources becoming less > > environment friendly (no jokes). Using --cc or moving to the behind > > the commit message will keep email copied with cleaner commit > > messages. Yet, all email tags are available in lore archive > > (lore.kernel.org). Please, really reconsider the commit messages > > content in the Linux kernel project and elsewhere, it will help to > > make the world more friendly. > > Believe it or not, I'm working on some patches for Git that, I believe, > should help a lot when it comes to handling Cc: addresses. Would you > like to be included in the list of recipients for those Git patches, so > you could, hopefully, provide some feeback? You may Cc me if you want to, but I can't guarantee I have time or valuable input to that.
On 2024-02-27 13:48, Andy Shevchenko wrote: > On Mon, Feb 26, 2024 at 10:31 PM Dragan Simic <dsimic@manjaro.org> > wrote: >> On 2024-02-23 15:39, Andy Shevchenko wrote: >> > On Fri, Feb 23, 2024 at 3:10 PM Quentin Schulz >> > <quentin.schulz@theobroma-systems.com> wrote: >> >> On 2/23/24 14:00, Andy Shevchenko wrote: > > ... > >> >> I would still be reachable at that Cc address without having to modify >> >> the .mailmap after the fact (which won't make it to an earlier version >> >> of the kernel for example). Some maintainers don't really like this, >> >> some don't mind, we'll see in which category the IIO maintainer(s) >> >> fall >> >> in :) (I don't mind either way just to be clear). >> > >> > My point is that Cc and other similar (non-real-tags) stuff is >> > polluting commit messages. It means that this will be copied to the >> > Git index to all kernel git repositories in the world from now and >> > then, This is at bare minimum makes additional burden on git log (and >> > parsing and so on) and moreover, wastes resources becoming less >> > environment friendly (no jokes). Using --cc or moving to the behind >> > the commit message will keep email copied with cleaner commit >> > messages. Yet, all email tags are available in lore archive >> > (lore.kernel.org). Please, really reconsider the commit messages >> > content in the Linux kernel project and elsewhere, it will help to >> > make the world more friendly. >> >> Believe it or not, I'm working on some patches for Git that, I >> believe, >> should help a lot when it comes to handling Cc: addresses. Would you >> like to be included in the list of recipients for those Git patches, >> so >> you could, hopefully, provide some feeback? > > You may Cc me if you want to, but I can't guarantee I have time or > valuable input to that. Thanks, I'll be happy to have another set of eyes on those Git patches.
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 <quentin.schulz@theobroma-systems.com> --- 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,