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=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 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 536F2C432BE for ; Thu, 22 Jul 2021 23:18:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 2E43860EB6 for ; Thu, 22 Jul 2021 23:18:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232620AbhGVWh0 (ORCPT ); Thu, 22 Jul 2021 18:37:26 -0400 Received: from foss.arm.com ([217.140.110.172]:35088 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232024AbhGVWhZ (ORCPT ); Thu, 22 Jul 2021 18:37:25 -0400 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4E767106F; Thu, 22 Jul 2021 16:17:59 -0700 (PDT) Received: from slackpad.fritz.box (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5BC713F694; Thu, 22 Jul 2021 16:17:57 -0700 (PDT) Date: Fri, 23 Jul 2021 00:17:21 +0100 From: Andre Przywara To: Maxime Ripard Cc: Chen-Yu Tsai , Jernej Skrabec , Rob Herring , Icenowy Zheng , Samuel Holland , linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, Ondrej Jirman , Alessandro Zummo , Alexandre Belloni , linux-rtc@vger.kernel.org Subject: Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs Message-ID: <20210723001721.0bb02cf2@slackpad.fritz.box> In-Reply-To: <20210616091431.6tm3zdf77p2x3upc@gilmour> References: <20210615110636.23403-1-andre.przywara@arm.com> <20210615110636.23403-7-andre.przywara@arm.com> <20210616091431.6tm3zdf77p2x3upc@gilmour> Organization: Arm Ltd. X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 16 Jun 2021 11:14:31 +0200 Maxime Ripard wrote: Hi Maxime, > On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote: > > Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack > > a pin for an external 32768 Hz oscillator. As a consequence, this LOSC > > can't be selected as the RTC clock source, and we must rely on the > > internal RC oscillator. > > To allow additions of clocks to the RTC node, add a feature bit to ignore > > any provided clocks for now (the current code would think this is the > > external LOSC). Later DTs and code can then for instance add the PLL > > based clock input, and older kernel won't get confused. > > > > Signed-off-by: Andre Przywara > > Honestly, I don't really know if it's worth it at this point. > > If we sums this up: > > - The RTC has 2 features that we use, mostly centered around 2 > registers set plus a global one > > - Those 2 features are programmed in a completely different way > > - Even the common part is different, given the discussion around the > clocks that we have. > > What is there to share in that driver aside from the probe, and maybe > the interrupt handling? Instead of complicating this further with more > special case that you were (rightfully) complaining about, shouldn't we > just acknowledge the fact that it's a completely separate design and > should be treated as such, with a completely separate driver? So I had a look, and I don't think it justifies a separate driver: - Indeed it looks like the core functionality is different, but there are a lot of commonalities, with all the RTC and driver boilerplate, register offsets, and also the special access pattern (rtc_wait and rtc_setaie). - The actual difference is really in the way the *date* is stored (the time is still in 24h H/M/S format), and the missing LOSC input clock - which is already optional for existing devices. The two patches just make this obvious, by using if() statements at the parts where they differ. So we would end up with possibly some shared .c file, and two driver front-end files, which I am not sure is really worth it. Next I thought about providing separate rtc_class_ops, but even they share a lot of code, so they would be possibly be calling a shared function each. I don't think that is really better. If you dislike the rather large if/else branches in the previous two patches, I could move that out into separate functions, but I feel this is more code, for no real benefit. So for now I am tempted to keep it shared. I think Samuel had ideas for bigger changes in the clock part, at which point we could revisit this decision - for instance keep the RTC part (still quite similar) mostly in a shared file, while modelling the clocks in separate files - in a more "common clock" style for the new SoCs. Feel free to disagree, but when I tried to actually separate the drivers it just felt wrong. Cheers, Andre 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.7 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_2 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 38915C4338F for ; Thu, 22 Jul 2021 23:19:55 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E030D60E8F for ; Thu, 22 Jul 2021 23:19:54 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E030D60E8F Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=arm.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LbVanY731n2hSPYlVxP6kqiiDY6p5X7qKnUAF3Fncvk=; b=qvoO8t3ieLf/Nx kmuL4pfe6ckcYAUU0iRVUejj60HxOEp7s1bCpF2gWO32X/Olzh/36YntAY6I1v9ZFMxvrAMvuZ3MC VlLPXve1CEAimvJy8phUfdHt24lhV5TiUZx2hk9CF23/QmSdcMkcIK/x4WGiH5lS+pX7cN2cReDvB 13pmJB1KRvojgwSjV9JwlRqK79Z4J5JIcWK1XZK7DrBCaQhLZWBS9r/PeUxCjLVmMvbKkvvvFfqaN icPoQ8cCz00uJCvWfVWUN7+668t5dmdoCfX5vU1XaD3zRcYK48KoGr4gaL+hggprN2I25z/OkQtvT 5QnfJNSauE3+8sr9J+Ww==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m6hxA-002w5x-47; Thu, 22 Jul 2021 23:18:08 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1m6hx6-002w5V-7B for linux-arm-kernel@lists.infradead.org; Thu, 22 Jul 2021 23:18:05 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4E767106F; Thu, 22 Jul 2021 16:17:59 -0700 (PDT) Received: from slackpad.fritz.box (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 5BC713F694; Thu, 22 Jul 2021 16:17:57 -0700 (PDT) Date: Fri, 23 Jul 2021 00:17:21 +0100 From: Andre Przywara To: Maxime Ripard Cc: Chen-Yu Tsai , Jernej Skrabec , Rob Herring , Icenowy Zheng , Samuel Holland , linux-arm-kernel@lists.infradead.org, linux-sunxi@googlegroups.com, linux-sunxi@lists.linux.dev, linux-kernel@vger.kernel.org, Ondrej Jirman , Alessandro Zummo , Alexandre Belloni , linux-rtc@vger.kernel.org Subject: Re: [PATCH v7 06/19] rtc: sun6i: Add support for RTCs without external LOSCs Message-ID: <20210723001721.0bb02cf2@slackpad.fritz.box> In-Reply-To: <20210616091431.6tm3zdf77p2x3upc@gilmour> References: <20210615110636.23403-1-andre.przywara@arm.com> <20210615110636.23403-7-andre.przywara@arm.com> <20210616091431.6tm3zdf77p2x3upc@gilmour> Organization: Arm Ltd. X-Mailer: Claws Mail 3.17.1 (GTK+ 2.24.31; x86_64-slackware-linux-gnu) MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210722_161804_415685_8EAB3ADD X-CRM114-Status: GOOD ( 35.20 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 16 Jun 2021 11:14:31 +0200 Maxime Ripard wrote: Hi Maxime, > On Tue, Jun 15, 2021 at 12:06:23PM +0100, Andre Przywara wrote: > > Some newer Allwinner RTCs (for instance the one in the H616 SoC) lack > > a pin for an external 32768 Hz oscillator. As a consequence, this LOSC > > can't be selected as the RTC clock source, and we must rely on the > > internal RC oscillator. > > To allow additions of clocks to the RTC node, add a feature bit to ignore > > any provided clocks for now (the current code would think this is the > > external LOSC). Later DTs and code can then for instance add the PLL > > based clock input, and older kernel won't get confused. > > > > Signed-off-by: Andre Przywara > > Honestly, I don't really know if it's worth it at this point. > > If we sums this up: > > - The RTC has 2 features that we use, mostly centered around 2 > registers set plus a global one > > - Those 2 features are programmed in a completely different way > > - Even the common part is different, given the discussion around the > clocks that we have. > > What is there to share in that driver aside from the probe, and maybe > the interrupt handling? Instead of complicating this further with more > special case that you were (rightfully) complaining about, shouldn't we > just acknowledge the fact that it's a completely separate design and > should be treated as such, with a completely separate driver? So I had a look, and I don't think it justifies a separate driver: - Indeed it looks like the core functionality is different, but there are a lot of commonalities, with all the RTC and driver boilerplate, register offsets, and also the special access pattern (rtc_wait and rtc_setaie). - The actual difference is really in the way the *date* is stored (the time is still in 24h H/M/S format), and the missing LOSC input clock - which is already optional for existing devices. The two patches just make this obvious, by using if() statements at the parts where they differ. So we would end up with possibly some shared .c file, and two driver front-end files, which I am not sure is really worth it. Next I thought about providing separate rtc_class_ops, but even they share a lot of code, so they would be possibly be calling a shared function each. I don't think that is really better. If you dislike the rather large if/else branches in the previous two patches, I could move that out into separate functions, but I feel this is more code, for no real benefit. So for now I am tempted to keep it shared. I think Samuel had ideas for bigger changes in the clock part, at which point we could revisit this decision - for instance keep the RTC part (still quite similar) mostly in a shared file, while modelling the clocks in separate files - in a more "common clock" style for the new SoCs. Feel free to disagree, but when I tried to actually separate the drivers it just felt wrong. Cheers, Andre _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel