Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5173a16a-83c5-5cfe-f6ce-03e1c90e8790@ideasonboard.com>
Date:   Mon, 9 Jan 2023 14:59:01 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.4.2
Subject: Re: [PATCH v6 7/8] media: i2c: add DS90UB913 driver
Content-Language: en-US
To:     Andy Shevchenko <andriy.shevchenko@intel.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc:     linux-media@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Wolfram Sang <wsa@kernel.org>,
        Luca Ceresoli <luca.ceresoli@bootlin.com>,
        Matti Vaittinen <Matti.Vaittinen@fi.rohmeurope.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Peter Rosin <peda@axentia.se>,
        Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Sakari Ailus <sakari.ailus@linux.intel.com>,
        Michael Tretter <m.tretter@pengutronix.de>,
        Shawn Tu <shawnx.tu@intel.com>,
        Hans Verkuil <hverkuil@xs4all.nl>,
        Mike Pagano <mpagano@gentoo.org>,
        =?UTF-8?Q?Krzysztof_Ha=c5=82asa?= <khalasa@piap.pl>,
        Marek Vasut <marex@denx.de>
References: <20230105140307.272052-1-tomi.valkeinen@ideasonboard.com>
 <20230105140307.272052-8-tomi.valkeinen@ideasonboard.com>
 <Y7pBSq49dL8Fzxsc@pendragon.ideasonboard.com>
 <Y7v1Wrma/Ev8KEzy@smile.fi.intel.com>
From:   Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
In-Reply-To: <Y7v1Wrma/Ev8KEzy@smile.fi.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

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