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;
