From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-dy1-f174.google.com (mail-dy1-f174.google.com [74.125.82.174]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 1E9331DF980 for ; Wed, 14 Jan 2026 00:47:44 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=74.125.82.174 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768351665; cv=none; b=t/bvh7fa3lQeHXZlvTPBpeDZft7pzrVbOCEkwly8/T63mWpqY5n36XfuuEZ3qOnmJiNiXzO6osPETuYDujj6sv1tNG/DqN/ZL3/sA8ErN8ZWIlGDVMyL3qwLAf44x8HVhmL25L8KZ3o4vT/Kx/REd9EBwth22TsHy+4z0oZVQJU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1768351665; c=relaxed/simple; bh=FiEQxWP0gBazySZVKdQMaxlFntTAJXhqfST8zZRauYQ=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=U4bgTkSkPBwcGx7vcrrxCPo95BMQ+O67UGrTd9p0w4J2ihZYxiA2l2+RsiZ8gZ5nasWM+6l7kIrLYGIfV9yaY5zunXE6a0ptBPSNI56MpNzKNemnGxWClFVioyyD6+ozFKU2DOmQ0KTc5Rh1V2jl7DTGE59/IwEduuA69WUP3OY= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com; spf=pass smtp.mailfrom=google.com; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b=hoGGEBWN; arc=none smtp.client-ip=74.125.82.174 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=google.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="hoGGEBWN" Received: by mail-dy1-f174.google.com with SMTP id 5a478bee46e88-2b453b17e41so218042eec.1 for ; Tue, 13 Jan 2026 16:47:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1768351663; x=1768956463; darn=vger.kernel.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=KrG7y4Xlsrv3rqpWtCTovQ72S57auUFAQpNG8Gw5noI=; b=hoGGEBWNb3UCiGtiX7SL85zovHBykVEUvanIGuU+d6EsgY6RjElXGIHFhDnGUukBSw 8rvzUvQtTY6QBHhWsAzT/n27TcfOFrmPZh02H2FbVdSFa+HG12io17DZhDdJzsrP0iyp 6jb895lHqHNVtxRWCCx4jVzpaTBVVi1sJdySxNhl7vOo5unXiZ/2I4ZxyFxJ7bJ1wQcx Ay0dk/gyDQWGa1ip+qJoxjv9Ayjb2mTsI3JiC9E+S/jSkQWgZxXVS3UGlEewbx97uMPU RoGa32TGp7wlcl/raBi/uQmpQr+zt5yrT8DiDDIx55n9K70eCQxPD2uRLkrxc8gCro3Q 9jew== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1768351663; x=1768956463; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=KrG7y4Xlsrv3rqpWtCTovQ72S57auUFAQpNG8Gw5noI=; b=uxm2kG8k33n986L3EX3XbLR9Q//a72R4AMIqezL06WPVjYIld1XlFzsFuUo/sBXXfY n1wKFl62psDU2uKfhkFqHjYM5q98iQ0xOov4hgMnbjAeDHgq7+S6WupouHVt9mrVSLcw sm4nqRF4rV3J7rIaE07NIXh/8VF5bphIUc3FVW6wuzsRfXFLcGo54upgMowTW0sw8R4l maNMgcUV79xunHj3Uw8iwMrg67apNUecXxgR5I6GVdNJ9y7VenP/K8PWTq6abGk80LHf w6Wulsqf8D5X05z7/h9MiM4MsuAR/Tz+DAgUY5iTpOSxs7J2lxcL/zNhWOkeJwY1hlao 6hOQ== X-Forwarded-Encrypted: i=1; AJvYcCX4Y1ssj2qWkk98F4d798TBdfl8issQ2hMU6tOe/NKtNiK+qb4pZNFl7N4SCHWcV7TYubty0ZPEv3w=@vger.kernel.org X-Gm-Message-State: AOJu0YzgWW3iTO56qfQC6se9k+a0ABvLnHtLG6uUUFlHYen1V2ylM1qP xGS6B17lG9SDkuguhnhV+7dhlrpwe0NWzt0c2jgrRdVoThgkIu4zoAJigIF6kF1tsg== X-Gm-Gg: AY/fxX497WvSkqJNOl5C4Iv3RfyPaWW1+9ZFsYMHMzvZmVKbqt9CWE99TwN2CML/MOE /vt7MKxxmb1aFjDbIvkCcdQYMmLd705Vd551Lgke7jGHVn7ruEKxZ9bOCoFl2UzXfOF67wgsXpY n/qVKRzptvCedoh/QcnQydkwcOOdlZlQ/Q1WAU8jbOZ4rYhfrr/JifWU5rJitEhr2M4jKHcIzGj enOMQJmThPH3UOeoPPsD2Jcjn8oIL9y6JT9SCaSGnKH3rR66ThaktK3v1vV+LBdjdpyCCZtJSg1 wSAYlOGfX48ZRMjPfuM+7rQB9GH1qDRnqBj7z07KqFuxaRLzciQH1bvnOiP3OFGTKF/1zF+IWu7 4Telhf0rtn8QD28125FeflAMnpagP9hTCIFQ97IstIiXYELhsQg7Xl26vraFWD3cHMFTJEZts2h kO+NDlcC6KJiMHcPSfIhAMENsLnAwZmbb3BodJ93CkVL4/dzojJdivOuFCnJsZk/D2/9PfAue7B tLb7PIb8yt4Kg== X-Received: by 2002:a05:7301:19ad:b0:2a4:6c74:c54a with SMTP id 5a478bee46e88-2b486f41ca8mr940567eec.18.1768351662492; Tue, 13 Jan 2026 16:47:42 -0800 (PST) Received: from ?IPV6:2a00:79e0:2e7c:8:b660:2cbe:a9b7:fc12? ([2a00:79e0:2e7c:8:b660:2cbe:a9b7:fc12]) by smtp.gmail.com with ESMTPSA id 5a478bee46e88-2b1706a5d3dsm17693801eec.13.2026.01.13.16.47.41 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 13 Jan 2026 16:47:41 -0800 (PST) Message-ID: <378ee786-2b44-44e7-a3f6-0cd1db3c0481@google.com> Date: Tue, 13 Jan 2026 16:47:40 -0800 Precedence: bulk X-Mailing-List: linux-usb@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 4/5] power: supply: max77759: add charger driver To: =?UTF-8?Q?Andr=C3=A9_Draszik?= , Sebastian Reichel , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Lee Jones , Greg Kroah-Hartman , Badhri Jagan Sridharan , Heikki Krogerus , Peter Griffin , Tudor Ambarus , Alim Akhtar Cc: linux-kernel@vger.kernel.org, linux-pm@vger.kernel.org, devicetree@vger.kernel.org, linux-usb@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, RD Babiera , Kyle Tso References: <20251227-max77759-charger-v3-0-54e664f5ca92@google.com> <20251227-max77759-charger-v3-4-54e664f5ca92@google.com> <298ca35590d2180fdcf334f94964b6110e17c606.camel@linaro.org> <50c29a62-1fdb-4de2-8887-0d551eee5ec0@google.com> <255d7726-6758-43ed-b35f-db14726bcc9b@google.com> <2869d309358f27652289c40810ca36b2ec155d1d.camel@linaro.org> <6b37b88e9b7ee57eb1c006916fd995c813ab5e6e.camel@linaro.org> Content-Language: en-US From: Amit Sunil Dhamne In-Reply-To: <6b37b88e9b7ee57eb1c006916fd995c813ab5e6e.camel@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Andre', On 1/13/26 2:02 AM, André Draszik wrote: > Hi Amit, > > On Mon, 2026-01-12 at 11:37 -0800, Amit Sunil Dhamne wrote: >> Hi Andre', >> >> On 1/12/26 5:47 AM, André Draszik wrote: >>> Hi Amit, >>> >>> On Tue, 2026-01-06 at 17:14 -0800, Amit Sunil Dhamne wrote: >>>> On 1/6/26 3:41 PM, Amit Sunil Dhamne wrote: >>>>> Hi Andre', >>>>> >>>>> On 1/5/26 9:32 AM, André Draszik wrote: >>>>>> Hi Amit, >>>>>> >>>>>> I haven't done a full review, but a few things caught my eye. >>>>>> >>>>>> On Sat, 2025-12-27 at 00:04 +0000, Amit Sunil Dhamne via B4 Relay wrote: >>>>>>> diff --git a/drivers/power/supply/Makefile >>>>>>> b/drivers/power/supply/Makefile >>>>>>> index 4b79d5abc49a..6af905875ad5 100644 >>>>>>> --- a/drivers/power/supply/Makefile >>>>>>> +++ b/drivers/power/supply/Makefile >>>>>>> [...] >>>>>>> + >>>>>>> +static irqreturn_t irq_handler(int irq, void *data) >>>>>>> +{ >>>>>>> +    struct max77759_charger *chg = data; >>>>>>> +    struct device *dev = chg->dev; >>>>>>> +    u32 chgint_ok; >>>>>>> +    int i; >>>>>>> + >>>>>>> +    regmap_read(chg->regmap, MAX77759_CHGR_REG_CHG_INT_OK, >>>>>>> &chgint_ok); >>>>>> You might want to check the return value and return IRQ_NONE if it >>>>>> didn't >>>>>> work? >>>>>> >>>>>>> + >>>>>>> +    for (i = 0; i < ARRAY_SIZE(irqs); i++) { >>>>>>> +        if (irqs[i] == irq) >>>>>>> +            break; >>>>>>> +    } >>>>>>> + >>>>>>> +    switch (i) { >>>>>>> +    case AICL: >>>>>>> +        dev_dbg(dev, "AICL mode: %s", >>>>>>> +            str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_AICL)); >>>>>>> +        break; >>>>>>> +    case CHGIN: >>>>>>> +        dev_dbg(dev, "CHGIN input valid: %s", >>>>>>> +            str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHGIN)); >>>>>>> +        break; >>>>>>> +    case CHG: >>>>>>> +        dev_dbg(dev, "CHG status okay/off: %s", >>>>>>> +            str_yes_no(chgint_ok & MAX77759_CHGR_REG_CHG_INT_CHG)); >>>>>>> +        break; >>>>>>> +    case INLIM: >>>>>>> +        dev_dbg(dev, "Current Limit reached: %s", >>>>>>> +            str_no_yes(chgint_ok & MAX77759_CHGR_REG_CHG_INT_INLIM)); >>>>>>> +        break; >>>>>>> +    case BAT_OILO: >>>>>>> +        dev_dbg(dev, "Battery over-current threshold crossed"); >>>>>>> +        break; >>>>>>> +    case CHG_STA_CC: >>>>>>> +        dev_dbg(dev, "Charger reached CC stage"); >>>>>>> +        break; >>>>>>> +    case CHG_STA_CV: >>>>>>> +        dev_dbg(dev, "Charger reached CV stage"); >>>>>>> +        break; >>>>>>> +    case CHG_STA_TO: >>>>>>> +        dev_dbg(dev, "Charger reached TO stage"); >>>>>>> +        break; >>>>>>> +    case CHG_STA_DONE: >>>>>>> +        dev_dbg(dev, "Charger reached TO stage"); >>>>>>> +        break; >>>>>> Are the above debug messages really all needed? >>>> I forgot to respond to this comment in my previous email. >>>> >>>> I think we can keep AICL, BAT_OILO, INLIM. They're either special >>>> conditions (AICL) or faulty conditions (like BAT_OILO) and we can in >>>> fact keep them at dev_info level. Rest can be removed and a >>>> power_supply_changed() is sufficient. >>>> >>>> Let me know what you think? >>> I don't think dev_info() in an interrupt handler is appropriate. At >>> least it should be ratelimited. >>> >>> If it's something special / unexpected that needs attention, having >>> a dev_dbg() message only will usually not be visible to anybody. >> I agree. I can change the prints to dev_info_ratelimited for the stuff >> we care about. > If it's an erroneous condition, maybe warn or even err are more appropriate? > > But then, what is the expectation upon the user observing these messages? > What can or should they do? Who is going to look at these and can do > something sensible based on them? The logging will help in postmortem analysis which may or may not possible with just publishing uevents to userspace hoping that they log the psy properties. Illustrating a situation: 1. Over current situation happened where the Battery to System current exceeds the BAT_OILO threshold. This would also generate an interrupt. 2. The MAX77759 takes protective measures if the condition lasts for a certain specified time and reset. Resetting will cause Vsys to collapse to 0 if the system is only battery powered. 3. It'd be better that the BAT_OILO interrupt is logged in dmesg, instead of just delegating it to user space as user can debug this condition by looking at last_kmsg or pstore. 4. This signal can help the user debug conditions such as moisture (this signal + contaminant detection) or indicative of a mechanical failure. I do agree though that this is a hypothetical or very rare situation and if you have a strong opinion against this I am okay with removing the prints completely. > >>> Also will the call to power_supply_changed() down below handle the >>> special conditions (e.g. convey to upper levels)? If not, can it be >>> made to do so? >> Yes it does, as I can see a call to kobject_uevent() inside >> power_supply_changed_work(). Also, power_supply_changed() also notifies >> other subsystems that have registered their notifiers downstream of this >> power_supply object. So I believe we're good there. > If erroneous conditions are handled by other / upper layers, why print a > message in this interrupt handler in the first place? I tried illustrating an example above. > > Also, I just noticed there is a max77705 charger driver. It seems quite > similar to this one, maybe it can be leveraged / extended? Thanks for the feedback. I reviewed the max77705 charger driver. . Here is a breakdown of why I believe a separate driver may be a better approach: Similarities: 1. Helper Functions: We could potentially leverage common logic for get_charge_type, get_status, get_health, and get_input_current. 2. Register Access: MAX77705 uses regfield abstractions to handle register operations which can also be potentially leveraged. 3. Initialization: Some hardware initialization steps appear similar, though about 60% of the max77705 initialization (e.g., switching frequency, WCIN regulation voltage, top-off time) is irrelevant for the max77759 configuration I need. Differences: 1. OTG Support: The max77759 driver supports OTG boost mode, which is a key requirement for my use case. While the max77705 hardware might support OTG based on its registers, the current driver implementation does not support it. 2. TCPCI/TCPM Integration: The max77759 driver is explicitly architected to work with a TCPCI/TCPM-compliant Type-C controller to set input current limits dynamically. It is ambiguous whether the max77705 device uses a standard TCPCI/TCPM model or a proprietary one. 3. Register Incompatibility: There are distinct register differences. For example, the max77705 driver relies on BATP and BATP_DTLS registers, which do not exist in the max77759. Conversely, the max77759 has a dedicated second interrupt register (CHG_INT2) that reports critical signals like BAT_OILO, SYS_UVLO, and charging stages, which appear absent or handled differently in the max77705. Additionally, MAX77759 has input selection (wireless, usb) and uses it in the driver but it's not evident from register definitions whether max77705 has it. 4. Parameter Calculations: The formulas for calculating parameters like Fast Charge Current (CHGCC) and Float Voltage are different between the two chips. Merging the drivers would require separate, chip-specific getter/setter functions for these core properties. 5. Device-Specific Workarounds: The max77705 driver includes a workaround in max77705_aicl_irq that is not relevant to the max77759. There may also be future workarounds which may not be applicable to one or the other. Logistical Constraints: I don't have access to max77705 hardware or its full datasheet. This makes it impossible for me to test any shared code changes to ensure I haven't introduced regressions on the max77705. IMO, given these constraints and technical divergences, maintaining separate drivers maybe a better choice. Please let me know wdyt? BR, Amit > > > Cheers, > Andre'