Date:   Tue, 14 Mar 2023 15:33:10 +0800
From:   "Ye, Xiang" <xiang.ye@intel.com>
To:     Andi Shyti <andi.shyti@kernel.org>
CC:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Arnd Bergmann <arnd@arndb.de>,
        Matthias Kaehlcke <mka@chromium.org>,
        Lee Jones <lee@kernel.org>, Wolfram Sang <wsa@kernel.org>,
        Tyrone Ting <kfting@nuvoton.com>,
        Mark Brown <broonie@kernel.org>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        <linux-usb@vger.kernel.org>, <linux-i2c@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-spi@vger.kernel.org>,
        <linux-gpio@vger.kernel.org>, <srinivas.pandruvada@intel.com>,
        <heikki.krogerus@linux.intel.com>,
        <andriy.shevchenko@linux.intel.com>,
        <sakari.ailus@linux.intel.com>, <zhifeng.wang@intel.com>,
        <wentong.wu@intel.com>, <lixu.zhang@intel.com>
Subject: Re: [PATCH v5 1/5] usb: Add support for Intel LJCA device
Message-ID: <ZBAjNlv+NbC2Le0v@ye-NUC7i7DNHE>
References: <20230312190435.3568212-1-xiang.ye@intel.com>
 <20230312190435.3568212-2-xiang.ye@intel.com>
 <20230313162146.eag5z6micbpczbt2@intel.intel>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230313162146.eag5z6micbpczbt2@intel.intel>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ZziNnyjBfRlqT3kXYB+j+vRnM1CBS6ZzSnP9AR0+tWQYE43s8PXuZ9fvVWhN?=
 =?us-ascii?Q?vXnIPkOkoabVFFqJpySLSGukaQ6opBzuSXWi04s8X1LPRVGTioI5RTy53w1Q?=
 =?us-ascii?Q?bYgGkNiYqUN6KcJpX1U0vxMfRzCL7LVTl71t1qSpg9/jcQOILhsWbZ/Ni5vZ?=
 =?us-ascii?Q?dblzuLdhkCMpOsZDVyAiE0P4ZmogPWagHnBnaBhsSwntdRCoZf/dZyHLh6G2?=
 =?us-ascii?Q?q7kR3UWUZrkOI/O3IfyfGesx+H2NzExFQX8MRuq5v9hdNxD3mcNjERoqli7x?=
 =?us-ascii?Q?glVaybYUhEo08t1rz3Bu9nsFVMm+eS4Q761ay0m8cwOrWSaNPZZjXP8bEOkm?=
 =?us-ascii?Q?6atx7l0xz5RdgZwdzQeWaR7NSY2FlolRfbSlaKw3TR62ShDsVrYCpJa/V+r0?=
 =?us-ascii?Q?zG513HaqLpFrElw8Edtr3bvz2gJ4uVAnK32lUEAn7BoYXCzmtirURhJyW3i/?=
 =?us-ascii?Q?gNr76YaIfl3BywgKyFeSqrgrcibB94dGi8jwI1v4Nz0TPKHMV/Zum5+2B9zL?=
 =?us-ascii?Q?f3T/WSHqTAUzKAfwwj5sKGTd7F3SFtQJXVxp+6WS0iCRnNam63j422zw7bFj?=
 =?us-ascii?Q?keBPqRcXfiJ5Eqblz+5DVq9tp+pWVKaKF1hwCjpNAkNLh6qSVc441GO1tTr3?=
 =?us-ascii?Q?/EHzhUbrc8mqv8AFUx7PvSDgxSoPXbc9p6+mf+U5H+Dd6P7pZy4qF4SAvFIR?=
 =?us-ascii?Q?2ewWG61y/5+BHCrYoEI1Q8yWabuILn1FfArZ7jdVQdJpd68/aTMFc9E3GD1B?=
 =?us-ascii?Q?bBEojVey8Cqu10BrfL/Emd7k9hWe3HrBDjLYRZH/uQEIlhXNuzzaJSqKKNeZ?=
 =?us-ascii?Q?5816RXfGNHZt3JpVEDUdlF/eG8KE0FgwXmJT5mlamPwxwxcqaMwQ+Yz9sgXR?=
 =?us-ascii?Q?s0l0U/YECN7F62HIosUqoudlyIlSP1kQJTPYfs8gPADk/bT765+Zo50LaMxe?=
 =?us-ascii?Q?KJgSlC6lW2pwyfx7ItVm39MXcvMmyUNZfKap7Zb997ata2L5CPAeqIE2xXoC?=
 =?us-ascii?Q?XN7l6knkWj7yFaBINHmj9rVeHtrkrcdyrWRhGqE0EL6nvrzPXNQ0a+q1HfnE?=
 =?us-ascii?Q?Nqy9Yx+T6MTw1TF4XvQkA598aIbDmp7I/iN0zV02uiCU5bxVXHTFB1veGV9S?=
 =?us-ascii?Q?vgJd48X2LyvbFtEco9YJrVOdx/m+K3T3i0+M/D6sb32tp9waeJ+1EE91RlV5?=
 =?us-ascii?Q?4F5S6QL64PWk6rAUDeOdwDBnhuFz/djDzygvsfA2DNXaA/Iemtwq1RNLJI1w?=
 =?us-ascii?Q?+0nmaHIWweEOv9szebMprjtRkGOuFlhf9WhLiCeJF17acyxYJetskdts60eH?=
 =?us-ascii?Q?aRs+59XKiR838Ca+n1WwAElDeobqY3CnIOlrIxzRhOAizbgm8/dQkC4M1RmE?=
 =?us-ascii?Q?zepfB83Y0a7XUcYOBdSBW53GHcOeS0Sz8OD5RVizwhzl7krFu5C327Kpra3V?=
 =?us-ascii?Q?qnYTVRurSdVqUPLo8YOmoCMSMfpbWkkev5Qh7NBEjeMrk5jsmJm1RXRH1MEW?=
 =?us-ascii?Q?sGkkW3E+g/oZgpWagmujGjT91JJwnjsvWTxxRmHQ2IMY6n6c3Bdg2/3uEyOt?=
 =?us-ascii?Q?Q5p6X3AgpVZ+XBiRAg//k34bB+dCAPMHzOQH3P38?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 9509212d-0001-45d0-b4ee-08db245e6128
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1418.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Mar 2023 07:33:18.1296
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: dXa3O73yDUFhkWpgv2IVIYSx5+IOchDo/zvy3hl40zNCuT/dakTo6JFTOIeiMygaAKaQJo8CjdLE7I9p5IM+3g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW3PR11MB4697
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Andi,

