Re: [PATCH net-next 1/4] net: phy: marvell10g: Support firmware loading on 88X3310

[Daniel Golle <daniel@makrotopia.org>]

Hi Tobias,

On Tue, Jan 02, 2024 at 02:09:24PM +0100, Tobias Waldekranz wrote:
> On tis, jan 02, 2024 at 10:12, "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > On Tue, Dec 19, 2023 at 11:15:41AM +0100, Tobias Waldekranz wrote:
> >> On tis, dec 19, 2023 at 10:22, Marek Behún <kabel@kernel.org> wrote:
> >> > On Thu, 14 Dec 2023 21:14:39 +0100
> >> > Tobias Waldekranz <tobias@waldekranz.com> wrote:
> >> >
> >> >> +MODULE_FIRMWARE("mrvl/x3310fw.hdr");
> >> >
> >> > And do you have permission to publish this firmware into linux-firmware?
> >> 
> >> No, I do not.
> >> 
> >> > Because when we tried this with Marvell, their lawyer guy said we can't
> >> > do that...
> >> 
> >> I don't even have good enough access to ask the question, much less get
> >> rejected by Marvell :) I just used that path so that it would line up
> >> with linux-firmware if Marvell was to publish it in the future.
> >> 
> >> Should MODULE_FIRMWARE be avoided for things that are not in
> >> linux-firmware?
> >
> > Without the firmware being published, what use is having this code in
> > mainline kernels?
> 
> Personally, I primarily want this merged so that future contributions to
> the driver are easier to develop, since I'll be able test them on top of
> a clean net-next base.

I've been pointed to your series by Krzysztof Kozlowski who had reviewed
the DT part of it. Are you still working on that or going to eventually
re-submit it?

I understand that the suggested LED support pre-dates commit

7ae215ee7bb8 net: phy: add support for PHY LEDs polarity modes

which would allow using generic properties 'active-low' and
'inactive-high-impedance'. I assume that would be applicable to the LED
patch which was part of this series as well?

In that case, we would no longer need a vendor-specific property for that
purpose. If the LEDs are active-low by default (or early boot firmware
setting) and you would need a property for setting them to 'active-high'
instead, I just suggested that in

https://patchwork.kernel.org/project/netdevbpf/patch/e91ca84ac836fc40c94c52733f8fc607bcbed64c.1728145095.git.daniel@makrotopia.org/

which is why I'm now contacting you, as I was a bit confused by Krzysztof's
suggestion to take a look at marvell,marvell10g.yaml which would have been
introduced by your series.

Imho it would be better to use the (now existing) generic properties than
resorting to a vendor-specific one.

In every case, if you have a minute to look at commit 7ae215ee7bb8 and let
us know whether that structure, with or without my suggested addition,
would be suitable for your case as well, that would be nice.


Thank you for your time and support!


Daniel