Re: Re: [PATCH v6 2/2] hwmon: Add Eswin EIC7700 PVT sensor driver

[Huan He]

Hi Philipp,

Thank you very much for your detailed review. We appreciate the feedback.

> > 
> > Add support for ESWIN EIC7700 Voltage and Temperature sensor. The driver
> > supports temperature and voltage monitoring with polynomial conversion,
> > and provides sysfs interface for sensor data access.
> > 
> > The PVT IP contains one temperature sensor and one voltage sensor.
> > 
> > Signed-off-by: Yulin Lu <luyulin@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Huan He <hehuan1@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/hwmon/Kconfig       |  12 +
> >  drivers/hwmon/Makefile      |   1 +
> >  drivers/hwmon/eic7700-pvt.c | 495 ++++++++++++++++++++++++++++++++++++
> >  drivers/hwmon/eic7700-pvt.h |  99 ++++++++
> >  4 files changed, 607 insertions(+)
> >  create mode 100644 drivers/hwmon/eic7700-pvt.c
> >  create mode 100644 drivers/hwmon/eic7700-pvt.h
> > 
> [...]
> > diff --git a/drivers/hwmon/eic7700-pvt.c b/drivers/hwmon/eic7700-pvt.c
> > new file mode 100644
> > index 000000000000..ea0f1299cd29
> > --- /dev/null
> > +++ b/drivers/hwmon/eic7700-pvt.c
> > @@ -0,0 +1,495 @@
> [...]
> > +static int eic7700_pvt_probe(struct platform_device *pdev)
> > +{
> > +	struct pvt_hwmon *pvt;
> > +	int ret;
> > +
> > +	pvt = eic7700_pvt_create_data(pdev);
> > +	if (IS_ERR(pvt))
> > +		return PTR_ERR(pvt);
> > +
> > +	platform_set_drvdata(pdev, pvt);
> > +
> > +	pvt->regs = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(pvt->regs))
> > +		return PTR_ERR(pvt->regs);
> > +
> > +	pvt->clk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(pvt->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(pvt->clk),
> > +				     "Couldn't get clock\n");
> > +
> > +	pvt->rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev,
> > +							       NULL);
> 
> Why store this in struct pvt_hwmon? It's not used anywhere else.

The reset control is only needed during probe to deassert the reset line,
and it is not used afterwards.

I will change it to a local variable in eic7700_pvt_probe() and remove the
rst field from struct pvt_hwmon.

The code will be changed along these lines:

    struct reset_control *rst;

    rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL);
    if (IS_ERR(rst))
        return dev_err_probe(pvt->dev, PTR_ERR(rst),
                             "Couldn't get reset control\n");

Best regards,
Huan He