Re: [PATCH 09/15] iio: adc: at91-sama5d2_adc: adapt the driver for sama7d65

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Mon, Aug 4, 2025 at 12:03 PM Varshini Rajendran
<varshini.rajendran@microchip.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