linux-kernel - Re: [PATCH v11 2/2] mfd: max597x: Add support for MAX5970 and MAX5978

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 3 Jan 2023 11:34:30 +0530
From:   Naresh Solanki <naresh.solanki@...ements.com>
To:     Lee Jones <lee@...nel.org>
Cc:     linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        Patrick Rudolph <patrick.rudolph@...ements.com>,
        Marcello Sylvester Bauer <sylv@...v.io>
Subject: Re: [PATCH v11 2/2] mfd: max597x: Add support for MAX5970 and MAX5978

Hi Lee,

On 23-12-2022 06:06 pm, Lee Jones wrote:
> On Wed, 14 Dec 2022, Naresh Solanki wrote:
> 
>> Hi Lee,
>>
>> On 14-12-2022 03:17 pm, Lee Jones wrote:
>>> On Wed, 14 Dec 2022, Naresh Solanki wrote:
>>>
>>>> Hi Lee
>>>>
>>>> On 08-12-2022 05:53 pm, Lee Jones wrote:
>>>>> On Fri, 18 Nov 2022, Naresh Solanki wrote:
>>>>>
>>>>>>
>>>>>>
>>>>>> On 17-11-2022 03:45 pm, Lee Jones wrote:
>>>>>>> On Wed, 16 Nov 2022, Naresh Solanki wrote:
>>>>>>>
>>>>>>>> From: Patrick Rudolph <patrick.rudolph@...ements.com>
>>>>>>>>
>>>>>>>> Implement a regulator driver with IRQ support for fault management.
>>>>>>>> Written against documentation [1] and [2] and tested on real hardware.
>>>>>>>>
>>>>>>>> Every channel has its own regulator supplies nammed 'vss1-supply' and
>>>>>>>> 'vss2-supply'. The regulator supply is used to determine the output
>>>>>>>> voltage, as the smart switch provides no output regulation.
>>>>>>>> The driver requires the 'shunt-resistor-micro-ohms' property to be
>>>>>>>> present in Device Tree to properly calculate current related
>>>>>>>> values.
>>>>>>>>
>>>>>>>> Datasheet links:
>>>>>>>> 1: https://datasheets.maximintegrated.com/en/ds/MAX5970.pdf
>>>>>>>> 2: https://datasheets.maximintegrated.com/en/ds/MAX5978.pdf
>>>>>>>>
>>>>>>>> Signed-off-by: Patrick Rudolph <patrick.rudolph@...ements.com>
>>>>>>>> Co-developed-by: Marcello Sylvester Bauer <sylv@...v.io>
>>>>>>>> Signed-off-by: Marcello Sylvester Bauer <sylv@...v.io>
>>>>>>>> Co-developed-by: Naresh Solanki <Naresh.Solanki@...ements.com>
>>>>>>>> Signed-off-by: Naresh Solanki <Naresh.Solanki@...ements.com>
>>>>>>>> ---
>>>>>>>>      drivers/mfd/Kconfig         |  12 +++++
>>>>>>>>      drivers/mfd/Makefile        |   1 +
>>>>>>>>      drivers/mfd/max597x.c       |  93 +++++++++++++++++++++++++++++++++
>>>>>>>>      include/linux/mfd/max597x.h | 101 ++++++++++++++++++++++++++++++++++++
>>>>>>>>      4 files changed, 207 insertions(+)
>>>>>>>>      create mode 100644 drivers/mfd/max597x.c
>>>>>>>>      create mode 100644 include/linux/mfd/max597x.h
>>>>>>>
>>>>>>> Ignoring my comments won't make them go away. :)
>>>>>>>
>>>>>>> Please tell me why you need a whole new driver, instead of adding
>>>>>>> support to simple-mfd-i2c?
>>>>>>>
>>>>>> I felt current implementation to be simpler, clearer & straight forward.
>>>>>
>>>>> If you can make it work with simple-mfd-i2c, please do so.
>>>> simple-mfd-i2c doesn't has mechanism to pass device type(max5978 vs
>>>> max5970).
>>>
>>> `git grep silergy,sy7636a -- drivers/mfd`
>> I did check the driver but there is no mechanism to distinguish between chip
>> variant i.e., 597x-regulator driver should be able to distinguish between
>> max5978 vs max5970 chip type.
> 
> How is it doing that presently?
Using i2c_device_id. driver_data hold chip variant info based on 
compatible match.
> 
> Why can't the Regulator driver read the DT or match on the parent's
> compatible for itself?
There are three drivers i.e., max597x regulator, led & iio driver.
I'm not sure if checking compatible in each driver is ok.
Recommendation ?
> 
> Providing a 100 line driver just to figure out a single value that is
> only going to be used in a single driver is a no-go.  Please find a
> better way to solve this.Yes but simple-mfd-i2c doesn't help in distinguishing chip variant & in 
situation like absence of device id register, mfd cell driver cant 
determine chip type to initialise accordingly.
Can you please recommend me an approach that can also handle this kind 
of scenario.
> 

Regards,
Naresh