Re: [igt-dev] [PATCH i-g-t 2/3] lib/igt_sysfs: Add RPS sysfs helpers

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 21 Apr 2022 08:49:37 -0700, Kamil Konieczny wrote:
>
> Hi Ashutosh,

Hi Kamil,

> > +/**
> > + * igt_sysfs_dir_id_to_name:
> > + * @dir: sysfs directory fd
> > + * @id: sysfs attribute id
> > + *
> > + * Returns attribute name corresponding to attribute id in either the
> > + * per-gt or legacy per-device sysfs
> > + *
> > + * Returns:
> > + * Attribute name in sysfs
> > + */
> > +const char *igt_sysfs_dir_id_to_name(int dir, enum i915_attr_id id)
> > +{
> > +	igt_assert(id < SYSFS_NUM_ATTR);
>
> What about id < 0 ?

Changed to "igt_assert((uint32_t) id < SYSFS_NUM_ATTR);

> > +
> > +	if (igt_sysfs_has_attr(dir, i915_attr_name[GT][id]))
> > +		return i915_attr_name[GT][id];
> > +	else if (igt_sysfs_has_attr(dir, i915_attr_name[RPS][id]))
> ------- ^
> s/else //

Done.

> > +		return i915_attr_name[RPS][id];
> > +	else
> ------- ^
> drop this else here, checkpatch warn:
> WARNING: else is not generally useful after a break or return
> #102: FILE: lib/igt_sysfs.c:118:
> +               return i915_attr_name[RPS][id];
> +       else
>
> > +		igt_assert_f(0, "Invalid sysfs dir\n");
>
> and align this assert.

Done.

>
> Rest looks good, with that fixed you can add my r-b tag.

Thanks, merged with the above changes.