Re: [PATCH 3/7] thermal/drivers/qcom/tsens: Add support for IPQ5018 tsens

[Sricharan Ramabadhran <quic_srichara@quicinc.com>]

<..>

>> +{
>> +       u32 p1[10], p2[10];
>> +       u32 *qfprom_cdata;
>> +       int mode;
>> +
>> +       qfprom_cdata = (u32 *)qfprom_read(priv->dev, "calib");
>> +       if (IS_ERR(qfprom_cdata))
>> +               return PTR_ERR(qfprom_cdata);
>> +
>> +       mode = tsens_read_calibration_legacy(priv, &tsens_ipq5018_nvmem,
>> +                                            p1, p2,
>> +                                            qfprom_cdata, NULL);
> 
> No, this should be used only in _legacy_ cases. Please use
> tsens_calibrate_common() / tsens_calibrate_nvmem() / etc.
> 

  ok.

>> +
>> +       fixup_ipq5018_points(mode, p1, p2);
>> +       compute_intercept_slope(priv, p1, p2, mode);
>> +       kfree(qfprom_cdata);
>> +
>> +       return 0;
>> +}
>> +
>>   static int calibrate_v1(struct tsens_priv *priv)
>>   {
>>          u32 p1[10], p2[10];
>> @@ -79,6 +132,18 @@ static struct tsens_features tsens_v1_feat = {
>>          .trip_max_temp  = 120000,
>>   };
>>
>> +static struct tsens_features tsens_v1_ipq5018_feat = {
>> +       .ver_major      = VER_1_X,
>> +       .crit_int       = 0,
>> +       .combo_int      = 0,
>> +       .adc            = 1,
>> +       .srot_split     = 1,
>> +       .max_sensors    = 11,
>> +       .trip_min_temp  = -40000,
>> +       .trip_max_temp  = 120000,
>> +       .no_early_init  = 1,
>> +};
>> +
>>   static const struct reg_field tsens_v1_regfields[MAX_REGFIELDS] = {
>>          /* ----- SROT ------ */
>>          /* VERSION */
>> @@ -150,6 +215,43 @@ static int __init init_8956(struct tsens_priv *priv) {
>>          return init_common(priv);
>>   }
>>
>> +static int init_ipq5018(struct tsens_priv *priv)
>> +{
>> +       int ret;
>> +       u32 mask;
>> +
>> +       init_common(priv);
>> +       if (!priv->tm_map)
>> +               return -ENODEV;
>> +
>> +       ret = regmap_field_write(priv->rf[TSENS_SW_RST], 1);
>> +       if (ret) {
>> +               dev_err(priv->dev, "Reset failed\n");
>> +               return ret;
>> +       }
>> +
>> +       mask = GENMASK(10, 0);
> 
> #define this, then inline the variable. Or extract this codepiece into
> generic function which uses num_sensors to calculate the mask
> 

  ok.

>> +       ret = regmap_field_update_bits(priv->rf[SENSOR_EN], mask, mask);
>> +       if (ret) {
>> +               dev_err(priv->dev, "Sensor Enable failed\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = regmap_field_write(priv->rf[TSENS_EN], 1);
>> +       if (ret) {
>> +               dev_err(priv->dev, "Enable failed\n");
>> +               return ret;
>> +       }
>> +
>> +       ret = regmap_field_write(priv->rf[TSENS_SW_RST], 0);
>> +       if (ret) {
>> +               dev_err(priv->dev, "Reset failed\n");
> 
> This error message is useless. You can not determine if it comes from
> this error or from setting the reset bit.
> 

  ok, will drop it.

Regards,
  Sricharan