linux-kernel - Re: [PATCH v2] hwmon: (max77705) add initial support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CABTCjFDsutg-BOKqH++_pYNaeX6SezVL0iyuePVA7m5gXA3Ftw@mail.gmail.com>
Date: Tue, 4 Mar 2025 18:16:29 +0300
From: Dzmitry Sankouski <dsankouski@...il.com>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Jean Delvare <jdelvare@...e.com>, Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: (max77705) add initial support

вт, 4 мар. 2025 г. в 14:38, Guenter Roeck <linux@...ck-us.net>:
>
> On 3/4/25 03:08, Dzmitry Sankouski wrote:
> > Add support for max77705 hwmon. Includes charger input, system bus, and
> > vbyp measurements.
> >
> > Signed-off-by: Dzmitry Sankouski <dsankouski@...il.com>
> > ---
> > Maxim MAX77705 is a Companion Power Management and Type-C interface IC.
> > It includes charger and fuel gauge blocks, and is capable of measuring
> > charger input current, system bus volatage and current, and bypass
> > voltage.
>
> That makes me wonder if this should be implemented in drivers/power/supply/
> and utilize the power_supply->hwmon bridge. That seems to make more sense
> to me than implementing a separate hwmon driver.
>

This chip incorporates charger and fuel gauge, which are modeled as power
supply class drivers. These measurements, however, doesn't belong to any of
mentioned drivers. For example, ISYS is the system bus current, but it
not charger attribute, because charger output is split between battery and
system bus.

Powering diagram:

----------              ---------      ------------      --------------
|usb port|<--[input]--> |charger| <--> |fuel gauge| <--> |battery pack|
----------              ---------      ------------      --------------
                            |
                            |
                            |---> [system bus]

See also https://patches.linaro.org/project/linux-leds/patch/20241217-starqltechn_integration_upstream-v12-2-ed840944f948@gmail.com/#952337

> >
> > Add support for mentioned measurements.
> > ---
> > Changes in v2:
> > - EDITME: describe what is new in this series revision.
> > - EDITME: use bulletpoints and terse descriptions.
> > - Link to v1: https://lore.kernel.org/r/20250225-initial-support-for-max77705-sensors-v1-1-2be6467628b0@gmail.com
>
> ???
>

sorry, I forgot to edit that

(...)
> > +static umode_t max77705_is_visible(const void *data,
> > +                                enum hwmon_sensor_types type,
> > +                                u32 attr, int channel)
> > +{
> > +     switch (type) {
> > +     case hwmon_in:
> > +             if (channel >= ARRAY_SIZE(voltage_channel_desc))
> > +                     return 0;
> > +
> > +             switch (attr) {
> > +             case hwmon_in_input:
> > +             case hwmon_in_label:
> > +                     return 0444;
> > +             default:
> > +                     break;
> > +             }
> > +             break;
> > +     case hwmon_curr:
> > +             if (channel >= ARRAY_SIZE(current_channel_desc))
> > +                     return 0;
> > +
> > +             switch (attr) {
> > +             case hwmon_curr_input:
> > +             case hwmon_in_label:
> > +                     return 0444;
> > +             case hwmon_curr_average:
> > +                     if (current_channel_desc[channel].avg_reg)
> > +                             return 0444;
> > +             default:
> > +                     break;
> > +             }
> > +             break;
>
> The chip provides temperature measurements. Why not support it ?
>

It provides temperature sensing in the fuel gauge block, so it is handled there.

> Also, how about limits ? Doesn't the chip support any ? There do
> seem to be some threshold registers.
>

It has a register in the charger block to limit IIN current. It also has VBYP
interrupt, but I didn't found how the limit is set. Actually, I have a feeling
these measurements are implemented by chip firmware, rather than some IP
solution.

(...)

-- 
Best regards and thanks for review,
Dzmitry