Dear Florian, Andrew, Thanks for your email! On Tue, Jan 3, 2017 at 2:19 PM, Florian Fainelli <f.fainelli@xxxxxxxxx> wrote: > > Interestingly, I submitted a patch doing nearly the same thing recently > after hacking on a Buffalo Terastation Pro II two days after yours > without seeing yours: > > https://lkml.org/lkml/2016/12/28/273 Glad to know there's other developer working on linkstation/kurobox platform! > Some comments below. > >> + >> +static void __iomem *base; >> +static unsigned long tclk; >> +static const struct reset_cfg *cfg; > > How about avoiding the singletons here and pass this down from the > platform driver's private data after we (see below) also make use of a > reboot notifier? I see your patches. Indeed, it's a good idea to avoid the singletons and use private data instead. >> +static int linkstation_reset_probe(struct platform_device *pdev) >> +{ >> + struct device_node *np = pdev->dev.of_node; >> + struct resource *res; >> + struct clk *clk; >> + char symname[KSYM_NAME_LEN]; >> + >> + const struct of_device_id *match = >> + of_match_node(linkstation_reset_of_match_table, np); >> + cfg = match->data; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + if (!res) { >> + dev_err(&pdev->dev, "Missing resource"); >> + return -EINVAL; >> + } >> + >> + base = devm_ioremap(&pdev->dev, res->start, resource_size(res)); >> + if (!base) { >> + dev_err(&pdev->dev, "Unable to map resource"); >> + return -EINVAL; >> + } >> + >> + /* We need to know tclk in order to calculate the UART divisor */ >> + clk = devm_clk_get(&pdev->dev, NULL); >> + if (IS_ERR(clk)) { >> + dev_err(&pdev->dev, "Clk missing"); >> + return PTR_ERR(clk); >> + } >> + >> + tclk = clk_get_rate(clk); > > Does this work with the Terastation II which has not been converted to > DT yet? Is tclk available through the CLK API there? I have no idea whether device-based legacy code and make use of power reset driver. Maybe Sebastian Reichel can offer a comment? However, I think Terastation II should convert to DT first. If you're willing to test, I can help to provide a dts/dtb. (If you use Debian, I can even provide DEB of kernel image and flash-kernel patch, which is easy for you to test) On Tue, Jan 3, 2017 at 10:09 PM, Andrew Lunn <andrew@xxxxxxx> wrote: >> > + >> > + /* Check that nothing else has already setup a handler */ >> > + if (pm_power_off) { >> > + lookup_symbol_name((ulong)pm_power_off, symname); >> > + dev_err(&pdev->dev, >> > + "pm_power_off already claimed %p %s", >> > + pm_power_off, symname); >> > + return -EBUSY; >> > + } >> > + pm_power_off = linkstation_reset; >> >> That seems a bit complicated, why not just assume that there will be >> either this driver used, or not at all? > > That is probably my fault. This is a copy from code i wrote many years > ago for the QNAP. I guess at the time i was battling with two > different pm_power_off handlers, so put in this code. > >> Also, you are supposed to register a reboot notifier to which you can >> pass private context: > > At the time i wrote the QNAP code, this did not exist. So maybe my > code is no longer a good example to copy. Really appreciated, Andrew! I'll modify this part in next series. BTW. the private data passing to reboot notifier can be shared to power-off function as well? Do you have example? > https://lkml.org/lkml/2016/12/28/275 > > As indicated in my patch series, the UART1-attached micro controller > does a lot more things that just providing reboot, which is why I chose > to move this code to a MFD driver, as the starting point before adding > support for LEDs, FAN, PWM, beeper which would be other types of devices. > > Is adding support for other peripherals on your TODO as well? Except reset feature (power-off and reboot), other feature, such as LEDs / FAN speed / buttons, is managed by user-land program micro-evtd [0][1]. Since the upstream [1] is not active anymore, Ryan Tandy (in CC) and I are maintaining it in Debian [0]. [0] https://tracker.debian.org/pkg/micro-evtd [1] https://sourceforge.net/projects/ppc-evtd micro-evtd worked well for device-based legacy code, but after DT conversion, Ryan found the device cannot shutdown properly (reboot is OK). That why I created this patch series. I think for such old hardware and mature user-land program, it doesn't deserve the effort to implement those again in kernel side. What do you think? Cheers, -- Roger Shimizu, GMT +9 Tokyo PGP/GPG: 4096R/6C6ACD6417B3ACB1 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
