Re: [PATCH v3 09/10] gpio: bd73800: Support ROHM BD73800 PMIC GPIOs

[Matti Vaittinen <mazziesaccount@gmail.com>]

Hi dee Ho Bartosz,

Thanks for the reviews!

On 02/09/2026 15:58, Bartosz Golaszewski wrote:
> On Wed, 2 Sep 2026 13:07:32 +0200, Matti Vaittinen
> <matti.vaittinen@linux.dev> said:
>> From: Matti Vaittinen <mazziesaccount@gmail.com>
>>
>> The ROHM BD73800 PMIC has 4 pins (named GPIO1, CLKOUT, FAULT_B and
>> EXTEN_OUT) which might have been set to operate as a GPI or GPO when OTP
>> (One Time Programmable memory) is written at device manufacturing.
>> Support the GPI/GPO use-case via GPIO framework.
>>
>> The default OTP for these pins is to not use any of them as GPI or GPO.
>> (The GPIO1 defaults as an ADC input regardless the naming). Hence the
>> driver assumes none of these pins is a GPI/GPO unless explicitly pointed
>> as GPI or GPO via device tree.
>>
>> Furthermore, pin's direction can't be changed after OTP configuration is
>> done. Also the default drive type for a GPO (CMOS / Open Drain) is set
>> by the OTP configuration. The BD73800 has a set of undocumented test
>> registers which should allow changing the drive type. Access to the test
>> register area or the test registers aren't documented and so this driver
>> does not support configuring the drive type even though it might be
>> doable.
>>
>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>

//snip

>> +
>> +static const char * const bd73800_gpio_properties[BD73800_GPIO_MAX_PINS] = {
>> +	"rohm,pin-gpio1", "rohm,pin-clkout", "rohm,pin-fault_b", "rohm,pin-exten"
> 
> Can you put the properties on separate lines?

Sure, no problem, thanks. I just wonder if I should re-spin the whole 
series for this. I suppose I'll wait until the next week, to see if I'll 
get any other comments.
>> +};
>> +

//snip

>> +
>> +static int gpo_bd73800_probe(struct platform_device *pdev)
>> +{
>> +	struct gpio_regmap_config config = { };
>> +	struct bd73800_gpio *data;
>> +	struct device *parent, *dev;
>> +	struct gpio_regmap *gpio;
>> +	int ret;
>> +
>> +	dev = &pdev->dev;
>> +	/* The device-tree and regmap come from MFD => use parent for that */
>> +	parent = dev->parent;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->dev = dev;
>> +	data->regmap = dev_get_regmap(parent, NULL);
>> +	if (!data->regmap)
>> +		return dev_err_probe(dev, -ENODEV, "no parent regmap\n");
>> +
>> +	ret = bd73800_gpio_get_pins(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (bitmap_empty(data->valid_mask, BD73800_GPIO_MAX_PINS)) {
>> +		/*
>> +		 * The BD73800 may or may not have pins allocated for GPIO
>> +		 * depending on the OTP used at manufacturing.
>> +		 * If there are no pins, then we have nothing to do.
>> +		 */
>> +		dev_dbg(dev, "no GPIO pins\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	config.parent = parent;
>> +	config.regmap = data->regmap;
>> +	config.label = "bd73800";
>> +	config.ngpio = BD73800_GPIO_MAX_PINS;
>> +	config.reg_dat_base = BD73800_REG_INT_5_SRC;
>> +	config.reg_set_base = BD73800_REG_GPO_OUT;
>> +	config.reg_mask_xlate = bd73800_gpio_reg_mask_xlate;
>> +	config.init_valid_mask = bd73800_gpio_init_valid_mask;
>> +	/* All pins that are valid GPIO lines also have a fixed direction */
>> +	config.fixed_direction_mask = data->valid_mask;
>> +	config.fixed_direction_output = data->output_mask;
>> +	config.drvdata = data;
>> +
>> +	gpio = devm_gpio_regmap_register(dev, &config);
>> +
>> +	return PTR_ERR_OR_ZERO(gpio);
> 
> Why not return PTR_ERR_OR_ZERO(devm_gpio_regmap_register())?

How strongly do you feel about it? It's not a big deal, but I always 
find it a bit harder to read when functions / macros are called inside a 
parameter list. Thus I'd rather keep it like this, just for the sake of 
my own eyes :)

>> +}
>> +

> 
> With that:
> 
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Yours,
	-- Matti

-- 
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~