Re: [PATCH v3 04/11] thermal: armada: Rationalize register accesses

[Miquel RAYNAL <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

On Mon, 18 Dec 2017 00:02:35 +0200
Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org> wrote:

> Hi Miquèl,
> 
> On Sun, Dec 17, 2017 at 12:18:38AM +0200, Baruch Siach wrote:
> > On Thu, Dec 14, 2017 at 11:30:04AM +0100, Miquel Raynal wrote:  
> > > Bindings were incomplete for a long time by only exposing one of
> > > the two available control registers. To ease the migration to the
> > > full bindings (already in use for the Armada 375 SoC), rename the
> > > pointers for clarification. This way, it will only be needed to
> > > add another pointer to access the other control register when the
> > > time comes.
> > > 
> > > This avoids dangerous situations where the offset 0 of the control
> > > area can be either one register or the other depending on the
> > > bindings used. After this change, device trees of other SoCs
> > > could be migrated to the "full" bindings if they may benefit from
> > > features from the unaccessible register, without any change in
> > > the driver.
> > > 
> > > Signed-off-by: Miquel Raynal <miquel.raynal-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---  
> > 
> > [...]
> >   
> > > +	/*
> > > +	 * Legacy DT bindings only described "control1" register
> > > (also referred
> > > +	 * as "control MSB" on old documentation). New bindings
> > > cover
> > > +	 * "control0/control LSB" and "control1/control MSB"
> > > registers within
> > > +	 * the same resource, which is then of size 8 instead of
> > > 4.
> > > +	 */
> > > +	if ((res->end - res->start) == LEGACY_CONTROL_MEM_LEN) {
> > > +		/* ->control0 unavailable in this configuration
> > > */
> > > +		priv->control1 = control +
> > > LEGACY_CONTROL1_OFFSET;
> > > +	} else {
> > > +		priv->control0 = control + CONTROL0_OFFSET;
> > > +		priv->control1 = control + CONTROL1_OFFSET;
> > > +	}  
> > 
> > I think we need to add a check here that the control registers area
> > size matches the expected value given the compatible string. In
> > case of mismatch probe should fail.  

Ok I will check here for the bindings used.

Still, in the a380_init() I will have to check if control0 is
valid or not because this function should handle both bindings.

> 
> One more thing. You should probably use resource_size() instead of
> open coding it. resource_size() does "res->end - res->start + 1". Are
> you sure your code is correct?

It is not regarding the implementation of resource_size() (which I'm
gonna use).

> 
> > >  	priv->data = (struct armada_thermal_data *)match->data;
> > >  	priv->data->init_sensor(pdev, priv);  
> 
> baruch
> 

Thank you,
Miquèl
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html