[v4,2/2] rtc: max31335: add driver support

  RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with
Integrated MEMS Resonator.

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v4:
 - add Kconfig entry for HWMON dependency.
 MAINTAINERS                |   8 +
 drivers/rtc/Kconfig        |  20 +
 drivers/rtc/Makefile       |   1 +
 drivers/rtc/rtc-max31335.c | 765 +++++++++++++++++++++++++++++++++++++
 4 files changed, 794 insertions(+)
 create mode 100644 drivers/rtc/rtc-max31335.c
  

On Wed, Nov 01, 2023 at 11:48:14AM +0200, Antoniu Miclaus wrote:
> RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with
> Integrated MEMS Resonator.
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v4:
>  - add Kconfig entry for HWMON dependency.
>  MAINTAINERS                |   8 +
>  drivers/rtc/Kconfig        |  20 +
>  drivers/rtc/Makefile       |   1 +
>  drivers/rtc/rtc-max31335.c | 765 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 794 insertions(+)
>  create mode 100644 drivers/rtc/rtc-max31335.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dd5de540ec0b..bc484cb997ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12823,6 +12823,14 @@ F:	Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
>  F:	Documentation/hwmon/max31827.rst
>  F:	drivers/hwmon/max31827.c
>  
> +MAX31335 RTC DRIVER
> +M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
> +L:	linux-rtc@vger.kernel.org
> +S:	Supported
> +W:	https://ez.analog.com/linux-software-drivers
> +F:	Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> +F:	drivers/rtc/rtc-max31335.c
> +
>  MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
>  L:	linux-hwmon@vger.kernel.org
>  S:	Orphan
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index d7502433c78a..360da13fe61b 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -373,6 +373,26 @@ 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.
> +
> +config RTC_DRV_MAX31335_HWMON
> +	bool "HWMON support for Analog Devices MAX31335"
> +	depends on RTC_DRV_MAX31335 && HWMON
> +	depends on !(RTC_DRV_MAX31335=y && HWMON=m)
> +	default y
> +	help
> +	  Say Y here if you want to expose temperature sensor data on
> +	  rtc-max31335.
> +

This isn't used in the driver. Did you test with HWMON=n ?

Guenter
  

> On Wed, Nov 01, 2023 at 11:48:14AM +0200, Antoniu Miclaus wrote:
> > RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with
> > Integrated MEMS Resonator.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> > ---
> > changes in v4:
> >  - add Kconfig entry for HWMON dependency.
> >  MAINTAINERS                |   8 +
> >  drivers/rtc/Kconfig        |  20 +
> >  drivers/rtc/Makefile       |   1 +
> >  drivers/rtc/rtc-max31335.c | 765
> +++++++++++++++++++++++++++++++++++++
> >  4 files changed, 794 insertions(+)
> >  create mode 100644 drivers/rtc/rtc-max31335.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index dd5de540ec0b..bc484cb997ab 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12823,6 +12823,14 @@ F:
> 	Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
> >  F:	Documentation/hwmon/max31827.rst
> >  F:	drivers/hwmon/max31827.c
> >
> > +MAX31335 RTC DRIVER
> > +M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
> > +L:	linux-rtc@vger.kernel.org
> > +S:	Supported
> > +W:	https://ez.analog.com/linux-software-drivers
> > +F:	Documentation/devicetree/bindings/rtc/adi,max31335.yaml
> > +F:	drivers/rtc/rtc-max31335.c
> > +
> >  MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
> >  L:	linux-hwmon@vger.kernel.org
> >  S:	Orphan
> > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> > index d7502433c78a..360da13fe61b 100644
> > --- a/drivers/rtc/Kconfig
> > +++ b/drivers/rtc/Kconfig
> > @@ -373,6 +373,26 @@ 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.
> > +
> > +config RTC_DRV_MAX31335_HWMON
> > +	bool "HWMON support for Analog Devices MAX31335"
> > +	depends on RTC_DRV_MAX31335 && HWMON
> > +	depends on !(RTC_DRV_MAX31335=y && HWMON=m)
> > +	default y
> > +	help
> > +	  Say Y here if you want to expose temperature sensor data on
> > +	  rtc-max31335.
> > +
> 
> This isn't used in the driver. Did you test with HWMON=n ?
> 

