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@xxxxxxxxx> 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 */
>
