Hi Laurent, On 28/05/2020 5.10, Laurent Pinchart wrote: >>> As mentioned in the commit message, I plan to extend that, I just didn't >>> want to add the checks to all the prepare operation wrappers until an >>> agreement on the approach would be reached. I also thought it would be >>> good to not allow this API for other transaction types until use cases >>> arise, in order to force upstream discussions instead of silently >>> abusing the API :-) >> >> I would not object if slave_sg and memcpy got the same treatment. If the >> DMA driver did not set the DMA_REPEAT then clients can not use this >> feature anyways. > > Would you not object, or would you prefer if it was done in v5 ? :-) DMA_REPEAT is a generic flag, not limited to only interleaved, but you are going to be the first user of it with interleaved. > Overall I think that enabling APIs that have no user isn't necessarily > the best idea, as it's prone to design issues, but I don't mind doing so > if you think it needs to be done now. We would get the support in one go with the same commit. I don't think it makes much sense to add slave_sg later, then memcpy another time. True, there might be no users for them for some time, but their presents might invite users? >>> I can extend the flag to all other transaction types >>> (except for the cyclic transaction, as it doesn't make sense there). >> >> Yep, cyclic is a different type of transfer, it is for circular buffers. >> It could be seen as a special case of slave_sg. Some drivers actually >> create temporary sg_list in case of cyclic and use the same setup >> function to set up the transfer for slave_sg/cyclic... > > Cyclic is different for historical reasons, but if I had to redesign it > today, I'd make it slave_sg + DMA_PREP_REPEAT. We obviously can't, and I > have no issue with that. Which should be accompanied with a flag to tell that the sg_list is covering a circular buffer to save all drivers to check the sg_list that it is circular buffer (current cyclic) or really sg. Some DMA can only do repeat on circular buffers (omap-dma, tegra, etc). >> But, DMA drivers might support neither of them, either of them or both. >> It is up to the client to pick the preferred method for it's use. >> It is not far fetched that the next DMA the client is going to be >> serviced will have different capabilities and the client needs to handle >> EOT or NOW or it might even need to have fallback to case when neither >> is supported. >> >> I don't like excessive flags either, but based on my experience >> under-flagging can bite back sooner than later. >> >> I'm aware that at the moment it feels like it is too explicit, but never >> underestimate the creativity of the design - and in some cases the >> constraint the design must fulfill. > > I'm still very puzzled by why you think adding DMA_PREP_LOAD_EOT now is > a good idea, given that there's no existing and no foreseen use case for > not setting it. Creating an API element that is completely disconnected > from any known use case doesn't seem like good API design to me, > especially for an in-kernel API. If we document that DMA_REPEAT covers REPEAT _and_ LOAD_EOT with one flag then how would other drivers can implement REPEAT if they can not support LOAD_EOT? They should do DMA_REPEAT | NOT_LOAD_EOT | LOAD_ASAP? LOAD_EOT is a feature the HW can or can not support and it is an operation mode that you want to use or do not want to use. - Péter Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Attachment:
pEpkey.asc
Description: application/pgp-keys