I need to start thinking a bit more out of the box instead of 
taking other driver approaches as being the only way to go. 😊

Will fix in v5.

> Guenter
  

On 11/1/23 08:21, Miclaus, Antoniu wrote:
>> On Wed, Nov 01, 2023 at 11:48:14AM +0200, Antoniu Miclaus wrote:
>>> RTC driver for MAX31335 ±2ppm Automotive Real-Time Clock with
>>> Integrated MEMS Resonator.
>>>
>>> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
>>> ---
>>> changes in v4:
>>>   - add Kconfig entry for HWMON dependency.
>>>   MAINTAINERS                |   8 +
>>>   drivers/rtc/Kconfig        |  20 +
>>>   drivers/rtc/Makefile       |   1 +
>>>   drivers/rtc/rtc-max31335.c | 765
>> +++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 794 insertions(+)
>>>   create mode 100644 drivers/rtc/rtc-max31335.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index dd5de540ec0b..bc484cb997ab 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -12823,6 +12823,14 @@ F:
>> 	Documentation/devicetree/bindings/hwmon/adi,max31827.yaml
>>>   F:	Documentation/hwmon/max31827.rst
>>>   F:	drivers/hwmon/max31827.c
>>>
>>> +MAX31335 RTC DRIVER
>>> +M:	Antoniu Miclaus <antoniu.miclaus@analog.com>
>>> +L:	linux-rtc@vger.kernel.org
>>> +S:	Supported
>>> +W:	https://ez.analog.com/linux-software-drivers
>>> +F:	Documentation/devicetree/bindings/rtc/adi,max31335.yaml
>>> +F:	drivers/rtc/rtc-max31335.c
>>> +
>>>   MAX6650 HARDWARE MONITOR AND FAN CONTROLLER DRIVER
>>>   L:	linux-hwmon@vger.kernel.org
>>>   S:	Orphan
>>> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
>>> index d7502433c78a..360da13fe61b 100644
>>> --- a/drivers/rtc/Kconfig
>>> +++ b/drivers/rtc/Kconfig
>>> @@ -373,6 +373,26 @@ 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.
>>> +
>>> +config RTC_DRV_MAX31335_HWMON
>>> +	bool "HWMON support for Analog Devices MAX31335"
>>> +	depends on RTC_DRV_MAX31335 && HWMON
>>> +	depends on !(RTC_DRV_MAX31335=y && HWMON=m)
>>> +	default y
>>> +	help
>>> +	  Say Y here if you want to expose temperature sensor data on
>>> +	  rtc-max31335.
>>> +
>>
>> This isn't used in the driver. Did you test with HWMON=n ?
>>
> 
> I need to start thinking a bit more out of the box instead of
> taking other driver approaches as being the only way to go. 😊
> 

There are essentially two options: Either the above, or use
	depends on HWMON || HWMON=n
for RTC_DRV_MAX31335.

In both cases, the driver hwmon code has to be surrounded with #if[def].
Depending on your approach, that will be "#ifdef RTC_DRV_MAX31335_HWMON"
or "#if IS_REACHABLE(HWMON)".

You could also use "depends on HWMON" for RTC_DRV_MAX31335 to avoid the
#ifdef in the driver, but that is normally undesirable.

Guenter
  

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?

> +	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 ?

> +
> +	return 0;
> +}
> +
> +static int max31335_set_offset(struct device *dev, long offset)
> +{
> +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> +
> +	return regmap_write(max31335->regmap, MAX31335_AGING_OFFSET, offset);
> +}
> +
> +static int max31335_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> +	struct mutex *lock = &max31335->rtc->ops_lock;
> +	int ret, ctrl, status;
> +	struct rtc_time time;
> +	u8 regs[6];
> +
> +	mutex_lock(lock);

Use rtc_lock(), I'm not quite sure why you would need locking here
though.

