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.
>
>-Scott
>
As 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.
>> +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).
>
>-Scott
>
Can 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", },
{},
};
Thanks,
Wenhu
