Hi Morimoto-san,
Thanks for your comments.
quoted
+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.
quoted
+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
quoted
+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.
quoted
+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.
quoted
+ 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.
quoted
+ 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;
quoted
+ priv->data->thermal_init(priv);
thermal_init() has return value;
OK. Will fix in v2.
quoted
+ 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