On Sun, Jul 16, 2023 at 16:11 +0100 Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
[...]
>> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
>> +{
>> + u8 buf[2];
>
> __le16
>
>> + int ret;
>> +
>> + ret = regmap_bulk_read(data->regmap, IRS_REG_DATA_LO, buf,
>> + ARRAY_SIZE(buf));
>> + if (ret) {
>> + dev_err(data->dev, "Could not bulk read data (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + *val = (buf[1] << 8) | buf[0];
>
>
> If you make buf a __le16 then you can use aligned conversions otherwise
> get_unaligned_le16(buf)
>
>> +
>> + return 0;
>> +}
>> +
>
> ...
>
>> +
>> +static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
>> +{
>> + u8 buf[2];
>> + int ret;
>> +
>> + ret = regmap_bulk_read(data->regmap, IRS_REG_TIMER_LO, buf,
>> + ARRAY_SIZE(buf));
>> + if (ret) {
>> + dev_err(data->dev, "Could not bulk read timer (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = irsd200_read_data_rate(data, val2);
>> + if (ret)
>> + return ret;
>> +
>> + /* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
>> + *val = (buf[1] << 8) | buf[0];
>
> Another one where type should be __le16 and appropriate conversion functions
> used.
>
>> +
>> + return 0;
>> +}
>> +
>> +static int irsd200_write_timer(struct irsd200_data *data, int val, int val2)
>> +{
>> + unsigned int regval;
>> + int data_rate;
>> + u8 buf[2];
>
> __le16 might be more appropriate.
>
>> + int ret;
>> +
>> + if (val < 0 || val2 < 0)
>> + return -ERANGE;
>> +
>> + ret = irsd200_read_data_rate(data, &data_rate);
>> + if (ret)
>> + return ret;
>> +
>> + /* Quantize from seconds. */
>> + regval = val * data_rate + (val2 * data_rate) / 1000000;
>> +
>> + /* Value is 10 bits. */
>> + if (regval >= BIT(10))
>> + return -ERANGE;
>> +
>> + /* IRS_REG_TIMER_LO is the 8 LSBs and IRS_REG_TIMER_HI is the 2 MSBs. */
>> + buf[0] = FIELD_GET(GENMASK(7, 0), regval);
>> + buf[1] = FIELD_GET(GENMASK(9, 8), regval);
>
> I think I'd rather see this as appropriate put_unaligned_le16() or similar with
> the input masked.
I was choosing between these functions or manually doing things. I'll
update to use the endian conversions!
>> +
>> + ret = regmap_bulk_write(data->regmap, IRS_REG_TIMER_LO, buf,
>> + ARRAY_SIZE(buf));
>> + if (ret) {
>> + dev_err(data->dev, "Could not bulk write timer (%d)\n", ret);
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
> 0;
(The mystery continues! :) )
>> +static int irsd200_write_nr_count(struct irsd200_data *data, int val)
>> +{
>> + unsigned int regval;
>> + int ret;
>> +
>> + /* A value of zero means that IRS_REG_STATUS is never set. */
>> + if (val <= 0 || val >= BIT(3))
>
> BIT(3) is an unusual representation... Here, 8 is probably clearer unless
> it really has something to do with that bit being set.
Yeah, I agree.
[...]
>> +static int irsd200_write_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir, int state)
>> +{
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + unsigned int val;
>> + int ret;
>> +
>> + switch (type) {
>> + case IIO_EV_TYPE_THRESH:
>> + /* Clear the count register (by reading from it). */
>> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &val);
> I'd use a different variable than val, preferably named to indicate
> we don't care about the contents. Using val here and below is a little
> confusing.
I get that. Let's use `tmp`.
>
>> + if (ret)
>> + return ret;
>> +
>> + val = !!state;
>> + ret = regmap_field_write(
>> + data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
>> + if (ret)
>> + return ret;
>> +
>> + return val;
>
> Why return val? Should probably be 0 if this write succeeded.
Yes, will fix! (copy-pasting is dangerous...)
>
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
>> +{
>> + struct iio_dev *indio_dev = dev_id;
>> + struct irsd200_data *data = iio_priv(indio_dev);
>> + enum iio_event_direction dir;
>> + unsigned int lower_count;
>> + unsigned int upper_count;
>> + unsigned int status = 0;
>> + unsigned int source = 0;
>> + unsigned int clear = 0;
>> + unsigned int count = 0;
>> + int ret;
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
>> + if (ret) {
>> + dev_err(data->dev, "Could not read interrupt source (%d)\n",
>> + ret);
>> + return IRQ_NONE;
>
> As below. IRQ_NONE does not normally mean error, it means we are sure it is not
> our interrupt. Even in error cases, return IRQ_HANDLED.
>
> If you confirm via status bits that it isn't out interrupt, that's when you
> return IRQ_NONE.
Alright, let's do that then (and in `irsd200_trigger_handler()`).
>> + }
>> +
>> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
>> + if (ret) {
>> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
>> + ret);
>> + return IRQ_NONE;
>> + }
>> +
>> + if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
>> + iio_trigger_poll_nested(indio_dev->trig);
>> + clear |= BIT(IRS_INTR_DATA);
>> + }
>> +
>> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
>> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
>> + /*
>> + * The register value resets to zero after reading. We therefore
>> + * need to read once and manually extract the lower and upper
>> + * count register fields.
>> + */
>> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
>> + if (ret)
>> + dev_err(data->dev, "Could not read count (%d)\n", ret);
>> +
>> + upper_count = IRS_UPPER_COUNT(count);
>> + lower_count = IRS_LOWER_COUNT(count);
>> +
>> + /*
>> + * We only check the OR mode to be able to push events for
>> + * rising and falling thresholds. AND mode is covered when both
>> + * upper and lower count is non-zero, and is signaled with
>> + * IIO_EV_DIR_EITHER.
>> + */
>> + if (upper_count && !lower_count)
>> + dir = IIO_EV_DIR_RISING;
>> + else if (!upper_count && lower_count)
>> + dir = IIO_EV_DIR_FALLING;
>> + else
>> + dir = IIO_EV_DIR_EITHER;
>> +
>> + iio_push_event(indio_dev,
>> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
>> + IIO_EV_TYPE_THRESH, dir),
>> + iio_get_time_ns(indio_dev));
>> +
>> + /*
>> + * The OR mode will always trigger when the AND mode does, but
>> + * not vice versa. However, it seems like the AND bit needs to
>> + * be cleared if data capture _and_ threshold count interrupts
>> + * are desirable, even though it hasn't explicitly been selected
>> + * (with IRS_REG_INTR). Either way, it doesn't hurt...
>> + */
>> + clear |= BIT(IRS_INTR_COUNT_THR_OR) |
>> + BIT(IRS_INTR_COUNT_THR_AND);
>> + }
>> +
>> + if (clear) {
>
> if (!clear) then it's not our interrupt (I think) and you should return IRQ_NONE.
Yeah, this is probably the only place we can be sure it isn't our
interrupt.
[...]
>> +static int irsd200_probe(struct i2c_client *client)
>> +{
>> + struct iio_trigger *trigger;
>> + struct irsd200_data *data;
>> + struct iio_dev *indio_dev;
>> + size_t i;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return dev_err_probe(&client->dev, -ENOMEM,
>> + "Could not allocate iio device\n");
>> +
>> + data = iio_priv(indio_dev);
>> + data->dev = &client->dev;
>> +
>> + data->regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
>> + if (IS_ERR(data->regmap))
>> + return dev_err_probe(data->dev, PTR_ERR(data->regmap),
>> + "Could not initialize regmap\n");
>> +
>> + for (i = 0; i < IRS_REGF_MAX; ++i) {
>> + data->regfields[i] = devm_regmap_field_alloc(
>> + data->dev, data->regmap, irsd200_regfields[i]);
>> + if (IS_ERR(data->regfields[i]))
>> + return dev_err_probe(
>> + data->dev, PTR_ERR(data->regfields[i]),
>> + "Could not allocate register field %zu\n", i);
>> + }
>> +
>> + data->regulator = devm_regulator_get(data->dev, "vdd");
>
> devm_regulator_get_enable()
I suggested it, but didn't use it... (because I was testing with
hardware on an older tree where it was not present...). Sorry!
