Re: [PATCH v2 2/2] phy: add NXP PTN3222 eUSB2 to USB2 redriver

[Dmitry Baryshkov]

On Fri, 6 Sept 2024 at 11:40, Song Xue <quic_songxue@xxxxxxxxxxx> wrote:
>
>
>
> On 8/31/2024 7:45 AM, Dmitry Baryshkov wrote:
> > On Sat, 31 Aug 2024 at 02:13, Song Xue <quic_songxue@xxxxxxxxxxx> wrote:
> >> On 8/30/2024 4:20 PM, Dmitry Baryshkov wrote:
> >>> The NXP PTN3222 is the single-port eUSB2 to USB2 redriver that performs
> >>> translation between eUSB2 and USB2 signalling schemes. It supports all
> >>> three data rates: Low Speed, Full Speed and High Speed.
> >>>
> >>> The reset state enables autonegotiation of the PHY role and of the data
> >>> rate, so no additional programming is required.
> >>>
> >>> Reviewed-by: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
> >>> Tested-by: Konrad Dybcio <konradybcio@xxxxxxxxxx>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> >>> ---
> >>>    drivers/phy/Kconfig           |  11 ++++
> >>>    drivers/phy/Makefile          |   1 +
> >>>    drivers/phy/phy-nxp-ptn3222.c | 123 ++++++++++++++++++++++++++++++++++++++++++
> >>>    3 files changed, 135 insertions(+)
> >
> > [trimmed]
> >
> >>> +
> >>> +MODULE_DESCRIPTION("NXP PTN3222 eUSB2 Redriver driver");
> >>> +MODULE_LICENSE("GPL");
> >>>
> >> The I2C driver just realizes the function on reset and PWR. What about
> >> other I2C driver function like I2C interface operations,
> >
> > I don't quite understand what you mean by this. Could you please clarify?
> >
> >>   auto-suspend,
> >
> > I think you mean pm_runtime here. It's a valid case, but granted that
> > it should stay enabled when USB controller is enabled, the gain should
> > be pretty limited. I'll consider a followup patch implementing
> > pm_runtime for the sake of being able to disable I2C host if DWC3
> > controller disables the PHY.
> >
> >> remote wakeup,
> >
> > Not supported by design. PTN3222 doesn't have IRQ pins to report
> > events to the host.
> >
> >> memory maps etc.
> >
> > huh?
> >
> >>   Who will enable these? I think it is not
> >> incomplete I2C driver, if on someday, ptn3222 is used as I2C device.
> >
> > Well, I'm using it as an I2C device.
> >
> Sorry for the delayed response.
> The functions I listed, such as auto-suspend and wake-up, are just
> examples. My main point is that a basic I2C driver should include
> fundamental functions like setting up the I2C bus, configuring the
> clock, and setting the SDA (data line) and SCL (clock line). A basic I2C
> driver shouldn’t be limited to enabling the power supply and reset pin,
> as these features can be handled by other drivers as well.
> If you implement these fundamental functions, I think it will be sufficient.

I think you have mixed two things. You are describing an I2C bus
device, which PTN3222 isn't. I2C clients do not have to setup
anything. SDA/SCL and clock frequency are handled by the I2C bus
drivers and by the I2C framework.

-- 
With best wishes
Dmitry