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@xxxxxxxx>
> > > ---
> > > 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
