linux-kernel - Re: [PATCH v6 7/8] media: i2c: add DS90UB913 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Y7v1Wrma/Ev8KEzy@smile.fi.intel.com>
Date:   Mon, 9 Jan 2023 13:07:06 +0200
From:   Andy Shevchenko <andriy.shevchenko@...el.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Wolfram Sang <wsa@...nel.org>,
        Luca Ceresoli <luca.ceresoli@...tlin.com>,
        Matti Vaittinen <Matti.Vaittinen@...rohmeurope.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Peter Rosin <peda@...ntia.se>,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>,
        Michael Tretter <m.tretter@...gutronix.de>,
        Shawn Tu <shawnx.tu@...el.com>,
        Hans Verkuil <hverkuil@...all.nl>,
        Mike Pagano <mpagano@...too.org>,
        Krzysztof Hałasa <khalasa@...p.pl>,
        Marek Vasut <marex@...x.de>
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