Messages in this thread | | | From | Andy Shevchenko <> | Date | Thu, 15 Feb 2018 14:45:11 +0200 | Subject | Re: [PATCH v2] rtc: isl12026: Add driver. |
| |
On Wed, Feb 14, 2018 at 2:55 AM, David Daney <david.daney@cavium.com> wrote: > The ISL12026 is a combination RTC and EEPROM device with I2C > interface. The standard RTC driver interface is provided. The EEPROM > is accessed via the NVMEM interface via the "eeprom0" directory in the > sysfs entry for the device.
Thanks for an update, my comments below.
> +struct isl12026 { > + struct rtc_device *rtc; > + struct i2c_client *nvm_client; > + struct nvmem_config nvm_cfg; > + /* > + * RTC write operations require that multiple messages be > + * transmitted, we hold the lock for all accesses to the > + * device so that these sequences cannot be disrupted. Also,
> + * the write cycle to the nvmem takes many mS during which the
What mS means? milliseconds? The standard abbreviation for it 'ms'.
> + * device does not respond to commands, so holding the lock > + * also prevents access during these times. > + */ > + struct mutex lock; > +};
> +static int isl12026_read_reg(struct i2c_client *client, int reg) > +{
> + ret = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs)); > + if (ret != ARRAY_SIZE(msgs)) { > + dev_err(&client->dev, "read reg error, ret=%d\n", ret); > + ret = ret < 0 ? ret : -EIO; > + } else { > + ret = val; > + }
> + return val;
Something wrong. ret is not used after all.
> +}
Check entire code for such.
> + /* 2 bytes of address, most significant first */ > + addr[0] = (offset >> 8) & 0xff; > + addr[1] = offset & 0xff;
Consider to drop '& 0xff', they are pointless (you have u8 type).
> + payload[0] = (offset >> 8) & 0xff; > + payload[1] = offset & 0xff;
Ditto.
> +static void isl12026_force_power_modes(struct i2c_client *client) > +{ > + int ret; > + int pwr, requested_pwr; > + u32 bsw_val, sbib_val;
> + bool set_bsw, set_sbib; > +
> + ret = of_property_read_u32(client->dev.of_node, > + "isil,pwr-bsw", &bsw_val); > + set_bsw = (ret == 0);
Which is not fully correct. Better to do
set_bsw = of_property_present();
ret = of_property_read...(); if (ret) return ret;
> + > + ret = of_property_read_u32(client->dev.of_node, > + "isil,pwr-sbib", &sbib_val); > + set_sbib = (ret == 0);
Ditto.
> + > + /* Check if PWR.BSW and/or PWR.SBIB need specified values */ > +
> + if (set_bsw || set_sbib) {
if (!x && !y) return;
> + pwr = isl12026_read_reg(client, ISL12026_REG_PWR); > + if (pwr < 0) { > + dev_err(&client->dev, > + "Error: Failed to read PWR %d\n", pwr); > + return; > + } > + > + requested_pwr = pwr; > + > + if (set_bsw) { > + if (bsw_val) > + requested_pwr |= ISL12026_REG_PWR_BSW; > + else > + requested_pwr &= ~ISL12026_REG_PWR_BSW; > + }
Undefined state if no value?
> + if (set_sbib) { > + if (sbib_val) > + requested_pwr |= ISL12026_REG_PWR_SBIB; > + else > + requested_pwr &= ~ISL12026_REG_PWR_SBIB; > + }
Ditto.
> + > + if (pwr >= 0 && pwr != requested_pwr) {
> + dev_info(&client->dev, "PWR: %02x\n", (u8)pwr); > + dev_info(&client->dev, > + "Updating PWR to: %02x\n", (u8)requested_pwr); > + isl12026_write_reg(client, > + ISL12026_REG_PWR, requested_pwr);
If you do explicit casting in printf() parameters you are doing something wrong in 99.9% cases.
> + } > + } > +}
> +static int isl12026_probe_new(struct i2c_client *client) > +{ > + struct isl12026 *priv; > + int ret;
> + /* The NVMem array responds at i2c address 0x57 */ > + priv->nvm_client = i2c_new_dummy(client->adapter, 0x57);
Magic. Make it #define and put comment there.
> + if (!priv->nvm_client) > + return -ENOMEM;
> +}
> +#ifdef CONFIG_OF
Remove this ugly #ifdef. Your driver OF only one.
> +static const struct of_device_id isl12026_dt_match[] = { > + { .compatible = "isil,isl12026" }, > + { }, > +}; > +MODULE_DEVICE_TABLE(of, isl12026_dt_match);
> +#endif
> + .of_match_table = of_match_ptr(isl12026_dt_match),
Drop of_match_ptr().
-- With Best Regards, Andy Shevchenko
| |