> +
> +	ret = regmap_bulk_read(max31335->regmap, MAX31335_ALM1_SEC, regs,
> +			       sizeof(regs));
> +	if (ret)
> +		goto exit;
> +
> +	alrm->time.tm_sec  = bcd2bin(regs[0] & 0x7f);
> +	alrm->time.tm_min  = bcd2bin(regs[1] & 0x7f);
> +	alrm->time.tm_hour = bcd2bin(regs[2] & 0x3f);
> +	alrm->time.tm_mday = bcd2bin(regs[3] & 0x3f);
> +	alrm->time.tm_mon  = bcd2bin(regs[4] & 0x1f) - 1;
> +	alrm->time.tm_year = bcd2bin(regs[5]) + 100;
> +
> +	ret = max31335_read_time(dev, &time);
> +	if (ret)
> +		goto exit;
> +
> +	if (time.tm_year >= 200)
> +		alrm->time.tm_year += 100;
> +
> +	ret = regmap_read(max31335->regmap, MAX31335_INT_EN1, &ctrl);
> +	if (ret)
> +		goto exit;
> +
> +	ret = regmap_read(max31335->regmap, MAX31335_STATUS1, &status);
> +	if (ret)
> +		goto exit;
> +
> +	alrm->enabled = FIELD_GET(MAX31335_INT_EN1_A1IE, ctrl);
> +	alrm->pending = FIELD_GET(MAX31335_STATUS1_A1F, status);
> +
> +exit:
> +	mutex_unlock(lock);
> +
> +	return ret;
> +}
> +
> +static int max31335_set_alarm(struct device *dev, struct rtc_wkalrm *alrm)
> +{
> +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> +	struct mutex *lock = &max31335->rtc->ops_lock;
> +	unsigned int reg;
> +	u8 regs[6];
> +	int ret;
> +
> +	regs[0] = bin2bcd(alrm->time.tm_sec);
> +	regs[1] = bin2bcd(alrm->time.tm_min);
> +	regs[2] = bin2bcd(alrm->time.tm_hour);
> +	regs[3] = bin2bcd(alrm->time.tm_mday);
> +	regs[4] = bin2bcd(alrm->time.tm_mon + 1);
> +	regs[5] = bin2bcd(alrm->time.tm_year % 100);
> +
> +	mutex_lock(lock);

I'm not sure why you need locking here either, maybe you should simply
disable the alarm first?

> +
> +	ret = regmap_bulk_write(max31335->regmap, MAX31335_ALM1_SEC,
> +				regs, sizeof(regs));
> +	if (ret)
> +		goto exit;
> +
> +	reg = FIELD_PREP(MAX31335_INT_EN1_A1IE, alrm->enabled);
> +	ret = regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> +				 MAX31335_INT_EN1_A1IE, reg);
> +	if (ret)
> +		goto exit;
> +
> +	ret = regmap_update_bits(max31335->regmap, MAX31335_STATUS1,
> +				 MAX31335_STATUS1_A1F, 0);
> +
> +exit:
> +	mutex_unlock(lock);
> +
> +	return ret;
> +}
> +
> +static int max31335_alarm_irq_enable(struct device *dev, unsigned int enabled)
> +{
> +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> +
> +	return regmap_update_bits(max31335->regmap, MAX31335_INT_EN1,
> +				  MAX31335_INT_EN1_A1IE, enabled);
> +}
> +


