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

Uwe Kleine-König
2025-07-16T06:39:18Z

[-- Attachment #1: Type: text/plain, Size: 4733 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]