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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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