Messages in this thread | | | Subject | Re: [PATCH 6/6] soc: fsl: qe: fix sparse warnings for ucc_slow.c | From | Rasmus Villemoes <> | Date | Mon, 16 Mar 2020 22:07:25 +0100 |
| |
On 12/03/2020 23.28, Li Yang wrote: > Fixes the following sparse warnings: > [snip] > > Also removed the unneccessary clearing for kzalloc'ed structure.
Please don't mix that in the same patch, do it in a preparatory patch. That makes reviewing much easier.
> > /* Get PRAM base */ > uccs->us_pram_offset = > @@ -231,24 +224,24 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc > /* clear bd buffer */ > qe_iowrite32be(0, &bd->buf); > /* set bd status and length */ > - qe_iowrite32be(0, (u32 *)bd); > + qe_iowrite32be(0, (u32 __iomem *)bd);
It's cleaner to do two qe_iowrite16be to &bd->status and &bd->length, that avoids the casting altogether.
> bd++; > } > /* for last BD set Wrap bit */ > qe_iowrite32be(0, &bd->buf); > - qe_iowrite32be(cpu_to_be32(T_W), (u32 *)bd); > + qe_iowrite32be(T_W, (u32 __iomem *)bd);
Yeah, and this is why. Who can actually keep track of where that bit ends up being set with that casting going on. Please use qe_iowrite16be() with an appropriately modified constant to the appropriate field instead of these games.
And if the hardware doesn't support 16 bit writes, the definition of struct qe_bd is wrong and should have a single __be32 status_length field, with appropriate accessors defined.
> /* Init Rx bds */ > bd = uccs->rx_bd = qe_muram_addr(uccs->rx_base_offset); > for (i = 0; i < us_info->rx_bd_ring_len - 1; i++) { > /* set bd status and length */ > - qe_iowrite32be(0, (u32 *)bd); > + qe_iowrite32be(0, (u32 __iomem *)bd);
Same.
> /* clear bd buffer */ > qe_iowrite32be(0, &bd->buf); > bd++; > } > /* for last BD set Wrap bit */ > - qe_iowrite32be(cpu_to_be32(R_W), (u32 *)bd); > + qe_iowrite32be(R_W, (u32 __iomem *)bd);
Same.
> qe_iowrite32be(0, &bd->buf); > > /* Set GUMR (For more details see the hardware spec.). */ > @@ -273,8 +266,8 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc > qe_iowrite32be(gumr, &us_regs->gumr_h); > > /* gumr_l */ > - gumr = us_info->tdcr | us_info->rdcr | us_info->tenc | us_info->renc | > - us_info->diag | us_info->mode; > + gumr = (u32)us_info->tdcr | (u32)us_info->rdcr | (u32)us_info->tenc | > + (u32)us_info->renc | (u32)us_info->diag | (u32)us_info->mode;
Are the tdcr, rdcr, tenc, renc fields actually set anywhere (the same for the diag and mode, but word-grepping for those give way too many false positives)? They seem to be a somewhat pointless split out of the bitfields of gumr_l, and not populated anywhere?. That's not directly related to this patch, of course, but getting rid of them first (if they are indeed completely unused) might make the sparse cleanup a little simpler.
Rasmus
| |