| Message ID | 20231127131651.476795-3-rui.zhang@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (coretemp) Fix core count limitation | expand |
On Mon, Nov 27, 2023 at 09:16:50PM +0800, Zhang Rui wrote: > When sensor_device_attribute pointer is available, use container_of() to > get the temp_data address. > > This removes the unnecessary dependency of cached index in > pdata->core_data[]. > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > --- > drivers/hwmon/coretemp.c | 15 +++++---------- > 1 file changed, 5 insertions(+), 10 deletions(-) > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c > index 6053ed3761c2..cef43fedbd58 100644 > --- a/drivers/hwmon/coretemp.c > +++ b/drivers/hwmon/coretemp.c > @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev, > { > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > struct platform_data *pdata = dev_get_drvdata(dev); > - struct temp_data *tdata = pdata->core_data[attr->index]; > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]); > > if (tdata->is_pkg_data) > return sprintf(buf, "Package id %u\n", pdata->pkg_id); > @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev, > { > u32 eax, edx; > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > - struct platform_data *pdata = dev_get_drvdata(dev); > - struct temp_data *tdata = pdata->core_data[attr->index]; > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]); > > mutex_lock(&tdata->update_lock); > rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx); > @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev, > struct device_attribute *devattr, char *buf) > { > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > - struct platform_data *pdata = dev_get_drvdata(dev); > - struct temp_data *tdata = pdata->core_data[attr->index]; > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]); > int tjmax; > > mutex_lock(&tdata->update_lock); > @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev, > struct device_attribute *devattr, char *buf) > { > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > - struct platform_data *pdata = dev_get_drvdata(dev); > - struct temp_data *tdata = pdata->core_data[attr->index]; > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]); > int ttarget; > > mutex_lock(&tdata->update_lock); > @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev, > { > u32 eax, edx; > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > - struct platform_data *pdata = dev_get_drvdata(dev); > - struct temp_data *tdata = pdata->core_data[attr->index]; > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]); > int tjmax; > > mutex_lock(&tdata->update_lock); > @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev, > tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i]; > tdata->sd_attrs[i].dev_attr.attr.mode = 0444; > tdata->sd_attrs[i].dev_attr.show = rd_ptr[i]; > - tdata->sd_attrs[i].index = attr_no; I was naively thinking if we could nuke that "index". I can see that used in couple macros, but seems like we can lose it? Completely untested.. and uncertain :-) diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h index d896713359cd..4855893f9401 100644 --- a/include/linux/hwmon-sysfs.h +++ b/include/linux/hwmon-sysfs.h @@ -12,36 +12,35 @@ struct sensor_device_attribute{ struct device_attribute dev_attr; - int index; }; #define to_sensor_dev_attr(_dev_attr) \ container_of(_dev_attr, struct sensor_device_attribute, dev_attr) -#define SENSOR_ATTR(_name, _mode, _show, _store, _index) \ +#define SENSOR_ATTR(_name, _mode, _show, _store) \ { .dev_attr = __ATTR(_name, _mode, _show, _store), \ - .index = _index } + } -#define SENSOR_ATTR_RO(_name, _func, _index) \ +#define SENSOR_ATTR_RO(_name, _func) \ SENSOR_ATTR(_name, 0444, _func##_show, NULL, _index) -#define SENSOR_ATTR_RW(_name, _func, _index) \ - SENSOR_ATTR(_name, 0644, _func##_show, _func##_store, _index) +#define SENSOR_ATTR_RW(_name, _func) \ + SENSOR_ATTR(_name, 0644, _func##_show, _func##_store) -#define SENSOR_ATTR_WO(_name, _func, _index) \ - SENSOR_ATTR(_name, 0200, NULL, _func##_store, _index) +#define SENSOR_ATTR_WO(_name, _func) \ + SENSOR_ATTR(_name, 0200, NULL, _func##_store) -#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index) \ +#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store) \ struct sensor_device_attribute sensor_dev_attr_##_name \ - = SENSOR_ATTR(_name, _mode, _show, _store, _index) + = SENSOR_ATTR(_name, _mode, _show, _store) -#define SENSOR_DEVICE_ATTR_RO(_name, _func, _index) \ - SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL, _index) +#define SENSOR_DEVICE_ATTR_RO(_name, _func) \ + SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL) #define SENSOR_DEVICE_ATTR_RW(_name, _func, _index) \ - SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store, _index) + SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store) -#define SENSOR_DEVICE_ATTR_WO(_name, _func, _index) \ - SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store, _index) +#define SENSOR_DEVICE_ATTR_WO(_name, _func) \ + SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store) struct sensor_device_attribute_2 { struct device_attribute dev_attr; diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c index 975da8e7f2a9..c3bbf2f7d6eb 100644 --- a/drivers/gpu/drm/i915/i915_hwmon.c +++ b/drivers/gpu/drm/i915/i915_hwmon.c @@ -239,7 +239,7 @@ hwm_power1_max_interval_store(struct device *dev, static SENSOR_DEVICE_ATTR(power1_max_interval, 0664, hwm_power1_max_interval_show, - hwm_power1_max_interval_store, 0); + hwm_power1_max_interval_store); static struct attribute *hwm_attributes[] = { &sensor_dev_attr_power1_max_interval.dev_attr.attr,
On 11/30/23 17:27, Ashok Raj wrote: > On Mon, Nov 27, 2023 at 09:16:50PM +0800, Zhang Rui wrote: >> When sensor_device_attribute pointer is available, use container_of() to >> get the temp_data address. >> >> This removes the unnecessary dependency of cached index in >> pdata->core_data[]. >> >> Signed-off-by: Zhang Rui <rui.zhang@intel.com> >> --- >> drivers/hwmon/coretemp.c | 15 +++++---------- >> 1 file changed, 5 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c >> index 6053ed3761c2..cef43fedbd58 100644 >> --- a/drivers/hwmon/coretemp.c >> +++ b/drivers/hwmon/coretemp.c >> @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev, >> { >> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> struct platform_data *pdata = dev_get_drvdata(dev); >> - struct temp_data *tdata = pdata->core_data[attr->index]; >> + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]); >> >> if (tdata->is_pkg_data) >> return sprintf(buf, "Package id %u\n", pdata->pkg_id); >> @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev, >> { >> u32 eax, edx; >> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> - struct platform_data *pdata = dev_get_drvdata(dev); >> - struct temp_data *tdata = pdata->core_data[attr->index]; >> + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]); >> >> mutex_lock(&tdata->update_lock); >> rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx); >> @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev, >> struct device_attribute *devattr, char *buf) >> { >> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> - struct platform_data *pdata = dev_get_drvdata(dev); >> - struct temp_data *tdata = pdata->core_data[attr->index]; >> + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]); >> int tjmax; >> >> mutex_lock(&tdata->update_lock); >> @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev, >> struct device_attribute *devattr, char *buf) >> { >> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> - struct platform_data *pdata = dev_get_drvdata(dev); >> - struct temp_data *tdata = pdata->core_data[attr->index]; >> + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]); >> int ttarget; >> >> mutex_lock(&tdata->update_lock); >> @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev, >> { >> u32 eax, edx; >> struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >> - struct platform_data *pdata = dev_get_drvdata(dev); >> - struct temp_data *tdata = pdata->core_data[attr->index]; >> + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]); >> int tjmax; >> >> mutex_lock(&tdata->update_lock); >> @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev, >> tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i]; >> tdata->sd_attrs[i].dev_attr.attr.mode = 0444; >> tdata->sd_attrs[i].dev_attr.show = rd_ptr[i]; >> - tdata->sd_attrs[i].index = attr_no; > > I was naively thinking if we could nuke that "index". I can see that used > in couple macros, but seems like we can lose it? > > Completely untested.. and uncertain :-) > If you had suggested to replace struct sensor_device_attribute sd_attrs[TOTAL_ATTRS]; with struct device_attribute sd_attrs[TOTAL_ATTRS]; what you suggested may actually be possible and make sense. However, suggesting to dump the index parameter of SENSOR_ATTR() completely because _this_ driver may no longer need it seems to be a little excessive. > > diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h > index d896713359cd..4855893f9401 100644 > --- a/include/linux/hwmon-sysfs.h > +++ b/include/linux/hwmon-sysfs.h > @@ -12,36 +12,35 @@ > > struct sensor_device_attribute{ > struct device_attribute dev_attr; > - int index; > }; > #define to_sensor_dev_attr(_dev_attr) \ > container_of(_dev_attr, struct sensor_device_attribute, dev_attr) > > -#define SENSOR_ATTR(_name, _mode, _show, _store, _index) \ > +#define SENSOR_ATTR(_name, _mode, _show, _store) \ > { .dev_attr = __ATTR(_name, _mode, _show, _store), \ > - .index = _index } > + } > > -#define SENSOR_ATTR_RO(_name, _func, _index) \ > +#define SENSOR_ATTR_RO(_name, _func) \ > SENSOR_ATTR(_name, 0444, _func##_show, NULL, _index) > > -#define SENSOR_ATTR_RW(_name, _func, _index) \ > - SENSOR_ATTR(_name, 0644, _func##_show, _func##_store, _index) > +#define SENSOR_ATTR_RW(_name, _func) \ > + SENSOR_ATTR(_name, 0644, _func##_show, _func##_store) > > -#define SENSOR_ATTR_WO(_name, _func, _index) \ > - SENSOR_ATTR(_name, 0200, NULL, _func##_store, _index) > +#define SENSOR_ATTR_WO(_name, _func) \ > + SENSOR_ATTR(_name, 0200, NULL, _func##_store) > > -#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index) \ > +#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store) \ > struct sensor_device_attribute sensor_dev_attr_##_name \ > - = SENSOR_ATTR(_name, _mode, _show, _store, _index) > + = SENSOR_ATTR(_name, _mode, _show, _store) > > -#define SENSOR_DEVICE_ATTR_RO(_name, _func, _index) \ > - SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL, _index) > +#define SENSOR_DEVICE_ATTR_RO(_name, _func) \ > + SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL) > > #define SENSOR_DEVICE_ATTR_RW(_name, _func, _index) \ > - SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store, _index) > + SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store) > > -#define SENSOR_DEVICE_ATTR_WO(_name, _func, _index) \ > - SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store, _index) > +#define SENSOR_DEVICE_ATTR_WO(_name, _func) \ > + SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store) > > struct sensor_device_attribute_2 { > struct device_attribute dev_attr; > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > index 975da8e7f2a9..c3bbf2f7d6eb 100644 > --- a/drivers/gpu/drm/i915/i915_hwmon.c > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > @@ -239,7 +239,7 @@ hwm_power1_max_interval_store(struct device *dev, > > static SENSOR_DEVICE_ATTR(power1_max_interval, 0664, > hwm_power1_max_interval_show, > - hwm_power1_max_interval_store, 0); > + hwm_power1_max_interval_store); > That driver could and should have used DEVICE_ATTR() instead of SENSOR_DEVICE_ATTR(), and there are various other drivers where that would have made sense. Actually, it should have used DEVICE_ATTR_RW() but that is just a detail. However, there are more than 2,000 uses of SENSOR_DEVICE_ATTR() and derived macros in the kernel. The large majority of those do set index to values != 0, and the affected drivers would not be happy if that argument disappeared. Frankly, I am not entirely sure if you were serious with your suggestion. I tried to give a serious reply, but I am not entirely sure if I succeeded. My apologies if some sarcasm was bleeding through. Guenter
On Thu, Nov 30, 2023 at 07:26:31PM -0800, Guenter Roeck wrote: > On 11/30/23 17:27, Ashok Raj wrote: > > On Mon, Nov 27, 2023 at 09:16:50PM +0800, Zhang Rui wrote: > > > When sensor_device_attribute pointer is available, use container_of() to > > > get the temp_data address. > > > > > > This removes the unnecessary dependency of cached index in > > > pdata->core_data[]. > > > > > > Signed-off-by: Zhang Rui <rui.zhang@intel.com> > > > --- > > > drivers/hwmon/coretemp.c | 15 +++++---------- > > > 1 file changed, 5 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c > > > index 6053ed3761c2..cef43fedbd58 100644 > > > --- a/drivers/hwmon/coretemp.c > > > +++ b/drivers/hwmon/coretemp.c > > > @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev, > > > { > > > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > > struct platform_data *pdata = dev_get_drvdata(dev); > > > - struct temp_data *tdata = pdata->core_data[attr->index]; > > > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]); > > > if (tdata->is_pkg_data) > > > return sprintf(buf, "Package id %u\n", pdata->pkg_id); > > > @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev, > > > { > > > u32 eax, edx; > > > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > > - struct platform_data *pdata = dev_get_drvdata(dev); > > > - struct temp_data *tdata = pdata->core_data[attr->index]; > > > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]); > > > mutex_lock(&tdata->update_lock); > > > rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx); > > > @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev, > > > struct device_attribute *devattr, char *buf) > > > { > > > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > > - struct platform_data *pdata = dev_get_drvdata(dev); > > > - struct temp_data *tdata = pdata->core_data[attr->index]; > > > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]); > > > int tjmax; > > > mutex_lock(&tdata->update_lock); > > > @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev, > > > struct device_attribute *devattr, char *buf) > > > { > > > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > > - struct platform_data *pdata = dev_get_drvdata(dev); > > > - struct temp_data *tdata = pdata->core_data[attr->index]; > > > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]); > > > int ttarget; > > > mutex_lock(&tdata->update_lock); > > > @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev, > > > { > > > u32 eax, edx; > > > struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > > - struct platform_data *pdata = dev_get_drvdata(dev); > > > - struct temp_data *tdata = pdata->core_data[attr->index]; > > > + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]); > > > int tjmax; > > > mutex_lock(&tdata->update_lock); > > > @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev, > > > tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i]; > > > tdata->sd_attrs[i].dev_attr.attr.mode = 0444; > > > tdata->sd_attrs[i].dev_attr.show = rd_ptr[i]; > > > - tdata->sd_attrs[i].index = attr_no; > > > > I was naively thinking if we could nuke that "index". I can see that used > > in couple macros, but seems like we can lose it? > > > > Completely untested.. and uncertain :-) > > > > If you had suggested to replace > struct sensor_device_attribute sd_attrs[TOTAL_ATTRS]; > with > struct device_attribute sd_attrs[TOTAL_ATTRS]; > what you suggested may actually be possible and make sense. However, > suggesting to dump the index parameter of SENSOR_ATTR() completely > because _this_ driver may no longer need it seems to be a little excessive. I should have highlighted the uncertain :-).. Said naively thinking to add color that I'm calling it blind. But what you suggest might make more sense. I was just suggesting if there is more cleanup that could be accomplished along with this might be a good thing. I tried a quick and dirty cleanup.. apparently it was more dirty I guess :-) > > > > > diff --git a/include/linux/hwmon-sysfs.h b/include/linux/hwmon-sysfs.h > > index d896713359cd..4855893f9401 100644 > > --- a/include/linux/hwmon-sysfs.h > > +++ b/include/linux/hwmon-sysfs.h > > @@ -12,36 +12,35 @@ > > struct sensor_device_attribute{ > > struct device_attribute dev_attr; > > - int index; > > }; > > #define to_sensor_dev_attr(_dev_attr) \ > > container_of(_dev_attr, struct sensor_device_attribute, dev_attr) > > -#define SENSOR_ATTR(_name, _mode, _show, _store, _index) \ > > +#define SENSOR_ATTR(_name, _mode, _show, _store) \ > > { .dev_attr = __ATTR(_name, _mode, _show, _store), \ > > - .index = _index } > > + } > > -#define SENSOR_ATTR_RO(_name, _func, _index) \ > > +#define SENSOR_ATTR_RO(_name, _func) \ > > SENSOR_ATTR(_name, 0444, _func##_show, NULL, _index) > > -#define SENSOR_ATTR_RW(_name, _func, _index) \ > > - SENSOR_ATTR(_name, 0644, _func##_show, _func##_store, _index) > > +#define SENSOR_ATTR_RW(_name, _func) \ > > + SENSOR_ATTR(_name, 0644, _func##_show, _func##_store) > > -#define SENSOR_ATTR_WO(_name, _func, _index) \ > > - SENSOR_ATTR(_name, 0200, NULL, _func##_store, _index) > > +#define SENSOR_ATTR_WO(_name, _func) \ > > + SENSOR_ATTR(_name, 0200, NULL, _func##_store) > > -#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store, _index) \ > > +#define SENSOR_DEVICE_ATTR(_name, _mode, _show, _store) \ > > struct sensor_device_attribute sensor_dev_attr_##_name \ > > - = SENSOR_ATTR(_name, _mode, _show, _store, _index) > > + = SENSOR_ATTR(_name, _mode, _show, _store) > > -#define SENSOR_DEVICE_ATTR_RO(_name, _func, _index) \ > > - SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL, _index) > > +#define SENSOR_DEVICE_ATTR_RO(_name, _func) \ > > + SENSOR_DEVICE_ATTR(_name, 0444, _func##_show, NULL) > > #define SENSOR_DEVICE_ATTR_RW(_name, _func, _index) \ > > - SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store, _index) > > + SENSOR_DEVICE_ATTR(_name, 0644, _func##_show, _func##_store) > > -#define SENSOR_DEVICE_ATTR_WO(_name, _func, _index) \ > > - SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store, _index) > > +#define SENSOR_DEVICE_ATTR_WO(_name, _func) \ > > + SENSOR_DEVICE_ATTR(_name, 0200, NULL, _func##_store) > > struct sensor_device_attribute_2 { > > struct device_attribute dev_attr; > > diff --git a/drivers/gpu/drm/i915/i915_hwmon.c b/drivers/gpu/drm/i915/i915_hwmon.c > > index 975da8e7f2a9..c3bbf2f7d6eb 100644 > > --- a/drivers/gpu/drm/i915/i915_hwmon.c > > +++ b/drivers/gpu/drm/i915/i915_hwmon.c > > @@ -239,7 +239,7 @@ hwm_power1_max_interval_store(struct device *dev, > > static SENSOR_DEVICE_ATTR(power1_max_interval, 0664, > > hwm_power1_max_interval_show, > > - hwm_power1_max_interval_store, 0); > > + hwm_power1_max_interval_store); > > That driver could and should have used DEVICE_ATTR() instead of > SENSOR_DEVICE_ATTR(), and there are various other drivers where > that would have made sense. Actually, it should have used > DEVICE_ATTR_RW() but that is just a detail. > > However, there are more than 2,000 uses of SENSOR_DEVICE_ATTR() and derived > macros in the kernel. The large majority of those do set index to values != 0, > and the affected drivers would not be happy if that argument disappeared. > > Frankly, I am not entirely sure if you were serious with your suggestion. Certainly can't be serious.. but I was hinting at additional cleanups.. but I picked the wrong one obviously. > I tried to give a serious reply, but I am not entirely sure if I succeeded. > My apologies if some sarcasm was bleeding through. :-)... sarcasm is OK..
> > > mutex_lock(&tdata->update_lock); > > > @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data > > > *tdata, struct device *dev, > > > tdata->sd_attrs[i].dev_attr.attr.name = tdata- > > > >attr_name[i]; > > > tdata->sd_attrs[i].dev_attr.attr.mode = 0444; > > > tdata->sd_attrs[i].dev_attr.show = rd_ptr[i]; > > > - tdata->sd_attrs[i].index = attr_no; > > > > I was naively thinking if we could nuke that "index". I can see > > that used > > in couple macros, but seems like we can lose it? > > > > Completely untested.. and uncertain :-) > > > > If you had suggested to replace > struct sensor_device_attribute sd_attrs[TOTAL_ATTRS]; > with > struct device_attribute sd_attrs[TOTAL_ATTRS]; > what you suggested may actually be possible and make sense. Too late for me today. Let me check if I can convert it to use device_attribute instead tomorrow. thanks, rui
diff --git a/drivers/hwmon/coretemp.c b/drivers/hwmon/coretemp.c index 6053ed3761c2..cef43fedbd58 100644 --- a/drivers/hwmon/coretemp.c +++ b/drivers/hwmon/coretemp.c @@ -342,7 +342,7 @@ static ssize_t show_label(struct device *dev, { struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); struct platform_data *pdata = dev_get_drvdata(dev); - struct temp_data *tdata = pdata->core_data[attr->index]; + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_LABEL]); if (tdata->is_pkg_data) return sprintf(buf, "Package id %u\n", pdata->pkg_id); @@ -355,8 +355,7 @@ static ssize_t show_crit_alarm(struct device *dev, { u32 eax, edx; struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct platform_data *pdata = dev_get_drvdata(dev); - struct temp_data *tdata = pdata->core_data[attr->index]; + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_CRIT_ALARM]); mutex_lock(&tdata->update_lock); rdmsr_on_cpu(tdata->cpu, tdata->status_reg, &eax, &edx); @@ -369,8 +368,7 @@ static ssize_t show_tjmax(struct device *dev, struct device_attribute *devattr, char *buf) { struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct platform_data *pdata = dev_get_drvdata(dev); - struct temp_data *tdata = pdata->core_data[attr->index]; + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TJMAX]); int tjmax; mutex_lock(&tdata->update_lock); @@ -384,8 +382,7 @@ static ssize_t show_ttarget(struct device *dev, struct device_attribute *devattr, char *buf) { struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct platform_data *pdata = dev_get_drvdata(dev); - struct temp_data *tdata = pdata->core_data[attr->index]; + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TTARGET]); int ttarget; mutex_lock(&tdata->update_lock); @@ -402,8 +399,7 @@ static ssize_t show_temp(struct device *dev, { u32 eax, edx; struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); - struct platform_data *pdata = dev_get_drvdata(dev); - struct temp_data *tdata = pdata->core_data[attr->index]; + struct temp_data *tdata = container_of(attr, struct temp_data, sd_attrs[ATTR_TEMP]); int tjmax; mutex_lock(&tdata->update_lock); @@ -445,7 +441,6 @@ static int create_core_attrs(struct temp_data *tdata, struct device *dev, tdata->sd_attrs[i].dev_attr.attr.name = tdata->attr_name[i]; tdata->sd_attrs[i].dev_attr.attr.mode = 0444; tdata->sd_attrs[i].dev_attr.show = rd_ptr[i]; - tdata->sd_attrs[i].index = attr_no; tdata->attrs[i] = &tdata->sd_attrs[i].dev_attr.attr; } tdata->attr_group.attrs = tdata->attrs;
When sensor_device_attribute pointer is available, use container_of() to get the temp_data address. This removes the unnecessary dependency of cached index in pdata->core_data[]. Signed-off-by: Zhang Rui <rui.zhang@intel.com> --- drivers/hwmon/coretemp.c | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-)