Re: [RFC DO NOT MERGE] treewide: use __xchg in most obvious places
On Tue, Jan 10, 2023 at 11:53:06AM +0100, Andrzej Hajda wrote:
> This patch tries to show usability of __xchg helper.
> It is not intended to be merged, but I can convert
> it to proper patchset if necessary.
>
> There are many more places where __xchg can be used.
> This demo shows the most spectacular cases IMHO:
> - previous value is returned from function,
> - temporary variables are in use.
>
> As a result readability is much better and diffstat is quite
> nice, less local vars to look at.
> In many cases whole body of functions is replaced
> with __xchg(ptr, val), so as further refactoring the whole
> function can be removed and __xchg can be called directly.
...
> arch_uretprobe_hijack_return_addr(unsigned long trampoline_vaddr,
> struct pt_regs *regs)
> {
> - unsigned long orig_ret_vaddr;
> -
> - orig_ret_vaddr = regs->ARM_lr;
> - /* Replace the return addr with trampoline addr */
> - regs->ARM_lr = trampoline_vaddr;
> - return orig_ret_vaddr;
> + return __xchg(>ARM_lr, trampoline_vaddr);
> }
If it's not a callback, the entire function can be killed.
And this is a good example of the function usage.
OTOH, these places might have a side effect (if it's in deep CPU
handlers), means we need to do this carefully.
...
> static inline void *qed_chain_produce(struct qed_chain *p_chain)
> {
> - void *p_ret = NULL, *p_prod_idx, *p_prod_page_idx;
> + void *p_prod_idx, *p_prod_page_idx;
>
> if (is_chain_u16(p_chain)) {
> if ((p_chain->u.chain16.prod_idx &
> @@ -390,11 +391,8 @@ static inline void *qed_chain_produce(struct qed_chain
> *p_chain)
> p_chain->u.chain32.prod_idx++;
> }
>
> - p_ret = p_chain->p_prod_elem;
> - p_chain->p_prod_elem = (void *)(((u8 *)p_chain->p_prod_elem) +
> - p_chain->elem_size);
> -
> - return p_ret;
> + return __xchg(_chain->p_prod_elem,
> + (void *)(((u8 *)p_chain->p_prod_elem) +
> p_chain->elem_size));
Wondering if you still need a (void *) casting after the change. Ditto for the
rest of similar cases.
> }
...
Btw, is it done by coccinelle? If no, why not providing the script?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 10/15] staging: fbtft: core: Introduce backlight_is_blank()
On Sat, Jan 07, 2023 at 07:26:24PM +0100, Sam Ravnborg via B4 Submission Endpoint wrote: > From: Sam Ravnborg > > Avoiding direct access to backlight_properties.props. > > Access to the deprecated props.fb_blank replaced by backlight_is_blank(). > Access to props.power is dropped - it was only used for debug. ... > + if (blank) > gpiod_set_value(par->gpio.led[0], !polarity); > + else > + gpiod_set_value(par->gpio.led[0], polarity); if (blank) polarity = !polarity; gpiod_set_value(par->gpio.led[0], polarity); ? -- With Best Regards, Andy Shevchenko
Re: [PATCH 09/15] staging: fbtft: fb_ssd1351.c: Introduce backlight_is_blank()
On Sat, Jan 07, 2023 at 07:26:23PM +0100, Sam Ravnborg via B4 Submission Endpoint wrote: > From: Sam Ravnborg > > Avoiding direct access to backlight_properties.props. > > Access to the deprecated props.fb_blank replaced by backlight_is_blank(). > Access to props.power is dropped - it was only used for debug. > Signed-off-by: Sam Ravnborg > Cc: Stephen Kitt > Cc: Greg Kroah-Hartman > Cc: Daniel Thompson > Cc: Andy Shevchenko > Cc: linux-fb...@vger.kernel.org Not sure why you have this (at least) explicitly mentioned as get_maintainer.pl can generate it and git send-email can utilize the script output... ... > - write_reg(par, 0xB5, on ? 0x03 : 0x02); > + write_reg(par, 0xB5, !blank ? 0x03 : 0x02); Why not positive conditional? write_reg(par, 0xB5, blank ? 0x02 : 0x03); -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 3/9] drm/display: Add Type-C switch helpers
On Thu, Jan 05, 2023 at 09:24:51PM +0800, Pin-yen Lin wrote:
> Add helpers to register and unregister Type-C "switches" for bridges
> capable of switching their output between two downstream devices.
>
> The helper registers USB Type-C mode switches when the "mode-switch"
> and the "data-lanes" properties are available in Device Tree.
...
> + port_data->typec_mux = typec_mux_register(dev, _desc);
> + if (IS_ERR(port_data->typec_mux)) {
> + ret = PTR_ERR(port_data->typec_mux);
> + dev_err(dev, "Mode switch register for port %d failed: %d\n",
> + port_num, ret);
> + }
> +
> + return ret;
...
> + struct device_node *sw;
> + int ret = 0;
It's easy to break things if you squeeze more code in the future in this
function, so I recommend to split assignment to be closer to its first user
(see below).
> + for_each_child_of_node(port, sw) {
> + if (of_property_read_bool(sw, "mode-switch"))
> + switch_desc->num_typec_switches++;
> + }
> +
> + if (!switch_desc->num_typec_switches) {
> + dev_warn(dev, "No Type-C switches node found\n");
> + return ret;
return 0;
> + }
> +
> + switch_desc->typec_ports = devm_kcalloc(
> + dev, switch_desc->num_typec_switches,
> + sizeof(struct drm_dp_typec_port_data), GFP_KERNEL);
> +
> + if (!switch_desc->typec_ports)
> + return -ENOMEM;
> + /* Register switches for each connector. */
> + for_each_child_of_node(port, sw) {
> + if (!of_property_read_bool(sw, "mode-switch"))
> + continue;
> + ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data,
> mux_set);
> + if (ret) {
> + dev_err(dev, "Failed to register mode switch: %d\n",
> ret);
> + of_node_put(sw);
> + break;
> + }
> + }
> + if (ret)
> + drm_dp_unregister_typec_switches(switch_desc);
> +
> + return ret;
Why not adding a goto label?
ret = drm_dp_register_mode_switch(dev, sw, switch_desc, data,
mux_set);
if (ret)
goto err_unregister_typec_switches;
return 0;
err_unregister_typec_switches:
of_node_put(sw);
drm_dp_unregister_typec_switches(switch_desc);
dev_err(dev, "Failed to register mode switch: %d\n", ret);
return ret;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v7 2/9] platform/chrome: cros_ec_typec: Purge blocking switch devlinks
On Thu, Jan 05, 2023 at 09:24:50PM +0800, Pin-yen Lin wrote: > From: Prashant Malani > > When using OF graph, the fw_devlink code will create links between the > individual port driver (cros-ec-typec here) and the parent device for > a Type-C switch (like mode-switch). Since the mode-switch will in turn > have the usb-c-connector (i.e the child of the port driver) as a > supplier, fw_devlink will not be able to resolve the cyclic dependency > correctly. > > As a result, the mode-switch driver probe() never runs, so mode-switches > are never registered. Because of that, the port driver probe constantly > fails with -EPROBE_DEFER, because the Type-C connector class requires all > switch devices to be registered prior to port registration. > > To break this deadlock and allow the mode-switch registration to occur, > purge all the usb-c-connector nodes' absent suppliers. This eliminates > the connector as a supplier for a switch and allows it to be probed. > Signed-off-by: Prashant Malani > > Signed-off-by: Pin-yen Lin Tag block mustn't have the blank line(s). ... > + /* > + * OF graph may have set up some device links with switches, since > + * connectors have their own compatible. Purge these to avoid a deadlock > + * in switch probe (the switch mistakenly assumes the connector is a > + * supplier). > + */ Perhaps even /* * OF graph may have set up some device links with switches, * since connectors have their own compatible. Purge these * to avoid a deadlock in switch probe (the switch mistakenly * assumes the connector is a supplier). */ ? > + if (dev->of_node) I would use if (dev_of_node(dev)), but it's up to you and maintainers. > + device_for_each_child_node(dev, fwnode) > + fw_devlink_purge_absent_suppliers(fwnode); -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 0/3] Add generic framebuffer support to EFI earlycon driver
On Fri, Dec 23, 2022 at 03:42:33PM +0100, Ard Biesheuvel wrote: > (cc Andy) I believe there are two reasons I'm Cc'ed now: - the Cc was forgotten. because I remember reviewing some parts of this contribution - this conflicts (to some extent) with my patch that speeds up the scrolling For the first it's obvious what to do, I think Markuss can include me in his v4. For the second I don't see the functional clash. The scrolling in this series is not anyhow optimized. I think my patch should go first as - it is less intrusive - it has been tested, or can be tested easily Tell me if I'm missing something here. > On Wed, 21 Dec 2022 at 11:54, Markuss Broks wrote: > > > > Make the EFI earlycon driver be suitable for any linear framebuffers. > > This should be helpful for early porting of boards with no other means of > > output, like smartphones/tablets. There seems to be an issue with > > early_ioremap > > function on ARM32, but I am unable to find the exact cause. It appears the > > mappings > > returned by it are somehow incorrect, thus the driver is disabled on ARM. > > The reason that this driver is disabled on ARM is because the struct > screen_info is not populated early enough, as it is retrieved from a > UEFI configuration table. > > early_ioremap() works fine on ARM as long as they mapping is torn down > before paging_init() > > > EFI early > > console was disabled on IA64 previously because of missing > > early_memremap_prot, > > and this is inherited to this driver. > > > > This patch also changes > > "This patch also changes ..." is usually a strong hint to self that > the patches need to be split up. > > > behavior on EFI systems, by selecting the mapping type > > based on if the framebuffer region intersects with system RAM. If it does, > > it's > > common sense that it should be in RAM as a whole, and so the system RAM > > mapping is > > used. It was tested to be working on my PC (Intel Z490 platform), as well > > as several > > ARM64 boards (Samsung Galaxy S9 (Exynos), iPad Air 2, Xiaomi Mi Pad 4, ...). -- With Best Regards, Andy Shevchenko
Re: [PATCH 18/19] linux/include: add non-atomic version of xchg
On Thu, Dec 22, 2022 at 12:46:34PM +0100, Andrzej Hajda wrote:
> The pattern of setting variable with new value and returning old
> one is very common in kernel. Usually atomicity of the operation
> is not required, so xchg seems to be suboptimal and confusing in
> such cases.
FWIW,
Reviewed-by: Andy Shevchenko
> Signed-off-by: Andrzej Hajda
> ---
> include/linux/non-atomic/xchg.h | 19 +++
> 1 file changed, 19 insertions(+)
> create mode 100644 include/linux/non-atomic/xchg.h
>
> diff --git a/include/linux/non-atomic/xchg.h b/include/linux/non-atomic/xchg.h
> new file mode 100644
> index 00..f7fa5dd746f37d
> --- /dev/null
> +++ b/include/linux/non-atomic/xchg.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_NON_ATOMIC_XCHG_H
> +#define _LINUX_NON_ATOMIC_XCHG_H
> +
> +/**
> + * __xchg - set variable pointed by @ptr to @val, return old value
> + * @ptr: pointer to affected variable
> + * @val: value to be written
> + *
> + * This is non-atomic variant of xchg.
> + */
> +#define __xchg(ptr, val) ({ \
> + __auto_type __ptr = ptr;\
> + __auto_type __t = *__ptr; \
> + *__ptr = (val); \
> + __t; \
> +})
> +
> +#endif
> --
> 2.34.1
>
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 05:53:52PM +0200, Andy Shevchenko wrote: > On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > > wrote: > > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > > Please CC me on future revisions. > > > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > > There's no need to rename anything. So nack on this patch 1/5. > > > > > > It is not obvious (for casual developers like me) that p in prandom > > > stands for predictable. Some renaming would be useful IMHO. > > > > Renaming makes backports more complicated, because stable teams will > > have to 'undo' name changes. > > Stable teams are already overwhelmed by the amount of backports, and > > silly merge conflicts. > > > > Take another example : > > > > u64 timecounter_read(struct timecounter *tc) > > > > You would think this function would read the timecounter, right ? > > > > Well, it _updates_ many fields from @tc, so a 'better name' would also > > be useful. > > Right, at some point we become into the world of > > #define true 0 > > because... (read below) > > > linux kernel is not for casual readers. > > P.S. I believe you applied a common sense and in some cases > the renames are necessary. And before you become to a wrong conclusion by reading between the lines, no, I'm not taking either side (to rename or not to rename) in this case. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Wed, Dec 14, 2022 at 04:15:49PM +0100, Eric Dumazet wrote: > On Wed, Dec 14, 2022 at 1:34 PM Stanislaw Gruszka > wrote: > > On Mon, Dec 12, 2022 at 03:35:20PM +0100, Jason A. Donenfeld wrote: > > > Please CC me on future revisions. > > > > > > As of 6.2, the prandom namespace is *only* for predictable randomness. > > > There's no need to rename anything. So nack on this patch 1/5. > > > > It is not obvious (for casual developers like me) that p in prandom > > stands for predictable. Some renaming would be useful IMHO. > > Renaming makes backports more complicated, because stable teams will > have to 'undo' name changes. > Stable teams are already overwhelmed by the amount of backports, and > silly merge conflicts. > > Take another example : > > u64 timecounter_read(struct timecounter *tc) > > You would think this function would read the timecounter, right ? > > Well, it _updates_ many fields from @tc, so a 'better name' would also > be useful. Right, at some point we become into the world of #define true 0 because... (read below) > linux kernel is not for casual readers. P.S. I believe you applied a common sense and in some cases the renames are necessary. -- With Best Regards, Andy Shevchenko
Re: [Intel-gfx] [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg
On Tue, Dec 13, 2022 at 11:09:12AM +0100, Andrzej Hajda wrote: > On 09.12.2022 19:56, Andy Shevchenko wrote: > > On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote: ... > > > I hope there will be place for such tiny helper in kernel. > > > Quick cocci analyze shows there is probably few thousands places > > > where it could be used, of course I do not intend to do it :). > > > > > > I was not sure where to put this macro, I hope near swap definition > > > is the most suitable place. > > > > Ah, swap() in this context is not the same. minmax.h hosts it because > > it's often related to the swap function in the sort-type algorithms. > >> Moreover sorry if to/cc is not correct - get_maintainers.pl was > > > more confused than me, to who address this patch. ... > > > include/linux/minmax.h | 14 ++ > > > > Does it really suit this header? I would expect something else. > > Maybe include/linux/non-atomic/xchg.h, dunno. > > non-atomic seems quite strange for me, I would assume everything not in > atomic is non-atomic, unless explicitly specified. > > > > > Btw, have you looked if Ingo's gigantic series have done anything to > > cmpxchg.h > > and related headers? Maybe some ideas can be taken from there? > > Grepping it didn't give any clue. > > Looking at 'near' languages just to get an idea (they name the function > differently): > > C++ [1]: exchange and swap are in utility header > Rust[2]: replace and swap are in std::mem module > > This is some argument to put them together. Again, I left the above part on top of this message, the swap() in Linux kernel is not related to __xchg() or similar. That said, minmax.h is not a good place for the latter. > [1]: https://en.cppreference.com/w/cpp/header/utility > [2]: https://doc.rust-lang.org/std/mem/index.html -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] Renaming weak prng invocations - prandom_bytes_state, prandom_u32_state
On Mon, Dec 12, 2022 at 12:16:04AM +0200, david.keisars...@mail.huji.ac.il
wrote:
> From: David
>
> Since the two functions
> prandom_byte_state and prandom_u32_state
> use the weak prng prandom_u32,
> we added the prefix predictable_rng,
> to their signatures so it is clear they are weak.
It's fancy indentation.
...
> /* Fisher-Yates shuffle */
> for (i = count - 1; i > 0; i--) {
> - rand = prandom_u32_state(_state);
> + rand =
> predictable_rng_prandom_u32_state(_state);
Isn't it too many "random":s encoded in the name?
I would leave either "rng" or "[p]random".
> rand %= (i + 1);
> swap_free_obj(slab, i, rand);
> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 00/11] pwm: Allow .get_state to fail
On Sat, Dec 10, 2022 at 11:41:54PM +0100, Uwe Kleine-König wrote: > On Sat, Dec 10, 2022 at 10:57:16PM +0200, Andy Shevchenko wrote: > > On Sat, Dec 10, 2022 at 10:18:33AM +0100, Uwe Kleine-König wrote: > > > On Fri, Dec 09, 2022 at 11:47:54PM +0200, Andy Shevchenko wrote: > > > > On Wed, Nov 30, 2022 at 04:21:37PM +0100, Uwe Kleine-König wrote: ... > > > > I'm wondering why we didn't see a compiler warning about mistyped > > > > function > > > > prototypes in some drivers. > > > > > > I don't understand where you expected a warning. Care to elaborate? > > > > intel-lpss.c has the prototype that returns an int. IIRC it was like this > > Do you mean drivers/mfd/intel-lpss.c? That one doesn't implement a PWM?! Nope, I meant pwm-lpss.c. > And drivers/pwm/pwm-lpss.c is adapted by this series. That's what I didn't see how. > One of us is confused ... Yes, because when I have checked the branch based on Linux Next I already saw that get_state() returns a code and I wasn't aware that the series is already there. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 00/11] pwm: Allow .get_state to fail
On Sat, Dec 10, 2022 at 10:18:33AM +0100, Uwe Kleine-König wrote: > On Fri, Dec 09, 2022 at 11:47:54PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 30, 2022 at 04:21:37PM +0100, Uwe Kleine-König wrote: ... > > I'm wondering why we didn't see a compiler warning about mistyped function > > prototypes in some drivers. > > I don't understand where you expected a warning. Care to elaborate? intel-lpss.c has the prototype that returns an int. IIRC it was like this before your patches. Now the above wondering passage... -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 00/11] pwm: Allow .get_state to fail
On Wed, Nov 30, 2022 at 04:21:37PM +0100, Uwe Kleine-König wrote: > Hello, > > I forgot about this series and was remembered when I talked to Conor > Dooley about how .get_state() should behave in an error case. > > Compared to (implicit) v1, sent with Message-Id: > 20220916151506.298488-1-u.kleine-koe...@pengutronix.de > I changed: > > - Patch #1 which does the prototype change now just adds "return 0" to >all implementations and so gets simpler and doesn't change behaviour. >The adaptions to the different .get_state() implementations are split >out into individual patches to ease review. > - One minor inconsistency fixed in "pwm: Handle .get_state() failures" >that I noticed while looking into this patch. > - I skipped changing sun4i.c as I don't know how to handle the error >there. Someone might want to have a look. (That's not ideal, but it's >not worse than the same issue before this series.) > > In v1 Thierry had the concern: > > | That raises the question about what to do in these cases. If we return > | an error, that could potentially throw off consumers. So perhaps the > | closest would be to return a disabled PWM? Or perhaps it'd be up to the > | consumer to provide some fallback configuration for invalidly configured > | or unconfigured PWMs. > > .get_state() is only called in pwm_device_request on a pwm_state that a > consumer might see. Before my series a consumer might have seen a > partial modified pwm_state (because .get_state() might have modified > .period, then stumbled and returned silently). The last patch ensures > that this partial modification isn't given out to the consumer. Instead > they now see the same as if .get_state wasn't implemented at all. I'm wondering why we didn't see a compiler warning about mistyped function prototypes in some drivers. P.S. The series is good thing to do, thank you. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg
On Fri, Dec 09, 2022 at 08:56:28PM +0200, Andy Shevchenko wrote: > On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote: ... > > I hope there will be place for such tiny helper in kernel. > > Quick cocci analyze shows there is probably few thousands places > > where it could be used, of course I do not intend to do it :). > > > > I was not sure where to put this macro, I hope near swap definition > > is the most suitable place. > > Ah, swap() in this context is not the same. minmax.h hosts it because > it's often related to the swap function in the sort-type algorithms. > > > Moreover sorry if to/cc is not correct - get_maintainers.pl was > > more confused than me, to who address this patch. > > ... > > > include/linux/minmax.h | 14 ++ > > Does it really suit this header? I would expect something else. > Maybe include/linux/non-atomic/xchg.h, dunno. It may become a candidate to host io-64 non-atomic versions and other non-atomic generic headers... > Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h > and related headers? Maybe some ideas can be taken from there? -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg
On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote: > The pattern of setting variable with new value and returning old > one is very common in kernel. Usually atomicity of the operation > is not required, so xchg seems to be suboptimal and confusing in > such cases. Since name xchg is already in use and __xchg is used > in architecture code, proposition is to name the macro exchange. > > Signed-off-by: Andrzej Hajda > --- > Hi, > > I hope there will be place for such tiny helper in kernel. > Quick cocci analyze shows there is probably few thousands places > where it could be used, of course I do not intend to do it :). > > I was not sure where to put this macro, I hope near swap definition > is the most suitable place. Ah, swap() in this context is not the same. minmax.h hosts it because it's often related to the swap function in the sort-type algorithms. > Moreover sorry if to/cc is not correct - get_maintainers.pl was > more confused than me, to who address this patch. ... > include/linux/minmax.h | 14 ++ Does it really suit this header? I would expect something else. Maybe include/linux/non-atomic/xchg.h, dunno. Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h and related headers? Maybe some ideas can be taken from there? -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 1/4] i915: Move list_count() to list.h as list_count_nodes() for broader use
On Thu, Dec 08, 2022 at 02:14:54PM +0100, Greg Kroah-Hartman wrote: > On Thu, Dec 08, 2022 at 02:54:45PM +0200, Andy Shevchenko wrote: > > On Wed, Nov 30, 2022 at 03:48:35PM +0200, Andy Shevchenko wrote: > > > Some of the existing users, and definitely will be new ones, want to > > > count existing nodes in the list. Provide a generic API for that by > > > moving code from i915 to list.h. > > > > Greg, I believe this one is ready to be taken. Or please tell me what I need > > to do. > > Wait for me to get through the current backlog of patches that I have in > my review queue. Odds are, it will have to wait until after 6.2-rc1 is > out based on when 6.1 is going to be released. It's fine, no hurry and take your time! > Don't worry, it's not lost. Thank you, got it! -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 1/4] i915: Move list_count() to list.h as list_count_nodes() for broader use
On Wed, Nov 30, 2022 at 03:48:35PM +0200, Andy Shevchenko wrote: > Some of the existing users, and definitely will be new ones, want to > count existing nodes in the list. Provide a generic API for that by > moving code from i915 to list.h. Greg, I believe this one is ready to be taken. Or please tell me what I need to do. -- With Best Regards, Andy Shevchenko
[PATCH v5 3/4] usb: gadget: udc: bcm63xx: Convert to use list_count_nodes()
The list API provides the list_count_nodes() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v5: used renamed API (LKP)
v4: no change
v3: no change
v2: no change
drivers/usb/gadget/udc/bcm63xx_udc.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/bcm63xx_udc.c
b/drivers/usb/gadget/udc/bcm63xx_udc.c
index 2cdb07905bde..771ba1ffce95 100644
--- a/drivers/usb/gadget/udc/bcm63xx_udc.c
+++ b/drivers/usb/gadget/udc/bcm63xx_udc.c
@@ -2172,7 +2172,6 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
for (ch_idx = 0; ch_idx < BCM63XX_NUM_IUDMA; ch_idx++) {
struct iudma_ch *iudma = >iudma[ch_idx];
- struct list_head *pos;
seq_printf(s, "IUDMA channel %d -- ", ch_idx);
switch (iudma_defaults[ch_idx].ep_type) {
@@ -2205,14 +2204,10 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
seq_printf(s, " desc: %d/%d used", iudma->n_bds_used,
iudma->n_bds);
- if (iudma->bep) {
- i = 0;
- list_for_each(pos, >bep->queue)
- i++;
- seq_printf(s, "; %d queued\n", i);
- } else {
+ if (iudma->bep)
+ seq_printf(s, "; %zu queued\n",
list_count_nodes(>bep->queue));
+ else
seq_printf(s, "\n");
- }
for (i = 0; i < iudma->n_bds; i++) {
struct bcm_enet_desc *d = >bd_ring[i];
--
2.35.1
[PATCH v5 2/4] usb: gadget: hid: Convert to use list_count_nodes()
The list API provides the list_count_nodes() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v5: used renamed API (LKP)
v4: no change
v3: fixed typo in the commit message (Fabio)
v2: no change
drivers/usb/gadget/legacy/hid.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/legacy/hid.c b/drivers/usb/gadget/legacy/hid.c
index 1187ee4f316a..133daf88162e 100644
--- a/drivers/usb/gadget/legacy/hid.c
+++ b/drivers/usb/gadget/legacy/hid.c
@@ -133,14 +133,11 @@ static struct usb_configuration config_driver = {
static int hid_bind(struct usb_composite_dev *cdev)
{
struct usb_gadget *gadget = cdev->gadget;
- struct list_head *tmp;
struct hidg_func_node *n = NULL, *m, *iter_n;
struct f_hid_opts *hid_opts;
- int status, funcs = 0;
-
- list_for_each(tmp, _func_list)
- funcs++;
+ int status, funcs;
+ funcs = list_count_nodes(_func_list);
if (!funcs)
return -ENODEV;
--
2.35.1
[PATCH v5 1/4] i915: Move list_count() to list.h as list_count_nodes() for broader use
Some of the existing users, and definitely will be new ones, want to
count existing nodes in the list. Provide a generic API for that by
moving code from i915 to list.h.
Reviewed-by: Lucas De Marchi
Acked-by: Jani Nikula
Signed-off-by: Andy Shevchenko
---
v5: added tag (Lucas), renamed API to list_count_nodes() (LKP)
v4: fixed prototype when converting to static inline
v3: added tag (Jani), changed to be static inline (Mike)
v2: dropped the duplicate code in i915 (LKP)
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 15 ++-
include/linux/list.h | 15 +++
2 files changed, 17 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 1f7188129cd1..370164363b0d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -2004,17 +2004,6 @@ static void print_request_ring(struct drm_printer *m,
struct i915_request *rq)
}
}
-static unsigned long list_count(struct list_head *list)
-{
- struct list_head *pos;
- unsigned long count = 0;
-
- list_for_each(pos, list)
- count++;
-
- return count;
-}
-
static unsigned long read_ul(void *p, size_t x)
{
return *(unsigned long *)(p + x);
@@ -2189,8 +2178,8 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_lock_irqsave(>sched_engine->lock, flags);
engine_dump_active_requests(engine, m);
- drm_printf(m, "\tOn hold?: %lu\n",
- list_count(>sched_engine->hold));
+ drm_printf(m, "\tOn hold?: %zu\n",
+ list_count_nodes(>sched_engine->hold));
spin_unlock_irqrestore(>sched_engine->lock, flags);
drm_printf(m, "\tMMIO base: 0x%08x\n", engine->mmio_base);
diff --git a/include/linux/list.h b/include/linux/list.h
index 61762054b4be..f10344dbad4d 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -655,6 +655,21 @@ static inline void list_splice_tail_init(struct list_head
*list,
!list_is_head(pos, (head)); \
pos = n, n = pos->prev)
+/**
+ * list_count_nodes - count nodes in the list
+ * @head: the head for your list.
+ */
+static inline size_t list_count_nodes(struct list_head *head)
+{
+ struct list_head *pos;
+ size_t count = 0;
+
+ list_for_each(pos, head)
+ count++;
+
+ return count;
+}
+
/**
* list_entry_is_head - test if the entry points to the head of the list
* @pos: the type * to cursor
--
2.35.1
[PATCH v5 4/4] xhci: Convert to use list_count_nodes()
The list API provides the list_count_nodes() to help with counting
existing nodes in the list. Utilise it.
Acked-by: Mathias Nyman
Signed-off-by: Andy Shevchenko
---
v5: used renamed API (LKP)
v4: added tag (Mathias)
v3: no change
v2: no change
drivers/usb/host/xhci-ring.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ddc30037f9ce..aa4d34efecd2 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2528,7 +2528,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
union xhci_trb *ep_trb;
int status = -EINPROGRESS;
struct xhci_ep_ctx *ep_ctx;
- struct list_head *tmp;
u32 trb_comp_code;
int td_num = 0;
bool handling_skipped_tds = false;
@@ -2582,10 +2581,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
}
/* Count current td numbers if ep->skip is set */
- if (ep->skip) {
- list_for_each(tmp, _ring->td_list)
- td_num++;
- }
+ if (ep->skip)
+ td_num += list_count_nodes(_ring->td_list);
/* Look for common error cases */
switch (trb_comp_code) {
--
2.35.1
Re: [PATCH v6 2/7] platform/chrome: cros_ec_typec: Purge blocking switch devlinks
On Thu, Nov 24, 2022 at 06:20:51PM +0800, Pin-yen Lin wrote: > From: Prashant Malani > > When using OF graph, the fw_devlink code will create links between the > individual port driver (cros-ec-typec here) and the parent device for > a Type-C switch (like mode-switch). Since the mode-switch will in turn > have the usb-c-connector (i.e the child of the port driver) as a > supplier, fw_devlink will not be able to resolve the cyclic dependency > correctly. > > As a result, the mode-switch driver probe() never runs, so mode-switches > are never registered. Because of that, the port driver probe constantly > fails with -EPROBE_DEFER, because the Type-C connector class requires all > switch devices to be registered prior to port registration. > > To break this deadlock and allow the mode-switch registration to occur, > purge all the usb-c-connector nodes' absent suppliers. This eliminates > the connector as a supplier for a switch and allows it to be probed. ... > + /* > + * OF graph may have set up some device links with switches, since > connectors have their > + * own compatible. Purge these to avoid a deadlock in switch probe (the > switch mistakenly > + * assumes the connector is a supplier). > + */ A bit too long lines... > + if (dev->of_node) Why do you need this check? > + device_for_each_child_node(dev, fwnode) > + fw_devlink_purge_absent_suppliers(fwnode); -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 7/7] drm/bridge: it6505: Register Type C mode switches
On Thu, Nov 24, 2022 at 06:20:56PM +0800, Pin-yen Lin wrote:
> Register USB Type-C mode switches when the "mode-switch" property and
> relevant port are available in Device Tree. Configure the "lane_swap"
> state based on the entered alternate mode for a specific Type-C
> connector, which ends up updating the lane swap registers of the it6505
> chip.
...
> config DRM_ITE_IT6505
> tristate "ITE IT6505 DisplayPort bridge"
> depends on OF
> + depends on TYPEC || TYPEC=n
> select DRM_DISPLAY_DP_HELPER
> select DRM_DISPLAY_HDCP_HELPER
> select DRM_DISPLAY_HELPER
Something went wrong with the indentation. Perhaps you need to fix it first.
...
> #include
> #include
> #include
> +#include
Make it ordered?
...
> +struct it6505_port_data {
> + bool dp_connected;
Perhaps make it last?
> + struct typec_mux_dev *typec_mux;
> + struct it6505 *it6505;
> +};
...
> +static void it6505_typec_ports_update(struct it6505 *it6505)
> +{
> + usleep_range(3000, 4000);
> +
> + if (it6505->typec_ports[0].dp_connected &&
> it6505->typec_ports[1].dp_connected)
> + /* Both ports available, do nothing to retain the current one.
> */
> + return;
> + else if (it6505->typec_ports[0].dp_connected)
> + it6505->lane_swap = false;
> + else if (it6505->typec_ports[1].dp_connected)
> + it6505->lane_swap = true;
> +
> + usleep_range(3000, 4000);
> +}
As per previous patch comments.
Also, comment out these long sleeps. Why they are needed.
...
> + int ret = pm_runtime_get_sync(dev);
> +
> + /*
> + * On system resume, mux_set can be triggered before
> + * pm_runtime_force_resume re-enables runtime power management.
We refer to the functions as func().
> + * Handling the error here to make sure the bridge is powered
> on.
> + */
> + if (ret < 0)
> + it6505_poweron(it6505);
This seems needed a bit more of explanation, esp. why you leave PM runtime
reference count bumped up.
...
> + num_lanes = drm_of_get_data_lanes_count(node, 0, 2);
> + if (num_lanes <= 0) {
> + dev_err(dev, "Error on getting data lanes count: %d\n",
> + num_lanes);
> + return num_lanes;
> + }
> +
> + ret = of_property_read_u32_array(node, "data-lanes", dp_lanes,
> num_lanes);
> + if (ret) {
> + dev_err(dev, "Failed to read the data-lanes variable: %d\n",
> + ret);
> + return ret;
> + }
> +
> + for (i = 0; i < num_lanes; i++) {
> + if (port_num != -1 && port_num != dp_lanes[i] / 2) {
> + dev_err(dev, "Invalid data lane numbers\n");
> + return -EINVAL;
> + }
As per previous patch comments.
> + port_num = dp_lanes[i] / 2;
> + }
The above seems like tons of duplicating code that drivers need to implement.
Can we shrink that burden by adding some library functions?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 5/7] drm/bridge: anx7625: Register Type C mode switches
On Thu, Nov 24, 2022 at 06:20:54PM +0800, Pin-yen Lin wrote:
> Register USB Type-C mode switches when the "mode-switch" property and
> relevant port are available in Device Tree. Configure the crosspoint
> switch based on the entered alternate mode for a specific Type-C
> connector.
...
> +static void anx7625_typec_two_ports_update(struct anx7625_data *ctx)
> +{
> + if (ctx->typec_ports[0].dp_connected &&
> ctx->typec_ports[1].dp_connected)
> + /* Both ports available, do nothing to retain the current one.
> */
> + return;
> + else if (ctx->typec_ports[0].dp_connected)
This 'else' is redundant. I would rewrite above as
/* Check if both ports available and do nothing to retain the current
one */
if (ctx->typec_ports[0].dp_connected &&
ctx->typec_ports[1].dp_connected)
return;
if (ctx->typec_ports[0].dp_connected)
> + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_NORMAL);
> + else if (ctx->typec_ports[1].dp_connected)
> + anx7625_set_crosspoint_switch(ctx, TYPEC_ORIENTATION_REVERSE);
> +}
...
> + data->dp_connected = (state->alt && state->alt->svid ==
> USB_TYPEC_DP_SID &&
> + state->alt->mode == USB_TYPEC_DP_MODE);
Parentheses are not needed.
...
> + /*
> + * <0 1> refers to SSRX1/SSTX1, and <2 3> refers to SSRX2/SSTX2.
> + */
> + for (i = 0; i < num_lanes; i++) {
> + if (port_num != -1 && port_num != dp_lanes[i] / 2) {
> + dev_err(dev, "Invalid data lane numbers\n");
> + return -EINVAL;
> + }
According to Rob Linux must not validate device tree. If you need it, use
proper YAML schema.
> + port_num = dp_lanes[i] / 2;
> + }
...
> + if (!ctx->num_typec_switches) {
> + dev_warn(dev, "No Type-C switches node found\n");
> + return ret;
Why not to return 0 explicitly?
> + }
...
> + ctx->typec_ports = devm_kcalloc(
Broken indentation.
> + dev, ctx->num_typec_switches, sizeof(struct anx7625_port_data),
> + GFP_KERNEL);
> + if (!ctx->typec_ports)
> + return -ENOMEM;
...
> +struct anx7625_port_data {
> + bool dp_connected;
You can save some bytes on some architectures if move this to be last field.
> + struct typec_mux_dev *typec_mux;
> + struct anx7625_data *ctx;
> +};
--
With Best Regards,
Andy Shevchenko
[PATCH v4 2/4] usb: gadget: hid: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v4: no change
v3: fixed typo in the commit message (Fabio)
v2: no change
drivers/usb/gadget/legacy/hid.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/legacy/hid.c b/drivers/usb/gadget/legacy/hid.c
index 1187ee4f316a..6196c3456e0b 100644
--- a/drivers/usb/gadget/legacy/hid.c
+++ b/drivers/usb/gadget/legacy/hid.c
@@ -133,14 +133,11 @@ static struct usb_configuration config_driver = {
static int hid_bind(struct usb_composite_dev *cdev)
{
struct usb_gadget *gadget = cdev->gadget;
- struct list_head *tmp;
struct hidg_func_node *n = NULL, *m, *iter_n;
struct f_hid_opts *hid_opts;
- int status, funcs = 0;
-
- list_for_each(tmp, _func_list)
- funcs++;
+ int status, funcs;
+ funcs = list_count(_func_list);
if (!funcs)
return -ENODEV;
--
2.35.1
[PATCH v4 3/4] usb: gadget: udc: bcm63xx: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v4: no change
v3: no change
v2: no change
drivers/usb/gadget/udc/bcm63xx_udc.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/bcm63xx_udc.c
b/drivers/usb/gadget/udc/bcm63xx_udc.c
index 2cdb07905bde..0762e49e85f8 100644
--- a/drivers/usb/gadget/udc/bcm63xx_udc.c
+++ b/drivers/usb/gadget/udc/bcm63xx_udc.c
@@ -2172,7 +2172,6 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
for (ch_idx = 0; ch_idx < BCM63XX_NUM_IUDMA; ch_idx++) {
struct iudma_ch *iudma = >iudma[ch_idx];
- struct list_head *pos;
seq_printf(s, "IUDMA channel %d -- ", ch_idx);
switch (iudma_defaults[ch_idx].ep_type) {
@@ -2205,14 +2204,10 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
seq_printf(s, " desc: %d/%d used", iudma->n_bds_used,
iudma->n_bds);
- if (iudma->bep) {
- i = 0;
- list_for_each(pos, >bep->queue)
- i++;
- seq_printf(s, "; %d queued\n", i);
- } else {
+ if (iudma->bep)
+ seq_printf(s, "; %zu queued\n",
list_count(>bep->queue));
+ else
seq_printf(s, "\n");
- }
for (i = 0; i < iudma->n_bds; i++) {
struct bcm_enet_desc *d = >bd_ring[i];
--
2.35.1
[PATCH v4 1/4] i915: Move list_count() to list.h for broader use
Some of the existing users, and definitely will be new ones, want to
count existing nodes in the list. Provide a generic API for that by
moving code from i915 to list.h.
Signed-off-by: Andy Shevchenko
Acked-by: Jani Nikula
---
v4: fixed prototype when converting to static inline
v3: added tag (Jani), changed to be static inline (Mike)
v2: dropped the duplicate code in i915 (LKP)
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 +
include/linux/list.h | 15 +++
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index c33e0d72d670..b96c8217743c 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -2094,17 +2094,6 @@ static void print_request_ring(struct drm_printer *m,
struct i915_request *rq)
}
}
-static unsigned long list_count(struct list_head *list)
-{
- struct list_head *pos;
- unsigned long count = 0;
-
- list_for_each(pos, list)
- count++;
-
- return count;
-}
-
static unsigned long read_ul(void *p, size_t x)
{
return *(unsigned long *)(p + x);
@@ -2279,7 +2268,7 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_lock_irqsave(>sched_engine->lock, flags);
engine_dump_active_requests(engine, m);
- drm_printf(m, "\tOn hold?: %lu\n",
+ drm_printf(m, "\tOn hold?: %zu\n",
list_count(>sched_engine->hold));
spin_unlock_irqrestore(>sched_engine->lock, flags);
diff --git a/include/linux/list.h b/include/linux/list.h
index 61762054b4be..632a298c7018 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -655,6 +655,21 @@ static inline void list_splice_tail_init(struct list_head
*list,
!list_is_head(pos, (head)); \
pos = n, n = pos->prev)
+/**
+ * list_count - count nodes in the list
+ * @head: the head for your list.
+ */
+static inline size_t list_count(struct list_head *head)
+{
+ struct list_head *pos;
+ size_t count = 0;
+
+ list_for_each(pos, head)
+ count++;
+
+ return count;
+}
+
/**
* list_entry_is_head - test if the entry points to the head of the list
* @pos: the type * to cursor
--
2.35.1
[PATCH v4 4/4] xhci: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
Acked-by: Mathias Nyman
---
v4: added tag (Mathias)
v3: no change
v2: no change
drivers/usb/host/xhci-ring.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ad81e9a508b1..817c31e3b0c8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2532,7 +2532,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
union xhci_trb *ep_trb;
int status = -EINPROGRESS;
struct xhci_ep_ctx *ep_ctx;
- struct list_head *tmp;
u32 trb_comp_code;
int td_num = 0;
bool handling_skipped_tds = false;
@@ -2580,10 +2579,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
}
/* Count current td numbers if ep->skip is set */
- if (ep->skip) {
- list_for_each(tmp, _ring->td_list)
- td_num++;
- }
+ if (ep->skip)
+ td_num += list_count(_ring->td_list);
/* Look for common error cases */
switch (trb_comp_code) {
--
2.35.1
Re: [PATCH v3 1/4] i915: Move list_count() to list.h for broader use
On Tue, Nov 22, 2022 at 05:35:13PM +0200, Andy Shevchenko wrote: > Some of the existing users, and definitely will be new ones, want to > count existing nodes in the list. Provide a generic API for that by > moving code from i915 to list.h. Sorry for the noise, forgot to squash one part, so it get's not compilable. -- With Best Regards, Andy Shevchenko
[PATCH v3 4/4] xhci: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v3: no change
v2: no change
drivers/usb/host/xhci-ring.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ad81e9a508b1..817c31e3b0c8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2532,7 +2532,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
union xhci_trb *ep_trb;
int status = -EINPROGRESS;
struct xhci_ep_ctx *ep_ctx;
- struct list_head *tmp;
u32 trb_comp_code;
int td_num = 0;
bool handling_skipped_tds = false;
@@ -2580,10 +2579,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
}
/* Count current td numbers if ep->skip is set */
- if (ep->skip) {
- list_for_each(tmp, _ring->td_list)
- td_num++;
- }
+ if (ep->skip)
+ td_num += list_count(_ring->td_list);
/* Look for common error cases */
switch (trb_comp_code) {
--
2.35.1
[PATCH v3 2/4] usb: gadget: hid: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v3: fixed typo in the commit message (Fabio)
v2: no change
drivers/usb/gadget/legacy/hid.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/legacy/hid.c b/drivers/usb/gadget/legacy/hid.c
index 1187ee4f316a..6196c3456e0b 100644
--- a/drivers/usb/gadget/legacy/hid.c
+++ b/drivers/usb/gadget/legacy/hid.c
@@ -133,14 +133,11 @@ static struct usb_configuration config_driver = {
static int hid_bind(struct usb_composite_dev *cdev)
{
struct usb_gadget *gadget = cdev->gadget;
- struct list_head *tmp;
struct hidg_func_node *n = NULL, *m, *iter_n;
struct f_hid_opts *hid_opts;
- int status, funcs = 0;
-
- list_for_each(tmp, _func_list)
- funcs++;
+ int status, funcs;
+ funcs = list_count(_func_list);
if (!funcs)
return -ENODEV;
--
2.35.1
[PATCH v3 3/4] usb: gadget: udc: bcm63xx: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Utilise it.
Signed-off-by: Andy Shevchenko
---
v3: no change
v2: no change
drivers/usb/gadget/udc/bcm63xx_udc.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/bcm63xx_udc.c
b/drivers/usb/gadget/udc/bcm63xx_udc.c
index 2cdb07905bde..0762e49e85f8 100644
--- a/drivers/usb/gadget/udc/bcm63xx_udc.c
+++ b/drivers/usb/gadget/udc/bcm63xx_udc.c
@@ -2172,7 +2172,6 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
for (ch_idx = 0; ch_idx < BCM63XX_NUM_IUDMA; ch_idx++) {
struct iudma_ch *iudma = >iudma[ch_idx];
- struct list_head *pos;
seq_printf(s, "IUDMA channel %d -- ", ch_idx);
switch (iudma_defaults[ch_idx].ep_type) {
@@ -2205,14 +2204,10 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
seq_printf(s, " desc: %d/%d used", iudma->n_bds_used,
iudma->n_bds);
- if (iudma->bep) {
- i = 0;
- list_for_each(pos, >bep->queue)
- i++;
- seq_printf(s, "; %d queued\n", i);
- } else {
+ if (iudma->bep)
+ seq_printf(s, "; %zu queued\n",
list_count(>bep->queue));
+ else
seq_printf(s, "\n");
- }
for (i = 0; i < iudma->n_bds; i++) {
struct bcm_enet_desc *d = >bd_ring[i];
--
2.35.1
[PATCH v3 1/4] i915: Move list_count() to list.h for broader use
Some of the existing users, and definitely will be new ones, want to
count existing nodes in the list. Provide a generic API for that by
moving code from i915 to list.h.
Signed-off-by: Andy Shevchenko
Acked-by: Jani Nikula
---
v3: added tag (Jani), changed to be static inline (Mike)
v2: dropped the duplicate code in i915 (LKP)
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 +
include/linux/list.h | 15 +++
2 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 6ae8b07cfaa1..b5d474be564d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -2085,17 +2085,6 @@ static void print_request_ring(struct drm_printer *m,
struct i915_request *rq)
}
}
-static unsigned long list_count(struct list_head *list)
-{
- struct list_head *pos;
- unsigned long count = 0;
-
- list_for_each(pos, list)
- count++;
-
- return count;
-}
-
static unsigned long read_ul(void *p, size_t x)
{
return *(unsigned long *)(p + x);
@@ -2270,7 +2259,7 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_lock_irqsave(>sched_engine->lock, flags);
engine_dump_active_requests(engine, m);
- drm_printf(m, "\tOn hold?: %lu\n",
+ drm_printf(m, "\tOn hold?: %zu\n",
list_count(>sched_engine->hold));
spin_unlock_irqrestore(>sched_engine->lock, flags);
diff --git a/include/linux/list.h b/include/linux/list.h
index 61762054b4be..65aab596ae33 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -655,6 +655,21 @@ static inline void list_splice_tail_init(struct list_head
*list,
!list_is_head(pos, (head)); \
pos = n, n = pos->prev)
+/**
+ * list_count - count nodes in the list
+ * @head: the head for your list.
+ */
+size_t list_count(struct list_head *head)
+{
+ struct list_head *pos;
+ size_t count = 0;
+
+ list_for_each(pos, head)
+ count++;
+
+ return count;
+}
+
/**
* list_entry_is_head - test if the entry points to the head of the list
* @pos: the type * to cursor
--
2.35.1
Re: [PATCH v2 1/4] i915: Move list_count() to list.h for broader use
On Tue, Nov 15, 2022 at 05:46:28PM +0200, Jani Nikula wrote: > On Mon, 14 Nov 2022, Andy Shevchenko > wrote: > > Some of the existing users, and definitely will be new ones, want to > > count existing nodes in the list. Provide a generic API for that by > > moving code from i915 to list.h. > > I think I'd find list_length() a much more natural name for this. i915 suggests my variant :-) > *shrug* > > Acked-by: Jani Nikula > > regardless of what you decide to do with name or static inline etc. Thanks! I will check which one looks and feels better and update for v3. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 2/4] usb: gadget: hid: Convert to use list_count()
On Mon, Nov 14, 2022 at 04:15:35PM -0300, Fabio Estevam wrote: > On Mon, Nov 14, 2022 at 1:22 PM Andy Shevchenko > wrote: > > > > The list API now provides the list_count() to help with counting > > existing nodes in the list. Uilise it. > > s/Uilise/Utilise Fixed for v3, thanks! -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 1/4] i915: Move list_count() to list.h for broader use
On Mon, Nov 14, 2022 at 06:11:51PM +, Ruhl, Michael J wrote: ... > So all of the non-list_for_each code appears to be an inline. This is not true. > This which, resembles the non-list_for_each pattern is a macro? > > Just curious as to why the macro rather than inline? See above. However, I'm fine with the inline. -- With Best Regards, Andy Shevchenko
[PATCH v2 3/4] usb: gadget: udc: bcm63xx: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Uilise it.
Signed-off-by: Andy Shevchenko
---
v2: no change
drivers/usb/gadget/udc/bcm63xx_udc.c | 11 +++
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/drivers/usb/gadget/udc/bcm63xx_udc.c
b/drivers/usb/gadget/udc/bcm63xx_udc.c
index 2cdb07905bde..0762e49e85f8 100644
--- a/drivers/usb/gadget/udc/bcm63xx_udc.c
+++ b/drivers/usb/gadget/udc/bcm63xx_udc.c
@@ -2172,7 +2172,6 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
for (ch_idx = 0; ch_idx < BCM63XX_NUM_IUDMA; ch_idx++) {
struct iudma_ch *iudma = >iudma[ch_idx];
- struct list_head *pos;
seq_printf(s, "IUDMA channel %d -- ", ch_idx);
switch (iudma_defaults[ch_idx].ep_type) {
@@ -2205,14 +2204,10 @@ static int bcm63xx_iudma_dbg_show(struct seq_file *s,
void *p)
seq_printf(s, " desc: %d/%d used", iudma->n_bds_used,
iudma->n_bds);
- if (iudma->bep) {
- i = 0;
- list_for_each(pos, >bep->queue)
- i++;
- seq_printf(s, "; %d queued\n", i);
- } else {
+ if (iudma->bep)
+ seq_printf(s, "; %zu queued\n",
list_count(>bep->queue));
+ else
seq_printf(s, "\n");
- }
for (i = 0; i < iudma->n_bds; i++) {
struct bcm_enet_desc *d = >bd_ring[i];
--
2.35.1
[PATCH v2 4/4] xhci: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Uilise it.
Signed-off-by: Andy Shevchenko
---
v2: no change
drivers/usb/host/xhci-ring.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index ad81e9a508b1..817c31e3b0c8 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2532,7 +2532,6 @@ static int handle_tx_event(struct xhci_hcd *xhci,
union xhci_trb *ep_trb;
int status = -EINPROGRESS;
struct xhci_ep_ctx *ep_ctx;
- struct list_head *tmp;
u32 trb_comp_code;
int td_num = 0;
bool handling_skipped_tds = false;
@@ -2580,10 +2579,8 @@ static int handle_tx_event(struct xhci_hcd *xhci,
}
/* Count current td numbers if ep->skip is set */
- if (ep->skip) {
- list_for_each(tmp, _ring->td_list)
- td_num++;
- }
+ if (ep->skip)
+ td_num += list_count(_ring->td_list);
/* Look for common error cases */
switch (trb_comp_code) {
--
2.35.1
[PATCH v2 1/4] i915: Move list_count() to list.h for broader use
Some of the existing users, and definitely will be new ones, want to
count existing nodes in the list. Provide a generic API for that by
moving code from i915 to list.h.
Signed-off-by: Andy Shevchenko
---
v2: dropped the duplicate code in i915 (LKP)
drivers/gpu/drm/i915/gt/intel_engine_cs.c | 13 +
include/linux/list.h | 13 +
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 6ae8b07cfaa1..b5d474be564d 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -2085,17 +2085,6 @@ static void print_request_ring(struct drm_printer *m,
struct i915_request *rq)
}
}
-static unsigned long list_count(struct list_head *list)
-{
- struct list_head *pos;
- unsigned long count = 0;
-
- list_for_each(pos, list)
- count++;
-
- return count;
-}
-
static unsigned long read_ul(void *p, size_t x)
{
return *(unsigned long *)(p + x);
@@ -2270,7 +2259,7 @@ void intel_engine_dump(struct intel_engine_cs *engine,
spin_lock_irqsave(>sched_engine->lock, flags);
engine_dump_active_requests(engine, m);
- drm_printf(m, "\tOn hold?: %lu\n",
+ drm_printf(m, "\tOn hold?: %zu\n",
list_count(>sched_engine->hold));
spin_unlock_irqrestore(>sched_engine->lock, flags);
diff --git a/include/linux/list.h b/include/linux/list.h
index 61762054b4be..098eccf8c1b6 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -655,6 +655,19 @@ static inline void list_splice_tail_init(struct list_head
*list,
!list_is_head(pos, (head)); \
pos = n, n = pos->prev)
+/**
+ * list_count - count nodes in the list
+ * @head: the head for your list.
+ */
+#define list_count(head) \
+({ \
+ struct list_head *__tmp;\
+ size_t __i = 0; \
+ list_for_each(__tmp, head) \
+ __i++; \
+ __i;\
+})
+
/**
* list_entry_is_head - test if the entry points to the head of the list
* @pos: the type * to cursor
--
2.35.1
[PATCH v2 2/4] usb: gadget: hid: Convert to use list_count()
The list API now provides the list_count() to help with counting
existing nodes in the list. Uilise it.
Signed-off-by: Andy Shevchenko
---
v2: no change
drivers/usb/gadget/legacy/hid.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/gadget/legacy/hid.c b/drivers/usb/gadget/legacy/hid.c
index 1187ee4f316a..6196c3456e0b 100644
--- a/drivers/usb/gadget/legacy/hid.c
+++ b/drivers/usb/gadget/legacy/hid.c
@@ -133,14 +133,11 @@ static struct usb_configuration config_driver = {
static int hid_bind(struct usb_composite_dev *cdev)
{
struct usb_gadget *gadget = cdev->gadget;
- struct list_head *tmp;
struct hidg_func_node *n = NULL, *m, *iter_n;
struct f_hid_opts *hid_opts;
- int status, funcs = 0;
-
- list_for_each(tmp, _func_list)
- funcs++;
+ int status, funcs;
+ funcs = list_count(_func_list);
if (!funcs)
return -ENODEV;
--
2.35.1
[PATCH v1 2/2] fbdev: ssd1307fb: Drop duplicate NULL checks for PWM APIs
pwm_disable() and pwm_put() are NULL-aware, no need to
duplicate the check in the caller.
Signed-off-by: Andy Shevchenko
---
drivers/video/fbdev/ssd1307fb.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 5c891aa00d59..046b9990d27c 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -803,10 +803,8 @@ static int ssd1307fb_probe(struct i2c_client *client)
bl_init_error:
unregister_framebuffer(info);
panel_init_error:
- if (par->device_info->need_pwm) {
- pwm_disable(par->pwm);
- pwm_put(par->pwm);
- }
+ pwm_disable(par->pwm);
+ pwm_put(par->pwm);
regulator_enable_error:
if (par->vbat_reg)
regulator_disable(par->vbat_reg);
@@ -827,10 +825,8 @@ static void ssd1307fb_remove(struct i2c_client *client)
backlight_device_unregister(info->bl_dev);
unregister_framebuffer(info);
- if (par->device_info->need_pwm) {
- pwm_disable(par->pwm);
- pwm_put(par->pwm);
- }
+ pwm_disable(par->pwm);
+ pwm_put(par->pwm);
if (par->vbat_reg)
regulator_disable(par->vbat_reg);
fb_deferred_io_cleanup(info);
--
2.35.1
[PATCH v1 1/2] fbdev: ssd1307fb: Drop optional dependency
Only a single out of three devices need a PWM, so from driver it's
optional. Moreover it's a single driver in the entire kernel that
currently selects PWM. Unfortunately this selection is a root cause
of the circular dependencies when we want to enable optional PWM
for some other drivers that select GPIOLIB.
Fixes: a2ed00da5047 ("drivers/video: add support for the Solomon SSD1307 OLED
Controller")
Signed-off-by: Andy Shevchenko
---
drivers/video/fbdev/Kconfig | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index a98987aa2784..b88d8bfe992e 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -2218,7 +2218,6 @@ config FB_SSD1307
select FB_SYS_COPYAREA
select FB_SYS_IMAGEBLIT
select FB_DEFERRED_IO
- select PWM
select FB_BACKLIGHT
help
This driver implements support for the Solomon SSD1307
--
2.35.1
Re: [PATCH v5 00/10] Driver of Intel(R) Gaussian & Neural Accelerator
On Thu, Oct 20, 2022 at 8:57 PM Maciej Kwapulinski wrote: > > Dear kernel maintainers, > > This submission is a kernel driver to support Intel(R) Gaussian & Neural > Accelerator (Intel(R) GNA). Intel(R) GNA is a PCI-based neural co-processor > available on multiple Intel platforms. AI developers and users can offload > continuous inference workloads to an Intel(R) GNA device in order to free > processor resources and save power. Noise reduction and speech recognition > are the examples of the workloads Intel(R) GNA deals with while its usage > is not limited to the two. > > For a list of processors equipped with Intel(R) GNA device, please refer to > this link: > https://docs.openvinotoolkit.org/latest/openvino_docs_IE_DG_supported_plugins_GNA.html > > We think contributing this driver to the upstream kernel project is the > best way for developers and users to get the latest Intel(R) GNA support in > a Linux kernel, through the mainline to any Linux distributions installed > on their systems. Upstreaming also enables contribution from developers > around the world to the driver once it is merged. Can you replace all those dev_dbg():s by trace events/points? -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 06/10] gna: add GNA_GEM_NEW and GNA_GEM_FREE ioctls
On Thu, Oct 20, 2022 at 8:57 PM Maciej Kwapulinski
wrote:
>
> drm_gem_shmem_object is base for memory objects provided by the patch
Here and in some other commit messages don't forget English
punctuation, like trailing period.
...
> +struct gna_gem_object {
> + struct drm_gem_shmem_object base;
> +
> + uint32_t handle;
Not u32 or __u32? Is it a subsystem requirement to have uint32_t? Or
your driver, then why are you not using similar types elsewhere?
> +};
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 04/10] gna: initialize MMU
On Thu, Oct 20, 2022 at 10:00 PM Andy Shevchenko wrote: > On Thu, Oct 20, 2022 at 8:57 PM Maciej Kwapulinski > wrote: ... > > + desc_size = round_up(gna_priv->info.desc_info.desc_size, PAGE_SIZE); > > PFN_UP() ? Or PFN_ALIGN() ? -- With Best Regards, Andy Shevchenko
Re: [PATCH v5 04/10] gna: initialize MMU
On Thu, Oct 20, 2022 at 8:57 PM Maciej Kwapulinski
wrote:
>
> From: Tomasz Jankowski
>
> Setup MMU in the driver with a new memory component.
...
> +#define GNA_FEATURES \
> + .max_hw_mem = 256 * 1024 * 1024,\
SZ_256M ?
> + .num_pagetables = 64, \
> + .num_page_entries = PAGE_SIZE / sizeof(u32),\
> + /* desc_info all in bytes */\
> + .desc_info = { \
> + .rsvd_size = 256, \
> + .cfg_size = 256,\
> + .desc_size = 784, \
> + .mmu_info = { \
> + .vamax_size = 4,\
> + .rsvd_size = 12,\
> + .pd_size = 4 * 64, \
> + }, \
> + }
Broken indentation?
...
> +#define GNA_DEV_HWID_CNL 0x5A11
> +#define GNA_DEV_HWID_EHL 0x4511
> +#define GNA_DEV_HWID_GLK 0x3190
> +#define GNA_DEV_HWID_ICL 0x8A11
> +#define GNA_DEV_HWID_JSL 0x4E11
> +#define GNA_DEV_HWID_TGL 0x9A11
> +#define GNA_DEV_HWID_RKL 0x4C11
> +#define GNA_DEV_HWID_ADL 0x464F
> +#define GNA_DEV_HWID_RPL 0xA74F
> +#define GNA_DEV_HWID_MTL 0x7E4C
Keep them sorted?
...
> + for (i = 0; i < mmu->num_pagetables; i++) {
> + pagetable_dma = mmu->pagetables_dma[i];
> + pgdirn[i] = pagetable_dma >> PAGE_SHIFT;
PFN_DOWN()
> + }
...
> + desc_size = round_up(gna_priv->info.desc_info.desc_size, PAGE_SIZE);
PFN_UP() ?
...
> + mmu->pagetables = drmm_kmalloc_array(_priv->drm,
> mmu->num_pagetables, sizeof(*mmu->pagetables), GFP_KERNEL);
> +
Redundant blank line.
> + if (!mmu->pagetables)
> + return -ENOMEM;
...
> +static const struct gna_dev_info cnl_dev_info = {
> + .hwid = GNA_DEV_HWID_CNL,
> + GNA_GEN1_FEATURES
Leave a comma at the end. Same for all similar declarations.
> +};
...
> +#define INTEL_GNA_DEVICE(hwid, info) \
> + { PCI_VDEVICE(INTEL, hwid), (kernel_ulong_t)(info) }
Drop this and use PCI_DEVICE_DATA() instead.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 02/10] gna: add GNA DRM device
On Thu, Oct 20, 2022 at 8:57 PM Maciej Kwapulinski
wrote:
Missed commit message.
> Signed-off-by: Maciej Kwapulinski
> Tested-by: Mikolaj Grzybowski
...
> + if (!(sizeof(dma_addr_t) > 4) ||
Do you really need this?
> + dma_set_mask(parent, DMA_BIT_MASK(64))) {
> + err = dma_set_mask(parent, DMA_BIT_MASK(32));
> + if (err)
> + return err;
IIRC if the 64-bit dma_set_mask() fails, there is no need to check
32-bit, i.e. it will fail.
> + }
...
> +#define DRIVER_DATE"20211201"
Really?
...
> +#include
Are you sure?
> +struct gna_dev_info {
> + u32 hwid;
> + u32 num_pagetables;
> + u32 num_page_entries;
> + u32 max_layer_count;
> + u64 max_hw_mem;
> +};
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v5 01/10] gna: add PCI driver module
On Thu, Oct 20, 2022 at 8:57 PM Maciej Kwapulinski wrote: > > Add a new PCI driver for Intel(R) Gaussian & Neural Accelerator > with basic support like module loading and unloading. The full > function of the driver will be added by further changes. ... > +GNA_GEM_NEW acquires new 4KB page aligned memory region ready for DMA > operations. a new ... > +GNA Library can allocate any number of memory regions for GNA usage. Its > number and total > +capacity are limited by the OSs’ resources. Due to GNA MMU restrictions, > even when using OSes' ? > +multiple memory regions, the sum of all the memory regions used within a > single inference > +request must be no larger than 256MB. ... > +++ b/drivers/gpu/drm/Kconfig > @@ -403,6 +403,8 @@ source "drivers/gpu/drm/solomon/Kconfig" > > source "drivers/gpu/drm/sprd/Kconfig" > > +source "drivers/gpu/drm/gna/Kconfig" It looks to me that you broke the order here. ... > @@ -147,3 +147,4 @@ obj-y += gud/ > obj-$(CONFIG_DRM_HYPERV) += hyperv/ > obj-y += solomon/ > obj-$(CONFIG_DRM_SPRD) += sprd/ > +obj-$(CONFIG_DRM_GNA) += gna/ Ditto. ... > +config DRM_GNA > + tristate "Intel(R) Gaussian & Neural Accelerator (Intel(R) GNA)" > + depends on X86 && PCI No compile test for the rest of possible options? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] drm: Remove drm_mode_config::fb_base
On Tue, Oct 18, 2022 at 08:26:17PM +, Zack Rusin wrote: > > On Oct 18, 2022, at 4:20 PM, Andy Shevchenko > > wrote: > > On Tue, Oct 18, 2022 at 12:11:51PM -0400, Zack Rusin wrote: > >> From: Zack Rusin > >> > >> v2: Thomas and Laurent noticed that in radeon_fb.c I forgot to set the > >> info->apertures->ranges[0].base and Laurent noticed a neat little cleanup > >> in the hisilicon driver as a result of the drm_mode_config::fb_base > >> removal. > > > > You need to address LKP comment. > > That came after v3, and I fixed it in the meantime. I will wait with sending > v4 until tomorrow to make sure I give everyone time to look at in case > there’s something else. Hmm... Am I missing v3? I answer to v2 and LKP complaint against v1. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] drm: Remove drm_mode_config::fb_base
On Tue, Oct 18, 2022 at 12:11:51PM -0400, Zack Rusin wrote: > From: Zack Rusin > > v2: Thomas and Laurent noticed that in radeon_fb.c I forgot to set the > info->apertures->ranges[0].base and Laurent noticed a neat little cleanup > in the hisilicon driver as a result of the drm_mode_config::fb_base > removal. You need to address LKP comment. > The fb_base in struct drm_mode_config has been unused for a long time. > Some drivers set it and some don't leading to a very confusing state > where the variable can't be relied upon, because there's no indication > as to which driver sets it and which doesn't. > > The only usage of fb_base is internal to two drivers so instead of trying > to force it into all the drivers to get it into a coherent state > completely remove it. ... > --- a/drivers/gpu/drm/ast/ast_mode.c > +++ b/drivers/gpu/drm/ast/ast_mode.c > @@ -1778,7 +1778,6 @@ int ast_mode_config_init(struct ast_private *ast) > dev->mode_config.min_width = 0; > dev->mode_config.min_height = 0; > dev->mode_config.preferred_depth = 24; > - dev->mode_config.fb_base = pci_resource_start(pdev, 0); Unused pdev. > if (ast->chip == AST2100 || > ast->chip == AST2200 || I suggest to compile with `make W=1 C=1` on your side before sending v3 and address all compiler complaints. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 2/6] treewide: use prandom_u32_max() when possible
On Fri, Oct 07, 2022 at 08:50:43PM -0700, Kees Cook wrote:
> On October 7, 2022 7:21:28 PM PDT, "Jason A. Donenfeld"
> wrote:
> >On Fri, Oct 07, 2022 at 03:47:44PM -0700, Kees Cook wrote:
> >> On Fri, Oct 07, 2022 at 12:01:03PM -0600, Jason A. Donenfeld wrote:
...
> >> These are more fun, but Coccinelle can still do them with a little
> >> Pythonic help:
> >>
> >> // Find a potential literal
> >> @literal_mask@
> >> expression LITERAL;
> >> identifier randfunc =~ "get_random_int|prandom_u32|get_random_u32";
> >> position p;
> >> @@
> >>
> >> (randfunc()@p & (LITERAL))
> >>
> >> // Add one to the literal.
> >> @script:python add_one@
> >> literal << literal_mask.LITERAL;
> >> RESULT;
> >> @@
> >>
> >> if literal.startswith('0x'):
> >> value = int(literal, 16) + 1
> >> coccinelle.RESULT = cocci.make_expr("0x%x" % (value))
> >> elif literal[0] in '123456789':
> >> value = int(literal, 10) + 1
> >> coccinelle.RESULT = cocci.make_expr("%d" % (value))
> >> else:
> >> print("I don't know how to handle: %s" % (literal))
Wouldn't Python take care about (known) prefixes itself?
try:
x = int(literal)
except ValueError as ex:
print(..., ex.error)
> >> // Replace the literal mask with the calculated result.
> >> @plus_one@
> >> expression literal_mask.LITERAL;
> >> position literal_mask.p;
> >> expression add_one.RESULT;
> >> identifier FUNC;
> >> @@
> >>
> >> - (FUNC()@p & (LITERAL))
> >> + prandom_u32_max(RESULT)
> >
> >Oh that's pretty cool. I can do the saturation check in python, since
> >`value` holds the parsed result. Neat.
>
> It is (at least how I have it here) just the string, so YMMV.
...
> >Thanks a bunch for the guidance.
>
> Sure thing! I was pleased to figure out how to do the python bit.
I believe it can be optimized
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/5] treewide: use prandom_u32_max() when possible
On Thu, Oct 06, 2022 at 09:55:19AM -0300, Jason Gunthorpe wrote: > On Thu, Oct 06, 2022 at 06:45:25AM -0600, Jason A. Donenfeld wrote: > > On Wed, Oct 05, 2022 at 09:16:50PM -0700, Kees Cook wrote: > > > On Wed, Oct 05, 2022 at 11:48:40PM +0200, Jason A. Donenfeld wrote: > > > > Rather than incurring a division or requesting too many random bytes for > > > > the given range, use the prandom_u32_max() function, which only takes > > > > the minimum required bytes from the RNG and avoids divisions. > > > > > > Yes please! > > > > > > Since this is a treewide patch, it's helpful for (me at least) doing > > > reviews to detail the mechanism of the transformation. > > > > This is hand done. There were also various wrong seds done. And then I'd > > edit the .diff manually, and then reapply it, as an iterative process. > > No internet on the airplane, and oddly no spatch already on my laptop (I > > think I had some Gentoo ocaml issues at some point and removed it?). > > > > > e.g. I imagine this could be done with something like Coccinelle and > > > > Feel free to check the work here by using Coccinelle if you're into > > that. > > Generally these series are a lot easier to review if it is structured > as a patches doing all the unusual stuff that had to be by hand > followed by an unmodified Coccinelle/sed/etc handling the simple > stuff. > > Especially stuff that is reworking the logic beyond simple > substitution should be one patch per subsystem not rolled into a giant > one patch conversion. > > This makes the whole workflow better because the hand-done stuff can > have a chance to flow through subsystem trees. +1 to all arguments for the splitting. I looked a bit into the code I have the interest to, but I won't spam people with not-so-important questions / comments / tags, etc. -- With Best Regards, Andy Shevchenko
Re: [f2fs-dev] [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 06:33:15AM -0600, Jason A. Donenfeld wrote:
> On Thu, Oct 06, 2022 at 10:43:31AM +0200, Jan Kara wrote:
...
> > The code here is effectively doing the
> >
> > parent_group = prandom_u32_max(ngroups);
> >
> > Similarly here we can use prandom_u32_max(ngroups) like:
> >
> > if (qstr) {
> > ...
> > parent_group = hinfo.hash % ngroups;
> > } else
> > parent_group = prandom_u32_max(ngroups);
>
> Nice catch. I'll move these to patch #1.
I believe coccinelle is able to handle this kind of code as well, so Kees'
proposal to use it seems more plausible since it's less error prone and more
flexible / powerful.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 3/5] treewide: use get_random_u32() when possible
On Thu, Oct 06, 2022 at 07:05:48AM -0600, Jason A. Donenfeld wrote: > On Thu, Oct 6, 2022 at 6:47 AM Jason Gunthorpe wrote: > > On Wed, Oct 05, 2022 at 11:48:42PM +0200, Jason A. Donenfeld wrote: ... > > > - u32 isn = (prandom_u32() & ~7UL) - 1; > > > + u32 isn = (get_random_u32() & ~7UL) - 1; > > > > Maybe this wants to be written as > > > > (prandom_max(U32_MAX >> 7) << 7) | 7 > > ? > > Holy smokes. Yea I guess maybe? It doesn't exactly gain anything or > make the code clearer though, and is a little bit more magical than > I'd like on a first pass. Shouldn't the two first 7s to be 3s? ... > > > - psn = prandom_u32() & 0xff; > > > + psn = get_random_u32() & 0xff; > > > > prandom_max(0xff + 1) > > That'd work, but again it's not more clear. Authors here are going for > a 24-bit number, and masking seems like a clear way to express that. We have some 24-bit APIs (and 48-bit) already in kernel, why not to have get_random_u24() ? -- With Best Regards, Andy Shevchenko
Re: [PATCH v9 09/10] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Wed, Sep 21, 2022 at 4:48 AM ChiaEn Wu wrote: > On Sun, Sep 18, 2022 at 3:22 AM Han Jingoo wrote: > > On Mon, Aug 29, 2022 ChiaEn Wu wrote: > > > +#define MT6370_ITORCH_MIN_uA 25000 > > > +#define MT6370_ITORCH_STEP_uA 12500 > > > +#define MT6370_ITORCH_MAX_uA 40 > > > +#define MT6370_ITORCH_DOUBLE_MAX_uA80 > > > +#define MT6370_ISTRB_MIN_uA5 > > > +#define MT6370_ISTRB_STEP_uA 12500 > > > +#define MT6370_ISTRB_MAX_uA150 > > > +#define MT6370_ISTRB_DOUBLE_MAX_uA 300 > > > > Use upper letters as below: For microseconds (and other -seconds) the common practice (I assume historically) is to use upper letters, indeed. But for current it's more natural to use small letters for unit multiplier as it's easier to read and understand. > > #define MT6370_ITORCH_MIN_UA 25000 > > #define MT6370_ITORCH_STEP_UA 12500 > > #define MT6370_ITORCH_MAX_UA 40 > > #define MT6370_ITORCH_DOUBLE_MAX_UA80 > > #define MT6370_ISTRB_MIN_UA5 > > #define MT6370_ISTRB_STEP_UA 12500 > > #define MT6370_ISTRB_MAX_UA150 > > #define MT6370_ISTRB_DOUBLE_MAX_UA 300 > > > > > +#define MT6370_STRBTO_MIN_US 64000 > > > +#define MT6370_STRBTO_STEP_US 32000 > > > +#define MT6370_STRBTO_MAX_US 2432000 > > Hi Jingoo, > > This coding style is in accordance with Andy's opinion in this mail: > https://lore.kernel.org/linux-arm-kernel/CAHp75Vciq4M4kVrabNV9vTLLcd1vR=bme8jledaf9mkrtpc...@mail.gmail.com/ True. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 05, 2022 at 03:07:48PM -0700, Guenter Roeck wrote: > On 9/5/22 12:55, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov > > wrote: > > > On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote: > > > > On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov > > > > wrote: > > > > > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: > > > > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > > > > > wrote: ... > > > > > > > + gpiod = devm_gpiod_get(>dev, > > > > > > > "nvidia,phy-reset", > > > > > > > + GPIOD_OUT_HIGH); > > > > > > > err = PTR_ERR_OR_ZERO(gpiod); > > > > > > > > > > > > What does _OR_ZERO mean now? > > > > > > > > > > This converts a pointer to an error code if a pointer represents > > > > > ERR_PTR() encoded error, or 0 to indicate success. > > > > > > > > Yes, I know that. My point is, how is it useful now (or even before)? > > > > I mean that devm_gpio_get() never returns NULL, right? > > > > > > What does returning NULL have to do with anything. > > > > It has to do with a dead code. If defm_gpiod_get() does not return > > NULL, then why do we even bother to check? > > PTR_ERR_OR_ZERO() converts into an error code (if the pointer is an > ERR_PTR) or 0 if it is a real pointer. Its purpose is not to convert > NULL into 0, its purpose is to convert a pointer either into an error > code or 0. That is what is done here, and it is done all over the place > in the kernel. I don't see your problem with it. Care to explain ? > > > > It converts a pointer > > > to a "classic" return code, with negative errors and 0 on success. > > > > > > It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1 > > > IS_ERR and 2 PTR_ERR, one in dev_err() and another to return). > > > > I don't see how this is relevant. > > You lost me. Really, please explain your problem with PTR_ERR_OR_ZERO(). I don't know what I was thinking about... You, guys, are right, sorry for my noise. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 5, 2022 at 10:51 PM Dmitry Torokhov wrote: > On Mon, Sep 05, 2022 at 10:41:40PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov > > wrote: > > > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: > > > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > > > wrote: ... > > > > > - gpiod = devm_gpiod_get_from_of_node(>dev, np, > > > > > - > > > > > "nvidia,phy-reset-gpio", > > > > > - 0, GPIOD_OUT_HIGH, > > > > > - > > > > > "ulpi_phy_reset_b"); > > > > > + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset", > > > > > + GPIOD_OUT_HIGH); > > > > > err = PTR_ERR_OR_ZERO(gpiod); > > > > > > > > What does _OR_ZERO mean now? > > > > > > This converts a pointer to an error code if a pointer represents > > > ERR_PTR() encoded error, or 0 to indicate success. > > > > Yes, I know that. My point is, how is it useful now (or even before)? > > I mean that devm_gpio_get() never returns NULL, right? > > What does returning NULL have to do with anything. It has to do with a dead code. If defm_gpiod_get() does not return NULL, then why do we even bother to check? > It converts a pointer > to a "classic" return code, with negative errors and 0 on success. > > It allows to not use multiple IS_ERR/PTR_ERR in the code (I'd need 1 > IS_ERR and 2 PTR_ERR, one in dev_err() and another to return). I don't see how this is relevant. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 5, 2022 at 10:40 PM Dmitry Torokhov wrote: > On Mon, Sep 05, 2022 at 01:59:44PM +0300, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov > > wrote: ... > > > - gpiod = devm_gpiod_get_from_of_node(>dev, np, > > > - > > > "nvidia,phy-reset-gpio", > > > - 0, GPIOD_OUT_HIGH, > > > - "ulpi_phy_reset_b"); > > > + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset", > > > + GPIOD_OUT_HIGH); > > > err = PTR_ERR_OR_ZERO(gpiod); > > > > What does _OR_ZERO mean now? > > This converts a pointer to an error code if a pointer represents > ERR_PTR() encoded error, or 0 to indicate success. Yes, I know that. My point is, how is it useful now (or even before)? I mean that devm_gpio_get() never returns NULL, right? -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 5, 2022 at 6:13 PM Guenter Roeck wrote:
> On 9/5/22 04:09, Andy Shevchenko wrote:
> > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
> > wrote:
...
> >> + count = device_property_count_u32(dev->parent,
> >> "rohm,hw-timeout-ms");
> >> + if (count < 0 && count != -EINVAL)
> >> + return count;
> >> +
> >> + if (count > 0) {
> >
> >> + if (count > ARRAY_SIZE(hw_margin))
> >> + return -EINVAL;
> >
> > Why double check? You may move it out of the (count > 0).
>
> Two checks will always be needed, so I don't entirely see
> how that would be better.
But not nested. That's my point:
if (count > ARRAY_SIZE())
return ...
if (count > 0)
...
> >> - if (ret == 1)
> >> - hw_margin_max = hw_margin[0];
> >
> >> + ret = device_property_read_u32_array(dev->parent,
> >> +"rohm,hw-timeout-ms",
> >> +hw_margin, count);
> >> + if (ret < 0)
> >> + return ret;
> >
> > So, only this needs the count > 0 check since below already has it
> > implicitly.
> >
> Sorry, I don't understand this comment.
if (count > 0) {
ret = device_property_read_u32_array(...);
...
}
if (count == 1)
...
if (count == 2)
...
But here it might be better to have the nested conditionals.
> >> - if (ret == 2) {
> >> - hw_margin_max = hw_margin[1];
> >> - hw_margin_min = hw_margin[0];
> >> + if (count == 1)
> >> + hw_margin_max = hw_margin[0];
> >> +
> >> + if (count == 2) {
> >> + hw_margin_max = hw_margin[1];
> >> + hw_margin_min = hw_margin[0];
> >> + }
> >> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 5, 2022 at 4:19 PM Matti Vaittinen wrote: > On 9/5/22 13:40, Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov > > wrote: ... > >> + vout_mode = device_property_read_bool(pdev->dev.parent, > >> + "rohm,vout1-en-low"); > > > > They all using parent device and you may make code neater by adding > > > >struct device *parent = pdev->dev.parent; > > This is a matter of personal preference. I prefer seeing > pdev->dev.parent - as it is more obvious (to me) what the 'pdev' is than > what 'parent' would be. > > I'd use the local variable only when it shortens at least one of the > lines so that we avoid splitting it. After that being said - I'm not > going to argue over this change either if one who is improving the > driver wants to use the "helper" variable here. And I believe the quoted one is exactly the case of what you are saying above. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 00/11] Get rid of [devm_]gpiod_get_from_of_node() public APIs
On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov wrote: > > I would like to stop exporting OF-specific [devm_]gpiod_get_from_of_node() > so that gpiolib can be cleaned a bit. We can do that by switching drivers > to use generic fwnode API ([devm_]fwnode_gpiod_get()). By doing so we open > the door to augmenting device tree and ACPI information through secondary > software properties (once we teach gpiolib how to handle those). > > I hope that relevant maintainers will take patches through their trees and > then we could merge the last one some time after -rc1. I'm in favour of the series, but some comments would be good to be addressed. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 10/11] watchdog: bd9576_wdt: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
wrote:
>
> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in
it, switch
> bd9576_wdt_probe() to the generic device property API as well.
...
> struct device *dev = >dev;
struct device *parent = dev->parent;
can make your code slightly neater.
...
> + count = device_property_count_u32(dev->parent, "rohm,hw-timeout-ms");
> + if (count < 0 && count != -EINVAL)
> + return count;
> +
> + if (count > 0) {
> + if (count > ARRAY_SIZE(hw_margin))
> + return -EINVAL;
Why double check? You may move it out of the (count > 0).
...
> - if (ret == 1)
> - hw_margin_max = hw_margin[0];
> + ret = device_property_read_u32_array(dev->parent,
> +"rohm,hw-timeout-ms",
> +hw_margin, count);
> + if (ret < 0)
> + return ret;
So, only this needs the count > 0 check since below already has it implicitly.
> - if (ret == 2) {
> - hw_margin_max = hw_margin[1];
> - hw_margin_min = hw_margin[0];
> + if (count == 1)
> + hw_margin_max = hw_margin[0];
> +
> + if (count == 2) {
> + hw_margin_max = hw_margin[1];
> + hw_margin_min = hw_margin[0];
> + }
> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 04/11] usb: phy: tegra: switch to using devm_gpiod_get()
On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov
wrote:
>
> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> device property API.
>
> I believe that the only reason the driver, instead of the standard
> devm_gpiod_get(), used devm_gpiod_get_from_of_node() is because it
> wanted to set up a pretty consumer name for the GPIO, and we now have
> a special API for that.
...
> - gpiod = devm_gpiod_get_from_of_node(>dev, np,
> - "nvidia,phy-reset-gpio",
> - 0, GPIOD_OUT_HIGH,
> - "ulpi_phy_reset_b");
> + gpiod = devm_gpiod_get(>dev, "nvidia,phy-reset",
> + GPIOD_OUT_HIGH);
> err = PTR_ERR_OR_ZERO(gpiod);
What does _OR_ZERO mean now?
> if (err) {
> dev_err(>dev,
> "Request failed for reset GPIO: %d\n", err);
> return err;
> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 02/11] drm/tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 5, 2022 at 9:32 AM Dmitry Torokhov wrote: > > I would like to limit (or maybe even remove) use of > [devm_]gpiod_get_from_of_node in drivers so that gpiolib can be cleaned > a bit, so let's switch to the generic device property API. > It may even > help with handling secondary fwnodes when gpiolib is taught to handle > gpios described by swnodes. I would remove this sentence from all commit messages since it's a debatable thing and might even not happen, so the above is a pure speculation. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 5, 2022 at 1:53 PM Pali Rohár wrote: > On Monday 05 September 2022 13:49:21 Andy Shevchenko wrote: ... > > It's not the same dev and its node in this case. There is one reset > > for _all_ ports, here is the reset on _per port_ basis. > > aardvark is single port controller. So it is basically same. Yep, just replied to my message. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 5, 2022 at 1:49 PM Andy Shevchenko wrote: > > On Mon, Sep 5, 2022 at 10:23 AM Pali Rohár wrote: > > On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote: > > ... > > > > - rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, > > > - "reset-gpios", > > > 0, > > > - GPIOD_OUT_LOW, > > > - label); > > > + rp->reset_gpio = devm_fwnode_gpiod_get(dev, > > > + > > > of_fwnode_handle(port), > > > +"reset", > > > +GPIOD_OUT_LOW, > > > +label); > > > > Why in pci-aardvark.c for PERST# reset-gpio you have used > > devm_gpiod_get_optional() and here in pci-tegra.c you have used > > devm_fwnode_gpiod_get()? I think that PERST# logic is same in both > > drivers. > > It's not the same dev and its node in this case. There is one reset > for _all_ ports, here is the reset on _per port_ basis. Actually I'm wrong, the aardvark has only one port (?) to serve there. In any case, here dev == dev->of_node, here dev != port. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 01/11] PCI: tegra: switch to using devm_fwnode_gpiod_get
On Mon, Sep 5, 2022 at 10:23 AM Pali Rohár wrote: > On Sunday 04 September 2022 23:30:53 Dmitry Torokhov wrote: ... > > - rp->reset_gpio = devm_gpiod_get_from_of_node(dev, port, > > - "reset-gpios", 0, > > - GPIOD_OUT_LOW, > > - label); > > + rp->reset_gpio = devm_fwnode_gpiod_get(dev, > > +of_fwnode_handle(port), > > +"reset", > > +GPIOD_OUT_LOW, > > +label); > > Why in pci-aardvark.c for PERST# reset-gpio you have used > devm_gpiod_get_optional() and here in pci-tegra.c you have used > devm_fwnode_gpiod_get()? I think that PERST# logic is same in both > drivers. It's not the same dev and its node in this case. There is one reset for _all_ ports, here is the reset on _per port_ basis. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 06/11] PCI: aardvark: switch to using devm_gpiod_get_optional()
On Mon, Sep 5, 2022 at 10:02 AM Pali Rohár wrote:
> On Sunday 04 September 2022 23:30:58 Dmitry Torokhov wrote:
> > I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> > so that gpiolib can be cleaned a bit, so let's switch to the generic
> > device property API.
> >
> > I believe that the only reason the driver, instead of the standard
> > devm_gpiod_get_optional(), used devm_gpiod_get_from_of_node() is
> > because it wanted to set up a pretty consumer name for the GPIO,
>
> IIRC consumer name is not used at all.
It's. The user space tools use it as a label. So, GPIO line can have
"name" (this is provider specific) and "label" (which is consumer
specific, i.o.w. how we use this line).
...
> > + if (ret != -EPROBE_DEFER)
> > + dev_err(dev, "Failed to get reset-gpio: %i\n",
> > + ret);
> > + return ret;
I understand that in the input subsystem maintainer's hat you don't
like dev_err_probe(), but it's a good case to have it here.
...
> > + ret = gpiod_set_consumer_name(pcie->reset_gpio, "pcie1-reset");
> > + if (ret) {
> > + dev_err(dev, "Failed to set reset gpio name: %d\n", ret);
> > + return ret;
> > }
Ditto.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 08/11] regulator: bd71815: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
wrote:
>
> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> fwnode property API.
Special thanks for using dev_fwnode().
Reviewed-by: Andy Shevchenko
(Dunno if my suggestion about parent applies here)
> Signed-off-by: Dmitry Torokhov
>
> diff --git a/drivers/regulator/bd71815-regulator.c
> b/drivers/regulator/bd71815-regulator.c
> index acaa6607898e..c2b8b8be7824 100644
> --- a/drivers/regulator/bd71815-regulator.c
> +++ b/drivers/regulator/bd71815-regulator.c
> @@ -571,11 +571,10 @@ static int bd7181x_probe(struct platform_device *pdev)
> dev_err(>dev, "No parent regmap\n");
> return -ENODEV;
> }
> - ldo4_en = devm_gpiod_get_from_of_node(>dev,
> - pdev->dev.parent->of_node,
> -"rohm,vsel-gpios", 0,
> -GPIOD_ASIS, "ldo4-en");
>
> + ldo4_en = devm_fwnode_gpiod_get(>dev,
> + dev_fwnode(pdev->dev.parent),
> + "rohm,vsel", GPIOD_ASIS, "ldo4-en");
> if (IS_ERR(ldo4_en)) {
> ret = PTR_ERR(ldo4_en);
> if (ret != -ENOENT)
>
> --
> b4 0.10.0-dev-fc921
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 09/11] regulator: bd9576: switch to using devm_fwnode_gpiod_get()
On Mon, Sep 5, 2022 at 9:33 AM Dmitry Torokhov
wrote:
>
> I would like to stop exporting OF-specific devm_gpiod_get_from_of_node()
> so that gpiolib can be cleaned a bit, so let's switch to the generic
> fwnode property API.
>
> While at it switch the rest of the calls to read properties in
> bd957x_probe() to the generic device property API as well.
With or without below addressed,
Reviewed-by: Andy Shevchenko
> Signed-off-by: Dmitry Torokhov
>
> diff --git a/drivers/regulator/bd9576-regulator.c
> b/drivers/regulator/bd9576-regulator.c
> index aa42da4d141e..393c8693b327 100644
> --- a/drivers/regulator/bd9576-regulator.c
> +++ b/drivers/regulator/bd9576-regulator.c
> @@ -12,6 +12,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -939,8 +940,8 @@ static int bd957x_probe(struct platform_device *pdev)
> }
>
> ic_data->regmap = regmap;
> - vout_mode = of_property_read_bool(pdev->dev.parent->of_node,
> -"rohm,vout1-en-low");
> + vout_mode = device_property_read_bool(pdev->dev.parent,
> + "rohm,vout1-en-low");
They all using parent device and you may make code neater by adding
struct device *parent = pdev->dev.parent;
at the definition block of the probe function.
> if (vout_mode) {
> struct gpio_desc *en;
>
> @@ -948,10 +949,10 @@ static int bd957x_probe(struct platform_device *pdev)
>
> /* VOUT1 enable state judged by VOUT1_EN pin */
> /* See if we have GPIO defined */
> - en = devm_gpiod_get_from_of_node(>dev,
> -pdev->dev.parent->of_node,
> -"rohm,vout1-en-gpios", 0,
> -GPIOD_OUT_LOW, "vout1-en");
> + en = devm_fwnode_gpiod_get(>dev,
> + dev_fwnode(pdev->dev.parent),
> + "rohm,vout1-en", GPIOD_OUT_LOW,
> + "vout1-en");
> if (!IS_ERR(en)) {
> /* VOUT1_OPS gpio ctrl */
> /*
> @@ -986,8 +987,8 @@ static int bd957x_probe(struct platform_device *pdev)
> * like DDR voltage selection.
> */
> platform_set_drvdata(pdev, ic_data);
> - ddr_sel = of_property_read_bool(pdev->dev.parent->of_node,
> -"rohm,ddr-sel-low");
> + ddr_sel = device_property_read_bool(pdev->dev.parent,
> + "rohm,ddr-sel-low");
> if (ddr_sel)
> ic_data->regulator_data[2].desc.fixed_uV = 135;
> else
>
> --
> b4 0.10.0-dev-fc921
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v9 00/10] Add MediaTek MT6370 PMIC support
On Tue, Aug 30, 2022 at 11:37:20AM +0800, ChiaEn Wu wrote: > From: ChiaEn Wu > > This patch series add MediaTek MT6370 PMIC support and add a index macro > to . The MT6370 is a highly-integrated smart power > management IC, which includes a single cell Li-Ion/Li-Polymer switching > battery charger, a USB Type-C & Power Delivery (PD) controller, dual > Flash LED current sources, a RGB LED driver, a backlight WLED driver, > a display bias driver and a general LDO for portable devices. > > First, in this series of patches, > 'dt-binding: mfd', 'mfd driver' has been applied by Lee in the v7. > https://lore.kernel.org/all/yvjdpq0mwnpqz...@google.com/ > https://lore.kernel.org/all/yvjdxepc2cb58...@google.com/ > > 'tcpci driver' has been applied by Greg in the v8. > https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/commit/?h=usb-next=c2a8ea5997fdfeb43eda259d5533234c3cae05d7 > > Second, the LED RGB driver is based on Andy's patch which moves > led_init_default_state_get() to the global header. > https://lore.kernel.org/all/20220805154907.32263-3-andriy.shevche...@linux.intel.com/ > > In addition, we added a macro to the for declaring the > linear_range struct simply (see patch v9-0005) and made some changes for > MT6370 drivers (see v9 section of the change log below). Your cover letter is dangling. Make sure you are using --cover-letter --thread when preparing the series. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 00/14] Use devm helpers for regulator get and enable
On Fri, Aug 19, 2022 at 10:20 PM Matti Vaittinen
wrote:
>
> Use devm helpers for regulator get and enable
>
> NOTE: The series depends on commit
> ee94aff2628b ("Devm helpers for regulator get and enable")
> which currently sits in Mark's regulator/for-next
>
> A few* drivers seem to pattern demonstrated by pseudocode:
>
> - devm_regulator_get()
> - regulator_enable()
> - devm_add_action_or_reset(regulator_disable())
>
> devm helpers for this pattern were added to remove bunch of code from
remove a bunch
> drivers. Typically following:
>
> - replace 3 calls (devm_regulator_get[_optional](), regulator_enable(),
> devm_add_action_or_reset()) with just one
> (devm_regulator_get_enable[_optional]()).
> - drop disable callback.
>
> I believe this simplifies things by removing some dublicated code.
duplicated
> This series reowrks a few drivers. There is still plenty of fish in the
reworks
> sea for people who like to improve the code (or count the beans ;]).
>
> Finally - most of the converted drivers have not been tested (other than
> compile-tested) due to lack of HW. All reviews and testing is _highly_
> appreciated (as always!).
...
> docs: devres: regulator: Add new get_enable functions to devres.rst
> clk: cdce925: simplify using devm_regulator_get_enable()
> gpu: drm: simplify drivers using devm_regulator_*get_enable*()
> hwmon: lm90: simplify using devm_regulator_get_enable()
> hwmon: adm1177: simplify using devm_regulator_get_enable()
hwmon uses a different pattern for the Subject line.
--
With Best Regards,
Andy Shevchenko
Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable
On Tue, Aug 16, 2022 at 8:37 AM Laurent Pinchart
wrote:
> On Mon, Aug 15, 2022 at 01:58:55PM -0700, Stephen Boyd wrote:
> > Quoting Laurent Pinchart (2022-08-15 11:52:36)
> > > On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote:
...
> > > we'll run into trouble. Supplying active high input signals
> > > to a device that is not powered can lead to latch-up, which tends to
> > > only manifest after a statistically significant number of occurrences of
> > > the condition, and can slowly damage the hardware over time. This is a
> > > real concern as it will typically not be caught during early
> > > development. I think we would still be better off with requiring drivers
> > > to manually handle powering off the device until we provide a mechanism
> > > that can do so safely in an automated way.
> >
> > Can you describe the error scenario further? I think it's driver author
> > error that would lead to getting and enabling the regulator after
> > getting and enabling a clk that drives out a clock signal on some pins
> > that aren't powered yet. I'm not sure that's all that much easier to do
> > with these sorts of devm APIs, but if it is then I'm concerned.
>
> You will very quickly see drivers doing this (either directly or
> indirectly):
>
> probe()
> {
> devm_clk_get_enabled();
> devm_regulator_get_enable();
> }
And how is it devm specific? If the author puts the same without devm
the ordering would be wrong, correct? devm allows us to focus on
ordering in a *single* place, which is a win. You seem to be proposing
to make a high burden on a driver's author to focus on ordering in the
*three* places. I disagree with that. Yet the driver author has to
understand many issues with any tool they use. So the root cause of
your whining is rather on the edge of documentation and education.
(Yes, I have heard about issues with object lifetime in v4l2
subdevices regarding to devm, but it seems irrelevant to devm
mechanism itself.)
> Without a devres-based get+enable API drivers can get the resources they
> need in any order, possibly moving some of those resource acquisition
> operations to different functions, and then have a clear block of code
> that enables the resources in the right order. These devres helpers give
> a false sense of security to driver authors and they will end up
> introducing problems, the same way that devm_kzalloc() makes it
> outrageously easy to crash the kernel by disconnecting a device that is
> in use.
--
With Best Regards,
Andy Shevchenko
Re: (subset) [PATCH v2 0/7] Devm helpers for regulator get and enable
On Mon, Aug 15, 2022 at 11:20 PM Laurent Pinchart wrote: > On Mon, Aug 15, 2022 at 05:33:06PM +0100, Mark Brown wrote: ... > However, should a devm_clk_get_enable() or similar function be > implemented, we'll run into trouble. And in 5.19 we have devm_clk_get_enable(), are we already in trouble? -- With Best Regards, Andy Shevchenko
Re: tiny drm driver for NOKIA-5110 LCD
On Tue, Aug 9, 2022 at 6:14 PM Aliliche Larbi wrote: > > Good morning sir, > > I am developing a linux driver for the nokia 5110 lcd(monochrome), > I would like to know if I can use the drm API or tiny drm for this lcd. > I'm writing to you because I saw your commit in /staging/fbtft/TODO > where you talk about converting all fbtft drivers to drm_simple_display_pipe. > > Is this possible for simple monochrome lcds like the 5110 one? You may convert _existing_ driver [1] to real DRM, like it's done for SSD130x [2]. [1]: https://elixir.bootlin.com/linux/latest/source/drivers/staging/fbtft/fb_pcd8544.c [2]: https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/solomon -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 3/3] efi: earlycon: Add support for generic framebuffers and move to console subsystem
On Sat, Aug 6, 2022 at 6:38 PM Markuss Broks wrote:
>
> Add early console support for generic linear framebuffer devices.
> This driver supports probing from cmdline early parameters
> or from the device-tree using information in simple-framebuffer node.
> The EFI functionality should be retained in whole.
> The driver was disabled on ARM because of a bug in early_ioremap
We refer to functions like func().
> implementation on ARM and on IA64 because of lack of early_memremap_prot.
Ditto.
...
> +#include
Can it be placed after linux/* ones?
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
...
> +static int __init simplefb_earlycon_remap_fb(void)
> +{
> + unsigned long mapping;
+ Blank line.
> + /* bail if there is no bootconsole or it has been disabled already */
> + if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
> + return 0;
> +
> + if (region_intersects(info.phys_base, info.size,
> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE) ==
> REGION_INTERSECTS)
> + mapping = MEMREMAP_WB;
> + else
> + mapping = MEMREMAP_WC;
> + info.virt_base = memremap(info.phys_base, info.size, mapping);
> +
> + return info.virt_base ? 0 : -ENOMEM;
Easier to read the standard pattern:
if (!info.virt_base)
return -ENOMEM;
return 0;
> +}
...
> +static void simplefb_earlycon_write_char(u8 *dst, unsigned char c, unsigned
> int h)
> +{
> + const u8 *src;
> + int m, n, bytes;
> + u8 x;
> +
> + bytes = BITS_TO_BYTES(font->width);
> + src = font->data + c * font->height * bytes + h * bytes;
> +
> + for (m = 0; m < font->width; m++) {
> + n = m % 8;
> + x = *(src + m / 8);
I would write it as
x = src[m / 8];
> + if ((x >> (7 - n)) & 1)
> + memset(dst, 0xff, (info.depth / 8));
Too many parentheses.
> + else
> + memset(dst, 0, (info.depth / 8));
Ditto.
> + dst += (info.depth / 8);
Ditto.
> + }
> +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2 1/3] drivers: serial: earlycon: Correct argument name
On Sat, Aug 6, 2022 at 6:37 PM Markuss Broks wrote: > > The "node" argument is actually an offset, and it's also > an "int", and not "unsigned long". Correct the of_setup_earlycon > function. Suggested-by: Greg KH? > Signed-off-by: Markuss Broks -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 12/13] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote: > > From: Alice Chen > > The MediaTek MT6370 is a highly-integrated smart power management IC, > which includes a single cell Li-Ion/Li-Polymer switching battery > charger, a USB Type-C & Power Delivery (PD) controller, dual Flash > LED current sources, a RGB LED driver, a backlight WLED driver, > a display bias driver and a general LDO for portable devices. > > Add a support for the MT6370 Flash LED driver. Flash LED in MT6370 > has 2 channels and support torch/strobe mode. Same comments as per previous LED related patch. ... > + /* > +* For the flash to turn on/off, we need to wait HW ramping up/down > time > +* 5ms/500us to prevent the unexpected problem. > +*/ > + if (!priv->fled_strobe_used && curr) > + usleep_range(5000, 6000); > + else if (priv->fled_strobe_used && !curr) > + usleep_range(500, 600); Now it's much better! ... > + /* > +* Always configure as min level when off to > +* prevent flash current spike /* * You need to check the style * of multi-line comments like * this one. */ > +*/ -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 13/13] video: backlight: mt6370: Add MediaTek MT6370 support
On Fri, Aug 5, 2022 at 9:08 AM ChiaEn Wu wrote: > > From: ChiaEn Wu > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > Add a support for the MediaTek MT6370 backlight driver. > It controls 4 channels of 8 series WLEDs in > 2048 (only for MT6370/MT6371) / 16384 (only for MT6372) > current steps (30 mA) in exponential or linear mapping curves. ... > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include Why? Since below drop this and use fully fwnode / device property APIs. > +#include Missed property.h which is heavily used in.the driver > +#include ... > + /* > +* MT6372 uses 14 bits to control the brightness but MT6370 and MT6371 > +* use 11 bits. They are different so we have to use this function to > +* check the vendor ID and use different mask, shift and default > +* maxiimum brightness value. Use spell-checker for all your patches. > +*/ -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 11/13] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote:
>
> From: ChiYuan Huang
>
> The MediaTek MT6370 is a highly-integrated smart power management IC,
> which includes a single cell Li-Ion/Li-Polymer switching battery
> charger, a USB Type-C & Power Delivery (PD) controller, dual
> Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> a display bias driver and a general LDO for portable devices.
>
> Add a support for the MediaTek MT6370 Current Sink Type LED Indicator
Add support
(This is also for all other commit messages)
> driver. It can control four channels current-sink RGB LEDs with 3 modes,
3 modes:
> constant current, PWM, and breath mode.
...
> +static int mt6370_gen_breath_pattern(struct mt6370_priv *priv,
> +struct led_pattern *pattern, u32 len,
> +u8 *pattern_val, u32 val_len)
> +{
> + enum mt6370_led_ranges sel_range;
> + struct led_pattern *curr;
> + unsigned int sel;
> + u32 val = 0;
> + int i;
> +
> + if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2)
> + return -EINVAL;
> +
> + /*
> +* Pattern list
> +* tr1: byte 0, b'[7: 4]
> +* tr2: byte 0, b'[3: 0]
> +* tf1: byte 1, b'[7: 4]
> +* tf2: byte 1, b'[3: 0]
> +* ton: byte 2, b'[7: 4]
> +* toff: byte 2, b'[3: 0]
> +*/
> + for (i = 0; i < P_MAX_PATTERNS; i++) {
> + curr = pattern + i;
> +
> + sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON;
> +
> + linear_range_get_selector_within(priv->ranges + sel_range,
> +curr->delta_t, );
> +
> + val <<= i % 2 == 0 ? 8 : 0;
> + val |= sel << (i % 2 == 0 ? 4 : 0);
It's too cryptic, why not simply:
if (i % 2) {
val |= sel;
} else {
val <<= 8;
val |= sel << 4;
}
?
> + }
> +
> + put_unaligned_be24(val, pattern_val);
> +
> + return 0;
> +}
...
> + const char * const states[] = { "off", "keep", "on" };
> + ret = fwnode_property_read_string(init_data->fwnode, "default-state",
> + _str);
> + if (!ret) {
> + ret = match_string(states, ARRAY_SIZE(states), stat_str);
> + if (ret < 0)
> + ret = STATE_OFF;
> +
> + led->default_state = ret;
> + }
Replace this by using led_init_default_state_get().
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v7 10/13] power: supply: mt6370: Add MediaTek MT6370 charger driver
On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote: > > From: ChiaEn Wu > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > Add a support for the MediaTek MT6370 Charger driver. The charger module > of MT6370 supports High-Accuracy Voltage/Current Regulation, > Average Input Current Regulation, Battery Temperature Sensing, > Over-Temperature Protection, DPDM Detection for BC1.2. On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote: > > From: ChiYuan Huang > > The MediaTek MT6370 is a highly-integrated smart power management IC, > which includes a single cell Li-Ion/Li-Polymer switching battery > charger, a USB Type-C & Power Delivery (PD) controller, dual > Flash LED current sources, a RGB LED driver, a backlight WLED driver, > a display bias driver and a general LDO for portable devices. > > Add a support for the Type-C & Power Delivery controller in > MediaTek MT6370 IC. FWIW, Reviewed-by: Andy Shevchenko > Signed-off-by: ChiaEn Wu > --- > > v7 > - Revise the method to enable/disable irq > - Revise all 'if (ret < 0)' to 'if (ret)' after using > mt6370_chg_field_set/get() > - Revise all 'OTG' text > --- > drivers/power/supply/Kconfig | 14 + > drivers/power/supply/Makefile | 1 + > drivers/power/supply/mt6370-charger.c | 965 > ++ > 3 files changed, 980 insertions(+) > create mode 100644 drivers/power/supply/mt6370-charger.c > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index 1aa8323..591deb8 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -619,6 +619,20 @@ config CHARGER_MT6360 > Average Input Current Regulation, Battery Temperature Sensing, > Over-Temperature Protection, DPDM Detection for BC1.2. > > +config CHARGER_MT6370 > + tristate "MediaTek MT6370 Charger Driver" > + depends on MFD_MT6370 > + depends on REGULATOR > + select LINEAR_RANGES > + help > + Say Y here to enable MT6370 Charger Part. > + The device supports High-Accuracy Voltage/Current Regulation, > + Average Input Current Regulation, Battery Temperature Sensing, > + Over-Temperature Protection, DPDM Detection for BC1.2. > + > + This driver can also be built as a module. If so, the module > + will be called "mt6370-charger". > + > config CHARGER_QCOM_SMBB > tristate "Qualcomm Switch-Mode Battery Charger and Boost" > depends on MFD_SPMI_PMIC || COMPILE_TEST > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 7f02f36..8c95276 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -82,6 +82,7 @@ obj-$(CONFIG_CHARGER_MAX8997) += max8997_charger.o > obj-$(CONFIG_CHARGER_MAX8998) += max8998_charger.o > obj-$(CONFIG_CHARGER_MP2629) += mp2629_charger.o > obj-$(CONFIG_CHARGER_MT6360) += mt6360_charger.o > +obj-$(CONFIG_CHARGER_MT6370) += mt6370-charger.o > obj-$(CONFIG_CHARGER_QCOM_SMBB)+= qcom_smbb.o > obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o > obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o > diff --git a/drivers/power/supply/mt6370-charger.c > b/drivers/power/supply/mt6370-charger.c > new file mode 100644 > index 000..42fddc3 > --- /dev/null > +++ b/drivers/power/supply/mt6370-charger.c > @@ -0,0 +1,965 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Richtek Technology Corp. > + * > + * Author: ChiaEn Wu > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define MT6370_REG_CHG_CTRL1 0x111 > +#define MT6370_REG_CHG_CTRL2 0x112 > +#define MT6370_REG_CHG_CTRL3 0x113 > +#define MT6370_REG_CHG_CTRL4 0x114 > +#define MT6370_REG_CHG_CTRL5 0x115 > +#define MT6370_REG_CHG_CTRL6 0x116 > +#define MT6370_REG_CHG_CTRL7 0x117 > +#define MT6370_REG_CHG_CTRL8 0x118 > +#define MT6370_REG_CHG_CTRL9 0x119 > +#define MT6370_REG_CHG_CTRL10 0x11A > +#define MT6370_REG_DEVICE_TYPE 0x122 > +#define MT6370_REG_USB_STATUS1 0x127 > +#define MT6370_REG_C
Re: [PATCH v7 09/13] iio: adc: mt6370: Add MediaTek MT6370 support
On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote: > > From: ChiaEn Wu > > MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger > with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight > driver, display bias voltage supply, one general purpose LDO, and the > USB Type-C & PD controller complies with the latest USB Type-C and PD > standards. > > Add a support for the MT6370 ADC driver for system monitoring, including > charger current, voltage, and temperature. On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote: > > From: ChiYuan Huang > > The MediaTek MT6370 is a highly-integrated smart power management IC, > which includes a single cell Li-Ion/Li-Polymer switching battery > charger, a USB Type-C & Power Delivery (PD) controller, dual > Flash LED current sources, a RGB LED driver, a backlight WLED driver, > a display bias driver and a general LDO for portable devices. > > Add a support for the Type-C & Power Delivery controller in > MediaTek MT6370 IC. Reviewed-by: Andy Shevchenko > Reviewed-by: AngeloGioacchino Del Regno > > Signed-off-by: ChiaEn Wu > --- > > v7 > - Add AICR(100mA ~ 350mA), ICHG(100mA ~ 800mA) macros > - Remove 400mA AICR and 900mA ICHG macros > - Revise using 'if-else' to 'switch-case' in mt6370_adc_read_scale() > where the adc channel is ibus or ibat > --- > drivers/iio/adc/Kconfig | 12 ++ > drivers/iio/adc/Makefile | 1 + > drivers/iio/adc/mt6370-adc.c | 305 > +++ > 3 files changed, 318 insertions(+) > create mode 100644 drivers/iio/adc/mt6370-adc.c > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig > index 7fe5930..995cbb5 100644 > --- a/drivers/iio/adc/Kconfig > +++ b/drivers/iio/adc/Kconfig > @@ -736,6 +736,18 @@ config MEDIATEK_MT6360_ADC > is used in smartphones and tablets and supports a 11 channel > general purpose ADC. > > +config MEDIATEK_MT6370_ADC > + tristate "MediaTek MT6370 ADC driver" > + depends on MFD_MT6370 > + help > + Say yes here to enable MediaTek MT6370 ADC support. > + > + This ADC driver provides 9 channels for system monitoring (charger > + current, voltage, and temperature). > + > + This driver can also be built as a module. If so, the module > + will be called "mt6370-adc". > + > config MEDIATEK_MT6577_AUXADC > tristate "MediaTek AUXADC driver" > depends on ARCH_MEDIATEK || COMPILE_TEST > diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile > index 1772a54..c6bc35f 100644 > --- a/drivers/iio/adc/Makefile > +++ b/drivers/iio/adc/Makefile > @@ -68,6 +68,7 @@ obj-$(CONFIG_MCP320X) += mcp320x.o > obj-$(CONFIG_MCP3422) += mcp3422.o > obj-$(CONFIG_MCP3911) += mcp3911.o > obj-$(CONFIG_MEDIATEK_MT6360_ADC) += mt6360-adc.o > +obj-$(CONFIG_MEDIATEK_MT6370_ADC) += mt6370-adc.o > obj-$(CONFIG_MEDIATEK_MT6577_AUXADC) += mt6577_auxadc.o > obj-$(CONFIG_MEN_Z188_ADC) += men_z188_adc.o > obj-$(CONFIG_MESON_SARADC) += meson_saradc.o > diff --git a/drivers/iio/adc/mt6370-adc.c b/drivers/iio/adc/mt6370-adc.c > new file mode 100644 > index 000..2a46471 > --- /dev/null > +++ b/drivers/iio/adc/mt6370-adc.c > @@ -0,0 +1,305 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Richtek Technology Corp. > + * > + * Author: ChiaEn Wu > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +#define MT6370_REG_CHG_CTRL3 0x113 > +#define MT6370_REG_CHG_CTRL7 0x117 > +#define MT6370_REG_CHG_ADC 0x121 > +#define MT6370_REG_ADC_DATA_H 0x14C > + > +#define MT6370_ADC_START_MASK BIT(0) > +#define MT6370_ADC_IN_SEL_MASK GENMASK(7, 4) > +#define MT6370_AICR_ICHG_MASK GENMASK(7, 2) > + > +#define MT6370_AICR_100_mA 0x0 > +#define MT6370_AICR_150_mA 0x1 > +#define MT6370_AICR_200_mA 0x2 > +#define MT6370_AICR_250_mA 0x3 > +#define MT6370_AICR_300_mA 0x4 > +#define MT6370_AICR_350_mA 0x5 > + > +#define MT6370_ICHG_100_mA 0x0 > +#define MT6370_ICHG_200_mA 0x1 > +#define MT6370_ICHG_300_mA 0x2 > +#define MT6370_ICHG_400_mA 0x3 > +#define MT6370_ICHG_500_mA 0x4 > +#define MT6370_ICHG_600_mA 0x5 > +#define MT6370_ICHG_700_mA 0x6 > +#define MT6370_ICHG_800_mA 0x7 > + > +#define A
Re: [PATCH v7 08/13] usb: typec: tcpci_mt6370: Add MediaTek MT6370 tcpci driver
On Fri, Aug 5, 2022 at 9:07 AM ChiaEn Wu wrote:
>
> From: ChiYuan Huang
>
> The MediaTek MT6370 is a highly-integrated smart power management IC,
> which includes a single cell Li-Ion/Li-Polymer switching battery
> charger, a USB Type-C & Power Delivery (PD) controller, dual
> Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> a display bias driver and a general LDO for portable devices.
>
> Add a support for the Type-C & Power Delivery controller in
> MediaTek MT6370 IC.
Reviewed-by: Andy Shevchenko
> Reviewed-by: AngeloGioacchino Del Regno
>
> Reviewed-by: Guenter Roeck
> Signed-off-by: ChiYuan Huang
> Signed-off-by: ChiaEn Wu
> ---
>
> v7
> - Revise 'devm_add_action_or_reset(dev, ...)' to one line
> - Revise 'return regmap_update_bits(...)' with using positive
> conditional
> ---
> drivers/usb/typec/tcpm/Kconfig| 11 ++
> drivers/usb/typec/tcpm/Makefile | 1 +
> drivers/usb/typec/tcpm/tcpci_mt6370.c | 207
> ++
> 3 files changed, 219 insertions(+)
> create mode 100644 drivers/usb/typec/tcpm/tcpci_mt6370.c
>
> diff --git a/drivers/usb/typec/tcpm/Kconfig b/drivers/usb/typec/tcpm/Kconfig
> index 073fd2e..e6b88ca 100644
> --- a/drivers/usb/typec/tcpm/Kconfig
> +++ b/drivers/usb/typec/tcpm/Kconfig
> @@ -35,6 +35,17 @@ config TYPEC_MT6360
> USB Type-C. It works with Type-C Port Controller Manager
> to provide USB PD and USB Type-C functionalities.
>
> +config TYPEC_TCPCI_MT6370
> + tristate "MediaTek MT6370 Type-C driver"
> + depends on MFD_MT6370
> + help
> + MediaTek MT6370 is a multi-functional IC that includes
> + USB Type-C. It works with Type-C Port Controller Manager
> + to provide USB PD and USB Type-C functionalities.
> +
> + This driver can also be built as a module. The module
> + will be called "tcpci_mt6370".
> +
> config TYPEC_TCPCI_MAXIM
> tristate "Maxim TCPCI based Type-C chip driver"
> help
> diff --git a/drivers/usb/typec/tcpm/Makefile b/drivers/usb/typec/tcpm/Makefile
> index 7d499f3..906d9dc 100644
> --- a/drivers/usb/typec/tcpm/Makefile
> +++ b/drivers/usb/typec/tcpm/Makefile
> @@ -6,4 +6,5 @@ typec_wcove-y := wcove.o
> obj-$(CONFIG_TYPEC_TCPCI) += tcpci.o
> obj-$(CONFIG_TYPEC_RT1711H)+= tcpci_rt1711h.o
> obj-$(CONFIG_TYPEC_MT6360) += tcpci_mt6360.o
> +obj-$(CONFIG_TYPEC_TCPCI_MT6370) += tcpci_mt6370.o
> obj-$(CONFIG_TYPEC_TCPCI_MAXIM)+= tcpci_maxim.o
> diff --git a/drivers/usb/typec/tcpm/tcpci_mt6370.c
> b/drivers/usb/typec/tcpm/tcpci_mt6370.c
> new file mode 100644
> index 000..c5bb201
> --- /dev/null
> +++ b/drivers/usb/typec/tcpm/tcpci_mt6370.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2022 Richtek Technology Corp.
> + *
> + * Author: ChiYuan Huang
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define MT6370_REG_SYSCTRL80x9B
> +
> +#define MT6370_AUTOIDLE_MASK BIT(3)
> +
> +#define MT6370_VENDOR_ID 0x29CF
> +#define MT6370_TCPC_DID_A 0x2170
> +
> +struct mt6370_priv {
> + struct device *dev;
> + struct regulator *vbus;
> + struct tcpci *tcpci;
> + struct tcpci_data tcpci_data;
> +};
> +
> +static const struct reg_sequence mt6370_reg_init[] = {
> + REG_SEQ(0xA0, 0x1, 1000),
> + REG_SEQ(0x81, 0x38, 0),
> + REG_SEQ(0x82, 0x82, 0),
> + REG_SEQ(0xBA, 0xFC, 0),
> + REG_SEQ(0xBB, 0x50, 0),
> + REG_SEQ(0x9E, 0x8F, 0),
> + REG_SEQ(0xA1, 0x5, 0),
> + REG_SEQ(0xA2, 0x4, 0),
> + REG_SEQ(0xA3, 0x4A, 0),
> + REG_SEQ(0xA4, 0x01, 0),
> + REG_SEQ(0x95, 0x01, 0),
> + REG_SEQ(0x80, 0x71, 0),
> + REG_SEQ(0x9B, 0x3A, 1000),
> +};
> +
> +static int mt6370_tcpc_init(struct tcpci *tcpci, struct tcpci_data *data)
> +{
> + u16 did;
> + int ret;
> +
> + ret = regmap_register_patch(data->regmap, mt6370_reg_init,
> + ARRAY_SIZE(mt6370_reg_init));
> + if (ret)
> + return ret;
> +
> + ret = regmap_raw_read(data->regmap, TCPC_BCD_DEV, , sizeof(u16));
> + if (ret)
> + return ret;
> +
> + if (did == MT6370_TCPC_DID_A)
> + return regmap_write(data->regmap, TCPC_FAULT_CTRL, 0x80);
> +
&
Re: [PATCH 2/2] efi: earlycon: Add support for generic framebuffers and move to fbdev subsystem
On Sat, Jul 30, 2022 at 10:55 AM Markuss Broks wrote: > On 7/29/22 00:19, Andy Shevchenko wrote: > > On Thu, Jul 28, 2022 at 4:32 PM Markuss Broks > > wrote: ... > I suppose we could use something like: > > if (region_intersects() == REGION_INTERSECTS) Yes. ... > >> + ret = sscanf(device->options, "%u,%u,%u", , , > >> ); > >> + if (ret != 3) > >> + return -ENODEV; > > > > Don't we have some standard template of this, something like XxYxD, > > where X, Y, and D are respective decimal numbers? > > I'm not aware of this. I believe we won't introduce more chaos in already existing formats of one or another thing, so I prefer to be stuck with in practice use (e.g. "1024x768x16" without quotes for x=1024, y=768, depth=16). -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] drivers: serial: earlycon: Pass device-tree node
On Fri, Jul 29, 2022 at 9:57 AM Greg Kroah-Hartman wrote: > On Thu, Jul 28, 2022 at 11:04:24PM +0200, Andy Shevchenko wrote: > > On Thu, Jul 28, 2022 at 4:41 PM Greg Kroah-Hartman > > wrote: > > > On Thu, Jul 28, 2022 at 05:28:18PM +0300, Markuss Broks wrote: ... > > > > + unsigned long node; > > > > > > That should not be an unsigned long, but rather an 'int'. Something got > > > messed up, of_setup_earlycon() should be changed to reflect this before > > > propagating the error to other places in the kernel. > > > > It's a pointer, but what puzzles me, why it can't be declared as a such: > > > > struct device_node *node; > > > > ? > > It should not be a pointer, trace things backwards, it comes from a call > to of_setup_earlycon() from early_init_dt_scan_chosen_stdout() which has > offset declared as an int, and then does: > if (of_setup_earlycon(match, offset, options) == 0) > > So why would it be a node? This is a very good question. > > > And it's not really a "node" but an "offset", right? > > > > Seems no. > > Really? What am I missing here? It's me who is missing something here, thanks for your elaboration! After it it becomes clear that your first question should be addressed. -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 12/13] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Fri, Jul 29, 2022 at 8:17 AM szuni chen wrote: > Andy Shevchenko 於 2022年7月25日 週一 下午4:51寫道： > > On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu wrote: > > > > > > From: Alice Chen ... > > > Signed-off-by: Alice Chen > > > > This SoB chain is wrong. Prioritize and read Submitting Patches! > > After reading the Submitted Patches, > ChiaEn Wu wasn't involved in the development but he submitted the patch, > So, ChiaEn Wu should be the last SoB, right? Right. Submitter's SoB is the last SoB in the chain. > I will revise SoB to > > Signed-off-by: SzuNi Chen Not sure I understand the SzuNi <--> Alice transformation... > Signed-off-by: ChiaEn Wu > > If there is anything else I need to fix, please let me know. Thank you. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] efi: earlycon: Add support for generic framebuffers and move to fbdev subsystem
On Thu, Jul 28, 2022 at 4:32 PM Markuss Broks wrote:
>
> Add early console support for generic linear framebuffer devices.
> This driver supports probing from cmdline early parameters
> or from the device-tree using information in simple-framebuffer node.
> The EFI functionality should be retained in whole.
> The driver was disabled on ARM because of a bug in early_ioremap
> implementation on ARM.
...
> - efifb,[options]
> + efifb
> Start an early, unaccelerated console on the EFI
> - memory mapped framebuffer (if available). On cache
> - coherent non-x86 systems that use system memory for
> - the framebuffer, pass the 'ram' option so that it is
> - mapped with the correct attributes.
> + memory mapped framebuffer (if available).
If somebody passes those (legacy) options, what will happen?
...
> config EFI_EARLYCON
> - def_bool y
> - depends on SERIAL_EARLYCON && !ARM && !IA64
> - select FONT_SUPPORT
> - select ARCH_USE_MEMREMAP_PROT
> + bool "EFI early console support"
> + depends on FB_EARLYCON && !IA64
This doesn't sound right. Previously on my configuration it was
selected automatically, now I need to select it explicitly? I mean
that for me EFI_EARLYCON should be selected by default as it was
before.
...
> +static int __init simplefb_earlycon_remap_fb(void)
> +{
> + int is_ram;
+ blank line.
> + /* bail if there is no bootconsole or it has been disabled already */
> + if (!earlycon_console || !(earlycon_console->flags & CON_ENABLED))
> + return 0;
> +
> + is_ram = region_intersects(info.phys_base, info.size,
> + IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE);
> + is_ram = is_ram == REGION_INTERSECTS;
Was it in the original code? Otherwise, I would go with plain conditional:
if (region_intersects())
base = ...
else
base = ...
> + info.virt_base = memremap(info.phys_base, info.size,
> + is_ram ? MEMREMAP_WB : MEMREMAP_WC);
> +
> + return info.virt_base ? 0 : -ENOMEM;
> +}
...
> +static void simplefb_earlycon_write_char(u8 *dst, unsigned char c, unsigned
> int h)
> +{
> + const u8 *src;
> + int m, n, bytes;
> + u8 x;
> +
> + bytes = BITS_TO_BYTES(font->width);
> + src = font->data + c * font->height * bytes + h * bytes;
> +
> + for (m = 0; m < font->width; m++) {
> + n = m % 8;
> + x = *(src + m / 8);
> + if ((x >> (7 - n)) & 1)
> + memset(dst, 0xff, (info.depth / 8));
> + else
> + memset(dst, 0, (info.depth / 8));
> + dst += (info.depth / 8);
> + }
> +}
Wondering if we already have something like this in DRM/fbdev and can
split into a generic helper.
...
> + ret = sscanf(device->options, "%u,%u,%u", , ,
> );
> + if (ret != 3)
> + return -ENODEV;
Don't we have some standard template of this, something like XxYxD,
where X, Y, and D are respective decimal numbers?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 2/2] efi: earlycon: Add support for generic framebuffers and move to fbdev subsystem
On Thu, Jul 28, 2022 at 4:58 PM Markuss Broks wrote: > On 7/28/22 17:39, Greg Kroah-Hartman wrote: > > On Thu, Jul 28, 2022 at 05:28:19PM +0300, Markuss Broks wrote: > >> delete mode 100644 drivers/firmware/efi/earlycon.c > >> create mode 100644 drivers/video/fbdev/earlycon.c > > > > That should be a rename, not a delete/create, right? > > Should this change be split into two separate commits, > one for moving the file and the second for making changes? I think it's a pointer to use `git format-patch -M -C ...` when preparing patches. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] drivers: serial: earlycon: Pass device-tree node
On Thu, Jul 28, 2022 at 4:41 PM Greg Kroah-Hartman wrote: > On Thu, Jul 28, 2022 at 05:28:18PM +0300, Markuss Broks wrote: > > Pass a pointer to device-tree node in case the driver probed from > > OF. This makes early console drivers able to fetch options from > > device-tree node properties. ... > > + unsigned long node; > > That should not be an unsigned long, but rather an 'int'. Something got > messed up, of_setup_earlycon() should be changed to reflect this before > propagating the error to other places in the kernel. It's a pointer, but what puzzles me, why it can't be declared as a such: struct device_node *node; ? > And it's not really a "node" but an "offset", right? Seems no. -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 11/13] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
On Wed, Jul 27, 2022 at 9:37 AM ChiaEn Wu wrote:
> On Tue, Jul 26, 2022 at 8:18 PM Andy Shevchenko
> wrote:
>
> ...
>
> > > Just for saving memory space.
> > > Because these led_classdevs do not be used at the same time.
> > > Or do you think it would be better to rewrite it as follows?
> > > -
> > > struct mt6370_led {
> > >struct led_classdev isink;
> > >struct led_classdev_mc mc;
> > >struct mt6370_priv *priv;
> > >u32 default_state;
> > >u32 index;
> > > };
> > > -
> >
> > You obviously didn't get what I'm talking about...
> > Each union to work properly should have an associated variable that
> > holds the information of which field of the union is in use. Do you
> > have such a variable? If not, how does your code know which one to
> > use? If yes, add a proper comment there.
> >
>
> Ummm... from my understanding,
> if the colors of these four LEDs are set to 'LED_COLOR_ID_RGB' or
> 'LED_COLOR_ID_MULTI' in DT,
> their 'led->index' will be set to 'MT6370_VIRTUAL_MULTICOLOR' in
> 'mt6370_leds_probe()'.
> If so, these led devices will be set as 'struct led_classdev_mc' and
> use related ops functions in 'mt6370_init_led_properties()'.
> Instead, they whose 'led->index' is not 'MT6370_VIRTUAL_MULTICOLOR'
> will be set as 'struct led_classdev'.
> So, maybe the member 'index' of the 'struct mt6370_led' is what you
> describe the information of which field of the union is in use?
>From this description it sounds like it is.
> I will add the proper comment here to describe this thing. I'm so
> sorry for misunderstanding your mean last time.
Yes, please add a compressed version of what you said above to the code.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 11/13] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
On Tue, Jul 26, 2022 at 1:46 PM ChiaEn Wu wrote:
> On Mon, Jul 25, 2022 at 4:41 PM Andy Shevchenko
> wrote:
...
> > > +struct mt6370_led {
> > > + union {
> > > + struct led_classdev isink;
> > > + struct led_classdev_mc mc;
> > > + };
> >
> > Where is the field that makes union work?
>
> Just for saving memory space.
> Because these led_classdevs do not be used at the same time.
> Or do you think it would be better to rewrite it as follows?
> -
> struct mt6370_led {
>struct led_classdev isink;
>struct led_classdev_mc mc;
>struct mt6370_priv *priv;
>u32 default_state;
>u32 index;
> };
> -
You obviously didn't get what I'm talking about...
Each union to work properly should have an associated variable that
holds the information of which field of the union is in use. Do you
have such a variable? If not, how does your code know which one to
use? If yes, add a proper comment there.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 12/13] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Tue, Jul 26, 2022 at 6:15 AM szuni chen wrote: ... > > > +#define MT6370_ITORCH_MIN_UA 25000 > > > +#define MT6370_ITORCH_STEP_UA 12500 > > > +#define MT6370_ITORCH_MAX_UA 40 > > > +#define MT6370_ITORCH_DOUBLE_MAX_UA80 > > > +#define MT6370_ISTRB_MIN_UA5 > > > +#define MT6370_ISTRB_STEP_UA 12500 > > > +#define MT6370_ISTRB_MAX_UA150 > > > +#define MT6370_ISTRB_DOUBLE_MAX_UA 300 > > > > Perhaps _uA would be better and consistent across your series > > regarding current units. > > Yes, _uA will be more consistent, but in general, using upper case in > the define macro is a convention, doesn't it? There is general convention, but there are also: 1) common sense; 2) usage in practice (e.g. _US, etc for *seconds and _HZ for *frequency). My common sense tells me that it is convenient to use mA,uA, etc. Plus "in practice" it's related to use as in your series and elsewhere. But of course it's minor to me, decide yourself. ... > > > + /* > > > +* For the flash to turn on/off, need to wait for HW ramping > > > up/down time > > > > we need > > > > > +* 5ms/500us to prevent the unexpected problem. > > > +*/ > > > + if (!prev && curr) > > > + usleep_range(5000, 6000); > > > + else if (prev && !curr) > > > + udelay(500); > > > > This still remains unanswered, why in the first place we allow > > switching, and a busy loop in the other place? > > If I refine the description to > "For the flash to turn on/off, need to wait for 5ms/500us analog settling > time. > If any flash led is already used, then the analog is settled done, we > don't need to wait again." > is it answer the question? No. I'm talking from the Linux APIs perspective. There is a huge difference between those branches. Please, conduct research, read documentation to understand what my question is about. -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 07/13] mfd: mt6370: Add MediaTek MT6370 support
On Mon, Jul 25, 2022 at 11:06 AM ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 4:43 PM Andy Shevchenko > wrote: ... > > > > > +#define MT6370_REG_DEV_INFO0x100 > > > > > +#define MT6370_REG_CHG_IRQ10x1C0 > > > > > +#define MT6370_REG_CHG_MASK1 0x1E0 > > > > > + > > > > > +#define MT6370_VENID_MASK GENMASK(7, 4) > > > > > + > > > > > +#define MT6370_NUM_IRQREGS 16 > > > > > +#define MT6370_USBC_I2CADDR0x4E > > > > > > > > > +#define MT6370_REG_ADDRLEN 2 > > > > > +#define MT6370_REG_MAXADDR 0x1FF > > > > > > > > These two more logically to have near to other _REG_* definitions above. ... > > You lost me. Namespace has a meaning, i.e. grouping items of a kind. > > In your proposal I don't see that. If REG_MAXADDR and REG_ADDRLEN are > > _not_ of the _REG_ kind as per above, why do they have this namespace > > in the first place? > oh... Sorry, I just got the wrong meaning > maybe it should be revised like this, right?? I don't know. I am not an author of the code, I do not have access (and don't want to) to the hardware datasheets, all up to you. From the style perspective below looks good. > --- > #define MT6370_REG_DEV_INFO0x100 > #define MT6370_REG_CHG_IRQ10x1C0 > #define MT6370_REG_CHG_MASK1 0x1E0 > #define MT6370_REG_MAXADDR 0x1FF // Move it to here > > #define MT6370_VENID_MASK GENMASK(7, 4) > > #define MT6370_NUM_IRQREGS 16 > #define MT6370_USBC_I2CADDR0x4E > > #define MT6370_MAX_ADDRLEN 2// Rename -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 13/13] video: backlight: mt6370: Add MediaTek MT6370 support
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu wrote:
>
> From: ChiaEn Wu
>
> MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
> with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
> driver, display bias voltage supply, one general purpose LDO, and the
> USB Type-C & PD controller complies with the latest USB Type-C and PD
> standards.
>
> This adds support for MediaTek MT6370 Backlight driver. It's commonly used
Read Submitting Patches, please!
(In this case, find "This patch" in the above mentioned document, read
and act accordingly)
> to drive the display WLED. There are 4 channels inside, and each channel
> supports up to 30mA of current capability with 2048 current steps in
> exponential or linear mapping curves.
...
> + brightness_val[1] = (brightness - 1) >>
> fls(MT6370_BL_DIM2_MASK);
(see below)
...
> + /*
> +* To make MT6372 using 14 bits to control the brightness
> +* backward compatible with 11 bits brightness control
> +* (like MT6370 and MT6371 do), we left shift the value
> +* and pad with 1 to remaining bits. Hence, the MT6372's
to the remaining
> +* backlight brightness will be almost the same as MT6370's
> +* and MT6371's.
> +*/
> + if (priv->vid_type == MT6370_VID_6372) {
> + brightness_val[0] <<= MT6370_BL_DIM2_6372_SHIFT;
> + brightness_val[0] |= MT6370_BL_DUMMY_6372_MASK;
> + }
Nice! Why not...
...
> + gpiod_set_value(priv->enable_gpio, brightness ? 1 : 0);
!!brightness will do as well.
...
> + brightness = brightness_val[1] << fls(MT6370_BL_DIM2_MASK);
> + val |= prop_val << (ffs(MT6370_BL_PWM_HYS_SEL_MASK) - 1);
> + val |= ovp_uV << (ffs(MT6370_BL_OVP_SEL_MASK) - 1);
> + val |= ocp_uA << (ffs(MT6370_BL_OC_SEL_MASK) - 1);
> + val = prop_val << (ffs(MT6370_BL_CH_MASK) - 1);
...to use respective _SHIFTs in all these?
...
> + priv->enable_gpio = devm_gpiod_get_optional(dev, "enable",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(priv->enable_gpio))
> + dev_err(dev, "Failed to get 'enable' gpio\n");
What does this mean? Shouldn't be
return dev_err_probe()?
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 12/13] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu wrote: Forgot to add a couple of things... ... > +#define MT6370_ITORCH_MIN_UA 25000 > +#define MT6370_ITORCH_STEP_UA 12500 > +#define MT6370_ITORCH_MAX_UA 40 > +#define MT6370_ITORCH_DOUBLE_MAX_UA80 > +#define MT6370_ISTRB_MIN_UA5 > +#define MT6370_ISTRB_STEP_UA 12500 > +#define MT6370_ISTRB_MAX_UA150 > +#define MT6370_ISTRB_DOUBLE_MAX_UA 300 Perhaps _uA would be better and consistent across your series regarding current units. ... > + /* > +* For the flash to turn on/off, need to wait HW ramping up/down time we need > +* 5ms/500us to prevent the unexpected problem. > +*/ > + if (!prev && curr) > + usleep_range(5000, 6000); > + else if (prev && !curr) > + udelay(500); This still remains unanswered, why in the first place we allow switching, and a busy loop in the other place? -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 12/13] leds: flash: mt6370: Add MediaTek MT6370 flashlight support
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu wrote:
>
> From: Alice Chen
>
> The MediaTek MT6370 is a highly-integrated smart power management IC,
> which includes a single cell Li-Ion/Li-Polymer switching battery
> charger, a USB Type-C & Power Delivery (PD) controller, dual Flash
> LED current sources, a RGB LED driver, a backlight WLED driver,
> a display bias driver and a general LDO for portable devices.
>
> The Flash LED in MT6370 has 2 channels and support torch/strobe mode.
> Add the support of MT6370 FLASH LED.
>
> Signed-off-by: Alice Chen
This SoB chain is wrong. Prioritize and read Submitting Patches!
...
> +static int mt6370_torch_brightness_set(struct led_classdev *lcdev,
> + enum led_brightness level)
> +{
> + struct mt6370_led *led = to_mt6370_led(lcdev, flash.led_cdev);
> + struct mt6370_priv *priv = led->priv;
> + u32 led_enable_mask = (led->led_no == MT6370_LED_JOINT) ?
> + MT6370_FLCSEN_MASK_ALL :
> + MT6370_FLCSEN_MASK(led->led_no);
> + u32 enable_mask = MT6370_TORCHEN_MASK | led_enable_mask;
> + u32 val = level ? led_enable_mask : 0;
> + u32 prev = priv->fled_torch_used, curr;
Here and in the other functions with similar variables it seems you
never use prev after assigning curr.
Just define a single variable and use it accordingly.
> + int ret, i;
> +
> + mutex_lock(>lock);
> +
> + /*
> +* Only one set of flash control logic,
> +* use the flag to avoid strobe is currently used.
> +*/
> + if (priv->fled_strobe_used) {
> + dev_warn(lcdev->dev, "Please disable strobe first [%d]\n",
> +priv->fled_strobe_used);
> + ret = -EBUSY;
> + goto unlock;
> + }
> +
> + if (level)
> + curr = prev | BIT(led->led_no);
> + else
> + curr = prev & ~BIT(led->led_no);
> +
> + if (curr)
> + val |= MT6370_TORCHEN_MASK;
> +
> + if (level) {
> + level -= 1;
> + if (led->led_no == MT6370_LED_JOINT) {
> + int flevel[MT6370_MAX_LEDS];
> +
> + flevel[0] = level / 2;
> + flevel[1] = level - flevel[0];
> + for (i = 0; i < MT6370_MAX_LEDS; i++) {
> + ret = regmap_update_bits(priv->regmap,
> + MT6370_REG_FLEDITOR(i),
> + MT6370_ITORCH_MASK,
> flevel[i]);
> + if (ret)
> + goto unlock;
> + }
> + } else {
> + ret = regmap_update_bits(priv->regmap,
> + MT6370_REG_FLEDITOR(led->led_no),
> + MT6370_ITORCH_MASK, level);
> + if (ret)
> + goto unlock;
> + }
> + }
> +
> + ret = regmap_update_bits(priv->regmap, MT6370_REG_FLEDEN,
> +enable_mask, val);
> + if (ret)
> + goto unlock;
> +
> + priv->fled_torch_used = curr;
> +
> +unlock:
> + mutex_unlock(>lock);
> + return ret;
> +}
...
> + struct v4l2_flash_config v4l2_config = {0};
0 is not needed.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 07/13] mfd: mt6370: Add MediaTek MT6370 support
On Mon, Jul 25, 2022 at 10:30 AM ChiaEn Wu wrote: > On Mon, Jul 25, 2022 at 4:00 PM Andy Shevchenko > wrote: ... > > > +#define MT6370_REG_DEV_INFO0x100 > > > +#define MT6370_REG_CHG_IRQ10x1C0 > > > +#define MT6370_REG_CHG_MASK1 0x1E0 > > > + > > > +#define MT6370_VENID_MASK GENMASK(7, 4) > > > + > > > +#define MT6370_NUM_IRQREGS 16 > > > +#define MT6370_USBC_I2CADDR0x4E > > > > > +#define MT6370_REG_ADDRLEN 2 > > > +#define MT6370_REG_MAXADDR 0x1FF > > > > These two more logically to have near to other _REG_* definitions above. > > Hi Andy, > Thanks for your review. > Do you mean that we should move '#define MT6370_USBC_I2CADDR' and > '#define MT6370_REG_MAXADDR' after the line '#define > MT6370_REG_CHG_MASK1'? > --- > #define MT6370_REG_DEV_INFO0x100 > #define MT6370_REG_CHG_IRQ10x1C0 > #define MT6370_REG_CHG_MASK1 0x1E0 > #define MT6370_USBC_I2CADDR0x4E > #define MT6370_REG_MAXADDR 0x1FF > > #define MT6370_VENID_MASK GENMASK(7, 4) > > #define MT6370_NUM_IRQREGS 16 > #define MT6370_REG_ADDRLEN 2 > --- > Like this? You lost me. Namespace has a meaning, i.e. grouping items of a kind. In your proposal I don't see that. If REG_MAXADDR and REG_ADDRLEN are _not_ of the _REG_ kind as per above, why do they have this namespace in the first place? -- With Best Regards, Andy Shevchenko
Re: [PATCH v6 11/13] leds: rgb: mt6370: Add MediaTek MT6370 current sink type LED Indicator support
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu wrote:
>
> From: ChiYuan Huang
(Note this and read below)
>
> The MediaTek MT6370 is a highly-integrated smart power management IC,
> which includes a single cell Li-Ion/Li-Polymer switching battery
> charger, a USB Type-C & Power Delivery (PD) controller, dual
> Flash LED current sources, a RGB LED driver, a backlight WLED driver,
> a display bias driver and a general LDO for portable devices.
>
> In MediaTek MT6370, there are four channel current-sink RGB LEDs that
> support hardware pattern for constant current, PWM, and breath mode.
> Isink4 channel can also be used as a CHG_VIN power good indicator.
> Signed-off-by: Alice Chen
> Signed-off-by: ChiYuan Huang
In conjunction with above what SoB of Alice means?
You really need to take your time and (re-)read
https://www.kernel.org/doc/html/latest/process/submitting-patches.html.
...
> + * Author: Alice Chen
> + * Author: ChiYuan Huang
Would
* Authors:
*Name_of_Author 1
*Name_of_Author 2
work for you?
...
> +struct mt6370_led {
> + union {
> + struct led_classdev isink;
> + struct led_classdev_mc mc;
> + };
Where is the field that makes union work?
> + struct mt6370_priv *priv;
> + u32 default_state;
> + u32 index;
> +};
...
> +static int mt6370_gen_breath_pattern(struct mt6370_priv *priv,
> +struct led_pattern *pattern, u32 len,
> +u8 *pattern_val, u32 val_len)
> +{
> + enum mt6370_led_ranges sel_range;
> + struct led_pattern *curr;
> + unsigned int sel;
> + u8 val[P_MAX_PATTERNS / 2] = {};
> + int i;
> +
> + if (len < P_MAX_PATTERNS && val_len < P_MAX_PATTERNS / 2)
> + return -EINVAL;
> +
> + /*
> +* Pattern list
> +* tr1: byte 0, b'[7: 4]
> +* tr2: byte 0, b'[3: 0]
> +* tf1: byte 1, b'[7: 4]
> +* tf2: byte 1, b'[3: 0]
> +* ton: byte 2, b'[7: 4]
> +* toff: byte 2, b'[3: 0]
> +*/
> + for (i = 0; i < P_MAX_PATTERNS; i++) {
> + curr = pattern + i;
> +
> + sel_range = i == P_LED_TOFF ? R_LED_TOFF : R_LED_TRFON;
> +
> + linear_range_get_selector_within(priv->ranges + sel_range,
> +curr->delta_t, );
> +
> + val[i / 2] |= sel << (4 * ((i + 1) % 2));
> + }
> +
> + memcpy(pattern_val, val, 3);
Isn't it something like put_unaligned_be24()/put_unaligned_le24()?
> + return 0;
> +}
...
> +static inline int mt6370_mc_pattern_clear(struct led_classdev *lcdev)
> +{
> + struct led_classdev_mc *mccdev = lcdev_to_mccdev(lcdev);
> + struct mt6370_led *led = container_of(mccdev, struct mt6370_led, mc);
> + struct mt6370_priv *priv = led->priv;
> + struct mc_subled *subled;
> + int i, ret = 0;
Redundant assignment.
> + mutex_lock(>priv->lock);
> +
> + for (i = 0; i < mccdev->num_colors; i++) {
> + subled = mccdev->subled_info + i;
> +
> + ret = mt6370_set_led_mode(priv, subled->channel,
> + MT6370_LED_REG_MODE);
> + if (ret)
> + break;
> + }
> +
> + mutex_unlock(>priv->lock);
> +
> + return ret;
> +}
...
> + if (!fwnode_property_read_string(init_data->fwnode, "default-state",
> +_str)) {
ret = fwnode_...(...);
if (!ret)
> + ret = match_string(states, ARRAY_SIZE(states), stat_str);
> + if (ret < 0)
> + ret = STATE_OFF;
> +
> + led->default_state = ret;
> + }
...
> + int i = 0, ret;
unsigned int i = 0;
int ret;
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v6 10/13] power: supply: mt6370: Add MediaTek MT6370 charger driver
On Fri, Jul 22, 2022 at 12:25 PM ChiaEn Wu wrote:
>
> From: ChiaEn Wu
>
> MediaTek MT6370 is a SubPMIC consisting of a single cell battery charger
> with ADC monitoring, RGB LEDs, dual channel flashlight, WLED backlight
> driver, display bias voltage supply, one general purpose LDO, and the
> USB Type-C & PD controller complies with the latest USB Type-C and PD
> standards.
>
> This adds MediaTek MT6370 Charger driver support. The charger module
> of MT6370 supports High-Accuracy Voltage/Current Regulation,
> Average Input Current Regulation, Battery Temperature Sensing,
> Over-Temperature Protection, DPDM Detection for BC1.2.
...
> +static inline void mt6370_chg_enable_irq(struct mt6370_priv *priv,
> +const char *irq_name, bool en)
> +{
> + int irq_num;
> + struct platform_device *pdev = to_platform_device(priv->dev);
> +
> + irq_num = platform_get_irq_byname(pdev, irq_name);
Every time the IRQ is not found you will get an error message printed here.
1) Is IRQ optional?
2) If not, can't you do validation only once?
> + if (irq_num < 0)
> + return;
> +
> + if (en)
> + enable_irq(irq_num);
> + else
> + disable_irq_nosync(irq_num);
> +}
...
> + ret = mt6370_chg_field_set(priv, F_USBCHGEN, 0);
> + if (ret < 0) {
> + ret = mt6370_chg_field_set(priv, F_ICHG, 90);
> + if (ret < 0) {
> + ret = mt6370_chg_field_set(priv, F_IINLMTSEL, 3);
> + if (ret < 0) {
Do all these ' < 0' parts make sense?
(Not only these cases, but in many in the entire driver)
...
> + /* Check in otg mode or not */
OTG
...
> + ret = devm_request_threaded_irq(priv->dev, ret, NULL,
> + mt6370_chg_irqs[i].handler,
> + IRQF_TRIGGER_FALLING,
> + dev_name(priv->dev), priv);
> +
Redundant blank line.
> + if (ret < 0)
> + return dev_err_probe(priv->dev, ret,
> + "Failed to request irq %s\n",
> +mt6370_chg_irqs[i].name);
> + }
--
With Best Regards,
Andy Shevchenko