> +static int max31335_trickle_charger_setup(struct device *dev,
> +					  struct max31335_data *max31335)
> +{
> +	u32 ohms, chargeable;
> +	bool diode = false;
> +	int i;
> +
> +	if (device_property_read_u32(dev, "trickle-resistor-ohms", &ohms))
> +		return 0;
> +
> +	if (!device_property_read_u32(dev, "aux-voltage-chargeable",
> +				      &chargeable)) {
> +		switch (chargeable) {
> +		case 0:
> +			diode = false;
> +			break;
> +		case 1:
> +			diode = true;
> +			break;
> +		default:
> +			dev_warn(dev,
> +				 "unsupported aux-voltage-chargeable value\n");

I don't think the string is necessary, checking the DT should be done at
compile time.

> +			break;
> +		}
> +	}
> +
> +	for (i = 0; i < ARRAY_SIZE(max31335_trickle_resistors); i++)
> +		if (ohms == max31335_trickle_resistors[i])
> +			break;
> +
> +	if (i >= ARRAY_SIZE(max31335_trickle_resistors)) {
> +		dev_warn(dev, "invalid trickle resistor value\n");

Ditto.

> +
> +		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?

> +
> +	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?

> +
> +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, &regmap_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?

> +
> +	max31335->rtc = devm_rtc_allocate_device(&client->dev);
> +	if (IS_ERR(max31335->rtc))
> +		return PTR_ERR(max31335->rtc);
> +
> +	max31335->rtc->ops = &max31335_rtc_ops;
> +	max31335->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> +	max31335->rtc->range_max = RTC_TIMESTAMP_END_2199;

Please set alarm_offset_max too.

> +
> +	ret = devm_rtc_register_device(max31335->rtc);
> +	if (ret)
> +		return ret;
> +
> +	ret = max31335_clkout_register(&client->dev);
> +	if (ret)
> +		return ret;
> +
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, max31335_handle_irq,
> +						IRQF_ONESHOT,
> +						"max31335", max31335);
> +		if (ret) {
> +			dev_warn(&client->dev,
> +				 "unable to request IRQ, alarm max31335 disabled\n");
> +			client->irq = 0;
> +		}
> +	}
> +
> +	if (!client->irq)
> +		clear_bit(RTC_FEATURE_ALARM, max31335->rtc->features);
> +
> +	max31335_nvmem_cfg.priv = max31335;
> +	ret = devm_rtc_nvmem_register(max31335->rtc, &max31335_nvmem_cfg);
> +	if (ret)
> +		dev_err_probe(&client->dev, ret, "cannot register rtc nvmem\n");
> +
> +	hwmon = devm_hwmon_device_register_with_info(&client->dev, client->name,
> +						     max31335,
> +						     &max31335_chip_info,
> +						     NULL);
> +	if (IS_ERR(hwmon))
> +		dev_err_probe(&client->dev, PTR_ERR(hwmon),
> +			      "cannot register hwmon device\n");
> +
> +	return max31335_trickle_charger_setup(&client->dev, max31335);

You must never fail probe after calling devm_rtc_register_device, else
you are open to a race condition with userspace.

> +}
> +
> +static const struct i2c_device_id max31335_id[] = {
> +	{ "max31335", 0 },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, max31335_id);
> +
> +static const struct of_device_id max31335_of_match[] = {
> +	{ .compatible = "adi,max31335" },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, max31335_of_match);
> +
> +static struct i2c_driver max31335_driver = {
> +	.driver = {
> +		.name = "rtc-max31335",
> +		.of_match_table = max31335_of_match,
> +	},
> +	.probe = max31335_probe,
> +	.id_table = max31335_id,
> +};
> +module_i2c_driver(max31335_driver);
> +
> +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com>");
> +MODULE_DESCRIPTION("MAX31335 RTC driver");
> +MODULE_LICENSE("GPL");
> -- 
> 2.42.0
>
  

> 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.

> 
> > +	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?

