Re: [PATCH 2/4] iio: frequency: adf4377: add support for ADF4377

[Jonathan Cameron]

> > > > +static int adf4377_probe(struct spi_device *spi)
> > > > +{
> > > > +       struct iio_dev *indio_dev;
> > > > +       struct regmap *regmap;
> > > > +       struct adf4377_state *st;
> > > > +       int ret;
> > > > +
> > > > +       indio_dev = devm_iio_device_alloc(&spi->dev,
> > > > sizeof(*st));
> > > > +       if (!indio_dev)
> > > > +               return -ENOMEM;
> > > > +
> > > > +       regmap = devm_regmap_init_spi(spi,
> > > > &adf4377_regmap_config);
> > > > +       if (IS_ERR(regmap))
> > > > +               return PTR_ERR(regmap);
> > > > +
> > > > +       st = iio_priv(indio_dev);
> > > > +
> > > > +       indio_dev->info = &adf4377_info;
> > > > +       indio_dev->name = "adf4377";
> > > > +       indio_dev->channels = adf4377_channels;
> > > > +       indio_dev->num_channels = ARRAY_SIZE(adf4377_channels);
> > > > +
> > > > +       st->regmap = regmap;
> > > > +       st->spi = spi;
> > > > +       st->type = spi_get_device_id(spi)->driver_data;    
> > > 
> > > Hmm this is something that came up internally the other day. Are we
> > > guaranteed that this will always work? For OF I think it is but I'm
> > > not
> > > sure about ACPI? At first glance, it seems that it might be ok but
> > > I
> > > did not went too deep in the ACPI code.  
> > 
> > Better indeed to not assume it and indeed ACPI can't do this magic,
> > because
> > there isn't a match between the actual ACPI ID and the
> > spi_device_ids.
> > Not sure what it does with PRP0001 case (where it uses the
> > of_device_id table).  
> 
> Some comments/questions on the pattern:
> 
> > 
> > 
> >         st->type = device_get_match_id()->driver_data;  
> 
> Are we sure that device_get_match_id() (I guess you meant
> device_get_match_data()) cannot return NULL? I'm always a bit on the
> careful side with these dereferences...

Oops. Yes I got confused.  It can indeed return
NULL and when using right function we won't dereference it.
We'll just check st->type which will have been set NULL if that fails.

Jonathan
 
> 
> >         if (!st->type) {
> >                 const struct spi_device_id *id =
> > spi_get_device_id(spi);
> > 
> >                 if (!id)
> >                         return -EINVAL;
> > 
> >                 st->type = spi_get_device_id(spi)->driver_data;  
> 
> id->driver_data :)
> 
> >         }  
> 
> - Nuno Sá
> 
> 
> 
> 
> device_get_match_id()->driver_data;
> 
>