Re: [PATCH 2/2] iio: temperature: tmp117: add support for HIGH and LOW alerts

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Thomas,

The code overall looks fine but I have a couple of style improvement
proposals.

> +
> +	/*
> +	 * The threshold is passed in millidegrees Celsius as
> +	 * value = val + val2 / 1000000 (val2 holds the fractional part, with
> +	 * the sign taken from val). Convert it back with the same LSB ratio:
> +	 * raw = value / (TMP117_LIMIT_LSB_NUM / TMP117_LIMIT_LSB_DEN).
> +	 */
> +	tmp = (val < 0) ? (s64)val * 1000000 - val2
> +			: (s64)val * 1000000 + val2;

                        ^
Usually operators stay at the end of the previous line

> +	tmp = DIV_ROUND_CLOSEST(tmp * TMP117_LIMIT_LSB_DEN,
> +				(s64)TMP117_LIMIT_LSB_NUM * 1000000);
> +	tmp = clamp_t(s64, tmp, S16_MIN, S16_MAX);
> +
> +	return i2c_smbus_write_word_swapped(data->client,
> +					    reg, tmp);

Overall the wrapping looks too aggressive in several places

> +}

...

>  static int tmp117_probe(struct i2c_client *client)
> @@ -208,6 +343,16 @@ static int tmp117_probe(struct i2c_client *client)
>  	indio_dev->num_channels = match_data->num_channels;
>  	indio_dev->name = match_data->name;
>  
> +	if (client->irq) {

Since you're adding the binding for it, I'm surprised the irq already
exists?

> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, tmp117_interrupt_handler,
> +						IRQF_ONESHOT, match_data->name,
> +						indio_dev);
> +		if (ret) {
> +			dev_err_probe(&client->dev, ret, "irq request error\n");
> +			return ret;

Isn't dev_err_probe() return ret already? so return `dev_err_probe()`?

> +		}
> +	}
>  
>  	return devm_iio_device_register(&client->dev, indio_dev);
>  }

Thanks,
Miquèl