| 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
| ||
|
Message-ID: <20250716070616.GA5639@legfed1>
Date: Wed, 16 Jul 2025 09:06:16 +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 v4 1/2] pwm: mc33xs2410: add hwmon support
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, ®, 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
Powered by blists - more mailing lists