linux-kernel - RE: [PATCH v3 2/2] rtc: max31335: add driver 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CY4PR03MB3399DEF73E052036AC7EDBA99BA0A@CY4PR03MB3399.namprd03.prod.outlook.com>
Date:   Tue, 31 Oct 2023 16:28:07 +0000
From:   "Miclaus, Antoniu" <Antoniu.Miclaus@...log.com>
To:     Guenter Roeck <linux@...ck-us.net>,
        Alessandro Zummo <a.zummo@...ertech.it>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-hwmon@...r.kernel.org" <linux-hwmon@...r.kernel.org>
Subject: RE: [PATCH v3 2/2] rtc: max31335: add driver support

> On 10/31/23 08:30, Antoniu Miclaus wrote:
> > RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with
> > Integrated MEMS Resonator.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@...log.com>
> > ---
> > changes in v3:
> >   - drop MAX31335_STATUS1 register check inside probe function.
> >   drivers/rtc/Kconfig        |  11 +
> >   drivers/rtc/Makefile       |   1 +
> >   drivers/rtc/rtc-max31335.c | 765
> +++++++++++++++++++++++++++++++++++++
> >   3 files changed, 777 insertions(+)
> >   create mode 100644 drivers/rtc/rtc-max31335.c
> >
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index d7502433c78a..11c7d7fe1e85 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -373,6 +373,17 @@ config RTC_DRV_MAX8997
> >   	  This driver can also be built as a module. If so, the module
> >   	  will be called rtc-max8997.
> >
> > +config RTC_DRV_MAX31335
> > +	tristate "Analog Devices MAX31335"
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +	  If you say yes here you get support for the Analog Devices
> > +	  MAX31335.
> > +
> > +	  This driver can also be built as a module. If so, the module
> > +	  will be called rtc-max31335.
> > +
> 
> Just out of curiosity, is this an unreleased chip ? I only find
> MAX311331 and MAX31334 on the Analog website, but the register
> map for those is different, and they don't support reporting
> the chip temperature.
> 
> [ ... ]
> 
> > +
> > +static const struct hwmon_channel_info *max31335_info[] = {
> > +	HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> 
> According to the register map above, the chip does support
> low and high temperature limits as well as over- and undertemperature
> alarms and interrupts. I would suggest to add support for all of those.
> You might also consider adding support for temperature alarm interrupts
> and report temperature alarm events by calling hwmon_notify_event()
> if a thermal event occurs.

I've sent in the first version of this patch series a cover letter:

"Although the datasheet is not public yet, the driver can be made public (on
other linux custom trees it is already).

The driver was tested with actual hardware and works.

Even though the datasheet is not available, if there are any queries about
the functionality of the part, these can be provided/inserted as code comments
inside the driver."

The reason why I am rushing this a bit is because the customer that uses the
driver wants the driver released and mainline kernel compliant.

This is an initial version of the driver covering the main use cases (which were
requested, therefore actually used).

Additional features can be added afterwards, if requested.

> 
> > +
> > +	hwmon = devm_hwmon_device_register_with_info(&client->dev,
> client->name,
> > +						     max31335,
> > +						     &max31335_chip_info,
> > +						     NULL);
> 
> There is no "depends on HWMON" in the Kconfig entry, meaning this will fail
> to compile if HWMON=n or if HWMON=m and RTC_DRV_MAX31335=y.
>

Will do in v4.

> Guenter