Re: [PATCH v6 10/11] rtc: isl1208: Add isl1208_set_xtoscb()

[Trent Piepho <tpiepho@gmail.com>]

On Friday, June 2, 2023 7:24:25 AM PDT Biju Das wrote:
> 
> +static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int xtosb_val) +{
> +	if (xtosb_val)
> +		sr &= ~ISL1208_REG_SR_XTOSCB;
> +	else
> +		sr |= ISL1208_REG_SR_XTOSCB;
> +
> +	return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
> +}

Since the contents of this register are preserved by battery power, it will
normally already have the correct value.

Setting it this way adds an extra I2C transaction to every driver init to
set the register to the value it's already at.

It would be better to check if the bit is not set correctly, and then only
set it and write to the register if it is not.

You can do that like this:

/* Strangely, xtosb_val of 0 means to set the bit and 1 means to clear it!
 * Hopefully, that is really what you want to do.  Seems backward of what
 * I would expect.
 */
static int isl1208_set_xtoscb(struct i2c_client *client, int sr, int xtosb_val)
{
        /* Do nothing if bit is already set to desired value */
        if (!(st & ISL1208_REG_SR_XTOSCB) == xtosb_val)
                return 0;
        sr ^= ISL1208_REG_SR_XTOSCB;
        return i2c_smbus_write_byte_data(client, ISL1208_REG_SR, sr);
}


>  static int
>  isl1208_probe(struct i2c_client *client)
>  {
> -	int rc = 0;
>  	struct isl1208_state *isl1208;
>  	int evdet_irq = -1;
> +	int xtosb_val = 1;

So you assume XTOSCB should be unset by default?  Prior behavior of the
driver was to preserve this bit.  Maybe it was set the bootloader to the
correct value?  This would break such a setup.

> +	rc = isl1208_clk_present(client, "xin");
> +	if (rc < 0)
> +		return rc;
> +
> +	if (!rc) {
> +		rc = isl1208_clk_present(client, "clkin");

Why do you support two names for the same clock?  I don't see this discussed
in any of the threads.