Re: [PATCH v3 2/4] thermal: rcar_gen3_thermal: Add R-Car Gen3 thermal driver

[Eduardo Valentin <edubezval@gmail.com>]

Hey,

On Tue, Nov 29, 2016 at 09:32:42AM +0100, Wolfram Sang wrote:
> Hi Eduardo,
> 
> > No commit message ? :-( generally speaking here, it is a good practice
> > to describe what you are doing, what you have considered, and what you
> > have tested, just for the sake of code history/documentation.
> 
> OK, can do.

cool!

> 
> > > +	spin_lock_irqsave(&tsc->lock, flags);
> > 
> > Why do you need a full blown spin lock irqsave? Can this locking be
> > solved by a simple mutex?
> 
> Currently, it can be a mutex, yes. This is a "left-over" from the
> version which had interrupt support. I am not fully done with
> refactoring the irqs, but I thought it is likely we need this lock then
> again, so I chose to leave it. I can use a mutex for now if you prefer.

yes, yield the processor when possible, please. So, if your locking
don't really need to disable IRQs, then avoid doing it. If a mutex is
enough, use it.

> 
> > 
> > > +static void r8a7795_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
> > > +{
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&tsc->lock, flags);
> > > +
> > > +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  CTSR_THBGR);
> > > +	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,  0x0);
> > > +
> > > +	udelay(1000);
> > 
> > Why do you disable irqs, then busyloop for 1ms?
> 
> Uh yes, this is ugly. I don't think we need to lock during init, but
> I'll double check later.
> 

OK. You could probably leave the (mutex) lock, if you think you need it.
But also consider using usleep_range(1000) instead. Have a look at:
Documentation/timers/timers-howto.txt

for a better explanation.


> > > +	/* default values if FUSEs are missing */
> > > +	int ptat[3] = { 2351, 1509, 435 };
> > > +	int thcode[TSC_MAX_NUM][3] = {
> > > +		{ 3248, 2800, 2221 },
> > > +		{ 3245, 2795, 2216 },
> > > +		{ 3250, 2805, 2237 },
> > > +	};
> > 
> > are these fuses valid for both? 7796 and 7797? or would you need a
> > different array for each version?
> 
> According to the information I have today, it is the same array. And all
> later versions are promised to have fuse registers. This is already
> documented, but such HW is not available to me currently.
> 

Ok. If you have the confirmation for that, then it is fine. I just asked
because cases I saw (different manufacturer of course) would have different values
to be programmed, depending on the chip version.

Anyways, just checking.

> Thanks for the quick review, very much appreciated!
> 
>    Wolfram
>