> > +
> > +	return 0;
> > +}
> > +
> > +static int max31335_set_offset(struct device *dev, long offset)
> > +{
> > +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> > +
> > +	return regmap_write(max31335->regmap,
> MAX31335_AGING_OFFSET, offset);
> > +}
> > +
> > +static int max31335_read_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> > +{
> > +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> > +	struct mutex *lock = &max31335->rtc->ops_lock;
> > +	int ret, ctrl, status;
> > +	struct rtc_time time;
> > +	u8 regs[6];
> > +
> > +	mutex_lock(lock);
> 
> Use rtc_lock(), I'm not quite sure why you would need locking here
> though.
> 
> > +
> > +	ret = regmap_bulk_read(max31335->regmap,
> MAX31335_ALM1_SEC, regs,
> > +			       sizeof(regs));
> > +	if (ret)
> > +		goto exit;
> > +
> > +	alrm->time.tm_sec  = bcd2bin(regs[0] & 0x7f);
> > +	alrm->time.tm_min  = bcd2bin(regs[1] & 0x7f);
> > +	alrm->time.tm_hour = bcd2bin(regs[2] & 0x3f);
> > +	alrm->time.tm_mday = bcd2bin(regs[3] & 0x3f);
> > +	alrm->time.tm_mon  = bcd2bin(regs[4] & 0x1f) - 1;
> > +	alrm->time.tm_year = bcd2bin(regs[5]) + 100;
> > +
> > +	ret = max31335_read_time(dev, &time);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	if (time.tm_year >= 200)
> > +		alrm->time.tm_year += 100;
> > +
> > +	ret = regmap_read(max31335->regmap, MAX31335_INT_EN1, &ctrl);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	ret = regmap_read(max31335->regmap, MAX31335_STATUS1,
> &status);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	alrm->enabled = FIELD_GET(MAX31335_INT_EN1_A1IE, ctrl);
> > +	alrm->pending = FIELD_GET(MAX31335_STATUS1_A1F, status);
> > +
> > +exit:
> > +	mutex_unlock(lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int max31335_set_alarm(struct device *dev, struct rtc_wkalrm
> *alrm)
> > +{
> > +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> > +	struct mutex *lock = &max31335->rtc->ops_lock;
> > +	unsigned int reg;
> > +	u8 regs[6];
> > +	int ret;
> > +
> > +	regs[0] = bin2bcd(alrm->time.tm_sec);
> > +	regs[1] = bin2bcd(alrm->time.tm_min);
> > +	regs[2] = bin2bcd(alrm->time.tm_hour);
> > +	regs[3] = bin2bcd(alrm->time.tm_mday);
> > +	regs[4] = bin2bcd(alrm->time.tm_mon + 1);
> > +	regs[5] = bin2bcd(alrm->time.tm_year % 100);
> > +
> > +	mutex_lock(lock);
> 
> I'm not sure why you need locking here either, maybe you should simply
> disable the alarm first?
> 
> > +
> > +	ret = regmap_bulk_write(max31335->regmap,
> MAX31335_ALM1_SEC,
> > +				regs, sizeof(regs));
> > +	if (ret)
> > +		goto exit;
> > +
> > +	reg = FIELD_PREP(MAX31335_INT_EN1_A1IE, alrm->enabled);
> > +	ret = regmap_update_bits(max31335->regmap,
> MAX31335_INT_EN1,
> > +				 MAX31335_INT_EN1_A1IE, reg);
> > +	if (ret)
> > +		goto exit;
> > +
> > +	ret = regmap_update_bits(max31335->regmap,
> MAX31335_STATUS1,
> > +				 MAX31335_STATUS1_A1F, 0);
> > +
> > +exit:
> > +	mutex_unlock(lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int max31335_alarm_irq_enable(struct device *dev, unsigned int
> enabled)
> > +{
> > +	struct max31335_data *max31335 = dev_get_drvdata(dev);
> > +
> > +	return regmap_update_bits(max31335->regmap,
> MAX31335_INT_EN1,
> > +				  MAX31335_INT_EN1_A1IE, enabled);
> > +}
> > +
> 
> 
> > +static int max31335_trickle_charger_setup(struct device *dev,
> > +					  struct max31335_data *max31335)
> > +{
> > +	u32 ohms, chargeable;
> > +	bool diode = false;
> > +	int i;
> > +
> > +	if (device_property_read_u32(dev, "trickle-resistor-ohms", &ohms))
> > +		return 0;
> > +
> > +	if (!device_property_read_u32(dev, "aux-voltage-chargeable",
> > +				      &chargeable)) {
> > +		switch (chargeable) {
> > +		case 0:
> > +			diode = false;
> > +			break;
> > +		case 1:
> > +			diode = true;
> > +			break;
> > +		default:
> > +			dev_warn(dev,
> > +				 "unsupported aux-voltage-chargeable
> value\n");
> 
> I don't think the string is necessary, checking the DT should be done at
> compile time.
> 
> > +			break;
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(max31335_trickle_resistors); i++)
> > +		if (ohms == max31335_trickle_resistors[i])
> > +			break;
> > +
> > +	if (i >= ARRAY_SIZE(max31335_trickle_resistors)) {
> > +		dev_warn(dev, "invalid trickle resistor value\n");
> 
> Ditto.
> 
> > +
> > +		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

> 
> > +
> > +	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.

> 
> > +
> > +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,
> &regmap_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.

> > +
> > +	max31335->rtc = devm_rtc_allocate_device(&client->dev);
> > +	if (IS_ERR(max31335->rtc))
> > +		return PTR_ERR(max31335->rtc);
> > +
> > +	max31335->rtc->ops = &max31335_rtc_ops;
> > +	max31335->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> > +	max31335->rtc->range_max = RTC_TIMESTAMP_END_2199;
> 
> Please set alarm_offset_max too.
> 
> > +
> > +	ret = devm_rtc_register_device(max31335->rtc);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = max31335_clkout_register(&client->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (client->irq > 0) {
> > +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> > +						NULL, max31335_handle_irq,
> > +						IRQF_ONESHOT,
> > +						"max31335", max31335);
> > +		if (ret) {
> > +			dev_warn(&client->dev,
> > +				 "unable to request IRQ, alarm max31335
> disabled\n");
> > +			client->irq = 0;
> > +		}
> > +	}
> > +
> > +	if (!client->irq)
> > +		clear_bit(RTC_FEATURE_ALARM, max31335->rtc->features);
> > +
> > +	max31335_nvmem_cfg.priv = max31335;
> > +	ret = devm_rtc_nvmem_register(max31335->rtc,
> &max31335_nvmem_cfg);
> > +	if (ret)
> > +		dev_err_probe(&client->dev, ret, "cannot register rtc
> nvmem\n");
> > +
> > +	hwmon = devm_hwmon_device_register_with_info(&client->dev,
> client->name,
> > +						     max31335,
> > +						     &max31335_chip_info,
> > +						     NULL);
> > +	if (IS_ERR(hwmon))
> > +		dev_err_probe(&client->dev, PTR_ERR(hwmon),
> > +			      "cannot register hwmon device\n");
> > +
> > +	return max31335_trickle_charger_setup(&client->dev, max31335);
> 
> You must never fail probe after calling devm_rtc_register_device, else
> you are open to a race condition with userspace.
> 
> > +}
> > +
> > +static const struct i2c_device_id max31335_id[] = {
> > +	{ "max31335", 0 },
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(i2c, max31335_id);
> > +
> > +static const struct of_device_id max31335_of_match[] = {
> > +	{ .compatible = "adi,max31335" },
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, max31335_of_match);
> > +
> > +static struct i2c_driver max31335_driver = {
> > +	.driver = {
> > +		.name = "rtc-max31335",
> > +		.of_match_table = max31335_of_match,
> > +	},
> > +	.probe = max31335_probe,
> > +	.id_table = max31335_id,
> > +};
> > +module_i2c_driver(max31335_driver);
> > +
> > +MODULE_AUTHOR("Antoniu Miclaus <antoniu.miclaus@analog.com>");
> > +MODULE_DESCRIPTION("MAX31335 RTC driver");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.42.0
> >
> 
> --
> Alexandre Belloni, co-owner and COO, Bootlin
> Embedded Linux and Kernel engineering
> https://urldefense.com/v3/__https://bootlin.com__;!!A3Ni8CS0y2Y!9L5oCv
> HC0oT_FwMzkX2RA8fSIOBFPOxEQ_aycnNEcZqpwVS5HKLHl_s1Cji_iNKNHLFI
> Y37QpXsMZUPCyBmurhBxVvwaPVOU$
  

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,
> > &regmap_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.
  

On 11/2/23 09:57, Alexandre Belloni wrote:
[ ... ]
>>>> +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.
> 

The max31334 datasheet says "Resolution = 0.477ppm". Again, the datasheet for
max31335 is not public, so it is impossible to say what the resolution is
for that chip, but I would assume that it is documented.

Guenter