Re: [PATCH 2/2] drivers: iio: accel adxl312 and adxl314 support

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 16 Aug 2022 16:34:59 +0300
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 16/08/2022 15:44, Jonathan Cameron wrote:
> >>>  
> >>>  MODULE_DEVICE_TABLE(spi, adxl313_spi_id);
> >>>  
> >>>  static const struct of_device_id adxl313_of_match[] = {
> >>> +	{ .compatible = "adi,adxl312" },
> >>>  	{ .compatible = "adi,adxl313" },
> >>> +	{ .compatible = "adi,adxl314" },    
> >>
> >> You miss here driver data. I don't remember which driver matching takes
> >> precedence (especially in various cases like DT platforms with device
> >> instantiated by MFD), but for consistency I think both device id tables
> >> should have same driver data.  
> > 
> > You can set it up to try device_get_match_data() first then fallback
> > to the adxl313_spi_id[] table but there isn't a nice 'standard' way to
> > do it.
> > 
> > If that isn't done, then IIRC the match is against the compatible with
> > the vendor ID dropped and the table used is the spi_device_id one.
> > Which is just annoyingly complex and relies on the strings matching.
> > 
> > In the ideal world the spi_device_id table would go away but there are
> > still a few users (greybus - I think + remaining board files).
> > So for now something like
> > 
> > a = device_get_match_data(dev);
> > if (!a)
> > 	a = &adxl31x_spi_regmap_config[id->data];
> > 
> > Provides a good way of ensuring the id tables don't need to remain
> > in sync.
> >   
> 
> I guess the only minor issue is that first driver data - ADXL312 - is
> equal to 0, so above code would make consider ADXL312 as missing data.
Should have given a type to a.

struct adxl31x_chip_info *a;

It would be a pointer not an enum.  Though we might run into some problems
with that clang issue of whether array lookups are const (I've not really
gotten my head around that yet). 

Jonathan


> 
> Best regards,
> Krzysztof