On Sat, Dec 18, 2021 at 8:08 AM Andy Shevchenko <andy.shevchenko@xxxxxxxxx> wrote: > > On Sat, Dec 18, 2021 at 11:58 AM Gwendal Grignou <gwendal@xxxxxxxxxxxx> wrote: > > > > Semtech SAR sensor SX9324 is an evolution of the SX9310: > > It has 4 phases that can be configure to capture and process data > > from any of 3 CS pins and provide independent detection: > > proximity, table proximity or body proximity. > > > > Gather antenna data: > > > echo sx9324-dev3 > trigger/current_trigger > > echo 1 > scan_elements/in_proximity0_en > > echo 1 > buffer/enable > > od -v -An --endian=big -t d2 -w2 /dev/iio\:device3 > > Can you shift this right by two spaces? Done > > > (at 10Hz, the default). > > > > Trigger events: > > Setting: > > > thresh_falling_period: 2 (events) > > thresh_rising_period: 2 (events) > > in_proximity0_thresh_either_value: 300 > > in_proximity0_thresh_either_hysteresis: 72 > > Ditto. > > > using iio_event_monitor /dev/iio\:deviceX, approaching my hand to the > > antenna pad, I see: > > ... > > Event: time: 1634763907532035297, type: proximity, channel: 0, evtype: > > thresh, direction: falling > > Event: time: 1634763910138104640, type: proximity, channel: 0, evtype: > > thresh, direction: rising > > ... > > > + * Based on SX9324 driver and copy of datasheet at: > > + * https://edit.wpgdadawant.com/uploads/news_file/program/2019/30184/tech_files/program_30184_suggest_other_file.pdf > > Can you provide the link to the datasheet as Datasheet: tag? Done. > > ... > > > + for (i = 0; i < SX9324_NUM_PINS; i++) { > > + pin_idx = (val & SX9324_REG_AFE_PH0_PIN_MASK(i)) >> (2 * i); > > > > + len += scnprintf(buf + len, PAGE_SIZE - len, "%s,", > > Shouldn't this be sysfs_emit_at()? Done (and for other snprintf in sx9360 as well). > > > + sx9324_cs_pin_usage[pin_idx]); > > + } > > ... > > > + *val = 1 << FIELD_GET(SX9324_REG_PROX_CTRL0_GAIN_MASK, regval); > > Hmm... perhaps BIT()? I prefer to use '1 <<' explicitly, as val is a physical number, not a bit in a bitfield. > > ... > > > + switch (mask) { > > + case IIO_CHAN_INFO_HARDWAREGAIN: > > + *type = IIO_VAL_INT; > > + *length = ARRAY_SIZE(sx9324_gain_vals); > > + *vals = sx9324_gain_vals; > > + return IIO_AVAIL_LIST; > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + *type = IIO_VAL_INT_PLUS_MICRO; > > + *length = ARRAY_SIZE(sx9324_samp_freq_table) * 2; > > + *vals = (int *)sx9324_samp_freq_table; > > + return IIO_AVAIL_LIST; > > + } > > + > > + return -EINVAL; > > Make it the default case? Done. Fixed sx9360 as well. > > ... > > > + regval = FIELD_GET(SX9324_REG_PROX_CTRL5_FAR_DEBOUNCE_MASK, regval); > > + if (regval) > > + *val = 1 << regval; > > BIT() ? I prefer to use '1 <<' explicitly, as val is a physical number, not a bit in a bitfield. > > > + else > > + *val = 0; > > ... > > > + regval = FIELD_GET(SX9324_REG_PROX_CTRL5_CLOSE_DEBOUNCE_MASK, regval); > > + if (regval) > > + *val = 1 << regval; > > Ditto? > > > + else > > + *val = 0; > > ... > > > + switch (mask) { > > + case IIO_CHAN_INFO_SAMP_FREQ: > > + return sx9324_set_samp_freq(data, val, val2); > > + case IIO_CHAN_INFO_HARDWAREGAIN: > > + return sx9324_write_gain(data, chan, val); > > + } > > + > > + return -EINVAL; > > In some cases you have default in some not, can it be consistent? Done. > > ... > > > + dev_err(&data->client->dev, > > + "initial compensation timed out: 0x%02x\n", > > + val); > > Isn't it what user space will get anyway? Remove similar dev_err call in other drivers as well. > > ... > > > +static const struct acpi_device_id sx9324_acpi_match[] = { > > + { "STH9324", SX9324_WHOAMI_VALUE}, > > Missed whitespace? Done. Note `checkpatch.pl --strict` does not complain, and not having whitespace in tables is widespread: grep -re "[0-9A-Za-z]}" drivers/iio/ | wc -l 1239 > > > + { } > > +}; > > ... > > > +static const struct of_device_id sx9324_of_match[] = { > > + { .compatible = "semtech,sx9324", (void *)SX9324_WHOAMI_VALUE}, > > Ditto. > > > + { } > > +}; > > -- > With Best Regards, > Andy Shevchenko
