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

["Tanwar, Rahul" <rahul.tanwar@linux.intel.com>]

Hi Andy,

Thanks for the review & feedback.

On 9/9/2020 6:33 pm, Andy Shevchenko wrote:
> 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
> ?

of.h is needed because of of_property_read_u8_array(). I will add
mod_devicetable.h.
property.h seems not required at all.

>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/reset.h>
> ...
>
>> +#define PVT_POLL_TIMEOUT	20000
> Units?

Well noted.

> ...
>
>> +	sys_freq = clk_get_rate(pvt->clk) / 1000000;
> HZ_PER_MHZ ?

Well noted.

>> +	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;

Will update.

>> +		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.

Hmm..I will try to add some more info. Problem is that the datasheet doesn't
explain it clearly.

> ...
>
>> +		regmap_write(p_map, SDIF_DISABLE, GENMASK(p_num - 1, 0));
> For non-constants better would be BIT(p_num) - 1.

Well noted.

> ...
>
>> +		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?

Yes, missed that, will update.

> ...
>
>> +static struct regmap_config pvt_regmap_config = {
>> +	.reg_bits = 32,
>> +	.reg_stride = 4,
>> +	.val_bits = 32,
> How do you use regmap's lock?

We mutex lock whenever read temperature or voltage values from the registers.
All non-probe/non-init paths. We do not override regmap's internal 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?

Well noted.

>> +	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);
>
> ?

Well noted.

>> +	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?

Just populating the name in regmap config because of multiple register
regions.. 

>> +	*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() ?
>

Well noted. Thanks.

Regards,
Rahul