RE: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support

From: Khiem Nguyen <khiem.nguyen.xt@renesas.com>
To: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Wolfram Sang <wsa@the-dreams.de>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Magnus Damm <damm+renesas@opensource.se>,
	"Zhang Rui" <rui.zhang@intel.com>,
	Eduardo Valentin <edubezval@gmail.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	Mark Rutland <mark.rutland@arm.com>,
	"linux-pm@vger.kernel.org" <linux-pm@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Thao Phuong Le. Nguyen" <thao.nguyen.yb@rvc.renesas.com>,
	Khiem Nguyen <khiem.nguyen.xt@renesas.com>,
	Toru Oishi <toru.oishi.zj@renesas.com>,
	"Hien Dang" <hien.dang.eb@renesas.com>
Subject: RE: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
Date: Sat, 3 Sep 2016 02:10:43 +0000	[thread overview]
Message-ID: <SIXPR06MB09062091B864265A73F5D6FDA8E40@SIXPR06MB0906.apcprd06.prod.outlook.com> (raw)
In-Reply-To: <8760t4skro.wl%kuninori.morimoto.gx@renesas.com>

Hi Morimoto-san,

Thanks for your comments.

> > +int _linear_temp_converter(struct equation_coefs coef,
> > +					int temp_code)
> > +{
> > +	int temp, temp1, temp2;
> > +
> > +	temp1 = MCELSIUS((CODETSD(temp_code) - coef.b1)) / coef.a1;
> > +	temp2 = MCELSIUS((CODETSD(temp_code) - coef.b2)) / coef.a2;
> > +	temp = (temp1 + temp2) / 2;
> > +
> > +	return _round_temp(temp);
> > +}
> 
> You want to have "static" function here ?

Sound good. Will update in v2.

> > +static int rcar_gen3_thermal_get_temp(void *devdata, int *temp) {
> > +	struct rcar_gen3_thermal_priv *priv = devdata;
> > +	int ctemp;
> > +	unsigned long flags;
> > +
> > +	rcar_gen3_thermal_update_temp(priv);
> > +
> > +	spin_lock_irqsave(&priv->lock, flags);
> > +	ctemp = _linear_temp_converter(priv->coef, priv->ctemp);
> > +	spin_unlock_irqrestore(&priv->lock, flags);
> 
> using pointer on _linear_temp_converter() is reasonable ?
> especially for struct equation_coefs coef

I failed to see the benefit of the change.
Could you elaborate the points ?
e.g better memory protection, faster byte-code execution, readability, etc

> > +static const struct rcar_gen3_thermal_data r8a7795_data = {
> > +	.thermal_init = r8a7795_thermal_init, };
> > +
> > +static const struct rcar_gen3_thermal_data r8a7796_data = {
> > +	.thermal_init = r8a7796_thermal_init, };
> > +
> > +static const struct of_device_id rcar_gen3_thermal_dt_ids[] = {
> > +	{ .compatible = "renesas,thermal-r8a7795", .data = &r8a7795_data},
> > +	{ .compatible = "renesas,thermal-r8a7796", .data = &r8a7796_data},
> > +	{ .compatible = "renesas,rcar-gen3-thermal", .data = &r8a7796_data},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
> 
> We can't have general case in this case ?
> "renesas,rcar-gen3-thermal" is not needed IMO.
> Especially this driver doesn't need to care about back compatibility yet.

OK. I see your point. Will update in V2.

> > +static int rcar_gen3_thermal_probe(struct platform_device *pdev) {
> > +	struct rcar_gen3_thermal_priv *priv;
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res, *irq;
> > +	int ret = -ENODEV;
> > +	int idle;
> > +	struct device_node *tz_nd, *tmp_nd;
> > +
> > +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, priv);
> > +
> > +	priv->dev = dev;
> > +
> > +	pm_runtime_enable(dev);
> > +	pm_runtime_get_sync(dev);
> > +
> > +	priv->data = of_device_get_match_data(dev);
> > +	if (!priv->data)
> > +		goto error_unregister;
> > +
> > +	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > +	priv->irq = 0;
> > +	if (irq) {
> > +		priv->irq = 1;
> > +		for_each_node_with_property(tz_nd, "polling-delay") {
> > +			tmp_nd = of_parse_phandle(tz_nd,
> > +					"thermal-sensors", 0);
> > +			if (tmp_nd && !strcmp(tmp_nd->full_name,
> > +					dev->of_node->full_name)) {
> > +				of_property_read_u32(tz_nd, "polling-delay",
> > +					&idle);
> > +				(idle > 0) ? (priv->irq = 0) :
> > +						(priv->irq = 1);
> > +				break;
> > +			}
> 
> it is not readable for me.
> 
> 	if (idle > 0)
> 		priv->irq = 0;
> 	break;
> 
> is enough ?

Unfortunately, it's not.
The code tries to check "polling-delay" in order to select polling mode and get polling duration from DT.
So, your proposal just do 1st part.

> 
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res)
> > +		goto error_unregister;
> > +
> > +	priv->base = devm_ioremap_resource(dev, res);
> > +	if (IS_ERR(priv->base)) {
> > +		ret = PTR_ERR(priv->base);
> > +		goto error_unregister;
> > +	}
> > +
> > +	spin_lock_init(&priv->lock);
> > +	INIT_DELAYED_WORK(&priv->work, rcar_gen3_thermal_work);
> > +
> > +	priv->id = of_alias_get_id(dev->of_node, "tsc");
> 
> Do we really need alias ?
> is "tsc" good naming ?

It's the abbreviation of Thermal sensor controller.
The term has been described in HW manual. Therefore, I think it's 'reasonable' name.

> Having this explanation on [1/4] patch document is useful.
> of_alias_get_id() can return -ENODEV, but no error check ?

Good point. Will fix in v2.

> > +	priv->zone = devm_thermal_zone_of_sensor_register(dev, 0, priv,
> > +				&rcar_gen3_tz_of_ops);
> > +
> > +	if (IS_ERR(priv->zone)) {
> > +		dev_err(dev, "Can't register thermal zone\n");
> > +		ret = PTR_ERR(priv->zone);
> > +		priv->zone = NULL;
> > +		goto error_unregister;
> > +	}
> 
> It is not bad operation, but not readable.
> How about to have local struct thermal_zone_device *zone, like this ?

It's good point.
I saw that other thermal drivers also did that way.

The original source code follows same code flow as rcar-thermal driver.
Perhaps, we can change the code flow for both drivers, as your idea.

> 	zone = devm_thermal_zone_of_sensor_register(xxxx);
> 	if (IS_ERR(zone)) {
> 		...
> 		ret = PTR_ERR(zone);
> 		goto error_unregister;
> 	}
> 	priv->zone = zone;
> 
> > +	priv->data->thermal_init(priv);
> 
> thermal_init() has return value;

OK. Will fix in v2.

> 
> > +	ret = _read_fuse_factor(priv);
> > +	if (ret)
> > +		goto error_unregister;
> > +	_linear_coefficient_calculation(priv);
> > +	ret = rcar_gen3_thermal_update_temp(priv);
> > +
> > +	if (ret < 0)
> > +		goto error_unregister;
> 
> This is very picky comment about empty line, but this is readable for me
> 
> 	ret = _read_fuse_factor(priv);
> 	if (ret)
> 		goto error_unregister;
> 
> 	_linear_coefficient_calculation(priv);
> 
> 	ret = rcar_gen3_thermal_update_temp(priv);
> 	if (ret < 0)
> 		goto error_unregister;

OK. Newline does not harm anything.
Will add in v2.

Thanks.

Best regards,
KHIEM Nguyen