On Wed, Sep 09, 2020 at 02:52:05PM +0800, Rahul Tanwar wrote:
> PVT controller (MR75203) is used to configure & control
> Moortec embedded analog IP which contains temprature
> sensor(TS), voltage monitor(VM) & process detector(PD)
> modules. Add driver to support MR75203 PVT controller.
...
> +#include <linux/clk.h>
> +#include <linux/hwmon.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
I don't see anything special about OF here.
Perhaps
mod_devicetable.h
property.h
?
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/reset.h>
...
> +#define PVT_POLL_TIMEOUT 20000
Units?
...
> + sys_freq = clk_get_rate(pvt->clk) / 1000000;
HZ_PER_MHZ ?
> + while (high >= low) {
> + middle = DIV_ROUND_CLOSEST(low + high, 2);
I'm wondering would it be better in the code like
middle = (low + high + 1) / 2;
> + key = DIV_ROUND_CLOSEST(sys_freq, middle);
> + if (key > 514) {
> + low = middle + 1;
> + continue;
> + } else if (key < 2) {
> + high = middle - 1;
> + continue;
> + }
> +
> + break;
> + }
> +
> + key = clamp_val(key, 2, 514) - 2;
I guess above deserves a comment with formulas.
...
> + regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
For non-constants better would be BIT(p_num) - 1.
...
> + regmap_write(v_map, SDIF_SMPL_CTRL, 0x0);
> + regmap_write(v_map, SDIF_HALT, 0x0);
> + regmap_write(v_map, SDIF_DISABLE, 0);
In some you have 0, in some 0x0 over the file, can it be consistent?
...
> +static struct regmap_config pvt_regmap_config = {
> + .reg_bits = 32,
> + .reg_stride = 4,
> + .val_bits = 32,
How do you use regmap's lock?
> +};
...
> +static int pvt_get_regmap(struct platform_device *pdev, char *reg_name)
> +{
> + struct device *dev = &pdev->dev;
> + struct pvt_device *pvt = platform_get_drvdata(pdev);
Can it be first line in definition block?
> + struct regmap **reg_map;
> + void __iomem *io_base;
> + struct resource *res;
> +
> + if (!strcmp(reg_name, "common"))
> + reg_map = &pvt->c_map;
> + else if (!strcmp(reg_name, "ts"))
> + reg_map = &pvt->t_map;
> + else if (!strcmp(reg_name, "pd"))
> + reg_map = &pvt->p_map;
> + else if (!strcmp(reg_name, "vm"))
> + reg_map = &pvt->v_map;
> + else
> + return -EINVAL;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, reg_name);
> + io_base = devm_ioremap_resource(dev, res);
io_base = devm_platform_ioremap_resource_by_name(pdev, reg_name);
?
> + if (IS_ERR(io_base))
> + return PTR_ERR(io_base);
> +
> + pvt_regmap_config.name = reg_name;
Hmm... regmap API keeps it in devres. Why is there a copy?
> + *reg_map = devm_regmap_init_mmio(dev, io_base, &pvt_regmap_config);
> + if (IS_ERR(*reg_map)) {
> + dev_err(dev, "failed to init register map\n");
> + return PTR_ERR(*reg_map);
> + }
> +
> + return 0;
> +}
...
> + for (i = 0; i < num; i++)
> + in_config[i] = HWMON_I_INPUT;
memset32() ?
--
With Best Regards,
Andy Shevchenko