Thanks for the review.
On Mon, Mar 13, 2023 at 05:21:46PM +0100, Andi Shyti wrote:
> Hi Ye,
> 
> On top of what Greg has already said, few things from my side
> through the lines.
> 
> [...]
> 
> > +static int ljca_mng_link(struct ljca_dev *dev, struct ljca_stub *stub)
> > +{
> > +	int ret;
> > +
> > +	ret = ljca_mng_reset_handshake(stub);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* try to enum all the stubs */
> > +	ljca_mng_enum_gpio(stub);
> > +	ljca_mng_enum_i2c(stub);
> > +	ljca_mng_enum_spi(stub);
> 
> We are ignoring here the return value. So either make the
> whole function call chain to be void or please check the return
> values here.
Ok, got it.
> 
> [...]
> 
> > +static ssize_t ljca_enable_dfu_store(struct device *dev, struct device_attribute *attr,
> > +				     const char *buf, size_t count)
> > +{
> > +	struct usb_interface *intf = to_usb_interface(dev);
> > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > +	struct ljca_stub *mng_stub = ljca_stub_find(ljca_dev, LJCA_MNG_STUB);
> > +	bool enable;
> > +	int ret;
> > +
> > +	ret = kstrtobool(buf, &enable);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (enable) {
> > +		ret = ljca_mng_set_dfu_mode(mng_stub);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> What is the DFU mode?
> Is it an operational mode?
> Do we enter and exit from it?
> Does the device leave this mode on its own?
> What if I write twice in a raw enable?
> Can I check if I am in DFU mode or not?
> 
> Would you mind adding some comments here?
DFU mode is used for updating LJCA device Firmware.
We have a Documentation/ABI/testing/sysfs-bus-usb-devices-ljca in patch
5 of this patch series. It has information about the entry. Maybe it
should be go after this patch (patch 2/5).
> 
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_WO(ljca_enable_dfu);
> > +
> > +static ssize_t ljca_trace_level_store(struct device *dev, struct device_attribute *attr,
> > +				      const char *buf, size_t count)
> > +{
> > +	struct usb_interface *intf = to_usb_interface(dev);
> > +	struct ljca_dev *ljca_dev = usb_get_intfdata(intf);
> > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > +	u8 level;
> > +	int ret;
> > +
> > +	if (kstrtou8(buf, 0, &level))
> > +		return -EINVAL;
> > +
> > +	ret = ljca_diag_set_trace_level(diag_stub, level);
> > +	if (ret)
> > +		return ret;
> 
> do we need any range check for the levels? What happens if I do:
> 
> echo "I am too cool" > /sys/.../ljca_trace_level
> 
> As there were questions here, would you mind adding some comments
> so that the next reader won't make the same questions?
We have a patch (patch 5 of this series) adding some Documentation/ABI/
entries to specify the correct value for each entry.

> 
> > +
> > +	return count;
> > +}
> > +static DEVICE_ATTR_WO(ljca_trace_level);
> 
> [...]
> 
> > +static int ljca_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > +{
> > +	struct ljca_dev *dev;
> > +	struct usb_endpoint_descriptor *bulk_in, *bulk_out;
> > +	int ret;
> > +
> > +	/* allocate memory for our device state and initialize it */
> > +	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> 
> devm_kzalloc()
Got it. Thanks.

--
Thanks
Ye Xiang