From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id A4A12C47DD9 for ; Sun, 24 Mar 2024 11:02:01 +0000 (UTC) Received: by smtp.kernel.org (Postfix) id 5DCF2C433B2; Sun, 24 Mar 2024 11:02:01 +0000 (UTC) Received: from fgw21-7.mail.saunalahti.fi (fgw21-7.mail.saunalahti.fi [62.142.5.82]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp.kernel.org (Postfix) with ESMTPS id 56A01C433C7 for ; Sun, 24 Mar 2024 11:01:59 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 smtp.kernel.org 56A01C433C7 Authentication-Results: smtp.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.kernel.org; spf=fail smtp.mailfrom=gmail.com Received: from localhost (88-113-26-217.elisa-laajakaista.fi [88.113.26.217]) by fgw21.mail.saunalahti.fi (Halon) with ESMTP id ed5c59f1-e9cd-11ee-abf4-005056bdd08f; Sun, 24 Mar 2024 13:01:56 +0200 (EET) From: Andy Shevchenko Date: Sun, 24 Mar 2024 13:01:55 +0200 To: Marek =?iso-8859-1?Q?Beh=FAn?= List-Id: Cc: Arnd Bergmann , Gregory CLEMENT , soc@kernel.org, arm@kernel.org, Linus Walleij , Bartosz Golaszewski , Andy Shevchenko , linux-gpio@vger.kernel.org, Alessandro Zummo , Alexandre Belloni , linux-rtc@vger.kernel.org, Wim Van Sebroeck , Guenter Roeck , linux-watchdog@vger.kernel.org Subject: Re: [PATCH v5 02/11] platform: cznic: Add preliminary support for Turris Omnia MCU Message-ID: References: <20240323164359.21642-1-kabel@kernel.org> <20240323164359.21642-3-kabel@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20240323164359.21642-3-kabel@kernel.org> 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. ... + array_size.h + bits.h > +#include > +#include > +#include > +#include > +#include > +#include + string.h > +#include ... > + 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)); ? ... > + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev)); What's wrong with dev_get_drvdata()? ... > +static ssize_t fw_features_show(struct device *dev, struct device_attribute *a, > + char *buf) One line? ... > + struct omnia_mcu *mcu = i2c_get_clientdata(to_i2c_client(dev)); Use direct call (see above). Same applies to all such constructions in your code. ... > +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() ... > +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. > +module_i2c_driver(omnia_mcu_driver); ... > +#ifndef __TURRIS_OMNIA_MCU_H > +#define __TURRIS_OMNIA_MCU_H + array_size.h > +#include > +#include > +#include > +#include ... > +static inline int omnia_cmd_read(const struct i2c_client *client, u8 cmd, > + void *reply, unsigned int len) > +{ Why is this in the header? > + 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 + bitfield.h > +#endif /* __TURRIS_OMNIA_MCU_INTERFACE_H */ -- With Best Regards, Andy Shevchenko