RE: [PATCH v8 2/4] iio: adc: Add Xilinx AMS driver

[Anand Ashok Dumbre]

Hi Andy,

Thank you for reviewing the patches.
 
> > > +static int ams_init_module(struct iio_dev *indio_dev, struct
> > > +fwnode_handle *np,
> 
> np --> fwnode

Will do. 

> 
> > > +                        struct iio_chan_spec *channels) {
> > > +     struct ams *ams = iio_priv(indio_dev);
> > > +     struct device *dev = indio_dev->dev.parent;
> 
> > > +     struct device_node *node;
> > > +     int num_channels = 0;
> > > +     int ret;
> > > +
> > > +     node = to_of_node(np);
> > > +     if (fwnode_property_match_string(np, "compatible",
> > > +                                      "xlnx,zynqmp-ams-ps") == 0) {
> > > +             ams->ps_base = of_iomap(node, 0);
> > > +             if (!ams->ps_base)
> > > +                     return -ENXIO;
> > > +             ret = devm_add_action_or_reset(dev, ams_iounmap, ams-
> >ps_base);
> > > +             if (ret < 0)
> > > +                     return ret;
> > > +
> > > +             /* add PS channels to iio device channels */
> > > +             memcpy(channels + num_channels, ams_ps_channels,
> > > +                    sizeof(ams_ps_channels));
> > > +             num_channels += ARRAY_SIZE(ams_ps_channels);
> > > +     } else if (fwnode_property_match_string(np, "compatible",
> > > +                                             "xlnx,zynqmp-ams-pl") == 0) {
> > > +             ams->pl_base = of_iomap(node, 0);
> >
> > Hmm. So of_iomap() leaves us dependent on dt specific calls. Whilst it
> > doesn't exactly look hard to create a generic version covering at
> > least dt and acpi I don' think there is an equivalent acpi function currently
> defined.
> >
> > Andy, do you think this is a good thing to add to the generic firmware
> > node handling?  It's a bit specialist as most of the time this will be
> > wrapped up in the platform device handling or similar rather than being in a
> child node like this.
> 
> I saw this issue previously somewhere else and we can do something about
> it.
> But first (before going to ACPI guts) we may indeed introduce a basic
> skeleton under fwnode API for this.
> 
> Something like
> 
> void __iomem *fwnode_iomap(...)
> {
>   if (is_of_node(fwnode))
>     return of_iomap();
>   return NULL;
> }
> EXPORT_SYMBOL_GPL(fwnode_iomap);
> 
> At least it will allow drivers to make them property provider agnostic.
> 
> Okay, I checked the current version of the ACPI specification and the
> proposed DTS here. With above API and something like
> 
> Device (AMS0) {
>   Name (_CRS, ...)
>   ...
>   Device (PLMN) {
>     Name (_CRS, ...)
>   }
>   Device (PSMN) {
>     Name (_CRS, ...)
>   }
> }
> 
> we may get the resource from the corresponding fwnode's _CRS object

Just to be sure, do I need to do anything for this in this iteration?

> 
> > > +             if (!ams->pl_base)
> > > +                     return -ENXIO;
> 
> ...
> 
> > > +     /* Initialize buffer for channel specification */
> > > +     ams_channels = kzalloc(sizeof(ams_ps_channels) +
> > > +                            sizeof(ams_pl_channels) +
> > > +                            sizeof(ams_ctrl_channels), GFP_KERNEL);
> 
> Make a use of something from overflow.h.

If my understanding is right, I need to use check_mul_overflow.

> 
> 
> > > +     if (!ams_channels)
> > > +             return -ENOMEM;
> 
> --
> With Best Regards,
> Andy Shevchenko

Thanks,
Anand