| Message ID | 20240111223020.3593558-9-nico@fluxnic.net |
|---|---|
| State | New |
| Headers | show |
| Series | Mediatek thermal sensor driver support for MT8186 and MT8188 | expand |
Hi Nico, On 11/01/2024 23:30, Nicolas Pitre wrote: > From: Nicolas Pitre <npitre@baylibre.com> > > Some systems don't always populate sensor controller slots starting > at slot 0. > > Signed-off-by: Nicolas Pitre <npitre@baylibre.com> > --- > drivers/thermal/mediatek/lvts_thermal.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c > index b20b70fd36..473ef91ea3 100644 > --- a/drivers/thermal/mediatek/lvts_thermal.c > +++ b/drivers/thermal/mediatek/lvts_thermal.c > @@ -112,6 +112,7 @@ struct lvts_ctrl_data { > struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX]; > int hw_tshut_temp; > int num_lvts_sensor; > + int skipped_sensors; > int offset; > int mode; > }; > @@ -555,6 +556,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, > const 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), > @@ -569,7 +571,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, > LVTS_IMMD3(lvts_ctrl->base) > }; > > - int i; > + int i, skip; > > for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) { > > @@ -604,8 +606,9 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, > /* > * Each sensor has its own register address to read from. > */ > + skip = lvts_ctrl_data->skipped_sensors; > lvts_sensor[i].msr = lvts_ctrl_data->mode == LVTS_MSR_IMMEDIATE_MODE ? > - imm_regs[i] : msr_regs[i]; > + imm_regs[i + skip] : msr_regs[i + skip]; Overall the series look ok but this changes is hard to understand. Could you propose a different approach to have the resulting code easier to understand ? > lvts_sensor[i].low_thresh = INT_MIN; > lvts_sensor[i].high_thresh = INT_MIN;
On Fri, 19 Jan 2024, Daniel Lezcano wrote: > > Hi Nico, > > On 11/01/2024 23:30, Nicolas Pitre wrote: > > From: Nicolas Pitre <npitre@baylibre.com> > > > > Some systems don't always populate sensor controller slots starting > > at slot 0. > > > > Signed-off-by: Nicolas Pitre <npitre@baylibre.com> > > --- > > drivers/thermal/mediatek/lvts_thermal.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/thermal/mediatek/lvts_thermal.c > > b/drivers/thermal/mediatek/lvts_thermal.c > > index b20b70fd36..473ef91ea3 100644 > > --- a/drivers/thermal/mediatek/lvts_thermal.c > > +++ b/drivers/thermal/mediatek/lvts_thermal.c > > @@ -112,6 +112,7 @@ struct lvts_ctrl_data { > > struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX]; > > int hw_tshut_temp; > > int num_lvts_sensor; > > + int skipped_sensors; > > int offset; > > int mode; > > }; > > @@ -555,6 +556,7 @@ static int lvts_sensor_init(struct device *dev, struct > > @@ lvts_ctrl *lvts_ctrl, > > const 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), > > @@ -569,7 +571,7 @@ static int lvts_sensor_init(struct device *dev, struct > > @@ lvts_ctrl *lvts_ctrl, > > LVTS_IMMD3(lvts_ctrl->base) > > }; > > - int i; > > + int i, skip; > > > > for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) { > > > > @@ -604,8 +606,9 @@ static int lvts_sensor_init(struct device *dev, struct > > @@ lvts_ctrl *lvts_ctrl, > > /* > > * Each sensor has its own register address to read from. > > */ > > + skip = lvts_ctrl_data->skipped_sensors; > > lvts_sensor[i].msr = lvts_ctrl_data->mode == > > LVTS_MSR_IMMEDIATE_MODE ? > > - imm_regs[i] : msr_regs[i]; > > + imm_regs[i + skip] : msr_regs[i + skip]; > > Overall the series look ok but this changes is hard to understand. > > Could you propose a different approach to have the resulting code easier to > understand ? I'm not sure how I could make it simpler. Maybe a comment is in order though? The sensor controller has 4 slots. Those slots are accessible either through imm_regs[<slot_number>] oe msr_regs[<slot_number>]. If, say, slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i = 0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this make sense? Nicolas
On Mon, 22 Jan 2024, Daniel Lezcano wrote: > > Hi Nico, > > On 19/01/2024 17:53, Nicolas Pitre wrote: > > [ ... ] > > >>> + skip = lvts_ctrl_data->skipped_sensors; > >>> lvts_sensor[i].msr = lvts_ctrl_data->mode == > >>> LVTS_MSR_IMMEDIATE_MODE ? > >>> - imm_regs[i] : msr_regs[i]; > >>> + imm_regs[i + skip] : msr_regs[i + skip]; > >> > >> Overall the series look ok but this changes is hard to understand. > >> > >> Could you propose a different approach to have the resulting code easier to > >> understand ? > > > > I'm not sure how I could make it simpler. Maybe a comment is in order > > though? > > > > The sensor controller has 4 slots. Those slots are accessible either > > through imm_regs[<slot_number>] oe msr_regs[<slot_number>]. If, say, > > slot 0 is unpopulated then sensor 0 (i = 0) needs to address slot 1 (i = > > 0, skip = 1), sensor 1 is in slot 2 (i = 1, skip = 1), etc. Does this > > make sense? > > Why not keep the sensor id = slot id and declare the ones which are disabled > with a mask? Then what do you do with the empty sensor 0? Do we want to present dead sensor IDs to users? Nicolas
diff --git a/drivers/thermal/mediatek/lvts_thermal.c b/drivers/thermal/mediatek/lvts_thermal.c index b20b70fd36..473ef91ea3 100644 --- a/drivers/thermal/mediatek/lvts_thermal.c +++ b/drivers/thermal/mediatek/lvts_thermal.c @@ -112,6 +112,7 @@ struct lvts_ctrl_data { struct lvts_sensor_data lvts_sensor[LVTS_SENSOR_MAX]; int hw_tshut_temp; int num_lvts_sensor; + int skipped_sensors; int offset; int mode; }; @@ -555,6 +556,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, const 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), @@ -569,7 +571,7 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, LVTS_IMMD3(lvts_ctrl->base) }; - int i; + int i, skip; for (i = 0; i < lvts_ctrl_data->num_lvts_sensor; i++) { @@ -604,8 +606,9 @@ static int lvts_sensor_init(struct device *dev, struct lvts_ctrl *lvts_ctrl, /* * Each sensor has its own register address to read from. */ + skip = lvts_ctrl_data->skipped_sensors; lvts_sensor[i].msr = lvts_ctrl_data->mode == LVTS_MSR_IMMEDIATE_MODE ? - imm_regs[i] : msr_regs[i]; + imm_regs[i + skip] : msr_regs[i + skip]; lvts_sensor[i].low_thresh = INT_MIN; lvts_sensor[i].high_thresh = INT_MIN;