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

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
To: Khiem Nguyen <khiem.nguyen.xt@rvc.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>,
	"Hien Duy. Dang" <hien.dang.eb@rvc.renesas.com>,
	Toru Oishi <toru.oishi.zj@rvc.renesas.com>
Subject: Re: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
Date: Mon, 20 Jun 2016 01:44:22 +0000	[thread overview]
Message-ID: <8760t4skro.wl%kuninori.morimoto.gx@renesas.com> (raw)
In-Reply-To: <576612DE.3070204@rvc.renesas.com>

Hi Khiem-san

Thank you for your patch

> +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 ?

> +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

> +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.

> +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 ?

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

> +	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 ?

	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;

> +	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;