| Message ID | 20240111223020.3593558-9-nico@fluxnic.net (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Daniel Lezcano |
| 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
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?
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
On 22/01/2024 16:23, Nicolas Pitre wrote: > 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? You can skip it somehow in lvts_ctrl_start(), right?
On Mon, 22 Jan 2024, Daniel Lezcano wrote: > On 22/01/2024 16:23, Nicolas Pitre wrote: > > 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? > > You can skip it somehow in lvts_ctrl_start(), right? I see what you mean. Indeed that would be better. 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;