RE: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
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(>lock, flags);
> > + ctemp = _linear_temp_converter(priv->coef, priv->ctemp);
> > + spin_unlock_irqrestore(>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 = _data},
> > + { .compatible = "renesas,thermal-r8a7796", .data = _data},
> > + { .compatible = "renesas,rcar-gen3-thermal", .data = _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 = >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 > 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(>lock);
> > + INIT_DELAYED_WORK(>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,
> > + _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
RE: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
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(>lock, flags);
> > + ctemp = _linear_temp_converter(priv->coef, priv->ctemp);
> > + spin_unlock_irqrestore(>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 = _data},
> > + { .compatible = "renesas,thermal-r8a7796", .data = _data},
> > + { .compatible = "renesas,rcar-gen3-thermal", .data = _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 = >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 > 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(>lock);
> > + INIT_DELAYED_WORK(>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,
> > + _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
Re: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
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(>lock, flags);
> + ctemp = _linear_temp_converter(priv->coef, priv->ctemp);
> + spin_unlock_irqrestore(>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 = _data},
> + { .compatible = "renesas,thermal-r8a7796", .data = _data},
> + { .compatible = "renesas,rcar-gen3-thermal", .data = _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 = >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 > 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(>lock);
> + INIT_DELAYED_WORK(>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,
> + _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();
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
Re: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
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(>lock, flags);
> + ctemp = _linear_temp_converter(priv->coef, priv->ctemp);
> + spin_unlock_irqrestore(>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 = _data},
> + { .compatible = "renesas,thermal-r8a7796", .data = _data},
> + { .compatible = "renesas,rcar-gen3-thermal", .data = _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 = >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 > 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(>lock);
> + INIT_DELAYED_WORK(>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,
> + _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();
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
