Received-SPF: pass (google.com: domain of linux-kernel+bounces-32979-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 22 Jan 2024 13:01:13 +0100
Message-ID: <87ede9y3ue.wl-tiwai@suse.de>
From: Takashi Iwai <tiwai@suse.de>
To: Javier Carrasco <javier.carrasco@wolfvision.net>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>,
	Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>,
	Rob Herring <robh@kernel.org>,
	devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-sound@vger.kernel.org,
	linux-usb@vger.kernel.org
Subject: Re: [PATCH 2/3] ASoC: dt-bindings: xmos,xvf3500: add bindings for XMOS XVF3500
In-Reply-To: <b888d958-4eb0-4c1f-ace6-b2b85faa5113@wolfvision.net>
References: <20240115-feature-xvf3500_driver-v1-0-ed9cfb48bb85@wolfvision.net>
	<20240115-feature-xvf3500_driver-v1-2-ed9cfb48bb85@wolfvision.net>
	<333c2986-c7c2-4a46-90cf-b59ae206e55a@linaro.org>
	<96abddcc-fa65-4f27-84fe-2281fe0fcf1c@wolfvision.net>
	<644f7f02-405d-47fb-bc72-4d54e897255f@linaro.org>
	<5db4b898-93d5-446f-bfed-b57847f9967a@wolfvision.net>
	<435f502c-1e1b-4d40-8dcc-34487905d69c@linaro.org>
	<b7f76546-9998-43e0-abff-a4e73817dbae@wolfvision.net>
	<47bdc31c-50d2-4d33-9339-5132b6364539@linaro.org>
	<16027339-0a82-4dd1-86aa-19fda6e23f88@wolfvision.net>
	<aeeb0dfb-87e2-4024-9d4a-0b9529477315@linaro.org>
	<b888d958-4eb0-4c1f-ace6-b2b85faa5113@wolfvision.net>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) Emacs/27.2 Mule/6.0
Precedence: bulk
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII

On Tue, 16 Jan 2024 08:29:04 +0100,
Javier Carrasco wrote:
> 
> On 15.01.24 21:43, Krzysztof Kozlowski wrote:
> > On 15/01/2024 20:43, Javier Carrasco wrote:
> >> On 15.01.24 19:11, Krzysztof Kozlowski wrote:
> >>> On 15/01/2024 17:24, Javier Carrasco wrote:
> >>>> Do you mean that the XVF3500 should not be represented as a platform
> >>>> device and instead it should turn into an USB device represented as a
> >>>> node of an USB controller? Something like this (Rockchip SoC):
> >>>>
> >>>> &usb_host1_xhci {
> >>>> 	...
> >>>>
> >>>> 	xvf3500 {
> >>>> 		...
> >>>> 	};
> >>>> };
> >>>>
> >>>> Did I get you right or is that not the correct representation? Thank you
> >>>> again.
> >>>
> >>> I believe it should be just like onboard hub. I don't understand why
> >>> onboard hub was limited to hub, because other USB devices also could be
> >>> designed similarly by hardware folks :/
> >>>
> >>> And if we talk about Linux drivers, then your current solution does not
> >>> support suspend/resume and device unbind.
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>>
> >>
> >> Actually this series is an attempt to get rid of a misuse of the
> >> onboard_usb_hub driver by a device that is not a HUB, but requires the
> >> platform-part of that driver for the initialization.
> > 
> > That's just naming issue, isn't it?
> > 
> >>
> >> What would be the best approach to provide support upstream? Should I
> >> turn this driver into a generic USB driver that does what the
> >> platform-part of the onboard HUB does? Or are we willing to accept
> > 
> > No, because you did not solve the problems I mentioned. This is neither
> > accurate hardware description nor proper Linux driver model handling PM
> > and unbind.
> > 
> You mentioned the PM handling twice, but I am not sure what you mean.
> The driver provides callbacks for SIMPLE_DEV_PM_OPS, which I tested in
> freeze and memory power states with positive results. On the other hand,
> I suppose that you insisted for a good reason, so I would be grateful if
> you could show me what I am doing wrong. The macro pattern was taken
> from other devices under sound/, which also check CONFIG_PM_SLEEP,
> but maybe I took a bad example or missed something.

FWIW, the patterns in sound/ are somewhat outdated and need to be
refreshed.  Nowadays one should use DEFINE_SIMPLE_DEV_PM_OPS() instead
(that should work without ifdef).


thanks,

Takashi