[PATCH v1 1/2] mfd: intel_soc_pmic_chtwc: Use dev_err_probe()

[PATCH v1 1/2] mfd: intel_soc_pmic_chtwc: Use dev_err_probe()

[Andy Shevchenko]

Simplify the mux error path a bit by using dev_err_probe().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
index 4eab191e053a..1cbe2bcce530 100644
--- a/drivers/mfd/intel_soc_pmic_chtwc.c
+++ b/drivers/mfd/intel_soc_pmic_chtwc.c
@@ -179,18 +179,13 @@ static int cht_wc_probe(struct i2c_client *client)
 	int ret;
 
 	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_HRV", NULL, &hrv);
-	if (ACPI_FAILURE(status)) {
-		dev_err(dev, "Failed to get PMIC hardware revision\n");
-		return -ENODEV;
-	}
-	if (hrv != CHT_WC_HRV) {
-		dev_err(dev, "Invalid PMIC hardware revision: %llu\n", hrv);
-		return -ENODEV;
-	}
-	if (client->irq < 0) {
-		dev_err(dev, "Invalid IRQ\n");
-		return -EINVAL;
-	}
+	if (ACPI_FAILURE(status))
+		return dev_err_probe(dev, -ENODEV, "Failed to get PMIC hardware revision\n");
+	if (hrv != CHT_WC_HRV)
+		return dev_err_probe(dev, -ENODEV, "Invalid PMIC hardware revision: %llu\n", hrv);
+
+	if (client->irq < 0)
+		return dev_err_probe(dev, -EINVAL, "Invalid IRQ\n");
 
 	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
 	if (!pmic)
-- 
2.35.1

[PATCH v1 2/2] mfd: intel_soc_pmic_chtwc: Switch from __maybe_unused to pm_sleep_ptr() etc

[Andy Shevchenko]

Letting the compiler remove these functions when the kernel is built
without CONFIG_PM_SLEEP support is simpler and less heavier for builds
than the use of __maybe_unused attributes.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/mfd/intel_soc_pmic_chtwc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
index 1cbe2bcce530..9216f0d34206 100644
--- a/drivers/mfd/intel_soc_pmic_chtwc.c
+++ b/drivers/mfd/intel_soc_pmic_chtwc.c
@@ -222,7 +222,7 @@ static void cht_wc_shutdown(struct i2c_client *client)
 	disable_irq(pmic->irq);
 }
 
-static int __maybe_unused cht_wc_suspend(struct device *dev)
+static int cht_wc_suspend(struct device *dev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
@@ -231,7 +231,7 @@ static int __maybe_unused cht_wc_suspend(struct device *dev)
 	return 0;
 }
 
-static int __maybe_unused cht_wc_resume(struct device *dev)
+static int cht_wc_resume(struct device *dev)
 {
 	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
 
@@ -239,7 +239,7 @@ static int __maybe_unused cht_wc_resume(struct device *dev)
 
 	return 0;
 }
