Re: [PATCH v3 09/10] gpio: bd73800: Support ROHM BD73800 PMIC GPIOs
From: Matti Vaittinen
Date: Thu Sep 03 2026 - 01:04:54 EST
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@xxxxxxxxx> said:
From: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
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@xxxxxxxxx>
//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@xxxxxxxxxxxxxxxx>
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~