Hi Andy,
Thanks for the review.
> -----Original Message-----
> From: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> Sent: Tuesday 16 November 2021 5:39 PM
> To: Anand Ashok Dumbre <ANANDASH@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; jic23@xxxxxxxxxx; lars@xxxxxxxxxx; linux-
> iio@xxxxxxxxxxxxxxx; git <git@xxxxxxxxxx>; Michal Simek
> <michals@xxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; rafael@xxxxxxxxxx;
> linux-acpi@xxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx; Manish Narani
> <MNARANI@xxxxxxxxxx>
> Subject: Re: [PATCH v9 3/5] iio: adc: Add Xilinx AMS driver
>
> On Tue, Nov 16, 2021 at 03:08:40PM +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.
>
> Something with indentation / paragraph splitting went wrong.
>
> ...
>
> > +#define AMS_ALARM_THR_MIN 0x0000
> > +#define AMS_ALARM_THR_MAX 0xFFFF
>
> If this is limited by hardware register, I would rather use (BIT(16) - 1)
> notation. It will give immediately amount of bits used for the value.
>
So ~(BIT(16) - 1) for AMS_ALARM_THR_MIN
(BIT(16) - 1) for AMS_ALARM_THR_MAX
> ...
>
> > +#define AMS_REGCFG1_ALARM_MASK
> (AMS_CONF1_ALARM_2_TO_0_MASK | \
> > +
> AMS_CONF1_ALARM_6_TO_3_MASK | BIT(0))
>
> Better to write as
>
> #define AMS_REGCFG1_ALARM_MASK \
> (AMS_CONF1_ALARM_2_TO_0_MASK |
> AMS_CONF1_ALARM_6_TO_3_MASK | BIT(0))
>
Will do.
> ...
>
> > +#define AMS_PL_CSTS_ACCESS_MASK 0x00000001U
>
> BIT()
>
Will fix.
> ...
>
> > + u32 reg;
> > + int ret;
>
> u32 expect = AMS_PS_CSTS_PS_READY;
>
> (Use similar approach for other readX_poll_timeout() cases)
>
> > + ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> > + (reg & AMS_PS_CSTS_PS_READY) ==
> > + AMS_PS_CSTS_PS_READY, 0,
> > + AMS_INIT_TIMEOUT_US);
>
> ret = readl_poll_timeout(ams->base + AMS_PS_CSTS, reg,
> (reg & expect) == expect,
> 0, AMS_INIT_TIMEOUT_US);
>
> 0?!
>
>
> > + if (ret)
> > + return ret;
>
> ...
>
> > + ret = readl(ams->base + AMS_PL_CSTS);
> > + if (ret == 0)
> > + return ret;
>
> Assigning u32 to int seems wrong.
It's a single bit register.
Even if I use u32 here, the return type is int.
So, is it ok if I read using u32 and return it by typecasting to int?
>
> ...
>
> > +static int ams_enable_single_channel(struct ams *ams, unsigned int
> > +offset) {
> > + u8 channel_num = 0;
>
> Assignment does not bring any value.
Agreed.
>
> > + switch (offset) {
> > + case AMS_VCC_PSPLL0:
> > + channel_num = AMS_VCC_PSPLL0_CH;
> > + break;
> > + case AMS_VCC_PSPLL3:
> > + channel_num = AMS_VCC_PSPLL3_CH;
> > + break;
> > + case AMS_VCCINT:
> > + channel_num = AMS_VCCINT_CH;
> > + break;
> > + case AMS_VCCBRAM:
> > + channel_num = AMS_VCCBRAM_CH;
> > + break;
> > + case AMS_VCCAUX:
> > + channel_num = AMS_VCCAUX_CH;
> > + break;
> > + case AMS_PSDDRPLL:
> > + channel_num = AMS_PSDDRPLL_CH;
> > + break;
> > + case AMS_PSINTFPDDR:
> > + channel_num = AMS_PSINTFPDDR_CH;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + /* set single channel, sequencer off mode */
> > + ams_ps_update_reg(ams, AMS_REG_CONFIG1,
> AMS_CONF1_SEQ_MASK,
> > + AMS_CONF1_SEQ_SINGLE_CHANNEL);
> > +
> > + /* write the channel number */
> > + ams_ps_update_reg(ams, AMS_REG_CONFIG0,
> AMS_CONF0_CHANNEL_NUM_MASK,
> > + channel_num);
> > +
> > + return 0;
> > +}
>
> ...
>
> > + regval = readl(ams->pl_base +
> > + AMS_REG_CONFIG4);
>
> One line?
>
> > + regval = readl(ams->pl_base +
> > + AMS_REG_CONFIG4);
>
> Ditto and so on...
>
It goes over 80 chars per line.
> ...
>
> > +static int ams_get_alarm_mask(int scan_index) {
> > + int bit = 0;
> > +
> > + if (scan_index >= AMS_PS_SEQ_MAX) {
> > + bit = AMS_PL_ALARM_START;
> > + scan_index -= AMS_PS_SEQ_MAX;
> > + }
> > +
> > + switch (scan_index) {
> > + case AMS_SEQ_TEMP:
> > + return BIT(AMS_ALARM_BIT_TEMP + bit);
> > + case AMS_SEQ_SUPPLY1:
> > + return BIT(AMS_ALARM_BIT_SUPPLY1 + bit);
> > + case AMS_SEQ_SUPPLY2:
> > + return BIT(AMS_ALARM_BIT_SUPPLY2 + bit);
> > + case AMS_SEQ_SUPPLY3:
> > + return BIT(AMS_ALARM_BIT_SUPPLY3 + bit);
> > + case AMS_SEQ_SUPPLY4:
> > + return BIT(AMS_ALARM_BIT_SUPPLY4 + bit);
> > + case AMS_SEQ_SUPPLY5:
> > + return BIT(AMS_ALARM_BIT_SUPPLY5 + bit);
> > + case AMS_SEQ_SUPPLY6:
> > + return BIT(AMS_ALARM_BIT_SUPPLY6 + bit);
> > + case AMS_SEQ_SUPPLY7:
> > + return BIT(AMS_ALARM_BIT_SUPPLY7 + bit);
> > + case AMS_SEQ_SUPPLY8:
> > + return BIT(AMS_ALARM_BIT_SUPPLY8 + bit);
> > + case AMS_SEQ_SUPPLY9:
> > + return BIT(AMS_ALARM_BIT_SUPPLY9 + bit);
> > + case AMS_SEQ_SUPPLY10:
> > + return BIT(AMS_ALARM_BIT_SUPPLY10 + bit);
> > + case AMS_SEQ_VCCAMS:
> > + return BIT(AMS_ALARM_BIT_VCCAMS + bit);
> > + case AMS_SEQ_TEMP_REMOTE:
> > + return BIT(AMS_ALARM_BIT_TEMP_REMOTE + bit);
> > + default:
> > + return 0;
> > + }
>
> > + return 0;
>
> Dead code.
Will remove return statement.
>
> > +}
>
> ...
>
> > + return (ams->alarm_mask & ams_get_alarm_mask(chan-
> >scan_index)) ? 1
> > +: 0;
>
> return !!(...);
>
> simply shorter.
Sure.
>
> ...
>
> > + schedule_delayed_work(&ams->ams_unmask_work,
> > + msecs_to_jiffies(AMS_UNMASK_TIMEOUT_MS));
>
> Can be one line.
Over 80 characters.
Oh! I just saw that upto 100 chars is ok.
>
> ...
>
> > + struct fwnode_handle *child_node = NULL,
>
> You may drop _node from the name.
>
> > + *fwnode = dev_fwnode(&pdev->dev);
>
> ...
>
> > + if (check_mul_overflow(num_chan, sizeof(struct iio_chan_spec),
> > + &ams_chan_size))
> > + return -EINVAL;
> > +
> > + /* Initialize buffer for channel specification */
> > + ams_channels = kzalloc(ams_chan_size, GFP_KERNEL);
>
> Simply use array_size(). Or why not kcalloc()?
>
> > + if (!ams_channels)
> > + return -ENOMEM;
>
> ...
>
> > + if (check_mul_overflow((size_t)num_channels, sizeof(struct
> iio_chan_spec),
> > + &dev_chan_size))
> > + return -EINVAL;
> > +
> > + dev_channels = devm_kzalloc(&pdev->dev, dev_chan_size,
> GFP_KERNEL);
>
> Why not devm_kcalloc()?
>
> > + if (!dev_channels) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
>
> ...
>
> > + ret = 0;
> > +err:
>
> Use better naming, you should describe what is going to be after goto.
Sure. Will do
>
> > + kfree(ams_channels);
> > +
> > + return ret;
>
> ...
>
> > + ret = devm_add_action_or_reset(&pdev->dev,
> ams_clk_disable_unprepare,
> > + ams->clk);
>
> One line?
>
> > + if (ret < 0)
> > + return ret;
>
> ...
>
> > + ret = ams_init_device(ams);
> > + if (ret) {
> > + dev_err(&pdev->dev, "failed to initialize AMS\n");
> > + return ret;
>
> It's fine to use dev_err_probe() for known error codes.
>
> > + }
>
> ...
>
> > + ret = devm_request_irq(&pdev->dev, irq, &ams_irq, 0, "ams-irq",
> > + indio_dev);
> > + if (ret < 0) {
> > + dev_err(&pdev->dev, "failed to register interrupt\n");
> > + return ret;
>
> Ditto.
>
> > + }
>
> --
> With Best Regards,
> Andy Shevchenko
>
Thanks,
Anand
