Hi Andy,
Thanks for the review.
...
>
> > +#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.
Yes. Will fix all instances in next patch.
>
> ...
>
> > +#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
>
Will do.
> ...
>
> > +#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.
Will do.
>
> ...
>
> > +/**
> > + * 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?
I don't understand.
>
> > + */
>
> ...
>
> > + 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.
>
Will fix.
> ...
>
> > + 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.
Accepted.
>
> > + 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;
>
> ?
Sure. That looks good.
>
> Also the question, why mutex only against INFO_RAW case?
All other cases return static values and don't have register configuration involved.
>
> ...
>
> > + 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?
I agree. Will move them to smaller functions.
>
> ...
>
> > +/**
> > + * 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.
Will fix it.
>
> > + * 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.
>
Agreed.
> > + 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;
> > + }
> > + }
>
Will fix as many one liners as I can see in the code.
> ...
>
> > + dev_size = sizeof(*dev_channels) * num_channels;
>
> Here you need to have an array_size(). Or introduce a
> devm_krealloc_array().
Oh yes, you are right.
>
> > + dev_channels = devm_krealloc(dev, ams_channels, dev_size,
> GFP_KERNEL);
> > + if (!dev_channels)
> > + ret = -ENOMEM;
>
>
> --
> With Best Regards,
> Andy Shevchenko
>
Thanks,
Anand
