Messages in this thread | | | Date | Thu, 25 Nov 2021 14:14:23 +0200 | From | Andy Shevchenko <> | Subject | Re: [PATCH v11 3/5] iio: adc: Add Xilinx AMS driver |
| |
On Wed, Nov 24, 2021 at 10:54:05PM +0000, Anand Ashok Dumbre wrote: > The AMS includes an ADC as well as on-chip sensors that can be used to > sample external voltages and monitor on-die operating conditions, such > as temperature and supply voltage levels. The AMS has two SYSMON blocks. > PL-SYSMON block is capable of monitoring off chip voltage and > temperature. > > PL-SYSMON block has DRP, JTAG and I2C interface to enable monitoring > from an external master. Out of these interfaces currently only DRP is > supported. Other block PS-SYSMON is memory mapped to PS. > > The AMS can use internal channels to monitor voltage and temperature as > well as one primary and up to 16 auxiliary channels for measuring > external voltages. > > The voltage and temperature monitoring channels also have event capability > which allows to generate an interrupt when their value falls below or > raises above a set threshold.
...
> +#define AMS_IDR_1 0x02c ... > +#define AMS_VCC_PSPLL3 0x06C ... > +#define AMS_VCCBRAM 0x07C ... > +#define AMS_PSINTFPDDR 0x09C ...and so on
Be consistent with the capitalization in the hex values.
...
> +#define AMS_INIT_POLL_TIME 200
Does it need unit?
> +#define AMS_SUPPLY_SCALE_1VOLT 1000 > +#define AMS_SUPPLY_SCALE_3VOLT 3000 > +#define AMS_SUPPLY_SCALE_6VOLT 6000
I would rather make units with these:
#define AMS_SUPPLY_SCALE_1VOLT_mV 1000 #define AMS_SUPPLY_SCALE_3VOLT_mV 3000 #define AMS_SUPPLY_SCALE_6VOLT_mV 6000
...
> +#define AMS_PL_AUX_CHAN_VOLTAGE(_auxno) \
> + AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(_auxno)), \ > + AMS_REG_VAUX(_auxno), false)
One line?
> +#define AMS_CTRL_CHAN_VOLTAGE(_scan_index, _addr) \
> + AMS_CHAN_VOLTAGE(PL_SEQ(AMS_SEQ(AMS_SEQ(_scan_index))), \ > + _addr, false)
Ditto.
...
> +/** > + * struct ams - Driver data for xilinx-ams > + * @base: physical base address of device > + * @ps_base: physical base address of PS device > + * @pl_base: physical base address of PL device > + * @clk: clocks associated with the device > + * @dev: pointer to device struct > + * @lock: to handle multiple user interaction > + * @intr_lock: to protect interrupt mask values > + * @alarm_mask: alarm configuration > + * @current_masked_alarm: currently masked due to alarm > + * @intr_mask: interrupt configuration > + * @ams_unmask_work: re-enables event once the event condition disappears
> + * This structure contains necessary state for Sysmon driver to operate
Shouldn't be this "state for Sysmon driver to operate" a summary above?
> + */
...
> + u32 reg, value; > + u32 expect = AMS_PS_CSTS_PS_READY; > + int ret;
u32 expect = AMS_PS_CSTS_PS_READY; u32 reg, value; int ret;
...
> + u32 reg; > + u32 expect = AMS_ISR1_EOC_MASK; > + int ret;
Ditto.
...
> + ret = readl_poll_timeout(ams->base + AMS_ISR_1, reg, > + (reg & expect), AMS_INIT_POLL_TIME, AMS_INIT_TIMEOUT_US);
Something wrong with line lengths... There is enough space on previous line for one parameter.
> + if (ret) > + return ret;
...
> + case IIO_CHAN_INFO_RAW: > + mutex_lock(&ams->lock); > + if (chan->scan_index >= AMS_CTRL_SEQ_BASE) { > + ret = ams_read_vcc_reg(ams, chan->address, val); > + if (ret) {
> + mutex_unlock(&ams->lock); > + return ret;
Can it be goto out_unlock;
> + } > + ams_enable_channel_sequence(indio_dev); > + } else if (chan->scan_index >= AMS_PS_SEQ_MAX) > + *val = readl(ams->pl_base + chan->address); > + else > + *val = readl(ams->ps_base + chan->address);
ret = IIO_VAL_INT; out_unlock:
> + mutex_unlock(&ams->lock); > + > + return IIO_VAL_INT;
mutex_unlock(&ams->lock); return ret;
?
Also the question, why mutex only against INFO_RAW case?
...
> + switch (chan->type) { > + case IIO_VOLTAGE: > + if (chan->scan_index < AMS_PS_SEQ_MAX) { > + switch (chan->address) { > + case AMS_SUPPLY1: > + case AMS_SUPPLY2: > + case AMS_SUPPLY3: > + case AMS_SUPPLY4: > + case AMS_SUPPLY9: > + case AMS_SUPPLY10: > + case AMS_VCCAMS: > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + case AMS_SUPPLY5: > + case AMS_SUPPLY6: > + case AMS_SUPPLY7: > + case AMS_SUPPLY8: > + *val = AMS_SUPPLY_SCALE_6VOLT; > + break; > + default: > + *val = AMS_SUPPLY_SCALE_1VOLT; > + break; > + } > + } else if (chan->scan_index >= AMS_PS_SEQ_MAX && > + chan->scan_index < AMS_CTRL_SEQ_BASE) { > + switch (chan->address) { > + case AMS_SUPPLY1: > + case AMS_SUPPLY2: > + case AMS_SUPPLY3: > + case AMS_SUPPLY4: > + case AMS_SUPPLY5: > + case AMS_SUPPLY6: > + case AMS_VCCAMS: > + case AMS_VREFP: > + case AMS_VREFN: > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + case AMS_SUPPLY7: > + regval = readl(ams->pl_base + AMS_REG_CONFIG4); > + if (FIELD_GET(AMS_VUSER0_MASK, regval)) > + *val = AMS_SUPPLY_SCALE_6VOLT; > + else > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + case AMS_SUPPLY8: > + regval = readl(ams->pl_base + AMS_REG_CONFIG4); > + if (FIELD_GET(AMS_VUSER1_MASK, regval)) > + *val = AMS_SUPPLY_SCALE_6VOLT; > + else > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + case AMS_SUPPLY9: > + regval = readl(ams->pl_base + AMS_REG_CONFIG4); > + if (FIELD_GET(AMS_VUSER2_MASK, regval)) > + *val = AMS_SUPPLY_SCALE_6VOLT; > + else > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + case AMS_SUPPLY10: > + regval = readl(ams->pl_base + AMS_REG_CONFIG4); > + if (FIELD_GET(AMS_VUSER3_MASK, regval)) > + *val = AMS_SUPPLY_SCALE_6VOLT; > + else > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + case AMS_VP_VN: > + case AMS_REG_VAUX(0) ... AMS_REG_VAUX(15): > + *val = AMS_SUPPLY_SCALE_1VOLT; > + break; > + default: > + *val = AMS_SUPPLY_SCALE_1VOLT; > + break; > + } > + } else { > + switch (chan->address) { > + case AMS_VCC_PSPLL0: > + case AMS_VCC_PSPLL3: > + case AMS_VCCINT: > + case AMS_VCCBRAM: > + case AMS_VCCAUX: > + case AMS_PSDDRPLL: > + case AMS_PSINTFPDDR: > + *val = AMS_SUPPLY_SCALE_3VOLT; > + break; > + default: > + *val = AMS_SUPPLY_SCALE_1VOLT; > + break; > + } > + } > + *val2 = AMS_SUPPLY_SCALE_DIV_BIT; > + return IIO_VAL_FRACTIONAL_LOG2; > + case IIO_TEMP: > + *val = AMS_TEMP_SCALE; > + *val2 = AMS_TEMP_SCALE_DIV_BIT; > + return IIO_VAL_FRACTIONAL_LOG2; > + default: > + return -EINVAL; > + }
Isn't it a bit too looong for a single switch case?
...
> +/** > + * ams_unmask_worker - ams alarm interrupt unmask worker
> + * @work : work to be done
Be consistent with a style on how you describe parameters in the kernel doc.
> + * The ZynqMP threshold interrupts are level sensitive. Since we can't make the > + * threshold condition go way from within the interrupt handler, this means as > + * soon as a threshold condition is present we would enter the interrupt handler > + * again and again. To work around this we mask all active threshold interrupts > + * in the interrupt handler and start a timer. In this timer we poll the > + * interrupt status and only if the interrupt is inactive we unmask it again. > + */
...
> + fwnode_for_each_child_node(chan_node, child) { > + ret = fwnode_property_read_u32(child, "reg", ®); > + if (ret || reg > AMS_PL_MAX_EXT_CHANNEL + 30) > + continue; > + > + chan = &channels[num_channels]; > + ext_chan = reg + AMS_PL_MAX_FIXED_CHANNEL - 30; > + memcpy(chan, &ams_pl_channels[ext_chan], sizeof(*channels)); > + > + if (fwnode_property_read_bool(child, "xlnx,bipolar")) > + chan->scan_type.sign = 's';
Needless double spacing.
> + num_channels++; > + }
...
> + /* add PS channels to iio device channels */ > + memcpy(channels, ams_ps_channels, > + sizeof(ams_ps_channels));
One line.
...
> + /* Copy only first 10 fix channels */
Be consistent with one line comments (pay attention to the capitalization, compare to the above).
> + memcpy(channels, ams_pl_channels, > + AMS_PL_MAX_FIXED_CHANNEL * sizeof(*channels));
One line?
...
> + /* add AMS channels to iio device channels */ > + memcpy(channels, ams_ctrl_channels, > + sizeof(ams_ctrl_channels));
One line.
...
> + fwnode_for_each_child_node(fwnode, child) { > + if (fwnode_device_is_available(child)) {
> + ret = ams_init_module(indio_dev, child, > + ams_channels + num_channels);
One line?
> + if (ret < 0) { > + fwnode_handle_put(child); > + return ret; > + } > + > + num_channels += ret; > + } > + }
...
> + dev_size = sizeof(*dev_channels) * num_channels;
Here you need to have an array_size(). Or introduce a devm_krealloc_array().
> + dev_channels = devm_krealloc(dev, ams_channels, dev_size, GFP_KERNEL); > + if (!dev_channels) > + ret = -ENOMEM;
-- With Best Regards, Andy Shevchenko
| |