Re: [PATCH 3/3] iio: imu: st_lsm6dsx: add support for rotation sensor

[Jonathan Cameron]

> 
> > > +       const struct st_lsm6dsx_reg *mux;
> > > +       int err;
> > > +
> > > +       mux = &hw->settings->sf_settings.page_mux;  
> > 
> > Just to check, can you use regmaps support for paging instead of doing
> > this by hand?
> > That would avoid future problems with enabling caching or similar  
> 
> No, the chip has multiple register sets with overlapping addresses, and
> struct regmap_range_cfg is not usable here.

Paged addressing always has overlapping addresses, so I'm not following.
The trick this does is to map those higher pages to a fake set of addresses.
An example is the ICM42600:
https://invensense.tdk.com/wp-content/uploads/2020/04/ds-000347_icm-42688-p-datasheet.pdf
for a suitable datasheet (some of the newer parts have a much more complex scheme)

I took a look at the datasheet and seems there are multiple types of paging
going on and effectively ends up with two nested paging controls. So fair enough it
doesn't fit here.

> 
> >   
> > > +
> > > +static
> > > IIO_DEV_ATTR_SAMP_FREQ_AVAIL(st_lsm6dsx_sf_sampling_freq_avail);
> > > +static struct attribute *st_lsm6dsx_sf_attributes[] = {
> > > +       &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > +       NULL,  
> > 
> > No comma as nothing should come after this.  Ideally would be replaced
> > with
> > code using the read_avail callback and appropriate bit set in the
> > info mask avail bitmaps.  
> 
> The read_avail callback would require available values to be in an int
> array, with one of the available IIO_VAL_* formats, but the driver uses a
> table (similarly to other parts of the existing code) where frequency
> values are expressed in mHz (which does not match any IIO_VAL_* format) and
> are interleaved with corresponding hardware register values. So we can't
> use the existing available frequency values in a read_avail callback.

You would need to do some data mangling to create the relevant table in order
to use that callback. We can always do that later I suppose if we find
that it matters for some inkernel consumer or other reason.

Jonathan

>