On Tue, Aug 22, 2017 at 09:29:02AM +0200, Stefan Roese wrote:
> Hi Vinod,
>
> On 21.08.2017 18:18, Vinod Koul wrote:
> >>@@ -166,6 +166,10 @@ struct msgdma_response {
> >> #define MSGDMA_RESP_EARLY_TERM BIT(8)
> >> #define MSGDMA_RESP_ERR_MASK 0xff
> >>+#define respoffs(a) (offsetof(struct msgdma_response, a))
> >>+#define csroffs(a) (offsetof(struct msgdma_csr, a))
> >>+#define descoffs(a) (offsetof(struct msgdma_extended_desc, a))
> >>+
> >> /**
> >> * struct msgdma_sw_desc - implements a sw descriptor
> >> * @async_tx: support for the async_tx api
> >>@@ -204,13 +208,13 @@ struct msgdma_device {
> >> int irq;
> >> /* mSGDMA controller */
> >>- struct msgdma_csr *csr;
> >>+ void __iomem *csr;
> >> /* mSGDMA descriptors */
> >>- struct msgdma_extended_desc *desc;
> >>+ void __iomem *desc;
> >> /* mSGDMA response */
> >>- struct msgdma_response *resp;
> >>+ void __iomem *resp;
> >> };
> >> #define to_mdev(chan) container_of(chan, struct msgdma_device, dmachan)
> >>@@ -576,21 +580,21 @@ static void msgdma_reset(struct msgdma_device *mdev)
> >> int ret;
> >> /* Reset mSGDMA */
> >>- iowrite32(MSGDMA_CSR_STAT_MASK, &mdev->csr->status);
> >>- iowrite32(MSGDMA_CSR_CTL_RESET, &mdev->csr->control);
> >>+ iowrite32(MSGDMA_CSR_STAT_MASK, mdev->csr + csroffs(status));
> >>+ iowrite32(MSGDMA_CSR_CTL_RESET, mdev->csr + csroffs(control));
> >
> >why do you need to use offsetof here and other places?
> >something doesnt seem right here...
>
> ioread32/iowrite32() operates on "void __iomem *" and not on
> pointer to struct members. So adding these offsetof constructs helps
> to use the correct function parameter types here. The other solutions
> would be to either add some casts (which I really mis-like)
> or to move away from using a struct for the IP core registers
> description to macros instead, like this:
>
> #define MSGDMA_CSR_STATUS 0x00
> #define MSGDMA_CSR_CONTROL 0x04
> ...
>
> If you prefer the macros, I can change it this way as well - just
> let me know.
I think moving away from structures is better and ore intutive. We can have
a void__iomeme * as hw base address and you can use offsets as described..
> BTW: I've received the linux-next build failure warning, resulting
> from DMA_SG being removed. I plan to remove the DMA_SG support from
> the driver and send a new patch version with all changes included
> (sparse warnings fixed, W=1 warning removed and DMA_SG removed),
> if this is okay for you.
I have already fixed that up (i think you were cced on my reply) with the
patch to remove, do check if that was fine...
> When do you plan to pull the DMA_MEMCPY_SG patchset? For v4.14?
> If yes, would it be okay to already integrate support for this
> OP-type into the next patch version then? Or do you prefer that
> I add support for this in some follow-up patch instead?
I think we might be bit late, MERGE window opens in a week ish...
--
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
