RE: [PATCH v4 1/2] thermal: Add thermal driver for Sunplus SP7021

[Lh Kuo 郭力豪]

Hi Mr Daniel Lezcano:

Thank you for your review 

> Please check the headers above. Some are not necessary (eg cpufreq,
> delay, rtc ...) AFAICT
> 
> > +#define DISABLE_THREMAL		(BIT(31) | BIT(15))
> > +#define ENABLE_THREMAL		BIT(31)
> > +#define SP_THREMAL_MASK		GENMASK(10, 0)
> 
> THREMAL ?
> 
> s/THREMAL/THERMAL/ ?

Sorry, I'll correct it in next commit.

> > +static char *sp7021_otp_coef_read(struct device *dev, ssize_t *len)
> > +{
> > +	char *ret = NULL;
> > +	struct nvmem_cell *c = nvmem_cell_get(dev, "calib");
> > +
> > +	if (IS_ERR_OR_NULL(c))
> > +		return NULL;
> > +
> > +	ret = nvmem_cell_read(c, len);
> > +	nvmem_cell_put(c);
> 
> That is wrong. Please refer to the documentation driver-api/nvmem.rst
> 
> nvmem_cell_put() must be called when the pointer returned by
> nvmem_cell_read() is no longer used.
>

Sorry for that. I don't know which part is wrong.
nvmem_cell_put() has been called after nvmem_cell_read()
I am coding according to the example.


> duplicate line
> 
> > +	sp_data->otp_temp0 = FIELD_GET(SP_THREMAL_MASK, sp_data->otp_temp0);
> > +	sp_data->otp_temp1 = (otp_v[1] >> 3) | (otp_v[2] << 5);
> > +	sp_data->otp_temp1 = FIELD_GET(SP_THREMAL_MASK, sp_data->otp_temp1);
> > +	if (sp_data->otp_temp0 == 0)
> > +		sp_data->otp_temp0 = TEMP_OTP_BASE;
> 
> Can you add a comment explaining how is stored the coef in the nvcell,
> so we can understand the above actions ?
> 
>

Is the comment below ok?

/*
 *When remanufacturing, the 35 degree T_CODE will be read and stored in nvcell.
 *TEMP_RATE is the SP7021 temperature slope.
 *T_CODE : 11 digits in total
*/