From: Andi Shyti <andi.shyti@linux.intel.com> To: Badal Nilawar <badal.nilawar@intel.com> Cc: intel-gfx@lists.freedesktop.org, linux-hwmon@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Date: Wed, 21 Sep 2022 14:44:35 +0200 [thread overview] Message-ID: <YysHM8Ot1escBzEy@ashyti-mobl2.lan> (raw) In-Reply-To: <20220916150054.807590-2-badal.nilawar@intel.com> 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. > +static void > +hwm_get_preregistration_info(struct drm_i915_private *i915) > +{ > +} > + > +void i915_hwmon_register(struct drm_i915_private *i915) > +{ > + struct device *dev = i915->drm.dev; > + struct i915_hwmon *hwmon; > + struct device *hwmon_dev; > + struct hwm_drvdata *ddat; > + > + /* hwmon is available only for dGfx */ > + if (!IS_DGFX(i915)) > + return; > + > + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); why don't we use devm_kzalloc? > + if (!hwmon) > + return; > + > + i915->hwmon = hwmon; > + mutex_init(&hwmon->hwmon_lock); > + ddat = &hwmon->ddat; > + > + ddat->hwmon = hwmon; > + ddat->uncore = &i915->uncore; > + snprintf(ddat->name, sizeof(ddat->name), "i915"); > + > + hwm_get_preregistration_info(i915); > + > + /* hwmon_dev points to device hwmon<i> */ > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, > + ddat, > + &hwm_chip_info, > + NULL); > + if (IS_ERR(hwmon_dev)) { > + mutex_destroy(&hwmon->hwmon_lock); there is not such a big need to destroy the mutex. Destroying mutexes is more useful when you actually are creating/destroying and there is some debug need. I don't think that's the case. With the devm_kzalloc this would be just a return. Andi > + i915->hwmon = NULL; > + kfree(hwmon); > + return; > + } > + > + ddat->hwmon_dev = hwmon_dev; > +} > + > +void i915_hwmon_unregister(struct drm_i915_private *i915) > +{ > + struct i915_hwmon *hwmon; > + struct hwm_drvdata *ddat; > + > + hwmon = fetch_and_zero(&i915->hwmon); > + if (!hwmon) > + return; > + > + ddat = &hwmon->ddat; > + if (ddat->hwmon_dev) > + hwmon_device_unregister(ddat->hwmon_dev); > + > + mutex_destroy(&hwmon->hwmon_lock); > + kfree(hwmon); > +} > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > new file mode 100644 > index 000000000000..7ca9cf2c34c9 > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: MIT */ > + > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#ifndef __I915_HWMON_H__ > +#define __I915_HWMON_H__ > + > +struct drm_i915_private; > + > +#if IS_REACHABLE(CONFIG_HWMON) > +void i915_hwmon_register(struct drm_i915_private *i915); > +void i915_hwmon_unregister(struct drm_i915_private *i915); > +#else > +static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > +static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > +#endif > + > +#endif /* __I915_HWMON_H__ */ > -- > 2.25.1
WARNING: multiple messages have this Message-ID (diff)
From: Andi Shyti <andi.shyti@linux.intel.com> To: Badal Nilawar <badal.nilawar@intel.com> Cc: linux-hwmon@vger.kernel.org, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Date: Wed, 21 Sep 2022 14:44:35 +0200 [thread overview] Message-ID: <YysHM8Ot1escBzEy@ashyti-mobl2.lan> (raw) In-Reply-To: <20220916150054.807590-2-badal.nilawar@intel.com> 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. > +static void > +hwm_get_preregistration_info(struct drm_i915_private *i915) > +{ > +} > + > +void i915_hwmon_register(struct drm_i915_private *i915) > +{ > + struct device *dev = i915->drm.dev; > + struct i915_hwmon *hwmon; > + struct device *hwmon_dev; > + struct hwm_drvdata *ddat; > + > + /* hwmon is available only for dGfx */ > + if (!IS_DGFX(i915)) > + return; > + > + hwmon = kzalloc(sizeof(*hwmon), GFP_KERNEL); why don't we use devm_kzalloc? > + if (!hwmon) > + return; > + > + i915->hwmon = hwmon; > + mutex_init(&hwmon->hwmon_lock); > + ddat = &hwmon->ddat; > + > + ddat->hwmon = hwmon; > + ddat->uncore = &i915->uncore; > + snprintf(ddat->name, sizeof(ddat->name), "i915"); > + > + hwm_get_preregistration_info(i915); > + > + /* hwmon_dev points to device hwmon<i> */ > + hwmon_dev = hwmon_device_register_with_info(dev, ddat->name, > + ddat, > + &hwm_chip_info, > + NULL); > + if (IS_ERR(hwmon_dev)) { > + mutex_destroy(&hwmon->hwmon_lock); there is not such a big need to destroy the mutex. Destroying mutexes is more useful when you actually are creating/destroying and there is some debug need. I don't think that's the case. With the devm_kzalloc this would be just a return. Andi > + i915->hwmon = NULL; > + kfree(hwmon); > + return; > + } > + > + ddat->hwmon_dev = hwmon_dev; > +} > + > +void i915_hwmon_unregister(struct drm_i915_private *i915) > +{ > + struct i915_hwmon *hwmon; > + struct hwm_drvdata *ddat; > + > + hwmon = fetch_and_zero(&i915->hwmon); > + if (!hwmon) > + return; > + > + ddat = &hwmon->ddat; > + if (ddat->hwmon_dev) > + hwmon_device_unregister(ddat->hwmon_dev); > + > + mutex_destroy(&hwmon->hwmon_lock); > + kfree(hwmon); > +} > diff --git a/drivers/gpu/drm/i915/i915_hwmon.h b/drivers/gpu/drm/i915/i915_hwmon.h > new file mode 100644 > index 000000000000..7ca9cf2c34c9 > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_hwmon.h > @@ -0,0 +1,20 @@ > +/* SPDX-License-Identifier: MIT */ > + > +/* > + * Copyright © 2022 Intel Corporation > + */ > + > +#ifndef __I915_HWMON_H__ > +#define __I915_HWMON_H__ > + > +struct drm_i915_private; > + > +#if IS_REACHABLE(CONFIG_HWMON) > +void i915_hwmon_register(struct drm_i915_private *i915); > +void i915_hwmon_unregister(struct drm_i915_private *i915); > +#else > +static inline void i915_hwmon_register(struct drm_i915_private *i915) { }; > +static inline void i915_hwmon_unregister(struct drm_i915_private *i915) { }; > +#endif > + > +#endif /* __I915_HWMON_H__ */ > -- > 2.25.1
next prev parent reply other threads:[~2022-09-21 12:44 UTC|newest] Thread overview: 127+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-09-16 15:00 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-16 15:00 ` [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-21 10:59 ` Gupta, Anshuman 2022-09-21 10:59 ` [Intel-gfx] " Gupta, Anshuman 2022-09-21 10:59 ` Gupta, Anshuman 2022-09-21 12:44 ` Andi Shyti [this message] 2022-09-21 12:44 ` [Intel-gfx] " Andi Shyti 2022-09-21 15:17 ` Nilawar, Badal 2022-09-21 15:17 ` Nilawar, Badal 2022-09-21 15:45 ` Andi Shyti 2022-09-21 15:45 ` Andi Shyti 2022-09-21 15:45 ` Andi Shyti 2022-09-24 3:10 ` Dixit, Ashutosh 2022-09-24 3:10 ` Dixit, Ashutosh 2022-09-24 3:10 ` Dixit, Ashutosh 2022-09-16 15:00 ` [PATCH 2/7] drm/i915/hwmon: Add HWMON current voltage support Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-21 11:08 ` Gupta, Anshuman 2022-09-21 11:08 ` [Intel-gfx] " Gupta, Anshuman 2022-09-21 11:08 ` Gupta, Anshuman 2022-09-16 15:00 ` [PATCH 3/7] drm/i915/hwmon: Power PL1 limit and TDP setting Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-16 18:29 ` [Intel-gfx] " kernel test robot 2022-09-21 0:02 ` Dixit, Ashutosh 2022-09-21 0:02 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-21 0:02 ` Dixit, Ashutosh 2022-09-21 11:44 ` [Intel-gfx] " Tvrtko Ursulin 2022-09-21 11:45 ` Gupta, Anshuman 2022-09-21 11:45 ` Gupta, Anshuman 2022-09-21 11:45 ` [Intel-gfx] " Gupta, Anshuman 2022-09-21 14:53 ` Nilawar, Badal 2022-09-21 14:53 ` [Intel-gfx] " Nilawar, Badal 2022-09-21 14:53 ` Nilawar, Badal 2022-09-22 7:08 ` Gupta, Anshuman 2022-09-22 7:08 ` [Intel-gfx] " Gupta, Anshuman 2022-09-22 7:08 ` Gupta, Anshuman 2022-09-23 2:26 ` Dixit, Ashutosh 2022-09-23 2:26 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-23 2:26 ` Dixit, Ashutosh 2022-09-16 15:00 ` [PATCH 4/7] drm/i915/hwmon: Show device level energy usage Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-21 12:02 ` Gupta, Anshuman 2022-09-21 12:02 ` [Intel-gfx] " Gupta, Anshuman 2022-09-21 12:02 ` Gupta, Anshuman 2022-10-13 15:53 ` Dixit, Ashutosh 2022-10-13 15:53 ` [Intel-gfx] " Dixit, Ashutosh 2022-10-13 15:53 ` Dixit, Ashutosh 2022-09-16 15:00 ` [PATCH 5/7] drm/i915/hwmon: Expose card reactive critical power Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-21 15:07 ` Gupta, Anshuman 2022-09-21 15:07 ` [Intel-gfx] " Gupta, Anshuman 2022-09-21 15:07 ` Gupta, Anshuman 2022-09-22 3:17 ` Dixit, Ashutosh 2022-09-22 3:17 ` Dixit, Ashutosh 2022-09-22 3:17 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-22 5:24 ` Gupta, Anshuman 2022-09-22 5:24 ` Gupta, Anshuman 2022-09-22 5:24 ` Gupta, Anshuman 2022-09-16 15:00 ` [PATCH 6/7] drm/i915/hwmon: Expose power1_max_interval Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-22 7:13 ` Gupta, Anshuman 2022-09-22 7:13 ` Gupta, Anshuman 2022-09-22 7:13 ` [Intel-gfx] " Gupta, Anshuman 2022-09-23 2:51 ` Dixit, Ashutosh 2022-09-23 2:51 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-23 2:51 ` Dixit, Ashutosh 2022-09-23 4:23 ` Dixit, Ashutosh 2022-09-23 4:23 ` [Intel-gfx] " Dixit, Ashutosh 2022-09-23 4:23 ` Dixit, Ashutosh 2022-09-16 15:00 ` [PATCH 7/7] drm/i915/hwmon: Extend power/energy for XEHPSDV Badal Nilawar 2022-09-16 15:00 ` [Intel-gfx] " Badal Nilawar 2022-09-16 15:00 ` Badal Nilawar 2022-09-22 7:37 ` Gupta, Anshuman 2022-09-22 7:37 ` [Intel-gfx] " Gupta, Anshuman 2022-09-22 7:37 ` Gupta, Anshuman 2022-09-16 17:37 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Add HWMON support (rev6) Patchwork 2022-09-16 17:37 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork 2022-09-16 17:59 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork 2022-09-19 10:45 ` Nilawar, Badal 2022-09-19 10:15 ` [PATCH 0/7] drm/i915: Add HWMON support Gupta, Anshuman 2022-09-19 10:15 ` [Intel-gfx] " Gupta, Anshuman 2022-09-19 10:15 ` Gupta, Anshuman 2022-09-19 12:13 ` Nilawar, Badal 2022-09-19 12:13 ` [Intel-gfx] " Nilawar, Badal 2022-09-19 12:13 ` Nilawar, Badal 2022-09-19 15:35 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Add HWMON support (rev6) Patchwork 2022-09-19 17:13 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork -- strict thread matches above, loose matches on Subject: below -- 2022-10-13 15:45 [PATCH 0/7] drm/i915: Add HWMON support Ashutosh Dixit 2022-10-13 15:45 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Ashutosh Dixit 2022-09-27 5:50 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-09-27 5:50 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-10-03 20:50 ` Andi Shyti 2022-10-03 20:50 ` Andi Shyti 2022-09-26 17:52 [PATCH 0/7] Add HWMON support Badal Nilawar 2022-09-26 17:52 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-09-23 19:56 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-09-23 19:56 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-09-24 3:54 ` Dixit, Ashutosh 2022-08-25 13:21 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-08-25 13:21 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-08-26 13:30 ` Guenter Roeck 2022-08-29 17:26 ` Dixit, Ashutosh 2022-08-18 19:38 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-08-18 19:38 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-08-19 10:35 ` Jani Nikula 2022-08-19 11:41 ` Guenter Roeck 2022-08-19 11:41 ` Guenter Roeck 2022-08-23 8:42 ` Nilawar, Badal 2022-08-23 8:42 ` Nilawar, Badal 2022-08-23 9:46 ` Jani Nikula 2022-08-23 9:46 ` Jani Nikula 2022-08-23 12:19 ` Guenter Roeck 2022-08-23 12:19 ` Guenter Roeck 2022-08-23 13:35 ` Jani Nikula 2022-08-23 13:35 ` Jani Nikula 2022-08-23 14:28 ` Nilawar, Badal 2022-08-23 14:28 ` Nilawar, Badal 2022-08-23 14:41 ` Jani Nikula 2022-08-23 14:41 ` Jani Nikula 2022-08-25 7:27 ` Nilawar, Badal 2022-08-25 7:27 ` Nilawar, Badal 2022-08-19 10:37 ` Jani Nikula 2022-08-12 17:37 [PATCH 0/7] drm/i915: Add HWMON support Badal Nilawar 2022-08-12 17:37 ` [Intel-gfx] [PATCH 1/7] drm/i915/hwmon: Add HWMON infrastructure Badal Nilawar 2022-08-12 18:05 ` Guenter Roeck
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=YysHM8Ot1escBzEy@ashyti-mobl2.lan \ --to=andi.shyti@linux.intel.com \ --cc=badal.nilawar@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=linux-hwmon@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.