On Wed, Feb 14, 2018 at 2:55 AM, David Daney <david.daney@xxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
