On 09/01/2023 13:07, Andy Shevchenko wrote:
On Sun, Jan 08, 2023 at 06:06:34AM +0200, Laurent Pinchart wrote:
On Thu, Jan 05, 2023 at 04:03:06PM +0200, Tomi Valkeinen wrote:
...
+ scnprintf(priv->gpio_chip_name, sizeof(priv->gpio_chip_name), "%s",
+ dev_name(dev));
I think you can use strscpy().
Actually I'm not sure we even need that variable. What is the lifetime of
the dev and gc? I believe they are the same or gc's one is shorter, hence
dev_name() can be used directly, no?
I think this is a valid point, no need for the extra variable afaics.
...
+ gc->of_node = priv->client->dev.of_node;
We don't have of_node anymore in gc. And if the parent device is set, you can
drop this line (it will work with older and newer kernels. Otherwise, use
fwnode.
What do you mean "we don't have of_node anymore"?
...
+ ret = gpiochip_add_data(gc, priv);
+ if (ret) {
+ dev_err(dev, "Failed to add GPIOs: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
return ret;
I'm not a fan of that style. I like my error handling ifs to return the
error inside the if block, and a successful function ends in a "return 0".
...
+ ep_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
Why this can't be fwnode_handle from day 1?
I guess it can. It's an old driver and there has been no need to convert
to fwnode, so we're still using OF.
+ if (!ep_node) {
+ dev_err(dev, "No graph endpoint\n");
+ return -ENODEV;
+ }
...
+ ep_np = of_graph_get_endpoint_by_regs(np, 0, 0);
+ if (!ep_np) {
+ dev_err(dev, "OF: no endpoint\n");
+ return -ENOENT;
+ }
Ditto.
+ ret = of_property_read_u32(ep_np, "pclk-sample", &priv->pclk_polarity);
+
+ of_node_put(ep_np);
Ditto.
...
+ return ret;
+ }
+
+ return 0;
return ret;
...
+ priv->plat_data = dev_get_platdata(&client->dev);
+ if (!priv->plat_data) {
+ dev_err(dev, "Platform data missing\n");
+ return -ENODEV;
return dev_err_probe(...); ?
Isn't the idea with dev_err_probe to use it where -EPROBE_DEFER might be
the error? That's not the case here.
Buuut reading the relevant docs a bit more shows that it's actually
recommended to be used in this kind of cases too, so you're right.
+ }
...
+ priv->regmap = devm_regmap_init_i2c(client, &ub913_regmap_config);
+ if (IS_ERR(priv->regmap)) {
+ dev_err(dev, "Failed to init regmap\n");
+ return PTR_ERR(priv->regmap);
Ditto?
+ }
...
+#ifdef CONFIG_OF
The driver depends on CONFIG_OF so I would drop this, as well as the
of_match_ptr().
Even if there is no OF dependency, these ugly ifdeffery with of_match_ptr()
are error prone (compilation wise).
...
+static const struct of_device_id ub913_dt_ids[] = {
+ { .compatible = "ti,ds90ub913a-q1", },
Inner comma is not needed.
Ok.
+ {}
+};
...
+static struct i2c_driver ds90ub913_driver = {
+ .probe_new = ub913_probe,
+ .remove = ub913_remove,
+ .id_table = ub913_id,
+ .driver = {
+ .name = "ds90ub913a",
+ .owner = THIS_MODULE,
This is something like for 5+ years is not needed, as the below macro sets it
for you.
Ok.
+ .of_match_table = of_match_ptr(ub913_dt_ids),
+ },
+};
+
Redundant blank line.
+module_i2c_driver(ds90ub913_driver);
Tomi
