Re: [PATCH v4 2/4] mfd: Support ROHM BD9576MUF and BD9573MUF

[Vaittinen, Matti]

On Thu, 2020-11-05 at 08:58 +0000, Lee Jones wrote:
> On Thu, 05 Nov 2020, Vaittinen, Matti wrote:
> 
> > On Thu, 2020-11-05 at 08:21 +0000, Lee Jones wrote:
> > > On Thu, 05 Nov 2020, Vaittinen, Matti wrote:
> > > 
> > > > On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote:
> > > > > Morning Lee,
> > > > > 
> > > > > Thanks for taking a look at this :) I see most of the
> > > > > comments
> > > > > being
> > > > > valid. There's two I would like to clarify though...
> > > > > 
> > > > > On Wed, 2020-11-04 at 15:51 +0000, Lee Jones wrote:
> > > > > > On Wed, 28 Oct 2020, Matti Vaittinen wrote:
> > > > > > 
> > > > > > > Add core support for ROHM BD9576MUF and BD9573MUF PMICs
> > > > > > > which
> > > > > > > are
> > > > > > > mainly used to power the R-Car series processors.
> > > > > > > 
> > > > > > > Signed-off-by: Matti Vaittinen <
> > > > > > > matti.vaittinen@xxxxxxxxxxxxxxxxx
> > > > > > > ---
> > > > > > > +	unsigned int chip_type;
> > > > > > > +
> > > > > > > +	chip_type = (unsigned int)(uintptr_t)
> > > > > > > +		    of_device_get_match_data(&i2c->dev);
> > > > > > 
> > > > > > Not overly keen on this casting.
> > > > > > 
> > > > > > Why not just leave it as (uintptr_t)?
> > > > > 
> > > > > I didn't do so because on x86_64 the address width is
> > > > > probably 64
> > > > > bits
> > > > > whereas the unsigned int is likely to be 32 bits. So the
> > > > > assignment
> > > > > will crop half of the value. It does not really matter as
> > > > > values
> > > > > are
> > > > > small - but I would be surprized if no compilers/analyzers
> > > > > emitted a
> > > > > warning.
> > > > > 
> > > > > I must admit I am not 100% sure though. I sure can change
> > > > > this if
> > > > > you
> > > > > know it better?
> > > 
> > > What if you used 'long', which I believe changed with the
> > > architecture's bus width in Linux?
> > 
> > I think this is exactly what uintptr_t was created for. To provide
> > type
> > which assures a pointer conversion to integer and back works.
> > 
> > I guess I can change the
> > 
> > unsigned int chip_type;
> > 
> > to uintptr_t and get away with single cast if it looks better to
> > you.
> > For me the double cast does not look that bad when it allows use of
> > native int size variable - but in this case it's really just a
> > matter
> > of taste. Both should work fine.
> 
> I do see people casting to uintptr and placing the result into a
> long.

That should work because long is same size as address on architectures
supported by Linux. But it still does not "feel correct" to me. Why
bothering to use the uintptr_t in first place then?

So ... I believe it should *work* on all currently supported
architectures if I do:

unsigned long chip_type;

chip_type = (unsigned long)of_device_get_match_data(&i2c->dev);

although fact it works does not make it *right* :]

But casting to something (uintptr_t) and then assigning to something
else (unsigned long) just trusting that it *works* does feel slightly
fishy when we could use same type for variable and cast.

But again - if 

unsigned long chip_type;
...
chip_type = (unsigned
long)of_device_get_match_data(&i2c->dev);

is what you prefer - I can do that too. Especially in this case where I
expect the highest number to definitely stay less than three digits. If
we some day hit to architecture where addresses are not of same size as
longs, then this is just one minor conversion to do...

--Matti