lkml.org 
[lkml]   [2016]   [Jun]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver support
    Date

    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;

    \
     
     \ /
      Last update: 2016-06-20 04:21    [W:2.398 / U:0.156 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site