Re: [PATCH 2/2] Add driver for Moortec MR75203 PVT controller

Andy Shevchenko
2020-09-09T10:34:38Z

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