Re: [PATCH v2 2/2] iio: adc: Add Nuvoton MA35D1 EADC driver

[Chi-Wen Weng]

Hi Andy,

Thank you for the detailed review.

> bitmap.h implies bitops.h...
>
>> +#include <linux/bitops.h>
>
> ...so you may drop this one.

Will do. I will drop linux/bitops.h. I will also drop
linux/mod_devicetable.h since platform_device.h is enough here.

> Please, make all register offsets fixed width, exempli gratia
>
> define MA35D1_EADC_STATUS2        0x0f8

Will fix all register offsets to use fixed-width hex values.

>> +#define MA35D1_EADC_INTSRC0_SPLIEN(n)    BIT(n)
>
> Useless? Can't BIT() be used directly?

The field selects which sample module raises ADINT0. This initial driver
only uses sample module 0, so I will replace this helper with a fixed
named bit for SPLIEN0.

>> +#define MA35D1_EADC_INTERNAL_VREF_MV    1600
>
> _mV (yes, as per SI).

Will rename the macro and field to use _mV.

>> +#define MA35D1_EADC_REF_STABLE_US    1000
>
> (1 * USEC_PER_MSEC) ?
> If go this way, include time.h for the multiplier definition.

Will change this to use USEC_PER_MSEC and include linux/time.h.

>> +static const struct regmap_config ma35d1_adc_regmap_config = {
>> +    .reg_bits = 32,
>> +    .val_bits = 32,
>> +    .reg_stride = 4,
>> +    .max_register = MA35D1_EADC_REFADJCTL,
>
> No cache?

No register cache is intended for this MMIO ADC block. I will make that
explicit by setting .cache_type = REGCACHE_NONE.

>> +static bool ma35d1_adc_valid_diff_pair(unsigned int vinp, unsigned 
int vinn)
>
> Wondering if this is just
>
>    return (vinp >= 0 && vinp < 4 && (vinn == vinp + 4));

Yes, the valid differential pairs are 0-4, 1-5, 2-6 and 3-7. Since vinp
is unsigned, I will simplify this to:

    return vinp < 4 && vinn == vinp + 4;

>> +static int ma35d1_adc_set_bits(...)
>> +static int ma35d1_adc_clear_bits(...)
>
> What's the point in these wrappers?

There is no strong reason to keep them. I will remove the wrappers and
use regmap_set_bits() / regmap_clear_bits() directly.

>> +static int ma35d1_adc_setup_reference(struct ma35d1_adc *adc)
>
> Add a short comment with reference to a datasheet table/section/et 
cetera.

Will add a short comment describing the REFADJCTL/PDREF and VREFSEL
settings according to the MA35D1 EADC reference control description.

> With
>
>    static regmap *map = adc->regmap;
>
> this becomes ...

Will use a local regmap pointer in functions that access several
registers. That should make the code shorter and easier to read.

>> +    ret = regmap_update_bits(adc->regmap, MA35D1_EADC_CTL,
>> +                 MA35D1_EADC_CTL_DIFFEN |
>> +                 MA35D1_EADC_CTL_DMOF, ctl);
>
> regmap_assign_bits()

Will use regmap_assign_bits() for the differential mode bits.

>> +static int ma35d1_adc_update_scan_mode(...)
>
> This is interesting check...

I will rework this to explicitly accept exactly one IIO_VOLTAGE scan
channel. This avoids depending on the last bit found in the scan mask.

>> +    guard(mutex)(&adc->lock);
>> +    if (adc->suspended || !adc->scan_chan)
>> +        goto done;
>
> This is usually leads to a mess in the compiler...

Will fix this by removing the goto from the trigger handler and using a
scoped_guard() block instead.

>> +    chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +                     BIT(IIO_CHAN_INFO_SCALE);
>
> Broken indentation...

Will fix the indentation.

>> +static struct iio_chan_spec
>> +ma35d1_adc_timestamp_channel(unsigned int scan_index)
>
> Useless wrapper. Just put _SOFT_TIMESTAMP() in place.

Will remove the helper and assign the timestamp channel directly using a
compound literal.

>> +    if (!num_channels)
>> +        return dev_err_probe(dev, -ENODATA,
>> +                     "no ADC channels configured\n");
>
> I would return -ENOENT...

Will change this to -ENOENT.

>> +    channels = devm_kcalloc(dev, num_channels + 1, 
sizeof(*channels), GFP_KERNEL);
>
> size_add() ?

Will use size_add(num_channels, 1).

>> +        differential = false;
>> +        vinn = 0;
>
> Make it an 'else' branch.

Will do.

>> +    adc->vref = devm_regulator_get_optional(dev, "vref");
>
> It can be unnested:

Will unnest the optional regulator handling in v3.

>> +    adc->vref_mv = ret / 1000;
>
> (MICRO / MILLI)
>
> instead of plain 1000.

Will fix this as well. The regulator API returns the voltage in
microvolts, while the driver stores the reference in millivolts for
IIO scale reporting. I will rename the field to vref_mV and use:

    adc->vref_mV = ret / (MICRO / MILLI);

with linux/units.h included, instead of using the plain 1000 constant.

Thanks again for the review. I will address these comments in the next
revision.

Best regards,
Chi-Wen