Re: [PATCH v5 02/11] platform: cznic: Add preliminary support for Turris Omnia MCU

From: "Marek Behún" <kabel@kernel.org>
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>,
	Gregory CLEMENT <gregory.clement@bootlin.com>,
	soc@kernel.org, arm@kernel.org,
	Linus Walleij <linus.walleij@linaro.org>,
	Bartosz Golaszewski <brgl@bgdev.pl>,
	Andy Shevchenko <andy@kernel.org>,
	linux-gpio@vger.kernel.org,
	Alessandro Zummo <a.zummo@towertech.it>,
	Alexandre Belloni <alexandre.belloni@bootlin.com>,
	linux-rtc@vger.kernel.org,
	Wim Van Sebroeck <wim@linux-watchdog.org>,
	Guenter Roeck <linux@roeck-us.net>,
	linux-watchdog@vger.kernel.org
Subject: Re: [PATCH v5 02/11] platform: cznic: Add preliminary support for Turris Omnia MCU
Date: Sun, 24 Mar 2024 16:04:08 +0100	[thread overview]
Message-ID: <20240324160408.77c8574e@thinkpad> (raw)
In-Reply-To: <ZgAII1B03bLUisWr@surfacebook.localdomain>

Hi Andy,

thank you very much for the review. I have some notes and some
questions, see below.

On Sun, 24 Mar 2024 13:01:55 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> Sat, Mar 23, 2024 at 05:43:50PM +0100, Marek Behún kirjoitti:
> > Add the basic skeleton for a new platform driver for the microcontroller
> > found on the Turris Omnia board.  
> 
> ...
> 
> > +++ b/drivers/platform/cznic/Makefile
> > @@ -0,0 +1,4 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +
> > +obj-$(CONFIG_TURRIS_OMNIA_MCU)	+= turris-omnia-mcu.o
> > +turris-omnia-mcu-objs		:= turris-omnia-mcu-base.o  
> 
> 'objs' is for user space. You need to use 'y'. Same applies to the entire
> series.

Fixed for v6.

> 
> + array_size.h
> + bits.h

Fixed for v6. Is there some tool for this?

> + string.h

Fixed for v6.
> 
> > +#include <linux/sysfs.h>  
> 
> ...
> 
> > +	err = omnia_cmd_read(mcu->client, bootloader ? CMD_GET_FW_VERSION_BOOT :
> > +						       CMD_GET_FW_VERSION_APP,
> > +			     reply, sizeof(reply));  
> 
> Wouldn't be better to have a logical split?
> 
> 	err = omnia_cmd_read(mcu->client,
> 			     bootloader ? CMD_GET_FW_VERSION_BOOT : CMD_GET_FW_VERSION_APP,
> 			     reply, sizeof(reply));

Changed for v6 to

> 	err = omnia_cmd_read(mcu->client,
> 			     bootloader ? CMD_GET_FW_VERSION_BOOT
> 					: CMD_GET_FW_VERSION_APP,
> 			     reply, sizeof(reply));

There are still some people wanting only 80 columns, and the whole
driver is written that way.

> 
> ?
> 
> ...
> 
> > +	struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev));  
> 
> What's wrong with dev_get_drvdata()?

Fixed for v6.

> ...
> 
> > +static ssize_t fw_features_show(struct device *dev, struct device_attribute *a,
> > +				char *buf)  
> 
> One line?

80 columns...

...

> > +static const struct attribute_group omnia_mcu_base_group = {
> > +	.attrs = omnia_mcu_base_attrs,
> > +	.is_visible = omnia_mcu_base_attrs_visible,
> > +};
> > +
> > +static const struct attribute_group *omnia_mcu_groups[] = {
> > +	&omnia_mcu_base_group,
> > +	NULL
> > +};  
> 
> __ATTRIBUTE_GROUPS()

The next patches add more groups into this array, after the whole
series it looks like this:

static const struct attribute_group *omnia_mcu_groups[] = {
	&omnia_mcu_base_group,
	&omnia_mcu_gpio_group,
	&omnia_mcu_poweroff_group,
	NULL
};

There is no macro for that. Should I still use __ATTRIBUTE_GROUPS() in
the first patch and than change it in the next one?

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

Fixed for v6.

> > +module_i2c_driver(omnia_mcu_driver);  
> 
> ...
> 
> > +#ifndef __TURRIS_OMNIA_MCU_H
> > +#define __TURRIS_OMNIA_MCU_H  
> 
> + array_size.h

Fixed for v6.

> 
> > +#include <linux/i2c.h>
> > +#include <linux/if_ether.h>
> > +#include <linux/types.h>
> > +#include <asm/byteorder.h>  
> 
> ...
> 
> > +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd,
> > +				 void *reply, unsigned int len)
> > +{  
> 
> Why is this in the header?

I considered it a helper function that should be defined in the header
file, like the rest of the cmd helpers in this file. If you disagree, I
will put it into the -base.c file.

> 
> > +	struct i2c_msg msgs[2];
> > +	int ret;
> > +
> > +	msgs[0].addr = client->addr;
> > +	msgs[0].flags = 0;
> > +	msgs[0].len = 1;
> > +	msgs[0].buf = &cmd;
> > +	msgs[1].addr = client->addr;
> > +	msgs[1].flags = I2C_M_RD;
> > +	msgs[1].len = len;
> > +	msgs[1].buf = reply;
> > +
> > +	ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != ARRAY_SIZE(msgs))
> > +		return -EIO;
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +#ifndef __TURRIS_OMNIA_MCU_INTERFACE_H
> > +#define __TURRIS_OMNIA_MCU_INTERFACE_H
> > +
> > +#include <linux/bits.h>  
> 
> + bitfield.h

Fixed for v6.

> 
> > +#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */  
> 