Re: [PATCH v2 1/2] rtc: add pcf85053a

[Guenter Roeck]

On 11/5/23 12:17, Carlos Menin wrote:
> On Fri, Nov 03, 2023 at 07:09:27AM -0700, Guenter Roeck wrote:
> > On 11/3/23 05:51, Carlos Menin wrote:
> > > Add support for NXP's PCF85053A RTC chip.
> > >
> > > Signed-off-by: Carlos Menin <menin@xxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Sergio Prado <sergio.prado@xxxxxxxxxxxxxx>
> > > ---
> >
> > [ ... ]
> >
> > > +static int pcf85053a_bvl_to_mv(unsigned int bvl)
> > > +{
> > > +	long mv_table[] = {
> > > +		1700,
> > > +		1900,
> > > +		2100,
> > > +		2300,
> > > +		2500,
> > > +		2700,
> > > +		2900,
> > > +		3100,
> >
> > How are those numbers determined ? The datasheet gives voltage ranges.
> > I'd have assumed that the center of those ranges is chosen, but for the
> > most part it is the maximum, except for 2900 which is a bit above center
> > and 3100 for "> 3.0V". Not that I care too much, but it seems to me that
> > using the center voltage for each range would be more consistent.
>
> I just used numbers that would result in the same step between levels
> (200 mV) at the same time they would fit in the ranges, but I agree

Ah, thanks for the explanation. Still, I find it a bit misleading.

> that using the center of the ranges makes sense. In this case which
> values would you suggest for <= 1.7 and > 3.0 ?

The datasheet says that the battery voltage must be between 1.55 V and 3.6 V.
With that in mind, and since the next voltages would be 1800 and 2850 if you
use center voltages, I'd probably use 1600 and 3100. You'd then have
	1600, 1800, 2000, 2200, 2400, 2600, 2850, 3100
This gets close to using the same step size but also reflects voltages
more accurately.

> > > +static int pcf85053a_hwmon_register(struct device *dev, const char *name)
> > > +{
> > > +	struct pcf85053a *pcf85053a = dev_get_drvdata(dev);
> > > +	struct device *hwmon_dev;
> > > +
> > > +	hwmon_dev = devm_hwmon_device_register_with_info(dev, name, pcf85053a,
> > > +							 &pcf85053a_hwmon_chip_info,
> > > +							 0);
> >
> > This won't compile if CONFIG_HWMON=n or if CONFIG_RTC_DRV_PCF85053A=y and
> > CONFIG_HWMON=m.
> >
> > Guenter
>
> I will add dependencies in the Kconfig file.
You'll also need something like IS_REACHABLE() here unless you make the driver
depend on HWMON, which is probably not what you want.

Thanks,
Guenter