Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1753284AbdHKNH7 (ORCPT <rfc822;w@1wt.eu>);
        Fri, 11 Aug 2017 09:07:59 -0400
Received: from bhuna.collabora.co.uk ([46.235.227.227]:48302 "EHLO
        bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1753160AbdHKNH4 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 11 Aug 2017 09:07:56 -0400
Subject: Re: [PATCH 7/8] platform/chrome: cros_ec: Add sysfs entry to set
 keyboard wake lid angle
To: Benson Leung <bleung@google.com>
Cc: Benson Leung <bleung@chromium.org>, Lee Jones <lee.jones@linaro.org>,
        linux-kernel@vger.kernel.org, gwendal@chromium.org
References: <1502403410-5233-1-git-send-email-thierry.escande@collabora.com>
 <1502403410-5233-8-git-send-email-thierry.escande@collabora.com>
 <20170811052947.GA155834@decatoncale.mtv.corp.google.com>
From: Thierry Escande <thierry.escande@collabora.com>
Message-ID: <016d7dc9-6568-0dec-8375-4ddbeea40167@collabora.com>
Date: Fri, 11 Aug 2017 15:07:52 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <20170811052947.GA155834@decatoncale.mtv.corp.google.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4488
Lines: 141

Hi Benson,

On 11/08/2017 07:29, Benson Leung wrote:
<snip>
>> +/* 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);
>> +
>> +	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);
> 
> Original CHROMIUM commit uses cros_ec_cmd_xfer here.
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/changes/15/456415/5/drivers/platform/chrome/cros_ec_sysfs.c#303
> 
> 
>> +	if (ret < 0)
>> +		return ret;
> 
> Original commit also has this check:
> 	if (msg->result != EC_RES_SUCCESS)
> 		return scnprintf(buf, PAGE_SIZE, "ERROR: EC returned %d\n",
> 				 msg->result);
> 
> Now, in the CHROMIUM commits, Gwendal removes this in the next patch, which
> is "[PATCH 8/8] platform/chrome: cros_ec: sysfs: Modify error handling" in your
> series, but can we try to keep these as close to the originals as possible?

Ok, sure. With the original patches I was pretty sure to get something 
like "hey, you modify something you've just added on the previous patch!" :)

I'll repost the whole series even if you've applied 2 of them already.

Regards,
  Thierry

> 
>> +	resp = (struct ec_response_motion_sense *)msg->data;
>> +	return scnprintf(buf, PAGE_SIZE, "%d\n",
>> +			resp->kb_wake_angle.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);
>> +	uint16_t 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);
>> +	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);
>>   
>>   static struct attribute *__ec_attrs[] = {
>> +	&dev_attr_kb_wake_angle.attr,
>>   	&dev_attr_reboot.attr,
>>   	&dev_attr_version.attr,
>>   	&dev_attr_flashinfo.attr,
>>   	NULL,
>>   };
>>   
>> +static umode_t cros_ec_ctrl_visible(struct kobject *kobj,
>> +				    struct attribute *a, int n)
>> +{
>> +	struct device *dev = container_of(kobj, struct device, kobj);
>> +	struct cros_ec_dev *ec = container_of(dev, struct cros_ec_dev,
>> +					      class_dev);
>> +
>> +	if (a == &dev_attr_kb_wake_angle.attr && !ec->has_kb_wake_angle)
>> +		return 0;
>> +
>> +	return a->mode;
>> +}
>> +
>>   struct attribute_group cros_ec_attr_group = {
>>   	.attrs = __ec_attrs,
>> +	.is_visible = cros_ec_ctrl_visible,
>>   };
>>   
>> diff --git a/include/linux/mfd/cros_ec.h b/include/linux/mfd/cros_ec.h
>> index 4e887ba..350b8a4 100644
>> --- a/include/linux/mfd/cros_ec.h
>> +++ b/include/linux/mfd/cros_ec.h
>> @@ -191,6 +191,7 @@ struct cros_ec_dev {
>>   	struct cros_ec_device *ec_dev;
>>   	struct device *dev;
>>   	struct cros_ec_debugfs *debug_info;
>> +	bool has_kb_wake_angle;
>>   	u16 cmd_offset;
>>   	u32 features[2];
>>   };
>> -- 
>> 2.7.4
>>
>