Re: TPS68470 PMIC config option
From: Sakari Ailus <sakari.ailus@linux.intel.com>
Date: 2021-09-02 12:15:36
Hi Jean, On Thu, Sep 02, 2021 at 11:57:31AM +0200, Jean Delvare wrote:
Hi Sakari, On Wed, 1 Sep 2021 22:32:51 +0300, Sakari Ailus wrote:quoted
On Wed, Sep 01, 2021 at 08:39:19PM +0300, Andy Shevchenko wrote:quoted
On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:quoted
Is there a reason why config TPS68470_PMIC_OPREGION is not under "if PMIC_OPREGION" where all other *_PMIC_OPREGION driver options are?It was originally like that. Sakari, do you know?The answer can be found in Makefile: obj-$(CONFIG_PMIC_OPREGION) += intel_pmic.o intel_pmic.c seems to contain common functionality for PMICs in Intel SoCs whereas the TPS68470 is an external chip. The two codebases are distinct. Perhaps it could make sense to either rename this as CONFIG_PMIC_INTEL_OPREGION, or move the TPS68470 driver in and change the Kconfig+Makefile to have the common code compiled if at least one of the drivers is enabled.OK, thanks for the explanation I get it now. Yes, the fact that the menu looks vendor-neutral while it is about Intel drivers only is confusing. Renaming it would help. I'm not sure about your alternative proposal as I can't actually see any common code or dependency between intel_pmic and tps68470_pmic.
There isn't. I was thinking whether all PMIC opregion drivers would be behind a menu entry. If you have any sort of a generic kernel then you'd probably want all of these in anyway. I don't really have an opinion at this point though.
quoted hunk ↗ jump to hunk
What about the following? From: Jean Delvare <redacted> Subject: ACPI / PMIC: Rename CONFIG_PMIC_OPREGION Rename the intel_pmic driver's Kconfig option to make it clear it's about the Intel chipset family. Signed-off-by: Jean Delvare <redacted> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: "Rafael J. Wysocki" <redacted> Cc: Len Brown <lenb@kernel.org> Cc: Andy Shevchenko <andy@kernel.org> Cc: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/acpi/pmic/Kconfig | 10 +++++----- drivers/acpi/pmic/Makefile | 2 +- drivers/gpu/drm/i915/display/intel_dsi_vbt.c | 4 ++-- drivers/staging/media/atomisp/Kconfig | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-)--- linux-5.14.orig/drivers/acpi/pmic/Kconfig 2021-08-30 00:04:50.000000000 +0200 +++ linux-5.14/drivers/acpi/pmic/Kconfig 2021-09-02 11:51:14.146662112 +0200@@ -1,14 +1,14 @@ # SPDX-License-Identifier: GPL-2.0 -menuconfig PMIC_OPREGION - bool "PMIC (Power Management Integrated Circuit) operation region support" +menuconfig INTEL_PMIC_OPREGION + bool "Intel PMIC (Power Management Integrated Circuit) operation region support" help Select this option to enable support for ACPI operation - region of the PMIC chip. The operation region can be used + region of the Intel PMIC chip. The operation region can be used to control power rails and sensor reading/writing on the PMIC chip. -if PMIC_OPREGION +if INTEL_PMIC_OPREGION config BYTCRC_PMIC_OPREGION bool "ACPI operation region support for Bay Trail Crystal Cove PMIC"@@ -48,7 +48,7 @@ config CHT_DC_TI_PMIC_OPREGION help This config adds ACPI operation region support for Dollar Cove TI PMIC. -endif # PMIC_OPREGION +endif # INTEL_PMIC_OPREGION config TPS68470_PMIC_OPREGION bool "ACPI operation region support for TPS68470 PMIC" --- linux-5.14.orig/drivers/acpi/pmic/Makefile 2021-08-30 00:04:50.000000000 +0200 +++ linux-5.14/drivers/acpi/pmic/Makefile 2021-09-02 11:21:34.527694178 +0200@@ -1,6 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 -obj-$(CONFIG_PMIC_OPREGION) += intel_pmic.o +obj-$(CONFIG_INTEL_PMIC_OPREGION) += intel_pmic.o obj-$(CONFIG_BYTCRC_PMIC_OPREGION) += intel_pmic_bytcrc.o obj-$(CONFIG_CHTCRC_PMIC_OPREGION) += intel_pmic_chtcrc.o obj-$(CONFIG_XPOWER_PMIC_OPREGION) += intel_pmic_xpower.o --- linux-5.14.orig/drivers/staging/media/atomisp/Kconfig 2021-08-30 00:04:50.000000000 +0200 +++ linux-5.14/drivers/staging/media/atomisp/Kconfig 2021-09-02 11:51:39.792007892 +0200@@ -12,7 +12,7 @@ menuconfig INTEL_ATOMISP config VIDEO_ATOMISP tristate "Intel Atom Image Signal Processor Driver" depends on VIDEO_V4L2 && INTEL_ATOMISP - depends on PMIC_OPREGION + depends on INTEL_PMIC_OPREGION select IOSF_MBI select VIDEOBUF_VMALLOC select VIDEO_V4L2_SUBDEV_API --- linux-5.14.orig/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 2021-08-30 00:04:50.000000000 +0200 +++ linux-5.14/drivers/gpu/drm/i915/display/intel_dsi_vbt.c 2021-09-02 11:52:38.230795493 +0200@@ -511,7 +511,7 @@ static const u8 *mipi_exec_spi(struct in static const u8 *mipi_exec_pmic(struct intel_dsi *intel_dsi, const u8 *data) { struct drm_i915_private *i915 = to_i915(intel_dsi->base.base.dev); -#ifdef CONFIG_PMIC_OPREGION +#ifdef CONFIG_INTEL_PMIC_OPREGION u32 value, mask, reg_address; u16 i2c_address; int ret;@@ -529,7 +529,7 @@ static const u8 *mipi_exec_pmic(struct i drm_err(&i915->drm, "%s failed, error: %d\n", __func__, ret); #else drm_err(&i915->drm, - "Your hardware requires CONFIG_PMIC_OPREGION and it is not set\n"); + "Your hardware requires CONFIG_INTEL_PMIC_OPREGION and it is not set\n");
I wonder if this is just an Intel PMIC or whether it could be any PMIC. Well, the dependency seems rather machine specific but could in principle appear anywhere. Cc Hans and Ville.
#endif return data + 15;
-- Kind regards, Sakari Ailus