On 01/26, Narendran Rajan wrote:
> TSENS supports reading temperature from multiple thermal
> sensors present in QCOM SOCs.
> TSENS HW is enabled only when the main sensor is requested.
> The TSENS block is disabled if the main senors is disabled
> irrespective of any other sensors that are being enabled.
> TSENS driver supports configurable threshold for temperature
> monitoring in which case it can generate an interrupt when specific
> thresholds are reached
>
> Based on code by Siddartha Mohanadoss and Stephen Boyd.
So as far as I can tell you removed my loop TODOs, added debug
prints and the device singleton, put the calibration data offset
into DT (another TODO), used regmap on the qfprom, made this into
a standalone device instead of a child of GCC, and moved to OF
thermal zones (forcing that THERMAL_TSENS8960_HWTRIPS ifdef I
suppose?) Anything else?
>
> Cc: Siddartha Mohanadoss <smohanad@xxxxxxxxxxxxxx>
> Cc: Stephen Boyd <sboyd@xxxxxxxxxxxxxx>
> Signed-off-by: Narendran Rajan <nrajan@xxxxxxxxxxxxxx>
Something is wrong with your git. There should only be one space
between your name and the email id.
> ---
> diff --git a/drivers/thermal/msm8960_tsens.c b/drivers/thermal/msm8960_tsens.c
> new file mode 100644
> index 0000000..307bdc8
> --- /dev/null
> +++ b/drivers/thermal/msm8960_tsens.c
> +
> +static u32 tsens_hi_code(int trip, u32 reg_cntl, u32 thresh)
> +{
> + u32 hi_code;
> +
> + switch (trip) {
> + case TSENS_TRIP_STAGE0:
> + if (!(reg_cntl & TSENS_LOWER_STATUS_CLR)) {
> + hi_code = thresh >> TSENS_THRESHOLD_LOWER_LIMIT_SHIFT;
> + break;
> + }
> + /* else fall through */
> + case TSENS_TRIP_STAGE1:
> + if (!(reg_cntl & TSENS_UPPER_STATUS_CLR)) {
> + hi_code = thresh >> TSENS_THRESHOLD_UPPER_LIMIT_SHIFT;
> + break;
> + }
> + /* else fall through */
> + case TSENS_TRIP_STAGE2:
> + if (!(reg_cntl & TSENS_MAX_STATUS_MASK)) {
> + hi_code = thresh >> TSENS_THRESHOLD_MAX_LIMIT_SHIFT;
> + break;
> + }
> + /* else fall through */
> + case TSENS_TRIP_STAGE3:
> + default:
> + hi_code = TSENS_THRESHOLD_MAX_CODE;
> + break;
> + }
> +
> + return hi_code & TSENS_THRESHOLD_MAX_CODE;
> +}
> +
> +static u32 tsens_lo_code(int trip, u32 reg_cntl, u32 thresh)
> +{
> + u32 lo_code;
> +
> + switch (trip) {
> + case TSENS_TRIP_STAGE3:
> + if (!(reg_cntl & TSENS_UPPER_STATUS_CLR)) {
> + lo_code = thresh >> TSENS_THRESHOLD_UPPER_LIMIT_SHIFT;
> + break;
> + }
> + /* else fall through */
> + case TSENS_TRIP_STAGE2:
> + if (!(reg_cntl & TSENS_LOWER_STATUS_CLR)) {
> + lo_code = thresh >> TSENS_THRESHOLD_LOWER_LIMIT_SHIFT;
> + break;
> + }
> + /* else fall through */
> + case TSENS_TRIP_STAGE1:
> + if (!(reg_cntl & TSENS_MIN_STATUS_MASK)) {
> + lo_code = thresh >> TSENS_THRESHOLD_MIN_LIMIT_SHIFT;
> + break;
> + }
> + /* else fall through */
> + case TSENS_TRIP_STAGE0:
> + default:
> + lo_code = TSENS_THRESHOLD_MIN_CODE;
> + break;
> + }
> +
> + return lo_code & TSENS_THRESHOLD_MAX_CODE;
> +}
I wonder if these can be loops, or something on top of
find_next_bit()? That's what my TODO was about, hoping to reduce
these functions even further.
> +
> +static int tsens_tz_set_trip_temp(struct tsens_sensor *tm_sensor,
> + int trip, unsigned long temp)
> +{
> + struct tsens_device *tmdev = tm_sensor->tmdev;
> + struct regmap_field *status = tmdev->status_field;
> + u32 thresh, reg_cntl, mask = TSENS_THRESHOLD_MAX_CODE;
> + u32 code, hi_code, lo_code, code_err_chk;
> +
> + code_err_chk = code = tsens_tz_mdegC_to_code(temp, tm_sensor);
> +
> + regmap_field_read(status, ®_cntl);
> + regmap_read(tmdev->map, TSENS_THRESHOLD_ADDR, &thresh);
> +
> + switch (trip) {
> + case TSENS_TRIP_STAGE3:
> + code <<= TSENS_THRESHOLD_MAX_LIMIT_SHIFT;
> + mask <<= TSENS_THRESHOLD_MAX_LIMIT_SHIFT;
> + break;
> + case TSENS_TRIP_STAGE2:
> + code <<= TSENS_THRESHOLD_UPPER_LIMIT_SHIFT;
> + mask <<= TSENS_THRESHOLD_UPPER_LIMIT_SHIFT;
> + break;
> + case TSENS_TRIP_STAGE1:
> + code <<= TSENS_THRESHOLD_LOWER_LIMIT_SHIFT;
> + mask <<= TSENS_THRESHOLD_LOWER_LIMIT_SHIFT;
> + break;
> + case TSENS_TRIP_STAGE0:
> + code <<= TSENS_THRESHOLD_MIN_LIMIT_SHIFT;
> + mask <<= TSENS_THRESHOLD_MIN_LIMIT_SHIFT;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + hi_code = tsens_hi_code(trip, reg_cntl, thresh);
> + lo_code = tsens_lo_code(trip, reg_cntl, thresh);
> +
> + if (code_err_chk < lo_code || code_err_chk > hi_code)
> + return -EINVAL;
> +
> + regmap_update_bits(tmdev->map, TSENS_THRESHOLD_ADDR, mask, code);
> +
> + return 0;
> +}
> +
> +static int tsens_set_trips(void *_sensor, long low, long high)
Is this function even used?
> +{
> + struct tsens_sensor *tm_sensor = _sensor;
> +
> + tsens_print_trip_temp(tm_sensor);
> +
> + if (tsens_tz_set_trip_temp(tm_sensor, TSENS_TRIP_STAGE2, high))
> + return -EINVAL;
> +
> + if (tsens_tz_set_trip_temp(tm_sensor, TSENS_TRIP_STAGE1, low))
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif
> +
> +static void tsens_scheduler_fn(struct work_struct *work)
> +{
> + struct tsens_device *tmdev;
> + struct regmap_field *status;
> + unsigned int threshold, threshold_low, i, code, bits, mask = 0;
> + unsigned long sensor;
> + bool upper_th_x, lower_th_x;
> +
> + tmdev = container_of(work, struct tsens_device, tsens_work);
> + status = tmdev->status_field;
> +
> + regmap_field_update_bits(status,
> + TSENS_LOWER_STATUS_CLR | TSENS_UPPER_STATUS_CLR,
> + TSENS_LOWER_STATUS_CLR | TSENS_UPPER_STATUS_CLR);
> +
> + regmap_read(tmdev->map, TSENS_THRESHOLD_ADDR, &threshold);
> + threshold_low = threshold >> TSENS_THRESHOLD_LOWER_LIMIT_SHIFT;
> + threshold_low &= TSENS_THRESHOLD_MAX_CODE;
> + threshold = threshold >> TSENS_THRESHOLD_UPPER_LIMIT_SHIFT;
> + threshold &= TSENS_THRESHOLD_MAX_CODE;
> +
> + regmap_read(tmdev->map, TSENS_CNTL_ADDR, &bits);
> + sensor = bits;
> + sensor >>= TSENS_SENSOR0_SHIFT;
> + for_each_set_bit(i, &sensor, tmdev->num_sensors) {
> + regmap_read(tmdev->map, tmdev->sensor[i].status, &code);
> + upper_th_x = code >= threshold;
> + lower_th_x = code <= threshold_low;
> +
> + if (upper_th_x)
> + mask |= TSENS_UPPER_STATUS_CLR;
> +
> + if (lower_th_x)
> + mask |= TSENS_LOWER_STATUS_CLR;
> +
> +#ifdef THERMAL_TSENS8960_HWTRIPS
> + if (upper_th_x || lower_th_x) {
> + dev_info(tsens_dev,
> + "Threshold reached for sensor(%d)\n", i);
This looks like debug noise. Please remove.
> + thermal_zone_device_update(tmdev->sensor[i].tz_dev);
> + }
> +#endif
> + }
> +
> + regmap_field_update_bits(status,
> + TSENS_UPPER_STATUS_CLR | TSENS_LOWER_STATUS_CLR, mask);
> +}
> +
> +static irqreturn_t tsens_isr(int irq, void *data)
> +{
> + schedule_work(data);
I was going to move this to threaded irqs, please do that.
> + return IRQ_HANDLED;
> +}
> +
> +static void tsens_disable_mode(const struct tsens_device *tmdev)
> +{
> + u32 reg_cntl;
> + u32 mask;
> +
> + mask = GENMASK(tmdev->num_sensors - 1, 0);
> + mask <<= TSENS_SENSOR0_SHIFT;
> + mask |= TSENS_EN;
> +
> + regmap_read(tmdev->map, TSENS_CNTL_ADDR, ®_cntl);
> + reg_cntl &= ~mask;
> + if (tmdev->num_sensors > 1)
> + reg_cntl &= ~TSENS_8960_SLP_CLK_ENA;
> + else
> + reg_cntl &= ~TSENS_8660_SLP_CLK_ENA;
> + regmap_write(tmdev->map, TSENS_CNTL_ADDR, reg_cntl);
> +}
> +
> +static void tsens_hw_init(struct tsens_device *tmdev)
> +{
> + u32 reg_cntl, reg_thr;
> +
> + reg_cntl = TSENS_SW_RST;
> + regmap_update_bits(tmdev->map, TSENS_CNTL_ADDR, TSENS_SW_RST, reg_cntl);
> +
> + if (tmdev->num_sensors > 1) {
> + reg_cntl |= TSENS_8960_SLP_CLK_ENA |
> + (TSENS_MEASURE_PERIOD << 18);
Unnecessary ()?
> + reg_cntl &= ~TSENS_SW_RST;
> + regmap_update_bits(tmdev->map, TSENS_8960_CONFIG_ADDR,
> + TSENS_8960_CONFIG_MASK, TSENS_8960_CONFIG);
> + } else {
> + reg_cntl |= TSENS_8660_SLP_CLK_ENA |
> + (TSENS_MEASURE_PERIOD << 16);
Unnecessary ()?
> + reg_cntl &= ~TSENS_8660_CONFIG_MASK;
> + reg_cntl |= TSENS_8660_CONFIG << TSENS_8660_CONFIG_SHIFT;
> + }
> +
> + reg_cntl |= GENMASK(tmdev->num_sensors - 1, 0) << TSENS_SENSOR0_SHIFT;
> + regmap_write(tmdev->map, TSENS_CNTL_ADDR, reg_cntl);
> +
> + regmap_field_update_bits(tmdev->status_field,
> + TSENS_LOWER_STATUS_CLR | TSENS_UPPER_STATUS_CLR |
> + TSENS_MIN_STATUS_MASK | TSENS_MAX_STATUS_MASK,
> + TSENS_LOWER_STATUS_CLR | TSENS_UPPER_STATUS_CLR |
> + TSENS_MIN_STATUS_MASK | TSENS_MAX_STATUS_MASK);
> +
> + reg_cntl |= TSENS_EN;
> + regmap_write(tmdev->map, TSENS_CNTL_ADDR, reg_cntl);
> +
> + reg_thr = (TSENS_LOWER_LIMIT_TH << TSENS_THRESHOLD_LOWER_LIMIT_SHIFT) |
> + (TSENS_UPPER_LIMIT_TH << TSENS_THRESHOLD_UPPER_LIMIT_SHIFT) |
> + (TSENS_MIN_LIMIT_TH << TSENS_THRESHOLD_MIN_LIMIT_SHIFT) |
> + (TSENS_MAX_LIMIT_TH << TSENS_THRESHOLD_MAX_LIMIT_SHIFT);
> + regmap_write(tmdev->map, TSENS_THRESHOLD_ADDR, reg_thr);
> +}
> +
> +static int
> +tsens_calib_sensors8660(struct tsens_device *tmdev, struct regmap *map)
> +{
> + u32 temp_data, data;
> + struct tsens_sensor *s = &tmdev->sensor[0];
> +
> + if (regmap_read(map, tmdev->calib_offset, &temp_data))
> + return -EINVAL;
> +
> + data = (temp_data >> 24) & 0xff;
> +
> + if (!data) {
> + dev_err(tsens_dev, "QFPROM TSENS calibration data not present\n");
Ah we do use it for an error here. It isn't hard to put a struct
device inside tmdev...
> + return -EINVAL;
> + }
> +
> + s->offset = TSENS_CAL_MDEGC - s->slope * data;
> +
> + return 0;
> +}
> +
> +static int
> +tsens_calib_sensors8960(struct tsens_device *tmdev, struct regmap *map)
> +{
> + int i;
> + u32 temp_data[TSENS_MAX_SENSORS];
> + u8 *byte_data;
> + u32 fuse, redun, num_read;
> + struct tsens_sensor *s = tmdev->sensor;
> +
> + fuse = tmdev->calib_offset;
> + redun = tmdev->backup_calib_offset;
> +
> + /**
> + * syscon regmap is 32-bit data, but calibration data is 8-bit.
> + * read 4 bytes from regmap in a loop and then extract bytes seprately
> + */
Weird comment style. Please do it like
/*
* This because this isn't kernel-doc.
*/
> +
> + num_read = DIV_ROUND_UP(tmdev->num_sensors, 4);
> +
> + for (i = 0; i < num_read; i++) {
> + if (regmap_read(map, (redun + i*4), &temp_data[i]))
> + return -EINVAL;
> +
> + if (!temp_data[i]) {
> + dev_dbg(tsens_dev, "Main calib data not valid\n");
> + if (regmap_read(map, (fuse + i*4), &temp_data[i]))
> + return -EINVAL;
> + }
> + }
> +
> + byte_data = (u8 *)temp_data;
> +
> + for (i = 0; i < tmdev->num_sensors; i++, s++) {
> + s->offset = TSENS_CAL_MDEGC - s->slope * byte_data[i];
> + dev_dbg(tsens_dev, "calib data %d is %c", i, byte_data[i]);
> + }
> +
> + return 0;
> +}
This is a lot of hoops to jump through just to use a syscon
regmap (does this even work in big-endian mode?). Given your
point about consumers wanting to read with different strides
makes me think that using a regmap is not a good idea. It would
be better if we had an API that could be used to read an
arbitrary number of bytes from an eeprom.
> +
> +static const struct thermal_zone_of_device_ops tsens_thermal_of_ops = {
> + .get_temp = tsens_tz_get_temp,
> +};
> +
> +
Nitpick: Two newlines instead of one.
> +static int tsens_register(struct tsens_device *tmdev, int i)
> +{
> + char name[18];
> + u32 addr = TSENS_S0_STATUS_ADDR;
> + struct tsens_sensor *s = &tmdev->sensor[i];
> +
> + /*
> + * The status registers for each sensor are discontiguous
> + * because some SoCs have 5 sensors while others have more
> + * but the control registers stay in the same place, i.e.
> + * directly after the first 5 status registers.
> + */
> + if (i >= 5)
> + addr += 40;
> +
> + addr += i * 4;
> +
> + snprintf(name, sizeof(name), "tsens_tz_sensor%d", i);
> + s->mode = THERMAL_DEVICE_ENABLED;
> + s->sensor_num = i;
> + s->status = addr;
> + s->tmdev = tmdev;
> + s->tz_dev = thermal_zone_of_sensor_register(tsens_dev, i, s,
> + &tsens_thermal_of_ops);
> +
> + if (IS_ERR(s->tz_dev))
> + return -ENODEV;
So if it's an error pointer we leave it there? And then if an
interrupt arrives and this sensor has a new temperature to report
we'll update the thermal framework with an invalid pointer? When
I wrote this code I didn't allow probe to continue if any of
these zones failed to register. Why change that because we're
using the sensor API?
> +
> + return 0;
> +}
> +
> +static int tsens_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct device_node *base_node;
> + struct platform_device *base_pdev;
> + int ret, i, irq, num;
> + struct tsens_sensor *s;
> + struct tsens_device *tmdev;
> + struct regmap *map, *imem_regmap;
> + struct reg_field *field;
> + static struct reg_field status_0 = {
> + .reg = TSENS_8064_STATUS_CNTL,
> + .lsb = 0,
> + .msb = 3,
> + };
> + static struct reg_field status_8 = {
> + .reg = TSENS_CNTL_ADDR,
> + .lsb = 8,
> + .msb = 11,
> + };
> +
> + tsens_dev = &pdev->dev;
> +
> + num = of_property_count_u32_elems(np, "qcom,tsens-slopes");
> + if (num <= 0) {
> + dev_err(tsens_dev, "invalid tsens slopes\n");
> + return -EINVAL;
> + }
> +
> + tmdev = devm_kzalloc(tsens_dev, sizeof(*tmdev) +
> + num * sizeof(struct tsens_sensor), GFP_KERNEL);
> + if (tmdev == NULL)
> + return -ENOMEM;
> +
> + tmdev->num_sensors = num;
> + for (i = 0, s = tmdev->sensor; i < num; i++, s++)
> + of_property_read_u32_index(np, "qcom,tsens-slopes", i,
> + &s->slope);
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(tsens_dev, "no irq resource?\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_index(np, "qcom,calib-offsets", 0,
> + &tmdev->calib_offset);
> + if (ret != 0) {
> + dev_err(tsens_dev, "No calibration offset set\n");
> + return -EINVAL;
> + }
> +
> + ret = of_property_read_u32_index(np, "qcom,calib-offsets", 1,
> + &tmdev->backup_calib_offset);
> + if (ret) {
> + dev_info(tsens_dev, "Missing backup calibration offset\n");
Why make any noise at all? A backup is here.
> + tmdev->backup_calib_offset = tmdev->calib_offset;
> + }
> +
> + imem_regmap = syscon_regmap_lookup_by_phandle(np, "qcom,imem");
> + if (IS_ERR(imem_regmap)) {
> + dev_err(tsens_dev, "syscon regmap look up error\n");
> + return PTR_ERR(imem_regmap);
> + }
> +
> + if (num == 1)
> + ret = tsens_calib_sensors8660(tmdev, imem_regmap);
> + else
> + ret = tsens_calib_sensors8960(tmdev, imem_regmap);
> +
> + if (ret < 0) {
> + dev_err(tsens_dev, "tsense calibration failed\n");
> + return ret;
> + }
> +
> + base_node = of_parse_phandle(np, "qcom,tsens-base", 0);
> + if (base_node == NULL) {
Kernel style is typically
if (!base_node)
> + dev_err(tsens_dev, "no base node present\n");
> + return -EINVAL;
> + }
> +
> + base_pdev = of_find_device_by_node(base_node);
> + if (base_pdev == NULL) {
> + dev_err(tsens_dev, "no base pdev node\n");
> + return -ENODEV;
> + }
> +
> + tmdev->map = map = dev_get_regmap(&base_pdev->dev, NULL);
> + if (map == NULL) {
> + dev_err(tsens_dev, "base regmap get failed\n");
> + return -ENODEV;
> + }
> +
> + /* Status bits move when the sensor bits next to them overlap */
> + if (num > 5)
> + field = &status_0;
> + else
> + field = &status_8;
> +
> + tmdev->status_field = devm_regmap_field_alloc(tsens_dev, map, *field);
> + if (IS_ERR(tmdev->status_field)) {
> + dev_err(tsens_dev, "regmap alloc failed\n");
> + return PTR_ERR(tmdev->status_field);
> + }
> +
> + tsens_hw_init(tmdev);
> +
> + /*
> + * Register sensor 0 separately. This sensor is always
> + * expected to be present and if this fails, thermal
> + * sensor probe would fail
> + * Other sensors are optional and if registration fails
> + * disable the sensor and continue
> + */
> + ret = tsens_register(tmdev, 0);
> + if (ret < 0) {
> + dev_err(tsens_dev, "Registering failed for primary sensor");
> + ret = -ENODEV;
> + goto fail;
> + } else {
> + tsens_tz_set_mode(&tmdev->sensor[0], THERMAL_DEVICE_ENABLED);
> + }
> +
> + for (i = 1; i < tmdev->num_sensors; i++) {
> + ret = tsens_register(tmdev, i);
> +
> + if (ret < 0) {
> + dev_err(tsens_dev,
> + "Registering failed. Sensor(%i), disabled", i);
Missing newline...
> + tsens_tz_set_mode(&tmdev->sensor[i],
> + THERMAL_DEVICE_DISABLED);
Does this do anything? The mode is already
THERMAL_DEVICE_DISABLED so I imagine it just bails out
immediately?
> + } else {
> + tsens_tz_set_mode(&tmdev->sensor[i],
> + THERMAL_DEVICE_ENABLED);
> + }
> + }
> +
> + INIT_WORK(&tmdev->tsens_work, tsens_scheduler_fn);
> +
> + ret = devm_request_irq(tsens_dev, irq, tsens_isr, IRQF_TRIGGER_RISING,
> + "tsens", &tmdev->tsens_work);
> + if (ret < 0)
> + goto err_irq;
> +
> + platform_set_drvdata(pdev, tmdev);
> +
> + dev_info(tsens_dev, "Tsens driver initialized\n");
Please remove.
> +
> + return 0;
> +err_irq:
> + for (i = 0, s = tmdev->sensor; i < tmdev->num_sensors; i++, s++)
I should have made this simpler like:
for (s = tmdev->sensor; s < tmdev->sensor + tmdev->num_sensors; s++)
> + thermal_zone_of_sensor_unregister(&pdev->dev, s->tz_dev);
> +fail:
> + tsens_disable_mode(tmdev);
> + return ret;
> +}
> +
> +static int tsens_remove(struct platform_device *pdev)
> +{
> + int i;
Because then we can save a whole variable here.
> + struct tsens_sensor *s;
> + struct tsens_device *tmdev = platform_get_drvdata(pdev);
> +
> + tsens_disable_mode(tmdev);
> + for (i = 0, s = tmdev->sensor; i < tmdev->num_sensors; i++, s++)
> + thermal_zone_device_unregister(s->tz_dev);
> +
> + return 0;
> +}
> +
> +static struct of_device_id tsens_match_table[] = {
> + {.compatible = "qcom,ipq806x-tsens"},
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, tsens_match_table);
I would prefer we leave it as a child of GCC like I had done in
my original patch. The one ugly part was passing the DT node to
the virtual tsens device so that it could figure out the number
of sensors, etc. We should be able to fix that by creating the
device, assigning the of_node, and then registering the device
with the platform bus.
> +
> +static struct platform_driver tsens_driver = {
> + .probe = tsens_probe,
> + .remove = tsens_remove,
> + .driver = {
> + .of_match_table = tsens_match_table,
> + .name = "tsens8960-thermal",
A better name may be tsens-tm or qcom-tsens. The same version of
the hardware exists in so many different SoCs there really isn't
anything SoC specific about the hardware. The SoC difference is
almost entirely in the qfprom layout, except for when the hw
engineers decide to put a different number of sensors on the
chip. The same comment applies to the file name. Probably
qcom-tsens.c or something.
> + .owner = THIS_MODULE,
> +#ifdef CONFIG_PM
> + .pm = &tsens_pm_ops,
> +#endif
> + },
> +};
> +module_platform_driver(tsens_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Temperature Sensor driver");
> +MODULE_ALIAS("platform:tsens8960-tm");
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
