linux-kernel - Re: [PATCH v2] pwm: mc33xs2410: add support for temperature sensors

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250519124028.GA423953@legfed1>
Date: Mon, 19 May 2025 14:40:28 +0200
From: Dimitri Fedrau <dima.fedrau@...il.com>
To: Uwe Kleine-König <ukleinek@...nel.org>
Cc: dimitri.fedrau@...bherr.com, Jean Delvare <jdelvare@...e.com>,
	Guenter Roeck <linux@...ck-us.net>, linux-pwm@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2] pwm: mc33xs2410: add support for temperature sensors

Hi Uwe,

Am Fri, May 16, 2025 at 11:24:33AM +0200 schrieb Uwe Kleine-König:
> Hello Dimitri,
> 
> On Thu, May 15, 2025 at 02:40:54PM +0200, Dimitri Fedrau via B4 Relay wrote:
> > From: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
> > 
> > The MC33XS2410 provides temperature sensors for the central die temperature
> > and the four outputs. Additionally a common temperature warning threshold
> > can be configured for the outputs. Add hwmon support for the sensors.
> > 
> > Signed-off-by: Dimitri Fedrau <dimitri.fedrau@...bherr.com>
> > ---
> > Changes in v2:
> > - Remove helper mc33xs2410_hwmon_read_out_status and report the last
> >   latched status.
> > - Link to v1: https://lore.kernel.org/r/20250512-mc33xs2410-hwmon-v1-1-addba77c78f9@liebherr.com
> > ---
> 
> Mostly fine from my POV. I suggest to squash the following change into
> your patch:
> 
> diff --git a/drivers/pwm/Kconfig b/drivers/pwm/Kconfig
> index a0c077af9c98..d9bcd1e8413e 100644
> --- a/drivers/pwm/Kconfig
> +++ b/drivers/pwm/Kconfig
> @@ -425,7 +425,6 @@ config PWM_LPSS_PLATFORM
>  
>  config PWM_MC33XS2410
>  	tristate "MC33XS2410 PWM support"
> -	depends on HWMON || HWMON=n
>  	depends on OF
>  	depends on SPI
>  	help
> diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> index c1b99b114314..f5bba1a7bcc5 100644
> --- a/drivers/pwm/pwm-mc33xs2410.c
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -163,7 +163,6 @@ static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val
>  	return mc33xs2410_write_reg(spi, reg, tmp);
>  }
>  
> -#if IS_ENABLED(CONFIG_HWMON)
>  static const struct hwmon_channel_info * const mc33xs2410_hwmon_info[] = {
>  	HWMON_CHANNEL_INFO(temp,
>  			   HWMON_T_LABEL | HWMON_T_INPUT,
> @@ -286,21 +285,20 @@ static const struct hwmon_chip_info mc33xs2410_hwmon_chip_info = {
>  static int mc33xs2410_hwmon_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> -	struct device *hwmon;
>  
> -	hwmon = devm_hwmon_device_register_with_info(dev, NULL, spi,
> -						     &mc33xs2410_hwmon_chip_info,
> -						     NULL);
> +	if (IS_REACHABLE(CONFIG_HWMON)) {
> +		struct device *hwmon;
>  
> -	return PTR_ERR_OR_ZERO(hwmon);
> -}
> +		hwmon = devm_hwmon_device_register_with_info(dev, NULL, spi,
> +							     &mc33xs2410_hwmon_chip_info,
> +							     NULL);
>  
> -#else
> -static int mc33xs2410_hwmon_probe(struct spi_device *spi)
> -{
> -	return 0;
> +		return PTR_ERR_OR_ZERO(hwmon);
> +	} else {
> +		dev_dbg(dev, "Not registering hwmon sensors\n");
> +		return 0;
> +	}
>  }
> -#endif
>  
>  static u8 mc33xs2410_pwm_get_freq(u64 period)
>  {
> @@ -523,7 +521,11 @@ static int mc33xs2410_probe(struct spi_device *spi)
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
>  
> -	return mc33xs2410_hwmon_probe(spi);
> +	ret = mc33xs2410_hwmon_probe(spi);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register hwmon sensors\n");
> +
> +	return 0;
>  }
>  
>  static const struct spi_device_id mc33xs2410_spi_id[] = {

Perfering IS_REACHABLE over IS_ENABLED is fine for me. Is there a reason
why you just didn't replace IS_ENABLED with IS_REACHABLE ?

Best regards,
Dimitri Fedrau