On Mon, Sep 16, 2024 at 4:16 PM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > Hi, > > On Sun, Sep 15, 2024 at 4:32 AM Chen-Yu Tsai <wenst@xxxxxxxxxxxx> wrote: > > > > > > + * Assumes that across the entire device tree the only instances of nodes > > > > + * prefixed with "type" are the ones that need handling for second source > > > > + * components. In other words, if "type" is "touchscreen", then all device > > > > + * nodes named "touchscreen*" are the ones that need probing. There cannot > > > > > > "touchscreen*" implies that it can have an arbitrary suffix. Can it? > > > > That is the idea. The use case is for components that have conflicting > > addresses and need special probing. Such device nodes obviously can't > > have the same node name. This is planned but not implemented in this > > series. > > Maybe "touchscreen@*" instead of "touchscreen*" if I'm understanding correctly. Then it would be "touchscreen*@*". > > > > + * be another "touchscreen" node that is already enabled. > > > > + * > > > > + * Assumes that for each "type" of component, only one actually exists. In > > > > + * other words, only one matching and existing device will be enabled. > > > > + * > > > > + * Context: Process context only. Does non-atomic I2C transfers. > > > > + * Should only be used from a driver probe function, as the function > > > > + * can return -EPROBE_DEFER if the I2C adapter or other resources > > > > + * are unavailable. > > > > + * Return: 0 on success or no-op, error code otherwise. > > > > + * A no-op can happen when it seems like the device tree already > > > > + * has components of the type to be probed already enabled. This > > > > + * can happen when the device tree had not been updated to mark > > > > + * the status of the to-be-probed components as "fail". Or this > > > > + * function was already run with the same parameters and succeeded > > > > + * in enabling a component. The latter could happen if the user > > > > > > s/latter/later > > > > Are you sure? > > No. latter looked weird to me and I searched quickly and thought I was > right. With a more full search looks like you're right. > > > > > > +int i2c_of_probe_component(struct device *dev, const struct i2c_of_probe_cfg *cfg, void *ctx) > > > > +{ > > > > + const struct i2c_of_probe_ops *ops; > > > > + const char *type; > > > > + struct device_node *i2c_node; > > > > + struct i2c_adapter *i2c; > > > > + int ret; > > > > + > > > > + if (!cfg) > > > > + return -EINVAL; > > > > > > Drop extra check of "!cfg". In general kernel conventions don't check > > > for NULL pointers passed by caller unless it's an expected case. You > > > don't check for a NULL "dev" and you shouldn't need to check for a > > > NULL "cfg". They are both simply required parameters. > > > > "dev" is only passed to dev_printk(), and that can handle "dev" being > > NULL. Same can't be said for "cfg". > > > > I don't know what the preference is though. Crashing is probably not the > > nicest thing, even if it only happens to developers. > > Honestly as a developer I'd prefer the crash. It points out the exact > line where I had an invalid NULL. Returning an error code means I've > got to compile/boot several more times to track down where the error > code is coming from. > > I'm fairly certain that the kernel convention is to only check things > for NULL if it's part of the API to accept NULL or if the value can be > NULL due to untrusted data. If the only way it can be NULL is due to > buggy code elsewhere in the kernel then you should omit the error > checks. Make sense. > > > > + if (!of_device_is_available(node)) > > > > + continue; > > > > + > > > > + /* > > > > + * Device tree has component already enabled. Either the > > > > + * device tree isn't supported or we already probed once. > > > > + */ > > > > + ret = 0; > > > > + goto out_put_i2c_node; > > > > + } > > > > + > > > > + i2c = of_get_i2c_adapter_by_node(i2c_node); > > > > + if (!i2c) { > > > > + ret = dev_err_probe(dev, -EPROBE_DEFER, "Couldn't get I2C adapter\n"); > > > > + goto out_put_i2c_node; > > > > + } > > > > + > > > > + /* Grab resources */ > > > > + ret = 0; > > > > + if (ops->get_resources) > > > > + ret = ops->get_resources(dev, i2c_node, ctx); > > > > + if (ret) > > > > + goto out_put_i2c_adapter; > > > > + > > > > + /* Enable resources */ > > > > + if (ops->enable) > > > > + ret = ops->enable(dev, ctx); > > > > + if (ret) > > > > + goto out_release_resources; > > > > > > I won't insist, but a part of me wonders whether we should just > > > combine "get_resources" and "enable" and then combine "cleanup" and > > > "free_resources_late". They are always paired one after another and > > > I'm having a hard time seeing why they need to be separate. It's not > > > like you'll ever get the resources and then enable/disable multiple > > > times. > > > > Maybe. The structure was carried over from the original non-callback > > version. I think it's easier to reason about if they are kept separate, > > especially since the outgoing path is slightly different when no working > > component is found and one of the callbacks ends up not getting called. > > Actually, both of the outgoing callbacks are always called. It's only > the 3rd callback (the "early" one) that's called sometimes. So IIRC do "get + enable" and "cleanup + release_late", and leave "release_early" alone? ChenYu > I won't insist on combining them, but I still feel like combining them > would be better. I'd be interested in other opinions, though. > > > -Doug >