Re: [PATCH v7 2/2] pwm: Add clock based PWM output driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jul 11, 2022 at 10:55:09AM +0500, Nikita Travkin wrote:
> Uwe Kleine-König писал(а) 01.07.2022 12:50:
> > On Sun, Jun 12, 2022 at 06:22:03PM +0500, Nikita Travkin wrote:
> >> Some systems have clocks exposed to external devices. If the clock
> >> controller supports duty-cycle configuration, such clocks can be used as
> >> pwm outputs. In fact PWM and CLK subsystems are interfaced with in a
> >> similar way and an "opposite" driver already exists (clk-pwm). Add a
> >> driver that would enable pwm devices to be used via clk subsystem.
> >>
> >> Signed-off-by: Nikita Travkin <nikita@xxxxxxx>
> >> --
> >>
> >> Changes in v2:
> >>  - Address Uwe's review comments:
> >>    - Round set clk rate up
> >>    - Add a description with limitations of the driver
> >>    - Disable and unprepare clock before removing pwmchip
> >> Changes in v3:
> >>  - Use 64bit version of div round up
> >>  - Address Uwe's review comments:
> >>    - Reword the limitations to avoid incorrect claims
> >>    - Move the clk_enabled flag assignment
> >>    - Drop unnecessary statements
> >> Changes in v5:
> >>  - add missed returns
> >> Changes in v6:
> >>  - Unprepare the clock on error
> >>  - Drop redundant limitations points
> >> Changes in v7:
> >>  - Rename some variables to be in line with common naming
> >>
> >> --
> >> It seems like my mailserver wasn't able to send the last review
> >> response to Uwe's so I'll repeat here that afaict clk.h has all the
> >> methods stubbed out so compiling without HAVE_CLK is possible.
> >> Sorry for a long delay with sending this since v6.

FTR: The only problems I have with mail sending in this thread is to
"devicetree@xxxxxxxxxxxxxx", I added a 'g' for this mail to that address
:-) Otherwise if you diagnose to have problems with the pengutronix
server accepting your mail, I'd like to hear about that.

> >> +	pcchip = devm_kzalloc(&pdev->dev, sizeof(*pcchip), GFP_KERNEL);
> >> +	if (!pcchip)
> >> +		return -ENOMEM;
> >> +
> >> +	pcchip->clk = devm_clk_get(&pdev->dev, NULL);
> > 
> > You can use devm_clk_get_prepared() here and drop the clk_prepare()
> > below and the clk_unprepare in .remove().
> 
> Here I spent a bit of time trying to remember why I thought
> I've already looked at this, but after figuring out that this
> devm helper didn't even exist earlier (I only see it in clk-next)
> I remembered considering a totally different thing (being
> clk_disable_unprepare in the _remove, which doesn't play well)
> 
> Given that this seems to be absent from 5.19-rc6, I'm afraid adding
> it here will upset the 0day as well as possibly cause issues in case
> both are taken for the same merge window...

Pass --base with a sensible parameter to git-format-patch (or
git-send-email) to make the 0day bots happy.

> On the other hand it takes me quite a while to provide replies for
> this series (the trend I'm not happy with) so maybe 3-4 weeks
> will indeed pass for 5.20-rc1 to have it...

It's not me who merges PWM patches but Thierry. I don't know his plans
and if he would be willing to pick up a new driver for the next cycle.
You might still get lucky with a fast next iteration.

If you want ignore the devm_clk_get_prepared() suggestion, we can still
convert to that once both patches hit Linus Torvald's tree.

> I think I will try to send a new version with just the comment
> added shortly in case it's still not too late for the next merge
> window and you can feel free to nack it if you think it already is :)

I think the driver looks good otherwise, so I don't expect to have more
feedback in the next round.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Attachment: signature.asc
Description: PGP signature


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux