Date:   Fri, 10 Mar 2023 01:42:36 +0800
From:   "Ye, Xiang" <xiang.ye@intel.com>
To:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     Andi Shyti <andi.shyti@kernel.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 v4 1/5] mfd: Add support for Intel LJCA device
Message-ID: <ZAoajF6iqfLRO1+w@ye-NUC7i7DNHE>
References: <20230309071100.2856899-1-xiang.ye@intel.com>
 <20230309071100.2856899-2-xiang.ye@intel.com>
 <ZAmQOIh/71rY4Pa4@kroah.com>
 <ZAmngB84ty1flD9K@ye-NUC7i7DNHE>
 <ZAmpse14Evvrfa/f@kroah.com>
 <ZAmvocpy68qurCvt@intel.intel>
 <ZAn/L8Dg/Ehx9bMo@ye-NUC7i7DNHE>
 <ZAoCNR/iAv3IOJ6D@kroah.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <ZAoCNR/iAv3IOJ6D@kroah.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?aXcOD9++T1/mGdemfdnfYNP8OIi9opL6t/Xy8qtJSdLUcAADC88L+V/So4xi?=
 =?us-ascii?Q?Vz37h/QyOg+HddFb7moGCmArD6FZWse1jPgvdEja4lJC/2apOqbCsgIUHE9m?=
 =?us-ascii?Q?5umBDKoVGcIrXQageWhG8mWgJHqy+feqzM+wEj/ZZcW/GXKSZ9sEfPVzR8Sl?=
 =?us-ascii?Q?keLu4BAUmGaNPESU2G3nM1jXw3KyHE1OjWMp3B7noldtoDMTC3+hZ1gWTZk3?=
 =?us-ascii?Q?rtLWGfQ18KoM2Ve1ttx+zRC1/doYT88l7JmIXev8XCF2QxcRiMzmm6lp65g+?=
 =?us-ascii?Q?qTpB/XW1EPerUwkDiYygb+3ID3tNWk3wft/eI81tI/x9GrExKEBjZG8AlIhJ?=
 =?us-ascii?Q?jIOPeXL0I64xvsPwdkqjw3eqmDTh1GL+jjOsOTADbsVYwHq/YR3i+UzPvayh?=
 =?us-ascii?Q?D7MWYU0fEzY0aOhy3uBszc/9/DJKOBNzVMO1OrMWvDkmUMkVzEAsVHFheMzo?=
 =?us-ascii?Q?AFd3EVe63YB4ULqWfVsL5ZE+l+HT9cSq+JrBw5NTAqpVHVpXiLevIZ85xUV8?=
 =?us-ascii?Q?4titrrkxClKe0YWzPdXbcqThbVKwpS0KKk7LkkL4QjqWBgMIlIrQ84P5oq5E?=
 =?us-ascii?Q?jLpxRAIeC7NyK4jY3GIkG3Od9D5sZpyklfOJTWoTH526esMaFWiaHMdkzvJE?=
 =?us-ascii?Q?xUy6Z8YYZLSCxL4eBfMvsF6A+ayYXCFWAnNgP/fv78O5TLs89KpwPI5dDX+9?=
 =?us-ascii?Q?AbzhEAYI/rOCgBusB6szDz17B79QNZ1IFaMVJaLA9VhAOkWZ8KKCpxcTQiFB?=
 =?us-ascii?Q?x0hRmI1NFPbNnPSK9hEg0Fdr0wIEMNb+yARVZfz7+uYfsfrKzwhMsovTDSqW?=
 =?us-ascii?Q?zJwvCzM9G0uupYvM9zEA1wR54NCo108DI5w3skqB5i8BXMrfNxAOh3MRySiK?=
 =?us-ascii?Q?f9xJWyicTJD1/o/IURLEj1cYgabMYnOyfjMqCVzGm0hnCWoBJGH8zs9scXGE?=
 =?us-ascii?Q?CNWKTO7qt3L575hON9Ky9+iSrka6UFdgJlHAAMUcfNPLzliTlIDbSrFsiuoI?=
 =?us-ascii?Q?wu6qTF4mMHPg5kPxwjHkm3Pk7zeh6PD20LbEIP7uMpYIie9UT4WBVGfN1WGO?=
 =?us-ascii?Q?LHhuj1IMBhlVxots78sFUODX4CZ07Yd17/WtbYTr89WRCmF/zKcEiGGaHni0?=
 =?us-ascii?Q?38VroGfp3m7Sca2TXMQ3bHzf0CutT8Oy6gofZl014IiEJp0zuKqxV4C7MxYx?=
 =?us-ascii?Q?BcaNapmI5vScO/OowYHasv6IfIMcMAPEEd6oQopgrAzdSYqaSchO36orUDQJ?=
 =?us-ascii?Q?YwjjEaZVgtvA8rxS0bV6uS9X3PFnNOjMp46Z/q3mN2Qa2U5tNsp3Pp38HZqU?=
 =?us-ascii?Q?kL6zXyEZHFu0ernw4GyR/yyBjJTRk7g3bmBW8tP0qguVXYNtddKwcLwFjBSO?=
 =?us-ascii?Q?vuRxuhfkvyRTCG1vO8UCy+eGoWdU2PiRzOy2lxGhSVzPMC/TQT8vVy8dKkeL?=
 =?us-ascii?Q?b4eLxBz3XwAFNp1HbZj2lcHiOxrTHTB6ZmiJ98gudMWXcRsO2cvoEPfCQhov?=
 =?us-ascii?Q?Rq9hcDh/reVLIAU7vil8qRCJ5Vea/XsXgoqUza5ffN1ZcT1TfbgVI8jZnX9b?=
 =?us-ascii?Q?fV+6gub3x/y0YPt/7BAjuKC6v+Uv1683M/LwbS+y?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b4c09fc6-fd20-4881-f075-08db20c5b0f4
