Date:   Tue, 31 Jan 2023 19:04:19 -0800
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Wei Fang <wei.fang@nxp.com>, Jakub Kicinski <kuba@kernel.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Shenwei Wang <shenwei.wang@nxp.com>,
        Clark Wang <xiaoning.wang@nxp.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Paolo Abeni <pabeni@redhat.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] net: fec: fix conversion to gpiod API
Message-ID: <Y9nWs+JXPHbE7SPo@google.com>
References: <Y9mar1COtT5z4mvT@google.com>
 <Y9nA4Mmi5hv5OzBh@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y9nA4Mmi5hv5OzBh@lunn.ch>
Precedence: bulk

Hi Andrew,

On Wed, Feb 01, 2023 at 02:31:12AM +0100, Andrew Lunn wrote:
> On Tue, Jan 31, 2023 at 02:48:15PM -0800, Dmitry Torokhov wrote:
> > The reset line is optional, so we should be using
> > devm_gpiod_get_optional() and not abort probing if it is not available.
> 
> > Also, gpiolib already handles phy-reset-active-high, continuing handling
> > it directly in the driver when using gpiod API results in flipped logic.
> 
> Please could you split this part into a separate patch. There is some
> history here, but i cannot remember which driver it actually applies
> to. It might be the FEC, it could be some other Ethernet driver.
> 
> For whatever driver it was, the initial support for GPIOs totally
> ignored the polarity value in DT. The API at the time meant you needed
> to take extra steps to get the polarity, and that was skipped. So it
> was hard coded. But developers copy/pasted DT statement from other DT
> files, putting in the opposite polarity to the hard coded
> value. Nobody noticed until somebody needed the opposite polarity to
> the hard coded implementation to make their board work. And then the
> problem was noticed. The simple solution to actually use the polarity
> in DT would break all the boards which had the wrong value. So a new
> property was added.
> 
> So i would like this change in a separate patch, so if it causes
> regressions, it can be reverted.

The quirk for the gpiod API to take into account "phy-reset-active-high"
for FEC driver is already in mainline:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/gpio/gpiolib-of.c?id=b02c85c9458cdd15e2c43413d7d2541a468cde57

This is limited only to devices matching compatibles handled by FEC
driver and is being considered automatically as soon as gpiod API is
used.


> 
> > While at this convert phy properties parsing from OF to generic device
> > properties to avoid #ifdef-ery.
> 
> We also need to be careful here. If you read fsl,fec.yaml, there are a
> number of deprecated properties. These need to keep working for OF,
> but we clearly don't want them exposed to ACPI or anything else. So if
> you use generic device properties, please ensure they are only for OF.

OK, if this is a concern I'll drop this from the patch and keep of_*
APIs since we need to keep #ifdef CONFIG_OF anyway.

Thanks.

-- 
Dmitry