Hi Badal,
> > > + /* val in hw units */
> > > + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon->scl_shift_time, SF_TIME);
> > > + /* Convert to 1.x * power(2,y) */
> > > + if (!val) {
> > > + /* Avoid ilog2(0) */
> > > + y = 0;
> > > + x = 0;
> > > + } else {
> > > + y = ilog2(val);
> > > + /* x = (val - (1 << y)) >> (y - 2); */
> >
> > this is some spurious development comment, can you please remove
> > it?
>
> This is kept intentionally to help to understand the calculations.
then this is confusing... Can you please expand the concept?
As it is it's not understandable and I would expect someone
sending a patch with title:
[PATCH] drm/xe/hwmon: Remove spurious comment
Because it just looks forgotten from previous development.
> > > + x = (val - (1ul << y)) << x_w >> y;
> > > + }
> > > +
> > > + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) | REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
> > > +
> > > + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> > > +
> > > + mutex_lock(&hwmon->hwmon_lock);
> > > +
> > > + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT, REG_RMW, (u32 *)&r,
> > > + PKG_PWR_LIM_1_TIME, rxy);
> > > +
> > > + mutex_unlock(&hwmon->hwmon_lock);
> >
> > why are we locking here?
>
> Since it is rmw operation we are using lock here.
OK... so what you are trying to protect here is the
read -> update -> write
and it makes sense. The problem is that if this is a generic
rule, which means that everyone who will do a rmw operation has
to take the lock, why not take the lock directly in
xe_hwmon_process_reg()?
But also this can be a bit confusing, because a function is
either locked or unlocked and purists might complain.
A suggestion would be to do something like:
static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation operation)
{
...
}
static int xe_hwmon_reg_read(...);
{
return xe_hwmon_process_reg(..., REG_READ);
}
static int xe_hwmon_reg_write(...);
{
return xe_hwmon_process_reg(..., REG_WRITE);
}
static int xe_hwmon_reg_rmw(...);
{
int ret;
/*
* Optional: you can check that the lock is not taken
* to shout loud if potential deadlocks arise.
*/
/*
* We want to protect the register update with the
* lock blah blah blah... explanatory comment.
*/
mutex_lock(&hwmon->hwmon_lock);
ret = xe_hwmon_process_reg(..., REG_RMW);
mutex_unlock(&hwmon->hwmon_lock);
return ret;
}
What do you think? It looks much clearer to me.
Andi
