* Re: [PATCH 16/19] dmaengine: pl330: drop owner assignment
From: Vinod Koul @ 2024-03-28 4:48 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Russell King, Suzuki K Poulose, Mike Leach, James Clark,
Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Andi Shyti, Olivia Mackall, Herbert Xu,
Dmitry Torokhov, Miquel Raynal, Michal Simek, Eric Auger,
Alex Williamson, linux-kernel, coresight, linux-arm-kernel,
linux-stm32, linux-i2c, linux-crypto, dmaengine, linux-input, kvm
In-Reply-To: <20240326-module-owner-amba-v1-16-4517b091385b@linaro.org>
On 26-03-24, 21:23, Krzysztof Kozlowski wrote:
> Amba bus core already sets owner, so driver does not need to.
Acked-by: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [PATCH v1 1/4] dt-bindings: input: Add Himax HX83102J touchscreen
From: Allen Lin @ 2024-03-28 6:05 UTC (permalink / raw)
To: Conor Dooley
Cc: Conor Dooley, Rob Herring, dmitry.torokhov,
krzysztof.kozlowski+dt, conor+dt, jikos, benjamin.tissoires,
linux-input, devicetree, linux-kernel
In-Reply-To: <20240327-pegboard-deodorize-17d8b0f1e31c@spud>
Conor Dooley <conor@kernel.org> 於 2024年3月28日 週四 上午12:44寫道：
>
> On Wed, Mar 27, 2024 at 03:48:48PM +0800, Allen Lin wrote:
> > Conor Dooley <conor@kernel.org> 於 2024年3月27日 週三 上午3:28寫道：
> > >
> > > On Tue, Mar 26, 2024 at 06:40:28PM +0800, Allen Lin wrote:
> > > > Conor Dooley <conor.dooley@microchip.com> 於 2024年3月26日 週二 下午4:48寫道：
> > > > >
> > > > > On Tue, Mar 26, 2024 at 01:46:56PM +0800, Allen Lin wrote:
> > > > > > Conor Dooley <conor@kernel.org> 於 2024年3月23日 週六 上午2:34寫道：
> > > > > > >
> > > > > > > On Fri, Mar 22, 2024 at 01:30:09PM -0500, Rob Herring wrote:
> > > > > > > > On Fri, Mar 22, 2024 at 05:54:08PM +0000, Conor Dooley wrote:
> > > > > > > > > On Fri, Mar 22, 2024 at 04:56:03PM +0800, Allen_Lin wrote:
> > > > > > > > > > Add the HX83102j touchscreen device tree bindings documents.
> > > > > > > > > > HX83102j is a Himax TDDI touchscreen controller.
> > > > > > > > > > It's power sequence should be bound with a lcm driver, thus it
> > > > > > > > > > needs to be a panel follower. Others are the same as normal SPI
> > > > > > > > > > touchscreen controller.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> > > > > > > > >
> > > > > > > > > note to self/Krzysztof/Rob:
> > > > > > > > > There was a previous attempt at this kind of device. This version looks
> > > > > > > > > better but might be incomplete given there's a bunch more properties in
> > > > > > > > > that patchset:
> > > > > > > > > https://lore.kernel.org/all/20231017091900.801989-1-tylor_yang@himax.corp-partner.google.com/
> > > > > > > >
> > > > > > > > Those don't look like properties we want coming back.
> > > > > > >
> > > > > > > Oh, I don't want most of them coming back either. There are some
> > > > > > > supplies in there though that I think we would like to come back, no?
> > > > > > > Maybe this particular device doesn't have any supplies, but that doesn't
> > > > > > > really seem credible.
> > > > > >
> > > > > > We will use Firmware-name in Device Tree.
> > > > >
> > > > > > For power supply settings, because there may be other device using
> > > > > > same regulator.
> > > > >
> > > > > If there are other devices using the same regulator is it more
> > > > > important that you document it so that it doesn't get disabled by the
> > > > > other users.
> > > > >
> > > > > > We plan to define it as an optional property for user to control in
> > > > > > next release.
> > > > >
> > > > > I don't see how the regulator would not be required, the device doesn't
> > > > > function without power.
> > > > >
> > > > > Thanks,
> > > > > Conor.
> > > >
> > > > I will set power supply as required.
> > > > The description of power supply as below,
> > > >
> > > > properties:
> > > > vccd-supply:
> > > > description: A phandle for the regualtor supplying IO power. Should be own
> > > > by TPIC only.
> > >
> > > What does "owned by TPIC" only mean? Why would the vccd supply not be
> > > allowed to be shared with another device?
> > >
> > > > This works for TP digital IO only, main power is
> > > > given by display part VSP/VSN power source which is controlled
> > > > by lcm driver.
> > >
> > > What drivers control things doesn't really matter here, we're just
> > > describing the hardware. If there's another supply to the controller,
> > > then document it too please.
> > >
> >
> > Below is IC power sequence introduction.
> > https://github.com/HimaxSoftware/Doc/tree/main/Himax_Chipset_Power_Sequence
> >
> > TDDI IC, which means Touch and Display Driver is integrated in one IC,
> > So some power supplies will be controlled by Display driver.
>
> If someone was to turn off the supplies, would the touch component stop
> working? The document says that these supplies must be turned on before
> the touch supplies, so I'm going to assume that both are needed for the
> device to function.
>
> > In yaml Document, can we just list power supplies controlled by touch driver?
>
> If the touch part of this device needs the supplies to function, then
> you need to mention them, regardless of where they're controlled. All we
> are doing in the binding is describing the hardware. What drivers do
> what depends entirely on what software you're using.
>
OK, I will list all supplies required by the driver in the Yaml
document as shown below,
properties:
compatible:
const: himax,hx83102j
reg:
maxItems: 1
interrupts:
maxItems: 1
reset-gpios:
maxItems: 1
vccd-supply:
description: A phandle for the regualtor supplying IO power.
vsn-supply:
description: Negative supply regulator.
vsp-supply:
description: Positive supply regulator.
ddreset-gpios:
description: A phandle of gpio for display reset controlled by the
LCD driver.
required:
- compatible
- reg
- interrupts
- reset-gpios
- panel
- vccd-supply
- vsn-supply
- vsp-supply
- ddreset-gpios
Thanks,
Allen
^ permalink raw reply
* Re: [RESEND PATCH v6 3/3] input: pm8xxx-vibrator: add new SPMI vibrator support
From: Fenglin Wu @ 2024-03-28 6:52 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Dmitry Baryshkov, linux-arm-msm, linux-kernel,
krzysztof.kozlowski+dt, robh+dt, agross, andersson, Konrad Dybcio,
linux-input, quic_collinsd, quic_subbaram, quic_kamalw, jestar,
Luca Weiss
In-Reply-To: <ZRhKAWYBLcBZHc73@google.com>
On 2023/10/1 0:17, Dmitry Torokhov wrote:
> On Mon, Sep 25, 2023 at 10:54:45AM +0800, Fenglin Wu wrote:
>>
>>
>> On 9/24/2023 3:07 AM, Dmitry Baryshkov wrote:
>>>> +
>>>> + switch (vib->data->hw_type) {
>>>> + case SSBI_VIB:
>>>> mask = SSBI_VIB_DRV_LEVEL_MASK;
>>>> shift = SSBI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB:
>>>> + mask = SPMI_VIB_DRV_LEVEL_MASK;
>>>> + shift = SPMI_VIB_DRV_SHIFT;
>>>> + break;
>>>> + case SPMI_VIB_GEN2:
>>>> + mask = SPMI_VIB_GEN2_DRV_MASK;
>>>> + shift = SPMI_VIB_GEN2_DRV_SHIFT;
>>>> + break;
>>>> + default:
>>>> + return -EINVAL;
>>> Could you please move the switch to the previous patch? Then it would
>>> be more obvious that you are just adding the SPMI_VIB_GEN2 here.
>>>
>>> Other than that LGTM.
>>
>> Sure, I can move the switch to the previous refactoring patch.
>
> Actually, the idea of having a const "reg" or "chip", etc. structure is
> to avoid this kind of runtime checks based on hardware type and instead
> use common computation. I believe you need to move mask and shift into
> the chip-specific structure and avoid defining hw_type.
>
> Thanks.
Hi Dmitry,
The v7 changes have been pending for a while, I am not sure if you are
still insist on this. As I explained, I actually did it this way in v2
and it got updated to this by following other comments.
Can you respond and tell me if you prefer changes similar to v2? I can
update and push v8 by following your suggestion.
v7:
https://lore.kernel.org/linux-arm-msm/20231108-pm8xxx-vibrator-v7-0-632c731d25a8@quicinc.com/
v2:
https://lore.kernel.org/linux-arm-msm/20230718062639.2339589-3-quic_fenglinw@quicinc.com/
Thanks
>
^ permalink raw reply
* Re: [PATCH 01/19] amba: store owner from modules with amba_driver_register()
From: Krzysztof Kozlowski @ 2024-03-28 7:51 UTC (permalink / raw)
To: Andi Shyti
Cc: Russell King, Suzuki K Poulose, Mike Leach, James Clark,
Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Olivia Mackall, Herbert Xu, Vinod Koul,
Dmitry Torokhov, Miquel Raynal, Michal Simek, Eric Auger,
Alex Williamson, linux-kernel, coresight, linux-arm-kernel,
linux-stm32, linux-i2c, linux-crypto, dmaengine, linux-input, kvm
In-Reply-To: <6p4cdmbhrezm7fqbe3kgrkblqgrhaq4fgiw5x4n5dnptii7kjp@vmbj2pkjglp7>
On 27/03/2024 21:33, Andi Shyti wrote:
> Hi Krzysztof,
>
> ...
>
>> /**
>> - * amba_driver_register - register an AMBA device driver
>> + * __amba_driver_register - register an AMBA device driver
>> * @drv: amba device driver structure
>> + * @owner: owning module/driver
>> *
>> * Register an AMBA device driver with the Linux device model
>> * core. If devices pre-exist, the drivers probe function will
>> * be called.
>> */
>> -int amba_driver_register(struct amba_driver *drv)
>> +int __amba_driver_register(struct amba_driver *drv,
>
> ...
>
>> +/*
>> + * use a macro to avoid include chaining to get THIS_MODULE
>> + */
>
> Should the documentation be moved here? Well... I don't see any
> documentation linking this file yet, but in case it comes we want
> documented amba_driver_register() rather than
> __amba_driver_register().
>
That's just a wrapper, not API... why would we care to have kerneldoc
for it?
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 14/19] i2c: nomadik: drop owner assignment
From: Linus Walleij @ 2024-03-28 8:13 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Russell King, Suzuki K Poulose, Mike Leach, James Clark,
Alexander Shishkin, Maxime Coquelin, Alexandre Torgue, Andi Shyti,
Olivia Mackall, Herbert Xu, Vinod Koul, Dmitry Torokhov,
Miquel Raynal, Michal Simek, Eric Auger, Alex Williamson,
linux-kernel, coresight, linux-arm-kernel, linux-stm32, linux-i2c,
linux-crypto, dmaengine, linux-input, kvm
In-Reply-To: <20240326-module-owner-amba-v1-14-4517b091385b@linaro.org>
On Tue, Mar 26, 2024 at 9:24 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
> Amba bus core already sets owner, so driver does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH 19/19] vfio: amba: drop owner assignment
From: Eric Auger @ 2024-03-28 8:15 UTC (permalink / raw)
To: Krzysztof Kozlowski, Russell King, Suzuki K Poulose, Mike Leach,
James Clark, Alexander Shishkin, Maxime Coquelin,
Alexandre Torgue, Linus Walleij, Andi Shyti, Olivia Mackall,
Herbert Xu, Vinod Koul, Dmitry Torokhov, Miquel Raynal,
Michal Simek, Alex Williamson
Cc: linux-kernel, coresight, linux-arm-kernel, linux-stm32, linux-i2c,
linux-crypto, dmaengine, linux-input, kvm
In-Reply-To: <20240326-module-owner-amba-v1-19-4517b091385b@linaro.org>
Hi,
On 3/26/24 21:23, Krzysztof Kozlowski wrote:
> Amba bus core already sets owner, so driver does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> ---
>
> Depends on first amba patch.
> ---
> drivers/vfio/platform/vfio_amba.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/vfio/platform/vfio_amba.c b/drivers/vfio/platform/vfio_amba.c
> index 485c6f9161a9..ff8ff8480968 100644
> --- a/drivers/vfio/platform/vfio_amba.c
> +++ b/drivers/vfio/platform/vfio_amba.c
> @@ -134,7 +134,6 @@ static struct amba_driver vfio_amba_driver = {
> .id_table = vfio_amba_ids,
> .drv = {
> .name = "vfio-amba",
> - .owner = THIS_MODULE,
> },
> .driver_managed_dma = true,
> };
>
>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks
Eric
^ permalink raw reply
* Re: [PATCH] input/touchscreen: synaptics_tcm_oncell: add driver
From: Krzysztof Kozlowski @ 2024-03-28 8:34 UTC (permalink / raw)
To: Frieder Hannenheim, dmitry.torokhov, linux-input, linux-kernel
In-Reply-To: <20240327214643.7055-1-friederhannenheim@riseup.net>
On 27/03/2024 22:39, Frieder Hannenheim wrote:
> This is a bit of a stripped down and partially reworked driver for the
> synaptics_tcm_oncell touchscreen. I based my work off the driver in the
> LineageOS kernel found at
> https://github.com/LineageOS/android_kernel_oneplus_sm8250 branch
> lineage-20. The code was originally written by OnePlus developers but
> I'm not sure how to credit them correctly.
>
> Currently the driver is in a pretty good shape, the only thing that is
> not working is setting a report config. To me it looks like some data is
> sent by the touchscreen firmware after setting the report config that is
> making the irq handler crash. Sadly I haven't been able to test out why.
> The driver works fine also with the default touch report config so maybe
> we can just use that and not set our own.
>
> Signed-off-by: Frieder Hannenheim <friederhannenheim@riseup.net>
> ---
> .../input/touchscreen/syna,s3908.yaml | 63 +
Please run scripts/checkpatch.pl and fix reported warnings. Then please
run `scripts/checkpatch.pl --strict` and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.
Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.
Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.
You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time, thus I will skip this patch entirely till you follow
the process allowing the patch to be tested.
Please kindly resend and include all necessary To/Cc entries.
> drivers/input/touchscreen/Kconfig | 11 +
(skipping untested bindings)
...
> +
> +static int syna_tcm_probe(struct i2c_client *client)
> +{
> + struct syna_tcm_data *tcm_info;
> + int err;
> +
> + pr_info("starting probe for syna_tcm_oncell touchscreen");
No, drop. Drop such msgs everywhere if you have more than only here.
> +
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_I2C | I2C_FUNC_SMBUS_BYTE_DATA |
> + I2C_FUNC_SMBUS_I2C_BLOCK))
> + return -ENODEV;
> +
> + tcm_info = devm_kzalloc(&client->dev, sizeof(*tcm_info), GFP_KERNEL);
> + if (!tcm_info)
> + return -ENOMEM;
> +
> + i2c_set_clientdata(client, tcm_info);
> + tcm_info->client = client;
> + tcm_info->response_buf = NULL;
> +
> + of_property_read_u32(client->dev.of_node, "max-objects",
> + &tcm_info->touchpanel_max_objects);
> +
> + tcm_info->reset_gpio =
> + gpiod_get_index(&client->dev, "reset", 0, GPIOD_OUT_HIGH);
Misaligned / wrongly wrapped. There is no wrapping of code after =.
> +
> + tcm_info->regulators[SYNA_TCM_ONCELL_REGULATOR_VDD].supply = "vdd";
> + tcm_info->regulators[SYNA_TCM_ONCELL_REGULATOR_VCC].supply = "vcc";
> + err = devm_regulator_bulk_get(&client->dev,
> + ARRAY_SIZE(tcm_info->regulators),
> + tcm_info->regulators);
> + if (err)
> + return err;
> +
> + // TODO: uncomment once syna_tcm_power_off is implemented
> + // err = devm_add_action_or_reset(&client->dev, syna_tcm_oncell_power_off, tcm_info);
> + // if (err)
> + // return err;
No dead code.
> +
> + err = syna_tcm_power_on(tcm_info);
> + if (err < 0)
> + return err;
> +
> + // This needs to happen before the first write to the device
> + err = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + syna_irq_handler,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + "syna_tcm_oncell_irq", tcm_info);
> + if (err)
> + return err;
> +
> + err = syna_tcm_run_application_firmware(tcm_info);
> + if (err < 0)
> + return err;
> +
> + // err = syna_tcm_set_normal_report_config(tcm_info);
> + // if (err < 0)
> + // pr_err("syna_tcm: failed to set normal touch report config")
No dead code in the driver.
> +
> + err = syna_tcm_get_report_config(tcm_info);
> + if (err < 0)
> + return err;
> +
> + tcm_info->input = devm_input_allocate_device(&client->dev);
> + if (!tcm_info->input)
> + return -ENOMEM;
> +
> + tcm_info->input->name = TOUCHPANEL_DEVICE;
> + tcm_info->input->id.bustype = BUS_I2C;
> +
> + input_set_abs_params(tcm_info->input, ABS_MT_POSITION_X, 0,
> + le2_to_uint(tcm_info->app_info.max_x), 0, 0);
> + input_set_abs_params(tcm_info->input, ABS_MT_POSITION_Y, 0,
> + le2_to_uint(tcm_info->app_info.max_y), 0, 0);
> + input_set_abs_params(tcm_info->input, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
> + input_set_abs_params(tcm_info->input, ABS_MT_TOUCH_MINOR, 0, 255, 0, 0);
> + input_set_abs_params(tcm_info->input, ABS_MT_PRESSURE, 0, 255, 0, 0);
> +
> + touchscreen_parse_properties(tcm_info->input, true, &tcm_info->prop);
> +
> + err = input_mt_init_slots(tcm_info->input,
> + tcm_info->touchpanel_max_objects,
> + INPUT_MT_DIRECT);
> + if (err)
> + return err;
> +
> + input_set_drvdata(tcm_info->input, tcm_info);
> +
> + err = input_register_device(tcm_info->input);
> + if (err)
> + return err;
> +
> + pr_info("syna_tcm: probe done");
No, no simple function success messages. There is already infrastructure
for this (tracing, sysfs).
> + tcm_info->initialize_done = true;
> +
> + return 0;
> +}
> +
> +static const struct of_device_id syna_driver_ids[] = {
> + {
> + .compatible = "syna,s3908",
> + },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, syna_driver_ids);
> +
> +static const struct i2c_device_id syna_i2c_ids[] = { { TOUCHPANEL_DEVICE, 0 },
That does not look like kernel coding style.
> + {} };
> +MODULE_DEVICE_TABLE(i2c, syna_i2c_ids);
> +
> +// static const struct dev_pm_ops syna_pm_ops = {
> +// .suspend = syna_i2c_suspend,
> +// .resume = syna_i2c_resume,
> +// };
Please do not submit dead code.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 01/19] amba: store owner from modules with amba_driver_register()
From: Andi Shyti @ 2024-03-28 9:09 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Russell King, Suzuki K Poulose, Mike Leach, James Clark,
Alexander Shishkin, Maxime Coquelin, Alexandre Torgue,
Linus Walleij, Olivia Mackall, Herbert Xu, Vinod Koul,
Dmitry Torokhov, Miquel Raynal, Michal Simek, Eric Auger,
Alex Williamson, linux-kernel, coresight, linux-arm-kernel,
linux-stm32, linux-i2c, linux-crypto, dmaengine, linux-input, kvm
In-Reply-To: <c33833ad-9102-40e6-97bf-9a4e1bf0a3d9@linaro.org>
Hi Krzysztof,
> >> /**
> >> - * amba_driver_register - register an AMBA device driver
> >> + * __amba_driver_register - register an AMBA device driver
> >> * @drv: amba device driver structure
> >> + * @owner: owning module/driver
> >> *
> >> * Register an AMBA device driver with the Linux device model
> >> * core. If devices pre-exist, the drivers probe function will
> >> * be called.
> >> */
> >> -int amba_driver_register(struct amba_driver *drv)
> >> +int __amba_driver_register(struct amba_driver *drv,
> >
> > ...
> >
> >> +/*
> >> + * use a macro to avoid include chaining to get THIS_MODULE
> >> + */
> >
> > Should the documentation be moved here? Well... I don't see any
> > documentation linking this file yet, but in case it comes we want
> > documented amba_driver_register() rather than
> > __amba_driver_register().
> >
>
> That's just a wrapper, not API... why would we care to have kerneldoc
> for it?
Because everyone should use the wrapper while the real function
will be used only once or twice.
I see also that this is a common practice which I don't surely
like.
In any case there is no documentation exported for AMBA so that
this discussion does not bring any tangible benefit.
So that, considering that it's a good improvement,
Reviewed-by: Andi Shyti <andi.shyti@kernel.org>
Andi
^ permalink raw reply
* [GIT PULL] Immutable branch between MFD, Auxdisplay, HID and FB due for the v6.9 merge window
From: Lee Jones @ 2024-03-28 10:11 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: andy, daniel.thompson, jingoohan1, deller, robin, javierm,
dri-devel, linux-fbdev, linux-input, linux-pwm
In-Reply-To: <20240305162425.23845-1-tzimmermann@suse.de>
Enjoy!
The following changes since commit 4cece764965020c22cff7665b18a012006359095:
Linux 6.9-rc1 (2024-03-24 14:10:05 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git ib-backlight-auxdisplay-hid-fb-v6.9
for you to fetch changes up to 0a4be7263749945a3882f7a0e2e5b1c45c31064e:
backlight: Add controls_device callback to struct backlight_ops (2024-03-28 10:09:06 +0000)
----------------------------------------------------------------
Immutable branch between MFD, Auxdisplay, HID and FB due for the v6.9 merge window
----------------------------------------------------------------
Thomas Zimmermann (10):
backlight: Match backlight device against struct fb_info.bl_dev
auxdisplay: ht16k33: Remove struct backlight_ops.check_fb
hid: hid-picolcd: Fix initialization order
hid: hid-picolcd: Remove struct backlight_ops.check_fb
backlight: aat2870-backlight: Remove struct backlight.check_fb
backlight: pwm-backlight: Remove struct backlight_ops.check_fb
fbdev: sh_mobile_lcdc_fb: Remove struct backlight_ops.check_fb
fbdev: ssd1307fb: Init backlight before registering framebuffer
fbdev: ssd1307fb: Remove struct backlight_ops.check_fb
backlight: Add controls_device callback to struct backlight_ops
drivers/auxdisplay/ht16k33.c | 8 --------
drivers/hid/hid-picolcd_backlight.c | 7 -------
drivers/hid/hid-picolcd_core.c | 14 +++++++-------
drivers/hid/hid-picolcd_fb.c | 6 ++++++
drivers/video/backlight/aat2870_bl.c | 7 -------
drivers/video/backlight/backlight.c | 8 ++++++--
drivers/video/backlight/bd6107.c | 12 ++++++------
drivers/video/backlight/gpio_backlight.c | 12 ++++++------
drivers/video/backlight/lv5207lp.c | 12 ++++++------
drivers/video/backlight/pwm_bl.c | 12 ------------
drivers/video/fbdev/core/fb_backlight.c | 6 ++++++
drivers/video/fbdev/sh_mobile_lcdcfb.c | 7 -------
drivers/video/fbdev/ssd1307fb.c | 31 +++++++++++--------------------
include/linux/backlight.h | 16 ++++++++--------
include/linux/fb.h | 9 +++++++++
include/linux/pwm_backlight.h | 1 -
16 files changed, 71 insertions(+), 97 deletions(-)
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH 04/19] platform: chrome: drop owner assignment
From: Tzung-Bi Shih @ 2024-03-28 10:20 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rafael J. Wysocki, Len Brown, Robert Moore, Dmitry Torokhov,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Benson Leung, Corentin Chary, Luke D. Jones, Hans de Goede,
Ilpo Järvinen, Thadeu Lima de Souza Cascardo,
Daniel Oliveira Nascimento, Pali Rohár, Matan Ziv-Av,
Mattia Dongili, Azael Avalos, Richard Cochran, Jeff Sipek,
Ajay Kaher, Alexey Makhalov, VMware PV-Drivers Reviewers,
Theodore Ts'o, Jason A. Donenfeld, Rafael J. Wysocki,
linux-acpi, linux-kernel, acpica-devel, linux-input, netdev,
chrome-platform, platform-driver-x86
In-Reply-To: <20240327-b4-module-owner-acpi-v1-4-725241a2d224@linaro.org>
On Wed, Mar 27, 2024 at 08:43:51AM +0100, Krzysztof Kozlowski wrote:
> ACPI bus core already sets the .owner, so driver does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Tzung-Bi Shih <tzungbi@kernel.org>
I would prefer to use "platform/chrome: wilco_ec: " prefix though.
^ permalink raw reply
* Re: [PATCH 06/19] platform: classmate-laptop: drop owner assignment
From: Thadeu Lima de Souza Cascardo @ 2024-03-28 10:24 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Rafael J. Wysocki, Len Brown, Robert Moore, Dmitry Torokhov,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Benson Leung, Tzung-Bi Shih, Corentin Chary, Luke D. Jones,
Hans de Goede, Ilpo Järvinen, Daniel Oliveira Nascimento,
Pali Rohár, Matan Ziv-Av, Mattia Dongili, Azael Avalos,
Richard Cochran, Jeff Sipek, Ajay Kaher, Alexey Makhalov,
VMware PV-Drivers Reviewers, Theodore Ts'o,
Jason A. Donenfeld, Rafael J. Wysocki, linux-acpi, linux-kernel,
acpica-devel, linux-input, netdev, chrome-platform,
platform-driver-x86
In-Reply-To: <20240327-b4-module-owner-acpi-v1-6-725241a2d224@linaro.org>
On Wed, Mar 27, 2024 at 08:43:53AM +0100, Krzysztof Kozlowski wrote:
> ACPI bus core already sets the .owner, so driver does not need to.
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Acked-by: Thadeu Lima de Souza Cascardo <cascardo@holoscopio.com>
> ---
> drivers/platform/x86/classmate-laptop.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/platform/x86/classmate-laptop.c b/drivers/platform/x86/classmate-laptop.c
> index 2edaea2492df..87462e7c6219 100644
> --- a/drivers/platform/x86/classmate-laptop.c
> +++ b/drivers/platform/x86/classmate-laptop.c
> @@ -434,7 +434,6 @@ static const struct acpi_device_id cmpc_accel_device_ids_v4[] = {
> };
>
> static struct acpi_driver cmpc_accel_acpi_driver_v4 = {
> - .owner = THIS_MODULE,
> .name = "cmpc_accel_v4",
> .class = "cmpc_accel_v4",
> .ids = cmpc_accel_device_ids_v4,
> @@ -660,7 +659,6 @@ static const struct acpi_device_id cmpc_accel_device_ids[] = {
> };
>
> static struct acpi_driver cmpc_accel_acpi_driver = {
> - .owner = THIS_MODULE,
> .name = "cmpc_accel",
> .class = "cmpc_accel",
> .ids = cmpc_accel_device_ids,
> @@ -754,7 +752,6 @@ static const struct acpi_device_id cmpc_tablet_device_ids[] = {
> };
>
> static struct acpi_driver cmpc_tablet_acpi_driver = {
> - .owner = THIS_MODULE,
> .name = "cmpc_tablet",
> .class = "cmpc_tablet",
> .ids = cmpc_tablet_device_ids,
> @@ -996,7 +993,6 @@ static const struct acpi_device_id cmpc_ipml_device_ids[] = {
> };
>
> static struct acpi_driver cmpc_ipml_acpi_driver = {
> - .owner = THIS_MODULE,
> .name = "cmpc",
> .class = "cmpc",
> .ids = cmpc_ipml_device_ids,
> @@ -1064,7 +1060,6 @@ static const struct acpi_device_id cmpc_keys_device_ids[] = {
> };
>
> static struct acpi_driver cmpc_keys_acpi_driver = {
> - .owner = THIS_MODULE,
> .name = "cmpc_keys",
> .class = "cmpc_keys",
> .ids = cmpc_keys_device_ids,
>
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH v10 0/5] Add support for CS40L50
From: Lee Jones @ 2024-03-28 11:44 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
broonie, jeff, patches, linux-sound, linux-input, devicetree
In-Reply-To: <20240320192956.2395731-1-jogletre@opensource.cirrus.com>
On Wed, 20 Mar 2024, James Ogletree wrote:
> Changes in v10:
> - Minor refactoring and logical improvements all around
> - Renamed and added supplies
>
> Changes in v9:
> - Fixed empty struct by utilizing cs_dsp's post_run callback
> - Style fixes in MFD driver
>
> Changes in v8:
> - set_sysclk() -> set_bclk_ratio()
> - Added ID table to codec driver
> - Style improvements
> - Fixed ordering of new write sequence operations
>
> Changes in v7:
> - Fixed sparse warning
> - Moved write sequences to private data structure
> - Logical and style improvements in write sequence interface
>
> Changes in v6:
> - Updated write sequencer interface to be control-name based
> - Fixed a race condition and non-handling of repeats in playback callback
> - Stylistic and logical improvements all around
>
> Changes in v5:
> - Added a codec sub-device to support I2S streaming
> - Moved write sequencer code from cirrus_haptics to cs_dsp
> - Reverted cirrus_haptics library; future Cirrus input
> drivers will export and utilize cs40l50_vibra functions
> - Added more comments
> - Many small stylistic and logical improvements
>
> Changes in v4:
> - Moved from Input to MFD
> - Moved common Cirrus haptic functions to a library
> - Incorporated runtime PM framework
> - Many style improvements
>
> Changes in v3:
> - YAML formatting corrections
> - Fixed typo in MAINTAINERS
> - Used generic node name "haptic-driver"
> - Fixed probe error code paths
> - Switched to "sizeof(*)"
> - Removed tree reference in MAINTAINERS
>
> Changes in v2:
> - Fixed checkpatch warnings
>
> James Ogletree (5):
> firmware: cs_dsp: Add write sequence interface
> dt-bindings: input: cirrus,cs40l50: Add initial DT binding
> mfd: cs40l50: Add support for CS40L50 core driver
> Input: cs40l50 - Add support for the CS40L50 haptic driver
> ASoC: cs40l50: Support I2S streaming to CS40L50
>
> .../bindings/input/cirrus,cs40l50.yaml | 68 +++
> MAINTAINERS | 12 +
> drivers/firmware/cirrus/cs_dsp.c | 278 +++++++++
> drivers/input/misc/Kconfig | 10 +
> drivers/input/misc/Makefile | 1 +
> drivers/input/misc/cs40l50-vibra.c | 577 ++++++++++++++++++
> drivers/mfd/Kconfig | 30 +
> drivers/mfd/Makefile | 4 +
> drivers/mfd/cs40l50-core.c | 570 +++++++++++++++++
> drivers/mfd/cs40l50-i2c.c | 68 +++
> drivers/mfd/cs40l50-spi.c | 68 +++
> include/linux/firmware/cirrus/cs_dsp.h | 27 +
> include/linux/mfd/cs40l50.h | 137 +++++
> sound/soc/codecs/Kconfig | 11 +
> sound/soc/codecs/Makefile | 2 +
> sound/soc/codecs/cs40l50-codec.c | 308 ++++++++++
> 16 files changed, 2171 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/cirrus,cs40l50.yaml
> create mode 100644 drivers/input/misc/cs40l50-vibra.c
> create mode 100644 drivers/mfd/cs40l50-core.c
> create mode 100644 drivers/mfd/cs40l50-i2c.c
> create mode 100644 drivers/mfd/cs40l50-spi.c
> create mode 100644 include/linux/mfd/cs40l50.h
> create mode 100644 sound/soc/codecs/cs40l50-codec.c
MFD part looks okay.
I need Acks from all of the other maintainers before I can process the set.
--
Lee Jones [李琼斯]
^ permalink raw reply
* [PATCH][next] HID: sony: remove redundant assignment to variable ret
From: Colin Ian King @ 2024-03-28 12:22 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, linux-input
Cc: kernel-janitors, linux-kernel
The variable ret is being assigned a value that is never read
afterwards. The assignment is redundant and can be removed.
Cleans up clang scan build warning:
drivers/hid/hid-sony.c:2020:3: warning: Value stored to 'ret'
is never read [deadcode.DeadStores]
Signed-off-by: Colin Ian King <colin.i.king@gmail.com>
---
drivers/hid/hid-sony.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index ebc0aa4e4345..1d37b39e1171 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -2016,8 +2016,6 @@ static int sony_input_configured(struct hid_device *hdev,
} else if (sc->quirks & MOTION_CONTROLLER) {
sony_init_output_report(sc, motion_send_output_report);
- } else {
- ret = 0;
}
if (sc->quirks & SONY_LED_SUPPORT) {
--
2.39.2
^ permalink raw reply related
* Re: [PATCH RFC 1/7] regulator: devres: add APIs for reference supplies
From: Jonathan Cameron @ 2024-03-28 13:47 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-1-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:50 -0500
David Lechner <dlechner@baylibre.com> wrote:
> A common use case for regulators is to supply a reference voltage to an
> analog input or output device. This adds two new devres APIs to get,
> enable, and get the voltage in a single call. This allows eliminating
> boilerplate code in drivers that use reference supplies in this way.
>
> devm_regulator_get_enable_get_voltage() is intended for cases where the
Maybe avoid the double get?
devm_regulator_get_enable_read_voltage() perhaps?
> supply is required to provide an external reference voltage.
>
> devm_regulator_get_optional_enable_get_voltage() is intended for cases
> where the supply is optional and device typically uses an internal
> reference voltage if the supply is not available.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
In general I'll find this very useful (there are 50+ incidence
of the pattern this can replace in IIO).
I would keep it more similar to other devm regulator related functions
and not do error printing internally though.
Jonathan
> ---
> Documentation/driver-api/driver-model/devres.rst | 2 +
> drivers/regulator/devres.c | 83 ++++++++++++++++++++++++
> include/linux/regulator/consumer.h | 14 ++++
> 3 files changed, 99 insertions(+)
>
> diff --git a/Documentation/driver-api/driver-model/devres.rst b/Documentation/driver-api/driver-model/devres.rst
> index 7be8b8dd5f00..fd954d09e13c 100644
> --- a/Documentation/driver-api/driver-model/devres.rst
> +++ b/Documentation/driver-api/driver-model/devres.rst
> @@ -433,9 +433,11 @@ REGULATOR
> devm_regulator_bulk_put()
> devm_regulator_get()
> devm_regulator_get_enable()
> + devm_regulator_get_enable_get_voltage()
> devm_regulator_get_enable_optional()
> devm_regulator_get_exclusive()
> devm_regulator_get_optional()
> + devm_regulator_get_optional_enable_get_voltage()
> devm_regulator_irq_helper()
> devm_regulator_put()
> devm_regulator_register()
> diff --git a/drivers/regulator/devres.c b/drivers/regulator/devres.c
> index 90bb0d178885..e240926defc5 100644
> --- a/drivers/regulator/devres.c
> +++ b/drivers/regulator/devres.c
> @@ -145,6 +145,89 @@ struct regulator *devm_regulator_get_optional(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_regulator_get_optional);
>
> +static int _devm_regulator_get_enable_get_voltage(struct device *dev,
> + const char *id,
> + bool silent_enodev)
> +{
> + struct regulator *r;
> + int ret;
> +
> + /*
> + * Since we need a real voltage, we use devm_regulator_get_optional()
> + * here to avoid getting a dummy regulator if the supply is not present.
> + */
> + r = devm_regulator_get_optional(dev, id);
> + if (silent_enodev && PTR_ERR(r) == -ENODEV)
> + return -ENODEV;
> + if (IS_ERR(r))
> + return dev_err_probe(dev, PTR_ERR(r),
There isn't any obvious precedence that I can see for using dev_err_probe() in these
calls. I'd not introduce it here.
It's probably enough to print an error message at the caller that
just says we failed to get the voltage (rather than which step failed)./
> + "failed to get regulator '%s'\n", id);
> +
> + ret = regulator_enable(r);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to enable regulator '%s'\n", id);
> +
> + ret = devm_add_action_or_reset(dev, regulator_action_disable, r);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to add disable action for regulator '%s'\n",
> + id);
> +
> + ret = regulator_get_voltage(r);
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "failed to get voltage for regulator '%s'\n",
> + id);
> +
> + return ret;
> +}
> +
> +/**
> + * devm_regulator_get_enable_get_voltage - Resource managed regulator get and
> + * enable that returns the voltage
> + * @dev: device to supply
> + * @id: supply name or regulator ID.
> + *
> + * Get and enable regulator for duration of the device life-time.
> + * regulator_disable() and regulator_put() are automatically called on driver
> + * detach. See regulator_get_optional(), regulator_enable(), and
> + * regulator_get_voltage() for more information.
> + *
> + * This is a convenience function for supplies that provide a reference voltage
> + * where the consumer driver just needs to know the voltage and keep the
> + * regulator enabled. Also, as a convenience, this prints error messages on
> + * failure.
> + *
> + * Returns: voltage in microvolts on success, or an error code on failure.
> + */
> +int devm_regulator_get_enable_get_voltage(struct device *dev, const char *id)
> +{
> + return _devm_regulator_get_enable_get_voltage(dev, id, false);
> +}
> +EXPORT_SYMBOL_GPL(devm_regulator_get_enable_get_voltage);
> +
> +/**
> + * devm_regulator_get_optional_enable_get_voltage - Resource managed regulator
> + * get and enable that returns
> + * the voltage
> + * @dev: device to supply
> + * @id: supply name or regulator ID.
> + *
> + * This function is identical to devm_regulator_get_enable_get_voltage() except
> + * that it does not print an error message in the case of -ENODEV. Callers are
> + * expected to handle -ENODEV as a special case instead of passing it on as an
> + * error.
> + *
> + * Returns: voltage in microvolts on success, or an error code on failure.
> + */
> +int devm_regulator_get_optional_enable_get_voltage(struct device *dev,
> + const char *id)
> +{
> + return _devm_regulator_get_enable_get_voltage(dev, id, true);
> +}
> +EXPORT_SYMBOL_GPL(devm_regulator_get_optional_enable_get_voltage);
> +
> static int devm_regulator_match(struct device *dev, void *res, void *data)
> {
> struct regulator **r = res;
> diff --git a/include/linux/regulator/consumer.h b/include/linux/regulator/consumer.h
> index 4660582a3302..35596db521a0 100644
> --- a/include/linux/regulator/consumer.h
> +++ b/include/linux/regulator/consumer.h
> @@ -164,6 +164,8 @@ struct regulator *__must_check devm_regulator_get_optional(struct device *dev,
> const char *id);
> int devm_regulator_get_enable(struct device *dev, const char *id);
> int devm_regulator_get_enable_optional(struct device *dev, const char *id);
> +int devm_regulator_get_enable_get_voltage(struct device *dev, const char *id);
> +int devm_regulator_get_optional_enable_get_voltage(struct device *dev, const char *id);
> void regulator_put(struct regulator *regulator);
> void devm_regulator_put(struct regulator *regulator);
>
> @@ -329,6 +331,18 @@ static inline int devm_regulator_get_enable_optional(struct device *dev,
> return -ENODEV;
> }
>
> +static inline int devm_regulator_get_enable_get_voltage(struct device *dev,
> + const char *id)
> +{
> + return -ENODEV;
> +}
> +
> +static inline int devm_regulator_get_optional_enable_get_voltage(struct device *dev,
> + const char *id)
> +{
> + return -ENODEV;
> +}
> +
> static inline struct regulator *__must_check
> regulator_get_optional(struct device *dev, const char *id)
> {
>
^ permalink raw reply
* Re: [PATCH RFC 6/7] staging: iio: impedance-analyzer: admv1013: Use devm_regulator_get_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 13:50 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-6-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:55 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
Wrong device in patch title. I thought it odd we had a driver for same part
number in staging and out of it!.
Otherwise LGTM
Not sure how we'll merge this set assuming everyone agrees it's a good idea.
Immutable branch probably. On off chance we don't do that, with the patch title
fixed.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Jonathan
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> drivers/staging/iio/impedance-analyzer/ad5933.c | 24 +-----------------------
> 1 file changed, 1 insertion(+), 23 deletions(-)
>
> diff --git a/drivers/staging/iio/impedance-analyzer/ad5933.c b/drivers/staging/iio/impedance-analyzer/ad5933.c
> index 9149d41fe65b..e4942833b793 100644
> --- a/drivers/staging/iio/impedance-analyzer/ad5933.c
> +++ b/drivers/staging/iio/impedance-analyzer/ad5933.c
> @@ -84,7 +84,6 @@
>
> struct ad5933_state {
> struct i2c_client *client;
> - struct regulator *reg;
> struct clk *mclk;
> struct delayed_work work;
> struct mutex lock; /* Protect sensor state */
> @@ -660,13 +659,6 @@ static void ad5933_work(struct work_struct *work)
> }
> }
>
> -static void ad5933_reg_disable(void *data)
> -{
> - struct ad5933_state *st = data;
> -
> - regulator_disable(st->reg);
> -}
> -
> static int ad5933_probe(struct i2c_client *client)
> {
> const struct i2c_device_id *id = i2c_client_get_device_id(client);
> @@ -685,21 +677,7 @@ static int ad5933_probe(struct i2c_client *client)
>
> mutex_init(&st->lock);
>
> - st->reg = devm_regulator_get(&client->dev, "vdd");
> - if (IS_ERR(st->reg))
> - return PTR_ERR(st->reg);
> -
> - ret = regulator_enable(st->reg);
> - if (ret) {
> - dev_err(&client->dev, "Failed to enable specified VDD supply\n");
> - return ret;
> - }
> -
> - ret = devm_add_action_or_reset(&client->dev, ad5933_reg_disable, st);
> - if (ret)
> - return ret;
> -
> - ret = regulator_get_voltage(st->reg);
> + ret = devm_regulator_get_enable_get_voltage(&client->dev, "vdd");
> if (ret < 0)
> return ret;
>
>
^ permalink raw reply
* Re: [PATCH RFC 5/7] iio: frequency: admv1013: Use devm_regulator_get_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 13:51 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-5-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:54 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
>
> The common mode voltage is now passed as a parameter in the init
> functions so we can avoid adding a state member that is only used
> during init.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
LGTM - on basis this might not go through my tree
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
^ permalink raw reply
* Re: [PATCH RFC 4/7] iio: addac: ad74115: Use devm_regulator_get_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 13:58 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-4-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:53 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
There is a change in behaviour in this one. I'd like some
explanation in the patch title for why it is always safe to get
the voltage of avdd_mv when previously it wasn't.
There seems to me to be a corner case where a DTS is not providing
the entry because it's always powered on so we get a stub regulator
but that doesn't matter because we aren't in DIN_THRESHOLD_MOD_AVDD.
After this change that dts is broken as now we read the voltage
whatever.
You could use the optional form and then fail the probe if
in a mode where the value will be used?
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
> drivers/iio/addac/ad74115.c | 28 +++-------------------------
> 1 file changed, 3 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/iio/addac/ad74115.c b/drivers/iio/addac/ad74115.c
> index e6bc5eb3788d..01073d7de6aa 100644
> --- a/drivers/iio/addac/ad74115.c
> +++ b/drivers/iio/addac/ad74115.c
> @@ -199,7 +199,6 @@ struct ad74115_state {
> struct spi_device *spi;
> struct regmap *regmap;
> struct iio_trigger *trig;
> - struct regulator *avdd;
>
> /*
> * Synchronize consecutive operations when doing a one-shot
> @@ -1672,14 +1671,6 @@ static int ad74115_setup(struct iio_dev *indio_dev)
> if (ret)
> return ret;
>
> - if (val == AD74115_DIN_THRESHOLD_MODE_AVDD) {
> - ret = regulator_get_voltage(st->avdd);
> - if (ret < 0)
> - return ret;
> -
> - st->avdd_mv = ret / 1000;
> - }
> -
> st->din_threshold_mode = val;
>
> ret = ad74115_apply_fw_prop(st, &ad74115_dac_bipolar_fw_prop, &val);
> @@ -1788,11 +1779,6 @@ static int ad74115_reset(struct ad74115_state *st)
> return 0;
> }
>
> -static void ad74115_regulator_disable(void *data)
> -{
> - regulator_disable(data);
> -}
> -
> static int ad74115_setup_trigger(struct iio_dev *indio_dev)
> {
> struct ad74115_state *st = iio_priv(indio_dev);
> @@ -1855,19 +1841,11 @@ static int ad74115_probe(struct spi_device *spi)
> indio_dev->modes = INDIO_DIRECT_MODE;
> indio_dev->info = &ad74115_info;
>
> - st->avdd = devm_regulator_get(dev, "avdd");
> - if (IS_ERR(st->avdd))
> - return PTR_ERR(st->avdd);
> -
> - ret = regulator_enable(st->avdd);
> - if (ret) {
> - dev_err(dev, "Failed to enable avdd regulator\n");
> + ret = devm_regulator_get_enable_get_voltage(dev, "avdd");
> + if (ret < 0)
> return ret;
> - }
>
> - ret = devm_add_action_or_reset(dev, ad74115_regulator_disable, st->avdd);
> - if (ret)
> - return ret;
> + st->avdd_mv = ret / 1000;
>
> ret = devm_regulator_bulk_get_enable(dev, ARRAY_SIZE(regulator_names),
> regulator_names);
>
^ permalink raw reply
* Re: [PATCH RFC 2/7] hwmon: (adc128d818) Use devm_regulator_get_optional_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 14:05 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-2-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:51 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can reduce boilerplate code and eliminate the driver remove()
> function by using devm_regulator_get_optional_enable_get_voltage().
>
> A new external_vref flag is added since we no longer have the handle
> to the regulator to check if it is present.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
One trivial thing.
With that tidied up...
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> hwmon_dev = devm_hwmon_device_register_with_groups(dev, client->name,
> data, adc128_groups);
> if (IS_ERR(hwmon_dev)) {
> err = PTR_ERR(hwmon_dev);
> - goto error;
> + return err;
return PTR_ERR()
> }
>
> return 0;
> -
> -error:
> - if (data->regulator)
> - regulator_disable(data->regulator);
> - return err;
> -}
^ permalink raw reply
* Re: [PATCH RFC 3/7] hwmon: (da9052) Use devm_regulator_get_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 14:20 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-3-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:52 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
A few comments inline, but nothing substantial.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
> 1 file changed, 7 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> index 2bd7ae8100d7..70e7bc72e980 100644
> --- a/drivers/hwmon/da9052-hwmon.c
> +++ b/drivers/hwmon/da9052-hwmon.c
> @@ -26,7 +26,6 @@ struct da9052_hwmon {
> struct mutex hwmon_lock;
> bool tsi_as_adc;
> int tsiref_mv;
> - struct regulator *tsiref;
> struct completion tsidone;
> };
>
> @@ -414,32 +413,19 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
> device_property_read_bool(pdev->dev.parent, "dlg,tsi-as-adc");
>
> if (hwmon->tsi_as_adc) {
> - hwmon->tsiref = devm_regulator_get(pdev->dev.parent, "tsiref");
> - if (IS_ERR(hwmon->tsiref)) {
> - err = PTR_ERR(hwmon->tsiref);
> - dev_err(&pdev->dev, "failed to get tsiref: %d", err);
> + err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
> + "tsiref");
> + if (err < 0)
> return err;
> - }
> -
> - err = regulator_enable(hwmon->tsiref);
> - if (err)
> - return err;
> -
> - hwmon->tsiref_mv = regulator_get_voltage(hwmon->tsiref);
> - if (hwmon->tsiref_mv < 0) {
> - err = hwmon->tsiref_mv;
> - goto exit_regulator;
> - }
>
> /* convert from microvolt (DT) to millivolt (hwmon) */
> - hwmon->tsiref_mv /= 1000;
> + hwmon->tsiref_mv = err / 1000;
>
Using a variable called err for a good value is a bit ugly but fair enough if that
is precedence in this driver.
> }
> @@ -483,10 +466,8 @@ static void da9052_hwmon_remove(struct platform_device *pdev)
> {
> struct da9052_hwmon *hwmon = platform_get_drvdata(pdev);
>
> - if (hwmon->tsi_as_adc) {
> + if (hwmon->tsi_as_adc)
> da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
Superficially looks like devm_da9052_request_irq could be added that
uses devm_request_threaded_irq() to allow dropping this remaining handling.
Thanks,
Jonathan
> - regulator_disable(hwmon->tsiref);
> - }
> }
>
> static struct platform_driver da9052_hwmon_driver = {
>
^ permalink raw reply
* Re: [PATCH RFC 7/7] Input: mpr121: Use devm_regulator_get_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 14:21 UTC (permalink / raw)
To: David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240327-regulator-get-enable-get-votlage-v1-7-5f4517faa059@baylibre.com>
On Wed, 27 Mar 2024 18:18:56 -0500
David Lechner <dlechner@baylibre.com> wrote:
> We can reduce boilerplate code by using
> devm_regulator_get_enable_get_voltage().
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
LGTM though you may want to bring an error message back if you drop the
prints from the regulator functions.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
^ permalink raw reply
* Re: [PATCH RFC 3/7] hwmon: (da9052) Use devm_regulator_get_enable_get_voltage()
From: Guenter Roeck @ 2024-03-28 15:20 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Support Opensource, Cosmin Tanislav, Lars-Peter Clausen,
Michael Hennerich, Antoniu Miclaus, Greg Kroah-Hartman,
Dmitry Torokhov, linux-doc, linux-kernel, linux-hwmon, linux-iio,
linux-staging, linux-input
In-Reply-To: <20240328142038.33ad68bc@jic23-huawei>
On 3/28/24 07:20, Jonathan Cameron wrote:
> On Wed, 27 Mar 2024 18:18:52 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
>> We can reduce boilerplate code by using
>> devm_regulator_get_enable_get_voltage().
>>
>> Signed-off-by: David Lechner <dlechner@baylibre.com>
>
> A few comments inline, but nothing substantial.
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>> ---
>> drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
>> 1 file changed, 7 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
>> index 2bd7ae8100d7..70e7bc72e980 100644
>> --- a/drivers/hwmon/da9052-hwmon.c
>> +++ b/drivers/hwmon/da9052-hwmon.c
>> @@ -26,7 +26,6 @@ struct da9052_hwmon {
>> struct mutex hwmon_lock;
>> bool tsi_as_adc;
>> int tsiref_mv;
>> - struct regulator *tsiref;
>> struct completion tsidone;
>> };
>>
>> @@ -414,32 +413,19 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
>> device_property_read_bool(pdev->dev.parent, "dlg,tsi-as-adc");
>>
>> if (hwmon->tsi_as_adc) {
>> - hwmon->tsiref = devm_regulator_get(pdev->dev.parent, "tsiref");
>> - if (IS_ERR(hwmon->tsiref)) {
>> - err = PTR_ERR(hwmon->tsiref);
>> - dev_err(&pdev->dev, "failed to get tsiref: %d", err);
>> + err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
>> + "tsiref");
>> + if (err < 0)
>> return err;
>> - }
>> -
>> - err = regulator_enable(hwmon->tsiref);
>> - if (err)
>> - return err;
>> -
>> - hwmon->tsiref_mv = regulator_get_voltage(hwmon->tsiref);
>> - if (hwmon->tsiref_mv < 0) {
>> - err = hwmon->tsiref_mv;
>> - goto exit_regulator;
>> - }
>>
>> /* convert from microvolt (DT) to millivolt (hwmon) */
>> - hwmon->tsiref_mv /= 1000;
>> + hwmon->tsiref_mv = err / 1000;
>>
>
> Using a variable called err for a good value is a bit ugly but fair enough if that
> is precedence in this driver.
>
It isn't. The existing code assigns the return value from regulator_get_voltage()
to hwmon->tsiref_mv and then evaluates it. I would not oppose introducing a variable
such as tsiref_uv, but not the misuse of 'err'. I am not going to accept the code
as suggested. It is bad style, and it would invite others to use it as precedent
when trying to introduce similar code.
>> }
>> @@ -483,10 +466,8 @@ static void da9052_hwmon_remove(struct platform_device *pdev)
>> {
>> struct da9052_hwmon *hwmon = platform_get_drvdata(pdev);
>>
>> - if (hwmon->tsi_as_adc) {
>> + if (hwmon->tsi_as_adc)
>> da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
> Superficially looks like devm_da9052_request_irq could be added that
> uses devm_request_threaded_irq() to allow dropping this remaining handling.
>
That should be a separate series of patches. A local solution might be
to use devm_add_action_or_reset(), but that should also be a separate patch.
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH RFC 3/7] hwmon: (da9052) Use devm_regulator_get_enable_get_voltage()
From: Jonathan Cameron @ 2024-03-28 15:53 UTC (permalink / raw)
To: Guenter Roeck
Cc: David Lechner, Jonathan Corbet, Liam Girdwood, Mark Brown,
Jean Delvare, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <13405bc9-3c97-4063-87b3-45a7e686fa50@roeck-us.net>
On Thu, 28 Mar 2024 08:20:00 -0700
Guenter Roeck <linux@roeck-us.net> wrote:
> On 3/28/24 07:20, Jonathan Cameron wrote:
> > On Wed, 27 Mar 2024 18:18:52 -0500
> > David Lechner <dlechner@baylibre.com> wrote:
> >
> >> We can reduce boilerplate code by using
> >> devm_regulator_get_enable_get_voltage().
> >>
> >> Signed-off-by: David Lechner <dlechner@baylibre.com>
> >
> > A few comments inline, but nothing substantial.
> >
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> >> ---
> >> drivers/hwmon/da9052-hwmon.c | 33 +++++++--------------------------
> >> 1 file changed, 7 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/da9052-hwmon.c b/drivers/hwmon/da9052-hwmon.c
> >> index 2bd7ae8100d7..70e7bc72e980 100644
> >> --- a/drivers/hwmon/da9052-hwmon.c
> >> +++ b/drivers/hwmon/da9052-hwmon.c
> >> @@ -26,7 +26,6 @@ struct da9052_hwmon {
> >> struct mutex hwmon_lock;
> >> bool tsi_as_adc;
> >> int tsiref_mv;
> >> - struct regulator *tsiref;
> >> struct completion tsidone;
> >> };
> >>
> >> @@ -414,32 +413,19 @@ static int da9052_hwmon_probe(struct platform_device *pdev)
> >> device_property_read_bool(pdev->dev.parent, "dlg,tsi-as-adc");
> >>
> >> if (hwmon->tsi_as_adc) {
> >> - hwmon->tsiref = devm_regulator_get(pdev->dev.parent, "tsiref");
> >> - if (IS_ERR(hwmon->tsiref)) {
> >> - err = PTR_ERR(hwmon->tsiref);
> >> - dev_err(&pdev->dev, "failed to get tsiref: %d", err);
> >> + err = devm_regulator_get_enable_get_voltage(pdev->dev.parent,
> >> + "tsiref");
> >> + if (err < 0)
> >> return err;
> >> - }
> >> -
> >> - err = regulator_enable(hwmon->tsiref);
> >> - if (err)
> >> - return err;
> >> -
> >> - hwmon->tsiref_mv = regulator_get_voltage(hwmon->tsiref);
> >> - if (hwmon->tsiref_mv < 0) {
> >> - err = hwmon->tsiref_mv;
> >> - goto exit_regulator;
> >> - }
> >>
> >> /* convert from microvolt (DT) to millivolt (hwmon) */
> >> - hwmon->tsiref_mv /= 1000;
> >> + hwmon->tsiref_mv = err / 1000;
> >>
> >
> > Using a variable called err for a good value is a bit ugly but fair enough if that
> > is precedence in this driver.
> >
>
> It isn't. The existing code assigns the return value from regulator_get_voltage()
> to hwmon->tsiref_mv and then evaluates it. I would not oppose introducing a variable
> such as tsiref_uv, but not the misuse of 'err'. I am not going to accept the code
> as suggested. It is bad style, and it would invite others to use it as precedent
> when trying to introduce similar code.
I was too lazy to look and see if there were existing cases :) Local variable indeed
the right way to go.
>
> >> }
> >> @@ -483,10 +466,8 @@ static void da9052_hwmon_remove(struct platform_device *pdev)
> >> {
> >> struct da9052_hwmon *hwmon = platform_get_drvdata(pdev);
> >>
> >> - if (hwmon->tsi_as_adc) {
> >> + if (hwmon->tsi_as_adc)
> >> da9052_free_irq(hwmon->da9052, DA9052_IRQ_TSIREADY, hwmon);
> > Superficially looks like devm_da9052_request_irq could be added that
> > uses devm_request_threaded_irq() to allow dropping this remaining handling.
> >
>
> That should be a separate series of patches. A local solution might be
> to use devm_add_action_or_reset(), but that should also be a separate patch.
Agreed. Just a passing comment whilst the code was in front of me.
Jonathan
>
> Thanks,
> Guenter
>
^ permalink raw reply
* Re: [PATCH RFC 1/7] regulator: devres: add APIs for reference supplies
From: David Lechner @ 2024-03-28 15:54 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Jonathan Corbet, Liam Girdwood, Mark Brown, Jean Delvare,
Guenter Roeck, Support Opensource, Cosmin Tanislav,
Lars-Peter Clausen, Michael Hennerich, Antoniu Miclaus,
Greg Kroah-Hartman, Dmitry Torokhov, linux-doc, linux-kernel,
linux-hwmon, linux-iio, linux-staging, linux-input
In-Reply-To: <20240328134744.2fc263b9@jic23-huawei>
On Thu, Mar 28, 2024 at 8:48 AM Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Wed, 27 Mar 2024 18:18:50 -0500
> David Lechner <dlechner@baylibre.com> wrote:
>
> > A common use case for regulators is to supply a reference voltage to an
> > analog input or output device. This adds two new devres APIs to get,
> > enable, and get the voltage in a single call. This allows eliminating
> > boilerplate code in drivers that use reference supplies in this way.
> >
> > devm_regulator_get_enable_get_voltage() is intended for cases where the
> Maybe avoid the double get?
> devm_regulator_get_enable_read_voltage() perhaps?
ok with me
>
> > supply is required to provide an external reference voltage.
> >
> > devm_regulator_get_optional_enable_get_voltage() is intended for cases
> > where the supply is optional and device typically uses an internal
> > reference voltage if the supply is not available.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
>
> In general I'll find this very useful (there are 50+ incidence
I didn't find quite that many. Still close to 40 though.
> of the pattern this can replace in IIO).
> I would keep it more similar to other devm regulator related functions
> and not do error printing internally though.
I did this because it seems like we could be losing potentially losing
useful information when something goes wrong making it harder to
troubleshoot which function actually failed. But looking into it more,
the regulator functions already print errors for many of the error
paths, so printing more errors does seem a bit redundant. So I will
remove the dev_error_probe() in v2.
^ permalink raw reply
* Re: [PATCH] input/touchscreen: synaptics_tcm_oncell: add driver
From: Dmitry Torokhov @ 2024-03-28 17:52 UTC (permalink / raw)
To: Frieder Hannenheim; +Cc: linux-input, linux-kernel
In-Reply-To: <20240327214643.7055-1-friederhannenheim@riseup.net>
Hi Frieder,
On Wed, Mar 27, 2024 at 10:39:12PM +0100, Frieder Hannenheim wrote:
> This is a bit of a stripped down and partially reworked driver for the
> synaptics_tcm_oncell touchscreen. I based my work off the driver in the
> LineageOS kernel found at
> https://github.com/LineageOS/android_kernel_oneplus_sm8250 branch
> lineage-20. The code was originally written by OnePlus developers but
> I'm not sure how to credit them correctly.
So the first question is: does this device not use Synaptics RMI4
protocol?
I am CCing Marge Yang of Synaptics who may shed some light on the kind
of touch controller this is.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input/touchscreen: synaptics_tcm_oncell: add driver
From: Dmitry Torokhov @ 2024-03-28 17:53 UTC (permalink / raw)
To: Frieder Hannenheim, Marge Yang; +Cc: linux-input, linux-kernel
In-Reply-To: <20240327214643.7055-1-friederhannenheim@riseup.net>
[ now CCing for real ]
Hi Frieder,
On Wed, Mar 27, 2024 at 10:39:12PM +0100, Frieder Hannenheim wrote:
> This is a bit of a stripped down and partially reworked driver for the
> synaptics_tcm_oncell touchscreen. I based my work off the driver in the
> LineageOS kernel found at
> https://github.com/LineageOS/android_kernel_oneplus_sm8250 branch
> lineage-20. The code was originally written by OnePlus developers but
> I'm not sure how to credit them correctly.
So the first question is: does this device not use Synaptics RMI4
protocol?
I am CCing Marge Yang of Synaptics who may shed some light on the kind
of touch controller this is.
Thanks.
--
Dmitry
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox