Re: [PATCH 1/2] iio: cros_ec: do an early exit if not physical_device case
On Sun, Mar 7, 2021 at 1:59 PM Jonathan Cameron wrote: > > On Tue, 2 Mar 2021 11:46:06 +0100 > Enric Balletbo i Serra wrote: > > > Hi all, > > > > On 21/2/21 17:29, Jonathan Cameron wrote: > > > On Mon, 23 Nov 2020 16:40:16 +0200 > > > Alexandru Ardelean wrote: > > > > > >> This whole code-block was put under one big if() condition/block. > > >> This change does an early return if the 'physical_device' boolean is > > >> false, > > >> thus unindenting the block by one level. > > >> > > >> No other functional change has been done. > > >> > > >> Signed-off-by: Alexandru Ardelean > > > @Gwendal, others This series from Alex has been outstanding for a while > > > but may well still apply. > > > Ideally looking for an ack. > > > > > > > This looks good to me. > > > > Acked-by: Enric Balletbo i Serra > > Hi Enric, Ack or both patches or just this one? > > @Alex, series needs a rebase. I'm not totally sure what's changed. > If you don't get to it I'll do it at somepoint but unlikely to > be terribly soon! It's likely that it's that iio_device_attach_buffer() went away and the block got replaced by devm_iio_kfifo_buffer_setup(). I'll re-spin it. I dropped this set from my main work branch because I didn't know if it was forgotten, and rebasing it with every change on devm_iio_kfifo_buffer_setup() and devm_iio_triggered_buffer_setup_ext() became annoying. But it shouldn't be hard to re-spin. > > Jonathan > > > > > Thanks, > > Enric > > > > > Thanks, > > > > > > Jonathan > > > > > >> --- > > >> .../cros_ec_sensors/cros_ec_sensors_core.c| 161 +- > > >> 1 file changed, 81 insertions(+), 80 deletions(-) > > >> > > >> 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 5c6c4e6fec9b..9470014936f2 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 > > >> @@ -287,89 +287,90 @@ int cros_ec_sensors_core_init(struct > > >> platform_device *pdev, > > >> > > >>indio_dev->name = pdev->name; > > >> > > >> - if (physical_device) { > > >> - state->param.cmd = MOTIONSENSE_CMD_INFO; > > >> - state->param.info.sensor_num = sensor_platform->sensor_num; > > >> - ret = cros_ec_motion_send_host_cmd(state, 0); > > >> - if (ret) { > > >> - dev_warn(dev, "Can not access sensor info\n"); > > >> + if (!physical_device) > > >> + return 0; > > >> + > > >> + state->param.cmd = MOTIONSENSE_CMD_INFO; > > >> + state->param.info.sensor_num = sensor_platform->sensor_num; > > >> + ret = cros_ec_motion_send_host_cmd(state, 0); > > >> + if (ret) { > > >> + dev_warn(dev, "Can not access sensor info\n"); > > >> + return ret; > > >> + } > > >> + state->type = state->resp->info.type; > > >> + state->loc = state->resp->info.location; > > >> + > > >> + /* Set sign vector, only used for backward compatibility. */ > > >> + memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); > > >> + > > >> + for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) > > >> + state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; > > >> + > > >> + /* 0 is a correct value used to stop the device */ > > >> + if (state->msg->version < 3) { > > >> + get_default_min_max_freq(state->resp->info.type, > > >> + [1], > > >> + [2], > > >> + >fifo_max_event_count); > > >> + } else { > > >> + frequencies[1] = state->resp->info_3.min_frequency; > > >> + frequencies[2] = state->resp->info_3.max_frequency; > > >> + state->fifo_max_event_count = > > >> + state->resp->info_3.fifo_max_event_count; > > >> + } > > >> + for (i = 0; i < ARRAY_SIZE(frequencies); i++) { > > >> + state->frequencies[2 * i] = frequencies[i] / 1000; > > >> + state->frequencies[2 * i + 1] = > > >> + (frequencies[i] % 1000) * 1000; > > >> + } > > >> + > > >> + if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) { > > >> + /* > > >> + * Create a software buffer, feed by the EC FIFO. > > >> + * We can not use trigger here, as events are generated > > >> + * as soon as sample_frequency is set. > > >> + */ > > >> + struct iio_buffer *buffer; > > >> + > > >> + buffer = devm_iio_kfifo_allocate(dev); > > >> + if (!buffer) > > >> + return -ENOMEM; > > >> + > > >> + iio_device_attach_buffer(indio_dev, buffer); > > >> + indio_dev->modes = INDIO_BUFFER_SOFTWARE; > > >> + > > >> + ret = cros_ec_sensorhub_register_push_data( > > >> + sensor_hub, sensor_platform->sensor_num, > > >> + indio_dev, push_data); > > >> + if (ret) > > >>
Re: [PATCH 1/2] iio: cros_ec: do an early exit if not physical_device case
On Tue, 2 Mar 2021 11:46:06 +0100 Enric Balletbo i Serra wrote: > Hi all, > > On 21/2/21 17:29, Jonathan Cameron wrote: > > On Mon, 23 Nov 2020 16:40:16 +0200 > > Alexandru Ardelean wrote: > > > >> This whole code-block was put under one big if() condition/block. > >> This change does an early return if the 'physical_device' boolean is false, > >> thus unindenting the block by one level. > >> > >> No other functional change has been done. > >> > >> Signed-off-by: Alexandru Ardelean > > @Gwendal, others This series from Alex has been outstanding for a while > > but may well still apply. > > Ideally looking for an ack. > > > > This looks good to me. > > Acked-by: Enric Balletbo i Serra Hi Enric, Ack or both patches or just this one? @Alex, series needs a rebase. I'm not totally sure what's changed. If you don't get to it I'll do it at somepoint but unlikely to be terribly soon! Jonathan > > Thanks, > Enric > > > Thanks, > > > > Jonathan > > > >> --- > >> .../cros_ec_sensors/cros_ec_sensors_core.c| 161 +- > >> 1 file changed, 81 insertions(+), 80 deletions(-) > >> > >> 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 5c6c4e6fec9b..9470014936f2 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 > >> @@ -287,89 +287,90 @@ int cros_ec_sensors_core_init(struct platform_device > >> *pdev, > >> > >>indio_dev->name = pdev->name; > >> > >> - if (physical_device) { > >> - state->param.cmd = MOTIONSENSE_CMD_INFO; > >> - state->param.info.sensor_num = sensor_platform->sensor_num; > >> - ret = cros_ec_motion_send_host_cmd(state, 0); > >> - if (ret) { > >> - dev_warn(dev, "Can not access sensor info\n"); > >> + if (!physical_device) > >> + return 0; > >> + > >> + state->param.cmd = MOTIONSENSE_CMD_INFO; > >> + state->param.info.sensor_num = sensor_platform->sensor_num; > >> + ret = cros_ec_motion_send_host_cmd(state, 0); > >> + if (ret) { > >> + dev_warn(dev, "Can not access sensor info\n"); > >> + return ret; > >> + } > >> + state->type = state->resp->info.type; > >> + state->loc = state->resp->info.location; > >> + > >> + /* Set sign vector, only used for backward compatibility. */ > >> + memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); > >> + > >> + for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) > >> + state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; > >> + > >> + /* 0 is a correct value used to stop the device */ > >> + if (state->msg->version < 3) { > >> + get_default_min_max_freq(state->resp->info.type, > >> + [1], > >> + [2], > >> + >fifo_max_event_count); > >> + } else { > >> + frequencies[1] = state->resp->info_3.min_frequency; > >> + frequencies[2] = state->resp->info_3.max_frequency; > >> + state->fifo_max_event_count = > >> + state->resp->info_3.fifo_max_event_count; > >> + } > >> + for (i = 0; i < ARRAY_SIZE(frequencies); i++) { > >> + state->frequencies[2 * i] = frequencies[i] / 1000; > >> + state->frequencies[2 * i + 1] = > >> + (frequencies[i] % 1000) * 1000; > >> + } > >> + > >> + if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) { > >> + /* > >> + * Create a software buffer, feed by the EC FIFO. > >> + * We can not use trigger here, as events are generated > >> + * as soon as sample_frequency is set. > >> + */ > >> + struct iio_buffer *buffer; > >> + > >> + buffer = devm_iio_kfifo_allocate(dev); > >> + if (!buffer) > >> + return -ENOMEM; > >> + > >> + iio_device_attach_buffer(indio_dev, buffer); > >> + indio_dev->modes = INDIO_BUFFER_SOFTWARE; > >> + > >> + ret = cros_ec_sensorhub_register_push_data( > >> + sensor_hub, sensor_platform->sensor_num, > >> + indio_dev, push_data); > >> + if (ret) > >>return ret; > >> - } > >> - state->type = state->resp->info.type; > >> - state->loc = state->resp->info.location; > >> > >> - /* Set sign vector, only used for backward compatibility. */ > >> - memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); > >> + ret = devm_add_action_or_reset( > >> + dev, cros_ec_sensors_core_clean, pdev); > >> + if (ret) > >> + return ret; > >> > >> - for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) > >> - state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; > >> - > >> - /* 0
Re: [PATCH 1/2] iio: cros_ec: do an early exit if not physical_device case
Hi all, On 21/2/21 17:29, Jonathan Cameron wrote: > On Mon, 23 Nov 2020 16:40:16 +0200 > Alexandru Ardelean wrote: > >> This whole code-block was put under one big if() condition/block. >> This change does an early return if the 'physical_device' boolean is false, >> thus unindenting the block by one level. >> >> No other functional change has been done. >> >> Signed-off-by: Alexandru Ardelean > @Gwendal, others This series from Alex has been outstanding for a while > but may well still apply. > Ideally looking for an ack. > This looks good to me. Acked-by: Enric Balletbo i Serra Thanks, Enric > Thanks, > > Jonathan > >> --- >> .../cros_ec_sensors/cros_ec_sensors_core.c| 161 +- >> 1 file changed, 81 insertions(+), 80 deletions(-) >> >> 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 5c6c4e6fec9b..9470014936f2 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 >> @@ -287,89 +287,90 @@ int cros_ec_sensors_core_init(struct platform_device >> *pdev, >> >> indio_dev->name = pdev->name; >> >> -if (physical_device) { >> -state->param.cmd = MOTIONSENSE_CMD_INFO; >> -state->param.info.sensor_num = sensor_platform->sensor_num; >> -ret = cros_ec_motion_send_host_cmd(state, 0); >> -if (ret) { >> -dev_warn(dev, "Can not access sensor info\n"); >> +if (!physical_device) >> +return 0; >> + >> +state->param.cmd = MOTIONSENSE_CMD_INFO; >> +state->param.info.sensor_num = sensor_platform->sensor_num; >> +ret = cros_ec_motion_send_host_cmd(state, 0); >> +if (ret) { >> +dev_warn(dev, "Can not access sensor info\n"); >> +return ret; >> +} >> +state->type = state->resp->info.type; >> +state->loc = state->resp->info.location; >> + >> +/* Set sign vector, only used for backward compatibility. */ >> +memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); >> + >> +for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) >> +state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; >> + >> +/* 0 is a correct value used to stop the device */ >> +if (state->msg->version < 3) { >> +get_default_min_max_freq(state->resp->info.type, >> + [1], >> + [2], >> + >fifo_max_event_count); >> +} else { >> +frequencies[1] = state->resp->info_3.min_frequency; >> +frequencies[2] = state->resp->info_3.max_frequency; >> +state->fifo_max_event_count = >> +state->resp->info_3.fifo_max_event_count; >> +} >> +for (i = 0; i < ARRAY_SIZE(frequencies); i++) { >> +state->frequencies[2 * i] = frequencies[i] / 1000; >> +state->frequencies[2 * i + 1] = >> +(frequencies[i] % 1000) * 1000; >> +} >> + >> +if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) { >> +/* >> + * Create a software buffer, feed by the EC FIFO. >> + * We can not use trigger here, as events are generated >> + * as soon as sample_frequency is set. >> + */ >> +struct iio_buffer *buffer; >> + >> +buffer = devm_iio_kfifo_allocate(dev); >> +if (!buffer) >> +return -ENOMEM; >> + >> +iio_device_attach_buffer(indio_dev, buffer); >> +indio_dev->modes = INDIO_BUFFER_SOFTWARE; >> + >> +ret = cros_ec_sensorhub_register_push_data( >> +sensor_hub, sensor_platform->sensor_num, >> +indio_dev, push_data); >> +if (ret) >> return ret; >> -} >> -state->type = state->resp->info.type; >> -state->loc = state->resp->info.location; >> >> -/* Set sign vector, only used for backward compatibility. */ >> -memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); >> +ret = devm_add_action_or_reset( >> +dev, cros_ec_sensors_core_clean, pdev); >> +if (ret) >> +return ret; >> >> -for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) >> -state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; >> - >> -/* 0 is a correct value used to stop the device */ >> -if (state->msg->version < 3) { >> -get_default_min_max_freq(state->resp->info.type, >> - [1], >> - [2], >> - >fifo_max_event_count); >> -} else { >> -
Re: [PATCH 1/2] iio: cros_ec: do an early exit if not physical_device case
On Mon, 23 Nov 2020 16:40:16 +0200 Alexandru Ardelean wrote: > This whole code-block was put under one big if() condition/block. > This change does an early return if the 'physical_device' boolean is false, > thus unindenting the block by one level. > > No other functional change has been done. > > Signed-off-by: Alexandru Ardelean @Gwendal, others This series from Alex has been outstanding for a while but may well still apply. Ideally looking for an ack. Thanks, Jonathan > --- > .../cros_ec_sensors/cros_ec_sensors_core.c| 161 +- > 1 file changed, 81 insertions(+), 80 deletions(-) > > 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 5c6c4e6fec9b..9470014936f2 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 > @@ -287,89 +287,90 @@ int cros_ec_sensors_core_init(struct platform_device > *pdev, > > indio_dev->name = pdev->name; > > - if (physical_device) { > - state->param.cmd = MOTIONSENSE_CMD_INFO; > - state->param.info.sensor_num = sensor_platform->sensor_num; > - ret = cros_ec_motion_send_host_cmd(state, 0); > - if (ret) { > - dev_warn(dev, "Can not access sensor info\n"); > + if (!physical_device) > + return 0; > + > + state->param.cmd = MOTIONSENSE_CMD_INFO; > + state->param.info.sensor_num = sensor_platform->sensor_num; > + ret = cros_ec_motion_send_host_cmd(state, 0); > + if (ret) { > + dev_warn(dev, "Can not access sensor info\n"); > + return ret; > + } > + state->type = state->resp->info.type; > + state->loc = state->resp->info.location; > + > + /* Set sign vector, only used for backward compatibility. */ > + memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); > + > + for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) > + state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; > + > + /* 0 is a correct value used to stop the device */ > + if (state->msg->version < 3) { > + get_default_min_max_freq(state->resp->info.type, > + [1], > + [2], > + >fifo_max_event_count); > + } else { > + frequencies[1] = state->resp->info_3.min_frequency; > + frequencies[2] = state->resp->info_3.max_frequency; > + state->fifo_max_event_count = > + state->resp->info_3.fifo_max_event_count; > + } > + for (i = 0; i < ARRAY_SIZE(frequencies); i++) { > + state->frequencies[2 * i] = frequencies[i] / 1000; > + state->frequencies[2 * i + 1] = > + (frequencies[i] % 1000) * 1000; > + } > + > + if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) { > + /* > + * Create a software buffer, feed by the EC FIFO. > + * We can not use trigger here, as events are generated > + * as soon as sample_frequency is set. > + */ > + struct iio_buffer *buffer; > + > + buffer = devm_iio_kfifo_allocate(dev); > + if (!buffer) > + return -ENOMEM; > + > + iio_device_attach_buffer(indio_dev, buffer); > + indio_dev->modes = INDIO_BUFFER_SOFTWARE; > + > + ret = cros_ec_sensorhub_register_push_data( > + sensor_hub, sensor_platform->sensor_num, > + indio_dev, push_data); > + if (ret) > return ret; > - } > - state->type = state->resp->info.type; > - state->loc = state->resp->info.location; > > - /* Set sign vector, only used for backward compatibility. */ > - memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); > + ret = devm_add_action_or_reset( > + dev, cros_ec_sensors_core_clean, pdev); > + if (ret) > + return ret; > > - for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) > - state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; > - > - /* 0 is a correct value used to stop the device */ > - if (state->msg->version < 3) { > - get_default_min_max_freq(state->resp->info.type, > - [1], > - [2], > - >fifo_max_event_count); > - } else { > - frequencies[1] = state->resp->info_3.min_frequency; > - frequencies[2] = state->resp->info_3.max_frequency; > -
[PATCH 1/2] iio: cros_ec: do an early exit if not physical_device case
This whole code-block was put under one big if() condition/block. This change does an early return if the 'physical_device' boolean is false, thus unindenting the block by one level. No other functional change has been done. Signed-off-by: Alexandru Ardelean --- .../cros_ec_sensors/cros_ec_sensors_core.c| 161 +- 1 file changed, 81 insertions(+), 80 deletions(-) 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 5c6c4e6fec9b..9470014936f2 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 @@ -287,89 +287,90 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, indio_dev->name = pdev->name; - if (physical_device) { - state->param.cmd = MOTIONSENSE_CMD_INFO; - state->param.info.sensor_num = sensor_platform->sensor_num; - ret = cros_ec_motion_send_host_cmd(state, 0); - if (ret) { - dev_warn(dev, "Can not access sensor info\n"); + if (!physical_device) + return 0; + + state->param.cmd = MOTIONSENSE_CMD_INFO; + state->param.info.sensor_num = sensor_platform->sensor_num; + ret = cros_ec_motion_send_host_cmd(state, 0); + if (ret) { + dev_warn(dev, "Can not access sensor info\n"); + return ret; + } + state->type = state->resp->info.type; + state->loc = state->resp->info.location; + + /* Set sign vector, only used for backward compatibility. */ + memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); + + for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) + state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; + + /* 0 is a correct value used to stop the device */ + if (state->msg->version < 3) { + get_default_min_max_freq(state->resp->info.type, +[1], +[2], +>fifo_max_event_count); + } else { + frequencies[1] = state->resp->info_3.min_frequency; + frequencies[2] = state->resp->info_3.max_frequency; + state->fifo_max_event_count = + state->resp->info_3.fifo_max_event_count; + } + for (i = 0; i < ARRAY_SIZE(frequencies); i++) { + state->frequencies[2 * i] = frequencies[i] / 1000; + state->frequencies[2 * i + 1] = + (frequencies[i] % 1000) * 1000; + } + + if (cros_ec_check_features(ec, EC_FEATURE_MOTION_SENSE_FIFO)) { + /* +* Create a software buffer, feed by the EC FIFO. +* We can not use trigger here, as events are generated +* as soon as sample_frequency is set. +*/ + struct iio_buffer *buffer; + + buffer = devm_iio_kfifo_allocate(dev); + if (!buffer) + return -ENOMEM; + + iio_device_attach_buffer(indio_dev, buffer); + indio_dev->modes = INDIO_BUFFER_SOFTWARE; + + ret = cros_ec_sensorhub_register_push_data( + sensor_hub, sensor_platform->sensor_num, + indio_dev, push_data); + if (ret) return ret; - } - state->type = state->resp->info.type; - state->loc = state->resp->info.location; - /* Set sign vector, only used for backward compatibility. */ - memset(state->sign, 1, CROS_EC_SENSOR_MAX_AXIS); + ret = devm_add_action_or_reset( + dev, cros_ec_sensors_core_clean, pdev); + if (ret) + return ret; - for (i = CROS_EC_SENSOR_X; i < CROS_EC_SENSOR_MAX_AXIS; i++) - state->calib[i].scale = MOTION_SENSE_DEFAULT_SCALE; - - /* 0 is a correct value used to stop the device */ - if (state->msg->version < 3) { - get_default_min_max_freq(state->resp->info.type, -[1], -[2], ->fifo_max_event_count); - } else { - frequencies[1] = state->resp->info_3.min_frequency; - frequencies[2] = state->resp->info_3.max_frequency; - state->fifo_max_event_count = - state->resp->info_3.fifo_max_event_count; - } - for (i = 0; i < ARRAY_SIZE(frequencies); i++) { - state->frequencies[2 * i] = frequencies[i] / 1000; -