Re: [PATCH v2 2/7] platform: cznic: Add preliminary support for Turris Omnia MCU

["Marek Behún" <kabel@kernel.org>]

Hello Andy,

thank you for your review. I have some questions regarding your
comments.

On Tue, 19 Sep 2023 15:29:08 +0300
Andy Shevchenko <andy@kernel.org> wrote:

> On Tue, Sep 19, 2023 at 12:38:10PM +0200, Marek Behún wrote:
> > Add a new platform driver that exposes the features provided by the
> > microcontroller on the Turris Omnia board.  
> 
> > This commit adds the basic skeleton for the driver.  
> 
> Read "Submitting Patches" documentation (find "This patch" in it) and amend
> your commit message accordingly.

OK.

> > +Date:		August 2023
> > +KernelVersion:	6.6  
> 
> Wrong and outdated. Use phb-crystall-ball to find the closest possible values
> for both parameters here.
> 
> Ditto for all others with the same issue.
> 
> Note, it likely might be part of v6.7, not earlier.

OK

> >  obj-$(CONFIG_GOLDFISH)		+= goldfish/
> >  obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
> >  obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
> > +obj-$(CONFIG_CZNIC_PLATFORMS)	+= cznic/  
> 
> Why not ordered (to some extent) here (as you did in the other file)?

Where should I put it? The other entries are not ordered, see
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/platform/Makefile?h=v6.6-rc2

> > +turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o  
> 
> objs is for user space tools. Kernel code should use m,y.

I don't understand. There are plenty driver using -objs. The driver
consists of multiple C source files. I thought this should be done as
  obj-$(CONFIG_XXX) += xxx.o
  xxx-objs := xxx-a.o xxx-b.o xxx-c.o

If you choose CONFIG_XXX = y, then the driver will be compiled into
kernel, if you choose CONFIG_XXX = m, then the driver will be a module
xxx.ko.

> > +#include <linux/hex.h>
> > +#include <linux/module.h>  
> 
> Missing types.h, sysfs.h, mod_devicetable.h, i2c.h, ...

i2c.h comes from turris-omnia-mcu.h. The others I will fix. Does the
the .c file also include all those headers if it includes a local
header already?

> > +static int omnia_get_version_hash(struct omnia_mcu *mcu, bool
> > bootloader,
> > +				  u8 *version)
> > +{
> > +	u8 reply[20];
> > +	int ret;
> > +
> > +	ret = omnia_cmd_read(mcu->client, bootloader ?
> > CMD_GET_FW_VERSION_BOOT :
> > +n average development ti
> > CMD_GET_FW_VERSION_APP,
> > +			     reply, sizeof(reply));  
> 
> > +	if (ret < 0)  
> 
> Can it return positive value? What would be the meaning of it?

Originally I had it return 0 on success, but recently I've had a
discussion about this same thing with LED subsystem maintainer about
some LED patches, Lee Jones, who requested the _read function to return
number of bytes read on success. See at
  https://lore.kernel.org/linux-leds/20230821122644.GJ1380343@google.com/
where he says:
  Read and write APIs, even abstracted ones such as these generally
  return the number of bytes successfully read and written respectively.
  If you are going to normalise, then please do so against this
  standard.
I do not agree with him, since this is a local command accessor which
only succeeds if all the bytes are read/written as requested. But I
adjusted my code as per his request since he is the maintainer.

I will change the code back so that it returns 0 on success instead of
the number of bytes written.


> > +		return ret;  
> 
> > +	version[40] = '\0';  
> 
> How do you know the version has enough space?

The function is only use within the file, and both users know how much
space it needs. But ok, I shall enforce it with the static keyword as
  static int
  omnia_get_version_hash(...,
			 u8 version[static 2*OMNIA_FW_VERSION_LEN + 1]);
Would that be okay?

> > +	bin2hex(version, reply, 20);
> > +
> > +	return 0;
> > +}
> > +
> > +static ssize_t fw_version_hash_show(struct device *dev, char *buf,
> > +				    bool bootloader)
> > +{
> > +	struct omnia_mcu *mcu =
> > i2c_get_clientdata(to_i2c_client(dev));
> > +	u8 version[41];  
> 
> > +	int err;  
> 
> In two near functions you already inconsistent in the naming of the
> return code variable. Be consistent across all your code, i.e. choose
> one name and use it everywhere.

I use
  ret - when the return value can also be positive and mean something
  err - when the return value is either 0 (no error) or negative errno
Is this inconsistent?

> > +	err = omnia_get_version_hash(mcu, bootloader, version);
> > +	if (err)
> > +		return err;
> > +
> > +	return sysfs_emit(buf, "%s\n", version);
> > +}  
> 
> ...
> 
> > +	return sysfs_emit(buf, "%#x\n", mcu->features);  
> 
> "0" will be printed differently, is this on purpose?

No, I wanted it to print 0x0, didn't know '#' would print just 0.
Thanks.

> > +	int ret = omnia_cmd_read_u8(to_i2c_client(dev),
> > CMD_GET_RESET); +
> > +	if (ret < 0)
> > +		return ret;  
> 
> Better from maintenance perspective to have
> 
> 	int ret;
> 
> 	ret = omnia_cmd_read_u8(to_i2c_client(dev), CMD_GET_RESET);
> 	if (ret < 0)
> 		return ret;

OK

> > +static struct attribute *omnia_mcu_attrs[] = {
> > +	&dev_attr_fw_version_hash_application.attr,
> > +	&dev_attr_fw_version_hash_bootloader.attr,
> > +	&dev_attr_fw_features.attr,
> > +	&dev_attr_mcu_type.attr,
> > +	&dev_attr_reset_selector.attr,  
> 
> > +	NULL,  
> 
> No comma for the terminator line. It will make your code robust at
> compile time against some misplaced entries.

OK

> > +};  
> 
> > +  
> 
> Unneeded blank line.

