Le 16/04/2020 à 07:22, Wang Wenhu a écrit :
Yes, kzalloc() would clean the allocated areas and the init of remaining array elements are redundant. I will remove the block in v3.+ dev_err(&pdev->dev, "error no valid uio-map configured\n"); + ret = -EINVAL; + goto err_info_free_internel; + } + + info->version = "0.1.0";Could you define some DRIVER_VERSION in the top of the file next to DRIVER_NAME instead of hard coding in the middle on a function ?That's what v1 had, and Greg KH said to remove it. I'm guessing that he thought it was the common-but-pointless practice of having the driver print a version number that never gets updated, rather than something the UIO API (unfortunately, compared to a feature query interface) expects. That said, I'm not sure what the value is of making it a macro since it should only be used once, that use is self documenting, it isn't tunable, etc. Though if this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro again, it should be UIO_VERSION, not DRIVER_VERSION). Does this really need a three-part version scheme? What's wrong with a version of "1", to be changed to "2" in the hopefully-unlikely event that the userspace API changes? Assuming UIO is used for this at all, which doesn't seem like a great fit to me. -ScottAs Scott mentioned, the version define as necessity by uio core but actually useless for us here(and for many other type of devices I guess). So maybe the better way is to set it optionally, but this belong first to uio core. For the cache-sram uio driver, I will define an UIO_VERSION micro as a compromise fit all wonders, no confusing as Greg first mentioned.
Yes I like it.
+static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = { + { .compatible = "uio,fsl,p2020-l2-cache-controller", }, + { .compatible = "uio,fsl,p2010-l2-cache-controller", }, + { .compatible = "uio,fsl,p1020-l2-cache-controller", }, + { .compatible = "uio,fsl,p1011-l2-cache-controller", }, + { .compatible = "uio,fsl,p1013-l2-cache-controller", }, + { .compatible = "uio,fsl,p1022-l2-cache-controller", }, + { .compatible = "uio,fsl,mpc8548-l2-cache-controller", }, + { .compatible = "uio,fsl,mpc8544-l2-cache-controller", }, + { .compatible = "uio,fsl,mpc8572-l2-cache-controller", }, + { .compatible = "uio,fsl,mpc8536-l2-cache-controller", }, + { .compatible = "uio,fsl,p1021-l2-cache-controller", }, + { .compatible = "uio,fsl,p1012-l2-cache-controller", }, + { .compatible = "uio,fsl,p1025-l2-cache-controller", }, + { .compatible = "uio,fsl,p1016-l2-cache-controller", }, + { .compatible = "uio,fsl,p1024-l2-cache-controller", }, + { .compatible = "uio,fsl,p1015-l2-cache-controller", }, + { .compatible = "uio,fsl,p1010-l2-cache-controller", }, + { .compatible = "uio,fsl,bsc9131-l2-cache-controller", }, + {}, +};NACK The device tree describes the hardware, not what driver you want to bind the hardware to, or how you want to allocate the resources. And even if defining nodes for sram allocation were the right way to go, why do you have a separate compatible for each chip when you're just describing software configuration? Instead, have module parameters that take the sizes and alignments you'd like to allocate and expose to userspace. Better still would be some sort of dynamic allocation (e.g. open a fd, ioctl to set the requested size/alignment, if it succeeds you can mmap it, and when the fd is closed the region is freed). -ScottCan not agree more. But what if I want to define more than one cache-sram uio devices? How about use the device tree for pseudo uio cache-sram driver? static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = { { .compatible = "uio,cache-sram", }, {}, };
You can still give it a name in line with your driver, ie: "uio,mpc85xx-cache-sram"
After, it you have different behaviours depending on the compatible, then you have to add a .data field which will tell the driver which behaviour to implement.
Christophe
