Re: [PATCH v3 2/2] iio: light: isl76682: Add ISL76682 driver

[Marek Vasut <marex@denx.de>]

On 11/21/23 18:20, Andy Shevchenko wrote:
> On Tue, Nov 21, 2023 at 04:14:54AM +0100, Marek Vasut wrote:
>> On 11/20/23 13:02, Andy Shevchenko wrote:
> 
> [...]
> 
>>>> +static int isl76682_clear_configure_reg(struct isl76682_chip *chip)
>>>> +{
>>>> +	struct device *dev = regmap_get_device(chip->regmap);
>>>> +	int ret;
>>>> +
>>>> +	ret = regmap_write(chip->regmap, ISL76682_REG_COMMAND, 0x0);
>>>> +	if (ret < 0)
>>>> +		dev_err(dev, "Error %d clearing the CONFIGURE register\n", ret);
>>>
>>>> +	chip->command = 0;
>>>
>>> Even in an error case? Is it a problem?
>>
>> I added a comment in V4, if the I2C communication fails, hopefully the next
>> time user reads data the command register will be rewritten again and the
>> communication would have succeeded by then (assuming this was some weird
>> glitch on the I2C bus). So this is best effort attempt to recover from that.
> 
> OK.
> 
> ...
> 
>>>> +
>>>
>>> Redundant blank line.
>>>
>>>> +module_i2c_driver(isl76682_driver);
>>
>> That ^ newline is above the module_i2c_driver or below it ?
>>
>> I removed the one below .
> 
> Hmm... Comment was clearly about above one (as you see a single + there).

I'll fix that up in V5 then, will wait a bit for further feedback before 
posting that one.