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

[Dimitri Fedrau <dima.fedrau@gmail.com>]

Hi Uwe,

Am Wed, Jul 16, 2025 at 08:39:14AM +0200 schrieb Uwe Kleine-König:
> 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.
> 
Will add it in V5.

> > -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.
>
Yes, you are right.

> > +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);
>
Thanks for finding this, will implement it in V5.

> > +
> >  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.
> 
Yes, will implement it as you suggested.

Thanks for your input.

Best regards,
Dimitri Fedrau