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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
