On Tue, Jun 30, 2020 at 8:12 AM Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx> wrote: > > Hi Gwendal, > > Thank you for the patch. > > On 30/6/20 9:59, Gwendal Grignou wrote: > > Minimal and maximal frequencies supported by a sensor is queried. > > On some older machines, these frequencies are not returned properly and > > the EC returns 0 instead. > > When returned maximal frequency is 0, ignore the information and use > > default frequencies instead. > > > > Signed-off-by: Gwendal Grignou <gwendal@xxxxxxxxxxxx> > > --- > > .../common/cros_ec_sensors/cros_ec_sensors_core.c | 15 ++++++++++++--- > > 1 file changed, 12 insertions(+), 3 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 36e3f20891f05..8437ff659260b 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 > > @@ -289,7 +289,7 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > struct cros_ec_sensorhub *sensor_hub = dev_get_drvdata(dev->parent); > > struct cros_ec_dev *ec = sensor_hub->ec; > > struct cros_ec_sensor_platform *sensor_platform = dev_get_platdata(dev); > > - u32 ver_mask; > > + u32 ver_mask, temp; > > int frequencies[ARRAY_SIZE(state->frequencies) / 2] = { 0 }; > > int ret, i; > > > > @@ -345,8 +345,17 @@ int cros_ec_sensors_core_init(struct platform_device *pdev, > > &frequencies[2], > > &state->fifo_max_event_count); > > } else { > > - frequencies[1] = state->resp->info_3.min_frequency; > > - frequencies[2] = state->resp->info_3.max_frequency; > > + if (state->resp->info_3.max_frequency == 0) { > > I might miss some use cases but I am wondering if this can be OR'ed with the > above if, so there is only one call to get_default_min_max_freq > > > + get_default_min_max_freq(state->resp->info.type, > > + &frequencies[1], > > + &frequencies[2], > > + &temp); > > and use &state->fifo_max_event_count instead of temp, so you don't need to > create a new variable that is not used anymore. Or is in purpose? It is on purpose: on some machines, min_frequency and max_frequency are both 0, but fifo_max_event_count is correct and should be used. > > > + } else { > > + frequencies[1] = > > + state->resp->info_3.min_frequency; > > nit: I think that you can take advantage of the new 100 character line length > limit, here. Fixed in the next patch. > > > + frequencies[2] = > > + state->resp->info_3.max_frequency; > > nit: ditto > > > + } > > state->fifo_max_event_count = > > state->resp->info_3.fifo_max_event_count; > > } > >