* TPS68470 PMIC config option
@ 2021-09-01 14:02 Jean Delvare
2021-09-01 17:39 ` Andy Shevchenko
0 siblings, 1 reply; 8+ messages in thread
From: Jean Delvare @ 2021-09-01 14:02 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Rafael J. Wysocki, Len Brown, linux-acpi
Hi Andy,
Is there a reason why config TPS68470_PMIC_OPREGION is not under "if
PMIC_OPREGION" where all other *_PMIC_OPREGION driver options are?
Thanks,
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-01 14:02 TPS68470 PMIC config option Jean Delvare
@ 2021-09-01 17:39 ` Andy Shevchenko
2021-09-01 19:32 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2021-09-01 17:39 UTC (permalink / raw)
To: Jean Delvare, Sakari Ailus; +Cc: Rafael J. Wysocki, Len Brown, linux-acpi
+Cc: Sakari.
On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:
> Hi Andy,
>
> 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?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-01 17:39 ` Andy Shevchenko
@ 2021-09-01 19:32 ` Sakari Ailus
2021-09-02 9:57 ` Jean Delvare
0 siblings, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2021-09-01 19:32 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jean Delvare, Rafael J. Wysocki, Len Brown, linux-acpi
Hi Andy, Jean,
On Wed, Sep 01, 2021 at 08:39:19PM +0300, Andy Shevchenko wrote:
> +Cc: Sakari.
>
> On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:
> > Hi Andy,
> >
> > 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.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-01 19:32 ` Sakari Ailus
@ 2021-09-02 9:57 ` Jean Delvare
2021-09-02 10:49 ` Andy Shevchenko
2021-09-02 12:15 ` Sakari Ailus
0 siblings, 2 replies; 8+ messages in thread
From: Jean Delvare @ 2021-09-02 9:57 UTC (permalink / raw)
To: Sakari Ailus
Cc: Andy Shevchenko, Rafael J. Wysocki, Len Brown, linux-acpi,
Mika Westerberg
Hi Sakari,
On Wed, 1 Sep 2021 22:32:51 +0300, Sakari Ailus wrote:
> On Wed, Sep 01, 2021 at 08:39:19PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:
> > > 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.
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");
#endif
return data + 15;
--
Jean Delvare
SUSE L3 Support
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-02 9:57 ` Jean Delvare
@ 2021-09-02 10:49 ` Andy Shevchenko
2021-09-02 12:15 ` Sakari Ailus
1 sibling, 0 replies; 8+ messages in thread
From: Andy Shevchenko @ 2021-09-02 10:49 UTC (permalink / raw)
To: Jean Delvare
Cc: Sakari Ailus, Rafael J. Wysocki, Len Brown, linux-acpi, Mika Westerberg
On Thu, Sep 02, 2021 at 11:57:31AM +0200, Jean Delvare wrote:
> On Wed, 1 Sep 2021 22:32:51 +0300, Sakari Ailus wrote:
> > On Wed, Sep 01, 2021 at 08:39:19PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:
> > > > 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.
>
> What about the following?
LGTM,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-02 9:57 ` Jean Delvare
2021-09-02 10:49 ` Andy Shevchenko
@ 2021-09-02 12:15 ` Sakari Ailus
2021-09-03 9:41 ` Hans de Goede
1 sibling, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2021-09-02 12:15 UTC (permalink / raw)
To: Jean Delvare
Cc: Andy Shevchenko, Rafael J. Wysocki, Len Brown, linux-acpi,
Mika Westerberg, Hans de Goede, Ville Syrjälä
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:
> > On Wed, Sep 01, 2021 at 08:39:19PM +0300, Andy Shevchenko wrote:
> > > On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:
> > > > 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.
>
> 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.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-02 12:15 ` Sakari Ailus
@ 2021-09-03 9:41 ` Hans de Goede
2021-09-07 22:28 ` Sakari Ailus
0 siblings, 1 reply; 8+ messages in thread
From: Hans de Goede @ 2021-09-03 9:41 UTC (permalink / raw)
To: Sakari Ailus, Jean Delvare
Cc: Andy Shevchenko, Rafael J. Wysocki, Len Brown, linux-acpi,
Mika Westerberg, Ville Syrjälä
Hi,
On 9/2/21 2:15 PM, Sakari Ailus wrote:
> 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:
>>> On Wed, Sep 01, 2021 at 08:39:19PM +0300, Andy Shevchenko wrote:
>>>> On Wed, Sep 01, 2021 at 04:02:34PM +0200, Jean Delvare wrote:
>>>>> 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
That is not correct, it implements an ACPI opregion handler where the
"API" is defined by Intel, but these are external PMICs which are made
by Intel, TI and X-Powers.
Note these are only used on hardware with Intel Bay Trail and Cherry Trail
SoCs (which are more or less Atom series SoCs).
>>> whereas the TPS68470 is an external chip. The two codebases are distinct.
The others are external chips too, where the TPS68470 is different is
that the ACPI OpRegion interface "API" is a different one (defined by Google
AFAIK) and is only used on ChromeBook devices, in combination with
various *other* Intel SoCs, mostly standard (Y-series) Core models,
but possibly also Apollo Lake / Gemini Lake (Atom like) SoCs.
>>>
>>> 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.
>> 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.
I hope this helps clarify things a bit, if not feel free to ask questions.
Regards,
Hans
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: TPS68470 PMIC config option
2021-09-03 9:41 ` Hans de Goede
@ 2021-09-07 22:28 ` Sakari Ailus
0 siblings, 0 replies; 8+ messages in thread
From: Sakari Ailus @ 2021-09-07 22:28 UTC (permalink / raw)
To: Hans de Goede
Cc: Jean Delvare, Andy Shevchenko, Rafael J. Wysocki, Len Brown,
linux-acpi, Mika Westerberg, Ville Syrjälä
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-09-07 22:28 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01 14:02 TPS68470 PMIC config option Jean Delvare
2021-09-01 17:39 ` Andy Shevchenko
2021-09-01 19:32 ` Sakari Ailus
2021-09-02 9:57 ` Jean Delvare
2021-09-02 10:49 ` Andy Shevchenko
2021-09-02 12:15 ` Sakari Ailus
2021-09-03 9:41 ` Hans de Goede
2021-09-07 22:28 ` Sakari Ailus
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.