Messages in this thread |  | | Date | Mon, 9 Jan 2023 14:59:01 +0200 | | Subject | Re: [PATCH v6 7/8] media: i2c: add DS90UB913 driver | | From | Tomi Valkeinen <> |
| |
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
|  |