Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 23 Sep 2022 17:13:03 +0000
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Michal Simek <michal.simek@amd.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        "Mehta, Piyush" <piyush.mehta@amd.com>,
        Marek Vasut <marex@denx.de>,
        Linux USB List <linux-usb@vger.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "git (AMD-Xilinx)" <git@amd.com>,
        "Paladugu, Siva Durga Prasad" <siva.durga.prasad.paladugu@amd.com>
Subject: Re: USB companion HUB for usb5744
Message-ID: <Yy3pH7pHQ8H364OE@google.com>
References: <479b13a3-08f6-c7c9-dce3-13831d69da3e@amd.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <479b13a3-08f6-c7c9-dce3-13831d69da3e@amd.com>
Precedence: bulk

Hi Michal,

On Fri, Sep 23, 2022 at 03:08:54PM +0200, Michal Simek wrote:
> Hi Matthias,
> 
> I want to follow up with you on discussion we had couple of months ago
> around usb companion HUB series which is finally merged to the tree.
> We are using USB5744 hub(4 ports), it supports USB3.0 functionality backward
> compatible with USB2.0.
> 
> To generic driver support for regulator and gpio reset was added.
> In our case usb hub has i2c interface connected and hub expects
> initialization via i2c to start to operate.
> 
> In past you mentioned to create new file just for handling hubs itself.

Yes, code that is specific for certain hub models should be separate from
the core driver. As long as the hub specific code isn't too large it
could be an option to host it in a single file for multiple models.

The core driver could invoke an ->init callback if it exists to perform
the hub specific initialization.

> Anyway from DT perspective I discussed description with Rob some time ago
> https://lore.kernel.org/all/CAL_JsqJZBbu+UXqUNdZwg-uv0PAsNg55026PTwhKr5wQtxCjVQ@mail.gmail.com/
> where he wanted to use i2c-bus link.
> 
> It will end up with description like this.
> 
> 	usbhub0_2_0: hub@1 { /* u43 */
> 		compatible = "usb424,2744";
> 		reg = <1>;
> 		i2c-bus = <&usbhub_i2c0>;
> 		reset-gpios = <&slg7xl45106 3 GPIO_ACTIVE_LOW>;
> 		peer-hub = <&usbhub0_3_0>;
> 	};
> 
> 	usbhub0_3_0: hub@2 { /* u43 */
> 		compatible = "usb424,5744";
> 		reg = <2>;
> 		i2c-bus = <&usbhub_i2c0>;
> 		reset-gpios = <&slg7xl45106 3 GPIO_ACTIVE_LOW>;
> 		peer-hub = <&usbhub0_2_0>;
> 	};
> 
> In probe onboard_hub_power_on is called to toggle gpio line which we need
> and then we need to write 16 bits to basic initialization (or more for
> different one).
> Marek also mentioned that he has another chip from Microchip where i2c
> address can be setup. I think it would be good to get any guidance how this
> should be integrated.
> 
> From my perspective i2c address should be described in DT.
> 
> Can we use any description like?
> i2c-bus = <&usbhub_i2c0 0x2d>;

I don't have objections as long as the DT folks are happy with that.

> And initialization sequence via any property with
> uint32-array/uint16-array/uint8-array types.

I very much doubt that you could convince DT maintainers to approve such a
binding. The device tree is supposed to describe the hardware, an
initialization sequence does not do that. Also a sequence of bytes is very
opaque, in source code comments can be added if something is not evident.
IMO the initialization should be done in an ->init callback that is specific
to the USB5744 hub.