Re: [PATCH v6 09/12] gpio: max7360: Add MAX7360 gpio support

[Mathieu Dubois-Briand]

On Thu Apr 17, 2025 at 8:13 PM CEST, Andy Shevchenko wrote:
> On Wed, Apr 09, 2025 at 04:55:56PM +0200, Mathieu Dubois-Briand wrote:
>
>> +#include <linux/slab.h>
>
> I don't think you use this header directly.
>

Right.

> ...
>
>> +static int max7360_gpio_probe(struct platform_device *pdev)
>> +{
>> +	struct regmap_irq_chip *irq_chip;
>> +	struct gpio_regmap_config gpio_config = { };
>> +	struct device *dev = &pdev->dev;
>> +	unsigned long gpio_function;
>> +	struct regmap *regmap;
>> +	unsigned int outconf;
>> +	int ret;
>> +
>> +	regmap = dev_get_regmap(dev->parent, NULL);
>> +	if (!regmap)
>> +		return dev_err_probe(dev, -ENODEV, "could not get parent regmap\n");
>
>> +	gpio_function = (uintptr_t)device_get_match_data(dev);
>
> Somebody pointed me out the Linus' rant on uintptr_t, so he prefers not to see
> this in the entire kernel. He suggested to use (unsigned long), but ideally one
> should operate with the info structures instead.
>

Ok, let's define my own platform data structure, this is not a lot of
work to be honest.

> ...
>
>> +
>> +		/*
>> +		 * Port GPIOs: set output mode configuration (constant-current or not).
>> +		 * This property is optional.
>> +		 */
>> +		outconf = 0;
>> +		ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf);
>> +		if (!ret) {
>> +			ret = regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
>> +			if (ret)
>> +				return dev_err_probe(dev, ret,
>> +						     "Failed to set constant-current configuration\n");
>> +		}
>
> This will look better as if-else:
>
> 		ret = device_property_read_u32(dev, "maxim,constant-current-disable", &outconf);
> 		if (ret) {
> 			outconf = 0;
> 		} else {
> 			ret = regmap_write(regmap, MAX7360_REG_GPIOOUTM, outconf);
> 			if (ret)
> 				return dev_err_probe(dev, ret,
> 						     "Failed to set constant-current configuration\n");
> 		}

Yes, actually there is no need to set outconf if the property was not
specified.


Thanks for your review.

-- 
Mathieu Dubois-Briand, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com