On 2020-09-23 3:34 PM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 02:25:03PM +0200, Cezary Rojewski wrote:
>> Add sysfs entries for displaying version of FW currently in use as well
>> as dumping full FW information including build and log-providers hashes.
...
>> #include "registers.h"
>
>> struct catpt_dev;
>> +extern const struct attribute_group *catpt_attr_groups[];
>
> Grouping these are not okay — they are different by meaning, just put blank
> line in between.
>
Sure, ack.
...
>> +#include <linux/pm_runtime.h>
>> +#include "core.h"
>> +
>> +static ssize_t fw_version_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct catpt_dev *cdev = dev_get_drvdata(dev);
>> + struct catpt_fw_version version;
>> + int ret;
>
>> + pm_runtime_get_sync(cdev->dev);
>> +
>> + ret = catpt_ipc_get_fw_version(cdev, &version);
>> +
>> + pm_runtime_mark_last_busy(cdev->dev);
>> + pm_runtime_put_autosuspend(cdev->dev);
>
> Is it subject to change at run-time?
>
No it does not. However, I do not intent to have the fw_version occupy
memory for device's drvdata (i.e. send the IPC internally and store it
inside struct catpt_dev). So, I'd rather wake the device, dump the
version and leave the bytes alone.
One could think about statics but then again, how many times this sysfs
file is going to get read anyway?
It's more readable and simple this way, losing nothing in return TBH.
>> + if (ret)
>> + return CATPT_IPC_ERROR(ret);
>> +
>> + return sprintf(buf, "%d.%d.%d.%d\n", version.type, version.major,
>> + version.minor, version.build);
>> +}
>
>> +
>
> This blank line is not needed.
>
Ack.
>> +static DEVICE_ATTR_RO(fw_version);
>> +
>> +static ssize_t fw_info_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct catpt_dev *cdev = dev_get_drvdata(dev);
>> +
>> + return sprintf(buf, "%s\n", cdev->ipc.config.fw_info);
>> +}
>
>> +
>
> This blank line is not needed.
>
Ack.