OK

> > +ATTRIBUTE_GROUPS(omnia_mcu);  
> 
> ...
> 
> > +	u8 version[41];  
> 
> This magic sizes should go away. Use predefined constant, or allocate
> on the heap, depending on the case(s) you have.

OK

> > +	int status;  
> 
> My gosh, it's a _third_ name for the same!

The variable holds value of the status register, which is needed at
another point in this function. I thought naming it just "ret" would
cause confusions... Do you really propose to rename it such?

> > +	status = omnia_cmd_read_u16(mcu->client,
> > CMD_GET_STATUS_WORD);
> > +	if (status < 0)
> > +		return status;  
> 
> ...
> 
> > +	for (int i = 0; i < ARRAY_SIZE(features); ++i) {  
> 
> Why signed?

Because it is shorter and makes no difference and there are tons of
  for (int i = 0; i < ARRAY_SIZE(x); i++)
in the kernel already. But if you really want, I can change it. Please
let me know.

> Why pre-increment?

Again, it makes no difference, is widely used in the kernel and I
personally prefer it. But I will change it if you want.


> 
> > +		if (!(mcu->features & features[i].mask)) {  
> 
> Wouldn't be better
> 
> 		if (mcu->features & features[i].mask)
> 			continue;
> 
> ?

OK

> > +			omnia_info_missing_feature(dev,
> > features[i].name);
> > +			suggest_fw_upgrade = true;
> > +		}
> > +	}  
> 
> ...
> 
> > +		dev_info(dev,
> > +			 "Consider upgrading MCU firmware with the
> > omnia-mcutool utility.\n");  
> 
> You have so-o many dev_info() calls, are you sure you not abusing use
> of that?

I want the users to upgrade the MCU firmware. These infos are only
printed during probe time, and won't be printed at all if the firmware
is upgraded. Is this a problem?

> 
> ...
> 
> > +	if (ret) {
> > +		dev_err(dev, "Cannot determine MCU supported
> > features: %d\n",
> > +			ret);
> > +		return ret;  
> 
> 		return dev_err_probe(...);

Thx. Didn't know this one.

> > +	}  
> 
> ...
> 
> > +	if (!client->irq) {
> > +		dev_err(dev, "IRQ resource not found\n");
> > +		return -ENODATA;
> > +	}  
> 
> Why you need to allocate memory, go through the initial checks, etc
> and then fail? What's wrong with checking this first?
> 
> Why -ENODATA?!

OK

> > +static const struct of_device_id of_omnia_mcu_match[] = {
> > +	{ .compatible = "cznic,turris-omnia-mcu", },  
> 
> Inner comma is not needed.

OK

> > +	{},  
> 
> No comma for the terminator entry.

OK

> > +};  
> 
> ...
> 
> > +static const struct i2c_device_id omnia_mcu_id[] = {
> > +	{ "turris-omnia-mcu", 0 },  
> 
> ', 0' part is redundant.

OK

> > +	{ }
> > +};  
> 
> ...
> 
> > +static struct i2c_driver omnia_mcu_driver = {
> > +	.probe		= omnia_mcu_probe,
> > +	.id_table	= omnia_mcu_id,
> > +	.driver		= {
> > +		.name	= "turris-omnia-mcu",
> > +		.of_match_table = of_omnia_mcu_match,
> > +		.dev_groups = omnia_mcu_groups,
> > +	},
> > +};  
> 
> > +  
> 
> Redundant blank line.

OK

> > +module_i2c_driver(omnia_mcu_driver);  
> 
> ...
> 
> > +#ifndef __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H
> > +#define __DRIVERS_PLATFORM_CZNIC_TURRIS_OMNIA_MCU_H  
> 
> WE_LIKE_VERY_LONG_AND_OVERFLOWED_DEFINITIONS_H!

Sigh... OK, I will change it :)

> > +#include <asm/byteorder.h>  
> 
> This usually goes after linux/*.h.

OK
> 
> > +#include <linux/i2c.h>  
> 
> Missing types.h, errno.h, ...
> 
> + blank line?

Will change.

> > +#include <linux/turris-omnia-mcu-interface.h>  
> 
> ...
> 
> > +	ret = i2c_transfer(client->adapter, msgs,
> > ARRAY_SIZE(msgs));
> > +	if (likely(ret == ARRAY_SIZE(msgs)))  
> 
> Why likely()? Please, justify.

Becuase it is unlikely the I2C transaction will fail. In most cases, it
does not.

> > +		return len;  
> 
> > +	else if (ret < 0)
> > +		return ret;
> > +	else
> > +		return -EIO;  
> 
> In both cases the 'else' is redundant. Moreover, the usual pattern is
> to check for the errors first.
> 
> > +}  
> 
> ...
> 
> > +#ifndef __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H
> > +#define __INCLUDE_LINUX_TURRIS_OMNIA_MCU_INTERFACE_H  
> 
> My gosh!

:)

> > +#include <linux/bits.h>  
> 
> > +enum commands_e {  
> 
> Are you sure the name is unique enough / properly namespaced?
> Same Q to all enums.

I can change it. I wanted it to be compatible with the header in the
firmware repository.
> 
> ...
> 
> > +	/*CTL_RESERVED		= BIT(2),*/  
> 
> Missing spaces?
> Also, needs a comment why this is commented out.

OK

> > +	CTL_BOOTLOADER		= BIT(7)  
> 
> Add trailing comma.

OK

Marek