Re: [RFC PATCH] iio: adc: Add Xilinx AMS driver

[Jonathan Cameron]

On Sat, 17 Mar 2018 01:49:25 +0530
Himanshu Jha <himanshujha199640@xxxxxxxxx> wrote:

> On Thu, Mar 15, 2018 at 08:12:27PM +0530, Manish Narani 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
> > external master. Out of these interface 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.
> > 
> > Signed-off-by: Manish Narani <mnarani@xxxxxxxxxx>
> > ---  
> 
> [..]
> 
> > +static const struct of_device_id ams_of_match_table[] = {
> > +       { .compatible = "xlnx,zynqmp-ams", &ams_pl_apb },
> > +       { }
> > +};
> > +MODULE_DEVICE_TABLE(of, ams_of_match_table);  
> 
> [..]
> 
> > +static int ams_probe(struct platform_device *pdev)
> > +{
> > +       struct iio_dev *indio_dev;
> > +       struct ams *ams;
> > +       struct resource *res;
> > +       const struct of_device_id *id;
> > +       int ret;
> > +
> > +       if (!pdev->dev.of_node)
> > +               return -ENODEV;
> > +
> > +       id = of_match_node(ams_of_match_table, pdev->dev.of_node);
> > +       if (!id)
> > +               return -ENODEV;  
> 
> Is the above check required ?
> 
> Isn't the probe function called if and only if a match is found in
> ams_of_match_table[] since it is a pure OF driver ?
> 
> And therefore the above condition would never happen!
Agreed in principle.  However, from a reviewer point of view, it is sometimes
easier to have an error check that can never happen than have to check whether
or not it can.  Hence I'd keep this in place (well actually not because there
are better ways of doing this block of code, but that is unconnected to your
comment Himanshu!)

Was a good point to raise though and sometimes people add comments
saying "This can never fail but is here for completeness" or similar.
Not necessary but does avoid reviewers then asking why it was done.

Jonathan

> 
> > +static struct platform_driver ams_driver = {
> > +       .probe = ams_probe,
> > +       .remove = ams_remove,
> > +       .driver = {
> > +               .name = "ams",
> > +               .pm     = &ams_pm_ops,
> > +               .of_match_table = ams_of_match_table,
> > +       },
> > +};
> > +module_platform_driver(ams_driver);  
> 
>