Re: TPS68470 PMIC config option

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Hans, others,

On Fri, Sep 03, 2021 at 11:41:25AM +0200, Hans de Goede wrote:
...
> >>> 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.
> 
> See above, note with the caveats from above noted I'm fine with the
> rename. OTOH I'm fine with keeping things as is too.
> 
> A better name might by PMIC_BYT_CHT_OPREGION or PMIC_INTEL_BYT_CHT_OPREGION
> though, since the TPS68470 OpRegion is also found only on devices where
> the main SoC is from Intel.

The same API could be used elsewhere, too. Nothing prevents that. Whether
or not that happens is another question.

> 
> >> What about the following?
> >>
> >> From: Jean Delvare <jdelvare@suse.de>
> >> 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 <jdelvare@suse.de>
> >> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> >> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> >> 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.
> 
> This is used on devices which use the PMICs which use the ACPI OpRegion
> handler defined in the shared intel_pmic.c code, so changing the check
> as part of renaming the Kconfig option is fine.

Sounds reasonable. And if there's a need to change the check it's easy to
do anyway.

So for the patch:

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

-- 
Kind regards,

Sakari Ailus