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;
Date:   Sun, 13 Nov 2022 17:48:32 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Vincent Mailhol <mailhol.vincent@wanadoo.fr>
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
Subject: Re: [PATCH v3 0/3] can: etas_es58x: report firmware, bootloader and
 hardware version
Message-ID: <Y3Ef4K5lbilY3EQT@lunn.ch>
References: <20221104073659.414147-1-mailhol.vincent@wanadoo.fr>
 <20221113040108.68249-1-mailhol.vincent@wanadoo.fr>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221113040108.68249-1-mailhol.vincent@wanadoo.fr>
Precedence: bulk

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.

Have you considered using devlink?

https://www.kernel.org/doc/html/latest/networking/devlink/devlink-info.html

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.

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

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. Since there is a bootloader, i
assume the firmware is upgradeable? devlink supports that.

	  Andrew