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

Mathieu Dubois-Briand
2025-04-18T15:26:53Z

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