Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure

Andi Shyti
2022-09-21T15:45:56Z

Hi Badal,

> > > +struct hwm_reg {
> > > +};
> > > +
> > > +struct hwm_drvdata {
> > > + struct i915_hwmon *hwmon;
> > > + struct intel_uncore *uncore;
> > > + struct device *hwmon_dev;
> > > + char name[12];
> > > +};
> > > +
> > > +struct i915_hwmon {
> > > + struct hwm_drvdata ddat;
> > > + struct mutex hwmon_lock; /* counter overflow logic and rmw */
> > > + struct hwm_reg rg;
> > > +};
> > > +
> > > +static const struct hwmon_channel_info *hwm_info[] = {
> > > + NULL
> > > +};
> > > +
> > > +static umode_t
> > > +hwm_is_visible(const void *drvdata, enum hwmon_sensor_types type,
> > > + u32 attr, int channel)
> > > +{
> > > + switch (type) {
> > > + default:
> > > + return 0;
> > > + }
> > > +}
> > > +
> > > +static int
> > > +hwm_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> > > + int channel, long *val)
> > > +{
> > > + switch (type) {
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > +}
> > > +
> > > +static int
> > > +hwm_write(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> > > + int channel, long val)
> > > +{
> > > + switch (type) {
> > > + default:
> > > + return -EOPNOTSUPP;
> > > + }
> > > +}
> > > +
> > > +static const struct hwmon_ops hwm_ops = {
> > > + .is_visible = hwm_is_visible,
> > > + .read = hwm_read,
> > > + .write = hwm_write,
> > > +};
> > > +
> > > +static const struct hwmon_chip_info hwm_chip_info = {
> > > + .ops = &hwm_ops,
> > > + .info = hwm_info,
> > > +};
> >
> > what's the point for splitting so much? Can't you just send the
> > hwmon driver all at once? With this patch you are not actually
> > doing anything useful. In my opinion this should be squashed with
> > the next ones.

> During discussion in cover letter of rev0 series we decided to create
> separate infrastructure patch, as we wanted to keep kconfig, i915 hwmon
> structures and new file addition in separate patch. Further feature wise we
> kept adding new patches.

I don't really like this patch splitting, but it's my fault I
haven't reviewed it already in v1. Please, ignore then.

Andi