Re: [PATCH v2 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support

From: Michael Walle <michael@walle.cc>
To: okan.sahin@analog.com
Cc: brgl@bgdev.pl, devicetree@vger.kernel.org,
	krzysztof.kozlowski+dt@linaro.org, linus.walleij@linaro.org,
	linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org,
	robh+dt@kernel.org, Michael Walle <michael@walle.cc>
Subject: Re: [PATCH v2 2/2] gpio: ds4520: Add ADI DS4520 GPIO Expander Support
Date: Tue,  2 May 2023 10:44:06 +0200	[thread overview]
Message-ID: <20230502084406.3529645-1-michael@walle.cc> (raw)
In-Reply-To: <20230501230517.4491-3-okan.sahin@analog.com>

[Please include any former reviewers in new versions.]

> The DS4520 is a 9-bit nonvolatile (NV) I/O expander.
> It offers users a digitally programmable alternative
> to hardware jumpers and mechanical switches that are
> being used to control digital logic node.

Ok, what I just noticed is that this is an open-drain output buffer
with an optional pull-up, that should really go into the commit
message.

Also the commit message is misleading "it offers users a digitally
programmable alternative to hardware jumpers". While the hardware is
capable of that, this driver doesn't make use of it.

> +	config.reg_dat_base = base + IO_STATUS0;
> +	config.reg_set_base = base + PULLUP0;
> +	config.reg_dir_out_base = base + IO_CONTROL0;

Given the above, I don't think this is correct. You pull the line low if
the line is in input mode (?). The line will be pulled low if the
corresponding bit in IO_CONTROL is zero. A one means, the pin is
floating. With open-drain buffers there are usually an external pull-ups,
so I'd treat the internal pull-up as optional and it is not necessary to
switch the actual line state.

In that case the following should be sufficient:
	config.reg_dat_base = base + IO_STATUS0;
	config.reg_set_base = base + IO_CONTROL0;

I'm not sure about the direction though. Technically speaking there is
no direction register. I'm not familiar with how open drain output are
modeled in linux. I'd expect the above is enough. Bartosz/Linus/Andy?

To enable the optional pull-up, you should refer to .set_config.
(You don't need to disable the pull-up if you pull the line low).

Regarding the SEE bit and wear out: The SEE bit seem to be shadowed by the
EEPROM, so if someone is setting the SEE bit it will be persisent. Changing
direction or output value will result in an EEPROM write and might wear out
the EEPROM. I'd like to hear others opinion on that. The worst case write
cycles are 50000. Fail the probe if the SEE bit is set seems not ideal.
Just ignoring that problem for now (or at least warn the user)?

-michael