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

[Tobias Waldekranz <tobias@waldekranz.com>]

On sön, okt 06, 2024 at 17:15, Daniel Golle <daniel@makrotopia.org> wrote:
> Hi Tobias,

Hi Daniel,

> 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'm not actively working on it, no, but I still want to get it
merged. I, perhaps wrongly, interpreted Russell's lack of reply to my
motivation for accepting the firmware loading without having the binary
in linux-firmware, as a NAK, and moved on to other things.

> 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.

To me, it seems cleaner to have a single attribute that defines the
behavior you want on the pin (as a string enum). That way you can also
explicitly declare that the kernel shouldn't mess with the settings
(e.g., `polarity = "keep";`, like you can do with the initial
brightness).

If you go that way, I would prefer if the "old" attributes where
deprecated and only evaluated in case the new attribute is not
available.

As for how generic it should be: To me it doesn't seem like there's
anything PHY-specific about it. I suppose where it might be confusing
would be when you have a gpio-led, when that is already taken care of at
the GPIO layer.