X-MS-Exchange-CrossTenant-AuthSource: DM5PR11MB1418.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Mar 2023 17:42:45.4889
 (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: KketjV/46VBIJ+7nrIkAeBt3nlDbwV0WYLNxX4XjkGfM2Y0xKgfmzg8RJMiNrwbg91/jtnQuX2fCfUPNn3SgwQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7461
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thu, Mar 09, 2023 at 04:58:45PM +0100, Greg Kroah-Hartman wrote:
> On Thu, Mar 09, 2023 at 11:45:51PM +0800, Ye, Xiang wrote:
> > On Thu, Mar 09, 2023 at 11:06:25AM +0100, Andi Shyti wrote:
> > > On Thu, Mar 09, 2023 at 10:41:05AM +0100, Greg Kroah-Hartman wrote:
> > > > On Thu, Mar 09, 2023 at 05:31:44PM +0800, Ye, Xiang wrote:
> > > > > On Thu, Mar 09, 2023 at 08:52:24AM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Thu, Mar 09, 2023 at 03:10:56PM +0800, Ye Xiang wrote:
> > > > > > > +static int ljca_mng_get_version(struct ljca_stub *stub, char *buf)
> > > > > > > +{
> > > > > > > +	struct fw_version version = {};
> > > > > > > +	unsigned int len = sizeof(version);
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	ret = ljca_stub_write(stub, LJCA_MNG_GET_VERSION, NULL, 0, &version, &len, true,
> > > > > > > +			      LJCA_USB_WRITE_ACK_TIMEOUT_MS);
> > > > > > > +	if (ret)
> > > > > > > +		return ret;
> > > > > > > +
> > > > > > > +	if (len != sizeof(version)) {
> > > > > > > +		dev_err(&stub->intf->dev, "get version failed, len:%d\n", len);
> > > > > > > +		return -EINVAL;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return sysfs_emit(buf, "%d.%d.%d.%d\n", version.major, version.minor,
> > > > > > > +			  le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > > > > > > +}
> > > > > > 
> > > > > > You have sysfs files, yet no Documentation/ABI/ entries?  That's not
> > > > > > allowed, you know this :(
> > > > > The Documentation/ABI/ entries is added for the sysfs on patch 5 of this series.
> > > > > https://patchwork.kernel.org/project/linux-usb/patch/20230309071100.2856899-6-xiang.ye@intel.com/
> > > > 
> > > > Ah, missed that, sorry.
> > > > 
> > > > > > 
> > > > > > > +static ssize_t cmd_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);
> > > > > > > +	struct ljca_stub *diag_stub = ljca_stub_find(ljca_dev, LJCA_DIAG_STUB);
> > > > > > > +
> > > > > > > +	if (sysfs_streq(buf, "dfu"))
> > > > > > > +		ljca_mng_set_dfu_mode(mng_stub);
> > > > > > > +	else if (sysfs_streq(buf, "debug"))
> > > > > > > +		ljca_diag_set_trace_level(diag_stub, 3);
> > > > > > 
> > > > > > Sorry, but no, you can't do this in a sysfs file.
> > > > > Do you mean that we can't use sysfs to send "debug" command to device?
> > > > 
> > > > That is correct, use the kernel-wide debugging facilities that we have
> > > > for this NEVER create your own custom interface just for one tiny
> > > > driver, that is not allowed.
> > > > 
> > > > > Could you provide some detail or hints?
> > > > 
> > > > dev_dbg().
> > But, this command is sent to SET LJCA Firmware logging level.
> 
> What command?
ljca_diag_set_trace_level send LJCA_DIAG_SET_TRACE_LEVEL command to LJCA
FW to set FW log level.
> 
> This isn't documented at all, sorry, I don't understand what you are
> trying to do here.
Sorry for the doc missing. will try to add more details on comments and
Documentation/ABI/ entries.

> 
> > > I'm not sure this is the same thing, though, as it's not a drvier
> > > to user debug message.
> > > 
> > > Ye, can you please explain better what this command does? You are
> > > sending a LJCA_DIAG_SET_TRACE_LEVEL command to the device with a
> > > parameter "3" which has a meaining only for you :)
> > Sure, the LJCA_DIAG_SET_TRACE_LEVEL command is used to set LJCA FW
> > logging level. 3 means debug level for FW. It is used for LJCA FW
> > debugging: when FW got some issue, we can send debug level to FW
> > to make FW print degging log for analysis.
> 
> And where is that printed?  In the kernel log?  Somewhere else?  What
> does the firmware have to do with any of this?
The firmware log was printed through UART port of LJCA device to another
computer just for debugging purposes. After sending LJCA_DIAG_SET_TRACE_LEVEL
command to LJCA device, it will print logging according to the log level sent
before.

--
Thanks
Ye Xiang