Re: [PATCH v2 2/3] iio: common: cros_ec_sensors: add sysfs attribute for frequencies

[Fabien Lahoudere <fabien.lahoudere@collabora.com>]

Le mardi 28 mai 2019 à 04:04 -0700, Gwendal Grignou a écrit :
> On Mon, May 27, 2019 at 2:55 AM Fabien Lahoudere
> <fabien.lahoudere@collabora.com> wrote:
> > Le dimanche 26 mai 2019 à 18:45 +0100, Jonathan Cameron a écrit :
> > > On Thu, 23 May 2019 11:07:36 +0200
> > > Fabien Lahoudere <fabien.lahoudere@collabora.com> wrote:
> > > 
> > > > In order to provide minimum and maximum frequencies for each
> > > > sensors,
> > > > we use a standard API (sampling_frequency_available) to provide
> > > > them
> > > > to userland.
> > > > As cros_ec_sensors_core_init do not manage default attrs, we
> > > > change
> > > > the signature to let all kind of sensors to provide "struct
> > > > iio_info"
> > > > with their callback. This change impact drivers using that
> > > > function.
> > > > 
> > > > Then cros_ec_* sensors provides frequencies range in sysfs like
> > > > this:
> > > > [min step max]
> > > > 
> > > > Signed-off-by: Fabien Lahoudere <fabien.lahoudere@collabora.com
> > > > >
> > > When I was pointing at the _available syntax I was meaning that
> > > the ideal is to implement this using the associated callbacks
> > > rather
> > > than as a custom sysfs attribute.
> > > 
> > 
> > Sorry, I misunderstood. Let me retry with that callback
> > implemented.
> > 
> > > > ---
> > > >  .../common/cros_ec_sensors/cros_ec_sensors.c  |  6 +--
> > > >  .../cros_ec_sensors/cros_ec_sensors_core.c    | 38
> > > > +++++++++++++++++++
> > > >  drivers/iio/light/cros_ec_light_prox.c        |  6 +--
> > > >  drivers/iio/pressure/cros_ec_baro.c           |  6 +--
> > > >  .../linux/iio/common/cros_ec_sensors_core.h   |  4 +-
> > > >  5 files changed, 50 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git
> > > > a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > index 17af4e0fd5f8..a0ecee15a6c8 100644
> > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors.c
> > > > @@ -172,7 +172,7 @@ static int cros_ec_sensors_write(struct
> > > > iio_dev
> > > > *indio_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > -static const struct iio_info ec_sensors_info = {
> > > > +static struct iio_info ec_sensors_info = {
> > > >     .read_raw = &cros_ec_sensors_read,
> > > >     .write_raw = &cros_ec_sensors_write,
> > > >  };
> > > > @@ -195,11 +195,11 @@ static int cros_ec_sensors_probe(struct
> > > > platform_device *pdev)
> > > >     if (!indio_dev)
> > > >             return -ENOMEM;
> > > > 
> > > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > > &ec_sensors_info,
> > > > +                                   true);
> > > >     if (ret)
> > > >             return ret;
> > > > 
> > > > -   indio_dev->info = &ec_sensors_info;
> > > >     state = iio_priv(indio_dev);
> > > >     for (channel = state->channels, i = CROS_EC_SENSOR_X;
> > > >          i < CROS_EC_SENSOR_MAX_AXIS; i++, channel++) {
> > > > diff --git
> > > > a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > > b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > > index ac53ea32c1b1..08fb5d3dc7b5 100644
> > > > --- a/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > > +++ b/drivers/iio/common/cros_ec_sensors/cros_ec_sensors_core.c
> > > > @@ -10,6 +10,7 @@
> > > >  #include <linux/iio/buffer.h>
> > > >  #include <linux/iio/common/cros_ec_sensors_core.h>
> > > >  #include <linux/iio/iio.h>
> > > > +#include <linux/iio/sysfs.h>
> > > >  #include <linux/iio/kfifo_buf.h>
> > > >  #include <linux/iio/trigger_consumer.h>
> > > >  #include <linux/kernel.h>
> > > > @@ -86,8 +87,42 @@ static int
> > > > cros_ec_get_host_cmd_version_mask(struct cros_ec_device
> > > > *ec_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > +/**
> > > > + * cros_ec_sensors_read_freq() - sysfs function to get
> > > > available
> > > > frequencies
> > > > + * @dev: Device structure for this device.
> > > > + * @attr: Description of the attribute.
> > > > + * @buf: Incoming string
> > > > + *
> > > > + * The later modes are only relevant to the ring buffer - and
> > > > depend on current
> > > > + * mode. Note that data sheet gives rather wide tolerances for
> > > > these so integer
> > > > + * division will give good enough answer and not all chips
> > > > have
> > > > them specified
> > > > + * at all.
> > > > + **/
> > > > +static ssize_t cros_ec_sensors_read_freq(struct device *dev,
> > > > +                                    struct device_attribute
> > > > *attr,
> > > > +                                    char *buf)
> > > > +{
> > > > +   struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > > > +   struct cros_ec_sensors_core_state *state =
> > > > iio_priv(indio_dev);
> > > > +
> > > > +   return snprintf(buf, PAGE_SIZE, "[%d 1 %d]\n", state-
> > > > >min_freq,
> > > > +                   state->max_freq);
> Step to 1 [Hz?] is not right; EC uses the frequency the sensors
> supports, rounded up by default.
> Given EC is hiding the sensors it controls, it is not straight-
> forward
> to find the supported frequencies between min_freq and max_freq.
> However, sensors mostly follow the following rules:
> - "fast" sensors: accelerometer, gyroscope, magnetometer,  barometer:
> Supported frequencies are: [ min_freq, max_freq >> n, max_freq >> (n
> -
> 1), ... max_freq >> 1, max_freq], where (min_freq >> n) > min_freq.
> frequency are expressed in mHz resolution.
> - "Slow" sensors: light, proximity:
> Any frequencies between min_freq and max_freq are supported.
> 
> Note that frequency 0 is accepted, it puts the given sensor in
> suspend mode.
> 
> Given that information, it would make sense to follow the existing
> macro IIO_DEV_ATTR_SAMP_FREQ_AVAIL and report an array of
> frequencies,
> like bmc150_magn_show_samp_freq_avail does.

Thanks Gwendal.
IIUC, it is wroing to give a range and better to give a set of value.
Where can i find all available values for each sensors?


> 
> Gwendal.
> 
> > > Whilst it is a bit more fiddly I would much prefer if this was
> > > done
> > > with
> > > the info_mask_shared_by_all_available bit mask in the iio_dev and
> > > providing
> > > the read_avail callback.
> > > 
> > > The original reason to introduce this form was as part of trying
> > > to
> > > (far too slowly) kill off as much hand defined ABI as possible.
> > > Ultimate aim is to make the IIO interface optional for cases
> > > where
> > > the channels are mostly being used by other consumer drivers
> > > rather
> > > than
> > > being directly consumed by userspace.  To do that we need all of
> > > these elements to be easily accessible from the consumer hooks.
> > > 
> > > 
> > > 
> > > > +}
> > > > +
> > > > +static
> > > > IIO_DEV_ATTR_SAMP_FREQ_AVAIL(cros_ec_sensors_read_freq);
> > > > +
> > > > +static struct attribute *cros_ec_sensors_attributes[] = {
> > > > +   &iio_dev_attr_sampling_frequency_available.dev_attr.attr,
> > > > +   NULL,
> > > > +};
> > > > +
> > > > +static const struct attribute_group
> > > > cros_ec_sensors_attribute_group = {
> > > > +   .attrs = cros_ec_sensors_attributes,
> > > > +};
> > > > +
> > > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > >                           struct iio_dev *indio_dev,
> > > > +                         struct iio_info *info,
> > > >                           bool physical_device)
> > > >  {
> > > >     struct device *dev = &pdev->dev;
> > > > @@ -149,6 +184,9 @@ int cros_ec_sensors_core_init(struct
> > > > platform_device *pdev,
> > > >             }
> > > >     }
> > > > 
> > > > +   info->attrs = &cros_ec_sensors_attribute_group;
> > > > +   indio_dev->info = info;
> > > > +
> > > >     return 0;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(cros_ec_sensors_core_init);
> > > > diff --git a/drivers/iio/light/cros_ec_light_prox.c
> > > > b/drivers/iio/light/cros_ec_light_prox.c
> > > > index 308ee6ff2e22..1772e339cf14 100644
> > > > --- a/drivers/iio/light/cros_ec_light_prox.c
> > > > +++ b/drivers/iio/light/cros_ec_light_prox.c
> > > > @@ -161,7 +161,7 @@ static int cros_ec_light_prox_write(struct
> > > > iio_dev *indio_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > -static const struct iio_info cros_ec_light_prox_info = {
> > > > +static struct iio_info cros_ec_light_prox_info = {
> > > >     .read_raw = &cros_ec_light_prox_read,
> > > >     .write_raw = &cros_ec_light_prox_write,
> > > >  };
> > > > @@ -184,11 +184,11 @@ static int
> > > > cros_ec_light_prox_probe(struct
> > > > platform_device *pdev)
> > > >     if (!indio_dev)
> > > >             return -ENOMEM;
> > > > 
> > > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > > +                                   &cros_ec_light_prox_info,
> > > > true);
> > > >     if (ret)
> > > >             return ret;
> > > > 
> > > > -   indio_dev->info = &cros_ec_light_prox_info;
> > > >     state = iio_priv(indio_dev);
> > > >     state->core.type = state->core.resp->info.type;
> > > >     state->core.loc = state->core.resp->info.location;
> > > > diff --git a/drivers/iio/pressure/cros_ec_baro.c
> > > > b/drivers/iio/pressure/cros_ec_baro.c
> > > > index 034ce98d6e97..cd3be0f16226 100644
> > > > --- a/drivers/iio/pressure/cros_ec_baro.c
> > > > +++ b/drivers/iio/pressure/cros_ec_baro.c
> > > > @@ -107,7 +107,7 @@ static int cros_ec_baro_write(struct
> > > > iio_dev
> > > > *indio_dev,
> > > >     return ret;
> > > >  }
> > > > 
> > > > -static const struct iio_info cros_ec_baro_info = {
> > > > +static struct iio_info cros_ec_baro_info = {
> > > >     .read_raw = &cros_ec_baro_read,
> > > >     .write_raw = &cros_ec_baro_write,
> > > >  };
> > > > @@ -130,11 +130,11 @@ static int cros_ec_baro_probe(struct
> > > > platform_device *pdev)
> > > >     if (!indio_dev)
> > > >             return -ENOMEM;
> > > > 
> > > > -   ret = cros_ec_sensors_core_init(pdev, indio_dev, true);
> > > > +   ret = cros_ec_sensors_core_init(pdev, indio_dev,
> > > > &cros_ec_baro_info,
> > > > +                                   true);
> > > >     if (ret)
> > > >             return ret;
> > > > 
> > > > -   indio_dev->info = &cros_ec_baro_info;
> > > >     state = iio_priv(indio_dev);
> > > >     state->core.type = state->core.resp->info.type;
> > > >     state->core.loc = state->core.resp->info.location;
> > > > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h
> > > > b/include/linux/iio/common/cros_ec_sensors_core.h
> > > > index 32fd08bbcf52..f170a72ac08d 100644
> > > > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > > > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > > > @@ -114,12 +114,14 @@ struct platform_device;
> > > >   * cros_ec_sensors_core_init() - basic initialization of the
> > > > core
> > > > structure
> > > >   * @pdev:          platform device created for the sensors
> > > >   * @indio_dev:             iio device structure of the device
> > > > + * @info:          iio info structure with read and write
> > > > callback
> > > >   * @physical_device:       true if the device refers to a
> > > > physical
> > > > device
> > > >   *
> > > >   * Return: 0 on success, -errno on failure.
> > > >   */
> > > >  int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > > -                         struct iio_dev *indio_dev, bool
> > > > physical_device);
> > > > +                         struct iio_dev *indio_dev, struct
> > > > iio_info *info,
> > > > +                         bool physical_device);
> > > > 
> > > >  /**
> > > >   * cros_ec_sensors_capture() - the trigger handler function