Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Mon, May 20, 2024 at 07:51:24PM +0530, Devarsh Thakkar wrote: > On 20/05/24 17:52, Andy Shevchenko wrote: > > On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: > >> On 18/05/24 01:44, Andy Shevchenko wrote: > >>> On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > > Yes, and one should follow IWYU principle and not cargo cult or whatever > > arbitrary lists. > > Agreed. > >>>> +#include > >>> > >>> + math.h // obviously > >>> + module.h > >>> > >>>> +#include > >>> > >>> + types.h > >> > >> All the above headers are already included as part of kernel.h > > > > Yes, that's why you should not use "proxy" headers. > > Have you read the top comment in the kernel.h? > > Yes, it says it is not recommended to include this inside another header file. > Although here we are adding it inside c file, but I can still try avoid it and > include only the required headers instead of kernel.h as you recommended. Right, but the first sentence there is "This header has combined a lot of unrelated to each other stuff." Can you explain how you use in your code all that unrelated stuff? For example, how do you use *trace_*() calls? Or maybe might_*() calls? or anything else that is directly provided by kernel.h? Besides IWYU principle above, it's good to have a justification for each inclusion the C file has. I believe there is no a such in _this_ case. -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Mon, May 20, 2024 at 05:11:18PM +0530, Devarsh Thakkar wrote: > On 18/05/24 01:44, Andy Shevchenko wrote: > > On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: [..] > >> +#include > >> +#include > > > >> +#include > > > > Do you know why this header is included? > > It includes all the other required headers (including those you mentioned > below), and header list is probably copied from other files in same directory. > But it does suffice the requirements as I have verified the compilation. Yes, and one should follow IWYU principle and not cargo cult or whatever arbitrary lists. > >> +#include > > > > + math.h // obviously > > + module.h > > > >> +#include > > > > + types.h > > All the above headers are already included as part of kernel.h Yes, that's why you should not use "proxy" headers. Have you read the top comment in the kernel.h? -- With Best Regards, Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 01:53:47PM -0700, Daniel Latypov wrote:
> On Fri, May 17, 2024 at 1:14 PM Andy Shevchenko
> wrote:
...
> > > [devarsht: Rebase to 6.9 and change license to GPL]
> >
> > I'm not sure that you may change license. It needs the author's
> > confirmation.
>
> Checking, this is referring to the MODULE_LICENSE, which used to be
> MODULE_LICENSE("GPL v2");
>
> and is now
> MODULE_LICENSE("GPL");
>
> If checkpatch is suggesting that now, then changing it sounds good to me.
In this case I agree on the change as it's purely syntax and not semantic.
> > > ---
> > > Changes since v6:
> > > * Rebase to linux-next, change license to GPL as suggested by checkpatch.
> >
> > Note, checkpatch.pl is not false positives free. Be careful
> > with what it suggests.
> >
> > > +#include
> > > +#include
> >
> > > +#include
> >
> > Do you know why this header is included?
>
> I think I had put it in the original before a lot of the work you did
> to split things out of kernel.h.
> I haven't had time to look apply this patch series locally yet, but
> I'd be pretty sure we can remove it without anything breaking.
Briefly looking at the code I even not sure it was needed before, but maybe
I missed something, in any case, please remove / replace it.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v8 07/10] lib: add basic KUnit test for lib/math
On Fri, May 17, 2024 at 11:06:07PM +0530, Devarsh Thakkar wrote: > From: Daniel Latypov > > Add basic test coverage for files that don't require any config options: > * part of math.h (what seem to be the most commonly used macros) > * gcd.c > * lcm.c > * int_sqrt.c > * reciprocal_div.c > (Ignored int_pow.c since it's a simple textbook algorithm.) > > These tests aren't particularly interesting, but they > * provide short and simple examples of parameterized tests > * provide a place to add tests for any new files in this dir > * are written so adding new test cases to cover edge cases should be > easy > * looking at code coverage, we hit all the branches in the .c files ... > [devarsht: Rebase to 6.9 and change license to GPL] I'm not sure that you may change license. It needs the author's confirmation. > --- > Changes since v6: > * Rebase to linux-next, change license to GPL as suggested by checkpatch. Note, checkpatch.pl is not false positives free. Be careful with what it suggests. > +#include > +#include > +#include Do you know why this header is included? > +#include + math.h // obviously + module.h > +#include + types.h ... Other than above, LGTM. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Mon, May 13, 2024 at 06:34:19PM +0530, Devarsh Thakkar wrote: > On 13/05/24 17:55, Andy Shevchenko wrote: > > On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: > >> On 13/05/24 14:29, Andy Shevchenko wrote: > >>> On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > >>>> On 10/05/24 20:45, Jani Nikula wrote: [...] > > - align naming (with the existing round*() macros) > > I think round_closest_up/round_closest_down align already and inspired by the > existing naming convention used for round*() and DIV_ROUND_CLOSEST() macros in > math.h as explained below (copied from my previous reply [1]) > > "Coming back to naming, this is as per existing convention used for naming > round_up, round_down (notice the `_` being used for macros working with pow of > 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer > to be nearest to specified value)" > > But do let me know if you have any other suggestions for naming? Just make sure that semantically the naming is aligned, that's it. If you think it's already done that way, fine! -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Mon, May 13, 2024 at 04:55:58PM +0530, Devarsh Thakkar wrote: > On 13/05/24 14:29, Andy Shevchenko wrote: > > On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > >> On 10/05/24 20:45, Jani Nikula wrote: [...] > >>> Moreover, I think the naming of round_up() and round_down() should have > >>> reflected the fact that they operate on powers of 2. It's unfortunate > >>> that the difference to roundup() and rounddown() is just the underscore! > >>> That's just a trap. > >>> > >>> So let's perhaps not repeat the same with round_closest_up() and > >>> round_closest_down()? > >> > >> Yes the naming is inspired by existing macros i.e. round_up, round_down > >> (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which > >> round the divided value to closest value) and there are already a lot of > >> users for these API's : > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc > >> 7304261 74775 > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc > >> 2261293 22194 > >> > >> linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST > >> drivers | wc > >>12077461 111822 > > > > Side note, discover `git grep ...`: it's much much faster on Git index, > > than classic one on a working copy. > > > >> so I thought to align with existing naming convention assuming > >> developers are already familiar with this. > >> > >> But if a wider consensus is to go with a newer naming convention then I > >> am open to it, although a challenge there would be to keep it short. For > >> e.g. this one is already 3 words, if we go with more explicit > >> "round_closest_up_pow_2" it looks quite long in my opinion :) . > > > > You need properly name the macros. Again, round_up() / roundup() and > > roundup_pow_of_two() are three _different_ macros, and it's not clear > > why you can't use one of them in your case. > > I can't use any of these because these macros either round up or round down, > whereas I want to round to closest value for the argument specified by the > user, be it achieved either by rounding up or rounding down depending upon > whichever makes the answer closer to the user-specified argument. > > To make it clear, I have already included the examples in the macro > description [2], copying it here, maybe I can put the same examples in the > commit message too to avoid confusions : > > round_closest_up(17, 4) = 16 > round_closest_up(15, 4) = 16 > round_closest_up(14, 4) = 16 > > round_closest_down(17, 4) = 16 > round_closest_down(15, 4) = 16 > round_closest_down(14, 4) = 12 > > Coming back to naming, this is as per existing convention used for naming > round_up, round_down (notice the `_` being used for macros working with pow of > 2) and DIV_ROUND_CLOSEST (notice the work `closest` used to specify the answer > to be nearest to specified value). Naming is a bit subjective, but I > personally don't think it is a good idea to go away with the existing naming > convention or go with longer names. > > > The patch that changes those to a new one are doubtful to begin with. > > I.e. need a careful review on the arithmetics side of the change > > including HW capabilities of handling "closest" results. > > This is already tested from my side, in-fact I have posted some of the results > in cover-letter with these macros [1] : > > Regarding hardware capabilities, it uses existing round_up, round_down macros > underneath which are optimized to handle pow of 2 after modifying the user > provided argument using addition/subtraction and division, so I don't think it > should generally a problem with the hardware. > And I see other macros DIV_ROUND_CLOSEST [3] already using similar operations > i.e. addition/subtraction and division so don't think it should be a problem > to keep similar other macros in the same file. Okay, thank you for elaborating. So, what I would expect in the new version of the series is that: - align naming (with the existing round*() macros) - add examples into commit message of the math.h patch - add test cases (you need to create lib/math_kunit.c for that) - elaborate in the commit message of the IPU3 change what you posted above (some kind of a summary) > [1]: > https://gist.github.com/devarsht/de6f5142f678bb1a5338abfd9f814abd#file-v7_jpeg_encoder_crop_validation-L204 > [2]: https://lore.kernel.org/all/20240509183952.4064331-1-devar...@ti.com/ > [3]: https://elixir.bootlin.com/linux/latest/source/include/linux/math.h#L86 -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Sat, May 11, 2024 at 11:11:14PM +0530, Devarsh Thakkar wrote: > On 10/05/24 20:45, Jani Nikula wrote: [...] > > Moreover, I think the naming of round_up() and round_down() should have > > reflected the fact that they operate on powers of 2. It's unfortunate > > that the difference to roundup() and rounddown() is just the underscore! > > That's just a trap. > > > > So let's perhaps not repeat the same with round_closest_up() and > > round_closest_down()? > > Yes the naming is inspired by existing macros i.e. round_up, round_down > (which round up/down to next pow of 2) and DIV_ROUND_CLOSEST (which > round the divided value to closest value) and there are already a lot of > users for these API's : > > linux-next git:(heads/next-20240509) ✗ grep -nr round_up drivers | wc > 7304261 74775 > > linux-next git:(heads/next-20240509) ✗ grep -nr round_down drivers | wc > 2261293 22194 > > linux-next git:(heads/next-20240509) ✗ grep -nr DIV_ROUND_CLOSEST > drivers | wc >12077461 111822 Side note, discover `git grep ...`: it's much much faster on Git index, than classic one on a working copy. > so I thought to align with existing naming convention assuming > developers are already familiar with this. > > But if a wider consensus is to go with a newer naming convention then I > am open to it, although a challenge there would be to keep it short. For > e.g. this one is already 3 words, if we go with more explicit > "round_closest_up_pow_2" it looks quite long in my opinion :) . You need properly name the macros. Again, round_up() / roundup() and roundup_pow_of_two() are three _different_ macros, and it's not clear why you can't use one of them in your case. The patch that changes those to a new one are doubtful to begin with. I.e. need a careful review on the arithmetics side of the change including HW capabilities of handling "closest" results. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Sun, May 12, 2024 at 07:46:58AM +0300, Alexey Dobriyan wrote: > I think > > roundup(x, 1 << n) Since it's about power-of-two, round_up() is better. > is more readable. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 06:16:42PM +0300, Laurent Pinchart wrote: > On Fri, May 10, 2024 at 06:03:52PM +0300, Andy Shevchenko wrote: > > On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > > > Use generic macro round_closest_up for rounding to nearest multiple > > > instead > > > > round_closest_up() > > > > We refer to the functions as func(). > > > > > of using local function. ... > > > @@ -565,7 +563,7 @@ static void find_best_seam(struct > > > ipu_image_convert_ctx *ctx, > > >* The closest input sample position that we could actually > > >* start the input tile at, 19.13 fixed point. > > >*/ > > > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > > > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > > > /* Convert 19.13 fixed point to integer */ > > > in_pos_rounded = in_pos_aligned / 8192U; > > > > Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() > > families of macros. What the semantic of 8192 is? > > The comment mentions 19.13 fixed point, so I assume that's the > fractional part of the integer. It doesn't seem related to pages. Okay, and align word in all those variable names? -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Fri, May 10, 2024 at 06:15:34PM +0300, Jani Nikula wrote: > On Fri, 10 May 2024, Andy Shevchenko > wrote: > > On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: > >> Add macros to round to nearest specified power of 2. > > > > This is not what they are doing. For the above we already have macros > > defined. > > > >> Two macros are added : > > > > (Yes, after I wrapped to comment this line looks better on its own, > > so whatever will be the first sentence, this line should be separated > > from.) > > > >> round_closest_up and round_closest_down which round up to nearest multiple > > > > round_closest_up() and round_closest_down() > > > > > >> of 2 with a preference to round up or round down respectively if there are > >> two possible nearest values to the given number. > > > > You should reformulate, because AFAICS there is the crucial difference > > from these and existing round_*_pow_of_two(). > > Moreover, I think the naming of round_up() and round_down() should have > reflected the fact that they operate on powers of 2. It's unfortunate > that the difference to roundup() and rounddown() is just the underscore! > That's just a trap. FYI: https://stackoverflow.com/questions/58139219/difference-of-align-and-round-up-macro-in-the-linux-kernel > So let's perhaps not repeat the same with round_closest_up() and > round_closest_down()? -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 7/8] media: imagination: Round to closest multiple for cropping region
On Fri, May 10, 2024 at 12:10:01AM +0530, Devarsh Thakkar wrote: > If neither of the flags to round down (V4L2_SEL_FLAG_LE) or round up > (V4L2_SEL_FLAG_GE) are specified by the user, then round to nearest > multiple of requested value while updating the crop rectangle coordinates. > > Use the rounding macro which gives preference to rounding down in case two > nearest values (high and low) are possible to raise the probability of > cropping rectangle falling inside the bound region. This is arguable. How do we know that the bigger range is supported? The safest side is to go smaller than bigger. -- With Best Regards, Andy Shevchenko
Re: [PATCH RESEND v7 0/8] Add V4L2 M2M Driver for E5010 JPEG Encoder
On Fri, May 10, 2024 at 01:56:03PM +0530, Devarsh Thakkar wrote: > Resending this V7 series to have proper linking of patches in the series > with cover-letter while doing git send-email. > > Original cover letter: > This adds support for V4L2 M2M based driver for E5010 JPEG Encoder > which is a stateful JPEG encoder from Imagination technologies > and is present in TI AM62A SoC. > > While adding support for it, following additional framework changes were > made: > - Moved reference quantization and huffman tables provided in >ITU-T-REC-T.81 to v4l2-jpeg.c as suggested in mailing list [1]. > - Add macros to round to closest integer (either higher or lower) while >rounding in order of 2. Nice, now it's chained! But all my comments to the previous mails are still applicable here. -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 8/8] gpu: ipu-v3: Use generic macro for rounding to nearest multiple
On Fri, May 10, 2024 at 12:10:10AM +0530, Devarsh Thakkar wrote: > Use generic macro round_closest_up for rounding to nearest multiple instead round_closest_up() We refer to the functions as func(). > of using local function. ... > @@ -565,7 +563,7 @@ static void find_best_seam(struct ipu_image_convert_ctx > *ctx, >* The closest input sample position that we could actually >* start the input tile at, 19.13 fixed point. >*/ > - in_pos_aligned = round_closest(in_pos, 8192U * in_align); > + in_pos_aligned = round_closest_up(in_pos, 8192U * in_align); > /* Convert 19.13 fixed point to integer */ > in_pos_rounded = in_pos_aligned / 8192U; Oh, these seems to be better to use either ALIGN*(), or PFN_*() / PAGE_*() families of macros. What the semantic of 8192 is? -- With Best Regards, Andy Shevchenko
Re: [PATCH v7 6/8] math.h Add macros to round to closest specified power of 2
On Fri, May 10, 2024 at 12:09:52AM +0530, Devarsh Thakkar wrote: > Add macros to round to nearest specified power of 2. This is not what they are doing. For the above we already have macros defined. > Two macros are added : (Yes, after I wrapped to comment this line looks better on its own, so whatever will be the first sentence, this line should be separated from.) > round_closest_up and round_closest_down which round up to nearest multiple round_closest_up() and round_closest_down() > of 2 with a preference to round up or round down respectively if there are > two possible nearest values to the given number. You should reformulate, because AFAICS there is the crucial difference from these and existing round_*_pow_of_two(). > This patch is inspired from the Mentor Graphics IPU driver [1] which uses > similar macro locally and which can be updated to use this generic macro > instead along with other drivers having similar requirements. > > [1]: > https://elixir.bootlin.com/linux/v6.8.9/source/drivers/gpu/ipu-v3/ipu-image-convert.c#L480 Instead of this, just add a patch to convert that driver to use this new macro. Besides, this paragraph should go to the comment/changelog area below. > Signed-off-by: Devarsh Thakkar > --- > V1->V6 (No change, patch introduced in V7) > --- -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, May 03, 2024 at 12:57:33PM +0800, Sui Jingfeng wrote:
> On 2024/5/3 01:28, Andy Shevchenko wrote:
> > On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote:
> > > On 2024/5/2 16:32, Andy Shevchenko wrote:
> > > > On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote:
> > > > > On 2024/4/30 22:13, Andy Shevchenko wrote:
> > > > > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
...
> > > > > > the former might be subdivided to "is it swnode backed or real
> > > > > > fwnode one?"
> > > > > >
> > > > > Yeah,
> > > > > On non-DT cases, it can be subdivided to swnode backed case and ACPI
> > > > > fwnode backed case.
> > > > >
> > > > >- For swnode backed case: the device_get_match_data() don't has a
> > > > > implemented backend.
> > > > >- For ACPI fwnode backed case: the device_get_match_data() has a
> > > > > implemented backend.
> > > > >
> > > > > But the driver has *neither* software node support
> > > > True.
> > > >
> > > > > nor ACPI support,
> > > > Not true.
> > > Why this is not true? Are you means that the panel-ilitek-ili9341 driver
> > > has ACPI support?
> > That's the idea (as far as I see the copy of the code from tinyDRM),
> > but broken over the copy'n'paste. This patch rectifies that to be
> > in align with the original code, which *does* support ACPI.
> >
> > > I'm asking because I don't see struct acpi_device_id related stuff in
> > > that source file,
> > > am I miss something?
> > Yes, you are. I leave it for you to research.
>
> After researching a few hours I still don't understand how does
> the panel-ilitek-ili9341 driver has the ACPI support and be able
> to ACPI probed when compiled as module.
>
> As far as I know, drivers that has the ACPI support *must* has the
> .acpi_match_table hooked, so that be able to be probed when the
> driver is compiled as a module.
No, and this is the thing. Hint: there is a glue code to reuse compatible
strings from OF, that's why dependency to OF prevents *some* systems from
being able to use that. But it's easy to fix by enabling OF in the
configuration, however the ID tables are orthogonal to the environment.
That's why all those ACPI_PTR() and of_match_ptr() are design mistakes
that are going to be removed eventually (the work is ongoing, btw,
as well as killing specific *_device_get_match_data() calls).
> For example, see commit 75a1b44a54bd9 ("spi: tegra210-quad: add acpi support")
> to get a feel what a SPI device with *real* ACPI support looks like.
If under *real* you assume the allocated _HID in use, yes, that's how it's
done. But there is the other tricky way of achieving similar effect w/o
allocating a new / custom ACPI _HID.
Hint: it's all documented in kernel under firmware-guide/acpi/.
> I have double checked the panel-ilitek-ili9341 driver, it doesn't
> has acpi_match_table hooked, which means that this driver won't
> even be able probed. And probed as pure SPI device still out of
> the scope of "correct use of device property APIs". Because SPI
> device specific method don't belong to the device property API.
> I don't really know what's we are missing, but we already intend
> to let it go, thanks.
>
> > > > So, slow down and take your time to get into the code and understand
> > > > how it works.
> > > >
> > > > > so that the rotation property can not get and ili9341_dpi_probe()
> > > > > will fails.
> > > > > So in total, this is not a 100% correct use of device property APIs.
> > > > >
> > > > > But I'm fine that if you want to leave(ignore) those less frequent
> > > > > use cases temporarily,
> > > > > there may have programmers want to post patches, to complete the
> > > > > missing in the future.
> > > > >
> > > > > So, there do have some gains on non-DT cases.
> > > > >
> > > > >- As you make it be able to compiled on X86 with the
> > > > > drm-misc-defconfig.
> > > > >- You cleanup the code up (at least patch 2 in this series is no
> > > > > obvious problem).
> > > > >- You allow people to modprobe it, and maybe half right and half
> > > > > undefined.
> > > > >
> > > > > But you do helps moving something forward, so congratulations for the
> > > > > wake up.
--
With Best Regards,
Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, May 03, 2024 at 01:28:26AM +0800, Sui Jingfeng wrote: > On 2024/5/2 15:34, Neil Armstrong wrote: > > On 30/04/2024 11:34, Maxime Ripard wrote: > > > On Tue, Apr 30, 2024 at 12:54:39AM +0800, Sui Jingfeng wrote: > > > > On 2024/4/29 19:55, Maxime Ripard wrote: > > > > > On Sat, Apr 27, 2024 at 01:57:46PM +0800, Sui Jingfeng wrote: > > > > > > On 2024/4/26 14:23, Maxime Ripard wrote: > > > > > > > On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote: > > > > > > > > On 2024/4/26 03:10, Andy Shevchenko wrote: > > > > > > > > > On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: > > > > > > > > > > On 2024/4/25 22:26, Andy Shevchenko wrote: ... > > > > > > > > > > > It seems driver missed the point of proper use of device > > > > > > > > > > > property APIs. Correct this by updating headers and > > > > > > > > > > > calls respectively. > > > > > > > > > > You are using the 'seems' here exactly saying that you are > > > > > > > > > > not 100% sure. > > > > > > > > > > Please allow me to tell you the truth: This patch again has > > > > > > > > > > ZERO effect. It fix nothing. And this patch is has the > > > > > > > > > > risks to be wrong. > > > > > > > > > Huh?! Really, stop commenting the stuff you do not understand. > > > > > > > > I'm actually a professional display drivers developer at the > > > > > > > > downstream in the past, despite my contribution to upstream is > > > > > > > > less. But I believe that all panel driver developers know what > > > > > > > > I'm talking about. So please have take a look at my replies. > > > > > > > Most of the interactions you had in this series has been uncalled > > > > > > > for. You might be against a patch, but there's no need to go to > > > > > > > such length. > > > > > > > > > > > > > > As far as I'm concerned, this patch is fine to me in itself, and > > > > > > > I don't see anything that would prevent us from merging it. > > > > > > No one is preventing you, as long as don't misunderstanding what > > > > > > other people's technical replies intentionally. I'm just a usual > > > > > > and normal contributor, I hope the world will better than > > > > > > yesterday. > > > > > You should seriously consider your tone when replying then. > > > > > > > > > > > Saying such thing to me may not proper, I guess you may want to talk > > > > > > to peoples who has the push rights > > > > > I think you misunderstood me. My point was that your several rants > > > > > were uncalled for and aren't the kind of things we're doing here. > > > > > > > > > > I know very well how to get a patch merged, thanks. > > > > > > > > > > > just make sure it isn't a insult to the professionalism of drm > > > > > > bridge > > > > > > community itself though. > > > > > I'm not sure why you're bringing the bridge community or its > > > > > professionalism. It's a panel, not a bridge, and I never doubted the > > > > > professionalism of anyone. > > > > > > > > I means that the code itself could be adopted, as newer and younger > > > > programmer (like Andy) need to be encouraged to contribute. > > > > > > Andy has thousands of commits in Linux. He's *very* far from being a new > > > contributor. > > > > > > > I express no obvious objections, just hints him that something else > > > > probably should also be taken into consideration as well. > > > > > > That might be what you wanted to express, but you definitely didn't > > > express it that way. > > > > > > > On the other hand, we probably should allow other people participate in > > > > discussion so that it is sufficient discussed and ensure that it won't > > > > be reverted by someone in the future for some reasons. Backing to out > > > > case happens here, we may need to move things forward. Therefore, it > > > > definitely deserve to have a t
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, May 03, 2024 at 12:25:17AM +0800, Sui Jingfeng wrote: > On 2024/5/2 16:32, Andy Shevchenko wrote: > > On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote: > > > On 2024/4/30 22:13, Andy Shevchenko wrote: > > > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote: ... > > > > the former might be subdivided to "is it swnode backed or real fwnode > > > > one?" > > > > > > > Yeah, > > > On non-DT cases, it can be subdivided to swnode backed case and ACPI > > > fwnode backed case. > > > > > > - For swnode backed case: the device_get_match_data() don't has a > > > implemented backend. > > > - For ACPI fwnode backed case: the device_get_match_data() has a > > > implemented backend. > > > > > > But the driver has *neither* software node support > > True. > > > > > nor ACPI support, > > Not true. > > Why this is not true? Are you means that the panel-ilitek-ili9341 driver has > ACPI support? That's the idea (as far as I see the copy of the code from tinyDRM), but broken over the copy'n'paste. This patch rectifies that to be in align with the original code, which *does* support ACPI. > I'm asking because I don't see struct acpi_device_id related stuff in that > source file, > am I miss something? Yes, you are. I leave it for you to research. > > So, slow down and take your time to get into the code and understand how it > > works. > > > > > so that the rotation property can not get and ili9341_dpi_probe() will > > > fails. > > > So in total, this is not a 100% correct use of device property APIs. > > > > > > But I'm fine that if you want to leave(ignore) those less frequent use > > > cases temporarily, > > > there may have programmers want to post patches, to complete the missing > > > in the future. > > > > > > So, there do have some gains on non-DT cases. > > > > > > - As you make it be able to compiled on X86 with the drm-misc-defconfig. > > > - You cleanup the code up (at least patch 2 in this series is no > > > obvious problem). > > > - You allow people to modprobe it, and maybe half right and half > > > undefined. > > > > > > But you do helps moving something forward, so congratulations for the > > > wake up. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes
On Thu, May 02, 2024 at 09:43:42AM +0200, Neil Armstrong wrote: > Hi, > > On Thu, 25 Apr 2024 17:26:16 +0300, Andy Shevchenko wrote: > > A few obvious fixes to the driver. > > > > Andy Shevchenko (3): > > drm/panel: ili9341: Correct use of device property APIs > > drm/panel: ili9341: Respect deferred probe > > drm/panel: ili9341: Use predefined error codes > > > > [...] > > Thanks, Applied to https://gitlab.freedesktop.org/drm/misc/kernel.git > (drm-misc-fixes) > > [1/3] drm/panel: ili9341: Correct use of device property APIs > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/d43cd48ef1791801c61a54fade4a88d294dedf77 > [2/3] drm/panel: ili9341: Respect deferred probe > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/740fc1e0509be3f7e2207e89125b06119ed62943 > [3/3] drm/panel: ili9341: Use predefined error codes > > https://gitlab.freedesktop.org/drm/misc/kernel/-/commit/da85f0aaa9f21999753b01d45c0343f885a8f905 Thank you, Neil, appreciated! -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Thu, May 02, 2024 at 09:34:17AM +0200, Neil Armstrong wrote: > On 30/04/2024 11:34, Maxime Ripard wrote: ... > Anyway since the rant is finished I'll land the patches. Thank you all for the review and discussion! -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Wed, May 01, 2024 at 12:27:14AM +0800, Sui Jingfeng wrote: > On 2024/4/30 22:13, Andy Shevchenko wrote: > > On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote: ... > > the former might be subdivided to "is it swnode backed or real fwnode one?" > > > Yeah, > On non-DT cases, it can be subdivided to swnode backed case and ACPI fwnode > backed case. > > - For swnode backed case: the device_get_match_data() don't has a > implemented backend. > - For ACPI fwnode backed case: the device_get_match_data() has a implemented > backend. > > But the driver has *neither* software node support True. > nor ACPI support, Not true. So, slow down and take your time to get into the code and understand how it works. > so that the rotation property can not get and ili9341_dpi_probe() will fails. > So in total, this is not a 100% correct use of device property APIs. > > But I'm fine that if you want to leave(ignore) those less frequent use cases > temporarily, > there may have programmers want to post patches, to complete the missing in > the future. > > So, there do have some gains on non-DT cases. > > - As you make it be able to compiled on X86 with the drm-misc-defconfig. > - You cleanup the code up (at least patch 2 in this series is no obvious > problem). > - You allow people to modprobe it, and maybe half right and half undefined. > > But you do helps moving something forward, so congratulations for the wake up. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 04:43:18AM +0800, Sui Jingfeng wrote:
> On 2024/4/26 03:10, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/25 22:26, Andy Shevchenko wrote:
> > > > It seems driver missed the point of proper use of device property APIs.
> > > > Correct this by updating headers and calls respectively.
> > > You are using the 'seems' here exactly saying that you are not 100% sure.
To add here, "seems" is used to show that I have no knowledge on what was
the idea behind this implementation by the original author. Plus my knowledge
in the firmware node / device property APIs and use cases in Linux kernel.
> > > Please allow me to tell you the truth: This patch again has ZERO effect.
> > > It fix nothing. And this patch is has the risks to be wrong.
> > Huh?! Really, stop commenting the stuff you do not understand.
>
> I'm actually a professional display drivers developer at the downstream
> in the past, despite my contribution to upstream is less. But I believe
> that all panel driver developers know what I'm talking about. So please
> have take a look at my replies.
Okay.
> > > Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path
> > > is DT dependent.
> > >
> > > First of all, the devm_of_find_backlight() is called in
> > > ili9341_dbi_probe()
> > > under *non-DT* environment, devm_of_find_backlight() is just a just a
> > > no-op and will return NULL. NULL is not an error code, so
> > > ili9341_dbi_probe()
> > > won't rage quit. But the several side effect is that the backlight will
> > > NOT works at all.
> > Is it a problem?
>
> Yes, it is.
>
> The core problem is that the driver you are modifying has *implicit*
> *dependency* on DT.
> The implicit dependency is due to the calling of devm_of_find_backlight().
> This function
> is a no-op under non-DT systems.
Okay.
> Therefore, before the devm_of_find_backlight() and
> the device_get_match_data() function can truly DT independent.
True for the first part, not true for the second.
> Removing the "OF" dependency just let the tigers run out from the jail.
>
> It is not really meant to targeting at you, but I thinks, all of drm_panel
> drivers
> that has the devm_of_find_backlight() invoked will suffer such concerns.
> In short, the reason is that the *implicit* *dependency* populates and
> the undefined behavior gets triggered.
Still no problem statement. My hardware works nicely on non-DT environment.
(And since it's Arduino-based one, I assume it will work on DT environments
the very same way.)
> I'm sure you know that device_get_match_data() is same with
> of_device_get_match_data()
> for DT based systems. For non DT based systems, device_get_match_data() is
> just *undefined*
> Note that ACPI is not in the scope of the discussion here, as all of the drm
> bridges and
> panels driver under drivers/gpu/drm/ hasn't the ACPI support yet.
This patch shows exactly how to bring back the ACPI support to one of them
(as it's done for tinyDRM cases).
> Therefore, at present,
> it safe to say that device_get_match_data() is *undefined* under no-DT
> environment.
This is not true.
> Removing the "OF" dependency hints to us that it allows the driver to be
> probed as a
> pure SPI device under non DT systems. When device_get_match_data() is called,
> it returns
> NULL to us now. As a result, the drm driver being modified will tears down.
>
> See bellow code snippet extracted frompanel-ilitek-ili9341.c:
>
>
> ```
> ili->conf = of_device_get_match_data(dev);
> if (!ili->conf) {
> dev_err(dev, "missing device configuration\n");
> return -ENODEV;
> }
> ```
>
> > > It is actually considered as fatal bug for *panels* if the backlight of
> > > it is not light up, at least the brightness of *won't* be able to adjust.
> > > What's worse, if there is no sane platform setup code at the firmware
> > > or boot loader stage to set a proper initial state. The screen is complete
> > > dark. Even though the itself panel is refreshing framebuffers, it can not
> > > be seen by human's eye. Simple because of no backlight.
> > Can you imagine that I may have different hardware that considered
> > this is non-fatal error?
> >
> Yes, I can imagine.
>
> I believe you have the hardware which make you patch correct to run
> in 99.9% of all cases. But as long as there one bug happened, you patch
> are going to be blamed.
>
> Because its your patch that open the door, b
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 05:13:43AM +0800, Sui Jingfeng wrote:
> On 2024/4/26 03:12, Andy Shevchenko wrote:
> > On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote:
> > > On 2024/4/26 02:08, Sui Jingfeng wrote:
...
> > Are you speaking to yourself? I'm totally lost.
> >
> > Please, if you want to give a constructive feedback, try to understand
> > the topic from different aspects and then clearly express it.
>
> OK,
>
> The previous email analysis the non-DT cases exhaustively, this email intend
> to
> demonstrate the more frequently use case.
>
> That is, in the *DT('OF')* based systems,
> device_get_match_data() is completely equivalent to
> of_device_get_match_data().
> So the net results of applying this patch are "no gains and no lost".
This is not true. It's only part of the cases, i.e. DT. So, I assume you meant
"So the net results of applying this patch are "no gains and no lost" in DT
case".
> Things will become clear if we divide the whole problem into two cases(DT and
> non-DT)
> to discuss, that's it. That's all I can tell.
Not really. non-DT cases can also be divided to "fwnode backed or not", and
the former might be subdivided to "is it swnode backed or real fwnode one?"
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
On Mon, Apr 29, 2024 at 01:39:06PM +0200, Maxime Ripard wrote: > On Thu, Apr 25, 2024 at 06:04:50PM +0300, Andy Shevchenko wrote: > > On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote: > > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote: > > > > First of all, the driver was introduced when it was already > > > > two drivers available for Ilitek 9341 panels. > > > > > > > > Second, the most recent (fourth!) driver has incorporated this one > > > > and hence, when enabled, it covers the provided functionality. > > > > > > > > Taking into account the above, remove duplicative driver and make > > > > maintenance and support eaiser for everybody. > > > > > > > > Also see discussion [1] for details about Ilitek 9341 duplication > > > > code. > > > > > > > > Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1] > > > > Signed-off-by: Andy Shevchenko > > > > > > I think it should be the other way around and we should remove the > > > mipi-dbi handling from panel/panel-ilitek-ili9341.c > > > > Then please do it! I whining already for a few years about this. > > I have neither the hardware nor the interest to do so. Seems it looks > like you have plenty of the latter at least, I'm sure you'll find some > time to tackle this. Hmm... Since the use of Arduino part in panel IliTek 9341 is clarified in this thread, I won't use that, but I have no time to clean up the mess in DRM in the nearest future, sorry. And TBH it seems you, guys, know much better what you want. FYI: The drivers/gpu/drm/tiny/mi0283qt.c works for me (the plenty of the HW you referred to). TL;DR: consider this as a (bug/feature/cleanup) report. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 02:53:22AM +0800, Sui Jingfeng wrote: > On 2024/4/26 02:08, Sui Jingfeng wrote: Are you speaking to yourself? I'm totally lost. Please, if you want to give a constructive feedback, try to understand the topic from different aspects and then clearly express it. -- With Best Regards, Andy Shevchenko
Re: [v1,1/3] drm/panel: ili9341: Correct use of device property APIs
On Fri, Apr 26, 2024 at 02:08:16AM +0800, Sui Jingfeng wrote: > On 2024/4/25 22:26, Andy Shevchenko wrote: > > It seems driver missed the point of proper use of device property APIs. > > Correct this by updating headers and calls respectively. > > You are using the 'seems' here exactly saying that you are not 100% sure. > > Please allow me to tell you the truth: This patch again has ZERO effect. > It fix nothing. And this patch is has the risks to be wrong. Huh?! Really, stop commenting the stuff you do not understand. > Simple because the "ili9341_probe() ---> ili9341_dbi_prob()" code path > is DT dependent. > > First of all, the devm_of_find_backlight() is called in ili9341_dbi_probe() > under *non-DT* environment, devm_of_find_backlight() is just a just a > no-op and will return NULL. NULL is not an error code, so ili9341_dbi_probe() > won't rage quit. But the several side effect is that the backlight will > NOT works at all. Is it a problem? > It is actually considered as fatal bug for *panels* if the backlight of > it is not light up, at least the brightness of *won't* be able to adjust. > What's worse, if there is no sane platform setup code at the firmware > or boot loader stage to set a proper initial state. The screen is complete > dark. Even though the itself panel is refreshing framebuffers, it can not > be seen by human's eye. Simple because of no backlight. Can you imagine that I may have different hardware that considered this is non-fatal error? > Second, the ili9341_dbi_probe() requires additional device properties to > be able to works very well on the rotation screen case. See the calling > of "device_property_read_u32(dev, "rotation", )" in > ili9341_dbi_probe() function. Yes, exactly, and how does it object the purpose of this patch? > Combine with those two factors, it is actually can conclude that the > panel-ilitek-ili9394 driver has the *implicit* dependency on 'OF'. > Removing the 'OF' dependency from its Kconfig just trigger the > leakage of such risks. What?! > My software node related patches can help to reduce part of the potential > risks, but it still need some extra work. And it is not landed yet. Your patch has nothing to do with this series. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] drm: fixed: Don't use "proxy" headers
On Mon, Apr 22, 2024 at 09:49:04PM +0300, Jani Nikula wrote: > On Mon, 22 Apr 2024, Andy Shevchenko > wrote: > > Update header inclusions to follow IWYU (Include What You Use) > > principle. > > > > Signed-off-by: Andy Shevchenko > > Assuming it builds, and nothing depends on other stuff from kernel.h via > drm_fixed.h, For the record, I have built-tested this via `make allyesconfig` on x86_64. > Reviewed-by: Jani Nikula Thank you! -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 0/3] drm/panel: ili9341: Obvious fixes
On Thu, Apr 25, 2024 at 05:26:16PM +0300, Andy Shevchenko wrote: > A few obvious fixes to the driver. Note, despite the desire of removal Adafruit support from this driver, the older (read: stable) kernels will need this. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
On Thu, Apr 25, 2024 at 04:58:06PM +0200, Maxime Ripard wrote: > Hi, > > On Thu, Apr 25, 2024 at 03:42:07PM +0300, Andy Shevchenko wrote: > > First of all, the driver was introduced when it was already > > two drivers available for Ilitek 9341 panels. > > > > Second, the most recent (fourth!) driver has incorporated this one > > and hence, when enabled, it covers the provided functionality. > > > > Taking into account the above, remove duplicative driver and make > > maintenance and support eaiser for everybody. > > > > Also see discussion [1] for details about Ilitek 9341 duplication > > code. > > > > Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1] > > Signed-off-by: Andy Shevchenko > > I think it should be the other way around and we should remove the > mipi-dbi handling from panel/panel-ilitek-ili9341.c Then please do it! I whining already for a few years about this. > It's basically two drivers glued together for no particular reason and > handling two very different use cases which just adds more complexity > than it needs to. > > And it's the only driver doing so afaik, so it's definitely not "least > surprise" compliant. -- With Best Regards, Andy Shevchenko
[PATCH v1 2/3] drm/panel: ili9341: Respect deferred probe
GPIO controller might not be available when driver is being probed.
There are plenty of reasons why, one of which is deferred probe.
Since GPIOs are optional, return any error code we got to the upper
layer, including deferred probe. With that in mind, use dev_err_probe()
in order to avoid spamming the logs.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 7584ddb0e441..24c74c56e564 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -715,11 +715,11 @@ static int ili9341_probe(struct spi_device *spi)
reset = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
if (IS_ERR(reset))
- dev_err(dev, "Failed to get gpio 'reset'\n");
+ return dev_err_probe(dev, PTR_ERR(reset), "Failed to get gpio
'reset'\n");
dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW);
if (IS_ERR(dc))
- dev_err(dev, "Failed to get gpio 'dc'\n");
+ return dev_err_probe(dev, PTR_ERR(dc), "Failed to get gpio
'dc'\n");
if (!strcmp(id->name, "sf-tc240t-9370-t"))
return ili9341_dpi_probe(spi, dc, reset);
--
2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 3/3] drm/panel: ili9341: Use predefined error codes
In one case the -1 is returned which is quite confusing code for
the wrong device ID, in another the ret is returning instead of
plain 0 that also confusing as readed may ask the possible meaning
of positive codes, which are never the case there. Convert both
to use explicit predefined error codes to make it clear what's going
on there.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 24c74c56e564..b933380b7eb7 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -422,7 +422,7 @@ static int ili9341_dpi_prepare(struct drm_panel *panel)
ili9341_dpi_init(ili);
- return ret;
+ return 0;
}
static int ili9341_dpi_enable(struct drm_panel *panel)
@@ -726,7 +726,7 @@ static int ili9341_probe(struct spi_device *spi)
else if (!strcmp(id->name, "yx240qv29"))
return ili9341_dbi_probe(spi, dc, reset);
- return -1;
+ return -ENODEV;
}
static void ili9341_remove(struct spi_device *spi)
--
2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 0/3] drm/panel: ili9341: Obvious fixes
A few obvious fixes to the driver. Andy Shevchenko (3): drm/panel: ili9341: Correct use of device property APIs drm/panel: ili9341: Respect deferred probe drm/panel: ili9341: Use predefined error codes drivers/gpu/drm/panel/Kconfig| 2 +- drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 13 +++-- 2 files changed, 8 insertions(+), 7 deletions(-) -- 2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 1/3] drm/panel: ili9341: Correct use of device property APIs
It seems driver missed the point of proper use of device property APIs.
Correct this by updating headers and calls respectively.
Fixes: 5a04227326b0 ("drm/panel: Add ilitek ili9341 panel driver")
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/panel/Kconfig| 2 +-
drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 5 +++--
2 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index e54f6f5604ed..2d451820 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -177,7 +177,7 @@ config DRM_PANEL_ILITEK_IL9322
config DRM_PANEL_ILITEK_ILI9341
tristate "Ilitek ILI9341 240x320 QVGA panels"
- depends on OF && SPI
+ depends on SPI
select DRM_KMS_HELPER
select DRM_GEM_DMA_HELPER
depends on BACKLIGHT_CLASS_DEVICE
diff --git a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
index 3574681891e8..7584ddb0e441 100644
--- a/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
+++ b/drivers/gpu/drm/panel/panel-ilitek-ili9341.c
@@ -22,8 +22,9 @@
#include
#include
#include
+#include
#include
-#include
+#include
#include
#include
@@ -691,7 +692,7 @@ static int ili9341_dpi_probe(struct spi_device *spi, struct
gpio_desc *dc,
* Every new incarnation of this display must have a unique
* data entry for the system in this driver.
*/
- ili->conf = of_device_get_match_data(dev);
+ ili->conf = device_get_match_data(dev);
if (!ili->conf) {
dev_err(dev, "missing device configuration\n");
return -ENODEV;
--
2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Thu, Apr 25, 2024 at 09:42:53PM +0800, Sui Jingfeng wrote: > On 2024/4/25 00:44, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote: > > > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: > > > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > > > > > wrote: > > > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > > > > > > > But let me throw an argument why this patch (or something > > > > > > > similar) looks > > > > > > > to be necessary. > > > > > > > > > > > > > > Both on DT and non-DT systems the kernel allows using the non-OF > > > > > > > based > > > > > > > matching. For the platform devices there is > > > > > > > platform_device_id-based > > > > > > > matching. > > > > > > > > > > > > > > Currently handling the data coming from such device_ids requires > > > > > > > using > > > > > > > special bits of code, e.g. > > > > > > > platform_get_device_id(pdev)->driver_data to > > > > > > > get the data from the platform_device_id. Having such codepaths > > > > > > > goes > > > > > > > against the goal of unifying DT and non-DT paths via generic > > > > > > > property / > > > > > > > fwnode code. > > > > > > > > > > > > > > As such, I support Sui's idea of being able to use > > > > > > > device_get_match_data > > > > > > > for non-DT, non-ACPI platform devices. > > > > > > I'm not sure I buy this. We have a special helpers based on the bus > > > > > > type to > > > > > > combine device_get_match_data() with the respective ID table > > > > > > crawling, see > > > > > > the SPI and I²C cases as the examples. > > > > > I was thinking that we might be able to deprecate these helpers and > > > > > always use device_get_match_data(). > > > > True, but that is orthogonal to swnode match_data support, right? > > > > There even was (still is?) a patch series to do something like a new > > > > member to struct device_driver (? don't remember) to achieve that. > > > Maybe the scenario was not properly described in the commit message, or > > > maybe I missed something. The usecase that I understood from the commit > > > message was to use instatiated i2c / spi devices, which means > > > i2c_device_id / spi_device_id. The commit message should describe why > > > the usecase requires using 'compatible' property and swnode. Ideally it > > > should describe how these devices are instantiated at the first place. > > Yep. I also do not clearly understand the use case and why we need to have > > a board file, because the swnodes all are about board files that we must not > > use for the new platforms. > > Would you like to tell us what's the 'board file'? > > I am asking because I can not understand those two words at all. > I'm really don't know what's the meanings of 'board file'. Hmm... This is very well established term meaning the hard coded platform description (you may consider that as "device tree" written in C inside the Linux kernel). There are plenty of legacy platforms still exist in the Linux kernel source tree, you may find examples, like (first comes to mind) arch/arm/mach-pxa/spitz.c. > Do you means that board file is something like the dts, or > somethings describe the stuff on the motherboard but outside > the CPU? > > Does the hardware IP core belong to the "board file"? > > Can we using more concrete vocabulary instead of the vague > vocabulary to communicate? Most of (I though 100% before this message) the Linux kernel developers _know_ this term, sorry that you maybe young enough :-) -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] drm/mipi-dbi: Add missing MODULE_DESCRIPTION()
The modpost script is not happy
WARNING: modpost: missing MODULE_DESCRIPTION() in
drivers/gpu/drm/drm_mipi_dbi.o
because there is a missing module description.
Add it to the module.
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/drm_mipi_dbi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index daac649aabdb..ee6fa8185b13 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1475,4 +1475,5 @@ EXPORT_SYMBOL(mipi_dbi_debugfs_init);
#endif
+MODULE_DESCRIPTION("MIPI Display Bus Interface (DBI) LCD controller support");
MODULE_LICENSE("GPL");
--
2.43.0.rc1.1336.g36b5255a03ac
[PATCH v1 1/1] drm/ili9341: Remove the duplicative driver
First of all, the driver was introduced when it was already
two drivers available for Ilitek 9341 panels.
Second, the most recent (fourth!) driver has incorporated this one
and hence, when enabled, it covers the provided functionality.
Taking into account the above, remove duplicative driver and make
maintenance and support eaiser for everybody.
Also see discussion [1] for details about Ilitek 9341 duplication
code.
Link: https://lore.kernel.org/r/zxm9pg-53v4s8...@smile.fi.intel.com [1]
Signed-off-by: Andy Shevchenko
---
drivers/gpu/drm/tiny/Kconfig | 13 --
drivers/gpu/drm/tiny/Makefile | 1 -
drivers/gpu/drm/tiny/ili9341.c | 253 -
3 files changed, 267 deletions(-)
delete mode 100644 drivers/gpu/drm/tiny/ili9341.c
diff --git a/drivers/gpu/drm/tiny/Kconfig b/drivers/gpu/drm/tiny/Kconfig
index f6889f649bc1..2ab07bd0bb44 100644
--- a/drivers/gpu/drm/tiny/Kconfig
+++ b/drivers/gpu/drm/tiny/Kconfig
@@ -134,19 +134,6 @@ config TINYDRM_ILI9225
If M is selected the module will be called ili9225.
-config TINYDRM_ILI9341
- tristate "DRM support for ILI9341 display panels"
- depends on DRM && SPI
- select DRM_KMS_HELPER
- select DRM_GEM_DMA_HELPER
- select DRM_MIPI_DBI
- select BACKLIGHT_CLASS_DEVICE
- help
- DRM driver for the following Ilitek ILI9341 panels:
- * YX240QV29-T 2.4" 240x320 TFT (Adafruit 2.4")
-
- If M is selected the module will be called ili9341.
-
config TINYDRM_ILI9486
tristate "DRM support for ILI9486 display panels"
depends on DRM && SPI
diff --git a/drivers/gpu/drm/tiny/Makefile b/drivers/gpu/drm/tiny/Makefile
index 76dde89a044b..37cc9b27e79d 100644
--- a/drivers/gpu/drm/tiny/Makefile
+++ b/drivers/gpu/drm/tiny/Makefile
@@ -10,7 +10,6 @@ obj-$(CONFIG_DRM_SIMPLEDRM) += simpledrm.o
obj-$(CONFIG_TINYDRM_HX8357D) += hx8357d.o
obj-$(CONFIG_TINYDRM_ILI9163) += ili9163.o
obj-$(CONFIG_TINYDRM_ILI9225) += ili9225.o
-obj-$(CONFIG_TINYDRM_ILI9341) += ili9341.o
obj-$(CONFIG_TINYDRM_ILI9486) += ili9486.o
obj-$(CONFIG_TINYDRM_MI0283QT) += mi0283qt.o
obj-$(CONFIG_TINYDRM_REPAPER) += repaper.o
diff --git a/drivers/gpu/drm/tiny/ili9341.c b/drivers/gpu/drm/tiny/ili9341.c
deleted file mode 100644
index 47b61c3bf145..
--- a/drivers/gpu/drm/tiny/ili9341.c
+++ /dev/null
@@ -1,253 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * DRM driver for Ilitek ILI9341 panels
- *
- * Copyright 2018 David Lechner
- *
- * Based on mi0283qt.c:
- * Copyright 2016 Noralf Trønnes
- */
-
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#define ILI9341_FRMCTR10xb1
-#define ILI9341_DISCTRL0xb6
-#define ILI9341_ETMOD 0xb7
-
-#define ILI9341_PWCTRL10xc0
-#define ILI9341_PWCTRL20xc1
-#define ILI9341_VMCTRL10xc5
-#define ILI9341_VMCTRL20xc7
-#define ILI9341_PWCTRLA0xcb
-#define ILI9341_PWCTRLB0xcf
-
-#define ILI9341_PGAMCTRL 0xe0
-#define ILI9341_NGAMCTRL 0xe1
-#define ILI9341_DTCTRLA0xe8
-#define ILI9341_DTCTRLB0xea
-#define ILI9341_PWRSEQ 0xed
-
-#define ILI9341_EN3GAM 0xf2
-#define ILI9341_PUMPCTRL 0xf7
-
-#define ILI9341_MADCTL_BGR BIT(3)
-#define ILI9341_MADCTL_MV BIT(5)
-#define ILI9341_MADCTL_MX BIT(6)
-#define ILI9341_MADCTL_MY BIT(7)
-
-static void yx240qv29_enable(struct drm_simple_display_pipe *pipe,
-struct drm_crtc_state *crtc_state,
-struct drm_plane_state *plane_state)
-{
- struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(pipe->crtc.dev);
- struct mipi_dbi *dbi = >dbi;
- u8 addr_mode;
- int ret, idx;
-
- if (!drm_dev_enter(pipe->crtc.dev, ))
- return;
-
- DRM_DEBUG_KMS("\n");
-
- ret = mipi_dbi_poweron_conditional_reset(dbidev);
- if (ret < 0)
- goto out_exit;
- if (ret == 1)
- goto out_enable;
-
- mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_OFF);
-
- mipi_dbi_command(dbi, ILI9341_PWCTRLB, 0x00, 0xc1, 0x30);
- mipi_dbi_command(dbi, ILI9341_PWRSEQ, 0x64, 0x03, 0x12, 0x81);
- mipi_dbi_command(dbi, ILI9341_DTCTRLA, 0x85, 0x00, 0x78);
- mipi_dbi_command(dbi, ILI9341_PWCTRLA, 0x39, 0x2c, 0x00, 0x34, 0x02);
- mipi_dbi_command(dbi, ILI9341_PUMPCTRL, 0x20);
- mipi_dbi_command(dbi, ILI9341_DTCTRLB, 0x00, 0x00);
-
- /* Power Control */
- mipi_dbi_command(dbi, ILI9341_PWCTRL1, 0x23);
- mipi_dbi_command(dbi, ILI9341_PWCTRL2, 0x10);
- /* VCOM */
- mipi_dbi_command(dbi, ILI9341
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Wed, Apr 24, 2024 at 07:34:54PM +0300, Dmitry Baryshkov wrote: > On Wed, Apr 24, 2024 at 05:52:03PM +0300, Andy Shevchenko wrote: > > On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > > > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > > > wrote: > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > > > > > But let me throw an argument why this patch (or something similar) > > > > > looks > > > > > to be necessary. > > > > > > > > > > Both on DT and non-DT systems the kernel allows using the non-OF based > > > > > matching. For the platform devices there is platform_device_id-based > > > > > matching. > > > > > > > > > > Currently handling the data coming from such device_ids requires using > > > > > special bits of code, e.g. platform_get_device_id(pdev)->driver_data > > > > > to > > > > > get the data from the platform_device_id. Having such codepaths goes > > > > > against the goal of unifying DT and non-DT paths via generic property > > > > > / > > > > > fwnode code. > > > > > > > > > > As such, I support Sui's idea of being able to use > > > > > device_get_match_data > > > > > for non-DT, non-ACPI platform devices. > > > > > > > > I'm not sure I buy this. We have a special helpers based on the bus > > > > type to > > > > combine device_get_match_data() with the respective ID table crawling, > > > > see > > > > the SPI and I²C cases as the examples. > > > > > > I was thinking that we might be able to deprecate these helpers and > > > always use device_get_match_data(). > > > > True, but that is orthogonal to swnode match_data support, right? > > There even was (still is?) a patch series to do something like a new > > member to struct device_driver (? don't remember) to achieve that. > > Maybe the scenario was not properly described in the commit message, or > maybe I missed something. The usecase that I understood from the commit > message was to use instatiated i2c / spi devices, which means > i2c_device_id / spi_device_id. The commit message should describe why > the usecase requires using 'compatible' property and swnode. Ideally it > should describe how these devices are instantiated at the first place. Yep. I also do not clearly understand the use case and why we need to have a board file, because the swnodes all are about board files that we must not use for the new platforms. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Wed, Apr 24, 2024 at 04:34:39PM +0300, Dmitry Baryshkov wrote: > On Wed, 24 Apr 2024 at 16:11, Andy Shevchenko > wrote: > > > > On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > > > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > > > But let me throw an argument why this patch (or something similar) looks > > > to be necessary. > > > > > > Both on DT and non-DT systems the kernel allows using the non-OF based > > > matching. For the platform devices there is platform_device_id-based > > > matching. > > > > > > Currently handling the data coming from such device_ids requires using > > > special bits of code, e.g. platform_get_device_id(pdev)->driver_data to > > > get the data from the platform_device_id. Having such codepaths goes > > > against the goal of unifying DT and non-DT paths via generic property / > > > fwnode code. > > > > > > As such, I support Sui's idea of being able to use device_get_match_data > > > for non-DT, non-ACPI platform devices. > > > > I'm not sure I buy this. We have a special helpers based on the bus type to > > combine device_get_match_data() with the respective ID table crawling, see > > the SPI and I²C cases as the examples. > > I was thinking that we might be able to deprecate these helpers and > always use device_get_match_data(). True, but that is orthogonal to swnode match_data support, right? There even was (still is?) a patch series to do something like a new member to struct device_driver (? don't remember) to achieve that. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Wed, Apr 24, 2024 at 12:37:16AM +0300, Dmitry Baryshkov wrote: > On Wed, Apr 24, 2024 at 12:49:18AM +0800, Sui Jingfeng wrote: > > On 2024/4/23 21:28, Andy Shevchenko wrote: > > > On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote: ... > But let me throw an argument why this patch (or something similar) looks > to be necessary. > > Both on DT and non-DT systems the kernel allows using the non-OF based > matching. For the platform devices there is platform_device_id-based > matching. > > Currently handling the data coming from such device_ids requires using > special bits of code, e.g. platform_get_device_id(pdev)->driver_data to > get the data from the platform_device_id. Having such codepaths goes > against the goal of unifying DT and non-DT paths via generic property / > fwnode code. > > As such, I support Sui's idea of being able to use device_get_match_data > for non-DT, non-ACPI platform devices. I'm not sure I buy this. We have a special helpers based on the bus type to combine device_get_match_data() with the respective ID table crawling, see the SPI and I²C cases as the examples. -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] fbtft: seps525: Don't use "proxy" headers
Update header inclusions to follow IWYU (Include What You Use) principle. Signed-off-by: Andy Shevchenko --- drivers/staging/fbtft/fb_seps525.c | 7 +++ 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/staging/fbtft/fb_seps525.c b/drivers/staging/fbtft/fb_seps525.c index 05882e2cde7f..46c257308b49 100644 --- a/drivers/staging/fbtft/fb_seps525.c +++ b/drivers/staging/fbtft/fb_seps525.c @@ -16,11 +16,10 @@ * GNU General Public License for more details. */ -#include -#include -#include -#include +#include #include +#include +#include #include "fbtft.h" -- 2.43.0.rc1.1336.g36b5255a03ac
Re: [PATCH v2] software node: Implement device_get_match_data fwnode callback
On Tue, Apr 23, 2024 at 12:46:58AM +0800, Sui Jingfeng wrote:
> Because the software node backend of the fwnode API framework lacks an
> implementation for the .device_get_match_data function callback. This
> makes it difficult to use(and/or test) a few drivers that originates
Missing space before opening parenthesis.
> from DT world on the non-DT platform.
>
> Implement the .device_get_match_data fwnode callback, device drivers or
> platform setup codes are expected to provide a string property, named as
> "compatible", the value of this software node string property is used to
> match against the compatible entries in the of_device_id table.
Yep and again, how is this related? If you want to test a driver originating
from DT, you would probably want to have a DT (overlay) to be provided.
> This also helps to keep the three backends of the fwnode API aligned as
> much as possible, which is a fundamential step to make device driver
> OF-independent truely possible.
>
> Fixes: ffb42e64561e ("drm/tiny/repaper: Make driver OF-independent")
> Fixes: 5703d6ae9573 ("drm/tiny/st7735r: Make driver OF-independent")
How is it a fix?
> Closes: https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
Yes, and then Reported-by, which is missing here.
> Cc: Andy Shevchenko
> Cc: Daniel Scally
> Cc: Heikki Krogerus
> Cc: Sakari Ailus
> Cc: Greg Kroah-Hartman
> Cc: "Rafael J. Wysocki"
Please, move these after the cutter '---' line (note you may have that line in
your local repo).
...
> +static const void *
> +software_node_get_match_data(const struct fwnode_handle *fwnode,
> + const struct device *dev)
> +{
> + struct swnode *swnode = to_swnode(fwnode);
> + const struct of_device_id *matches = dev->driver->of_match_table;
> + const char *val = NULL;
> + int ret;
> + ret = property_entry_read_string_array(swnode->node->properties,
> +"compatible", , 1);
And if there are more than one compatible provided?
> + if (ret < 0 || !val)
> + return NULL;
> + while (matches && matches->compatible[0]) {
First part of the conditional is invariant to the loop. Can be simply
matches = dev->driver->of_match_table;
if (!matches)
return NULL;
while (...)
> + if (!strcmp(matches->compatible, val))
> + return matches->data;
> +
> + matches++;
> + }
> +
> + return NULL;
> +}
--
With Best Regards,
Andy Shevchenko
[PATCH v1 1/1] drm: fixed: Don't use "proxy" headers
Update header inclusions to follow IWYU (Include What You Use)
principle.
Signed-off-by: Andy Shevchenko
---
include/drm/drm_fixed.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 81572d32db0c..387fb81d5b81 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -25,8 +25,9 @@
#ifndef DRM_FIXED_H
#define DRM_FIXED_H
-#include
#include
+#include
+#include
typedef union dfixed {
u32 full;
--
2.43.0.rc1.1336.g36b5255a03ac
Re: In kernel virtual HID devices (was Future handling of complex RGB devices on Linux v3)
On Mon, Mar 25, 2024 at 07:38:46PM +0100, Miguel Ojeda wrote: > On Mon, Mar 25, 2024 at 3:25 PM Hans de Goede wrote: > > > > +Cc: Bentiss, Jiri > > Cc'ing Andy and Geert as well who recently became the > maintainers/reviewers of auxdisplay, in case they are interested in > these threads (one of the initial solutions discussed in a past thread > a while ago was to extend auxdisplay). Without diving into this, just sharing my view on auxdisplay subsystem: I consider it _mostly_ (like lim->100% mathematically speaking) as for 7-segment and alike displays, not any comples RGB or so devices. If those devices are capable of representing characters/digits in similar way, we may export linedisp library for them to utilise. -- With Best Regards, Andy Shevchenko
Re: [PATCH] staging: fbtft: core: fix potential memory leak in fbtft_probe_common()
On Wed, Sep 28, 2022 at 02:23:01PM +0800, Jianglei Nie wrote:
> fbtft_probe_common() allocates a memory chunk for "info" with
> fbtft_framebuffer_alloc(). When "display->buswidth == 0" is true, the
> function returns without releasing the "info", which will lead to a
> memory leak.
>
> Fix it by calling fbtft_framebuffer_release() when "display->buswidth
> == 0" is true.
Fixes tag?
...
> if (display->buswidth == 0) {
> dev_err(dev, "buswidth is not set\n");
> + fbtft_framebuffer_release(info);
> return -EINVAL;
ret = dev_err_probe(dev, -EINVAL, "buswidth is not set\n");
goto out_release;
> }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Sat, Mar 23, 2024 at 02:30:08AM +0800, Sui Jingfeng wrote: > On 2024/3/23 02:16, Andy Shevchenko wrote: > > On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: > > > On 2024/3/23 02:05, Andy Shevchenko wrote: > > > >Besides that, the kernel project rule is "we do not add > > > > the dead (unused) code". > > > This rule is good an correct and I admit. > > > > > > But the problem is that it is chicken-and-egg problem, > > > it probably have at least two user now. > > Then show them! Convert in the same series and show that. > > I believe that Vladimir has show enough to you. I have read that thread, > I think Vladimit have explained very well. Let's continue there. I replied there just now. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Sat, Mar 23, 2024 at 02:12:14AM +0800, Sui Jingfeng wrote: > On 2024/3/23 02:05, Andy Shevchenko wrote: > > Besides that, the kernel project rule is "we do not add > > the dead (unused) code". > > This rule is good an correct and I admit. > > But the problem is that it is chicken-and-egg problem, > it probably have at least two user now. Then show them! Convert in the same series and show that. > it's possible that it will gain more users in the future. > > But if you reject everybody from now, then it is zero. As a no-user patch, yes, I reject this. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Sat, Mar 23, 2024 at 01:43:56AM +0800, Sui Jingfeng wrote:
> On 2024/3/23 00:14, Andy Shevchenko wrote:
> > On Fri, Mar 22, 2024 at 05:00:05PM +0800, Sui Jingfeng wrote:
> > > On 2024/3/21 04:28, Andy Shevchenko wrote:
...
> > > > > > > By replacing it with device_get_match_data() and creating a
> > > > > > > software
> > > > > > > graph that mimics the OF graph, everything else works fine,
> > > > > > > except that
> > > > > > > there isn't an out-of-box replacement for the
> > > > > > > of_device_get_match_data()
> > > > > > > function. Because the software node backend of the fwnode
> > > > > > > framework lacks
> > > > > > > an implementation for the device_get_match_data callback.
> > > > > > .device_get_match_data
> > > > > >
> > > > > > > Implement device_get_match_data fwnode callback fwnode callback
> > > > > > > to fill
> > > > > > .device_get_match_data
> > > > > OK, thanks a lot.
> > > > >
> > > > > > > this gap. Device drivers or platform setup codes are expected to
> > > > > > > provide
> > > > > > > a "compatible" string property. The value of this string property
> > > > > > > is used
> > > > > > > to match against the compatible entries in the of_device_id
> > > > > > > table. Which
> > > > > > > is consistent with the original usage style.
> > > > > > Why do you need to implement the graph in the board file?
> > > > > It can be inside the chip, there is no clear cut.\
> > > > Which chip? Flash memory / ROM or you meant something like FPGA here?
> > > > For the latter there is another discussion on how to use DT overlays
> > > > in ACPI-enabled environments for the FPGA configurations.
> > > There are some hardware resource or software entity is created on the
> > > driver runtime. But DT or DT overlays are compiled before device driver
> > > get loaded. GPIO-emulated-I2C is just an example, this is kind of driver
> > > level knowledge on the runtime. With the GPIO or programmable some
> > > hardware IP unit, device driver authors can change the connection
> > > relationship
> > > at their will at the runtime. While with DT, every thing has to be sure
> > > before the compile time.
> > >
> > > DT overlays can be a alternative solution, but this doesn't conflict with
> > > this patch. This patch won't assume how device drives go to use it, and
> > > allow device driver creating device instead enumerating by DT. In one
> > > word: "flexibility".
> > Software nodes in general for the device driver / platform quirks.
>
> The real problem is that we probably shouldn't make an assumption
> how does the user is going to use the infrastructure, right?
>
> You could say it is *mostly* for quirks or whatever, Like the
> ./drivers/i2c/busses/i2c-cht-wc.c. But software nodes *can* also
> be something else.
>
> Can we stop restricting its usage by limited understanding or someone
> personal judgement?
Please, try to research the topic before calling it 'personal judgement'.
59abd83672f7 ("drivers: base: Introducing software nodes to the firmware node
framework")
(Read the first paragraph carefully.)
Let's say it's not personal, it's by design. Extending this to cover more needs
a good justification. I do not see a such.
> A workaround or quirk may be enough for some corner usage. Vladimir is also
> encounter similar problem, right?
> > They are not designed for what you are talking about here.
>
> I have never hint anything about any real applications, the materials
> and/or talk given here is just for example purpose.
>
> What we are doing here is to keep the three back-ends aligned.
>
>
> > Consider using SSDT / DT overlays instead.
> >
> NAK,
>
> When developers are doing task 'A' , reviewers ask them to do task 'B'.
> And when developers doing task 'B', reviewers then recommend that the tool
> 'C' is a better alternative.
> ...
> ...
>
> This is not good.
>
>
> As I have read the lengthy thread in link [1] as you pointed to me.
>
> The boring coding review is just as the following scheme:
>
> 1) Asking details about what they do with software nodes impolitely.
> 2) Wasting time to talk about irreverent things by brute force.
> 3) Tell everybody that software nodes are not designed for what you
> application.
> 4) Recommending DT overlays or something else.
>
> Again, this is non-technical discussion, the time being wasting is not
> worthwhile.
> And the judgements being given is irrelevant to the *patch itself*.
The patch tries to tight the driver data to the device description provided by
a software node, which is 100% equivalent to the legacy board files which we
do NOT want to have. Besides that, the kernel project rule is "we do not add
the dead (unused) code".
I believe these two is quite enough to NAK patch.
You may come with a better explanation AND a user of this in the same series.
People at least can see your use case.
> [1] https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/
--
With Best Regards,
Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Fri, Mar 22, 2024 at 05:00:05PM +0800, Sui Jingfeng wrote: > On 2024/3/21 04:28, Andy Shevchenko wrote: ... > > > > > By replacing it with device_get_match_data() and creating a software > > > > > graph that mimics the OF graph, everything else works fine, except > > > > > that > > > > > there isn't an out-of-box replacement for the > > > > > of_device_get_match_data() > > > > > function. Because the software node backend of the fwnode framework > > > > > lacks > > > > > an implementation for the device_get_match_data callback. > > > > .device_get_match_data > > > > > > > > > Implement device_get_match_data fwnode callback fwnode callback to > > > > > fill > > > > .device_get_match_data > > > OK, thanks a lot. > > > > > > > > this gap. Device drivers or platform setup codes are expected to > > > > > provide > > > > > a "compatible" string property. The value of this string property is > > > > > used > > > > > to match against the compatible entries in the of_device_id table. > > > > > Which > > > > > is consistent with the original usage style. > > > > Why do you need to implement the graph in the board file? > > > It can be inside the chip, there is no clear cut.\ > > Which chip? Flash memory / ROM or you meant something like FPGA here? > > For the latter there is another discussion on how to use DT overlays > > in ACPI-enabled environments for the FPGA configurations. > > There are some hardware resource or software entity is created on the > driver runtime. But DT or DT overlays are compiled before device driver > get loaded. GPIO-emulated-I2C is just an example, this is kind of driver > level knowledge on the runtime. With the GPIO or programmable some > hardware IP unit, device driver authors can change the connection relationship > at their will at the runtime. While with DT, every thing has to be sure > before the compile time. > > DT overlays can be a alternative solution, but this doesn't conflict with > this patch. This patch won't assume how device drives go to use it, and > allow device driver creating device instead enumerating by DT. In one > word: "flexibility". Software nodes in general for the device driver / platform quirks. They are not designed for what you are talking about here. Consider using SSDT / DT overlays instead. -- With Best Regards, Andy Shevchenko
Re: [PATCH 2/5] drm/bridge: simple-bridge: Extend match support for non-DT based systems
On Tue, Jan 23, 2024 at 12:32:17AM +0800, Sui Jingfeng wrote: > Which is intended to be used on non-DT environment, where the simple-bridge > platform device is created by either the display controller driver side or > platform firmware subsystem. To avoid duplication and to keep consistent, > we choose to reuse the OF match tables. Because the potentional user may > not has a of_node attached, nor a ACPI match id. If this is the case, > a software node string property can be provide to fill the niche. ... > - sbridge->info = of_device_get_match_data(>dev); > + if (pdev->dev.of_node) > + sbridge->info = of_device_get_match_data(>dev); > + else > + sbridge->info = simple_bridge_get_match_data(>dev); This is wrong. Just use device_get_match_data() instead of of_ counter part. The rest, if required, has to be addressed elsewhere. So, formal NAK for the changes like above. -- With Best Regards, Andy Shevchenko
Re: [PATCH 5/5] drm-bridge: display-connector: Switch to use fwnode API
On Tue, Jan 23, 2024 at 03:20:26AM +0200, Laurent Pinchart wrote: > On Tue, Jan 23, 2024 at 12:32:20AM +0800, Sui Jingfeng wrote: ... > > conn->bridge.of_node = pdev->dev.of_node; > > + conn->bridge.fwnode = pdev->dev.fwnode; > > This goes in the right direction. Let's address the other drivers and > drop the OF-based calls in the same series :-) +1. BUT, please use device_set_node() instead of both lines. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
On Thu, Mar 21, 2024 at 03:22:05AM +0800, Sui Jingfeng wrote: > On 2024/3/20 18:39, Andy Shevchenko wrote: > > On Tue, Mar 19, 2024 at 07:42:22AM +0800, Sui Jingfeng wrote: > > > This makes it possible to support (and/or test) a few drivers that > > > originates from DT World on the x86-64 platform. Originally, those > > > drivers using the of_device_get_match_data() function to get match > > > data. For example, drivers/gpu/drm/bridge/simple-bridge.c and > > > drivers/gpu/drm/bridge/display-connector.c. Those drivers works very > > > well in the DT world, however, there is no counterpart to > > > of_device_get_match_data() when porting them to the x86 platform, > > > because x86 CPUs lack DT support. > > This is not true. > > > > First of all, there is counter part that called device_get_match_data(). > > Are you means that the acpi_fwnode_device_get_match_data() implementation? > As the fwnode API framework has three backend: OF, ACPI, and software node. > If you are hinting me that the acpi backend has the .device_get_match_data > implemented. Then you are right. Yes, for all firmware property providers there is a callback. > > Second, there *is* DT support for the _selected_ x86 based platforms. > > Yeah, you maybe right again here. I guess you means that some special > hardware or platform may have a *limited* support? > > Can you pointed it out for study of learning purpose? Point to what? This arch/x86/kernel/devicetree.c ? > To speak precisely, there are some drm display bridges drivers are > lack of the DT support on X86. Those display bridges belong to the > device drivers catalogs. Do they support Device Tree? Do you want to enable them in ACPI environment? > OK, I will update my commit message at the next version if possible, > and try my best to describe the problem precisely. Please do. > > > By replacing it with device_get_match_data() and creating a software > > > graph that mimics the OF graph, everything else works fine, except that > > > there isn't an out-of-box replacement for the of_device_get_match_data() > > > function. Because the software node backend of the fwnode framework lacks > > > an implementation for the device_get_match_data callback. > > .device_get_match_data > > > > > Implement device_get_match_data fwnode callback fwnode callback to fill > > .device_get_match_data > > OK, thanks a lot. > > > > this gap. Device drivers or platform setup codes are expected to provide > > > a "compatible" string property. The value of this string property is used > > > to match against the compatible entries in the of_device_id table. Which > > > is consistent with the original usage style. > > Why do you need to implement the graph in the board file? > > It can be inside the chip, there is no clear cut.\ Which chip? Flash memory / ROM or you meant something like FPGA here? For the latter there is another discussion on how to use DT overlays in ACPI-enabled environments for the FPGA configurations. > I means that > the graph(including fwnode graph, OF graph or swnode graph) can > be used at anywhere. The examples given here may lead you to > think it is board specific, but it is not limited to board specific. > > fwnode graph, OF graph and swnode graph, all of them are implements > of the graph. Its common that different hardware vendors bought the > some IP and has been integrated it into their SoC. So it can be inside > of the chip if you want *code sharing*. > > > Back to the patch itself, we want to keep the three backends aligned as much > as possible. Is this reasonable enough? Yes, but it misses details about board files approach. See also above. ... > > Have you seen this discussion? > > https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ > > I really didn't have seen that thread before this patch is sent, > I'm a graphic developer, I'm mainly focus on graphics domain. > > Previously, I have implemented similar functionality at the drivers > layer [1][2]. But as the instances grows, I realized there is a > risk to introducing *boilerplate*. So I send this patch. [1][2] can > be drop if this patch could be merged. > > [1] https://patchwork.freedesktop.org/patch/575414/?series=129040=1 > > [2] https://patchwork.freedesktop.org/patch/575411/?series=129040=1 > > > After a brief skim, I guess we encounter similar problems. Oops! > In a nutshell, there is a need to *emulation* on X86 platform, > to suit the need of device-driver coding style of DT world. What does "emulation" mean? Can you elaborate a bit? > Besides, at the swnode backend layer, we should not call > fwnode_property_read_string(), instead, we should usethe > property_entry_read_string_array() function. Because the > fwnode_property_read_string() is belong to upper layer. > While backend implementations should call functions from > bottom layer only. -- With Best Regards, Andy Shevchenko
Re: [PATCH] software node: Implement device_get_match_data fwnode callback
+Cc: Vladimir On Tue, Mar 19, 2024 at 07:42:22AM +0800, Sui Jingfeng wrote: > This makes it possible to support (and/or test) a few drivers that > originates from DT World on the x86-64 platform. Originally, those > drivers using the of_device_get_match_data() function to get match > data. For example, drivers/gpu/drm/bridge/simple-bridge.c and > drivers/gpu/drm/bridge/display-connector.c. Those drivers works very > well in the DT world, however, there is no counterpart to > of_device_get_match_data() when porting them to the x86 platform, > because x86 CPUs lack DT support. This is not true. First of all, there is counter part that called device_get_match_data(). Second, there *is* DT support for the _selected_ x86 based platforms. > By replacing it with device_get_match_data() and creating a software > graph that mimics the OF graph, everything else works fine, except that > there isn't an out-of-box replacement for the of_device_get_match_data() > function. Because the software node backend of the fwnode framework lacks > an implementation for the device_get_match_data callback. .device_get_match_data > Implement device_get_match_data fwnode callback fwnode callback to fill .device_get_match_data > this gap. Device drivers or platform setup codes are expected to provide > a "compatible" string property. The value of this string property is used > to match against the compatible entries in the of_device_id table. Which > is consistent with the original usage style. Why do you need to implement the graph in the board file? ... Have you seen this discussion? https://lore.kernel.org/lkml/20230223203713.hcse3mkbq3m6sogb@skbuf/ -- With Best Regards, Andy Shevchenko
Re: [PATCH] backlight: mp3309c: fix signedness bug in mp3309c_parse_fwnode()
On Sat, Mar 16, 2024 at 12:45:27PM +0300, Dan Carpenter wrote: > The "num_levels" variable is used to store error codes from > device_property_count_u32() so it needs to be signed. This doesn't > cause an issue at runtime because devm_kcalloc() won't allocate negative > sizes. However, it's still worth fixing. Agree. Reviewed-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
[PATCH v1 1/1] drm/gma500: Remove unused intel-mid.h
intel-mid.h is providing some core parts of the South Complex PM, which are usually are not used by individual drivers. In particular, this driver doesn't use it, so simply remove the unused header. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/gma500/oaktrail_lvds.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/gpu/drm/gma500/oaktrail_lvds.c b/drivers/gpu/drm/gma500/oaktrail_lvds.c index d974d0c60d2a..72191d6f0d06 100644 --- a/drivers/gpu/drm/gma500/oaktrail_lvds.c +++ b/drivers/gpu/drm/gma500/oaktrail_lvds.c @@ -11,8 +11,6 @@ #include #include -#include - #include #include #include -- 2.43.0.rc1.1.gbec44491f096
[PATCH v1 1/1] drm/msm/hdmi: Replace of_gpio.h by proper one
of_gpio.h is deprecated and subject to remove. The driver doesn't use it directly, replace it with what is really being used. Signed-off-by: Andy Shevchenko --- drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c index c8ebd75176bb..24abcb7254cc 100644 --- a/drivers/gpu/drm/msm/hdmi/hdmi.c +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c @@ -5,8 +5,8 @@ * Author: Rob Clark */ +#include #include -#include #include #include -- 2.43.0.rc1.1.gbec44491f096
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Thu, Feb 29, 2024 at 12:21:34PM -0600, Lucas De Marchi wrote: > On Thu, Feb 29, 2024 at 12:49:57PM +0200, Andy Shevchenko wrote: > > On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote: > > > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote: ... > > > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the > > > need to fork the __GENMASK() implementation on the 2 sides of the ifdef > > > since I think the GENMASK_INPUT_CHECK() should be the one covering the > > > input checks. However to make it common we'd need to solve 2 problems: > > > the casts and the sizeof. The sizeof can be passed as arg to > > > __GENMASK(), however the casts I think would need a __CAST_U8(x) > > > or the like and sprinkle it everywhere, which would hurt readability. > > > Not pretty. Or go back to the original submission and make it less > > > horrible :-/ > > > > I'm wondering if we can use _Generic() approach here. > > in assembly? Yes. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 28, 2024 at 05:39:21PM -0600, Lucas De Marchi wrote: > On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote: > > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote: > > > On Wed, Feb 21, 2024 at 11:04:22PM +0200, Andy Shevchenko wrote: > > > > On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote: > > > > > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi > > > > > wrote: ... > I build-tested this in x86-64, x86-32 and arm64. I didn't like much the > need to fork the __GENMASK() implementation on the 2 sides of the ifdef > since I think the GENMASK_INPUT_CHECK() should be the one covering the > input checks. However to make it common we'd need to solve 2 problems: > the casts and the sizeof. The sizeof can be passed as arg to > __GENMASK(), however the casts I think would need a __CAST_U8(x) > or the like and sprinkle it everywhere, which would hurt readability. > Not pretty. Or go back to the original submission and make it less > horrible :-/ I'm wondering if we can use _Generic() approach here. ... > > #define GENMASK_INPUT_CHECK(h, l) 0 > > +#define __GENMASK(t, h, l) \ > > + ((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h > > humn... this builds, but does it work if GENMASK_ULL() is used in > assembly? That BITS_PER_LONG does not match the type width. UL()/ULL() macros are not just for fun. -- With Best Regards, Andy Shevchenko
Re: [PATCH 1/2] devm-helpers: Add resource managed version of mutex init
Thu, Feb 22, 2024 at 03:58:37PM +0100, Marek Behún kirjoitti:
> A few drivers are doing resource-managed mutex initialization by
> implementing ad-hoc one-liner mutex dropping functions and using them
> with devm_add_action_or_reset(). Help drivers avoid these repeated
> one-liners by adding managed version of mutex initialization.
>
> Use the new function devm_mutex_init() in the following drivers:
> drivers/gpio/gpio-pisosr.c
> drivers/gpio/gpio-sim.c
> drivers/gpu/drm/xe/xe_hwmon.c
> drivers/hwmon/nzxt-smart2.c
> drivers/leds/leds-is31fl319x.c
> drivers/power/supply/mt6370-charger.c
> drivers/power/supply/rt9467-charger.c
Pardon me, but why?
https://lore.kernel.org/linux-leds/20231214173614.2820929-1-gnst...@salutedevices.com/
Can you cooperate, folks, instead of doing something independently?
> --- a/include/linux/devm-helpers.h
> +++ b/include/linux/devm-helpers.h
> @@ -24,6 +24,8 @@
> */
>
> #include
> +#include
> +#include
> #include
>
> static inline void devm_delayed_work_drop(void *res)
> @@ -76,4 +78,34 @@ static inline int devm_work_autocancel(struct device *dev,
> return devm_add_action(dev, devm_work_drop, w);
> }
>
> +static inline void devm_mutex_drop(void *res)
> +{
> + mutex_destroy(res);
> +}
> +
> +/**
> + * devm_mutex_init - Resource managed mutex initialization
> + * @dev: Device which lifetime mutex is bound to
> + * @lock:Mutex to be initialized (and automatically destroyed)
> + *
> + * Initialize mutex which is automatically destroyed when driver is detached.
> + * A few drivers initialize mutexes which they want destroyed before driver
> is
> + * detached, for debugging purposes.
> + * devm_mutex_init() can be used to omit the explicit mutex_destroy() call
> when
> + * driver is detached.
> + */
> +static inline int devm_mutex_init(struct device *dev, struct mutex *lock)
> +{
> + mutex_init(lock);
> +
> + /*
> + * mutex_destroy() is an empty function if CONFIG_DEBUG_MUTEXES is
> + * disabled. No need to allocate an action in that case.
> + */
> + if (IS_ENABLED(CONFIG_DEBUG_MUTEXES))
> + return devm_add_action_or_reset(dev, devm_mutex_drop, lock);
> + else
> + return 0;
> +}
Cc: George Stark
--
With Best Regards,
Andy Shevchenko
Re: Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Thu, Feb 22, 2024 at 06:49:59AM -0800, Yury Norov wrote: > On Wed, Feb 21, 2024 at 03:59:06PM -0600, Lucas De Marchi wrote: ... > +#define __GENMASK(t, h, l) \ > + ((~0 - (1 << (l)) + 1) & (~0 >> (BITS_PER_LONG - 1 - (h What's wrong on using the UL/ULL() macros? Also it would be really good to avoid bifurcation of the implementations of __GENMASK() for both cases. ... > -#define __GENMASK(h, l) \ > - (((~UL(0)) - (UL(1) << (l)) + 1) & \ > - (~UL(0) >> (BITS_PER_LONG - 1 - (h This at bare minimum can be left untouched for asm case, no? -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 21, 2024 at 11:06:10PM +0200, Andy Shevchenko wrote: > On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote: > > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov > > wrote: ... > > Excuse me, it seems a c from gitlab didn't work as expected. > > No problem, it's clear that the patch has not been properly tested and > obviously wrong. Has to be dropped. If somebody wants that, it will > be material for v6.10 cycle. ...which makes me think that bitmap(bitops) lack of assembly test case(s). -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 21, 2024 at 10:37:30PM +0200, Dmitry Baryshkov wrote: > On Wed, 21 Feb 2024 at 22:30, Dmitry Baryshkov > wrote: ... > Excuse me, it seems a c from gitlab didn't work as expected. No problem, it's clear that the patch has not been properly tested and obviously wrong. Has to be dropped. If somebody wants that, it will be material for v6.10 cycle. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Wed, Feb 21, 2024 at 10:30:02PM +0200, Dmitry Baryshkov wrote: > On Thu, 8 Feb 2024 at 09:45, Lucas De Marchi wrote: ... > > +#define BITS_PER_TYPE(type)(sizeof(type) * BITS_PER_BYTE) Can sizeof() be used in assembly? ... > > -#define __GENMASK(h, l) \ > > - (((~UL(0)) - (UL(1) << (l)) + 1) & \ > > -(~UL(0) >> (BITS_PER_LONG - 1 - (h > > -#define GENMASK(h, l) \ > > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l)) > > +#define __GENMASK(t, h, l) \ > > + (GENMASK_INPUT_CHECK(h, l) + \ > > +(((t)~0ULL - ((t)(1) << (l)) + 1) & \ > > +((t)~0ULL >> (BITS_PER_TYPE(t) - 1 - (h) Nevertheless, the use ~0ULL is not proper assembly, this broke initial implementation using UL() / ULL(). > > -#define __GENMASK_ULL(h, l) \ > > - (((~ULL(0)) - (ULL(1) << (l)) + 1) & \ > > -(~ULL(0) >> (BITS_PER_LONG_LONG - 1 - (h > > -#define GENMASK_ULL(h, l) \ > > - (GENMASK_INPUT_CHECK(h, l) + __GENMASK_ULL(h, l)) Ditto. > > +#define GENMASK(h, l) __GENMASK(unsigned long, h, l) > > +#define GENMASK_ULL(h, l) __GENMASK(unsigned long long, h, l) > > +#define GENMASK_U8(h, l) __GENMASK(u8, h, l) > > +#define GENMASK_U16(h, l) __GENMASK(u16, h, l) > > +#define GENMASK_U32(h, l) __GENMASK(u32, h, l) > > +#define GENMASK_U64(h, l) __GENMASK(u64, h, l) > > This breaks drm-tip on arm64 architecture: > > arch/arm64/kernel/entry-fpsimd.S: Assembler messages: > 465arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 466arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 467arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 468arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 469arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 470arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 471arch/arm64/kernel/entry-fpsimd.S:271: Error: found 'l', expected: ')' > 472arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 473arch/arm64/kernel/entry-fpsimd.S:271: Error: unexpected characters > following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned > long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned > long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)' > 474arch/arm64/kernel/entry-fpsimd.S:66: Info: macro invoked from here > 475arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 476arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 477arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 478arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 479arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 480arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 481arch/arm64/kernel/entry-fpsimd.S:282: Error: found 'l', expected: ')' > 482arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here > 483arch/arm64/kernel/entry-fpsimd.S:282: Error: unexpected characters > following instruction at operand 3 -- `bic x2,x1,(0+(((unsigned > long)~0ULL-((unsigned long)(1)<<(0))+1)&((unsigned > long)~0ULL>>((sizeof(unsigned long)*8)-1-(3)' > 484arch/arm64/kernel/entry-fpsimd.S:98: Info: macro invoked from here -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 01/10] backlight: Match backlight device against struct fb_info.bl_dev
On Wed, Feb 21, 2024 at 5:45 PM Thomas Zimmermann wrote: > Am 21.02.24 um 15:34 schrieb Andy Shevchenko: > > On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote: > >> Framebuffer drivers for devices with dedicated backlight are supposed > >> to set struct fb_info.bl_dev to the backlight's respective device. Use > >> the value to match backlight and framebuffer in the backlight core code. ... > >> if (!bd->ops) > >> goto out; > >> -if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info)) > >> +else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info)) > > What's the point of adding redundant 'else'? > > > >> goto out; > >> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT) > >> +else if (info->bl_dev && info->bl_dev != bd) > > Ditto. > > They group these tests into one single block of code; signaling that > these tests serve the same purpose. Commit message has nothing about this. Also if needed, it should be a separate change. > >> +goto out; > >> +#endif -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
On Wed, Feb 21, 2024 at 10:41:29AM +0100, Thomas Zimmermann wrote: > The driver sets struct fb_info.bl_dev to the correct backlight > device. Thus rely on the backlight core code to match backlight > and framebuffer devices, and remove the extra check_fb function > from struct backlight_ops. check_fb() Acked-by: Andy Shevchenko -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 01/10] backlight: Match backlight device against struct fb_info.bl_dev
On Wed, Feb 21, 2024 at 10:41:28AM +0100, Thomas Zimmermann wrote: > Framebuffer drivers for devices with dedicated backlight are supposed > to set struct fb_info.bl_dev to the backlight's respective device. Use > the value to match backlight and framebuffer in the backlight core code. ... > if (!bd->ops) > goto out; > - if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info)) > + else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info)) What's the point of adding redundant 'else'? > goto out; > +#if IS_ENABLED(CONFIG_FB_BACKLIGHT) > + else if (info->bl_dev && info->bl_dev != bd) Ditto. > + goto out; > +#endif -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
On Fri, Feb 09, 2024 at 07:50:52AM +, Lee Jones wrote: > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > On Thu, Feb 08, 2024 at 06:14:55PM +, Lee Jones wrote: > > > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > > > On Thu, Feb 08, 2024 at 05:39:46PM +, Lee Jones wrote: > > > > > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > > > > > On Thu, Feb 08, 2024 at 11:34:25AM +, Lee Jones wrote: > > > > > > > On Thu, 01 Feb 2024, Andy Shevchenko wrote: ... > > > > > > > > backlight: mp3309c: Utilise temporary variable for struct > > > > > > > > device > > > > > > > > > > > > (1) > > > > > > > > > > > > > Set no longer applies. Please rebase, thanks. > > > > > > > > > > > > I got a contradictory messages: > > > > > > 1) email that says that all had been applied; > > > > > > 2) this email (that tells the complete opposite); > > > > > > 3) the repository where the first two were applied. > > > > > > > > > > > > While you can amend your scripts, I think I need to rebase only the > > > > > > last patch > > > > > > > > > > This is what I assume happened: > > > > > > > > > > 1. Attempted to apply the set (as a set) > > > > > 2. 2 commits applied cleanly > > > > > 3. The final commit conflicted > > > > > > > > Which is really strange. I have just applied (with b4) on top of your > > > > changes > > > > and no complains so far. > > > > > > > > $ git am > > > > ./v2_20240201_andriy_shevchenko_backlight_mp3309c_allow_to_use_on_non_of_platforms.mbx > > > > Applying: backlight: mp3309c: Make use of device properties > > > > Applying: backlight: mp3309c: use dev_err_probe() instead of dev_err() > > > > Applying: backlight: mp3309c: Utilise temporary variable for struct > > > > device > > > > > > > > Can you show what b4 tells you about this? > > > > > > Fetching patch(es) > > > Analyzing 14 messages in the thread > > > Checking attestation on all messages, may take a moment... > > > --- > > > ✓ [PATCH v2 1/3] backlight: mp3309c: Make use of device properties > > > + Reviewed-by: Daniel Thompson (✓ > > > DKIM/linaro.org) > > > + Link: > > > https://lore.kernel.org/r/20240201151537.367218-2-andriy.shevche...@linux.intel.com > > > + Signed-off-by: Lee Jones > > > ✓ [PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of > > > dev_err() > > > + Tested-by: Flavio Suligoi (✗ DKIM/asem.it) > > > + Reviewed-by: Daniel Thompson (✓ > > > DKIM/linaro.org) > > > + Link: > > > https://lore.kernel.org/r/20240201151537.367218-3-andriy.shevche...@linux.intel.com > > > + Signed-off-by: Lee Jones > > > ✓ [PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for > > > struct device > > > + Link: > > > https://lore.kernel.org/r/20240201151537.367218-4-andriy.shevche...@linux.intel.com > > > + Signed-off-by: Lee Jones > > > --- > > > ✓ Signed: DKIM/intel.com (From: andriy.shevche...@linux.intel.com) > > > --- > > > Total patches: 3 > > > Prepared a fake commit range for 3-way merge (672ecc5199b5..d507b9f4c5b9) > > > --- > > > Link: > > > https://lore.kernel.org/r/20240201151537.367218-1-andriy.shevche...@linux.intel.com > > > Base: not specified > > > > > > Running through checkpatch.pl > > > total: 0 errors, 0 warnings, 103 lines checked > > > > > > "[PATCH v2 1/3] backlight: mp3309c: Make use of device properties" has no > > > obvious style problems and is ready for submission. > > > total: 0 errors, 0 warnings, 41 lines checked > > > > > > "[PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of" has > > > no obvious style problems and is ready for submission. > > > total: 0 errors, 0 warnings, 81 lines checked > > > > > > "[PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for" has > > > no obvious style problems and is ready for submission. > > > > > > Check the results (hit return to continue or Ctrl+c to exit) > > > > > > >
Re: [PATCH v3 1/3] bits: introduce fixed-type genmasks
On Thu, Feb 08, 2024 at 08:48:59PM +0100, Andi Shyti wrote: > On Wed, Feb 07, 2024 at 11:45:19PM -0800, Lucas De Marchi wrote: ... > > Signed-off-by: Yury Norov > > Signed-off-by: Lucas De Marchi > > Acked-by: Jani Nikula > > Lucas' SoB should be at the bottom here. In any case, nice patch: And it's at the bottom (among SoB lines), there is no violation with Submitting Patches. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 1/3] backlight: mp3309c: Make use of device properties
On Thu, Feb 08, 2024 at 08:24:46PM +0200, Andy Shevchenko wrote: > Convert the module to be property provider agnostic and allow > it to be used on non-OF platforms. > > Add mod_devicetable.h include. It seems some emails are missing, I just sent a 'resend v3' hopefully without missing parts. -- With Best Regards, Andy Shevchenko
[PATCH v3 2/3] backlight: mp3309c: use dev_err_probe() instead of dev_err()
Replace dev_err() with dev_err_probe().
This helps in simplifing code and standardizing the error output.
Tested-by: Flavio Suligoi
Reviewed-by: Daniel Thompson
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 397f35dafc5e..426e9f2356ad 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -208,10 +208,8 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
unsigned int num_levels, tmp_value;
struct device *dev = chip->dev;
- if (!dev_fwnode(dev)) {
- dev_err(dev, "failed to get firmware node\n");
- return -ENODEV;
- }
+ if (!dev_fwnode(dev))
+ return dev_err_probe(dev, -ENODEV, "failed to get firmware
node\n");
/*
* Dimming mode: the MP3309C provides two dimming control mode:
@@ -287,8 +285,7 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
if (ret)
pdata->default_brightness = pdata->max_brightness;
if (pdata->default_brightness > pdata->max_brightness) {
- dev_err(chip->dev,
- "default brightness exceeds max brightness\n");
+ dev_err_probe(dev, -ERANGE, "default brightness exceeds max
brightness\n");
pdata->default_brightness = pdata->max_brightness;
}
@@ -329,16 +326,15 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
static int mp3309c_probe(struct i2c_client *client)
{
- struct mp3309c_platform_data *pdata = dev_get_platdata(>dev);
+ struct device *dev = >dev;
+ struct mp3309c_platform_data *pdata = dev_get_platdata(dev);
struct mp3309c_chip *chip;
struct backlight_properties props;
struct pwm_state pwmstate;
int ret;
- if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
- dev_err(>dev, "failed to check i2c functionality\n");
- return -EOPNOTSUPP;
- }
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return dev_err_probe(dev, -EOPNOTSUPP, "failed to check i2c
functionality\n");
chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 3/3] backlight: mp3309c: Utilise temporary variable for struct device
We have a temporary variable to keep pointer to struct device.
Utilise it where it makes sense.
Reviewed-by: Daniel Thompson
Tested-by: Flavio Suligoi
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 30 --
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 426e9f2356ad..708c053d492c 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -222,10 +222,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
*/
pdata->dimming_mode = DIMMING_ANALOG_I2C;
if (device_property_present(dev, "pwms")) {
- chip->pwmd = devm_pwm_get(chip->dev, NULL);
+ chip->pwmd = devm_pwm_get(dev, NULL);
if (IS_ERR(chip->pwmd))
- return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
-"error getting pwm data\n");
+ return dev_err_probe(dev, PTR_ERR(chip->pwmd), "error
getting pwm data\n");
pdata->dimming_mode = DIMMING_PWM;
pwm_apply_args(chip->pwmd);
}
@@ -243,11 +242,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
num_levels = ANALOG_I2C_NUM_LEVELS;
/* Enable GPIO used in I2C dimming mode only */
- chip->enable_gpio = devm_gpiod_get(chip->dev, "enable",
- GPIOD_OUT_HIGH);
+ chip->enable_gpio = devm_gpiod_get(dev, "enable",
GPIOD_OUT_HIGH);
if (IS_ERR(chip->enable_gpio))
- return dev_err_probe(chip->dev,
-PTR_ERR(chip->enable_gpio),
+ return dev_err_probe(dev, PTR_ERR(chip->enable_gpio),
"error getting enable gpio\n");
} else {
/*
@@ -265,8 +262,7 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
}
/* Fill brightness levels array */
- pdata->levels = devm_kcalloc(chip->dev, num_levels,
-sizeof(*pdata->levels), GFP_KERNEL);
+ pdata->levels = devm_kcalloc(dev, num_levels, sizeof(*pdata->levels),
GFP_KERNEL);
if (!pdata->levels)
return -ENOMEM;
if (device_property_present(dev, "brightness-levels")) {
@@ -336,21 +332,21 @@ static int mp3309c_probe(struct i2c_client *client)
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
return dev_err_probe(dev, -EOPNOTSUPP, "failed to check i2c
functionality\n");
- chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;
- chip->dev = >dev;
+ chip->dev = dev;
chip->regmap = devm_regmap_init_i2c(client, _regmap);
if (IS_ERR(chip->regmap))
- return dev_err_probe(>dev, PTR_ERR(chip->regmap),
+ return dev_err_probe(dev, PTR_ERR(chip->regmap),
"failed to allocate register map\n");
i2c_set_clientdata(client, chip);
if (!pdata) {
- pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
@@ -367,11 +363,10 @@ static int mp3309c_probe(struct i2c_client *client)
props.type = BACKLIGHT_RAW;
props.power = FB_BLANK_UNBLANK;
props.fb_blank = FB_BLANK_UNBLANK;
- chip->bl = devm_backlight_device_register(chip->dev, "mp3309c",
- chip->dev, chip,
+ chip->bl = devm_backlight_device_register(dev, "mp3309c", dev, chip,
_bl_ops, );
if (IS_ERR(chip->bl))
- return dev_err_probe(chip->dev, PTR_ERR(chip->bl),
+ return dev_err_probe(dev, PTR_ERR(chip->bl),
"error registering backlight device\n");
/* In PWM dimming mode, enable pwm device */
@@ -383,8 +378,7 @@ static int mp3309c_probe(struct i2c_client *client)
pwmstate.enabled = true;
ret = pwm_apply_might_sleep(chip->pwmd, );
if (ret)
- return dev_err_probe(chip->dev, ret,
-"error setting pwm device\n");
+ return dev_err_probe(dev, ret, "error setting pwm
device\n");
}
chip->pdata->status = FIRST_POWER_ON;
--
2.43.0.rc1.1.gbec44491f096
[resend, PATCH v3 0/3] backlight: mp3309c: Allow to use on non-OF platforms
Allow to use driver on non-OF platforms and other cleanups. Changelog v3: - rebased on top of the last changes against this driver (Lee) - added tags to patch 2 (Daniel, Flavio) Changelog v2: - rename pm3309c_parse_dt_node() --> mp3309c_parse_fwnode() (Daniel) - add tags (Daniel, Flavio) - new patch 2 Andy Shevchenko (3): backlight: mp3309c: Make use of device properties backlight: mp3309c: use dev_err_probe() instead of dev_err() backlight: mp3309c: Utilise temporary variable for struct device drivers/video/backlight/mp3309c.c | 88 --- 1 file changed, 35 insertions(+), 53 deletions(-) -- 2.43.0.rc1.1.gbec44491f096
[PATCH v3 1/3] backlight: mp3309c: Make use of device properties
Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.
Add mod_devicetable.h include.
Tested-by: Flavio Suligoi
Reviewed-by: Daniel Thompson
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 44 +--
1 file changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index b0d9aef6942b..397f35dafc5e 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -15,6 +15,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -199,18 +201,15 @@ static const struct backlight_ops mp3309c_bl_ops = {
.update_status = mp3309c_bl_update_status,
};
-static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
-struct mp3309c_platform_data *pdata)
+static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
+ struct mp3309c_platform_data *pdata)
{
- struct device_node *node = chip->dev->of_node;
- struct property *prop_pwms;
- struct property *prop_levels = NULL;
- int length = 0;
int ret, i;
unsigned int num_levels, tmp_value;
+ struct device *dev = chip->dev;
- if (!node) {
- dev_err(chip->dev, "failed to get DT node\n");
+ if (!dev_fwnode(dev)) {
+ dev_err(dev, "failed to get firmware node\n");
return -ENODEV;
}
@@ -224,8 +223,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
* found in the backlight node, the mode switches to PWM mode.
*/
pdata->dimming_mode = DIMMING_ANALOG_I2C;
- prop_pwms = of_find_property(node, "pwms", );
- if (prop_pwms) {
+ if (device_property_present(dev, "pwms")) {
chip->pwmd = devm_pwm_get(chip->dev, NULL);
if (IS_ERR(chip->pwmd))
return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
@@ -257,11 +255,9 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
/*
* PWM control mode: check for brightness level in DT
*/
- prop_levels = of_find_property(node, "brightness-levels",
- );
- if (prop_levels) {
+ if (device_property_present(dev, "brightness-levels")) {
/* Read brightness levels from DT */
- num_levels = length / sizeof(u32);
+ num_levels = device_property_count_u32(dev,
"brightness-levels");
if (num_levels < 2)
return -EINVAL;
} else {
@@ -275,10 +271,9 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
sizeof(*pdata->levels), GFP_KERNEL);
if (!pdata->levels)
return -ENOMEM;
- if (prop_levels) {
- ret = of_property_read_u32_array(node, "brightness-levels",
-pdata->levels,
-num_levels);
+ if (device_property_present(dev, "brightness-levels")) {
+ ret = device_property_read_u32_array(dev, "brightness-levels",
+pdata->levels, num_levels);
if (ret < 0)
return ret;
} else {
@@ -288,8 +283,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
pdata->max_brightness = num_levels - 1;
- ret = of_property_read_u32(node, "default-brightness",
- >default_brightness);
+ ret = device_property_read_u32(dev, "default-brightness",
>default_brightness);
if (ret)
pdata->default_brightness = pdata->max_brightness;
if (pdata->default_brightness > pdata->max_brightness) {
@@ -310,8 +304,8 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
* If missing, the default value for OVP is 35.5V
*/
pdata->over_voltage_protection = REG_I2C_1_OVP1;
- if (!of_property_read_u32(node, "mps,overvoltage-protection-microvolt",
- _value)) {
+ ret = device_property_read_u32(dev,
"mps,overvoltage-protection-microvolt", _value);
+ if (!ret) {
switch (tmp_value) {
case 1350:
pdata->over_voltage_protection = 0x00;
@@ -328,9 +322,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
}
/* Synchronous (default) and non-synchronous mo
[PATCH v3 2/3] backlight: mp3309c: use dev_err_probe() instead of dev_err()
Replace dev_err() with dev_err_probe().
This helps in simplifing code and standardizing the error output.
Tested-by: Flavio Suligoi
Reviewed-by: Daniel Thompson
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 397f35dafc5e..426e9f2356ad 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -208,10 +208,8 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
unsigned int num_levels, tmp_value;
struct device *dev = chip->dev;
- if (!dev_fwnode(dev)) {
- dev_err(dev, "failed to get firmware node\n");
- return -ENODEV;
- }
+ if (!dev_fwnode(dev))
+ return dev_err_probe(dev, -ENODEV, "failed to get firmware
node\n");
/*
* Dimming mode: the MP3309C provides two dimming control mode:
@@ -287,8 +285,7 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
if (ret)
pdata->default_brightness = pdata->max_brightness;
if (pdata->default_brightness > pdata->max_brightness) {
- dev_err(chip->dev,
- "default brightness exceeds max brightness\n");
+ dev_err_probe(dev, -ERANGE, "default brightness exceeds max
brightness\n");
pdata->default_brightness = pdata->max_brightness;
}
@@ -329,16 +326,15 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
static int mp3309c_probe(struct i2c_client *client)
{
- struct mp3309c_platform_data *pdata = dev_get_platdata(>dev);
+ struct device *dev = >dev;
+ struct mp3309c_platform_data *pdata = dev_get_platdata(dev);
struct mp3309c_chip *chip;
struct backlight_properties props;
struct pwm_state pwmstate;
int ret;
- if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
- dev_err(>dev, "failed to check i2c functionality\n");
- return -EOPNOTSUPP;
- }
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return dev_err_probe(dev, -EOPNOTSUPP, "failed to check i2c
functionality\n");
chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
--
2.43.0.rc1.1.gbec44491f096
[PATCH v3 0/3] backlight: mp3309c: Allow to use on non-OF platforms
Allow to use driver on non-OF platforms and other cleanups. Changelog v3: - rebased on top of the last changes against this driver (Lee) - added tags to patch 2 (Daniel, Flavio) Changelog v2: - rename pm3309c_parse_dt_node() --> mp3309c_parse_fwnode() (Daniel) - add tags (Daniel, Flavio) - new patch 2 Andy Shevchenko (3): backlight: mp3309c: Make use of device properties backlight: mp3309c: use dev_err_probe() instead of dev_err() backlight: mp3309c: Utilise temporary variable for struct device drivers/video/backlight/mp3309c.c | 88 --- 1 file changed, 35 insertions(+), 53 deletions(-) -- 2.43.0.rc1.1.gbec44491f096
[PATCH v3 1/3] backlight: mp3309c: Make use of device properties
Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.
Add mod_devicetable.h include.
Tested-by: Flavio Suligoi
Reviewed-by: Daniel Thompson
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 44 +--
1 file changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index b0d9aef6942b..397f35dafc5e 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -15,6 +15,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -199,18 +201,15 @@ static const struct backlight_ops mp3309c_bl_ops = {
.update_status = mp3309c_bl_update_status,
};
-static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
-struct mp3309c_platform_data *pdata)
+static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
+ struct mp3309c_platform_data *pdata)
{
- struct device_node *node = chip->dev->of_node;
- struct property *prop_pwms;
- struct property *prop_levels = NULL;
- int length = 0;
int ret, i;
unsigned int num_levels, tmp_value;
+ struct device *dev = chip->dev;
- if (!node) {
- dev_err(chip->dev, "failed to get DT node\n");
+ if (!dev_fwnode(dev)) {
+ dev_err(dev, "failed to get firmware node\n");
return -ENODEV;
}
@@ -224,8 +223,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
* found in the backlight node, the mode switches to PWM mode.
*/
pdata->dimming_mode = DIMMING_ANALOG_I2C;
- prop_pwms = of_find_property(node, "pwms", );
- if (prop_pwms) {
+ if (device_property_present(dev, "pwms")) {
chip->pwmd = devm_pwm_get(chip->dev, NULL);
if (IS_ERR(chip->pwmd))
return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
@@ -257,11 +255,9 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
/*
* PWM control mode: check for brightness level in DT
*/
- prop_levels = of_find_property(node, "brightness-levels",
- );
- if (prop_levels) {
+ if (device_property_present(dev, "brightness-levels")) {
/* Read brightness levels from DT */
- num_levels = length / sizeof(u32);
+ num_levels = device_property_count_u32(dev,
"brightness-levels");
if (num_levels < 2)
return -EINVAL;
} else {
@@ -275,10 +271,9 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
sizeof(*pdata->levels), GFP_KERNEL);
if (!pdata->levels)
return -ENOMEM;
- if (prop_levels) {
- ret = of_property_read_u32_array(node, "brightness-levels",
-pdata->levels,
-num_levels);
+ if (device_property_present(dev, "brightness-levels")) {
+ ret = device_property_read_u32_array(dev, "brightness-levels",
+pdata->levels, num_levels);
if (ret < 0)
return ret;
} else {
@@ -288,8 +283,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
pdata->max_brightness = num_levels - 1;
- ret = of_property_read_u32(node, "default-brightness",
- >default_brightness);
+ ret = device_property_read_u32(dev, "default-brightness",
>default_brightness);
if (ret)
pdata->default_brightness = pdata->max_brightness;
if (pdata->default_brightness > pdata->max_brightness) {
@@ -310,8 +304,8 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
* If missing, the default value for OVP is 35.5V
*/
pdata->over_voltage_protection = REG_I2C_1_OVP1;
- if (!of_property_read_u32(node, "mps,overvoltage-protection-microvolt",
- _value)) {
+ ret = device_property_read_u32(dev,
"mps,overvoltage-protection-microvolt", _value);
+ if (!ret) {
switch (tmp_value) {
case 1350:
pdata->over_voltage_protection = 0x00;
@@ -328,9 +322,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
}
/* Synchronous (default) and non-synchronous mo
[PATCH v3 3/3] backlight: mp3309c: Utilise temporary variable for struct device
We have a temporary variable to keep pointer to struct device.
Utilise it where it makes sense.
Reviewed-by: Daniel Thompson
Tested-by: Flavio Suligoi
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 30 --
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 426e9f2356ad..708c053d492c 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -222,10 +222,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
*/
pdata->dimming_mode = DIMMING_ANALOG_I2C;
if (device_property_present(dev, "pwms")) {
- chip->pwmd = devm_pwm_get(chip->dev, NULL);
+ chip->pwmd = devm_pwm_get(dev, NULL);
if (IS_ERR(chip->pwmd))
- return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
-"error getting pwm data\n");
+ return dev_err_probe(dev, PTR_ERR(chip->pwmd), "error
getting pwm data\n");
pdata->dimming_mode = DIMMING_PWM;
pwm_apply_args(chip->pwmd);
}
@@ -243,11 +242,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
num_levels = ANALOG_I2C_NUM_LEVELS;
/* Enable GPIO used in I2C dimming mode only */
- chip->enable_gpio = devm_gpiod_get(chip->dev, "enable",
- GPIOD_OUT_HIGH);
+ chip->enable_gpio = devm_gpiod_get(dev, "enable",
GPIOD_OUT_HIGH);
if (IS_ERR(chip->enable_gpio))
- return dev_err_probe(chip->dev,
-PTR_ERR(chip->enable_gpio),
+ return dev_err_probe(dev, PTR_ERR(chip->enable_gpio),
"error getting enable gpio\n");
} else {
/*
@@ -265,8 +262,7 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
}
/* Fill brightness levels array */
- pdata->levels = devm_kcalloc(chip->dev, num_levels,
-sizeof(*pdata->levels), GFP_KERNEL);
+ pdata->levels = devm_kcalloc(dev, num_levels, sizeof(*pdata->levels),
GFP_KERNEL);
if (!pdata->levels)
return -ENOMEM;
if (device_property_present(dev, "brightness-levels")) {
@@ -336,21 +332,21 @@ static int mp3309c_probe(struct i2c_client *client)
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
return dev_err_probe(dev, -EOPNOTSUPP, "failed to check i2c
functionality\n");
- chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;
- chip->dev = >dev;
+ chip->dev = dev;
chip->regmap = devm_regmap_init_i2c(client, _regmap);
if (IS_ERR(chip->regmap))
- return dev_err_probe(>dev, PTR_ERR(chip->regmap),
+ return dev_err_probe(dev, PTR_ERR(chip->regmap),
"failed to allocate register map\n");
i2c_set_clientdata(client, chip);
if (!pdata) {
- pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
@@ -367,11 +363,10 @@ static int mp3309c_probe(struct i2c_client *client)
props.type = BACKLIGHT_RAW;
props.power = FB_BLANK_UNBLANK;
props.fb_blank = FB_BLANK_UNBLANK;
- chip->bl = devm_backlight_device_register(chip->dev, "mp3309c",
- chip->dev, chip,
+ chip->bl = devm_backlight_device_register(dev, "mp3309c", dev, chip,
_bl_ops, );
if (IS_ERR(chip->bl))
- return dev_err_probe(chip->dev, PTR_ERR(chip->bl),
+ return dev_err_probe(dev, PTR_ERR(chip->bl),
"error registering backlight device\n");
/* In PWM dimming mode, enable pwm device */
@@ -383,8 +378,7 @@ static int mp3309c_probe(struct i2c_client *client)
pwmstate.enabled = true;
ret = pwm_apply_might_sleep(chip->pwmd, );
if (ret)
- return dev_err_probe(chip->dev, ret,
-"error setting pwm device\n");
+ return dev_err_probe(dev, ret, "error setting pwm
device\n");
}
chip->pdata->status = FIRST_POWER_ON;
--
2.43.0.rc1.1.gbec44491f096
Re: [PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
On Thu, Feb 08, 2024 at 06:14:55PM +, Lee Jones wrote: > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > On Thu, Feb 08, 2024 at 05:39:46PM +, Lee Jones wrote: > > > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > > > On Thu, Feb 08, 2024 at 11:34:25AM +, Lee Jones wrote: > > > > > On Thu, 01 Feb 2024, Andy Shevchenko wrote: ... > > > > > > backlight: mp3309c: Utilise temporary variable for struct device > > > > > > > > (1) > > > > > > > > > Set no longer applies. Please rebase, thanks. > > > > > > > > I got a contradictory messages: > > > > 1) email that says that all had been applied; > > > > 2) this email (that tells the complete opposite); > > > > 3) the repository where the first two were applied. > > > > > > > > While you can amend your scripts, I think I need to rebase only the > > > > last patch > > > > > > This is what I assume happened: > > > > > > 1. Attempted to apply the set (as a set) > > > 2. 2 commits applied cleanly > > > 3. The final commit conflicted > > > > Which is really strange. I have just applied (with b4) on top of your > > changes > > and no complains so far. > > > > $ git am > > ./v2_20240201_andriy_shevchenko_backlight_mp3309c_allow_to_use_on_non_of_platforms.mbx > > Applying: backlight: mp3309c: Make use of device properties > > Applying: backlight: mp3309c: use dev_err_probe() instead of dev_err() > > Applying: backlight: mp3309c: Utilise temporary variable for struct device > > > > Can you show what b4 tells you about this? > > Fetching patch(es) > Analyzing 14 messages in the thread > Checking attestation on all messages, may take a moment... > --- > ✓ [PATCH v2 1/3] backlight: mp3309c: Make use of device properties > + Reviewed-by: Daniel Thompson (✓ > DKIM/linaro.org) > + Link: > https://lore.kernel.org/r/20240201151537.367218-2-andriy.shevche...@linux.intel.com > + Signed-off-by: Lee Jones > ✓ [PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of > dev_err() > + Tested-by: Flavio Suligoi (✗ DKIM/asem.it) > + Reviewed-by: Daniel Thompson (✓ > DKIM/linaro.org) > + Link: > https://lore.kernel.org/r/20240201151537.367218-3-andriy.shevche...@linux.intel.com > + Signed-off-by: Lee Jones > ✓ [PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for struct > device > + Link: > https://lore.kernel.org/r/20240201151537.367218-4-andriy.shevche...@linux.intel.com > + Signed-off-by: Lee Jones > --- > ✓ Signed: DKIM/intel.com (From: andriy.shevche...@linux.intel.com) > --- > Total patches: 3 > Prepared a fake commit range for 3-way merge (672ecc5199b5..d507b9f4c5b9) > --- > Link: > https://lore.kernel.org/r/20240201151537.367218-1-andriy.shevche...@linux.intel.com > Base: not specified > > Running through checkpatch.pl > total: 0 errors, 0 warnings, 103 lines checked > > "[PATCH v2 1/3] backlight: mp3309c: Make use of device properties" has no > obvious style problems and is ready for submission. > total: 0 errors, 0 warnings, 41 lines checked > > "[PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of" has no > obvious style problems and is ready for submission. > total: 0 errors, 0 warnings, 81 lines checked > > "[PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for" has no > obvious style problems and is ready for submission. > > Check the results (hit return to continue or Ctrl+c to exit) > > > Applying patch(es) > Applying: backlight: mp3309c: Make use of device properties > Applying: backlight: mp3309c: use dev_err_probe() instead of dev_err() > Applying: backlight: mp3309c: Utilise temporary variable for struct device > Using index info to reconstruct a base tree... > M drivers/video/backlight/mp3309c.c > Checking patch drivers/video/backlight/mp3309c.c... > Applied patch drivers/video/backlight/mp3309c.c cleanly. > Falling back to patching base and 3-way merge... > error: Your local changes to the following files would be overwritten by > merge: > drivers/video/backlight/mp3309c.c > Please commit your changes or stash them before you merge. > Aborting > error: Failed to merge in the changes. > Patch failed at 0003 backlight: mp3309c: Utilise temporary variable for > struct device > hint: Use 'git am --show-current-patch=diff' to see the failed patch > When you have resolved this problem, run "git am --continue". > If you prefer to skip this patch, run "git am --skip" instead. > To restore the original branch and stop patching, run "git am --abort". Thank you! It seems I have reduced context, so if you do `git am -C2 ...` it should apply. Never mind, I'll send a new version which should work with -C3. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
On Thu, Feb 08, 2024 at 05:39:46PM +, Lee Jones wrote: > On Thu, 08 Feb 2024, Andy Shevchenko wrote: > > On Thu, Feb 08, 2024 at 11:34:25AM +, Lee Jones wrote: > > > On Thu, 01 Feb 2024, Andy Shevchenko wrote: ... > > > > backlight: mp3309c: Utilise temporary variable for struct device > > > > (1) > > > > > Set no longer applies. Please rebase, thanks. > > > > I got a contradictory messages: > > 1) email that says that all had been applied; > > 2) this email (that tells the complete opposite); > > 3) the repository where the first two were applied. > > > > While you can amend your scripts, I think I need to rebase only the last > > patch > > This is what I assume happened: > > 1. Attempted to apply the set (as a set) > 2. 2 commits applied cleanly > 3. The final commit conflicted Which is really strange. I have just applied (with b4) on top of your changes and no complains so far. $ git am ./v2_20240201_andriy_shevchenko_backlight_mp3309c_allow_to_use_on_non_of_platforms.mbx Applying: backlight: mp3309c: Make use of device properties Applying: backlight: mp3309c: use dev_err_probe() instead of dev_err() Applying: backlight: mp3309c: Utilise temporary variable for struct device Can you show what b4 tells you about this? > 4. I sent you a message to say that the set failed to apply > 5. *** I forgot to remove the 2 successful patches *** > 6. I applied another patch > 7. b4 noticed the 2 patches that were applied and thanked you for them > 8. *** I didn't notice that those tys were sent *** > > No need to update the scripts. :) > > > (1) that may not be found in your tree currently. > > I'm going to remove the other ones now. Please submit the set. I'll do, but I want to understand better what's going on. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
On Thu, Feb 08, 2024 at 11:34:25AM +, Lee Jones wrote: > On Thu, 01 Feb 2024, Andy Shevchenko wrote: ... > > backlight: mp3309c: Utilise temporary variable for struct device (1) > Set no longer applies. Please rebase, thanks. I got a contradictory messages: 1) email that says that all had been applied; 2) this email (that tells the complete opposite); 3) the repository where the first two were applied. While you can amend your scripts, I think I need to rebase only the last patch (1) that may not be found in your tree currently. -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/4] backlight: hx8357: Clean up and make OF-independent
On Thu, Feb 08, 2024 at 10:53:04AM +, Lee Jones wrote: > On Thu, 01 Feb 2024, Andy Shevchenko wrote: ... > Someone may wish to address this: > > WARNING: DT compatible string "himax,hx8369" appears un-documented -- check > ./Documentation/devicetree/bindings/ > #58: FILE: drivers/video/backlight/hx8357.c:636: > + .compatible = "himax,hx8369", I can do it if and when have more time. But apparently it was before this series, right? -- With Best Regards, Andy Shevchenko
Re: [PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
On Fri, Feb 02, 2024 at 10:52:32AM +, Flavio Suligoi wrote: > > Andy Shevchenko (3): > > backlight: mp3309c: Make use of device properties > > backlight: mp3309c: use dev_err_probe() instead of dev_err() > > backlight: mp3309c: Utilise temporary variable for struct device > I've just tested again all your three patches (using the last 8.8.0-rc1 > for-backlight-next ) on my board and all is ok. Thank you! -- With Best Regards, Andy Shevchenko
[PATCH v2 1/3] backlight: mp3309c: Make use of device properties
Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.
Add mod_devicetable.h include.
Tested-by: Flavio Suligoi
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 44 +--
1 file changed, 18 insertions(+), 26 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 34d71259fac1..762bd738c903 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -15,6 +15,8 @@
#include
#include
#include
+#include
+#include
#include
#include
@@ -199,18 +201,15 @@ static const struct backlight_ops mp3309c_bl_ops = {
.update_status = mp3309c_bl_update_status,
};
-static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
-struct mp3309c_platform_data *pdata)
+static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
+ struct mp3309c_platform_data *pdata)
{
- struct device_node *node = chip->dev->of_node;
- struct property *prop_pwms;
- struct property *prop_levels = NULL;
- int length = 0;
int ret, i;
unsigned int num_levels, tmp_value;
+ struct device *dev = chip->dev;
- if (!node) {
- dev_err(chip->dev, "failed to get DT node\n");
+ if (!dev_fwnode(dev)) {
+ dev_err(dev, "failed to get firmware node\n");
return -ENODEV;
}
@@ -224,8 +223,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
* found in the backlight node, the mode switches to PWM mode.
*/
pdata->dimming_mode = DIMMING_ANALOG_I2C;
- prop_pwms = of_find_property(node, "pwms", );
- if (prop_pwms) {
+ if (device_property_present(dev, "pwms")) {
chip->pwmd = devm_pwm_get(chip->dev, NULL);
if (IS_ERR(chip->pwmd))
return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
@@ -257,11 +255,9 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
/*
* PWM control mode: check for brightness level in DT
*/
- prop_levels = of_find_property(node, "brightness-levels",
- );
- if (prop_levels) {
+ if (device_property_present(dev, "brightness-levels")) {
/* Read brightness levels from DT */
- num_levels = length / sizeof(u32);
+ num_levels = device_property_count_u32(dev,
"brightness-levels");
if (num_levels < 2)
return -EINVAL;
} else {
@@ -275,10 +271,9 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
sizeof(*pdata->levels), GFP_KERNEL);
if (!pdata->levels)
return -ENOMEM;
- if (prop_levels) {
- ret = of_property_read_u32_array(node, "brightness-levels",
-pdata->levels,
-num_levels);
+ if (device_property_present(dev, "brightness-levels")) {
+ ret = device_property_read_u32_array(dev, "brightness-levels",
+pdata->levels, num_levels);
if (ret < 0)
return ret;
} else {
@@ -288,8 +283,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
pdata->max_brightness = num_levels - 1;
- ret = of_property_read_u32(node, "default-brightness",
- >default_brightness);
+ ret = device_property_read_u32(dev, "default-brightness",
>default_brightness);
if (ret)
pdata->default_brightness = pdata->max_brightness;
if (pdata->default_brightness > pdata->max_brightness) {
@@ -310,8 +304,8 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
* If missing, the default value for OVP is 35.5V
*/
pdata->over_voltage_protection = REG_I2C_1_OVP1;
- if (!of_property_read_u32(node, "mps,overvoltage-protection-microvolt",
- _value)) {
+ ret = device_property_read_u32(dev,
"mps,overvoltage-protection-microvolt", _value);
+ if (!ret) {
switch (tmp_value) {
case 1350:
pdata->over_voltage_protection = 0x00;
@@ -328,9 +322,7 @@ static int pm3309c_parse_dt_node(struct mp3309c_chip *chip,
}
/* Synchronous (default) and non-synchronous mode */
- pdata->sync_mode =
[PATCH v2 3/3] backlight: mp3309c: Utilise temporary variable for struct device
We have a temporary variable to keep pointer to struct device.
Utilise it where it makes sense.
Reviewed-by: Daniel Thompson
Tested-by: Flavio Suligoi
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 30 --
1 file changed, 12 insertions(+), 18 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 2d6bd1a180b3..e7abefd175a4 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -222,10 +222,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
*/
pdata->dimming_mode = DIMMING_ANALOG_I2C;
if (device_property_present(dev, "pwms")) {
- chip->pwmd = devm_pwm_get(chip->dev, NULL);
+ chip->pwmd = devm_pwm_get(dev, NULL);
if (IS_ERR(chip->pwmd))
- return dev_err_probe(chip->dev, PTR_ERR(chip->pwmd),
-"error getting pwm data\n");
+ return dev_err_probe(dev, PTR_ERR(chip->pwmd), "error
getting pwm data\n");
pdata->dimming_mode = DIMMING_PWM;
pwm_apply_args(chip->pwmd);
}
@@ -243,11 +242,9 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
num_levels = ANALOG_I2C_NUM_LEVELS;
/* Enable GPIO used in I2C dimming mode only */
- chip->enable_gpio = devm_gpiod_get(chip->dev, "enable",
- GPIOD_OUT_HIGH);
+ chip->enable_gpio = devm_gpiod_get(dev, "enable",
GPIOD_OUT_HIGH);
if (IS_ERR(chip->enable_gpio))
- return dev_err_probe(chip->dev,
-PTR_ERR(chip->enable_gpio),
+ return dev_err_probe(dev, PTR_ERR(chip->enable_gpio),
"error getting enable gpio\n");
} else {
/*
@@ -265,8 +262,7 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
}
/* Fill brightness levels array */
- pdata->levels = devm_kcalloc(chip->dev, num_levels,
-sizeof(*pdata->levels), GFP_KERNEL);
+ pdata->levels = devm_kcalloc(dev, num_levels, sizeof(*pdata->levels),
GFP_KERNEL);
if (!pdata->levels)
return -ENOMEM;
if (device_property_present(dev, "brightness-levels")) {
@@ -336,21 +332,21 @@ static int mp3309c_probe(struct i2c_client *client)
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
return dev_err_probe(dev, -EOPNOTSUPP, "failed to check i2c
functionality\n");
- chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
+ chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
return -ENOMEM;
- chip->dev = >dev;
+ chip->dev = dev;
chip->regmap = devm_regmap_init_i2c(client, _regmap);
if (IS_ERR(chip->regmap))
- return dev_err_probe(>dev, PTR_ERR(chip->regmap),
+ return dev_err_probe(dev, PTR_ERR(chip->regmap),
"failed to allocate register map\n");
i2c_set_clientdata(client, chip);
if (!pdata) {
- pdata = devm_kzalloc(chip->dev, sizeof(*pdata), GFP_KERNEL);
+ pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return -ENOMEM;
@@ -367,11 +363,10 @@ static int mp3309c_probe(struct i2c_client *client)
props.type = BACKLIGHT_RAW;
props.power = FB_BLANK_UNBLANK;
props.fb_blank = FB_BLANK_UNBLANK;
- chip->bl = devm_backlight_device_register(chip->dev, "mp3309c",
- chip->dev, chip,
+ chip->bl = devm_backlight_device_register(dev, "mp3309c", dev, chip,
_bl_ops, );
if (IS_ERR(chip->bl))
- return dev_err_probe(chip->dev, PTR_ERR(chip->bl),
+ return dev_err_probe(dev, PTR_ERR(chip->bl),
"error registering backlight device\n");
/* In PWM dimming mode, enable pwm device */
@@ -383,8 +378,7 @@ static int mp3309c_probe(struct i2c_client *client)
pwmstate.enabled = true;
ret = pwm_apply_state(chip->pwmd, );
if (ret)
- return dev_err_probe(chip->dev, ret,
-"error setting pwm device\n");
+ return dev_err_probe(dev, ret, "error setting pwm
device\n");
}
chip->pdata->status = FIRST_POWER_ON;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v2 1/4] backlight: hx8357: Make use of device properties
Convert the module to be property provider agnostic and allow
it to be used on non-OF platforms.
Include mod_devicetable.h explicitly to replace the dropped of.h
which included mod_devicetable.h indirectly.
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/hx8357.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index bf18337ff0c2..ac65609e5d84 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -8,9 +8,9 @@
#include
#include
#include
+#include
#include
-#include
-#include
+#include
#include
#define HX8357_NUM_IM_PINS 3
@@ -564,6 +564,8 @@ static struct lcd_ops hx8357_ops = {
.get_power = hx8357_get_power,
};
+typedef int (*hx8357_init_fn)(struct lcd_device *);
+
static const struct of_device_id hx8357_dt_ids[] = {
{
.compatible = "himax,hx8357",
@@ -582,7 +584,7 @@ static int hx8357_probe(struct spi_device *spi)
struct device *dev = >dev;
struct lcd_device *lcdev;
struct hx8357_data *lcd;
- const struct of_device_id *match;
+ hx8357_init_fn init_fn;
int i, ret;
lcd = devm_kzalloc(>dev, sizeof(*lcd), GFP_KERNEL);
@@ -597,8 +599,8 @@ static int hx8357_probe(struct spi_device *spi)
lcd->spi = spi;
- match = of_match_device(hx8357_dt_ids, >dev);
- if (!match || !match->data)
+ init_fn = device_get_match_data(dev);
+ if (!init_fn)
return -EINVAL;
lcd->reset = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
@@ -627,7 +629,7 @@ static int hx8357_probe(struct spi_device *spi)
hx8357_lcd_reset(lcdev);
- ret = ((int (*)(struct lcd_device *))match->data)(lcdev);
+ ret = init_fn(lcdev);
if (ret) {
dev_err(>dev, "Couldn't initialize panel\n");
return ret;
--
2.43.0.rc1.1.gbec44491f096
[PATCH v2 2/4] backlight: hx8357: Move OF table closer to its consumer
Move OF table near to the user.
While at it, drop comma at terminator entry.
Reviewed-by: Daniel Thompson
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/hx8357.c | 26 +-
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index ac65609e5d84..81d0984e9d8b 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -566,19 +566,6 @@ static struct lcd_ops hx8357_ops = {
typedef int (*hx8357_init_fn)(struct lcd_device *);
-static const struct of_device_id hx8357_dt_ids[] = {
- {
- .compatible = "himax,hx8357",
- .data = hx8357_lcd_init,
- },
- {
- .compatible = "himax,hx8369",
- .data = hx8369_lcd_init,
- },
- {},
-};
-MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
-
static int hx8357_probe(struct spi_device *spi)
{
struct device *dev = >dev;
@@ -640,6 +627,19 @@ static int hx8357_probe(struct spi_device *spi)
return 0;
}
+static const struct of_device_id hx8357_dt_ids[] = {
+ {
+ .compatible = "himax,hx8357",
+ .data = hx8357_lcd_init,
+ },
+ {
+ .compatible = "himax,hx8369",
+ .data = hx8369_lcd_init,
+ },
+ {}
+};
+MODULE_DEVICE_TABLE(of, hx8357_dt_ids);
+
static struct spi_driver hx8357_driver = {
.probe = hx8357_probe,
.driver = {
--
2.43.0.rc1.1.gbec44491f096
[PATCH v2 2/3] backlight: mp3309c: use dev_err_probe() instead of dev_err()
Replace dev_err() with dev_err_probe().
This helps in simplifing code and standardizing the error output.
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/mp3309c.c | 18 +++---
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/drivers/video/backlight/mp3309c.c
b/drivers/video/backlight/mp3309c.c
index 762bd738c903..2d6bd1a180b3 100644
--- a/drivers/video/backlight/mp3309c.c
+++ b/drivers/video/backlight/mp3309c.c
@@ -208,10 +208,8 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
unsigned int num_levels, tmp_value;
struct device *dev = chip->dev;
- if (!dev_fwnode(dev)) {
- dev_err(dev, "failed to get firmware node\n");
- return -ENODEV;
- }
+ if (!dev_fwnode(dev))
+ return dev_err_probe(dev, -ENODEV, "failed to get firmware
node\n");
/*
* Dimming mode: the MP3309C provides two dimming control mode:
@@ -287,8 +285,7 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
if (ret)
pdata->default_brightness = pdata->max_brightness;
if (pdata->default_brightness > pdata->max_brightness) {
- dev_err(chip->dev,
- "default brightness exceeds max brightness\n");
+ dev_err_probe(dev, -ERANGE, "default brightness exceeds max
brightness\n");
pdata->default_brightness = pdata->max_brightness;
}
@@ -329,16 +326,15 @@ static int mp3309c_parse_fwnode(struct mp3309c_chip *chip,
static int mp3309c_probe(struct i2c_client *client)
{
- struct mp3309c_platform_data *pdata = dev_get_platdata(>dev);
+ struct device *dev = >dev;
+ struct mp3309c_platform_data *pdata = dev_get_platdata(dev);
struct mp3309c_chip *chip;
struct backlight_properties props;
struct pwm_state pwmstate;
int ret;
- if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
- dev_err(>dev, "failed to check i2c functionality\n");
- return -EOPNOTSUPP;
- }
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return dev_err_probe(dev, -EOPNOTSUPP, "failed to check i2c
functionality\n");
chip = devm_kzalloc(>dev, sizeof(*chip), GFP_KERNEL);
if (!chip)
--
2.43.0.rc1.1.gbec44491f096
[PATCH v2 0/3] backlight: mp3309c: Allow to use on non-OF platforms
Allow to use driver on non-OF platforms and other cleanups. Changelog v2: - rename pm3309c_parse_dt_node() --> mp3309c_parse_fwnode() (Daniel) - add tags (Daniel, Flavio) - new patch 2 Andy Shevchenko (3): backlight: mp3309c: Make use of device properties backlight: mp3309c: use dev_err_probe() instead of dev_err() backlight: mp3309c: Utilise temporary variable for struct device drivers/video/backlight/mp3309c.c | 88 --- 1 file changed, 35 insertions(+), 53 deletions(-) -- 2.43.0.rc1.1.gbec44491f096
[PATCH v2 4/4] backlight: hx8357: Utilise temporary variable for struct device
We have a temporary variable to keep pointer to struct device.
Utilise it inside the ->probe() implementation.
Reviewed-by: Daniel Thompson
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/hx8357.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 70a62755805a..339d9128fbde 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -574,7 +574,7 @@ static int hx8357_probe(struct spi_device *spi)
hx8357_init_fn init_fn;
int i, ret;
- lcd = devm_kzalloc(>dev, sizeof(*lcd), GFP_KERNEL);
+ lcd = devm_kzalloc(dev, sizeof(*lcd), GFP_KERNEL);
if (!lcd)
return -ENOMEM;
@@ -604,8 +604,7 @@ static int hx8357_probe(struct spi_device *spi)
gpiod_set_consumer_name(lcd->im_pins->desc[i],
"im_pins");
}
- lcdev = devm_lcd_device_register(>dev, "mxsfb", >dev, lcd,
- _ops);
+ lcdev = devm_lcd_device_register(dev, "mxsfb", dev, lcd, _ops);
if (IS_ERR(lcdev)) {
ret = PTR_ERR(lcdev);
return ret;
@@ -618,7 +617,7 @@ static int hx8357_probe(struct spi_device *spi)
if (ret)
return dev_err_probe(dev, ret, "Couldn't initialize panel\n");
- dev_info(>dev, "Panel probed\n");
+ dev_info(dev, "Panel probed\n");
return 0;
}
--
2.43.0.rc1.1.gbec44491f096
[PATCH v2 3/4] backlight: hx8357: Make use of dev_err_probe()
Simplify the error handling in probe function by switching from
dev_err() to dev_err_probe().
Reviewed-by: Daniel Thompson
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Andy Shevchenko
---
drivers/video/backlight/hx8357.c | 12
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/video/backlight/hx8357.c b/drivers/video/backlight/hx8357.c
index 81d0984e9d8b..70a62755805a 100644
--- a/drivers/video/backlight/hx8357.c
+++ b/drivers/video/backlight/hx8357.c
@@ -579,10 +579,8 @@ static int hx8357_probe(struct spi_device *spi)
return -ENOMEM;
ret = spi_setup(spi);
- if (ret < 0) {
- dev_err(>dev, "SPI setup failed.\n");
- return ret;
- }
+ if (ret < 0)
+ return dev_err_probe(dev, ret, "SPI setup failed.\n");
lcd->spi = spi;
@@ -617,10 +615,8 @@ static int hx8357_probe(struct spi_device *spi)
hx8357_lcd_reset(lcdev);
ret = init_fn(lcdev);
- if (ret) {
- dev_err(>dev, "Couldn't initialize panel\n");
- return ret;
- }
+ if (ret)
+ return dev_err_probe(dev, ret, "Couldn't initialize panel\n");
dev_info(>dev, "Panel probed\n");
--
2.43.0.rc1.1.gbec44491f096
[PATCH v2 0/4] backlight: hx8357: Clean up and make OF-independent
A few ad-hoc cleanups and one patch to make driver OF-independent. Chagelog v2: - renamed init to init_fn and typedef accordingly (Daniel) - added tags (Daniel, Javier) Andy Shevchenko (4): backlight: hx8357: Make use of device properties backlight: hx8357: Move OF table closer to its consumer backlight: hx8357: Make use of dev_err_probe() backlight: hx8357: Utilise temporary variable for struct device drivers/video/backlight/hx8357.c | 57 +++- 1 file changed, 27 insertions(+), 30 deletions(-) -- 2.43.0.rc1.1.gbec44491f096
Re: [PATCH v1 4/4] backlight: hx8357: Utilise temporary variable for struct device
On Wed, Jan 24, 2024 at 05:25:07PM +, Daniel Thompson wrote: > On Sun, Jan 14, 2024 at 05:25:11PM +0200, Andy Shevchenko wrote: ... > Reviewed-by: Daniel Thompson Thank you for the review, I will address comments and send a new version at the end of the next week I hope. -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
On Wed, Jan 24, 2024 at 05:19:53PM +, Daniel Thompson wrote: > On Sun, Jan 14, 2024 at 05:25:08PM +0200, Andy Shevchenko wrote: ... > > +typedef int (*hx8357_init)(struct lcd_device *); > > + hx8357_init init; > > As somewhere else in this thread, I'd find this a lot more readable > as: > hx8357_init_fn init_fn; I agree with you, dunno why I haven't added _fn in the first place... -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 1/4] backlight: hx8357: Make use of device properties
On Mon, Jan 15, 2024 at 09:20:46AM +0100, Javier Martinez Canillas wrote: > Andy Shevchenko writes: ... > > +typedef int (*hx8357_init)(struct lcd_device *); > > This kind of typedef usage is frowned upon in the Linux coding style [0] > (per my understanding at least) and indeed in my opinion it makes harder > to grep. > > [0] https://www.kernel.org/doc/Documentation/process/coding-style.rst Thanks for pointing this out. However, this piece does _not_ clarify typedef:s for function pointers which I personally find a good to have. ... > > - ret = ((int (*)(struct lcd_device *))match->data)(lcdev); > > This is what I mean, before it was clear what was stored in match->data. > But after you changes, what is returned by the device_get_match_data() > function is opaque and you need to look at the typedef hx8357_init to > figure that out. The above is so ugly in my opinion, that justifies using typedef:s even if you are quite skeptical about them. ... > No strong opinion though and I see other drivers doing the same (but no > other driver in drivers/video/backlight). > > Reviewed-by: Javier Martinez Canillas Thank you for the review! -- With Best Regards, Andy Shevchenko
Re: [PATCH v1 2/4] backlight: hx8357: Move OF table closer to its consumer
On Mon, Jan 15, 2024 at 09:22:19AM +0100, Javier Martinez Canillas wrote:
> Andy Shevchenko writes:
...
> > + {}
>
> While at it, maybe add the { /* sentinel */ } convention to the last entry ?
Maybe. Is it a common for this subsystem?
...
> Reviewed-by: Javier Martinez Canillas
Thank you for the review!
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 09/10] pci: devres: remove legacy pcim_release()
Mon, Jan 15, 2024 at 03:46:20PM +0100, Philipp Stanner kirjoitti: > Thanks to preceding cleanup steps, pcim_release() is now not needed > anymore and can be replaced by pcim_disable_device(), which is the exact > counterpart to pcim_enable_device(). > This permits removing further parts of the old devres API. > > Replace pcim_release() with pcim_disable_device(). > Remove the now surplus get_dr() function. ... > + devm_add_action(>dev, pcim_disable_device, pdev); No error check? > + return pci_enable_device(pdev); Maybe ret = pci_enable_device(...); if (ret) return ret; return devm_add_action_or_reset(...)? I could think of side effects of this, so perhaps the commit message and/or code needs a comment on why the above proposal can _not_ be used? -- With Best Regards, Andy Shevchenko
Re: [PATCH 08/10] pci: devres: give pci(m)_intx its own callback
Mon, Jan 15, 2024 at 03:46:19PM +0100, Philipp Stanner kirjoitti:
> pci_intx() is one of the functions that have "hybrid mode" (i.e.,
> sometimes managed, sometimes not). Providing a separate pcim_intx()
> function with its own device resource and cleanup callback allows for
> removing further large parts of the legacy pci-devres implementation.
>
> As in the region-request-functions, pci_intx() has to call into its
> managed counterpart for backwards compatibility.
>
> Implement pcim_intx() with its own device resource.
> Make pci_intx() call pcim_intx() in the managed case.
> Remove the legacy devres struct from pci.h.
...
> + /*
> + * This is done for backwards compatibility, because the old pci-devres
> + * API had a mode in which this function became managed if the dev had
> + * been enabled with pcim_enable_device() instead of
> pci_enable_device().
> + */
> + if (pci_is_managed(pdev)) {
> + if (pcim_intx(pdev, enable) != 0)
> + WARN_ON_ONCE(1);
WARN_ON_ONCE(pcim_intx(pdev, enable));
?
> + return;
> + }
...
> + if (new != pci_command)
if (new == pci_command)
return;
?
> pci_write_config_word(pdev, PCI_COMMAND, new);
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 06/10] pci: move pinned status bit to pci_dev struct
Mon, Jan 15, 2024 at 03:46:17PM +0100, Philipp Stanner kirjoitti: > The bit describing whether the PCI device is currently pinned is stored > in the PCI devres struct. To clean up and simplify the pci-devres API, "PCI devres", 'pci-devres', ... Shouldn't these (and across entire series) be consistent terms? E.g., "PCI devres API". > it's better if this information is stored in the pci_dev struct, because pci_dev struct --> struct pci_dev > it allows for checking that device's pinned-status directly through the > device struct. > This will later permit simplifying pcim_enable_device(). > Move the 'pinned' boolean bit to struct pci_dev. ... > u8 pm_cap; /* PM capability offset */ > unsigned intenabled:1; /* Whether this dev is enabled */ > + unsigned intpinned:1; /* Whether this dev is pinned */ > unsigned intimm_ready:1;/* Supports Immediate Readiness */ > unsigned intpme_support:5; /* Bitmask of states from which PME# > can be generated */ First of all, I think it's better to group PM stuff, like u8 pm_cap; /* PM capability offset */ unsigned intpme_support:5; /* Bitmask of states from which PME# can be generated */ unsigned intimm_ready:1;/* Supports Immediate Readiness */ unsigned intenabled:1; /* Whether this dev is enabled */ unsigned intpinned:1; /* Whether this dev is pinned */ Second, does this layout anyhow related to the HW layout? (For example, PME bits and their location in some HW register vs. these bitfields) If so, but not sure, it might be good to preserve (to some extent) the order. -- With Best Regards, Andy Shevchenko
Re: [PATCH 00/10] Make PCI's devres API more consistent
Mon, Jan 15, 2024 at 03:46:11PM +0100, Philipp Stanner kirjoitti: > ¡Hola! i? Vim user? :-) > PCI's devres API suffers several weaknesses: > > 1. There are functions prefixed with pcim_. Those are always managed >counterparts to never-managed functions prefixed with pci_ – or so one >would like to think. There are some apparently unmanaged functions >(all region-request / release functions, and pci_intx()) which >suddenly become managed once the user has initialized the device with >pcim_enable_device() instead of pci_enable_device(). This "sometimes >yes, sometimes no" nature of those functions is confusing and >therefore bug-provoking. In fact, it has already caused a bug in DRM. >The last patch in this series fixes that bug. > 2. iomappings: Instead of giving each mapping its own callback, the >existing API uses a statically allocated struct tracking one mapping >per bar. This is not extensible. Especially, you can't create >_ranged_ managed mappings that way, which many drivers want. > 3. Managed request functions only exist as "plural versions" with a >bit-mask as a parameter. That's quite over-engineered considering >that each user only ever mapps one, maybe two bars. > > This series: > - add a set of new "singular" devres functions that use devres the way > its intended, with one callback per resource. > - deprecates the existing iomap-table mechanism. > - deprecates the hybrid nature of pci_ functions. > - preserves backwards compatibility so that drivers using the existing > API won't notice any changes. > - adds documentation, especially some warning users about the > complicated nature of PCI's devres. Instead of adding pcim_intx(), please provide proper one for pci_alloc_irq_vectors(). Ideally it would be nice to deprecate old IRQ management functions in PCI core and delete them in the future. -- With Best Regards, Andy Shevchenko
Re: [PATCH 01/10] pci: add new set of devres functions
> + return;
> +
> + if (len == 0 || maxlen == 0) /* This an unused BAR. Do nothing. */
> + return;
> +
> + start += offset;
> + len -= offset;
> +
> + if (len > maxlen)
> + len = maxlen;
This part is quite a duplication of the above function, no?
> + if (flags & IORESOURCE_IO)
> + release_region(start, len);
> + else if (flags & IORESOURCE_MEM)
> + release_mem_region(start, len);
> +}
...
> +static int __pcim_request_region(struct pci_dev *pdev, int bar,
> + const char *name, int exclusive)
> +{
> + const unsigned long offset = 0;
> + const unsigned long len = pci_resource_len(pdev, bar);
How const anyhow useful here?
Ditto for other places like this.
> + return __pcim_request_region_range(pdev, bar, offset, len, name,
> exclusive);
> +}
...
> +static int pcim_addr_resources_match(struct device *dev, void *a_raw, void
> *b_raw)
> +{
> + struct pcim_addr_devres *a, *b;
> +
> + a = a_raw;
> + b = b_raw;
> + (void)dev; /* unused. */
Why do we need this?
> + if (a->type != b->type)
> + return 0;
> +
> + switch (a->type) {
> + case PCIM_ADDR_DEVRES_TYPE_REGION:
> + case PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING:
> + return a->bar == b->bar;
> + case PCIM_ADDR_DEVRES_TYPE_MAPPING:
> + return a->baseaddr == b->baseaddr;
> + case PCIM_ADDR_DEVRES_TYPE_REGION_RANGE_MAPPING:
> + return a->bar == b->bar &&
> + a->offset == b->offset && a->len == b->len;
Indentation or made it a single line.
> + default:
> + break;
> + }
> +
> + return 0;
return directly from default case.
> +}
...
> +/**
> + * pcim_iomap_region - Request and iomap a PCI BAR
> + * @pdev: PCI device to map IO resources for
> + * @bar: Index of a BAR to map
> + * @name: Name associated with the request
> + *
> + * Returns __iomem pointer on success, an IOMEM_ERR_PTR on failure.
Please, make sure the kernel-doc won't complain
scripts/kernel-doc -v -none -Wall ...
> + * Mapping and region will get automatically released on driver detach. If
> + * desired, release manually only with pcim_iounmap_region().
> + */
> +void __iomem *pcim_iomap_region(struct pci_dev *pdev, int bar, const char
> *name)
> +{
> + int ret = 0;
Redundant assignment.
> + struct pcim_addr_devres *res;
Perhaps reversed xmas tree order?
> + res = pcim_addr_devres_alloc(pdev);
> + if (!res)
> + return IOMEM_ERR_PTR(-ENOMEM);
> +
> + res->type = PCIM_ADDR_DEVRES_TYPE_REGION_MAPPING;
> + res->bar = bar;
> +
> + ret = __pcim_request_region(pdev, bar, name, 0);
> + if (ret != 0)
> + goto err_region;
> +
> + res->baseaddr = pci_iomap(pdev, bar, 0);
> + if (!res->baseaddr) {
> + ret = -EINVAL;
> + goto err_iomap;
> + }
> +
> + devres_add(>dev, res);
> + return res->baseaddr;
> +
> +err_iomap:
> + __pcim_release_region(pdev, bar);
> +err_region:
> + pcim_addr_devres_free(res);
> +
> + return IOMEM_ERR_PTR(ret);
> +}
...
> +static int _pcim_request_region(struct pci_dev *pdev, int bar, const char
> *name,
> + int request_flags)
Indentation?
> +{
> + int ret = 0;
Unneded assignment. Also fix this in other places.
> + struct pcim_addr_devres *res;
> +
> + res = pcim_addr_devres_alloc(pdev);
> + if (!res)
> + return -ENOMEM;
> + res->type = PCIM_ADDR_DEVRES_TYPE_REGION;
> + res->bar = bar;
> +
> + ret = __pcim_request_region(pdev, bar, name, request_flags);
> + if (ret != 0) {
if (ret)
Also fix this in other places.
> + pcim_addr_devres_free(res);
> + return ret;
> + }
> +
> + devres_add(>dev, res);
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 04/10] pci: devres: make devres region requests consistent
Mon, Jan 15, 2024 at 03:46:15PM +0100, Philipp Stanner kirjoitti:
> Now that pure managed region request functions are available, the
> implementation of the hybrid-functions which are only sometimes managed
> can be made more consistent and readable by wrapping those
> always-managed functions.
>
> Implement a new pcim_ function for exclusively requested regions.
> Have the pci_request / release functions call their pcim_ counterparts.
> Remove the now surplus region_mask from the devres struct.
...
> + if (pci_is_managed(pdev)) {
> + if (exclusive == IORESOURCE_EXCLUSIVE)
> + return pcim_request_region_exclusive(pdev, bar,
> res_name);
> + else
Redundant 'else'
> + return pcim_request_region(pdev, bar, res_name);
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH 02/10] pci: deprecate iomap-table functions
Mon, Jan 15, 2024 at 03:46:13PM +0100, Philipp Stanner kirjoitti:
> The old plural devres functions tie PCI's devres implementation to the
> iomap-table mechanism which unfortunately is not extensible.
>
> As the purlal functions are almost never used with more than one bit set
> in their bit-mask, deprecating them and encouraging users to use the new
> singular functions instead is reasonable.
>
> Furthermore, to make the implementation more consistent and extensible,
> the plural functions should use the singular functions.
>
> Add new wrapper to request / release all BARs.
> Make the plural functions call into the singular functions.
> Mark the plural functions as deprecated.
> Remove as much of the iomap-table-mechanism as possible.
> Add comments describing the path towards a cleaned-up API.
...
> static void pcim_iomap_release(struct device *gendev, void *res)
> {
> - struct pci_dev *dev = to_pci_dev(gendev);
> - struct pcim_iomap_devres *this = res;
> - int i;
> -
> - for (i = 0; i < PCIM_IOMAP_MAX; i++)
> - if (this->table[i])
> - pci_iounmap(dev, this->table[i]);
> + /*
> + * Do nothing. This is legacy code.
> + *
> + * Cleanup of the mappings is now done directly through the callbacks
> + * registered when creating them.
> + */
> }
How many users we have? Can't we simply kill it for good?
...
> + * This function is DEPRECATED. Do not use it in new code.
We have __deprecated IIRC, can it be used?
...
> + if (pcim_add_mapping_to_legacy_table(pdev, mapping, bar) != 0)
Redundant ' != 0' part.
> + goto err_table;
...
> +static inline bool mask_contains_bar(int mask, int bar)
> +{
> + return mask & (1 << bar);
BIT() ?
> +}
But I believe this function is not needed (see below).
...
> /**
> - * pcim_iomap_regions - Request and iomap PCI BARs
> + * pcim_iomap_regions - Request and iomap PCI BARs (DEPRECATED)
> * @pdev: PCI device to map IO resources for
> * @mask: Mask of BARs to request and iomap
> * @name: Name associated with the requests
> *
> + * Returns 0 on success, negative error code on failure.
Validate the kernel-doc.
> * Request and iomap regions specified by @mask.
> + *
> + * This function is DEPRECATED. Don't use it in new code.
> + * Use pcim_iomap_region() instead.
> */
...
> + for (bar = 0; bar < DEVICE_COUNT_RESOURCE; bar++) {
> + if (!mask_contains_bar(mask, bar))
> + continue;
NIH for_each_set_bit().
...
> + ret = pcim_add_mapping_to_legacy_table(pdev, mapping, bar);
> + if (ret != 0)
if (ret)
> + goto err;
> + }
...
> + err:
Better to name it like
err_unmap_and_remove:
> + while (--bar >= 0) {
while (bar--)
is easier to read.
> + pcim_iounmap_region(pdev, bar);
> + pcim_remove_bar_from_legacy_table(pdev, bar);
> + }
...
> +/**
> + * pcim_request_all_regions - Request all regions
> + * @pdev: PCI device to map IO resources for
> + * @name: name associated with the request
> + *
> + * Requested regions will automatically be released at driver detach. If
> desired,
> + * release individual regions with pcim_release_region() or all of them at
> once
> + * with pcim_release_all_regions().
> + */
Validate kernel-doc.
...
> + ret = pcim_request_region(pdev, bar, name);
> + if (ret != 0)
if (ret)
> + goto err;
...
> + short bar;
Why signed?
> + int ret = -1;
Oy vei!
...
> + ret = pcim_request_all_regions(pdev, name);
> + if (ret != 0)
Here and in plenty other places
if (ret)
> + return ret;
> + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> + if (!mask_contains_bar(mask, bar))
> + continue;
NIH for_each_set_bit()
> + if (!pcim_iomap(pdev, bar, 0))
> + goto err;
> + }
...
> + if (!legacy_iomap_table)
What's wrong with positive check?
> + ret = -ENOMEM;
> + else
> + ret = -EINVAL;
Can be even one liner
What's wrong with positive check?
ret = legacy_iomap_table ? -EINVAL : -ENOMEM;
...
> + while (--bar >= 0)
while (bar--)
> + pcim_iounmap(pdev, legacy_iomap_table[bar]);
...
> + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> + if (!mask_contains_bar(mask, bar))
> continue;
NIH for_each_set_bit()
> - pcim_iounmap(pdev, iomap[i]);
> - pci_release_region(pdev, i);
> + pcim_iounmap_region(pdev, bar);
> + pcim_remove_bar_from_legacy_table(pdev, bar);
> }
--
With Best Regards,
Andy Shevchenko
