| Message ID | 20230715010407.1751715-1-fabrizio.castro.jz@renesas.com |
|---|---|
| Headers | show |
| Series | spi: rzv2m-csi: Code refactoring | expand |
On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > Clock "csiclk" gets divided by 2 * CSI_CLKSEL_CKS in order to generate > the serial clock (output from master), with CSI_CLKSEL_CKS ranging from > 0x1 (that means "csiclk" is divided by 2) to 0x3FFF ("csiclk" is divided > by 32766). CSI_CKS_MAX is used for referring to the setting > corresponding to the maximum frequency divider. > Value 0x3FFF for CSI_CKS_MAX doesn't really means much to the reader > without an explanation and a more readable definition. > > Add a comment with a meaningful description and also replace value > 0x3FFF with the corresponding GENMASK, to make it very clear what the > macro means. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > The ternary operators used to initialize tx_completed and rx_completed > are not necessary, replace them with a better implementation. > > Signed-off-by: Fabrizio Castro <fabrizio.castro.jz@renesas.com> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro <fabrizio.castro.jz@renesas.com> wrote: > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > {read,write}s{b,w} > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: ... > > According to the hardware documentation[1], the access size for both > > the > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use writel() > > resp. readl(). So please check with the hardware people first. > > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. > > You are right, access is 32 bits (and although this patch works fine, > we should avoid accessing those regs any other way). Now I remember > why I decided to go for the bespoke loops in the first place, writesl > and readsl provide the right register access, but the wrong pointer > arithmetic for this use case. > For this patch I ended up selecting writesw/writesb/readsw/readsb to > get the right pointer arithmetic, but the register access is not as > per manual. > > I can either fallback to using the bespoke loops (I can still > remove the unnecessary u8* and u16* casting ;-) ), or I can add > new APIs for this sort of access to io.h (e.g. writesbl, writeswl, > readsbl, readswl, in order to get the pointer arithmetic right for > the type of array handled, while keeping memory access as expected). > > What are your thoughts on that? I think that you need to use readsl() / writesl() on the custom buffer with something like *_sparse() / *_condence() APIs added (perhaps locally to this driver) as they may be reused by others in the future. Having all flavours of read*()/write*() does not scale in my opinion.
Hi Andy, Thanks for your reply. > From: Andy Shevchenko <andy.shevchenko@gmail.com> > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > {read,write}s{b,w} > > On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro > <fabrizio.castro.jz@renesas.com> wrote: > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > > {read,write}s{b,w} > > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > > > <fabrizio.castro.jz@renesas.com> wrote: > > ... > > > > According to the hardware documentation[1], the access size for > both > > > the > > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use > writel() > > > resp. readl(). So please check with the hardware people first. > > > > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. > > > > You are right, access is 32 bits (and although this patch works > fine, > > we should avoid accessing those regs any other way). Now I remember > > why I decided to go for the bespoke loops in the first place, > writesl > > and readsl provide the right register access, but the wrong pointer > > arithmetic for this use case. > > For this patch I ended up selecting writesw/writesb/readsw/readsb to > > get the right pointer arithmetic, but the register access is not as > > per manual. > > > > I can either fallback to using the bespoke loops (I can still > > remove the unnecessary u8* and u16* casting ;-) ), or I can add > > new APIs for this sort of access to io.h (e.g. writesbl, writeswl, > > readsbl, readswl, in order to get the pointer arithmetic right for > > the type of array handled, while keeping memory access as expected). > > > > What are your thoughts on that? > > I think that you need to use readsl() / writesl() on the custom buffer > with something like > > *_sparse() / *_condence() APIs added (perhaps locally to this driver) > as they may be reused by others in the future. > Having all flavours of read*()/write*() does not scale in my opinion. Maybe a "generic" macro then (one for reading and one for writing)? So that we can pass the data type (to get the pointer arithmetic right) and the function name to use for the read/write operations (to get the register operations right)? Maybe that would scale and cater for most needs (including the one from this patch)? Cheers, Fab > > -- > With Best Regards, > Andy Shevchenko
> From: Fabrizio Castro <fabrizio.castro.jz@renesas.com> > Subject: RE: [PATCH 07/10] spi: rzv2m-csi: Switch to using > {read,write}s{b,w} > > Hi Andy, > > Thanks for your reply. > > > From: Andy Shevchenko <andy.shevchenko@gmail.com> > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > {read,write}s{b,w} > > > > On Mon, Jul 17, 2023 at 1:36 PM Fabrizio Castro > > <fabrizio.castro.jz@renesas.com> wrote: > > > > From: Geert Uytterhoeven <geert@linux-m68k.org> > > > > Subject: Re: [PATCH 07/10] spi: rzv2m-csi: Switch to using > > > > {read,write}s{b,w} > > > > On Sat, Jul 15, 2023 at 3:04 AM Fabrizio Castro > > > > <fabrizio.castro.jz@renesas.com> wrote: > > > > ... > > > > > > According to the hardware documentation[1], the access size for > > both > > > > the > > > > CSI_OFIFO and CSI_IFIFO registers is 32 bits, so you must use > > writel() > > > > resp. readl(). So please check with the hardware people first. > > > > > > > > [1] RZ/V2M User's Manual Hardware, Rev. 1.30. > > > > > > You are right, access is 32 bits (and although this patch works > > fine, > > > we should avoid accessing those regs any other way). Now I > remember > > > why I decided to go for the bespoke loops in the first place, > > writesl > > > and readsl provide the right register access, but the wrong > pointer > > > arithmetic for this use case. > > > For this patch I ended up selecting writesw/writesb/readsw/readsb > to > > > get the right pointer arithmetic, but the register access is not > as > > > per manual. > > > > > > I can either fallback to using the bespoke loops (I can still > > > remove the unnecessary u8* and u16* casting ;-) ), or I can add > > > new APIs for this sort of access to io.h (e.g. writesbl, writeswl, > > > readsbl, readswl, in order to get the pointer arithmetic right for > > > the type of array handled, while keeping memory access as > expected). > > > > > > What are your thoughts on that? > > > > I think that you need to use readsl() / writesl() on the custom > buffer > > with something like > > > > *_sparse() / *_condence() APIs added (perhaps locally to this > driver) > > as they may be reused by others in the future. > > Having all flavours of read*()/write*() does not scale in my > opinion. > > Maybe a "generic" macro then (one for reading and one for writing)? > So that we can pass the data type (to get the pointer arithmetic > right) and the function name to use for the read/write operations > (to get the register operations right)? > Maybe that would scale and cater for most needs (including the one > from this patch)? Something like the below perhaps: #ifndef readsx #define readsx(atype, readc, addr, buffer, count) \ ({ \ if (count) { \ unsigned int cnt = count; \ atype *buf = buffer; \ \ do { \ atype x = readc(addr); \ *buf++ = x; \ } while (--cnt); \ } \ }) #endif #ifndef writesx #define writesx(atype, writec, addr, buffer, count) \ ({ \ if (count) { \ unsigned int cnt = count; \ const atype *buf = buffer; \ \ do { \ writec(*buf++, addr); \ } while (--cnt); \ } \ }) #endif Cheers, Fab > > Cheers, > Fab > > > > > -- > > With Best Regards, > > Andy Shevchenko
On Sat, 15 Jul 2023 02:03:57 +0100, Fabrizio Castro wrote: > this series is to follow up on Geert and Andy feedback: > https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230622113341.657842-4-fabrizio.castro.jz@renesas.com/ > > Thanks, > Fab > > Fabrizio Castro (10): > spi: rzv2m-csi: Add missing include > spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock > spi: rzv2m-csi: Rework CSI_CKS_MAX definition > spi: rzv2m-csi: Leave readl_poll_timeout calls for last > spi: rzv2m-csi: Replace unnecessary ternary operators > spi: rzv2m-csi: Squash timing settings into one statement > spi: rzv2m-csi: Switch to using {read,write}s{b,w} > spi: rzv2m-csi: Improve data types and alignment > spi: rzv2m-csi: Get rid of the x_trg{_words} tables > spi: rzv2m-csi: Make use of device_set_node > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [01/10] spi: rzv2m-csi: Add missing include commit: f572ba797c639c9b1705908d3f5d71ed7c3f53e0 [02/10] spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock commit: 74e27ce8d23c3aeb1a9fdcaf6261462506bbbfc3 [03/10] spi: rzv2m-csi: Rework CSI_CKS_MAX definition commit: aecf9fbdb7a4dc6d83e8d9984c8d9dc074d8ea2e [04/10] spi: rzv2m-csi: Leave readl_poll_timeout calls for last commit: 2ed2699f58891c72fcd462129345d09424f986c5 [05/10] spi: rzv2m-csi: Replace unnecessary ternary operators commit: 9f5ac599801c0f7c0969fa94c638265ed988b9bc All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Dear All, this series is to follow up on Geert and Andy feedback: https://patchwork.kernel.org/project/linux-renesas-soc/patch/20230622113341.657842-4-fabrizio.castro.jz@renesas.com/ Thanks, Fab Fabrizio Castro (10): spi: rzv2m-csi: Add missing include spi: rzv2m-csi: Adopt HZ_PER_MHZ for max spi clock spi: rzv2m-csi: Rework CSI_CKS_MAX definition spi: rzv2m-csi: Leave readl_poll_timeout calls for last spi: rzv2m-csi: Replace unnecessary ternary operators spi: rzv2m-csi: Squash timing settings into one statement spi: rzv2m-csi: Switch to using {read,write}s{b,w} spi: rzv2m-csi: Improve data types and alignment spi: rzv2m-csi: Get rid of the x_trg{_words} tables spi: rzv2m-csi: Make use of device_set_node drivers/spi/spi-rzv2m-csi.c | 139 +++++++++++++++--------------------- 1 file changed, 58 insertions(+), 81 deletions(-)