-static SIMPLE_DEV_PM_OPS(cht_wc_pm_ops, cht_wc_suspend, cht_wc_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(cht_wc_pm_ops, cht_wc_suspend, cht_wc_resume);
 
 static const struct i2c_device_id cht_wc_i2c_id[] = {
 	{ }
@@ -253,7 +253,7 @@ static const struct acpi_device_id cht_wc_acpi_ids[] = {
 static struct i2c_driver cht_wc_driver = {
 	.driver	= {
 		.name	= "CHT Whiskey Cove PMIC",
-		.pm     = &cht_wc_pm_ops,
+		.pm     = pm_sleep_ptr(&cht_wc_pm_ops),
 		.acpi_match_table = cht_wc_acpi_ids,
 	},
 	.probe_new = cht_wc_probe,
-- 
2.35.1

Re: [PATCH v1 1/2] mfd: intel_soc_pmic_chtwc: Use dev_err_probe()

[Hans de Goede]

Hi,

On 6/16/22 19:10, Andy Shevchenko wrote:
> Simplify the mux error path a bit by using dev_err_probe().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>  drivers/mfd/intel_soc_pmic_chtwc.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> index 4eab191e053a..1cbe2bcce530 100644
> --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> @@ -179,18 +179,13 @@ static int cht_wc_probe(struct i2c_client *client)
>  	int ret;
>  
>  	status = acpi_evaluate_integer(ACPI_HANDLE(dev), "_HRV", NULL, &hrv);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(dev, "Failed to get PMIC hardware revision\n");
> -		return -ENODEV;
> -	}
> -	if (hrv != CHT_WC_HRV) {
> -		dev_err(dev, "Invalid PMIC hardware revision: %llu\n", hrv);
> -		return -ENODEV;
> -	}
> -	if (client->irq < 0) {
> -		dev_err(dev, "Invalid IRQ\n");
> -		return -EINVAL;
> -	}
> +	if (ACPI_FAILURE(status))
> +		return dev_err_probe(dev, -ENODEV, "Failed to get PMIC hardware revision\n");
> +	if (hrv != CHT_WC_HRV)
> +		return dev_err_probe(dev, -ENODEV, "Invalid PMIC hardware revision: %llu\n", hrv);
> +
> +	if (client->irq < 0)
> +		return dev_err_probe(dev, -EINVAL, "Invalid IRQ\n");
>  
>  	pmic = devm_kzalloc(dev, sizeof(*pmic), GFP_KERNEL);
>  	if (!pmic)

Re: [PATCH v1 2/2] mfd: intel_soc_pmic_chtwc: Switch from __maybe_unused to pm_sleep_ptr() etc

[Hans de Goede]

Hi,

On 6/16/22 19:10, Andy Shevchenko wrote:
> Letting the compiler remove these functions when the kernel is built
> without CONFIG_PM_SLEEP support is simpler and less heavier for builds
> than the use of __maybe_unused attributes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I don't see how this helps, __maybe_unused only means the compiler should not
warn AFAIK it is still allowed to remove static globals which are unused
even of they are marked __maybe_unused ?

And DEFINE_SIMPLE_DEV_PM_OPS does not mark the dev_pm_ops as
__maybe_unused where as pm_sleep_ptr() will not reference the struct
when CONFIG_PM is not set.

But I guess that the new PTR_IF magic:

#define PTR_IF(cond, ptr)       ((cond) ? (ptr) : NULL)

Still counts as referencing the struct so we don't get
an unused warning and since cond is a const 0 value the compiler
can optimize things away ?

Regards,

Hans



> ---
>  drivers/mfd/intel_soc_pmic_chtwc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> index 1cbe2bcce530..9216f0d34206 100644
> --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> @@ -222,7 +222,7 @@ static void cht_wc_shutdown(struct i2c_client *client)
>  	disable_irq(pmic->irq);
>  }
>  
> -static int __maybe_unused cht_wc_suspend(struct device *dev)
> +static int cht_wc_suspend(struct device *dev)
>  {
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>  
> @@ -231,7 +231,7 @@ static int __maybe_unused cht_wc_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int __maybe_unused cht_wc_resume(struct device *dev)
> +static int cht_wc_resume(struct device *dev)
>  {
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>  
> @@ -239,7 +239,7 @@ static int __maybe_unused cht_wc_resume(struct device *dev)
>  
>  	return 0;
>  }
> -static SIMPLE_DEV_PM_OPS(cht_wc_pm_ops, cht_wc_suspend, cht_wc_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(cht_wc_pm_ops, cht_wc_suspend, cht_wc_resume);
>  
>  static const struct i2c_device_id cht_wc_i2c_id[] = {
>  	{ }
> @@ -253,7 +253,7 @@ static const struct acpi_device_id cht_wc_acpi_ids[] = {
>  static struct i2c_driver cht_wc_driver = {
>  	.driver	= {
>  		.name	= "CHT Whiskey Cove PMIC",
> -		.pm     = &cht_wc_pm_ops,
> +		.pm     = pm_sleep_ptr(&cht_wc_pm_ops),
>  		.acpi_match_table = cht_wc_acpi_ids,
>  	},
>  	.probe_new = cht_wc_probe,

Re: [PATCH v1 2/2] mfd: intel_soc_pmic_chtwc: Switch from __maybe_unused to pm_sleep_ptr() etc

[Hans de Goede]

Hi,

On 6/16/22 19:10, Andy Shevchenko wrote:
> Letting the compiler remove these functions when the kernel is built
> without CONFIG_PM_SLEEP support is simpler and less heavier for builds
> than the use of __maybe_unused attributes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I see that this basically converts the code to the new way of dealing
with pm_ops, so after a second look:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans





> ---
>  drivers/mfd/intel_soc_pmic_chtwc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mfd/intel_soc_pmic_chtwc.c b/drivers/mfd/intel_soc_pmic_chtwc.c
> index 1cbe2bcce530..9216f0d34206 100644
> --- a/drivers/mfd/intel_soc_pmic_chtwc.c
> +++ b/drivers/mfd/intel_soc_pmic_chtwc.c
> @@ -222,7 +222,7 @@ static void cht_wc_shutdown(struct i2c_client *client)
>  	disable_irq(pmic->irq);
>  }
>  
> -static int __maybe_unused cht_wc_suspend(struct device *dev)
> +static int cht_wc_suspend(struct device *dev)
>  {
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>  
> @@ -231,7 +231,7 @@ static int __maybe_unused cht_wc_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int __maybe_unused cht_wc_resume(struct device *dev)
> +static int cht_wc_resume(struct device *dev)
>  {
>  	struct intel_soc_pmic *pmic = dev_get_drvdata(dev);
>  
> @@ -239,7 +239,7 @@ static int __maybe_unused cht_wc_resume(struct device *dev)
>  
>  	return 0;
>  }
> -static SIMPLE_DEV_PM_OPS(cht_wc_pm_ops, cht_wc_suspend, cht_wc_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(cht_wc_pm_ops, cht_wc_suspend, cht_wc_resume);
>  
>  static const struct i2c_device_id cht_wc_i2c_id[] = {
>  	{ }
> @@ -253,7 +253,7 @@ static const struct acpi_device_id cht_wc_acpi_ids[] = {
>  static struct i2c_driver cht_wc_driver = {
>  	.driver	= {
>  		.name	= "CHT Whiskey Cove PMIC",
> -		.pm     = &cht_wc_pm_ops,
> +		.pm     = pm_sleep_ptr(&cht_wc_pm_ops),
>  		.acpi_match_table = cht_wc_acpi_ids,
>  	},
>  	.probe_new = cht_wc_probe,

Re: [PATCH v1 2/2] mfd: intel_soc_pmic_chtwc: Switch from __maybe_unused to pm_sleep_ptr() etc

[Andy Shevchenko]

On Mon, Jun 20, 2022 at 10:38:34AM +0200, Hans de Goede wrote:
> On 6/16/22 19:10, Andy Shevchenko wrote:
> > Letting the compiler remove these functions when the kernel is built
> > without CONFIG_PM_SLEEP support is simpler and less heavier for builds
> > than the use of __maybe_unused attributes.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I don't see how this helps, __maybe_unused only means the compiler should not
> warn AFAIK it is still allowed to remove static globals which are unused
> even of they are marked __maybe_unused ?

__maybe_unused will force compiler to compile and linker to drop the section.
The proposed approach saves resources on build, i.e. it's already compiler that
decides not to compile the code.

> And DEFINE_SIMPLE_DEV_PM_OPS does not mark the dev_pm_ops as
> __maybe_unused where as pm_sleep_ptr() will not reference the struct
> when CONFIG_PM is not set.
> 
> But I guess that the new PTR_IF magic:
> 
> #define PTR_IF(cond, ptr)       ((cond) ? (ptr) : NULL)
> 
> Still counts as referencing the struct so we don't get
> an unused warning and since cond is a const 0 value the compiler
> can optimize things away ?

See above, it's not olny about warning and compiler.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/2] mfd: intel_soc_pmic_chtwc: Switch from __maybe_unused to pm_sleep_ptr() etc

[Andy Shevchenko]

On Mon, Jun 20, 2022 at 10:48:01AM +0200, Hans de Goede wrote:
> On 6/16/22 19:10, Andy Shevchenko wrote:
> > Letting the compiler remove these functions when the kernel is built
> > without CONFIG_PM_SLEEP support is simpler and less heavier for builds
> > than the use of __maybe_unused attributes.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> I see that this basically converts the code to the new way of dealing
> with pm_ops, so after a second look:
> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/2] mfd: intel_soc_pmic_chtwc: Use dev_err_probe()

[Lee Jones]

On Thu, 16 Jun 2022, Andy Shevchenko wrote:

> Simplify the mux error path a bit by using dev_err_probe().
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/intel_soc_pmic_chtwc.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v1 2/2] mfd: intel_soc_pmic_chtwc: Switch from __maybe_unused to pm_sleep_ptr() etc

[Lee Jones]

On Thu, 16 Jun 2022, Andy Shevchenko wrote:

> Letting the compiler remove these functions when the kernel is built
> without CONFIG_PM_SLEEP support is simpler and less heavier for builds
> than the use of __maybe_unused attributes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/mfd/intel_soc_pmic_chtwc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Applied, thanks.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog