| 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: <aO-16hC5r7-FB9Pw@smile.fi.intel.com>
Date: Wed, 15 Oct 2025 17:55:38 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: "Herve Codina (Schneider Electric)" <herve.codina@...tlin.com>
Cc: Wolfram Sang <wsa+renesas@...g-engineering.com>,
Jonathan Cameron <jic23@...nel.org>,
David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>,
Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>, linux-iio@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org,
Pascal Eberhard <pascal.eberhard@...com>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH 2/4] iio: adc: Add support for the Renesas RZ/N1 ADC
On Wed, Oct 15, 2025 at 04:28:14PM +0200, Herve Codina (Schneider Electric) wrote:
Thanks for the contribution, my comments below.
TL;DR: seems like somebody just submitted a driver without doing an internal
review with the kernel developers familiar with the upstream (and I believe
there are such in your company). This is not good. You, guys, should try your
best and not put a burden on the maintainer's/reviewers' shoulders.
> The Renesas RZ/N1 ADC controller is the ADC controller available in the
> Renesas RZ/N1 SoCs family. It can use up to two internal ACD cores (ADC1
> and ADC2) those internal cores are not directly accessed but are handled
> through ADC controller virtual channels.
...
> +#include <linux/kernel.h>
Definitely big no for this one.
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/interrupt.h>
> +#include <linux/mutex.h>
> +#include <linux/completion.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/machine.h>
> +#include <linux/iio/driver.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/bits.h>
> +#include <linux/of.h>
And simple "no" to this one.
> +#include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
In IIO we do
1) follow IWYU principle;
2) keep headers ordered (with possible grouping of iio/* ones or others if more
than one and specific to a domain);
...
> +#define RZN1_ADC_CONTROL_REG 0x2c
> +#define RZN1_ADC_CONTROL_ADC_BUSY BIT(6)
> +#define RZN1_ADC_FORCE_REG 0x30
> +#define RZN1_ADC_SET_FORCE_REG 0x34
> +#define RZN1_ADC_CLEAR_FORCE_REG 0x38
> +#define RZN1_ADC_FORCE_VC(_n) BIT(_n)
Please, make sure _REG definition values are on the column, same for bitfield
defs (some people like indent them slightly differently to the regs).
> +#define RZN1_ADC_CONFIG_REG 0x40
> +#define RZN1_ADC_CONFIG_ADC_POWER_DOWN BIT(3)
...
> +#define RZN1_ADC_VC_REG(_n) (0xc0 + 0x4 * (_n))
0x4 --> 4 as it's just a stride. Same for the rest of the similar cases.
...
> +struct rzn1_adc_core {
> + int is_used;
> + struct regulator *avdd;
> + struct regulator *vref;
Possible waste of 4 bytes on some architectures. Please, run `pahole` and see
the opportunities to improve all data structure types in the driver.
> +};
...
> +static int rzn1_adc_core_power_on(struct rzn1_adc_core *adc_core)
> +{
> + int ret;
> +
> + if (!adc_core->is_used)
> + return 0;
> +
> + ret = regulator_enable(adc_core->avdd);
> + if (ret)
> + return ret;
> +
> + ret = regulator_enable(adc_core->vref);
> + if (ret) {
> + regulator_disable(adc_core->avdd);
> + return ret;
> + }
Don't we have bulk API? Can't it be used here? Why?
> + return 0;
> +}
...
> +static int rzn1_adc_core_get_regulators(struct rzn1_adc *rzn1_adc,
> + struct rzn1_adc_core *adc_core,
> + const char *avdd_name, const char *vref_name)
> +{
> + struct device *dev = rzn1_adc->dev;
> + int ret;
> +
> + adc_core->avdd = devm_regulator_get_optional(dev, avdd_name);
ret = PTR_ERR_OR_ZERO(...);
might simplify the below.
> + if (IS_ERR(adc_core->avdd)) {
> + ret = PTR_ERR(adc_core->avdd);
> + if (ret != -ENODEV)
> + return dev_err_probe(dev, ret,
> + "Failed to get '%s' regulator\n",
> + avdd_name);
> + adc_core->avdd = NULL;
> + }
Like
if (ret == -ENODEV)
...
else if (ret)
return dev_err_probe(...);
> + adc_core->vref = devm_regulator_get_optional(dev, vref_name);
> + if (IS_ERR(adc_core->vref)) {
> + ret = PTR_ERR(adc_core->vref);
> + if (ret != -ENODEV)
> + return dev_err_probe(dev, ret,
> + "Failed to get '%s' regulator\n",
> + vref_name);
> + adc_core->vref = NULL;
> + }
Ditto. (However see above about bulk API)
I'm so much wondering when somebody provides a wrapper that _optional() for
regulator will be optional from device source code pattern so we won't see
anymore these checks for ENODEV (with chance > 0 of a mistake in some cases as
it's usually an unintuitive for use API if you need to test for the specific
error code).
> + /*
> + * Both regulators (avdd and vref) need to be available to have the
> + * related adc_core used.
> + */
> + adc_core->is_used = adc_core->vref && adc_core->avdd;
> + return 0;
> +}
...
> +static int rzn1_adc_core_get_vref_mv(struct rzn1_adc_core *adc_core)
> +{
> + int vref_uv;
_uV ?
> +
> + if (!adc_core->vref)
> + return -ENODEV;
> +
> + vref_uv = regulator_get_voltage(adc_core->vref);
> + if (vref_uv < 0)
> + return vref_uv;
> +
> + return vref_uv / 1000;
Hmm... (MICRO/MILLI) ?
> +}
...
> +static int rzn1_adc_power(struct rzn1_adc *rzn1_adc, bool power)
> +{
> + u32 v;
> +
> + writel(power ? 0 : RZN1_ADC_CONFIG_ADC_POWER_DOWN,
> + rzn1_adc->regs + RZN1_ADC_CONFIG_REG);
> +
> + /*
> + * Wait for the ADC_BUSY to clear.
> + * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
Useless comment. It might be something else in some cases, we don't need to
explain this in every caller.
> + */
> + return readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_CONTROL_REG,
> + v, !(v & RZN1_ADC_CONTROL_ADC_BUSY),
> + 0, 500);
> +}
...
> +static void rzn1_adc_vc_setup_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> + int adc1_ch, int adc2_ch)
> +{
> + u32 vc = 0;
> +
> + if (adc1_ch != -1)
The >= 0 is more robust in case the value gets an error pointer or -err code.
> + vc |= RZN1_ADC_VC_ADC1_ENABLE | RZN1_ADC_VC_ADC1_CHANNEL_SEL(adc1_ch);
> +
> + if (adc2_ch != -1)
> + vc |= RZN1_ADC_VC_ADC2_ENABLE | RZN1_ADC_VC_ADC2_CHANNEL_SEL(adc2_ch);
> +
> + writel(vc, rzn1_adc->regs + RZN1_ADC_VC_REG(ch));
> +}
> +static int rzn1_adc_vc_wait_conversion(struct rzn1_adc *rzn1_adc, u32 ch,
> + u32 *adc1_data, u32 *adc2_data)
> +{
> + u32 data_reg;
> + int ret;
> + u32 v;
> +
> + /*
> + * When a VC is selected, it needs 20 ADC clocks to perform the
> + * conversion.
> + *
> + * The worst case is when the 16 VCs need to perform a conversion and
> + * our VC is the lowest in term of priority.
> + *
> + * In that case, the conversion is performed in 16 * 20 ADC clocks.
> + *
> + * The ADC clock can be set from 4MHz to 20MHz. This leads to a worst
> + * case of 16 * 20 * 1/4Mhz = 80us.
> + *
> + * Round it up to 100us
Missing period.
> + */
> +
> + /*
> + * Wait for the ADC_FORCE_VC(n) to clear.
> + *
> + * On timeout, ret is -ETIMEDOUT, otherwise it will be 0.
> + */
> + ret = readl_poll_timeout_atomic(rzn1_adc->regs + RZN1_ADC_FORCE_REG,
> + v, !(v & RZN1_ADC_FORCE_VC(ch)),
> + 0, 100);
> + if (ret)
> + return ret;
> +
> + if (adc1_data) {
> + data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC1_DATA_REG(ch));
> + *adc1_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
> + }
> +
> + if (adc2_data) {
> + data_reg = readl(rzn1_adc->regs + RZN1_ADC_ADC2_DATA_REG(ch));
> + *adc2_data = RZN1_ADC_ADCX_GET_DATA(data_reg);
> + }
> +
> + return 0;
> +}
I almost stopped here, see TL;DR above why.
...
> +static const struct of_device_id rzn1_adc_of_match[] = {
> + { .compatible = "renesas,rzn1-adc" },
> + { /* sentinel */ },
No comments, no commas in terminator entry. This is a common style in IIO.
> +};
> +
Unneeded blank line.
> +MODULE_DEVICE_TABLE(of, rzn1_adc_of_match);
...
> +static struct platform_driver rzn1_adc_driver = {
> + .probe = rzn1_adc_probe,
> + .remove = rzn1_adc_remove,
> + .driver = {
> + .name = "rzn1-adc",
> + .of_match_table = of_match_ptr(rzn1_adc_of_match),
New code shouldn't use of_match_ptr().
> + .pm = pm_ptr(&rzn1_adc_pm_ops),
> + },
> +};
> +
Unneeded blank line.
> +module_platform_driver(rzn1_adc_driver);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists