Re: [PATCH net-next 2/2] net: pse-pd: tps23881: support reset-gpios

[Kyle Swenson <kyle.swenson@est.tech>]

On Tue, Aug 20, 2024 at 03:06:19PM -0600, Kyle Swenson wrote:
> Hi Oleksij,
> 
> On Tue, Aug 20, 2024 at 08:23:58AM +0200, Oleksij Rempel wrote:
> > Hi Kyle,
> > 
> > thank you for you patch.
> Thanks for the review!
> > 
> > On Mon, Aug 19, 2024 at 07:02:14PM +0000, Kyle Swenson wrote:
> > > The TPS23880/1 has an active-low reset pin that some boards connect to
> > > the SoC to control when the TPS23880 is pulled out of reset.
> > > 
> > > Add support for this via a reset-gpios property in the DTS.
> > > 
> > > Signed-off-by: Kyle Swenson <kyle.swenson@est.tech>
> > > ---
> > >  drivers/net/pse-pd/tps23881.c | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/pse-pd/tps23881.c b/drivers/net/pse-pd/tps23881.c
> > > index 2ea75686a319..837e1a2119ee 100644
> > > --- a/drivers/net/pse-pd/tps23881.c
> > > +++ b/drivers/net/pse-pd/tps23881.c
> > > @@ -6,16 +6,16 @@
> > >   */
> > >  
> > >  #include <linux/bitfield.h>
> > >  #include <linux/delay.h>
> > >  #include <linux/firmware.h>
> > > +#include <linux/gpio/consumer.h>
> > >  #include <linux/i2c.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pse-pd/pse.h>
> > > -
> > 
> > No need to remove space here.
> 
> Sorry about this, somehow I missed this gratuitous diff
> 
> > 
> > >  #define TPS23881_MAX_CHANS 8
> > >  
> > >  #define TPS23881_REG_PW_STATUS	0x10
> > >  #define TPS23881_REG_OP_MODE	0x12
> > >  #define TPS23881_OP_MODE_SEMIAUTO	0xaaaa
> > > @@ -735,10 +735,11 @@ static int tps23881_flash_sram_fw(struct i2c_client *client)
> > >  
> > >  static int tps23881_i2c_probe(struct i2c_client *client)
> > >  {
> > >  	struct device *dev = &client->dev;
> > >  	struct tps23881_priv *priv;
> > > +	struct gpio_desc *reset;
> > >  	int ret;
> > >  	u8 val;
> > >  
> > >  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> > >  		dev_err(dev, "i2c check functionality failed\n");
> > > @@ -747,10 +748,20 @@ static int tps23881_i2c_probe(struct i2c_client *client)
> > >  
> > >  	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > >  	if (!priv)
> > >  		return -ENOMEM;
> > >  
> > > +	reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(reset))
> > > +		return dev_err_probe(&client->dev, PTR_ERR(reset), "Failed to get reset GPIO\n");
> > > +
> > > +	if (reset) {
> > > +		usleep_range(1000, 10000);
> > > +		gpiod_set_value_cansleep(reset, 0); /* De-assert reset */
> > > +		usleep_range(1000, 10000);
> > 
> > According to the datasheet, page 13:
> > https://www.ti.com/lit/ds/symlink/tps23880.pdf
> > 
> > Minimal reset time is 5 microseconds and the delay after power on reset should
> > be at least 20 milliseconds. Both sleep values should be corrected.
> 
> Sounds reasonable, I'll change the first delay to be closer to the 5us
> minimum reset time.  I need to review the docs around delays to pick the
> correct one for this case.
> 
> For the 2nd delay, I (now) see the 20ms you're referring to in the datasheet.
> 
> I was looking at the SRAM programming document
> (https://www.ti.com/lit/pdf/SLVAE1) and it indicates we should delay the

Sorry, this is the wrong link.  Let me try again:

https://www.ti.com/lit/pdf/slvae12


> SRAM and parity programming by at least 50ms after initial power on.
> 
> Should we guarantee we meet that 50ms requirement with the 2nd delay or
> would you prefer I just meet the 20ms requirement in the datasheet?
> 
> > 
> > Regards,
> > Oleksij
> > -- 
> > Pengutronix e.K.                           |                             |
> > Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> > 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> Thanks again for the review!
> 
> Cheers,
> Kyle