linux-kernel - Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z-aXnHig0HgVOLK2@smile.fi.intel.com>
Date: Fri, 28 Mar 2025 14:35:40 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Mathieu Dubois-Briand <mathieu.dubois-briand@...tlin.com>
Cc: Uwe Kleine-König <ukleinek@...nel.org>,
	Lee Jones <lee@...nel.org>, Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Kamel Bouhara <kamel.bouhara@...tlin.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Bartosz Golaszewski <brgl@...ev.pl>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Michael Walle <mwalle@...nel.org>, Mark Brown <broonie@...nel.org>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	"Rafael J. Wysocki" <rafael@...nel.org>,
	Danilo Krummrich <dakr@...nel.org>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
	linux-input@...r.kernel.org, linux-pwm@...r.kernel.org,
	Grégory Clement <gregory.clement@...tlin.com>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v5 04/11] pwm: max7360: Add MAX7360 PWM support

On Fri, Mar 28, 2025 at 09:13:12AM +0100, Mathieu Dubois-Briand wrote:
> On Thu Mar 27, 2025 at 6:50 PM CET, Andy Shevchenko wrote:
> > On Thu, Mar 27, 2025 at 03:28:08PM +0100, Mathieu Dubois-Briand wrote:
> > > On Wed Mar 26, 2025 at 4:49 PM CET, Andy Shevchenko wrote:

...

> > > > The use of this API is inappropriate here AFAICT. It drops the parent refcount
> > > > and on the second call to it you will have a warning from refcount library.
> > > >
> > > > It should be as simple as device_set_node().
> > > >
> > > > >         }
> > > >
> > > > With that, the conditional becomes
> > > >
> > > > 	} else if (is_of_node(fwnode)) {
> > > > 		device_set_node(&pdev->dev, fwnode);
> > > > 	}
> > > >
> > > > where fwnode is something like
> > > >
> > > > 	struct fwnode_handle *fwnode = dev_fwnode(parent);
> > > 
> > > I tried to use device_set_node(), but then I got some other issue: as we
> > > now have several devices with the same firmware node, they all share the
> > > same properties. In particular, if we do use pinctrl- properties to
> > > apply some pinmmuxing, all devices will try to apply this pinmuxing and
> > > of course all but one will fail.
> > > 
> > > And this makes me think again about the whole thing, maybe copying the
> > > fwnode or of_node from the parent is not the way to go.
> > > 
> > > So today we rely on the parent node for four drivers:
> > > - keypad and rotary, just to ease a bit the parsing of some properties,
> > >   such as the keymap with matrix_keypad_build_keymap(). I can easily do
> > >   it another way.
> > > - PWM and pinctrl drivers, are a bit more complicated, as in both case
> > >   the device tree node associated with the device is used internally. In
> > >   one case to find the correct PWM device for PWM clients listed in the
> > >   device tree, in the other case to find the pinctrl device when
> > >   applying pinctrl described in the device tree.
> > > 
> > > So maybe I have to find a better way for have this association. One way
> > > would be to modify the device tree bindings to add a PWM and a pinctrl
> > > node, with their own compatible, so they are associated to the
> > > corresponding device. But maybe there is a better way to do it.
> >
> > Okay, so the main question now, why do the device share their properties
> > to begin with? It can be done via fwnode graph or similar APIs (in case
> > it is _really_ needed).
> 
> I wouldn't say the properties are shared: we have a single node in the
> device tree as this is just one device. But as we create several
> (software) devices in the MFD driver, we now have several devices linked
> with a single device tree node.
> 
> One solution would be to create more subnodes in the device tree, one
> for pinctrl and one for PWM, but this feels a bit like describing our
> software implementation in the device tree instead of describing the
> hardware.

I see. From my point of view the above is the correct approach, but
you need to ask DT experts, I'm not one of them.

-- 
With Best Regards,
Andy Shevchenko