by Fabien Lahoudere

[permalink] [raw]

Subject: Re: [PATCH v3 6/8] iio: common: cros_ec_sensors: support protocol v3 message

Hi Enric

I will split in two patches and send it soon.
Nevertheless, I won't change the structure initialisation because it
was requested on a previous comment.
I will fix other comments.

Thanks for reviewing

Le mardi 25 juin 2019 à 19:04 +0200, Enric Balletbo i Serra a écrit :
> Hi Fabien, Jonathan,
>
> cc'ing Gwendal and Enrico who might be interested on this patch.
>
> It'd be nice if we can land this patch before [1], otherwise the
> legacy support
> for cros-ec sensors on veyron minnie won't work and we will mess the
> kernel log
> with a couple of errors.
>
> I just have a few comments that I think should be quick to respin.
>
> [1] https://lkml.org/lkml/2019/6/24/1464
>
> On 22/6/19 12:15, Jonathan Cameron wrote:
> > On Tue, 18 Jun 2019 11:06:37 +0200
> > Fabien Lahoudere <[email protected]> wrote:
> >
> > > Version 3 of the EC protocol provides min and max frequencies for
> > > EC sensors.
> > >
>
> I think we are mixing two things. One is determine what version of
> the
> MOTIONSENSE command the EC has, and another one is add some default
> values
> supported by the third version. I'd split this in two separate
> patches, and fix
> the subject and the commit description.
>
>
> > > Signed-off-by: Fabien Lahoudere <[email protected]>
> > > Signed-off-by: Nick Vaccaro <[email protected]>
> > Looks good to me. I'll pick up next time if no one else raises any
> > issues on this one.
> >
> > Thanks,
> >
> > Jonathan
> >
> > > ---
> > > .../cros_ec_sensors/cros_ec_sensors_core.c | 85
> > > ++++++++++++++++++-
> > > .../linux/iio/common/cros_ec_sensors_core.h | 3 +
> > > 2 files changed, 87 insertions(+), 1 deletion(-)
> > >
> > > 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 57034e212fe1..2ce077b576a4 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
> > > @@ -26,6 +26,66 @@ static char *cros_ec_loc[] = {
> > > [MOTIONSENSE_LOC_MAX] = "unknown",
> > > };
> > >
> > > +static void get_default_min_max_freq(enum motionsensor_type
> > > type,
> > > + u32 *min_freq,
> > > + u32 *max_freq)
> > > +{
> > > + switch (type) {
> > > + case MOTIONSENSE_TYPE_ACCEL:
> > > + case MOTIONSENSE_TYPE_GYRO:
> > > + *min_freq = 12500;
> > > + *max_freq = 100000;
> > > + break;
> > > + case MOTIONSENSE_TYPE_MAG:
> > > + *min_freq = 5000;
> > > + *max_freq = 25000;
> > > + break;
> > > + case MOTIONSENSE_TYPE_PROX:
> > > + case MOTIONSENSE_TYPE_LIGHT:
> > > + *min_freq = 100;
> > > + *max_freq = 50000;
> > > + break;
> > > + case MOTIONSENSE_TYPE_BARO:
> > > + *min_freq = 250;
> > > + *max_freq = 20000;
> > > + break;
> > > + case MOTIONSENSE_TYPE_ACTIVITY:
> > > + default:
> > > + *min_freq = 0;
> > > + *max_freq = 0;
> > > + break;
> > > + }
> > > +}
>
> This is the second part. It adds default values for version 3. I'd
> send this
> part on the patch that adds support min/max freq.
>
> > > +
> > > +static int cros_ec_get_host_cmd_version_mask(struct
> > > cros_ec_device *ec_dev,
> > > + u16 cmd_offset, u16 cmd,
> > > u32 *mask)
> > > +{
> > > + int ret;
> > > + struct {
> > > + struct cros_ec_command msg;
> > > + union {
> > > + struct ec_params_get_cmd_versions params;
> > > + struct ec_response_get_cmd_versions resp;
> > > + };
> > > + } __packed buf = {
> > > + .msg = {
> > > + .command = EC_CMD_GET_CMD_VERSIONS +
> > > cmd_offset,
> > > + .insize = sizeof(struct
> > > ec_response_get_cmd_versions),
> > > + .outsize = sizeof(struct
> > > ec_params_get_cmd_versions)
> > > + },
> > > + .params = {.cmd = cmd}
> > > + };
> > > +
>
> nit: Actually when someone is sending a command to the EC there is a
> bit of mess
> how to do it, some use dynamic allocations, other static. IMO is more
> readable
> have something that explicitly initializes the struct and then
> assigns the
> different fields. Something like this:
>
>
> struct {
> struct cros_ec_command cmd;
> union {
> struct ec_params_get_cmd_versions params;
> struct ec_response_get_cmd_versions resp;
> };
> } __packed msg = {};
> int ret;
>
> msg.cmd.command = EC_CMD_GET_CMD_VERSIONS + cmd_offset;
> msg.cmd.insize = sizeof(msg.resp);
> msg.cmd.outsize = sizeof(msg.params);
> msg.params.cmd = cmd;
>
>
> > > + ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> > > + if (ret >= 0) {
> > > + if (buf.msg.result == EC_RES_SUCCESS)
>
> Note that cros_ec_cmd_xfer_status returns a <0 on error and 0 or
> positive number
> when EC_RES_SUCCESS. So no need to double check the result.
>
> > > + *mask = buf.resp.version_mask;
> > > + else
> > > + *mask = 0;
> > > + }
> > > + return ret;
>
> So, I think that all this can be reworked as
>
> ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
> if (ret < 0)
> return ret;
>
> *mask = msg.resp.version_mask;
>
> return 0;
>
>
> > > +}
> > > +
> > > int cros_ec_sensors_core_init(struct platform_device *pdev,
> > > int num_channels,
> > > bool physical_device)
> > > @@ -35,6 +95,8 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > > struct cros_ec_dev *ec = dev_get_drvdata(pdev->dev.parent);
> > > struct cros_ec_sensor_platform *sensor_platform =
> > > dev_get_platdata(dev);
> > > struct iio_dev *indio_dev;
> > > + u32 ver_mask;
> > > + int ret;
> > >
> > > if (num_channels > CROS_EC_SENSORS_CORE_MAX_CHANNELS)
> > > return -EINVAL;
> > > @@ -57,8 +119,16 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > >
> > > mutex_init(&state->cmd_lock);
> > >
> > > + /* determine what version of MOTIONSENSE CMD EC has */
>
> nit: Capitalize
>
> > > + ret = cros_ec_get_host_cmd_version_mask(state->ec,
> > > + ec->cmd_offset,
> > > + EC_CMD_MOTION_SENSE_CMD
> > > ,
> > > + &ver_mask);
>
> It will return <0 on error
>
> > > + if (ret < 0 || ver_mask == 0)
> > > + return -ENODEV;
> > > +
>
> so no need to check ver_mask
>
> if (ret < 0)
> return ret;
>
>
> > > /* Set up the host command structure. */
> > > - state->msg->version = 2;
> > > + state->msg->version = fls(ver_mask) - 1;
> > > state->msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
> > > state->msg->outsize = sizeof(struct ec_params_motion_sense);
> > >
> > > @@ -76,6 +146,19 @@ int cros_ec_sensors_core_init(struct
> > > platform_device *pdev,
> > > }
> > > state->type = state->resp->info.type;
> > > state->loc = state->resp->info.location;
> > > +
> > > + /* Value to stop the device */
> > > + state->frequency_range[0] = 0;
> > > + if (state->msg->version < 3) {
> > > + get_default_min_max_freq(state->resp-
> > > >info.type,
> > > + &state-
> > > >frequency_range[1],
> > > + &state-
> > > >frequency_range[2]);
> > > + } else {
> > > + state->frequency_range[1] =
> > > + state->resp->info_3.min_frequency;
> > > + state->frequency_range[2] =
> > > + state->resp->info_3.max_frequency;
> > > + }
>
> This is part of the second patch.
>
> > > }
> > >
> > > indio_dev->info = &state->info;
> > > diff --git a/include/linux/iio/common/cros_ec_sensors_core.h
> > > b/include/linux/iio/common/cros_ec_sensors_core.h
> > > index 3e6de427076e..89937ad242ef 100644
> > > --- a/include/linux/iio/common/cros_ec_sensors_core.h
> > > +++ b/include/linux/iio/common/cros_ec_sensors_core.h
> > > @@ -74,6 +74,9 @@ struct cros_ec_sensors_core_state {
> > > int curr_sampl_freq;
> > > struct iio_info info;
> > > struct iio_chan_spec
> > > channels[CROS_EC_SENSORS_CORE_MAX_CHANNELS];
> > > +
> > > + /* Disable, Min and Max Sampling Frequency in mHz */
> > > + int frequency_range[3];
> > > };
> > >
> > > /**
>
> As I said I'd send a first patch with the EC protocol bits separated
> of this
> patchset and create a second patch into this patchset with the
> min/max frequency
> bits.
>
> Thanks,
> ~ Enric

2019-07-09 09:45:11

by Fabien Lahoudere

[permalink] [raw]

Subject: Re: [PATCH v3 8/8] docs: iio: add precision about sampling_frequency_available

Le samedi 22 juin 2019 à 11:21 +0100, Jonathan Cameron a écrit :
> On Tue, 18 Jun 2019 11:06:39 +0200
> Fabien Lahoudere <[email protected]> wrote:
>
> > The documentation give some exemple on what format can be expected
> > from sampling_frequency_available sysfs attribute
> >
> > Signed-off-by: Fabien Lahoudere <[email protected]>
> It seems I already applied this one, though probably haven't sent
> a pull request for it to Greg yet.
>
> Please drop it from your v4 posting as otherwise I'll get confused
> (again).
>

Sorry, it will be dropped next time.

> Thanks,
>
> Jonathan
>
>
> > ---
> > Documentation/ABI/testing/sysfs-bus-iio | 7 +++++--
> > 1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-iio
> > b/Documentation/ABI/testing/sysfs-bus-iio
> > index 6aef7dbbde44..680451695422 100644
> > --- a/Documentation/ABI/testing/sysfs-bus-iio
> > +++ b/Documentation/ABI/testing/sysfs-bus-iio
> > @@ -61,8 +61,11 @@ What: /sys/bus/iio/devices/triggerX/s
> > ampling_frequency_available
> > KernelVersion: 2.6.35
> > Contact: [email protected]
> > Description:
> > - When the internal sampling clock can only take a small
> > - discrete set of values, this file lists those
> > available.
> > + When the internal sampling clock can only take a
> > specific set of
> > + frequencies, we can specify the available values with:
> > + - a small discrete set of values like "0 2 4 6 8"
> > + - a range with minimum, step and maximum frequencies
> > like
> > + "[min step max]"
> >
> > What: /sys/bus/iio/devices/iio:deviceX/oversampling_r
> > atio
> > KernelVersion: 2.6.38