linux-kernel - Re: [PATCH 09/15] iio: adc: at91-sama5d2

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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VddM_4efeC_YpTya81DrSzdo2AQKJEQ4z21KiCT3gt0TQ@mail.gmail.com>
Date: Mon, 4 Aug 2025 15:19:55 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Varshini Rajendran <varshini.rajendran@...rochip.com>
Cc: eugen.hristev@...aro.org, jic23@...nel.org, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, nicolas.ferre@...rochip.com, 
	alexandre.belloni@...tlin.com, claudiu.beznea@...on.dev, srini@...nel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 09/15] iio: adc: at91-sama5d2_adc: adapt the driver for sama7d65

On Mon, Aug 4, 2025 at 12:03 PM Varshini Rajendran
<varshini.rajendran@...rochip.com> wrote:
>
> Add support to sama7d65 ADC. The differences are highlighted with the
> compatible. The init and parsing of the temperature sensor and
> calibration indexes are the main differences.

...

> +static int at91_sama7d65_adc_temp_sensor_init(struct at91_adc_state *st,
> +                                             struct device *dev);

Again, please try to avoid forward declarations. They make code harder
to maintain.

...

> +enum at91_sama7d65_adc_ts_clb_idx {
> +       AT91_SAMA7D65_ADC_TS_CLB_IDX_P1 = 2,
> +       AT91_SAMA7D65_ADC_TS_CLB_IDX_P4 = 1,
> +       AT91_SAMA7D65_ADC_TS_CLB_IDX_P6 = 4,
> +       AT91_SAMA7D65_ADC_TS_CLB_IDX_MAX = 10,

MAX and trailing comma are odd when they go together.

> +};

...

> +static const struct at91_adc_platform sama7d65_platform = {
> +       .layout = &sama7g5_layout,
> +       .adc_channels = &at91_sama7g5_adc_channels,
> +#define AT91_SAMA7D65_SINGLE_CHAN_CNT  16
> +#define AT91_SAMA7D65_DIFF_CHAN_CNT    8
> +#define AT91_SAMA7D65_TEMP_CHAN_CNT    1
> +       .nr_channels = AT91_SAMA7D65_SINGLE_CHAN_CNT +
> +                      AT91_SAMA7D65_DIFF_CHAN_CNT +
> +                      AT91_SAMA7D65_TEMP_CHAN_CNT,
> +#define AT91_SAMA7D65_MAX_CHAN_IDX     (AT91_SAMA7D65_SINGLE_CHAN_CNT + \
> +                                       AT91_SAMA7D65_DIFF_CHAN_CNT + \
> +                                       AT91_SAMA7D65_TEMP_CHAN_CNT)
> +       .max_channels = ARRAY_SIZE(at91_sama7g5_adc_channels),
> +       .max_index = AT91_SAMA7D65_MAX_CHAN_IDX,
> +#define AT91_SAMA7G5_HW_TRIG_CNT       3
> +       .hw_trig_cnt = AT91_SAMA7G5_HW_TRIG_CNT,
> +       .osr_mask = GENMASK(18, 16),
> +       .oversampling_avail = { 1, 4, 16, 64, 256, },
> +       .oversampling_avail_no = 5,
> +       .chan_realbits = 16,
> +       .temp_sensor = true,
> +       .temp_chan = AT91_SAMA7G5_ADC_TEMP_CHANNEL,
> +       .temp_init = at91_sama7d65_adc_temp_sensor_init,
> +};

It's harder to read the static assignment interleaved by the sparse
definitions. Can't you group definitions followed by the initialiser?

...

> +static int at91_sama7d65_adc_temp_sensor_init(struct at91_adc_state *st,
> +                                             struct device *dev)
> +{
> +       struct at91_adc_temp_sensor_clb *clb = &st->soc_info.temp_sensor_clb;
> +       struct nvmem_cell *temp_calib;
> +       u32 *buf = NULL;

What for this NULL initialiser?

> +       size_t len;
> +       int ret = 0;

This initialisation can be avoided (see below how).

> +       if (!st->soc_info.platform->temp_sensor)
> +               return 0;
> +
> +       /* Get the calibration data from NVMEM. */
> +       temp_calib = devm_nvmem_cell_get(dev, "temperature_calib");
> +       if (IS_ERR(temp_calib)) {
> +               ret = PTR_ERR(temp_calib);
> +               if (ret != -ENOENT)
> +                       dev_err(dev, "Failed to get temperature_calib cell!\n");
> +               return ret;
> +       }
> +
> +       buf = nvmem_cell_read(temp_calib, &len);
> +       if (IS_ERR(buf)) {
> +               dev_err(dev, "Failed to read calibration data!\n");
> +               return PTR_ERR(buf);
> +       }

...

> +       /*
> +        * We prepare here the conversion to milli from micro to avoid

Here we prepare the...

> +        * doing it on hotpath.
> +        */

...

> +free_buf:
> +       kfree(buf);
> +       return ret;

Can't use cleanup.h? I.e. __free().

-- 
With Best Regards,
Andy Shevchenko