[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7fde0674-c20a-4455-bb78-3a6521ae99ed@gmail.com>
Date: Tue, 25 Jun 2024 20:25:00 +0100
From: Mudit Sharma <muditsharma.info@...il.com>
To: Javier Carrasco <javier.carrasco.cruz@...il.com>, jic23@...nel.org,
lars@...afoo.de, krzk+dt@...nel.org, conor+dt@...nel.org, robh@...nel.org
Cc: ivan.orlov0322@...il.com, linux-kernel@...r.kernel.org,
linux-iio@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v5 2/2] iio: light: ROHM BH1745 colour sensor
On 24/06/2024 23:27, Javier Carrasco wrote:
>
>> +static int bh1745_set_trigger_state(struct iio_trigger *trig, bool state)
>> +{
>> + int ret;
>
> Why is value initialized here? If regmap returns an error, you will not
> use value anyway. I caught my eye because it is initialized here, and
> not in the other functions where you use the same pattern.
Hi Javier,
Thank you for the review on this.
'value' is initialized here for case when we un-set the trigger. In that
case, 'state' will be false and 'value' of 0 (default value for
BH1745_INTR register) will be written.
Best regards,
Mudit Sharma
>
>> + int value = 0;
>> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
>> + struct bh1745_data *data = iio_priv(indio_dev);
>> +
>> + guard(mutex)(&data->lock);
>> + if (state) {
>> + ret = regmap_read(data->regmap, BH1745_INTR, &value);
>> + if (ret)
>> + return ret;
>> + // Latch is always set when enabling interrupt
>> + value |= BH1745_INT_ENABLE |
>> + FIELD_PREP(BH1745_INT_SIGNAL_LATCHED, 1) |
>> + FIELD_PREP(BH1745_INT_SOURCE_MASK, data->int_src);
>> + return regmap_write(data->regmap, BH1745_INTR, value);
>> + }
>> +
>> + return regmap_write(data->regmap, BH1745_INTR, value);
>> +}
>
>
> Best regards,
> Javier Carrasco
>
Powered by blists - more mailing lists