| Message ID | 20190512082614.9045-1-tiny.windzz@gmail.com |
|---|---|
| Headers | show |
| Series | add thermal driver for h6 | expand |
Hello Yangtao, On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote: > HI, > > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <megous@megous.com> wrote: > > > > Hi Yangtao, > > > > thank you for work on this driver. > > > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote: > > > HI Ondřej, > > > > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > + > > > > > +/* Temp Unit: millidegree Celsius */ > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev, > > > > > + int reg) > > > > > > > > Please name all functions so that they are more clearly identifiable > > > > in stack traces as belonging to this driver. For example: > > > > > > > > sun8i_ths_reg2temp > > > > > > > > The same applies for all tsens_* functions below. tsens_* is too > > > > generic. > > > > > > Done but no sun8i_ths_reg2temp. > > > > > > ths_reg2tem() should be a generic func. > > > I think it should be suitable for all platforms， so no platform prefix. > > > > You've missed my point. The driver name is sun8i_thermal and if you get > > and oops from the kernel you'll get a stack trace where there are just function > > names. If you use too generic function names, it will not be clear which > > driver is oopsing. > > > > - sun8i_ths_reg2temp will tell you much more clearly where to search than > > - ths_reg2temp > > > > Of course you can always grep, but most thermal drivers are thermal sensor (ths) > > drivers, and if multiple of them used this too-generic naming scheme you'd > > have hard time debugging. > > > > Look at other thermal drivers. They usually encode driver name in the function > > names to help with identification (even if these are static driver-local > > functions). > > > > Can we change to sunxi_ths_ prefix? It should probably match the driver name, but yes, that's better. > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > +{ > > > > > + struct tsens_device *tmdev; > > > > > + struct device *dev = &pdev->dev; > > > > > + int ret; > > > > > + > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > + if (!tmdev) > > > > > + return -ENOMEM; > > > > > + > > > > > + tmdev->dev = dev; > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > + if (!tmdev->chip) > > > > > + return -EINVAL; > > > > > + > > > > > + ret = tsens_init(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + ret = tsens_register(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Why split this out of probe into separate functions? > > > > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > + > > > > > + return ret; > > > > > +} > > > > > + > > > > > +static int tsens_remove(struct platform_device *pdev) > > > > > +{ > > > > > + struct tsens_device *tmdev = platform_get_drvdata(pdev); > > > > > + > > > > > + tmdev->chip->disable(tmdev); > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev) > > > > > +{ > > > > > + int ret, val; > > > > > + > > > > > + ret = reset_control_deassert(tmdev->reset); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + ret = clk_prepare_enable(tmdev->bus_clk); > > > > > + if (ret) > > > > > + goto assert_reset; > > > > > + > > > > > + ret = tsens_calibrate(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset > > > > deasserted, and clock enabled. > > > > > > > > Overall, I think, reset/clock management and nvmem reading will be common > > > > to all the HW variants, so it doesn't make much sense splitting it out > > > > of probe into separate functions, and makes it more error prone. > > > > > > Our long-term goal is to support all platforms. > > > Bacicallt there is a differencr between each generation. > > > So I feel it necessary to isolate these differences. > > > > > > Maybe: > > > At some point, we can draw a part of the public part and platform > > > difference into different > > > files. something like qcom thermal driver. > > > > I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like > > all of them would share these 3 calls. > > > > You'll be enabling clock/reset and callibrating everywhere. So putting this to > > per-SoC function seems premature. > > In fact, enalbe and disable are the suspend and resume functions.(PM > callback will be added in the future) > When exiting from s2ram, the register will become the initial value. > We need to do all the work, enabling reset/clk ,calibrating and > initializing other reg. > > So I think it is no need to put enabling reset/clk and calibrating to > probe func, and I'd like > to keep enable and disable func. I know, I don't think it needs to be per-soc. These actions are all shared by all SoCs. Maybe with an exception that some SoCs may need one more clock, but that can be made optionally-required by some flag in struct sunxi_thermal_chip. Only highly SoC specific thing is configuring the THS registers for sampling frequency/averaging/enabling interrupts. The reset/clock enable is generic, and already abstracted by the clock/reset framework. So what I suggest is having: sunxi_ths_enable() reset deassert bus clock prepare enable optionally module clock prepare enable (in the future) call per-soc calibration call per-soc setup callback sunxi_ths_disable() reset assert bus clock unprepare disable optionally module clock unprepare disable And if you could move devm_nvmem_cell_get to probe that should make per-SoC calibration callback also less repetitive and could avoid undoing the enable in case it returns EPROBE_DEFER (which is possible). All this should make it easier to support PM in the future and add less cumbersome to add support for A83T and H3/H5. BTW, what are your plans for more SoC support? I'd like to add support for A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land early enough. If you don't have any plans I'll take it on. thank you and regards, o. > > > > thank you and regards, > > o. > > > > > Regards, > > > Yangtao > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, May 19, 2019 at 04:22:39PM +0200, Ondřej Jirman wrote: > On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote: > > HI, > > > > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > > Hi Yangtao, > > > > > > thank you for work on this driver. > > > > > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote: > > > > HI Ondřej, > > > > > > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > > + > > > > > > +/* Temp Unit: millidegree Celsius */ > > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev, > > > > > > + int reg) > > > > > > > > > > Please name all functions so that they are more clearly identifiable > > > > > in stack traces as belonging to this driver. For example: > > > > > > > > > > sun8i_ths_reg2temp > > > > > > > > > > The same applies for all tsens_* functions below. tsens_* is too > > > > > generic. > > > > > > > > Done but no sun8i_ths_reg2temp. > > > > > > > > ths_reg2tem() should be a generic func. > > > > I think it should be suitable for all platforms， so no platform prefix. > > > > > > You've missed my point. The driver name is sun8i_thermal and if you get > > > and oops from the kernel you'll get a stack trace where there are just function > > > names. If you use too generic function names, it will not be clear which > > > driver is oopsing. > > > > > > - sun8i_ths_reg2temp will tell you much more clearly where to search than > > > - ths_reg2temp > > > > > > Of course you can always grep, but most thermal drivers are thermal sensor (ths) > > > drivers, and if multiple of them used this too-generic naming scheme you'd > > > have hard time debugging. > > > > > > Look at other thermal drivers. They usually encode driver name in the function > > > names to help with identification (even if these are static driver-local > > > functions). > > > > > > > Can we change to sunxi_ths_ prefix? > > It should probably match the driver name, but yes, that's better. Not really. This driver will not support all the Allwinner devices, so sunxi is seriously misleading. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, On Sat, May 18, 2019 at 01:19:54AM +0800, Frank Lee wrote: > On Fri, May 17, 2019 at 3:32 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Fri, May 17, 2019 at 02:10:47AM +0800, Frank Lee wrote: > > > > On Sun, May 12, 2019 at 11:41:28PM +0200, Ondřej Jirman wrote: > > > > > > > +static int tsens_get_temp(void *data, int *temp) > > > > > > > +{ > > > > > > > + struct tsensor *s = data; > > > > > > > + struct tsens_device *tmdev = s->tmdev; > > > > > > > + int val; > > > > > > > + > > > > > > > + regmap_read(tmdev->regmap, tmdev->chip->temp_data_base + > > > > > > > + 0x4 * s->id, &val); > > > > > > > + > > > > > > > + if (unlikely(val == 0)) > > > > > > > + return -EBUSY; > > > > > > > > > > > > I'm not sure why a val equals to 0 would be associated with EBUSY? > > > > > > > > > > Thermal zone driver can (will) call get_temp before we got the > > > > > first interrupt and the thermal data. In that case val will be 0. > > > > > > > > > > Resulting in: > > > > > > > > > > (val + offset) * scale = (-2794) * -67 = 187198 > > > > > > > > > > 187°C and immediate shutdown during boot - based on cirtical > > > > > temperature being reached. > > > > > > > > > > Busy here means, get_temp does not yet have data. Thermal zone > > > > > driver just reports any error to dmesg output. > > > > > > > > Ah, that makes sense. > > > > > > > > I guess if we're switching to an interrupt-based driver, then we can > > > > just use a waitqueue, or is get_temp supposed to be atomic? > > > > > > I think get_temp should not be bloacked. > > > > Why not? > > Maybe, I am wrong. I also want to know if we should do this. I guess it really all depends on whether you can sleep or not in get_temps. If you can, then you should wait for the value to be converted and the THS raising an interrupt. If you can't, then we should ask what the thermal frameworks expects in such a case. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Fri, May 17, 2019 at 12:21:57PM -0700, Vasily Khoruzhick wrote: > On Sun, May 12, 2019 at 6:39 AM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > +static int tsens_get_temp(void *data, int *temp) > > > +{ > > > + struct tsensor *s = data; > > > + struct tsens_device *tmdev = s->tmdev; > > > + int val; > > > + > > > + regmap_read(tmdev->regmap, tmdev->chip->temp_data_base + > > > + 0x4 * s->id, &val); > > > + > > > + if (unlikely(val == 0)) > > > + return -EBUSY; > > > > I'm not sure why a val equals to 0 would be associated with EBUSY? > > First few reads of temp data return 0, and in case of H6 (and A64) it > means max temperature, so kernel does emergency shutdown. I used > -ETIMEDOUT as a workaround in my tree, but it's not appropriate here > either. Any suggestions are welcome. If we can use the interrupts and wait for a value to be converted before we read, then we should do that. > > Also, it's not in a fast path, so you can drop the unlikely. Chances > > are it's not that unlikely anyway. > > As I said earlier, it's just few samples after start up. That's not really my point though. unlikely is tricky to get right, because the compiler has his own meaning of what exactly unlikely means statistically to be able to do the right branching optimisations. However, this particular real case scenario might not have the same probability, which might result in a poor optimisation choice due to the unlikely being there. Moreover, this probability can evolve in the future. For example, let's assume that we enable dynamic PM in the driver. Starting from there, most of the reads become "first" reads, and your unlikely case becomes the likely one. My point was that, because of this, and because of the fact that it's really not a hot path, we should just drop it. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote: > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote: > > > > > +struct sun50i_thermal_chip { > > > > > + int sensor_num; > > > > > + int offset; > > > > > + int scale; > > > > > + int ft_deviation; > > > > > + int temp_calib_base; > > > > > + int temp_data_base; > > > > > + int (*enable)(struct tsens_device *tmdev); > > > > > + int (*disable)(struct tsens_device *tmdev); > > > > > +}; > > > > > > > > I'm not super fond of having a lot of quirks that are not needed. If > > > > we ever need those quirks when adding support for a new SoC, then > > > > yeah, we should totally have some, but only when and if it's needed. > > > > > > > > Otherwise, the driver is more complicated for no particular reason. > > > > > > This is unavoidable because of the difference in soc. > > > > I know, but this isn't my point. > > > > My point is that at this time of the driver development, we don't know > > what is going to be needed to support all of those SoCs. > > > > Some of the parameters you added might not be needed, some parameters > > might be missing, we don't know. So let's keep it simple for now. > > > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > +{ > > > > > + struct tsens_device *tmdev; > > > > > + struct device *dev = &pdev->dev; > > > > > + int ret; > > > > > + > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > + if (!tmdev) > > > > > + return -ENOMEM; > > > > > + > > > > > + tmdev->dev = dev; > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > + if (!tmdev->chip) > > > > > + return -EINVAL; > > > > > + > > > > > + ret = tsens_init(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + ret = tsens_register(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > > > Your registration should be the very last thing you do. Otherwise, you > > > > have a small window where the get_temp callback can be called, but the > > > > driver will not be functional yet. > > > > > > No. Anyway, ths data qcquisition is ms level. > > > > That's kind of irrelevant. There's nothing preventing get_temp to be > > called right away. > > As Ondřej said, > > Registration after enabling will lead to call tz update on non-registered tz > from an interrupt handler. I'm probably missing something but you're not using the interrupts, so how could an interrupt handler call it? Also, other drivers seem to be doing that just fine (mtk_thermal for example), so surely there's a way? > > > > > + ret = tsens_calibrate(tmdev); > > > > > + if (ret) > > > > > + return ret; > > > > > + > > > > > + /* > > > > > + * clkin = 24MHz > > > > > + * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1) > > > > > + * = 20us > > > > > + */ > > > > > + regmap_write(tmdev->regmap, SUN50I_THS_CTRL0, > > > > > + SUN50I_THS_CTRL0_T_ACQ(479)); > > > > > + /* average over 4 samples */ > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC, > > > > > + SUN50I_THS_FILTER_EN | > > > > > + SUN50I_THS_FILTER_TYPE(1)); > > > > > + /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */ > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_PC, > > > > > + SUN50I_H6_THS_PC_TEMP_PERIOD(58)); > > > > > + /* enable sensor */ > > > > > + val = GENMASK(tmdev->chip->sensor_num - 1, 0); > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val); > > > > > + > > > > > + return 0; > > > > > + > > > > > +assert_reset: > > > > > + reset_control_assert(tmdev->reset); > > > > > + > > > > > + return ret; > > > > > > > > Can't we do that with runtime_pm? > > > > > > Saving energy doesn't make much sense compared to system security. > > > > I'm not sure what you mean by security. > > Protect system hardware from damage. The point of runtime_pm is to keep the device on as long as it is used, so it wouldn't change anything there. I mean, you can even enable it in the probe if you want, my point is that the hooks that you have are exact equivalents to the one provided by runtime_pm already, so there's no need to define them in the first place. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Maxime, On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote: > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote: > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote: > > > > > > +struct sun50i_thermal_chip { > > > > > > + int sensor_num; > > > > > > + int offset; > > > > > > + int scale; > > > > > > + int ft_deviation; > > > > > > + int temp_calib_base; > > > > > > + int temp_data_base; > > > > > > + int (*enable)(struct tsens_device *tmdev); > > > > > > + int (*disable)(struct tsens_device *tmdev); > > > > > > +}; > > > > > > > > > > I'm not super fond of having a lot of quirks that are not needed. If > > > > > we ever need those quirks when adding support for a new SoC, then > > > > > yeah, we should totally have some, but only when and if it's needed. > > > > > > > > > > Otherwise, the driver is more complicated for no particular reason. > > > > > > > > This is unavoidable because of the difference in soc. > > > > > > I know, but this isn't my point. > > > > > > My point is that at this time of the driver development, we don't know > > > what is going to be needed to support all of those SoCs. > > > > > > Some of the parameters you added might not be needed, some parameters > > > might be missing, we don't know. So let's keep it simple for now. > > > > > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > > +{ > > > > > > + struct tsens_device *tmdev; > > > > > > + struct device *dev = &pdev->dev; > > > > > > + int ret; > > > > > > + > > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > > + if (!tmdev) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + tmdev->dev = dev; > > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > > + if (!tmdev->chip) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + ret = tsens_init(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + ret = tsens_register(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > > > > > Your registration should be the very last thing you do. Otherwise, you > > > > > have a small window where the get_temp callback can be called, but the > > > > > driver will not be functional yet. > > > > > > > > No. Anyway, ths data qcquisition is ms level. > > > > > > That's kind of irrelevant. There's nothing preventing get_temp to be > > > called right away. > > > > As Ondřej said, > > > > Registration after enabling will lead to call tz update on non-registered tz > > from an interrupt handler. > > I'm probably missing something but you're not using the interrupts, so > how could an interrupt handler call it? > > Also, other drivers seem to be doing that just fine (mtk_thermal for > example), so surely there's a way? Last version is using the interrupts. Drivers do it in various ways. For example imx_thermal (and others like hisi_thermal) does it the suggested way. It enables interrupts after thermal zone registration, so that IRQ handler doesn't get invoked before the tzd is registered. regards, o. > > > > > > + ret = tsens_calibrate(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + /* > > > > > > + * clkin = 24MHz > > > > > > + * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1) > > > > > > + * = 20us > > > > > > + */ > > > > > > + regmap_write(tmdev->regmap, SUN50I_THS_CTRL0, > > > > > > + SUN50I_THS_CTRL0_T_ACQ(479)); > > > > > > + /* average over 4 samples */ > > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC, > > > > > > + SUN50I_THS_FILTER_EN | > > > > > > + SUN50I_THS_FILTER_TYPE(1)); > > > > > > + /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */ > > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_PC, > > > > > > + SUN50I_H6_THS_PC_TEMP_PERIOD(58)); > > > > > > + /* enable sensor */ > > > > > > + val = GENMASK(tmdev->chip->sensor_num - 1, 0); > > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val); > > > > > > + > > > > > > + return 0; > > > > > > + > > > > > > +assert_reset: > > > > > > + reset_control_assert(tmdev->reset); > > > > > > + > > > > > > + return ret; > > > > > > > > > > Can't we do that with runtime_pm? > > > > > > > > Saving energy doesn't make much sense compared to system security. > > > > > > I'm not sure what you mean by security. > > > > Protect system hardware from damage. > > The point of runtime_pm is to keep the device on as long as it is > used, so it wouldn't change anything there. > > I mean, you can even enable it in the probe if you want, my point is > that the hooks that you have are exact equivalents to the one provided > by runtime_pm already, so there's no need to define them in the first > place. > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, May 21, 2019 at 12:27:21PM +0200, Ondřej Jirman wrote: > On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote: > > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote: > > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote: > > > > > > > +struct sun50i_thermal_chip { > > > > > > > + int sensor_num; > > > > > > > + int offset; > > > > > > > + int scale; > > > > > > > + int ft_deviation; > > > > > > > + int temp_calib_base; > > > > > > > + int temp_data_base; > > > > > > > + int (*enable)(struct tsens_device *tmdev); > > > > > > > + int (*disable)(struct tsens_device *tmdev); > > > > > > > +}; > > > > > > > > > > > > I'm not super fond of having a lot of quirks that are not needed. If > > > > > > we ever need those quirks when adding support for a new SoC, then > > > > > > yeah, we should totally have some, but only when and if it's needed. > > > > > > > > > > > > Otherwise, the driver is more complicated for no particular reason. > > > > > > > > > > This is unavoidable because of the difference in soc. > > > > > > > > I know, but this isn't my point. > > > > > > > > My point is that at this time of the driver development, we don't know > > > > what is going to be needed to support all of those SoCs. > > > > > > > > Some of the parameters you added might not be needed, some parameters > > > > might be missing, we don't know. So let's keep it simple for now. > > > > > > > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > > > +{ > > > > > > > + struct tsens_device *tmdev; > > > > > > > + struct device *dev = &pdev->dev; > > > > > > > + int ret; > > > > > > > + > > > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > > > + if (!tmdev) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + tmdev->dev = dev; > > > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > > > + if (!tmdev->chip) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > + ret = tsens_init(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + ret = tsens_register(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > > > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > > > > > > > Your registration should be the very last thing you do. Otherwise, you > > > > > > have a small window where the get_temp callback can be called, but the > > > > > > driver will not be functional yet. > > > > > > > > > > No. Anyway, ths data qcquisition is ms level. > > > > > > > > That's kind of irrelevant. There's nothing preventing get_temp to be > > > > called right away. > > > > > > As Ondřej said, > > > > > > Registration after enabling will lead to call tz update on non-registered tz > > > from an interrupt handler. > > > > I'm probably missing something but you're not using the interrupts, so > > how could an interrupt handler call it? > > > > Also, other drivers seem to be doing that just fine (mtk_thermal for > > example), so surely there's a way? > > Last version is using the interrupts. > > Drivers do it in various ways. For example imx_thermal (and others like > hisi_thermal) does it the suggested way. It enables interrupts after thermal > zone registration, so that IRQ handler doesn't get invoked before the tzd is > registered. Enabling the interrupts after the registration makes sense, yes, but filling the device private pointer with the private structure, enabling the clocks, setting up the controller and so on can be done before. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Tue, May 21, 2019 at 04:27:34PM +0200, Maxime Ripard wrote: > On Tue, May 21, 2019 at 12:27:21PM +0200, Ondřej Jirman wrote: > > On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote: > > > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote: > > > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard <maxime.ripard@bootlin.com> wrote: > > > > > > > > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote: > > > > > > > > +struct sun50i_thermal_chip { > > > > > > > > + int sensor_num; > > > > > > > > + int offset; > > > > > > > > + int scale; > > > > > > > > + int ft_deviation; > > > > > > > > + int temp_calib_base; > > > > > > > > + int temp_data_base; > > > > > > > > + int (*enable)(struct tsens_device *tmdev); > > > > > > > > + int (*disable)(struct tsens_device *tmdev); > > > > > > > > +}; > > > > > > > > > > > > > > I'm not super fond of having a lot of quirks that are not needed. If > > > > > > > we ever need those quirks when adding support for a new SoC, then > > > > > > > yeah, we should totally have some, but only when and if it's needed. > > > > > > > > > > > > > > Otherwise, the driver is more complicated for no particular reason. > > > > > > > > > > > > This is unavoidable because of the difference in soc. > > > > > > > > > > I know, but this isn't my point. > > > > > > > > > > My point is that at this time of the driver development, we don't know > > > > > what is going to be needed to support all of those SoCs. > > > > > > > > > > Some of the parameters you added might not be needed, some parameters > > > > > might be missing, we don't know. So let's keep it simple for now. > > > > > > > > > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > > > > +{ > > > > > > > > + struct tsens_device *tmdev; > > > > > > > > + struct device *dev = &pdev->dev; > > > > > > > > + int ret; > > > > > > > > + > > > > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > > > > + if (!tmdev) > > > > > > > > + return -ENOMEM; > > > > > > > > + > > > > > > > > + tmdev->dev = dev; > > > > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > > > > + if (!tmdev->chip) > > > > > > > > + return -EINVAL; > > > > > > > > + > > > > > > > > + ret = tsens_init(tmdev); > > > > > > > > + if (ret) > > > > > > > > + return ret; > > > > > > > > + > > > > > > > > + ret = tsens_register(tmdev); > > > > > > > > + if (ret) > > > > > > > > + return ret; > > > > > > > > + > > > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > > > > + if (ret) > > > > > > > > + return ret; > > > > > > > > > > > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > > > > > > > > > Your registration should be the very last thing you do. Otherwise, you > > > > > > > have a small window where the get_temp callback can be called, but the > > > > > > > driver will not be functional yet. > > > > > > > > > > > > No. Anyway, ths data qcquisition is ms level. > > > > > > > > > > That's kind of irrelevant. There's nothing preventing get_temp to be > > > > > called right away. > > > > > > > > As Ondřej said, > > > > > > > > Registration after enabling will lead to call tz update on non-registered tz > > > > from an interrupt handler. > > > > > > I'm probably missing something but you're not using the interrupts, so > > > how could an interrupt handler call it? > > > > > > Also, other drivers seem to be doing that just fine (mtk_thermal for > > > example), so surely there's a way? > > > > Last version is using the interrupts. > > > > Drivers do it in various ways. For example imx_thermal (and others like > > hisi_thermal) does it the suggested way. It enables interrupts after thermal > > zone registration, so that IRQ handler doesn't get invoked before the tzd is > > registered. > > Enabling the interrupts after the registration makes sense, yes, but > filling the device private pointer with the private structure, > enabling the clocks, setting up the controller and so on can be done > before. I agree. o. > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
HI Ondřej, On Sun, May 19, 2019 at 10:22 PM Ondřej Jirman <megous@megous.com> wrote: > > Hello Yangtao, > > On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote: > > HI, > > > > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > > Hi Yangtao, > > > > > > thank you for work on this driver. > > > > > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote: > > > > HI Ondřej, > > > > > > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > > + > > > > > > +/* Temp Unit: millidegree Celsius */ > > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev, > > > > > > + int reg) > > > > > > > > > > Please name all functions so that they are more clearly identifiable > > > > > in stack traces as belonging to this driver. For example: > > > > > > > > > > sun8i_ths_reg2temp > > > > > > > > > > The same applies for all tsens_* functions below. tsens_* is too > > > > > generic. > > > > > > > > Done but no sun8i_ths_reg2temp. > > > > > > > > ths_reg2tem() should be a generic func. > > > > I think it should be suitable for all platforms， so no platform prefix. > > > > > > You've missed my point. The driver name is sun8i_thermal and if you get > > > and oops from the kernel you'll get a stack trace where there are just function > > > names. If you use too generic function names, it will not be clear which > > > driver is oopsing. > > > > > > - sun8i_ths_reg2temp will tell you much more clearly where to search than > > > - ths_reg2temp > > > > > > Of course you can always grep, but most thermal drivers are thermal sensor (ths) > > > drivers, and if multiple of them used this too-generic naming scheme you'd > > > have hard time debugging. > > > > > > Look at other thermal drivers. They usually encode driver name in the function > > > names to help with identification (even if these are static driver-local > > > functions). > > > > > > > Can we change to sunxi_ths_ prefix? > > It should probably match the driver name, but yes, that's better. > > > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > > +{ > > > > > > + struct tsens_device *tmdev; > > > > > > + struct device *dev = &pdev->dev; > > > > > > + int ret; > > > > > > + > > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > > + if (!tmdev) > > > > > > + return -ENOMEM; > > > > > > + > > > > > > + tmdev->dev = dev; > > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > > + if (!tmdev->chip) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + ret = tsens_init(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + ret = tsens_register(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > > > > > Why split this out of probe into separate functions? > > > > > > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > + > > > > > > + return ret; > > > > > > +} > > > > > > + > > > > > > +static int tsens_remove(struct platform_device *pdev) > > > > > > +{ > > > > > > + struct tsens_device *tmdev = platform_get_drvdata(pdev); > > > > > > + > > > > > > + tmdev->chip->disable(tmdev); > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > + > > > > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev) > > > > > > +{ > > > > > > + int ret, val; > > > > > > + > > > > > > + ret = reset_control_deassert(tmdev->reset); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + ret = clk_prepare_enable(tmdev->bus_clk); > > > > > > + if (ret) > > > > > > + goto assert_reset; > > > > > > + > > > > > > + ret = tsens_calibrate(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > > > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset > > > > > deasserted, and clock enabled. > > > > > > > > > > Overall, I think, reset/clock management and nvmem reading will be common > > > > > to all the HW variants, so it doesn't make much sense splitting it out > > > > > of probe into separate functions, and makes it more error prone. > > > > > > > > Our long-term goal is to support all platforms. > > > > Bacicallt there is a differencr between each generation. > > > > So I feel it necessary to isolate these differences. > > > > > > > > Maybe: > > > > At some point, we can draw a part of the public part and platform > > > > difference into different > > > > files. something like qcom thermal driver. > > > > > > I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like > > > all of them would share these 3 calls. > > > > > > You'll be enabling clock/reset and callibrating everywhere. So putting this to > > > per-SoC function seems premature. > > > > In fact, enalbe and disable are the suspend and resume functions.(PM > > callback will be added in the future) > > When exiting from s2ram, the register will become the initial value. > > We need to do all the work, enabling reset/clk ,calibrating and > > initializing other reg. > > > > So I think it is no need to put enabling reset/clk and calibrating to > > probe func, and I'd like > > to keep enable and disable func. > > I know, I don't think it needs to be per-soc. These actions are all shared by > all SoCs. Maybe with an exception that some SoCs may need one more clock, but > that can be made optionally-required by some flag in struct sunxi_thermal_chip. > > Only highly SoC specific thing is configuring the THS registers for sampling > frequency/averaging/enabling interrupts. The reset/clock enable is generic, and > already abstracted by the clock/reset framework. > > So what I suggest is having: > > sunxi_ths_enable() > reset deassert > bus clock prepare enable > optionally module clock prepare enable (in the future) > call per-soc calibration > call per-soc setup callback > > sunxi_ths_disable() > reset assert > bus clock unprepare disable > optionally module clock unprepare disable > > And if you could move devm_nvmem_cell_get to probe that should make per-SoC > calibration callback also less repetitive and could avoid undoing the enable > in case it returns EPROBE_DEFER (which is possible). > > All this should make it easier to support PM in the future and add less > cumbersome to add support for A83T and H3/H5. > > BTW, what are your plans for more SoC support? I'd like to add support for > A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land > early enough. If you don't have any plans I'll take it on. > I plan to support h3 and a33 later. Can you support other platforms? Cheers, Yangtao > thank you and regards, > o. > > > > > > > thank you and regards, > > > o. > > > > > > > Regards, > > > > Yangtao > > > > _______________________________________________ > > linux-arm-kernel mailing list > > linux-arm-kernel@lists.infradead.org > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
HI, Following the discussion above, I made some changes. I think it's time to consider V3 and see what else needs to be modified. Thx, Yangtao
On Sat, May 25, 2019 at 11:48 AM Frank Lee <tiny.windzz@gmail.com> wrote: > > HI Ondřej, > > On Sun, May 19, 2019 at 10:22 PM Ondřej Jirman <megous@megous.com> wrote: > > > > Hello Yangtao, > > > > On Sat, May 18, 2019 at 12:34:57AM +0800, Frank Lee wrote: > > > HI, > > > > > > On Fri, May 17, 2019 at 2:29 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > > > > Hi Yangtao, > > > > > > > > thank you for work on this driver. > > > > > > > > On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote: > > > > > HI Ondřej, > > > > > > > > > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman <megous@megous.com> wrote: > > > > > > > + > > > > > > > +/* Temp Unit: millidegree Celsius */ > > > > > > > +static int tsens_reg2temp(struct tsens_device *tmdev, > > > > > > > + int reg) > > > > > > > > > > > > Please name all functions so that they are more clearly identifiable > > > > > > in stack traces as belonging to this driver. For example: > > > > > > > > > > > > sun8i_ths_reg2temp > > > > > > > > > > > > The same applies for all tsens_* functions below. tsens_* is too > > > > > > generic. > > > > > > > > > > Done but no sun8i_ths_reg2temp. > > > > > > > > > > ths_reg2tem() should be a generic func. > > > > > I think it should be suitable for all platforms， so no platform prefix. > > > > > > > > You've missed my point. The driver name is sun8i_thermal and if you get > > > > and oops from the kernel you'll get a stack trace where there are just function > > > > names. If you use too generic function names, it will not be clear which > > > > driver is oopsing. > > > > > > > > - sun8i_ths_reg2temp will tell you much more clearly where to search than > > > > - ths_reg2temp > > > > > > > > Of course you can always grep, but most thermal drivers are thermal sensor (ths) > > > > drivers, and if multiple of them used this too-generic naming scheme you'd > > > > have hard time debugging. > > > > > > > > Look at other thermal drivers. They usually encode driver name in the function > > > > names to help with identification (even if these are static driver-local > > > > functions). > > > > > > > > > > Can we change to sunxi_ths_ prefix? > > > > It should probably match the driver name, but yes, that's better. > > > > > > > > > +static int tsens_probe(struct platform_device *pdev) > > > > > > > +{ > > > > > > > + struct tsens_device *tmdev; > > > > > > > + struct device *dev = &pdev->dev; > > > > > > > + int ret; > > > > > > > + > > > > > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > > > > > + if (!tmdev) > > > > > > > + return -ENOMEM; > > > > > > > + > > > > > > > + tmdev->dev = dev; > > > > > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > > > > > + if (!tmdev->chip) > > > > > > > + return -EINVAL; > > > > > > > + > > > > > > > + ret = tsens_init(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + ret = tsens_register(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > > > > > > Why split this out of probe into separate functions? > > > > > > > > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > > + > > > > > > > + return ret; > > > > > > > +} > > > > > > > + > > > > > > > +static int tsens_remove(struct platform_device *pdev) > > > > > > > +{ > > > > > > > + struct tsens_device *tmdev = platform_get_drvdata(pdev); > > > > > > > + > > > > > > > + tmdev->chip->disable(tmdev); > > > > > > > + > > > > > > > + return 0; > > > > > > > +} > > > > > > > + > > > > > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev) > > > > > > > +{ > > > > > > > + int ret, val; > > > > > > > + > > > > > > > + ret = reset_control_deassert(tmdev->reset); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > + > > > > > > > + ret = clk_prepare_enable(tmdev->bus_clk); > > > > > > > + if (ret) > > > > > > > + goto assert_reset; > > > > > > > + > > > > > > > + ret = tsens_calibrate(tmdev); > > > > > > > + if (ret) > > > > > > > + return ret; > > > > > > > > > > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset > > > > > > deasserted, and clock enabled. > > > > > > > > > > > > Overall, I think, reset/clock management and nvmem reading will be common > > > > > > to all the HW variants, so it doesn't make much sense splitting it out > > > > > > of probe into separate functions, and makes it more error prone. > > > > > > > > > > Our long-term goal is to support all platforms. > > > > > Bacicallt there is a differencr between each generation. > > > > > So I feel it necessary to isolate these differences. > > > > > > > > > > Maybe: > > > > > At some point, we can draw a part of the public part and platform > > > > > difference into different > > > > > files. something like qcom thermal driver. > > > > > > > > I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like > > > > all of them would share these 3 calls. > > > > > > > > You'll be enabling clock/reset and callibrating everywhere. So putting this to > > > > per-SoC function seems premature. > > > > > > In fact, enalbe and disable are the suspend and resume functions.(PM > > > callback will be added in the future) > > > When exiting from s2ram, the register will become the initial value. > > > We need to do all the work, enabling reset/clk ,calibrating and > > > initializing other reg. > > > > > > So I think it is no need to put enabling reset/clk and calibrating to > > > probe func, and I'd like > > > to keep enable and disable func. > > > > I know, I don't think it needs to be per-soc. These actions are all shared by > > all SoCs. Maybe with an exception that some SoCs may need one more clock, but > > that can be made optionally-required by some flag in struct sunxi_thermal_chip. > > > > Only highly SoC specific thing is configuring the THS registers for sampling > > frequency/averaging/enabling interrupts. The reset/clock enable is generic, and > > already abstracted by the clock/reset framework. > > > > So what I suggest is having: > > > > sunxi_ths_enable() > > reset deassert > > bus clock prepare enable > > optionally module clock prepare enable (in the future) > > call per-soc calibration > > call per-soc setup callback > > > > sunxi_ths_disable() > > reset assert > > bus clock unprepare disable > > optionally module clock unprepare disable > > > > And if you could move devm_nvmem_cell_get to probe that should make per-SoC > > calibration callback also less repetitive and could avoid undoing the enable > > in case it returns EPROBE_DEFER (which is possible). > > > > All this should make it easier to support PM in the future and add less > > cumbersome to add support for A83T and H3/H5. > > > > BTW, what are your plans for more SoC support? I'd like to add support for > > A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land > > early enough. If you don't have any plans I'll take it on. > > > > I plan to support h3 and a33 later. > Can you support other platforms? I'll work on A64 once this driver lands. > Cheers, > Yangtao > > > thank you and regards, > > o. > > > > > > > > > > thank you and regards, > > > > o. > > > > > > > > > Regards, > > > > > Yangtao > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello Yangtao, On Sun, May 26, 2019 at 02:48:13AM +0800, Frank Lee wrote: > On Sun, May 19, 2019 at 10:22 PM Ondřej Jirman <megous@megous.com> wrote: > > > > I know, I don't think it needs to be per-soc. These actions are all shared by > > all SoCs. Maybe with an exception that some SoCs may need one more clock, but > > that can be made optionally-required by some flag in struct sunxi_thermal_chip. > > > > Only highly SoC specific thing is configuring the THS registers for sampling > > frequency/averaging/enabling interrupts. The reset/clock enable is generic, and > > already abstracted by the clock/reset framework. > > > > So what I suggest is having: > > > > sunxi_ths_enable() > > reset deassert > > bus clock prepare enable > > optionally module clock prepare enable (in the future) > > call per-soc calibration > > call per-soc setup callback > > > > sunxi_ths_disable() > > reset assert > > bus clock unprepare disable > > optionally module clock unprepare disable > > > > And if you could move devm_nvmem_cell_get to probe that should make per-SoC > > calibration callback also less repetitive and could avoid undoing the enable > > in case it returns EPROBE_DEFER (which is possible). > > > > All this should make it easier to support PM in the future and add less > > cumbersome to add support for A83T and H3/H5. > > > > BTW, what are your plans for more SoC support? I'd like to add support for > > A83T and H3/H5, maybe even during the 5.3 cycle if this driver happens to land > > early enough. If you don't have any plans I'll take it on. > > > > I plan to support h3 and a33 later. > Can you support other platforms? Yes, I can do A83T. H5 is similar (the same?) as H3. thank you and regards, o. > Cheers, > Yangtao > > > thank you and regards, > > o. > > > > > > > > > > thank you and regards, > > > > o. > > > > > > > > > Regards, > > > > > Yangtao > > > > > > _______________________________________________ > > > linux-arm-kernel mailing list > > > linux-arm-kernel@lists.infradead.org > > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel