Re: [PATCH V0 2/2] iio: adc: sprd_pmic_adc: Add support for UMP serise pmic adc

[杨明金]

> > > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > > +{
> > > > +     int ret = 0;
> > > > +     u32 reg_read = 0;
> > > > +
> > > > +     if (data->pm_data.clk_regmap) {
> > > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > > +                                      data->pm_data.clk_reg_mask,
> > > > +                                      data->pm_data.clk_reg_mask);
> > > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > > +             if (ret) {
> > > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > > +                     return ret;
> > > > +             }
> > > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
> > >
> > > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > > for this?
> >
> > This register is used to vote to enable/disable the pmic 26m clk which
> > is provided to modules like audio, typec and adc.
> > Therefore, this clk cannot be disabled or enabled directly.
>
> clk_enable() and friends support reference counted enable and disable
> so I don't understand why this needs something unusual.

Through communication with internal clk colleagues,
I learned that this register is not a traditional eb register,
so the current clk driver is not configured to support this.

Jonathan Cameron <jic23@xxxxxxxxxx> 于2023年9月3日周日 19:14写道：
>
> On Wed, 30 Aug 2023 15:15:12 +0800
> 杨明金 <magicyangmingjin@xxxxxxxxx> wrote:
>
> > Jonathan Cameron <jic23@xxxxxxxxxx> 于2023年8月28日周一 23:56写道：
> > >
> Hi,
>
> Please crop replies to relevant part only.  Hopefully I found it!
>
>
> > > > +static int sprd_adc_enable(struct sprd_adc_data *data, int channel)
> > > > +{
> > > > +     int ret = 0;
> > > > +     u32 reg_read = 0;
> > > > +
> > > > +     if (data->pm_data.clk_regmap) {
> > > > +             ret = regmap_update_bits(data->pm_data.clk_regmap, data->pm_data.clk_reg,
> > > > +                                      data->pm_data.clk_reg_mask,
> > > > +                                      data->pm_data.clk_reg_mask);
> > > > +             ret |= regmap_read(data->pm_data.clk_regmap, data->pm_data.clk_reg, &reg_read);
> > > > +             if (ret) {
> > > > +                     dev_err(data->dev, "failed to enable clk26m, channel %d\n", channel);
> > > > +                     return ret;
> > > > +             }
> > > > +             dev_dbg(data->dev, "enable clk26m: ch %d, reg_read 0x%x\n", channel, reg_read);
> > >
> > > Directly accessing the regmap of a clock seems unusual. Why not provide generic clock interfaces
> > > for this?
> >
> > This register is used to vote to enable/disable the pmic 26m clk which
> > is provided to modules like audio, typec and adc.
> > Therefore, this clk cannot be disabled or enabled directly.
>
> clk_enable() and friends support reference counted enable and disable
> so I don't understand why this needs something unusual.
>
>
> >
>
> > > > +static int sprd_adc_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct device_node *np = pdev->dev.of_node;
> > > > +     struct sprd_adc_data *sprd_data;
> > > > +     const struct sprd_adc_variant_data *pdata;
> > > > +     struct iio_dev *indio_dev;
> > > > +     int ret;
> > > > +
> > > > +     pdata = of_device_get_match_data(&pdev->dev);
> > >
> > > device_get_match_data()
> > >
> > >
> > > > +     if (!pdata) {
> > > > +             dev_err(&pdev->dev, "No matching driver data found\n");
> > > > +             return -EINVAL;
> > > > +     }
> > > > +
> > > > +     indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*sprd_data));
> > > > +     if (!indio_dev)
> > > > +             return -ENOMEM;
> > > > +
> > > > +     sprd_data = iio_priv(indio_dev);
> > > > +
> > > > +     sprd_data->regmap = dev_get_regmap(pdev->dev.parent, NULL);
> > > > +     if (!sprd_data->regmap) {
> > > > +             dev_err(&pdev->dev, "failed to get ADC regmap\n");
> > > > +             return -ENODEV;
> > > > +     }
> > > > +
> > > > +     ret = of_property_read_u32(np, "reg", &sprd_data->base);
> > >
> > > Even though some elements of this (of_hwspin...) don't have generic firmware
> > > interfaces, I would prefer to see those from linux/property.h used
> > > wherever possible.  It will take us a long time to make that a subsystem
> > > wide change, but good not to have more unnecessary instances of device tree
> > > specific property reading.
> >
> > Sorry, I don't understand what needs to be modified. Can you provide
> > more information or give an example?
> > Do you mean that the "reg"  property reading is unnecessary?
>
> No.  Where possibly use
>         device_property_read_u32(dev, "reg".. etc
> and similar functions from
> include/linux/property.h rather than device tree specific ones.
> The generic property handling deals with various different types of firmware
> without needing drivers to be aware of it.
>
> Some elements that you need here do not have generic property handling so
> for those you will need to continue using the of_ variants.
> Note that this is to support long term move of everything to the generic
> firmware framework.  Even if we drivers in IIO etc that are really device
> tree only there are benefits for maintenance in using one framework
> for all drivers. As some IIO drivers do support other firmware types
> (ACPI for example) the generic version is the preferred choice.
>
> Thanks,
>
> Jonathan