Messages in this thread | | | Date | Mon, 9 Jan 2023 13:07:06 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v6 7/8] media: i2c: add DS90UB913 driver |
| |
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?
...
> > + 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.
...
> > + ret = gpiochip_add_data(gc, priv); > > + if (ret) { > > + dev_err(dev, "Failed to add GPIOs: %d\n", ret);
> > + return ret; > > + } > > + > > + return 0;
return ret;
...
> > + ep_node = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
Why this can't be fwnode_handle from day 1?
> > + 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(...); ?
> > + }
...
> > + 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.
> > + {} > > +};
...
> > +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.
> > + .of_match_table = of_match_ptr(ub913_dt_ids), > > + }, > > +};
> > +
Redundant blank line.
> > +module_i2c_driver(ds90ub913_driver);
-- With Best Regards, Andy Shevchenko
| |