Received: by 2002:a05:7412:b795:b0:e2:908c:2ebd with SMTP id iv21csp516020rdb; Thu, 2 Nov 2023 09:57:47 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGNg7OZgFQmvup85lpdT3gIRLmkHGKq9QNKPWC+ZEwfo+iIQ02ZJ9gDhAzzfd3qtMvL4Zow X-Received: by 2002:a17:902:db11:b0:1cc:5a65:c535 with SMTP id m17-20020a170902db1100b001cc5a65c535mr11887603plx.6.1698944266779; Thu, 02 Nov 2023 09:57:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1698944266; cv=none; d=google.com; s=arc-20160816; b=IpTq2qY1TpuhwYNfH5/8bZxVSvQ1pRnMIW0wDo0BMndGgVaH1r9x/6w4rdPVuvQOoy +c4zJnHIxUpTo+VmmwGBdVUhVhikeKdR1glP+xCyF9RjHlKGCxziyuxHq1/nqH/OMXSG +PRQotUQFvA6koDSwtpGHPLX21lBCw2ZkGkvMtay1zsV0F7GWXQcWmZKe2ZKQReMYAxE QmJbQLUU9ThvL6zn+eGUxVMK4ivZwyzydVHRgIPwNsawAoeTXkOVMuu2qJxlMFYZTs2I UCsClwMb5woBd+awJiYWlF5hgmEgC42eyihoAdYFwokjROzec7rf61is84cIe09BMU5b kx1g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date:dkim-signature; bh=NVz3G4wL1rSrk0NYhI8ac9/fbtgh/W+2BcN82pui1bE=; fh=T2xVOedZ+RvQxnLqEy6qBCckCky24a5WCAyKvCggNK0=; b=FyhCDWu7W/FxNV0pAtNhYS7gH3IBLkNwisC828AhJA1gNF5DN1bqPwWCcD0F5S3hFC IRmjW2SIzG1t4vbHEeMsC+EXNboAMrILtdBKSX12UZW1+X//PhiHPp44inexjEdl81O4 R/zrJ8nwpSfPTr5ihUm7O3wZKRHaqhHUGNqL/i8YUoqs4/odJ73b5VUbB0EpBNGPAXNL Qrl/qQosshLWvvGCFEZP2nQ/5BBl/UWu1Iw2eD3TEQw6q0Z9eJSV6ac2zCTjq7oROuvM fXmnrqvqQExK7c7HKqDiQRnuqEFsPOqeonO5Jy7f4HeNT0304ZXgOMuOwLXT+/hG4U/J Rl0g== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=YS1ZbF5v; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Return-Path: Received: from fry.vger.email (fry.vger.email. [23.128.96.38]) by mx.google.com with ESMTPS id n4-20020a170902d2c400b001bbfbe6bf3esi137272plc.504.2023.11.02.09.57.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Nov 2023 09:57:46 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38; Authentication-Results: mx.google.com; dkim=pass header.i=@bootlin.com header.s=gm1 header.b=YS1ZbF5v; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=bootlin.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 6CF97837D956; Thu, 2 Nov 2023 09:57:38 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1377101AbjKBQ51 (ORCPT + 99 others); Thu, 2 Nov 2023 12:57:27 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:32908 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1347626AbjKBQ50 (ORCPT ); Thu, 2 Nov 2023 12:57:26 -0400 Received: from relay2-d.mail.gandi.net (relay2-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::222]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B59FE181; Thu, 2 Nov 2023 09:57:16 -0700 (PDT) Received: by mail.gandi.net (Postfix) with ESMTPSA id 6356B40004; Thu, 2 Nov 2023 16:57:14 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bootlin.com; s=gm1; t=1698944235; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NVz3G4wL1rSrk0NYhI8ac9/fbtgh/W+2BcN82pui1bE=; b=YS1ZbF5vPsUZVEV1gWd7BpaTy6XlyFUooET0lTUZk7BfYXMwO8LXKymhq1jnx1qLePrYXv k18yCMxjUwyGi+jApbW4A2AzaOZq7khKIOfIRW5S1Tqr9UEHycwe+r50v4/3CsuCro/K64 lL4CeDsH9Sw3nQZphTqfBf/rLgJnFAQIkINRDnyY5NnUkS5x3nUiMOIc7+BLwfE6pt3H41 SZunbBcy6SXiY9RCxwtx+6+8gQ8wYvl6TqSN3y318g0tocXPZz05apbwa2lX0Hbx+k/R7U iwqO4RDpVYJNbvT1/hRFLUNha1Q9wcqDlr0pWsKAlPC0yVmpSaeJJwn9KOrheQ== Date: Thu, 2 Nov 2023 17:57:13 +0100 From: Alexandre Belloni To: "Miclaus, Antoniu" Cc: Alessandro Zummo , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Jean Delvare , Guenter Roeck , "linux-rtc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "linux-hwmon@vger.kernel.org" Subject: Re: [PATCH v4 2/2] rtc: max31335: add driver support Message-ID: <20231102165713296ca50b@mail.local> References: <20231101094835.51031-1-antoniu.miclaus@analog.com> <20231101094835.51031-2-antoniu.miclaus@analog.com> <202311012223422e3387b3@mail.local> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: X-GND-Sasl: alexandre.belloni@bootlin.com X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Thu, 02 Nov 2023 09:57:38 -0700 (PDT) On 02/11/2023 13:44:16+0000, Miclaus, Antoniu wrote: > > > On 01/11/2023 11:48:14+0200, Antoniu Miclaus wrote: > > > +static int max31335_get_hour(u8 hour_reg) > > > +{ > > > + int hour; > > > + > > > + /* 24Hr mode */ > > > + if (!FIELD_GET(MAX31335_HOURS_F_24_12, hour_reg)) > > > + return bcd2bin(hour_reg & 0x3f); > > > + > > > + /* 12Hr mode */ > > > + hour = bcd2bin(hour_reg & 0x1f); > > > + if (hour == 12) > > > + hour = 0; > > > + > > > > Do you really need to support 12h mode? > > Is is a feature supported by the part, so I think is nice to have. > Is anything on the system going to use it? This driver is not setting 12h time so if there is no other component in the system accessing the time, there is no chance this is going to be used. Dead code is not nice to maintain. > > > > > + if (FIELD_GET(MAX31335_HOURS_HR_20_AM_PM, hour_reg)) > > > + hour += 12; > > > + > > > + return hour; > > > +} > > > + > > > +static int max31335_read_offset(struct device *dev, long *offset) > > > +{ > > > + struct max31335_data *max31335 = dev_get_drvdata(dev); > > > + u32 value; > > > + int ret; > > > + > > > + ret = regmap_read(max31335->regmap, MAX31335_AGING_OFFSET, > > &value); > > > + if (ret) > > > + return ret; > > > + > > > + *offset = value; > > > > This is super dubious, what is the unit of MAX31335_AGING_OFFSET ? > > > > There is not additional information on the AGING_OFFSET register (no > other offset registers). > I treated it as a raw value that user can write/read. Should I drop the > offset implementation? > The value exposed to userspace is in parts per billion. If you can't do the conversion, then you have to drop it. > > > + > > > + return 0; > > > + } > > > + > > > + if (diode) > > > + i = i + 4; > > > + else > > > + i = i + 1; > > > > Do you actually need to configure the trickle charger when there is > > nothing to charge? > > These are the options for the trickle chager: > MAX31335_TRICKLE_REG_TRICKLE bits > > 0x0: 3KΩ in series with a Schottky diode > 0x1: 3KΩ in series with a Schottky diode > 0x2: 6KΩ in series with a Schottky diode > 0x3: 11KΩ in series with a Schottky diode > 0x4: 3KΩ in series with a diode+Schottky diode > 0x5: 3KΩ in series with a diode+Schottky diode > 0x6: 6KΩ in series with a diode+Schottky diode > 0x7: 11KΩ in series with a diode+Schottky diode > Then you need a property to select with diodes you are going to use. The ABX80X supports something similar. > > > > > + > > > + return regmap_write(max31335->regmap, MAX31335_TRICKLE_REG, > > > + FIELD_PREP(MAX31335_TRICKLE_REG_TRICKLE, i) | > > > + MAX31335_TRICKLE_REG_EN_TRICKLE); > > > +} > > > + > > > +static int max31335_clkout_register(struct device *dev) > > > +{ > > > + struct max31335_data *max31335 = dev_get_drvdata(dev); > > > + int ret; > > > + > > > + if (!device_property_present(dev, "#clock-cells")) > > > + return 0; > > > > Is the clock output disabled by default? > > No, I will disable it. The CCF is responsible to disable the clock, else you will have a glitch in the clock at boot time which will break use cases. But for this to work, you will have to always register the clock. > > > > > > + > > > +static int max31335_probe(struct i2c_client *client) > > > +{ > > > + struct max31335_data *max31335; > > > + struct device *hwmon; > > > + int ret; > > > + > > > + max31335 = devm_kzalloc(&client->dev, sizeof(*max31335), > > GFP_KERNEL); > > > + if (!max31335) > > > + return -ENOMEM; > > > + > > > + max31335->regmap = devm_regmap_init_i2c(client, > > ®map_config); > > > + if (IS_ERR(max31335->regmap)) > > > + return PTR_ERR(max31335->regmap); > > > + > > > + i2c_set_clientdata(client, max31335); > > > + > > > + ret = regmap_write(max31335->regmap, MAX31335_RTC_RESET, 1); > > > + if (ret) > > > + return ret; > > > + > > > + ret = regmap_write(max31335->regmap, MAX31335_RTC_RESET, 0); > > > + if (ret) > > > + return ret; > > > > What does this register do? > > > > RTC Software Reset Register: > 0x0: Device is in normal mode. > 0x1: Resets the digital block and the I2C programmable registers except for > Timestamp/RAM registers and the SWRST bit. Oscillator is disabled. > > The bit doesn't clear itself. > Then you definitively don't want to do this, this will invalidate time and date as the oscillator is disabled and this renders the RTC useless. The whole point of the RTC is that it survives the reboot or shutdown of the system. -- Alexandre Belloni, co-owner and COO, Bootlin Embedded Linux and Kernel engineering https://bootlin.com