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
