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

[Vaittinen, Matti]

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 happens when you don't cast to (uintptr_t) first?

On some systems at least the gcc will warn:
> warning: cast from pointer to integer of different size [-Wpointer-
to-int-cast]

I am pretty sure I did end up this double casting via trial and error
:)
+
> > +static const struct of_device_id bd957x_of_match[] = {
> > +	{
> > +		.compatible = "rohm,bd9576",
> > +		.data = (void *)ROHM_CHIP_TYPE_BD9576,
> > +	},
> > +	{
> 
> You could put the 2 lines above on a single line.

Braces? I put braces on separate lines on purpose. Been doing this
after we had this discussion:

https://lore.kernel.org/lkml/20180705055226.GJ496@dell/
https://lore.kernel.org/lkml/20180706070559.GW496@dell/

;)

I can change it if you wishfeel it is important - not a point I feel
like fighting over ;)

> 
> > +		.compatible = "rohm,bd9573",
> > +		.data = (void *)ROHM_CHIP_TYPE_BD9573,
> > +	},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, bd957x_of_match);


Best Regards
	Matti

--
Matti Vaittinen, Linux device drivers
ROHM Semiconductors, Finland
SWDC
Kiviharjunlenkki 1E
90220 OULU
FINLAND

~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~

Simon says - in Latin please.
"non cogito me" dixit Rene Descarte, deinde evanescavit

(Thanks for the translation Simon)