| Message ID | 20250211-aux-device-create-helper-v3-3-7edb50524909@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | driver core: auxiliary bus: add device creation helper | expand |
| Context | Check | Description |
|---|---|---|
| bjorn/pre-ci_am | success | Success |
| bjorn/build-rv32-defconfig | success | build-rv32-defconfig |
| bjorn/build-rv64-clang-allmodconfig | success | build-rv64-clang-allmodconfig |
| bjorn/build-rv64-gcc-allmodconfig | success | build-rv64-gcc-allmodconfig |
| bjorn/build-rv64-nommu-k210-defconfig | success | build-rv64-nommu-k210-defconfig |
| bjorn/build-rv64-nommu-k210-virt | success | build-rv64-nommu-k210-virt |
| bjorn/checkpatch | success | checkpatch |
| bjorn/dtb-warn-rv64 | success | dtb-warn-rv64 |
| bjorn/header-inline | success | header-inline |
| bjorn/kdoc | success | kdoc |
| bjorn/module-param | success | module-param |
| bjorn/verify-fixes | success | verify-fixes |
| bjorn/verify-signedoff | success | verify-signedoff |
Hi, On Tue, Feb 11, 2025 at 9:28 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > The auxiliary device creation of this driver is simple enough to > use the available auxiliary device creation helper. > > Use it and remove some boilerplate code. > > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > --- > drivers/gpu/drm/bridge/ti-sn65dsi86.c | 84 +++++++++-------------------------- > 1 file changed, 20 insertions(+), 64 deletions(-) Thanks for creating the helpers and getting rid of some boilerplate! This conflicts with commit 574f5ee2c85a ("drm/bridge: ti-sn65dsi86: Fix multiple instances") which is in drm-next, though. Please resolve. Since nothing here is urgent, I would assume patch #1 would land and then we'd just wait until it made it to mainline before landing the other patches in their respective trees? > -static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, > - struct auxiliary_device **aux_out, > - const char *name) > -{ > - struct device *dev = pdata->dev; > - struct auxiliary_device *aux; > - int ret; > - > - aux = kzalloc(sizeof(*aux), GFP_KERNEL); > - if (!aux) > - return -ENOMEM; > - > - aux->name = name; > - aux->dev.parent = dev; > - aux->dev.release = ti_sn65dsi86_aux_device_release; > - device_set_of_node_from_dev(&aux->dev, dev); > - ret = auxiliary_device_init(aux); > - if (ret) { > - kfree(aux); > - return ret; > - } > - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux); > - if (ret) > - return ret; > - > - ret = auxiliary_device_add(aux); > - if (ret) > - return ret; > - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux); > - if (!ret) > - *aux_out = aux; I notice that your new code has one fewer devm_add_action_or_reset() than the code here which you're replacing. That means it needs to call "uninit" explicitly in one extra place. It still seems clean enough, though, so I don't have any real objections to the way you're doing it there. ;-) -Doug
On Wed 12 Feb 2025 at 08:38, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Tue, Feb 11, 2025 at 9:28 AM Jerome Brunet <jbrunet@baylibre.com> wrote: >> >> The auxiliary device creation of this driver is simple enough to >> use the available auxiliary device creation helper. >> >> Use it and remove some boilerplate code. >> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> >> --- >> drivers/gpu/drm/bridge/ti-sn65dsi86.c | 84 +++++++++-------------------------- >> 1 file changed, 20 insertions(+), 64 deletions(-) > > Thanks for creating the helpers and getting rid of some boilerplate! > This conflicts with commit 574f5ee2c85a ("drm/bridge: ti-sn65dsi86: > Fix multiple instances") which is in drm-next, though. Please resolve. Noted. this is based on v6.14-rc1 ATM > > Since nothing here is urgent, I would assume patch #1 would land and > then we'd just wait until it made it to mainline before landing the > other patches in their respective trees? That would simplest way to handle it I think. No rush. I'll rebase when the time comes. > > >> -static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, >> - struct auxiliary_device **aux_out, >> - const char *name) >> -{ >> - struct device *dev = pdata->dev; >> - struct auxiliary_device *aux; >> - int ret; >> - >> - aux = kzalloc(sizeof(*aux), GFP_KERNEL); >> - if (!aux) >> - return -ENOMEM; >> - >> - aux->name = name; >> - aux->dev.parent = dev; >> - aux->dev.release = ti_sn65dsi86_aux_device_release; >> - device_set_of_node_from_dev(&aux->dev, dev); >> - ret = auxiliary_device_init(aux); >> - if (ret) { >> - kfree(aux); >> - return ret; >> - } >> - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux); >> - if (ret) >> - return ret; >> - >> - ret = auxiliary_device_add(aux); >> - if (ret) >> - return ret; >> - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux); >> - if (!ret) >> - *aux_out = aux; > > I notice that your new code has one fewer devm_add_action_or_reset() > than the code here which you're replacing. That means it needs to call > "uninit" explicitly in one extra place. ... but it needs one memory allocation less ;) > It still seems clean enough, > though, so I don't have any real objections to the way you're doing it > there. ;-) Both ways are valid indeed. Just a matter of personal taste I guess. > > -Doug
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c index e4d9006b59f1b975cf63e26b221e985206caf867..e583b8ba1fd4f27d98e03d4382e0417bbd50436f 100644 --- a/drivers/gpu/drm/bridge/ti-sn65dsi86.c +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c @@ -454,62 +454,6 @@ static void ti_sn65dsi86_debugfs_init(struct ti_sn65dsi86 *pdata) debugfs_create_file("status", 0600, debugfs, pdata, &status_fops); } -/* ----------------------------------------------------------------------------- - * Auxiliary Devices (*not* AUX) - */ - -static void ti_sn65dsi86_uninit_aux(void *data) -{ - auxiliary_device_uninit(data); -} - -static void ti_sn65dsi86_delete_aux(void *data) -{ - auxiliary_device_delete(data); -} - -static void ti_sn65dsi86_aux_device_release(struct device *dev) -{ - struct auxiliary_device *aux = container_of(dev, struct auxiliary_device, dev); - - kfree(aux); -} - -static int ti_sn65dsi86_add_aux_device(struct ti_sn65dsi86 *pdata, - struct auxiliary_device **aux_out, - const char *name) -{ - struct device *dev = pdata->dev; - struct auxiliary_device *aux; - int ret; - - aux = kzalloc(sizeof(*aux), GFP_KERNEL); - if (!aux) - return -ENOMEM; - - aux->name = name; - aux->dev.parent = dev; - aux->dev.release = ti_sn65dsi86_aux_device_release; - device_set_of_node_from_dev(&aux->dev, dev); - ret = auxiliary_device_init(aux); - if (ret) { - kfree(aux); - return ret; - } - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_uninit_aux, aux); - if (ret) - return ret; - - ret = auxiliary_device_add(aux); - if (ret) - return ret; - ret = devm_add_action_or_reset(dev, ti_sn65dsi86_delete_aux, aux); - if (!ret) - *aux_out = aux; - - return ret; -} - /* ----------------------------------------------------------------------------- * AUX Adapter */ @@ -671,7 +615,12 @@ static int ti_sn_aux_probe(struct auxiliary_device *adev, * The eDP to MIPI bridge parts don't work until the AUX channel is * setup so we don't add it in the main driver probe, we add it now. */ - return ti_sn65dsi86_add_aux_device(pdata, &pdata->bridge_aux, "bridge"); + pdata->bridge_aux = devm_auxiliary_device_create(pdata->dev, "bridge", + NULL, 0); + if (IS_ERR(pdata->bridge_aux)) + return PTR_ERR(pdata->bridge_aux); + + return 0; } static const struct auxiliary_device_id ti_sn_aux_id_table[] = { @@ -1950,15 +1899,17 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) */ if (IS_ENABLED(CONFIG_OF_GPIO)) { - ret = ti_sn65dsi86_add_aux_device(pdata, &pdata->gpio_aux, "gpio"); - if (ret) - return ret; + pdata->gpio_aux = devm_auxiliary_device_create(pdata->dev, "gpio", + NULL, 0); + if (IS_ERR(pdata->gpio_aux)) + return PTR_ERR(pdata->gpio_aux); } if (IS_ENABLED(CONFIG_PWM)) { - ret = ti_sn65dsi86_add_aux_device(pdata, &pdata->pwm_aux, "pwm"); - if (ret) - return ret; + pdata->pwm_aux = devm_auxiliary_device_create(pdata->dev, "pwm", + NULL, 0); + if (IS_ERR(pdata->pwm_aux)) + return PTR_ERR(pdata->pwm_aux); } /* @@ -1967,7 +1918,12 @@ static int ti_sn65dsi86_probe(struct i2c_client *client) * AUX channel is there and this is a very simple solution to the * dependency problem. */ - return ti_sn65dsi86_add_aux_device(pdata, &pdata->aux_aux, "aux"); + pdata->aux_aux = devm_auxiliary_device_create(pdata->dev, "aux", + NULL, 0); + if (IS_ERR(pdata->aux_aux)) + return PTR_ERR(pdata->aux_aux); + + return 0; } static const struct i2c_device_id ti_sn65dsi86_id[] = {
The auxiliary device creation of this driver is simple enough to use the available auxiliary device creation helper. Use it and remove some boilerplate code. Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/gpu/drm/bridge/ti-sn65dsi86.c | 84 +++++++++-------------------------- 1 file changed, 20 insertions(+), 64 deletions(-)