Re: [PATCH 8/9] thermal/drivers/mediatek/lvts_thermal: allow early empty sensor slots

Nicolas Pitre
2024-01-19T16:53:45Z

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