linux-kernel - Re: [PATCH v4 1/2] pwm: mc33xs2410: add hwmon support

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <fxzkuflnasxp73fyf262wk5yx7yfnb5druegdujhzll3wjn6r5@n4xg6gs6segi>
Date: Wed, 16 Jul 2025 08:39:14 +0200
From: Uwe Kleine-König <ukleinek@...nel.org>
To: dimitri.fedrau@...bherr.com
Cc: 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, 
	Dimitri Fedrau <dima.fedrau@...il.com>
Subject: Re: [PATCH v4 1/2] pwm: mc33xs2410: add hwmon support

Hello Dimitri,

On Tue, Jul 08, 2025 at 06:13:03PM +0200, Dimitri Fedrau via B4 Relay wrote:
> diff --git a/drivers/pwm/pwm-mc33xs2410.c b/drivers/pwm/pwm-mc33xs2410.c
> index a1ac3445ccdb4709d92e0075d424a8abc1416eee..e70ed90bfdac77f5c777f0ba66d670331a515d12 100644
> --- a/drivers/pwm/pwm-mc33xs2410.c
> +++ b/drivers/pwm/pwm-mc33xs2410.c
> @@ -18,10 +18,12 @@
>   *   rather something in between.
>   */
>  
> +#include <linux/auxiliary_bus.h>
>  #include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/math64.h>
> +#include <linux/mc33xs2410.h>
>  #include <linux/minmax.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> @@ -120,12 +122,19 @@ static int mc33xs2410_read_reg(struct spi_device *spi, u8 reg, u16 *val, u8 flag
>  	return mc33xs2410_read_regs(spi, &reg, flag, val, 1);
>  }
>  
> -static int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val)
> +int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val)
>  {
>  	return mc33xs2410_read_reg(spi, reg, val, MC33XS2410_FRAME_IN_DATA_RD);
>  }
> +EXPORT_SYMBOL_NS_GPL(mc33xs2410_read_reg_ctrl, "PWM_MC33XS2410");

To reduce repetition (a bit) you can consider to define
DEFAULT_SYMBOL_NAMESPACE.

> -static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
> +int mc33xs2410_read_reg_diag(struct spi_device *spi, u8 reg, u16 *val)
> +{
> +	return mc33xs2410_read_reg(spi, reg, val, 0);
> +}
> +EXPORT_SYMBOL_NS_GPL(mc33xs2410_read_reg_diag, "PWM_MC33XS2410");
> +
> +int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val)
>  {
>  	u16 tmp;
>  	int ret;
> @@ -139,6 +148,7 @@ static int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val
>  
>  	return mc33xs2410_write_reg(spi, reg, tmp);
>  }
> +EXPORT_SYMBOL_NS_GPL(mc33xs2410_modify_reg, "PWM_MC33XS2410");
>  
>  static u8 mc33xs2410_pwm_get_freq(u64 period)
>  {
> @@ -297,6 +307,52 @@ static const struct pwm_ops mc33xs2410_pwm_ops = {
>  	.get_state = mc33xs2410_pwm_get_state,
>  };
>  
> +static void mc33xs2410_adev_release(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +
> +	kfree(adev);
> +}
> +
> +static void mc33xs2410_unregister_adev(void *_adev)
> +{
> +	struct auxiliary_device *adev = _adev;
> +
> +	auxiliary_device_delete(adev);
> +	auxiliary_device_uninit(adev);
> +}

This is a copy of auxiliary_device_destroy(). But see below.

> +static int mc33xs2410_hwmon_register(struct device *dev)
> +{
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	adev = kzalloc(sizeof(*adev), GFP_KERNEL);
> +	if (!adev)
> +		return -ENOMEM;
> +
> +	adev->name = "hwmon";
> +	adev->dev.parent = dev;
> +	adev->dev.release = mc33xs2410_adev_release;
> +	adev->id = 0;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (ret)
> +		return ret;
> +
> +	ret = auxiliary_device_add(adev);
> +	if (ret) {
> +		auxiliary_device_uninit(adev);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, mc33xs2410_unregister_adev, adev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

This function is equivalent to devm_auxiliary_device_create(dev, "hwmon", NULL);

> +
>  static int mc33xs2410_reset(struct device *dev)
>  {
>  	struct gpio_desc *reset_gpio;
> @@ -361,6 +417,10 @@ static int mc33xs2410_probe(struct spi_device *spi)
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "Failed to add pwm chip\n");
>  
> +	ret = mc33xs2410_hwmon_register(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to register hwmon device\n");
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/mc33xs2410.h b/include/linux/mc33xs2410.h
> new file mode 100644
> index 0000000000000000000000000000000000000000..15a0b0b595fe00a369cee45f2d30b2d912b612bb
> --- /dev/null
> +++ b/include/linux/mc33xs2410.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2024 Liebherr-Electronics and Drives GmbH
> + */
> +#ifndef _MC33XS2410_H
> +#define _MC33XS2410_H
> +
> +#include <linux/spi/spi.h>
> +
> +int mc33xs2410_read_reg_ctrl(struct spi_device *spi, u8 reg, u16 *val);
> +int mc33xs2410_read_reg_diag(struct spi_device *spi, u8 reg, u16 *val);
> +int mc33xs2410_modify_reg(struct spi_device *spi, u8 reg, u8 mask, u8 val);
> +
> +#endif /* _MC33XS2410_H */

I consider it elegant to have the

	MODULE_IMPORT_NS("PWM_MC33XS2410")

in the header. This is nice because the namespacing is completely
transparant to consumers and all they need it the right #include as if
there was no namespacing at all.

Best regards
Uwe

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)