| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGuA+oqLiCxb1g7pwf+RwUTWHV37pXdAWUXHV51TnUy1-xUOXQ@mail.gmail.com>
Date: Wed, 1 Feb 2023 17:46:30 +0100
From: Balsam CHIHI <bchihi@...libre.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
Cc: daniel.lezcano@...aro.org, angelogioacchino.delregno@...labora.com,
rafael@...nel.org, amitk@...nel.org, rui.zhang@...el.com,
matthias.bgg@...il.com, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org, rdunlap@...radead.org,
ye.xingchen@....com.cn, p.zabel@...gutronix.de,
linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
khilman@...libre.com, james.lo@...iatek.com,
rex-bc.chen@...iatek.com
Subject: Re: [PATCH v12] thermal: drivers: mediatek: Add the Low Voltage
Thermal Sensor driver
Hi Krzysztof,
Thank you very much for the review!
On Wed, Feb 1, 2023 at 8:55 AM Krzysztof Kozlowski
<krzysztof.kozlowski@...aro.org> wrote:
>
> On 31/01/2023 16:38, bchihi@...libre.com wrote:
> > From: Balsam CHIHI <bchihi@...libre.com>
> >
> > The Low Voltage Thermal Sensor (LVTS) is a multiple sensors, multi
> > controllers contained in a thermal domain.
> >
> > A thermal domains can be the MCU or the AP.
> >
> > Each thermal domains contain up to seven controllers, each thermal
> > controller handle up to four thermal sensors.
> >
> > The LVTS has two Finite State Machines (FSM), one to handle the
> > functionin temperatures range like hot or cold temperature and another
> > one to handle monitoring trip point. The FSM notifies via interrupts
> > when a trip point is crossed.
> >
>
> (...)
>
> > +
> > +struct lvts_domain {
> > + struct lvts_ctrl *lvts_ctrl;
> > + struct reset_control *reset;
> > + struct clk *clk;
> > + int num_lvts_ctrl;
> > + void __iomem *base;
> > + size_t calib_len;
> > + u8 *calib;
> > +};
> > +
> > +#ifdef CONFIG_MTK_LVTS_THERMAL_DEBUGFS
> > +
> > +static struct dentry *root;
>
> How do you handle two instances of driver?
This root node is the topmost directory for debugfs called 'lvts', the
different driver instances are below this. It is a singleton.
>
> > +
> > +#define LVTS_DEBUG_FS_REGS(__reg) \
> > +{ \
> > + .name = __stringify(__reg), \
> > + .offset = __reg(0), \
> > +}
> > +
>
> (...)
>
> > +
> > +static int lvts_set_trips(struct thermal_zone_device *tz, int low, int high)
> > +{
> > + struct lvts_sensor *lvts_sensor = tz->devdata;
> > + void __iomem *base = lvts_sensor->base;
> > + u32 raw_low = lvts_temp_to_raw(low);
> > + u32 raw_high = lvts_temp_to_raw(high);
> > +
> > + /*
> > + * Hot to normal temperature threshold
> > + *
> > + * LVTS_H2NTHRE
> > + *
> > + * Bits:
> > + *
> > + * 14-0 : Raw temperature for threshold
> > + */
> > + if (low != -INT_MAX) {
> > + dev_dbg(&tz->device, "Setting low limit temperature interrupt: %d\n", low);
> > + writel(raw_low, LVTS_H2NTHRE(base));
> > + }
> > +
> > + /*
> > + * Hot temperature threshold
> > + *
> > + * LVTS_HTHRE
> > + *
> > + * Bits:
> > + *
> > + * 14-0 : Raw temperature for threshold
> > + */
> > + dev_dbg(&tz->device, "Setting high limit temperature interrupt: %d\n", high);
> > + writel(raw_high, LVTS_HTHRE(base));
> > +
> > + return 0;
> > +}
> > +
> > +static irqreturn_t lvts_ctrl_irq_handler(struct lvts_ctrl *lvts_ctrl)
> > +{
> > + irqreturn_t iret = IRQ_NONE;
> > + u32 value, masks[] = {
>
> Don't mix different types in one declaration. u32 and a pointer are
> quite different types.
I'm not sure to understand.
LVTS_INT_SENSORx are not pointers but register values.
>
> > + LVTS_INT_SENSOR0,
> > + LVTS_INT_SENSOR1,
> > + LVTS_INT_SENSOR2,
> > + LVTS_INT_SENSOR3
> > + };
> > + int i;
> > +
> > + /*
> > + * Interrupt monitoring status
> > + *
> > + * LVTS_MONINTST
> > + *
> > + * Bits:
> > + *
> > + * 31 : Interrupt for stage 3
> > + * 30 : Interrupt for stage 2
> > + * 29 : Interrupt for state 1
> > + * 28 : Interrupt using filter on sensor 3
> > + *
> > + * 27 : Interrupt using immediate on sensor 3
> > + * 26 : Interrupt normal to hot on sensor 3
> > + * 25 : Interrupt high offset on sensor 3
> > + * 24 : Interrupt low offset on sensor 3
> > + *
> > + * 23 : Interrupt hot threshold on sensor 3
> > + * 22 : Interrupt cold threshold on sensor 3
> > + * 21 : Interrupt using filter on sensor 2
> > + * 20 : Interrupt using filter on sensor 1
> > + *
> > + * 19 : Interrupt using filter on sensor 0
> > + * 18 : Interrupt using immediate on sensor 2
> > + * 17 : Interrupt using immediate on sensor 1
> > + * 16 : Interrupt using immediate on sensor 0
> > + *
> > + * 15 : Interrupt device access timeout interrupt
> > + * 14 : Interrupt normal to hot on sensor 2
> > + * 13 : Interrupt high offset interrupt on sensor 2
> > + * 12 : Interrupt low offset interrupt on sensor 2
> > + *
> > + * 11 : Interrupt hot threshold on sensor 2
> > + * 10 : Interrupt cold threshold on sensor 2
> > + * 9 : Interrupt normal to hot on sensor 1
> > + * 8 : Interrupt high offset interrupt on sensor 1
> > + *
> > + * 7 : Interrupt low offset interrupt on sensor 1
> > + * 6 : Interrupt hot threshold on sensor 1
> > + * 5 : Interrupt cold threshold on sensor 1
> > + * 4 : Interrupt normal to hot on sensor 0
> > + *
> > + * 3 : Interrupt high offset interrupt on sensor 0
> > + * 2 : Interrupt low offset interrupt on sensor 0
> > + * 1 : Interrupt hot threshold on sensor 0
> > + * 0 : Interrupt cold threshold on sensor 0
> > + *
> > + * We are interested in the sensor(s) responsible of the
> > + * interrupt event. We update the thermal framework with the
> > + * thermal zone associated with the sensor. The framework will
> > + * take care of the rest whatever the kind of interrupt, we
> > + * are only interested in which sensor raised the interrupt.
> > + *
> > + * sensor 3 interrupt: 0001 1111 1100 0000 0000 0000 0000 0000
> > + * => 0x1FC00000
> > + * sensor 2 interrupt: 0000 0000 0010 0100 0111 1100 0000 0000
> > + * => 0x00247C00
> > + * sensor 1 interrupt: 0000 0000 0001 0001 0000 0011 1110 0000
> > + * => 0X000881F0
> > + * sensor 0 interrupt: 0000 0000 0000 1001 0000 0000 0001 1111
> > + * => 0x0009001F
> > + */
> > + value = readl(LVTS_MONINTSTS(lvts_ctrl->base));
> > +
> > + /*
> > + * Let's figure out which sensors raised the interrupt
> > + *
> > + * NOTE: the masks array must be ordered with the index
> > + * corresponding to the sensor id eg. index=0, mask for
> > + * sensor0.
> > + */
> > + for (i = 0; i < ARRAY_SIZE(masks); i++) {
> > +
> > + if (!(value & masks[i]))
> > + continue;
> > +
> > + thermal_zone_device_update(lvts_ctrl->sensors[i].tz,
> > + THERMAL_TRIP_VIOLATED);
> > + iret = IRQ_HANDLED;
> > + }
> > +
> > + /*
> > + * Write back to clear the interrupt status (W1C)
> > + */
> > + writel(value, LVTS_MONINTSTS(lvts_ctrl->base));
> > +
> > + return iret;
> > +}
> > +
> > +/*
> > + * Temperature interrupt handler. Even if the driver supports more
> > + * interrupt modes, we use the interrupt when the temperature crosses
> > + * the hot threshold the way up and the way down (modulo the
> > + * hysteresis).
> > + *
> > + * Each thermal domain has a couple of interrupts, one for hardware
> > + * reset and another one for all the thermal events happening on the
> > + * different sensors.
> > + *
> > + * The interrupt is configured for thermal events when crossing the
> > + * hot temperature limit. At each interrupt, we check in every
> > + * controller if there is an interrupt pending.
> > + */
> > +static irqreturn_t lvts_irq_handler(int irq, void *data)
> > +{
> > + struct lvts_domain *lvts_td = data;
> > + irqreturn_t aux, iret = IRQ_NONE;
> > + int i;
> > +
> > + for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
> > +
> > + aux = lvts_ctrl_irq_handler(lvts_td->lvts_ctrl);
> > + if (aux != IRQ_HANDLED)
> > + continue;
> > +
> > + iret = IRQ_HANDLED;
> > + }
> > +
> > + return iret;
> > +}
> > +
> > +static struct thermal_zone_device_ops lvts_ops = {
> > + .get_temp = lvts_get_temp,
> > + .set_trips = lvts_set_trips,
> > +};
> > +
> > +static int __init lvts_sensor_init(struct device *dev,
> > + struct lvts_ctrl *lvts_ctrl,
> > + struct lvts_ctrl_data *lvts_ctrl_data)
> > +{
> > + struct lvts_sensor *lvts_sensor = lvts_ctrl->sensors;
> > + void __iomem *msr_regs[] = {
> > + LVTS_MSR0(lvts_ctrl->base),
> > + LVTS_MSR1(lvts_ctrl->base),
> > + LVTS_MSR2(lvts_ctrl->base),
> > + LVTS_MSR3(lvts_ctrl->base)
> > + };
> > +
> > + void __iomem *imm_regs[] = {
> > + LVTS_IMMD0(lvts_ctrl->base),
> > + LVTS_IMMD1(lvts_ctrl->base),
> > + LVTS_IMMD2(lvts_ctrl->base),
> > + LVTS_IMMD3(lvts_ctrl->base)
> > + };
> > +
> > + int i;
> > +
> > + for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) {
> > +
> > + int dt_id = lvts_ctrl_data->lvts_sensor[i].dt_id;
> > +
> > + /*
> > + * At this point, we don't know which id matches which
> > + * sensor. Let's set arbitrally the id from the index.
> > + */
> > + lvts_sensor[i].id = i;
> > +
> > + /*
> > + * The thermal zone registration will set the trip
> > + * point interrupt in the thermal controller
> > + * register. But this one will be reset in the
> > + * initialization after. So we need to post pone the
> > + * thermal zone creation after the controller is
> > + * setup. For this reason, we store the device tree
> > + * node id from the data in the sensor structure
> > + */
> > + lvts_sensor[i].dt_id = dt_id;
> > +
> > + /*
> > + * We assign the base address of the thermal
> > + * controller as a back pointer. So it will be
> > + * accessible from the different thermal framework ops
> > + * as we pass the lvts_sensor pointer as thermal zone
> > + * private data.
> > + */
> > + lvts_sensor[i].base = lvts_ctrl->base;
> > +
> > + /*
> > + * Each sensor has its own register address to read from.
> > + */
> > + lvts_sensor[i].msr = lvts_ctrl_data->mode == LVTS_MSR_IMMEDIATE_MODE ?
> > + imm_regs[i] : msr_regs[i];
> > + };
> > +
> > + lvts_ctrl->num_lvts_sensor = lvts_ctrl_data->num_lvts_sensor;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * The efuse blob values follows the sensor enumeration per thermal
> > + * controller. The decoding of the stream is as follow:
> > + *
> > + * <--?-> <----big0 ???---> <-sensor0-> <-0->
> > + * ------------------------------------------
> > + * index in the stream: : | 0x0 | 0x1 | 0x2 | 0x3 | 0x4 | 0x5 | 0x6 |
> > + * ------------------------------------------
> > + *
> > + * <--sensor1--><-0-> <----big1 ???---> <-sen
> > + * ------------------------------------------
> > + * | 0x7 | 0x8 | 0x9 | 0xA | 0xB | OxC | OxD |
> > + * ------------------------------------------
> > + *
> > + * sor0-> <-0-> <-sensor1-> <-0-> ..........
> > + * ------------------------------------------
> > + * | 0x7 | 0x8 | 0x9 | 0xA | 0xB | OxC | OxD |
> > + * ------------------------------------------
> > + *
> > + * And so on ...
> > + *
> > + * The data description gives the offset of the calibration data in
> > + * this bytes stream for each sensor.
> > + *
> > + * Each thermal controller can handle up to 4 sensors max, we don't
> > + * care if there are less as the array of calibration is sized to 4
> > + * anyway. The unused sensor slot will be zeroed.
> > + */
> > +static int __init lvts_calibration_init(struct device *dev,
> > + struct lvts_ctrl *lvts_ctrl,
> > + struct lvts_ctrl_data *lvts_ctrl_data,
> > + u8 *efuse_calibration)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++)
> > + memcpy(&lvts_ctrl->calibration[i],
> > + efuse_calibration + lvts_ctrl_data->cal_offset[i], 2);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * The efuse bytes stream can be split into different chunk of
> > + * nvmems. This function reads and concatenate those into a single
> > + * buffer so it can be read sequentially when initializing the
> > + * calibration data.
> > + */
> > +static int lvts_calibration_read(struct device *dev, struct lvts_domain *lvts_td,
> > + struct lvts_data *lvts_data)
> > +{
> > + struct device_node *np = dev_of_node(dev);
> > + struct nvmem_cell *cell;
> > + struct property *prop;
> > + const char *cell_name;
> > +
> > + of_property_for_each_string(np, "nvmem-cell-names", prop, cell_name) {
> > + size_t len;
> > + u8 *efuse;
> > +
> > + cell = of_nvmem_cell_get(np, cell_name);
> > + if (IS_ERR(cell)) {
> > + dev_dbg(dev, "Failed to get cell '%s'\n", cell_name);
>
> Is this an error? If so, why debug? dbg is not for errors.
AFAIK using dev_dbg does not increase ELF size when DEBUG is disabled.
If this is not a good reason for you, then I will change it to dev_err.
>
> > + return PTR_ERR(cell);
> > + }
> > +
> > + efuse = nvmem_cell_read(cell, &len);
> > +
> > + nvmem_cell_put(cell);
> > +
> > + if (IS_ERR(efuse)) {
> > + dev_dbg(dev, "Failed to read cell '%s'\n", cell_name);
> > + return PTR_ERR(efuse);
> > + }
> > +
> > + lvts_td->calib = devm_krealloc(dev, lvts_td->calib,
> > + lvts_td->calib_len + len, GFP_KERNEL);
> > + if (!lvts_td->calib)
> > + return -ENOMEM;
> > +
> > + memcpy(lvts_td->calib + lvts_td->calib_len, efuse, len);
> > +
> > + lvts_td->calib_len += len;
> > +
> > + kfree(efuse);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int __init lvts_golden_temp_init(struct device *dev, u32 *value)
>
> You did not test it, right? Build with section mismatch analysis...
I'm not sure to fully understand this comment.
Would you explain, please?
>
> > +{
> > + u32 gt;
> > +
> > + gt = (*value) >> 24;
> > +
> > + if (gt && gt < LVTS_GOLDEN_TEMP_MAX)
> > + golden_temp = gt;
> > +
> > + coeff_b = golden_temp * 500 + LVTS_COEFF_B;
> > +
> > + return 0;
> > +}
> > +
> > +static int __init lvts_ctrl_init(struct device *dev,
>
> Same problem.
Would you explain, please?
>
> > + struct lvts_domain *lvts_td,
> > + struct lvts_data *lvts_data)
> > +{
> > + size_t size = sizeof(*lvts_td->lvts_ctrl) * lvts_data->num_lvts_ctrl;
> > + struct lvts_ctrl *lvts_ctrl;
> > + int i, ret;
> > +
>
> > +
> > +static inline int lvts_ctrl_enable(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + return lvts_ctrl_set_enable(lvts_ctrl, 1);
>
> Drop the wrapper, it's useless. true or false for enable are quite obvious.
OK.
>
> > +}
> > +
> > +static inline int lvts_ctrl_disable(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + return lvts_ctrl_set_enable(lvts_ctrl, 0);
>
> Drop the wrapper.
OK.
>
> > +}
> > +
> > +static int lvts_ctrl_connect(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + u32 id, cmds[] = { 0xC103FFFF, 0xC502FF55 };
> > +
> > + lvts_write_config(lvts_ctrl, cmds, ARRAY_SIZE(cmds));
> > +
> > + /*
> > + * LVTS_ID : Get ID and status of the thermal controller
> > + *
> > + * Bits:
> > + *
> > + * 0-5 : thermal controller id
> > + * 7 : thermal controller connection is valid
> > + */
> > + id = readl(LVTS_ID(lvts_ctrl->base));
> > + if (!(id & BIT(7)))
> > + return -EIO;
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_ctrl_initialize(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + /*
> > + * Write device mask: 0xC1030000
> > + */
> > + u32 cmds[] = {
> > + 0xC1030E01, 0xC1030CFC, 0xC1030A8C, 0xC103098D, 0xC10308F1,
> > + 0xC10307A6, 0xC10306B8, 0xC1030500, 0xC1030420, 0xC1030300,
> > + 0xC1030030, 0xC10300F6, 0xC1030050, 0xC1030060, 0xC10300AC,
> > + 0xC10300FC, 0xC103009D, 0xC10300F1, 0xC10300E1
> > + };
> > +
> > + lvts_write_config(lvts_ctrl, cmds, ARRAY_SIZE(cmds));
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_ctrl_calibrate(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + int i;
> > + void __iomem *lvts_edata[] = {
> > + LVTS_EDATA00(lvts_ctrl->base),
> > + LVTS_EDATA01(lvts_ctrl->base),
> > + LVTS_EDATA02(lvts_ctrl->base),
> > + LVTS_EDATA03(lvts_ctrl->base)
> > + };
> > +
> > + /*
> > + * LVTS_EDATA0X : Efuse calibration reference value for sensor X
> > + *
> > + * Bits:
> > + *
> > + * 20-0 : Efuse value for normalization data
> > + */
> > + for (i = 0; i < LVTS_SENSOR_MAX; i++)
> > + writel(lvts_ctrl->calibration[i], lvts_edata[i]);
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_ctrl_configure(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + u32 value;
> > +
> > + /*
> > + * LVTS_TSSEL : Sensing point index numbering
> > + *
> > + * Bits:
> > + *
> > + * 31-24: ADC Sense 3
> > + * 23-16: ADC Sense 2
> > + * 15-8 : ADC Sense 1
> > + * 7-0 : ADC Sense 0
> > + */
> > + value = LVTS_TSSEL_CONF;
> > + writel(value, LVTS_TSSEL(lvts_ctrl->base));
> > +
> > + /*
> > + * LVTS_CALSCALE : ADC voltage round
> > + */
> > + value = 0x300;
> > + value = LVTS_CALSCALE_CONF;
> > +
> > + /*
> > + * LVTS_MSRCTL0 : Sensor filtering strategy
> > + *
> > + * Filters:
> > + *
> > + * 000 : One sample
> > + * 001 : Avg 2 samples
> > + * 010 : 4 samples, drop min and max, avg 2 samples
> > + * 011 : 6 samples, drop min and max, avg 4 samples
> > + * 100 : 10 samples, drop min and max, avg 8 samples
> > + * 101 : 18 samples, drop min and max, avg 16 samples
> > + *
> > + * Bits:
> > + *
> > + * 0-2 : Sensor0 filter
> > + * 3-5 : Sensor1 filter
> > + * 6-8 : Sensor2 filter
> > + * 9-11 : Sensor3 filter
> > + */
> > + value = LVTS_HW_FILTER << 9 | LVTS_HW_FILTER << 6 |
> > + LVTS_HW_FILTER << 3 | LVTS_HW_FILTER;
> > + writel(value, LVTS_MSRCTL0(lvts_ctrl->base));
> > +
> > + /*
> > + * LVTS_MSRCTL1 : Measurement control
> > + *
> > + * Bits:
> > + *
> > + * 9: Ignore MSRCTL0 config and do immediate measurement on sensor3
> > + * 6: Ignore MSRCTL0 config and do immediate measurement on sensor2
> > + * 5: Ignore MSRCTL0 config and do immediate measurement on sensor1
> > + * 4: Ignore MSRCTL0 config and do immediate measurement on sensor0
> > + *
> > + * That configuration will ignore the filtering and the delays
> > + * introduced below in MONCTL1 and MONCTL2
> > + */
> > + if (lvts_ctrl->mode == LVTS_MSR_IMMEDIATE_MODE) {
> > + value = BIT(9) | BIT(6) | BIT(5) | BIT(4);
> > + writel(value, LVTS_MSRCTL1(lvts_ctrl->base));
> > + }
> > +
> > + /*
> > + * LVTS_MONCTL1 : Period unit and group interval configuration
> > + *
> > + * The clock source of LVTS thermal controller is 26MHz.
> > + *
> > + * The period unit is a time base for all the interval delays
> > + * specified in the registers. By default we use 12. The time
> > + * conversion is done by multiplying by 256 and 1/26.10^6
> > + *
> > + * An interval delay multiplied by the period unit gives the
> > + * duration in seconds.
> > + *
> > + * - Filter interval delay is a delay between two samples of
> > + * the same sensor.
> > + *
> > + * - Sensor interval delay is a delay between two samples of
> > + * different sensors.
> > + *
> > + * - Group interval delay is a delay between different rounds.
> > + *
> > + * For example:
> > + * If Period unit = C, filter delay = 1, sensor delay = 2, group delay = 1,
> > + * and two sensors, TS1 and TS2, are in a LVTS thermal controller
> > + * and then
> > + * Period unit time = C * 1/26M * 256 = 12 * 38.46ns * 256 = 118.149us
> > + * Filter interval delay = 1 * Period unit = 118.149us
> > + * Sensor interval delay = 2 * Period unit = 236.298us
> > + * Group interval delay = 1 * Period unit = 118.149us
> > + *
> > + * TS1 TS1 ... TS1 TS2 TS2 ... TS2 TS1...
> > + * <--> Filter interval delay
> > + * <--> Sensor interval delay
> > + * <--> Group interval delay
> > + * Bits:
> > + * 29 - 20 : Group interval
> > + * 16 - 13 : Send a single interrupt when crossing the hot threshold (1)
> > + * or an interrupt everytime the hot threshold is crossed (0)
> > + * 9 - 0 : Period unit
> > + *
> > + */
> > + value = LVTS_GROUP_INTERVAL << 20 | LVTS_PERIOD_UNIT;
> > + writel(value, LVTS_MONCTL1(lvts_ctrl->base));
> > +
> > + /*
> > + * LVTS_MONCTL2 : Filtering and sensor interval
> > + *
> > + * Bits:
> > + *
> > + * 25-16 : Interval unit in PERIOD_UNIT between sample on
> > + * the same sensor, filter interval
> > + * 9-0 : Interval unit in PERIOD_UNIT between each sensor
> > + *
> > + */
> > + value = LVTS_FILTER_INTERVAL << 16 | LVTS_SENSOR_INTERVAL;
> > + writel(value, LVTS_MONCTL2(lvts_ctrl->base));
> > +
> > + return lvts_irq_init(lvts_ctrl);
> > +}
> > +
> > +static int lvts_ctrl_start(struct device *dev, struct lvts_ctrl *lvts_ctrl)
> > +{
> > + struct lvts_sensor *lvts_sensors = lvts_ctrl->sensors;
> > + struct thermal_zone_device *tz;
> > + u32 sensor_map = 0;
> > + int i;
> > +
> > + for (i = 0; i < lvts_ctrl->num_lvts_sensor; i++) {
> > +
> > + int dt_id = lvts_sensors[i].dt_id;
> > +
> > + tz = devm_thermal_of_zone_register(dev, dt_id, &lvts_sensors[i],
> > + &lvts_ops);
> > + if (IS_ERR(tz)) {
> > + /*
> > + * This thermal zone is not described in the
> > + * device tree. It is not an error from the
> > + * thermal OF code POV, we just continue.
> > + */
> > + if (PTR_ERR(tz) == -ENODEV)
> > + continue;
> > +
> > + return PTR_ERR(tz);
> > + }
> > +
> > + /*
> > + * The thermal zone pointer will be needed in the
> > + * interrupt handler, we store it in the sensor
> > + * structure. The thermal domain structure will be
> > + * passed to the interrupt handler private data as the
> > + * interrupt is shared for all the controller
> > + * belonging to the thermal domain.
> > + */
> > + lvts_sensors[i].tz = tz;
> > +
> > + /*
> > + * This sensor was correctly associated with a thermal
> > + * zone, let's set the corresponding bit in the sensor
> > + * map, so we can enable the temperature monitoring in
> > + * the hardware thermal controller.
> > + */
> > + sensor_map |= BIT(i);
> > + }
> > +
> > + /*
> > + * Bits:
> > + * 9: Single point access flow
> > + * 0-3: Enable sensing point 0-3
> > + *
> > + * The initialization of the thermal zones give us
> > + * which sensor point to enable. If any thermal zone
> > + * was not described in the device tree, it won't be
> > + * enabled here in the sensor map.
> > + */
> > + writel(sensor_map | BIT(9), LVTS_MONCTL0(lvts_ctrl->base));
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_domain_init(struct device *dev, struct lvts_domain *lvts_td,
> > + struct lvts_data *lvts_data)
> > +{
> > + struct lvts_ctrl *lvts_ctrl;
> > + int i, ret;
> > +
> > + ret = lvts_ctrl_init(dev, lvts_td, lvts_data);
> > + if (ret)
> > + return ret;
> > +
> > + ret = lvts_domain_reset(dev, lvts_td->reset);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to reset domain");
> > + return ret;
> > + }
> > +
> > + for (i = 0; i < lvts_td->num_lvts_ctrl; i++) {
> > +
> > + lvts_ctrl = &lvts_td->lvts_ctrl[i];
> > +
> > + /*
> > + * Initialization steps:
> > + *
> > + * - Enable the clock
> > + * - Connect to the LVTS
> > + * - Initialize the LVTS
> > + * - Prepare the calibration data
> > + * - Select monitored sensors
> > + * [ Configure sampling ]
> > + * [ Configure the interrupt ]
> > + * - Start measurement
> > + */
> > + ret = lvts_ctrl_enable(dev, lvts_ctrl);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to enable LVTS clock");
> > + return ret;
> > + }
> > +
> > + ret = lvts_ctrl_connect(dev, lvts_ctrl);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to connect to LVTS controller");
> > + return ret;
> > + }
> > +
> > + ret = lvts_ctrl_initialize(dev, lvts_ctrl);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to initialize controller");
> > + return ret;
> > + }
> > +
> > + ret = lvts_ctrl_calibrate(dev, lvts_ctrl);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to calibrate controller");
> > + return ret;
> > + }
> > +
> > + ret = lvts_ctrl_configure(dev, lvts_ctrl);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to configure controller");
> > + return ret;
> > + }
> > +
> > + ret = lvts_ctrl_start(dev, lvts_ctrl);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to start controller");
> > + return ret;
> > + }
> > + }
> > +
> > + return lvts_debugfs_init(dev, lvts_td);
> > +}
> > +
> > +static int lvts_probe(struct platform_device *pdev)
> > +{
> > + struct lvts_data *lvts_data;
> > + struct lvts_domain *lvts_td;
> > + struct device *dev = &pdev->dev;
> > + struct resource *res;
> > + int irq, ret;
> > +
> > + lvts_td = devm_kzalloc(dev, sizeof(*lvts_td), GFP_KERNEL);
> > + if (!lvts_td)
> > + return -ENOMEM;
> > +
> > + lvts_data = (struct lvts_data *)of_device_get_match_data(dev);
>
> Why do you need case?
Would you explain, please?
>
> > + if (!lvts_data) {
> > + dev_dbg(dev, "No platforme
>
> Drop. How is it even possible?
OK,
All "dev_dbg" in "probe" function will be replaced by "dev_err_probe"
>
> > + return -ENODATA;
> > + };
> > +
> > + lvts_td->clk = devm_clk_get_enabled(dev, NULL);
> > + if (IS_ERR(lvts_td->clk)) {
> > + dev_dbg(dev, "Failed to retrieve clock\n");
>
> Drop all debug statements. Either this is an error (so return
> dev_err_probe) or core handles messages.
I will change it to dev_err_probe.
>
> > + return PTR_ERR(lvts_td->clk);
> > + }
> > +
> > + res = platform_get_mem_or_io(pdev, 0);
> > + if (!res) {
> > + dev_dbg(dev, "No IO resource\n");
>
> Ditto
>
> > + return -ENXIO;
> > + }
> > +
> > + lvts_td->base = devm_ioremap_resource(dev, res);
>
> Why not using single wrapper for this?
OK,
I will chnagne it to "lvts_td->base =
devm_platform_get_and_ioremap_resource(pdev, 0, &res);"
>
> > + if (IS_ERR(lvts_td->base)) {
> > + dev_dbg(dev, "Failed to map io resource\n");
>
> Ditto
I will change it to dev_err_probe.
>
> > + return PTR_ERR(lvts_td->base);
> > + }
> > +
> > + lvts_td->reset = devm_reset_control_get_by_index(dev, 0);
> > + if (IS_ERR(lvts_td->reset)) {
> > + dev_dbg(dev, "Failed to get reset control\n");
>
> Ditto
I will change it to dev_err_probe.
>
> > + return PTR_ERR(lvts_td->reset);
> > + }
> > +
> > + irq = platform_get_irq(pdev, 0);
> > + if (irq < 0) {
> > + dev_dbg(dev, "No irq resource\n");
>
> Ditto
I will change it to dev_err_probe.
>
> > + return irq;
> > + }
> > +
> > + ret = lvts_domain_init(dev, lvts_td, lvts_data);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to initialize the lvts domain\n");
>
> Why this is debug?
I will change it to dev_err_probe.
>
> > + return ret;
> > + }
> > +
> > + /*
> > + * At this point the LVTS is initialized and enabled. We can
> > + * safely enable the interrupt.
> > + */
> > + ret = devm_request_threaded_irq(dev, irq, NULL, lvts_irq_handler,
> > + IRQF_ONESHOT, dev_name(dev), lvts_td);
> > + if (ret) {
> > + dev_dbg(dev, "Failed to request interrupt\n");
>
> Ditto
I will change it to dev_err_probe.
>
> > + return ret;
> > + }
> > +
> > + platform_set_drvdata(pdev, lvts_td);
> > +
> > + return 0;
> > +}
> > +
> > +static int lvts_remove(struct platform_device *pdev)
> > +{
> > + struct lvts_domain *lvts_td;
> > + struct device *dev = &pdev->dev;
> > + int i;
> > +
> > + lvts_td = platform_get_drvdata(pdev);
> > +
> > + for (i = 0; i < lvts_td->num_lvts_ctrl; i++)
> > + lvts_ctrl_disable(dev, &lvts_td->lvts_ctrl[i]);
> > +
> > + lvts_debugfs_exit();
> > +
> > + return 0;
> > +}
> > +
> > +static struct lvts_ctrl_data mt8195_lvts_data_ctrl[] = {
>
> Why this cannot be const?
I've got the following warning when I added "const"
drivers/thermal/mediatek/lvts_thermal.c:1286:27: warning:
initialization discards ‘const’ qualifier from pointer target type
[-Wdiscarded-qualifiers]
1286 | .lvts_ctrl = mt8195_lvts_data_ctrl,
| ^~~~~~~~~~~~~~~~~~~~~~~~~
Still working on it.
Any suggestion will be helpful, thanks!
>
> > + {
> > + .cal_offset = { 0x4, 0x7 },
> > + .lvts_sensor = {
> > + { .dt_id = MT8195_MCU_BIG_CPU0 },
> > + { .dt_id = MT8195_MCU_BIG_CPU1 }
> > + },
> > + .num_lvts_sensor = 2,
> > + .offset = 0x0,
> > + .hw_tshut_temp = LVTS_HW_SHUTDOWN_MT8195,
> > + },
> > +
>
> Drop blank line
OK,
I will do the same for other blank lines.
>
> > + {
>
> Best regards,
> Krzysztof
>
Best regards,
Balsam.
Powered by blists - more mailing lists