Re: [PATCH 3/3] platform/chrome: mfd/cros_ec_dev: Add sysfs entry to set keyboard wake lid angle

From: Enric Balletbo i Serra
Date: Wed Feb 21 2018 - 11:38:59 EST


Hi Andy,

On 21/02/18 16:16, Andy Shevchenko wrote:
> On Wed, Feb 21, 2018 at 4:50 PM, Enric Balletbo i Serra
> <enric.balletbo@xxxxxxxxxxxxx> wrote:
>> From: Gwendal Grignou <gwendal@xxxxxxxxxxxx>
>>
>> This adds a sysfs attribute (/sys/class/chromeos/cros_ec/kb_wake_angle)
>> used to set and get the keyboard wake lid angle. This attribute is
>> present only if 2 accelerometers are controlled by the EC.
>>
>> This patch also moves the cros_ec features check before the device is
>> added so the features map obtained from the EC is ready on time.
>
>> +/* Keyboard wake angle control */
>> +static ssize_t show_kb_wake_angle(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct ec_response_motion_sense *resp;
>> + struct ec_params_motion_sense *param;
>> + struct cros_ec_command *msg;
>> + int ret;
>
>> + struct cros_ec_dev *ec = container_of(
>> + dev, struct cros_ec_dev, class_dev);
>
> First of all, do not split lines like this.
> Second, consider just to add a preparatory patch which introduces
>
> #define to_cros_ec_dev(dev) container_of(dev, struct cros_ec_dev, class_dev)
>
> and use it here.
>
>> +
>> + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> + if (!msg)
>> + return -ENOMEM;
>> +
>> + param = (struct ec_params_motion_sense *)msg->data;
>> + msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>> + msg->version = 2;
>> + param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
>> + param->kb_wake_angle.data = EC_MOTION_SENSE_NO_VALUE;
>> + msg->outsize = sizeof(*param);
>> + msg->insize = sizeof(*resp);
>> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> + if (ret < 0)
>> + goto exit;
>> + resp = (struct ec_response_motion_sense *)msg->data;
>
>> + ret = scnprintf(buf, PAGE_SIZE, "%d\n",
>> + resp->kb_wake_angle.ret);
>
> Looks like one line.
>
>> +exit:
>> + kfree(msg);
>> + return ret;
>> +}
>> +
>> +static ssize_t store_kb_wake_angle(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct ec_params_motion_sense *param;
>> + struct cros_ec_command *msg;
>> + int ret;
>> + struct cros_ec_dev *ec = container_of(
>> + dev, struct cros_ec_dev, class_dev);
>> + u16 angle;
>> +
>> + ret = kstrtou16(buf, 0, &angle);
>> + if (ret)
>> + return ret;
>> +
>> + msg = kmalloc(sizeof(*msg) + EC_HOST_PARAM_SIZE, GFP_KERNEL);
>> + if (!msg)
>> + return -ENOMEM;
>> +
>> + param = (struct ec_params_motion_sense *)msg->data;
>> + msg->command = EC_CMD_MOTION_SENSE_CMD + ec->cmd_offset;
>> + msg->version = 2;
>> + param->cmd = MOTIONSENSE_CMD_KB_WAKE_ANGLE;
>> + param->kb_wake_angle.data = angle;
>> + msg->outsize = sizeof(*param);
>> + msg->insize = sizeof(struct ec_response_motion_sense);
>> + ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
>> + kfree(msg);
>> + if (ret < 0)
>> + return ret;
>> + return count;
>> +}
>> +
>> /* Module initialization */
>>
>> static DEVICE_ATTR(reboot, S_IWUSR | S_IRUGO, show_ec_reboot, store_ec_reboot);
>> static DEVICE_ATTR(version, S_IRUGO, show_ec_version, NULL);
>> static DEVICE_ATTR(flashinfo, S_IRUGO, show_ec_flashinfo, NULL);
>> +static DEVICE_ATTR(kb_wake_angle, S_IWUSR | S_IRUGO, show_kb_wake_angle,
>> + store_kb_wake_angle);
>
> Consider to switch to
>
> DEVICE_ATTR_RO()
> DEVICE_ATTR_RW()
>

I'll send a second version soon, thanks for the review.

Regards,
Enric