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

On Thu, 05 Nov 2020, Vaittinen, Matti wrote:

On Thu, 2020-11-05 at 08:46 +0200, Matti Vaittinen wrote:

quoted

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:

quoted

On Wed, 28 Oct 2020, Matti Vaittinen wrote:

quoted

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@fi.rohmeurope.com

quoted

---
+	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?

quoted

quoted

What happens when you don't cast to (uintptr_t) first?

On some systems at least the gcc will warn:

quoted

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
:)

It's not uncommon. :)

quoted

quoted

quoted

+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/ (local)
https://lore.kernel.org/lkml/20180706070559.GW496@dell/ (local)

;)

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

Ah. I guess you meant:
static const struct of_device_id bd957x_of_match[] = {
        { .compatible = "rohm,bd9576", .data = (void *)ROHM_CHIP_TYPE_BD9576, },
        { .compatible = "rohm,bd9573", .data = (void *)ROHM_CHIP_TYPE_BD9573, },
        {},
}; 

This would be better, yes.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog