On 2019/8/20 下午10:33, Greg Kroah-Hartman wrote:
On Tue, Aug 20, 2019 at 08:36:50PM +0800, zhangfei wrote:
Hi, Greg
On 2019/8/19 下午6:24, Greg Kroah-Hartman wrote:
+static int uacce_create_chrdev(struct uacce *uacce)
+{
+ int ret;
+
+ ret = idr_alloc(&uacce_idr, uacce, 0, 0, GFP_KERNEL);
+ if (ret < 0)
+ return ret;
+
Shouldn't this function create the memory needed for this structure?
You are relying ont he caller to do it for you, why?
I think you mean uacce structure here.
Yes, currently we count on caller to prepare uacce structure and call
uacce_register(uacce).
We still think this method is simpler, prepare uacce, register uacce.
And there are other system using the same method, like crypto
(crypto_register_acomp), nand, etc.
crypto is not a subsystem to ever try to emulate :)
You are creating a structure with a lifetime that you control, don't
have someone else create your memory, that's almost never what you want
to do. Most all driver subsystems create their own memory chunks for
what they need to do, it's a much better pattern.
Especially when you get into pointer lifetime issues...
OK, understand now, thanks for your patience.
will use this instead.
struct uacce_interface {
char name[32];
unsigned int flags;
struct uacce_ops *ops;
};
struct uacce *uacce_register(struct device *dev, struct uacce_interface
*interface);
What? Why do you need a structure? A pointer to the name and the ops
should be all that is needed, right?
We are thinking transfer structure will be more flexible.
And modify api later would be difficult, requiring many drivers modify
together.
Currently parameters need a flag, a pointer to the name, and ops, but in
case more requirement from future drivers usage.
Also refer usb_register_dev, sdhci_pltfm_init etc, and the structure
para can be set as static.
And 'dev' here is a pointer to the parent, right? Might want to make
that explicit in the name of the variable :)
Yes, 'dev' is parent, will change to 'pdev', thanks.
+
+static int uacce_dev_match(struct device *dev, void *data)
+{
+ if (dev->parent == data)
+ return -EBUSY;
There should be in-kernel functions for this now, no need for you to
roll your own.
Sorry, do not find this function.
Only find class_find_device, which still require match.
It is in linux-next, look there...
Suppose you mean the funcs: device_match_name,
device_match_of_node,device_match_devt etc.
Here we need dev->parent, there still no such func.
You should NEVER be matching on a parent. If so, your use of the driver
model is wrong :)
Remind me to really review the use of the driver core code in your next
submission of this series please, I think it needs it.
OK, thanks Greg.
