From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.6 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_PASS,URIBL_BLOCKED,USER_IN_DEF_DKIM_WL autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id B0302C1B0F7 for ; Fri, 18 Jan 2019 21:25:09 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6211E2087E for ; Fri, 18 Jan 2019 21:25:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="DhF5wQw0" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729662AbfARVZH (ORCPT ); Fri, 18 Jan 2019 16:25:07 -0500 Received: from mail-oi1-f193.google.com ([209.85.167.193]:42446 "EHLO mail-oi1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729601AbfARVZH (ORCPT ); Fri, 18 Jan 2019 16:25:07 -0500 Received: by mail-oi1-f193.google.com with SMTP id w13so10072140oiw.9 for ; Fri, 18 Jan 2019 13:25:06 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=m62nHCbg2uZBmb5Zv3sRc+vOt+eT0C0LFTZYCHGPreY=; b=DhF5wQw0rgoMTJLOKz1bvBsoRmqezrygZWAseIfPPK3dCGD0tCMXMJxtS5drKsCrIJ z8usFlMWm+1RBNzy0kFdUd1/pQXn46I/+TjGcb1wJ6ClYO/8kiSxbUPQFCG0LKCabYne IEL7eY+eEK2xsdmpfLbj9T4QU+V+rPKVFzdECqRKzSArL36CrAnlJqlKJWcVYr0D+Cpd pMSP6SGSu9gEY6j+wUUAGQ4e9KGJRSWaz/QdsXBbsF/kP0Gb1iCzwtEdWhhCTZV14IMR jE5wWEpDfdYNegyE8ulYNG80eH3Ta3tC1AOz1/oUttgLqTBt2saz1hOXmS6Cqoz6kaet XArA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=m62nHCbg2uZBmb5Zv3sRc+vOt+eT0C0LFTZYCHGPreY=; b=TXWpJ6X9vjndTjTJpSbZwjC9zvtbhRxf0hGrtqZo6YX4OcMbJdZREwj4fXT3QrYq4C o2QgxszxFIWCNqZ3V8kmpe+iDTRofUciqtZi8o1+h2bP2dyF4JuNLTy115RfIxY+gAMR qjZt58pairw6K8TVdikFr/qEm4x4z+HOa+vcM8lv8hTPcWY+HmCVK8KMWbO9v709sOsg faXOBaO+FE36cDWKVgczs1Yu0x/xUttTDT3oGgUSeufuw6Tce0vPIq5p+6sALXtCznGb Xtje9aRHtJuE3W3cIeGoqaMHa3R37epwH8ikDQ34otbaMs04/kxUsTu7u7Yfbq6DV/Sg uqmA== X-Gm-Message-State: AJcUukd+ma7grxmxreFaxMLDhUHNGrIHyQv078yGihXpS/ijVCdY7f7r 7AhsB2kDjD9sjn+gzj2GjQ7ezJWXH6HLxKSfz9DfxwjNESQ= X-Google-Smtp-Source: ALg8bN7VGGrATgA0EQwFbxf0NaxN7CSU679NyZFc4gQWUo/u1XU2PgtP+zun9QsfLI1ABi6gr+wpQoC9D55Gl9b0igI= X-Received: by 2002:aca:844:: with SMTP id 65mr172654oii.333.1547846706049; Fri, 18 Jan 2019 13:25:06 -0800 (PST) MIME-Version: 1.0 References: <20190114220356.108916-1-ncrews@google.com> <20190114220356.108916-6-ncrews@google.com> <20190114222551.GC2589@piout.net> In-Reply-To: <20190114222551.GC2589@piout.net> From: Nick Crews Date: Fri, 18 Jan 2019 14:24:54 -0700 Message-ID: Subject: Re: [PATCH v2 5/9] platform/chrome: rtc: Add RTC driver for Wilco EC To: Alexandre Belloni Cc: linux-kernel@vger.kernel.org, groeck@chromium.org, sjg@chromium.org, dlaurie@chromium.org, Duncan Laurie , linux-rtc@vger.kernel.org, Enric Balletbo i Serra , Alessandro Zummo , Benson Leung Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Alexandre, thanks for taking the time to review this. I've responded to your comments inline below. I'll send out a new version of this patch soon. On Mon, Jan 14, 2019 at 3:26 PM Alexandre Belloni wrote: > > Hello, > > On 14/01/2019 15:03:52-0700, Nick Crews wrote: > > diff --git a/drivers/rtc/rtc-wilco-ec.c b/drivers/rtc/rtc-wilco-ec.c > > new file mode 100644 > > index 000000000000..d8ed791da82d > > --- /dev/null > > +++ b/drivers/rtc/rtc-wilco-ec.c > > @@ -0,0 +1,178 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * RTC interface for Wilco Embedded Controller with R/W abilities > > + * > > + * Copyright 2018 Google LLC > > + * > > + * The corresponding platform device is typically registered in > > + * drivers/platform/chrome/wilco_ec/core.c > > + */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +#define DRV_NAME "rtc-wilco-ec" > > Please get rid of that define. > Done. > > +int wilco_ec_rtc_read(struct device *dev, struct rtc_time *tm) > > +{ > > + struct wilco_ec_device *ec = dev_get_drvdata(dev->parent); > > + u8 param = EC_CMOS_TOD_READ; > > + struct ec_rtc_read rtc; > > + struct wilco_ec_message msg = { > > + .type = WILCO_EC_MSG_LEGACY, > > + .flags = WILCO_EC_FLAG_RAW_RESPONSE, > > + .command = EC_COMMAND_CMOS, > > + .request_data = ¶m, > > + .request_size = sizeof(param), > > + .response_data = &rtc, > > + .response_size = sizeof(rtc), > > + }; > > + struct rtc_time calc_tm; > > + unsigned long time; > > + int ret; > > + > > + ret = wilco_ec_mailbox(ec, &msg); > > + if (ret < 0) > > + return ret; > > + > > + tm->tm_sec = rtc.second; > > + tm->tm_min = rtc.minute; > > + tm->tm_hour = rtc.hour; > > + tm->tm_mday = rtc.day; > > + /* > > + * The RTC stores the month value as 1-12 but the kernel expects 0-11, > > + * so ensure invalid/zero month value from RTC is not converted to -1. > > I'm pretty sure this is not necessary and will certainly make the core > think the date is valid when it has obviously been corrupted. Done, I'll just use rtc.month - 1 always. > > > + */ > > + tm->tm_mon = rtc.month ? rtc.month - 1 : 0; > > + tm->tm_year = rtc.year + (rtc.century * 100) - 1900; > > + tm->tm_yday = rtc_year_days(tm->tm_mday, tm->tm_mon, tm->tm_year); > > + > > + /* Compute day of week */ > > + rtc_tm_to_time(tm, &time); > > + rtc_time_to_tm(time, &calc_tm); > > + tm->tm_wday = calc_tm.tm_wday; > > Do you really need an accurate wday? There are no userspace applications > that I know of that care and converting back and forth is quite > expensive. No, I'm not sure we do need this. Removing it. > > > + > > + return 0; > > +} > > + > > +int wilco_ec_rtc_write(struct device *dev, struct rtc_time *tm) > > +{ > > + struct wilco_ec_device *ec = dev_get_drvdata(dev->parent); > > + struct ec_rtc_write rtc; > > + struct wilco_ec_message msg = { > > + .type = WILCO_EC_MSG_LEGACY, > > + .flags = WILCO_EC_FLAG_RAW_RESPONSE, > > + .command = EC_COMMAND_CMOS, > > + .request_data = &rtc, > > + .request_size = sizeof(rtc), > > + }; > > + int year = tm->tm_year + 1900; > > + /* Convert from 0=Sunday to 0=Saturday for the EC */ > > + int wday = tm->tm_wday == 6 ? 0 : tm->tm_wday + 1; > > I'm not sure how useful it is to set an accurate wday when the RTC is > not able to give it back. This is actually needed. The RTC controls battery charging schedule, which varies by the day of the week, and it requires this to be set. > > > + int ret; > > + > > + rtc.param = EC_CMOS_TOD_WRITE; > > + rtc.century = bin2bcd(year / 100); > > + rtc.year = bin2bcd(year % 100); > > + rtc.month = bin2bcd(tm->tm_mon + 1); > > + rtc.day = bin2bcd(tm->tm_mday); > > + rtc.hour = bin2bcd(tm->tm_hour); > > + rtc.minute = bin2bcd(tm->tm_min); > > + rtc.second = bin2bcd(tm->tm_sec); > > + rtc.weekday = bin2bcd(wday); > > + > > + ret = wilco_ec_mailbox(ec, &msg); > > + if (ret < 0) > > + return ret; > > + > > + return 0; > > +} > > + > > +static const struct rtc_class_ops wilco_ec_rtc_ops = { > > + .read_time = wilco_ec_rtc_read, > > + .set_time = wilco_ec_rtc_write, > > +}; > > + > > +static int wilco_ec_rtc_probe(struct platform_device *pdev) > > +{ > > + struct rtc_device *rtc = devm_rtc_device_register(&pdev->dev, > > + DRV_NAME, > > + &wilco_ec_rtc_ops, > > + THIS_MODULE); > > Please use devm_rtc_allocate_device() and set rtc->range_min and > rtc->range_max before registering with rtc_register_device(). Done. > > > + if (IS_ERR(rtc)) > > + return PTR_ERR(rtc); > > + > > + return 0; > > +} > > + > > +static struct platform_driver wilco_ec_rtc_driver = { > > + .driver = { > > + .name = DRV_NAME, > > + }, > > + .probe = wilco_ec_rtc_probe, > > +}; > > + > > +module_platform_driver(wilco_ec_rtc_driver); > > + > > +MODULE_ALIAS("platform:" DRV_NAME); > > +MODULE_AUTHOR("Nick Crews "); > > +MODULE_LICENSE("GPL v2"); > > +MODULE_DESCRIPTION("Wilco EC RTC driver"); > > -- > > 2.20.1.97.g81188d93c3-goog > > > > -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com Cheers, Nick Crews