Hi Andrzej,
[...]
> > +static ssize_t act_freq_mhz_show(struct device *dev,
> > + struct device_attribute *attr, char *buff)
> > +{
> > + s64 actual_freq = sysfs_gt_attribute_r_func(dev, attr,
> > + __act_freq_mhz_show);
> > +
> > + return sysfs_emit(buff, "%u\n", (u32) actual_freq);
>
> Again, variable can be just u32.
yes
[...]
> > +static int __boost_freq_mhz_store(struct intel_gt *gt, u32 val)
> > +{
> > + struct intel_rps *rps = >->rps;
> > + bool boost = false;
> > +
> > + /* Validate against (static) hardware limits */
> > + val = intel_freq_opcode(rps, val);
> > + if (val < rps->min_freq || val > rps->max_freq)
> > + return -EINVAL;
> > +
> > + mutex_lock(&rps->lock);
> > + if (val != rps->boost_freq) {
> > + rps->boost_freq = val;
> > + boost = atomic_read(&rps->num_waiters);
> > + }
> > + mutex_unlock(&rps->lock);
> > + if (boost)
> > + schedule_work(&rps->work);
> > +
> > + return 0;
>
> Why not intel_rps_set_boost_frequency?
> Why copy/paste from rps_set_boost_freq?
you are right... this must be some ugly leftover. Thanks!
[...]
> > +/* For now we have a static number of RP states */
> > +static ssize_t rps_rp_mhz_show(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buff)
> > +{
> > + struct intel_gt *gt = intel_gt_sysfs_get_drvdata(dev, attr->attr.name);
> > + struct intel_rps *rps = >->rps;
> > + u32 val;
> > +
> > + if (attr == &dev_attr_gt_RP0_freq_mhz ||
> > + attr == &dev_attr_rps_RP0_freq_mhz) {
> > + val = intel_rps_get_rp0_frequency(rps);
> > + } else if (attr == &dev_attr_gt_RP1_freq_mhz ||
> > + attr == &dev_attr_rps_RP1_freq_mhz) {
> > + val = intel_rps_get_rp1_frequency(rps);
> > + } else if (attr == &dev_attr_gt_RPn_freq_mhz ||
> > + attr == &dev_attr_rps_RPn_freq_mhz) {
> > + val = intel_rps_get_rpn_frequency(rps);
> > + } else {
> > + GEM_WARN_ON(1);
> > + return -ENODEV;
>
> I guess this is not necessary, otherwise similar path should be in other
> helpers.
yeah... it's a bit hacky, I must agree... will split it properly.
[...]
Thanks,
Andi
