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;
MIME-Version: 1.0
References: <20221104073659.414147-1-mailhol.vincent@wanadoo.fr>
 <20221113040108.68249-1-mailhol.vincent@wanadoo.fr> <Y3Ef4K5lbilY3EQT@lunn.ch>
In-Reply-To: <Y3Ef4K5lbilY3EQT@lunn.ch>
From:   Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Date:   Tue, 15 Nov 2022 01:49:04 +0900
Message-ID: <CAMZ6RqLjcxDG_yCK3dfYr2dWb7sddRPMDGwXRy62c6WHDH5=Gw@mail.gmail.com>
Subject: Re: [PATCH v3 0/3] can: etas_es58x: report firmware, bootloader and
 hardware version
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Marc Kleine-Budde <mkl@pengutronix.de>, linux-can@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        linux-usb@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon. 14 Nov. 2022 at 02:03, Andrew Lunn <andrew@lunn.ch> wrote:
> On Sun, Nov 13, 2022 at 01:01:05PM +0900, Vincent Mailhol wrote:
> > The goal of this series is to report the firmware version, the
> > bootloader version and the hardware revision of ETAS ES58x
> > devices.
> >
> > These are already reported in the kernel log but this isn't best
> > practise. Remove the kernel log and instead export all these in
> > sysfs. In addition, the firmware version is also reported through
> > ethtool.
>
> Sorry to only comment on version 3, rather than version 1. I don't
> normally look at CAN patches.

Actually, I only started to CC linux-usb mailing from version 2.
Regardless, thanks a lot, this is a valuable feedback.

> Have you considered using devlink?
>
> https://www.kernel.org/doc/html/latest/networking/devlink/devlink-info.html

I have not thought about this (I simply did not know the existence of
this feature). A first quick look makes me think it is a good idea. I
will continue to investigate.

> fw and asic.id would cover two of your properties. Maybe talk to Jiri
> about the bootloader. It might make sense to add it is a new common
> property, or to use a custom property.

I will try to report the firmware version and the hardware version in
a first step and then see what we can do for the bootloader.

> devlink has the advantage of being a well defined, standardised API,
> rather than just random, per device sys files.

ACK.

> There might also be other interesting features in devlink, once you
> have basic support. Many Ethernet switch drivers use devlink regions
> to dump all the registers, for example.

I am aware of ethtool_drvinfo (which I implemented in the last patch
of this series to report the firmware version).
Do you have any reference of how to dump the other registers?

> Since there is a bootloader, i
> assume the firmware is upgradeable? devlink supports that.

True, it is upgradeable, however, I do not have an environment to test
for upgrades so there are no plans right now to develop an upgrade
feature.


Yours sincerely,
Vincent Mailhol